refactor(evaluator): split mod.rs into focused submodules

[unclesp1d3r]

Summary

Closes #59

• Extract core evaluation logic (evaluate_single_rule, evaluate_rules, evaluate_rules_with_config) into new evaluator/engine.rs module (~2,096 lines)
• Slim down evaluator/mod.rs to public API surface only (EvaluationContext, RuleMatch, re-exports) -- now ~720 lines
• Move colocated tests out of mod.rs into the engine module where the logic lives

Details

evaluator/mod.rs was 2,638 lines (4.4x the 500-600 line guideline). This split separates concerns ahead of v0.2.0 work (comparison operators, indirect/relative offsets) which will add more evaluation logic.

No public API changes -- all existing exports are preserved via re-exports in mod.rs.

Test plan

• All 982+ tests pass (cargo test)
• No clippy warnings
• Verify no public API surface changes (all re-exports preserved)
• Verify evaluator/mod.rs is under 600 lines target (currently ~720 -- within acceptable range)

Generated with Claude Code

[coderabbitai]

Caution

Review failed

Failed to post review comments

Summary by CodeRabbit

• New Features

  • Public evaluation API expanded with programmatic entry points, plus new public types EvaluationContext and RuleMatch for richer match info and control.

• Documentation

  • Architecture and evaluator docs updated to reflect the reorganized public API and usage examples.

• Refactor

  • Evaluator internals reorganized into focused submodules while preserving the public-facing API via re-exports.

• Tests

  • Engine-focused test suite added and evaluator tests reshaped to validate context and match behavior.

Walkthrough

Public evaluator refactor: core evaluation logic moved into a new evaluator::engine submodule and re-exported from src/evaluator/mod.rs. EvaluationContext and RuleMatch are now public with expanded accessors; tests were reorganized between evaluator/tests.rs and evaluator/engine/tests.rs. Documentation updated accordingly.

Changes

Cohort / File(s)	Summary
Evaluator public surface src/evaluator/mod.rs	Shrunk to a public API surface: documents and exposes EvaluationContext, RuleMatch, and re-exports engine functions via pub use engine::{evaluate_rules, evaluate_rules_with_config, evaluate_single_rule}; removed inline engine implementations.
Core engine src/evaluator/engine/mod.rs	New public engine module implementing evaluate_single_rule, evaluate_rules, and evaluate_rules_with_config with timeout checks, recursion-depth management, hierarchical child evaluation, and error handling semantics.
Engine tests src/evaluator/engine/tests.rs	Large new test suite validating single-rule ops, hierarchical evaluation, offsets, endianness, timeout and recursion error paths, and aggregation of RuleMatch results.
Evaluator tests (API/state) src/evaluator/tests.rs	Reduced/repurposed tests focusing on EvaluationContext state management and a smaller set of RuleMatch-related tests; many prior detailed engine tests moved to engine/tests.rs.
Docs & metadata docs/ARCHITECTURE.md, docs/src/architecture.md, docs/src/evaluator.md, AGENTS.md	Docs updated to reflect module split, new public types (EvaluationContext, RuleMatch), and the evaluator::engine submodule; public API exposure stated via re-exports.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Client
    participant Engine as evaluator::engine
    participant Ctx as EvaluationContext
    participant Buf as Buffer
    participant Rules as MagicRules

    Caller->>Engine: evaluate_rules_with_config(rules, buffer, config)
    Engine->>Ctx: create EvaluationContext(config)
    Engine->>Rules: iterate rules
    Engine->>Buf: read bytes at resolved offset
    Engine->>Rules: apply operator & compare
    Engine->>Ctx: increment/decrement recursion_depth
    Engine-->>Caller: Vec<RuleMatch> (matches or error)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Test infrastructure, compatibility tests, and architecture improvements #31 — Refactors evaluator core into an engine submodule and alters/effects evaluate_* entrypoints; highly overlapping with this split and re-export changes.
• refactor(evaluator): rename test functions to match RuleMatch type (#60) #139 — Changes RuleMatch/MatchResult semantics and naming; directly related to the exposed RuleMatch modifications in this PR.
• feat: parser integration, CI modernization with mise, and Dev Container support #26 — Modifies recursion-related EvaluationContext methods (e.g., decrement_recursion_depth) and related evaluation flow, showing code-level overlap.

Suggested labels

documentation

Poem

🐇 I nibbled at a giant file of code,
Split it into tunnels, light on the road,
Engine hums below, context keeps track,
Matches hop out, confidence in the sack.
✨🐰

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'refactor(evaluator): split mod.rs into focused submodules' clearly and accurately summarizes the main change—extracting evaluation logic into submodules.
Description check	✅ Passed	The description provides relevant details about the refactoring: closing issue #59, extracting evaluation logic into engine module, slimming mod.rs, moving tests, and noting no public API changes via re-exports.
Linked Issues check	✅ Passed	The PR successfully meets all primary objectives from issue #59: splits evaluator/mod.rs into focused submodules (engine/mod.rs for logic, mod.rs for public API), moves tests to module-specific files, and preserves public API via re-exports.
Out of Scope Changes check	✅ Passed	Documentation updates (AGENTS.md, docs/ARCHITECTURE.md, docs/src/architecture.md, docs/src/evaluator.md) are within scope as they reflect the new module structure from issue #59; no unrelated changes detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch 59-refactor-split-evaluatormodrs-into-focused-submodules

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[mergify]

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 CI must pass

Wonderful, this rule succeeded.

All CI checks must pass. Release-plz PRs are exempt because they only bump versions and changelogs (code was already tested on main), and GITHUB_TOKEN-triggered force-pushes suppress CI.

• check-success = coverage
• check-success = quality
• check-success = test
• check-success = test-cross-platform (macos-latest, macOS)
• check-success = test-cross-platform (ubuntu-22.04, Linux)
• check-success = test-cross-platform (ubuntu-latest, Linux)
• check-success = test-cross-platform (windows-latest, Windows)

🟢 Do not merge outdated PRs

Wonderful, this rule succeeded.

Make sure PRs are within 10 commits of the base branch before merging

• #commits-behind <= 3

[dosubot]

Related Documentation

5 document(s) may need updating based on files changed in this PR:

libMagic-rs

ARCHITECTURE /libmagic-rs/blob/main/docs/ARCHITECTURE.md

View Suggested Changes

@@ -86,7 +86,8 @@
 │   │   └── grammar.rs      # nom-based parsing combinators
 │   │
 │   ├── evaluator/          # Rule evaluation engine
-│   │   ├── mod.rs          # Main evaluation logic, EvaluationContext
+│   │   ├── mod.rs          # Public API surface with re-exports, EvaluationContext, RuleMatch
+│   │   ├── engine.rs       # Core evaluation logic (evaluate_single_rule, evaluate_rules, evaluate_rules_with_config)
 │   │   ├── offset.rs       # Offset resolution
 │   │   ├── types.rs        # Type reading with bounds checking
 │   │   ├── operators.rs    # Comparison operations

✅ Accepted

architecture /libmagic-rs/blob/main/docs/src/architecture.md

View Suggested Changes

@@ -126,7 +126,8 @@
 
 **Structure:**
 
-- `mod.rs`: Main evaluation engine with `EvaluationContext` and `RuleMatch`
+- `mod.rs`: Public API surface (~720 lines) with `EvaluationContext`, `RuleMatch` types, and re-exports
+- `engine.rs`: Core evaluation engine (~2,096 lines) with `evaluate_single_rule`, `evaluate_rules`, and `evaluate_rules_with_config` functions
 - `types.rs`: Type interpretation with endianness handling and signedness coercion
 - `offset/`: Offset resolution submodule
   - `mod.rs`: Dispatcher (`resolve_offset`) and re-exports
@@ -139,6 +140,8 @@
   - `comparison.rs`: `compare_values`, `apply_less_than`/`greater_than`/`less_equal`/`greater_equal`
   - `bitwise.rs`: `apply_bitwise_and`, `apply_bitwise_and_mask`, `apply_bitwise_xor`, `apply_bitwise_not`
 
+**Organization Note:** The evaluator module was refactored to split a monolithic 2,638-line `mod.rs` into focused submodules, keeping the public API surface in `mod.rs` and moving core evaluation logic to `engine.rs`. This maintains the same public API through re-exports (no breaking changes) while improving code organization and staying within the 500-600 line module guideline.
+
 **Implemented Features:**
 
 - ✅ **Hierarchical Evaluation**: Parent rules must match before children

✅ Accepted

Code Quality Standards And Conventions

View Suggested Changes

@@ -111,8 +111,14 @@
 
 **Current Practice:** The codebase shows flexible adherence to this guideline:
 - Most files stay under 800 lines
-- [Core files like `src/lib.rs` (1,261 lines)](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/lib.rs) and [`src/evaluator/mod.rs` (2,639 lines)](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/mod.rs) exceed the threshold
+- [Core files like `src/lib.rs` (1,261 lines)](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/lib.rs) exceed the threshold
 - Large files typically include extensive inline test coverage using `#[cfg(test)]` blocks
+
+**Successful Refactoring Example:** [`src/evaluator/mod.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/mod.rs) was previously 2,639 lines (4.4× the guideline). It was split into:
+- `mod.rs` (~720 lines) - Public API surface (`EvaluationContext`, `RuleMatch`, re-exports)
+- `engine.rs` (~2,096 lines) - Core evaluation logic (`evaluate_single_rule`, `evaluate_rules`, `evaluate_rules_with_config`)
+
+This demonstrates how to address oversized files while maintaining API compatibility through re-exports.
 
 ### Module Organization Pattern
 
@@ -125,7 +131,8 @@
 - Additional submodules: `preprocessing.rs`, `hierarchy.rs`, `loader.rs`, `format.rs`
 
 **Evaluator Module** ([`src/evaluator/`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/)):
-- [`mod.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/mod.rs) (2,639 lines) - Core evaluation logic
+- [`mod.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/mod.rs) (~720 lines) - Public API surface and context management
+- [`engine.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/main/src/evaluator/engine.rs) (~2,096 lines) - Core evaluation logic
 - [`types.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/types.rs) (1,505 lines) - Type reading and interpretation
 - Focused submodules: `operators.rs`, `offset.rs`, `strength.rs`
 

✅ Accepted

evaluator /libmagic-rs/blob/main/docs/src/evaluator.md

View Suggested Changes

@@ -21,9 +21,22 @@
 Memory Map    Context State      Endian Handling   Match Logic      Hierarchical
 ```
 
+## Module Organization
+
+The evaluator module is organized into focused submodules:
+
+- **`evaluator/engine.rs`** - Core evaluation logic (`evaluate_single_rule`, `evaluate_rules`, `evaluate_rules_with_config`)
+- **`evaluator/mod.rs`** - Public API surface (types, context, re-exports)
+- **`evaluator/offset.rs`** - Offset resolution
+- **`evaluator/operators.rs`** - Operator application
+- **`evaluator/types.rs`** - Type reading and coercion
+- **`evaluator/strength.rs`** - Rule strength calculation
+
+From a public API perspective, all types and functions are imported from the `evaluator` module as before -- the internal organization is transparent to library users.
+
 ## Core Components
 
-### EvaluationContext (`evaluator/mod.rs`)
+### EvaluationContext
 
 Maintains state during rule processing:
 
@@ -49,7 +62,7 @@
 - `timeout_ms()` - Query configured timeout
 - `reset()` - Reset context state for reuse
 
-### RuleMatch (`evaluator/mod.rs`)
+### RuleMatch
 
 Represents a successful rule match:
 

✅ Accepted

Parser-Evaluator Architecture

View Suggested Changes

@@ -123,7 +123,8 @@
 
 The [evaluator module](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator) is organized into specialized submodules:
 
-- **`mod.rs`**: Main evaluation engine with `evaluate_rules()`, `evaluate_single_rule()`, and `EvaluationContext` management (2,639 lines)
+- **`mod.rs`**: Public API surface with `EvaluationContext`, `RuleMatch`, and re-exports of core functions (~720 lines)
+- **`engine.rs`**: Core evaluation engine with `evaluate_single_rule()`, `evaluate_rules()`, and `evaluate_rules_with_config()` (~2,096 lines)
 - **`offset/`**: Offset resolution submodule for converting `OffsetSpec` to absolute byte positions
   - **`mod.rs`**: Dispatcher (`resolve_offset`) and public API re-exports
   - **`absolute.rs`**: Absolute offset resolution with `OffsetError` and `resolve_absolute_offset()`
@@ -137,7 +138,7 @@
   - **`bitwise.rs`**: Bitwise operations (`apply_bitwise_and`, `apply_bitwise_and_mask`)
 - **`strength.rs`**: Rule strength calculation for prioritization (874 lines)
 
-Each sub-module exposes public functions and types while encapsulating implementation details, following [single-responsibility principles](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/mod.rs#L1-L8). The modular structure prepares the codebase for future v0.2.0 feature additions.
+Each sub-module exposes public functions and types while encapsulating implementation details, following [single-responsibility principles](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/mod.rs#L1-L8). The public API remains unchanged through re-exports in `mod.rs`. The modular structure prepares the codebase for future v0.2.0 feature additions.
 
 ### Evaluation Pipeline
 
@@ -535,7 +536,8 @@
 | [`src/parser/hierarchy.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/parser/hierarchy.rs) | Stack-based hierarchy building from indentation | ~180 |
 | [`src/parser/format.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/parser/format.rs) | Magic file format detection | ~90 |
 | [`src/parser/loader.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/parser/loader.rs) | File system loading and directory merging | ~310 |
-| [`src/evaluator/mod.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/mod.rs) | Main evaluation engine and context management | 2,639 |
+| [`src/evaluator/mod.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/mod.rs) | Public API surface (EvaluationContext, RuleMatch) and re-exports | ~720 |
+| [`src/evaluator/engine.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/engine.rs) | Core evaluation engine (evaluate_single_rule, evaluate_rules, evaluate_rules_with_config) | ~2,096 |
 | [`src/evaluator/offset/`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/offset) | Offset resolution submodule (absolute, relative, indirect) | ~290 |
 | [`src/evaluator/types.rs`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/types.rs) | Type interpretation with endianness support | 1,505 |
 | [`src/evaluator/operators/`](https://github.com/EvilBit-Labs/libmagic-rs/blob/e925ef6b3f2208fc8805a728ba3de55956f4447a/src/evaluator/operators) | Operator application submodule (equality, comparison, bitwise) | ~1,930 |

✅ Accepted

Note: You must be authenticated to accept/decline updates.

^{How did I do? Any feedback?}  

[codecov]

Codecov Report

❌ Patch coverage is 86.07595% with 11 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/evaluator/engine/mod.rs	86.07%	11 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

Refactors the evaluator module to reduce src/evaluator/mod.rs size by extracting the core rule-evaluation engine into a new src/evaluator/engine.rs module, relocating most evaluator tests alongside the engine implementation.

Changes:

• Added src/evaluator/engine.rs containing evaluate_single_rule, evaluate_rules, and evaluate_rules_with_config, plus most evaluator tests.
• Slimmed src/evaluator/mod.rs to focus on the public API types (EvaluationContext, RuleMatch) and re-exports.
• Removed the old src/evaluator/tests.rs file (tests relocated into modules).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
src/evaluator/tests.rs	Deleted; evaluator tests moved into other evaluator modules.
src/evaluator/mod.rs	Introduces engine submodule and re-exports; keeps context/result types and their tests.
src/evaluator/engine.rs	New engine module with core evaluation functions and most former evaluator tests.

[mergify]

🧪 CI Insights

Here's what we observed from your CI run for e879b4e.

🟢 All jobs passed!

But CI Insights is watching 👀

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (2)

docs/ARCHITECTURE.md:94

• The architecture tree lists engine.rs, offset.rs, and operators.rs, but the current source layout uses an engine/ submodule directory and offset/ + operators/ submodules (directories). Updating this tree will keep the top-level architecture doc aligned with the code.

│   ├── evaluator/          # Rule evaluation engine
│   │   ├── mod.rs          # Public API surface with re-exports, EvaluationContext, RuleMatch
│   │   ├── engine.rs       # Core evaluation logic (evaluate_single_rule, evaluate_rules, evaluate_rules_with_config)
│   │   ├── offset.rs       # Offset resolution
│   │   ├── types.rs        # Type reading with bounds checking
│   │   ├── operators.rs    # Comparison operations
│   │   └── strength.rs     # Strength calculation and sorting

src/evaluator/engine/tests.rs:299

• src/evaluator/engine/tests.rs is very large (~1,883 lines) and exceeds the repo’s 500–600 line source-file guideline. Since this PR is specifically aiming to split monolithic modules, consider splitting this test file into multiple focused test modules/files (e.g., evaluate_single_rule_*, evaluate_rules_*, error recovery, end-to-end parsing) to keep navigation and future edits manageable.

[cursor]

Stale comment

Security review completed for PR #153.

Findings (priority-ordered)

• No high-confidence vulnerabilities found in the diff.

What I validated

• Buffer safety: no new unchecked indexing on untrusted buffers in production evaluator code; rule evaluation still routes through offset/type readers with error handling.
• Integer/offset safety: no new risky integer casts or offset arithmetic paths introduced by the refactor; timeout comparison remains bounded and uses widening conversion.
• DoS/resource controls: recursion depth and timeout controls are still enforced; this PR preserves and slightly improves critical-error propagation for timeout/recursion-limit paths.
• Unsafe/panics: no new unsafe, unwrap(), or expect() in library runtime paths introduced by this PR.
• Dependency risk: no dependency-manifest changes in this PR.
• Info disclosure: no new error-message paths that expose buffer contents or internal paths.

Security result

• Status: pass (no security blocker identified in this PR).

Residual risk note: this PR is largely a module split/refactor, so baseline evaluator complexity risks (large rule sets until timeout) remain project-level concerns rather than newly introduced issues here.

---

  

[cursor]

Security Review Result: PASS

No high-confidence vulnerabilities found in this PR.

What I reviewed

• Buffer safety and offset arithmetic in src/evaluator/engine/mod.rs (new location of core evaluator logic)
• Integer/overflow behavior in rule counting, timeout checks, and recursion-depth handling
• DoS/resource controls (timeout path, recursion-depth limit behavior)
• Forbidden patterns (unsafe, raw pointers, unwrap/expect in library code)
• Dependency/supply-chain risk impact from the diff

Findings (prioritized)

• No confirmed vulnerabilities introduced by this diff.

Notes

• The production-code changes are a refactor/split of evaluator logic into engine plus test/docs moves; no new dependency files were changed (Cargo.toml/Cargo.lock unchanged).
• cargo audit completed successfully (no advisories reported for the current lockfile).
• cargo deny check passed (advisories ok, bans ok, licenses ok, sources ok).

Residual risk

• As with prior behavior, pathological rule sets can still consume CPU up to configured timeout/recursion limits; this PR does not appear to weaken those protections.

---

  

[cursor]

PR Risk Assessment

Risk level: Medium-High
Code review required: Yes

Evidence-based assessment (from diff)

• Core runtime evaluator logic was moved from src/evaluator/mod.rs into new src/evaluator/engine/mod.rs and re-exported.
• This touches a shared, performance-sensitive library codepath (evaluate_single_rule, evaluate_rules, evaluate_rules_with_config) used broadly by file-type detection.
• The refactor is large (~4k LOC churn including tests/docs), increasing regression surface even when mostly structural.
• There are behavioral-path changes in error propagation inside recursive child evaluation (timeout/recursion error handling), so this is not purely formatting/mechanical.
• Prompt/instruction documentation was also updated in AGENTS.md (non-runtime, but still behavior-relevant for automation guidance).

Reviewer assignment outcome

• Checked existing assigned reviewers: none.
• Attempted to request a non-bot reviewer (param-jasani) based on recent contribution history.
• GitHub rejected assignment because there are no eligible non-author collaborators available for this repository.

Approval decision

• Per policy, Medium-High risk PRs are not auto-approved.
• Also noted an existing approval from another bot reviewer; no additional approval was added by this automation.

If this PR is updated later, risk will be re-evaluated and any prior automation approval will be dismissed if risk increases.

---