fix(static_yara): surface dropped rule files instead of reporting completed

[Souptik96]

Fixes #554

What was wrong

A rule file passed through --yara-rules-dir that YARA cannot compile, or that SkillSpector cannot decode as UTF-8/base64, is dropped whole with only debug-level logging. _load_rules already counted these (materialize_skipped + compile_skipped), but only logged the total — node() never saw it, so every scanned component could still report COMPLETED, analysis_completeness: complete, and the recommendation stayed SAFE, because the rule that would have flagged something simply never ran. --fail-on-incomplete correctly has nothing to key off, so it exits 0.

Reproduced with the issue's own scenario: a workspace with a valid custom rule and a syntactically broken one in the same --yara-rules-dir. The good rule fires, but the run reports a clean scan regardless.

What this changes, and a design choice I want to flag

• The skip count is recorded on the same module-level cache the compiled rules already live on (_rules_skipped_count, read back via the new rules_skipped_count()), and folded into a PARTIAL ledger event in node(), using the existing READ_ERROR reason and LedgerRecordType.SYSTEM (it isn't scoped to a scanned skill file, so I used a synthetic "yara_rules/" path — ledger paths must be relative POSIX, and the real rules directory is absolute).
• That event flows through the existing degraded/completed decision in node() unchanged, so this is additive to the existing status machinery rather than a new mechanism.
• Deliberately did not change _load_rules's return signature. My first pass returned (compiled_rules, skipped_count) as a tuple, which is the more obvious API, but 15 tests in test_static_yara.py do monkeypatch.setattr(static_yara, "_load_rules", lambda _extra_dir: rules), returning a bare yara.Rules object — all of them would have silently broken by unpacking a yara.Rules as a 2-tuple. I chose the module-global read-back instead specifically to avoid that blast radius for an internal detail those tests don't exercise. Happy to go the tuple route instead if you'd rather have the cleaner API and take the test-file diff — just say so.
• Did not use SYNTAX_ERROR (already reserved for "Python source could not be parsed" per REASON_MESSAGES) or invent a new LedgerReason for this; READ_ERROR's existing message ("File content could not be read") is generic enough to cover both the decode and compile failure cases the count already sums together.

Testing

.venv/bin/python -m pytest tests/nodes/analyzers/test_static_yara.py -q
# 87 passed

.venv/bin/python -m pytest -q
# 4971 passed, 14 skipped, 38 deselected, 4 xfailed

.venv/bin/python -m ruff check src/skillspector/nodes/analyzers/static_yara.py tests/nodes/analyzers/test_static_yara.py
# All checks passed!

.venv/bin/python -m ruff format --check src/skillspector/nodes/analyzers/static_yara.py tests/nodes/analyzers/test_static_yara.py
# 2 files already formatted

New test builds a valid rule and a syntactically broken one (missing closing brace, a real YARA syntax error, matching the issue's own repro rather than a decode failure) in the same --yara-rules-dir, asserts the valid rule still fires, the analyzer status is not "completed", and the ledger records the drop with observed_artifacts=1.

Negative control, reverting only static_yara.py and keeping the test:

AssertionError: a dropped custom rule must not report a clean scan
assert 'completed' != 'completed'

Restoring the fix, all 87 tests in the file pass again.

I did not reproduce this on Windows (the issue notes the same result on Windows 10 and Ubuntu/WSL2) — tested on Linux only, CPython 3.12.14, yara-python==4.5.4 (same version the issue reports).

[rng1995]

[SkillSpector Review]

Reviewed current head 4e753fe71cae2a3ecfe7df258c760115a1ed3f6c, including the complete two-file diff, surrounding rule-cache and analyzer-status logic, tests, existing discussion, and exact-head checks. The mixed valid/invalid-rule case is now surfaced in the ledger, and all five hosted checks pass.

Changes are requested because the skipped-rule count is stored in a module global and read separately after _load_rules returns. Concurrent scans with different rule directories can interleave those operations, so one scan can consume another rule set's count and still report completed after its own rule was dropped. Bind the skip metadata atomically to the returned/cached compiled rule set (or protect the load-and-read operation with appropriate synchronization) and add a deterministic concurrency regression.

[yashrajp22]

The review of head 4e753fe71cae2a3ecfe7df258c760115a1ed3f6c is complete. Two additional fixes are needed: the synthetic rule-load event can collide with a real file, and rejected rules still lack the default-level diagnostics requested in #554. The existing skip-count concurrency finding also remains reproducible; I have not duplicated that comment.

The ordinary mixed valid/invalid-rule case now correctly produces a nonfatal partial report, strict CLI exit 1, and safe_to_install=false through programmatic MCP.

Validation used fresh wheels and pinned source for base c13f70ebf14905912c616a58c9a8cb8112ef94a4 and this head. All 12 complete sample directories ran in all four combinations (48 scans), with matching source/wheel reports. The 98 selected tests passed in each HEAD mode. Focused checks covered malformed syntax/encoding/BOM, cache transitions, concurrency, ledger identity, CLI/MCP, suppression and all report formats, resource/failure precedence, and recursive/transitive aggregation. Greptile's cache-metadata observation was independently reproduced and grouped with the existing shared-metadata finding.

Scope: Linux and offline checks; transitive remote targets were mapped to local fixtures. Nine sample reports remain partial for unrelated reference/obfuscation limitations, so this is not an all-rule accuracy or live-provider result. The PR contribution is based on 2e9ae8d1cfa6e339f7035f876d3ac2e1c6ce24e6; unrelated AS3 differences from newer main were kept separate.

[Souptik96]

Thanks both — the reviews were specific enough to fix directly, and @rng1995's point about the two globals was the one I should have caught myself. 6e07493 addresses all three findings.

All three turned out to be the same shape: the dropped-rule total was reported through a channel that wasn't tied to the scan that produced it — a module global read after the fact, a ledger work ID shared with component work, and a DEBUG log nobody sees at default verbosity.

---

① Skip count bound to its rules — @rng1995 [P1]

rules and rules_skipped are obtained from two separately mutable module globals … Return/cache the compiled rules and their skip metadata as one value, or lock the load-and-read transaction, and add a regression that forces this interleaving.

Done both. New load_rules_with_skips() returns the rules and their own count from one transaction guarded by a reentrant _RULES_LOCK; node() now calls it instead of _load_rules() followed by a separate rules_skipped_count().

_load_rules() keeps its single-value signature, and load_rules_with_skips calls it through the module global, so every existing monkeypatch.setattr(static_yara, "_load_rules", ...) double still applies — that compatibility was the reason the count was a global in the first place, and it survives. rules_skipped_count() stays for single-threaded callers and now reads under the lock. The three cache globals are documented as one logical value that must only be written or read as a set.

Confirmed the interleaving before fixing it — scan A drops one rule, scan B loads a clean set, A then reads B's count:

OLD separate-read: A dropped 1, reads 0  -> misreports clean: True
NEW atomic:        A got 1 (expect 1), B got 0 (expect 0)
threaded 25x2 under contention: mismatches = 0

For the regression you asked for, I did not want a test that passes on timing luck, so there are two. test_load_and_read_is_serialized_against_other_scans proves the lock is genuinely held for the whole transaction: mid-transaction it starts another thread and asserts that thread cannot acquire _RULES_LOCK at all. test_concurrent_scans_never_report_another_rule_sets_count then runs two real scans over 50 observations and asserts each sees only its own total.

One trade to flag explicitly: the lock serializes rule compilation across concurrent scans. I judged that acceptable because compilation is cached and already deadline-bounded, and a scanner reporting a false clean is worse than one loading rules serially. If you would rather not serialize compilation, the alternative is caching (rules, skipped) as a single immutable value and having callers hold a reference to it — happy to switch if you prefer that shape.

---

② Rule-load work ID can no longer collide — @yashrajp22

Could we give rule-load events a work ID that cannot overlap with component work? … Changing only the synthetic filename would still allow another valid filename to collide.

Agreed, and that last sentence is why I did not just rename the path. ledger_event derives the identity as analyzer_id or f"{record_type}:{phase}", so passing analyzer_id=ANALYZER_ID made this event static_yara + yara_rules — identical to the planned work item for a component of that name. The event now omits analyzer_id, so the identity falls back to system:static, which is disjoint from every analyzer work item by construction. No filename can collide, not just not yara_rules.

Reproduced your exact case first (a component named yara_rules plus one rejected rule):

before:  work-9f4138c79afbaae... path='yara_rules' type=work_item
         work-9f4138c79afbaae... path='yara_rules' type=system     <- identical
         DUPLICATE work_ids: 1
after:   work-9f4138c79afbaae... path='yara_rules' type=work_item
         work-09561a184b9163d... path='yara_rules' type=system
         DUPLICATE work_ids: 0

The regression asserts both the event IDs and the advertised planned_work IDs stay distinct, since reconciliation requires exactly one event per planned target — and that the scan is still partial rather than clean.

---

③ Rejected rules named at default level — @yashrajp22, #554

Could we also report each rejected rule at the default WARNING level, including its filename and a bounded decode/compile reason, as #554 requests? … The rejection handlers remain at DEBUG.

Both handlers now log at WARNING with the filename and a bounded reason. Your three cases:

WARNING static_yara: rejected rule file bad_utf8.yar (could not decode): 'utf-8' codec can't decode byte 0xff in position...
WARNING static_yara: rejected rule file acme.yar (could not compile): line 0: syntax error, unexpected end of file...
WARNING static_yara: rejected rule file bom.yar (could not compile): line 1: non-ascii character

This needed one change beyond the log level, which is worth calling out: the compile path only had the namespace, and _rule_namespace() strips the extension, so it would have logged acme rather than acme.yar. _build_namespace_map now optionally fills a {namespace: filename} map — passed in rather than returned, to keep its two-value signature that existing callers and tests unpack directly.

_bounded_rejection_reason collapses newlines and caps the echoed text at 200 characters, because rule sources are attacker-influenced when --yara-rules-dir points at untrusted content and YARA errors can quote the offending source line.

---

Verification

• Negative control: all 9 new tests fail with the source change reverted and the tests kept; 9/9 pass with it.
• tests/nodes/analyzers/test_static_yara.py: 96 passed.
• Full suite: 4941 passed, 19 pre-existing failures — I ran the same suite on unmodified 4e753fe and the failure set is identical (build_context, compare_scan_accuracy, create_github_release, input_handler, json_container_ownership, security_end_to_end; none in the touched files). test_security_end_to_end.py gives the same 3 failed / 88 passed either way.
• ruff check, ruff format --check, and mypy clean on both files.
• Windows / Python 3.13 only. The pre-existing failures above are consistent with that environment rather than with this change, but I have not verified on Linux or macOS locally — the hosted matrix covers that.

One thing I found in my own tests rather than let it sit: the contended-threads test initially passed vacuously, because an exception inside a worker thread does not fail a pytest test. It now captures worker exceptions and asserts an observation count of 50, so it cannot pass when the scans never ran. Also extended the autouse cache fixture to reset _rules_skipped_count, which is part of that cache and was leaking between tests.

Disclosure: written with AI assistance under my direction. I reproduced each finding before fixing it, ran the negative control and full-suite comparison myself, and reviewed this comment before posting.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 6e07493c9956dcf2ad7b2f032f58b125cc978fa9 against all three prior threads, the complete rule-cache/ledger/logging diff, concurrency tests, surrounding no-rules paths, and exact-head checks.

The lock now makes a successful load-and-count transaction atomic, and the work-ID collision plus default-level rejected-file reporting are addressed. One cache-integrity path remains. _load_rules() sets _rules_skipped_count and returns without replacing or clearing _compiled_rules / _rules_hash when no rule files exist or compilation yields no rules. A later request for the previously cached hash then returns those cached rules paired with the intervening load's count. For example, load A with one valid and one rejected rule, load an empty/all-rejected set B, then load A again: the A cache hit can report B's count (including zero), recreating a false-complete YARA scan. Keep rules, hash, and skip count as one immutable cache entry or invalidate the cached rules/hash on every non-populating path; add this A→B→A sequence as a regression.

All six exact-head checks pass, but this remaining completeness-accounting defect and active change requests block merging.

Priority: P0 — incorrect rule-drop accounting can make an incomplete malware scan look complete.

[Souptik96]

Thanks — you were right, and the A→B→A sequence you described reproduces exactly.

_load_rules() sets _rules_skipped_count and returns without replacing or clearing _compiled_rules / _rules_hash when no rule files exist or compilation yields no rules. A later request for the previously cached hash then returns those cached rules paired with the intervening load's count.

Confirmed on 6e07493, both flavours of B, and it does surface as a false-complete
scan at the node() level:

--- B1 no rule files ---
load A  : rules=True skipped=1
load B  : rules=False skipped=0
load A' : rules=True skipped=0      <- A' served from A's cache entry
          DEFECT: A' reported 0, A dropped 1
--- B2 all rejected (A drops 2, B drops 1, asymmetric on purpose) ---
load A' : rules=True skipped=1      <- reported B's count, not A's
--- node() end to end ---
scan A  : status=degraded
scan A' : status=completed rule_skip_events=0

Fixed by taking both of the options you offered rather than one. The three globals
are now a single frozen _RuleCacheEntry(rules, rules_hash, skipped_count) in
_rule_cache, published only by replacing the entry wholesale, so a cache hit takes
its count from the entry and can't pair one load's rules with another's total. On
top of that, every path that doesn't produce usable rules clears the entry outright —
no rule files, all rules rejected, compile yielding nothing. I also clear
_rules_skipped_count at the top of the locked transaction, so a load that raises
part way through can't leave a previous total readable through
rules_skipped_count(); the cache entry itself is deliberately left alone on that
path, since an entry is keyed by its own content hash and stays a valid answer for it.

Same script after the fix:

load A' : rules=True skipped=1      (B1, was 0)
load A' : rules=True skipped=2      (B2, was 1)
scan A' : status=degraded rule_skip_events=1   (was completed / 0)
FAILURES=0

Added the regression you asked for as
test_cached_rules_never_report_a_later_loads_skip_count, parametrized over both
non-populating paths, with asymmetric counts in the all-rejected case so an inherited
count can't look correct by coincidence. Siblings cover the rest of the surface:
test_non_populating_load_leaves_no_cache_entry (both paths),
test_cache_entry_cannot_be_mutated_in_place, and
test_rescan_after_a_non_populating_load_still_reports_the_dropped_rule for the
end-to-end consequence. All deterministic and single-threaded — this is a
cache-integrity defect rather than a race, so load ordering alone reproduces it.

Negative control: with the source fix reverted and the tests kept, all six fail. Three
fail on the behaviour itself — rule set A dropped 1 rule(s) but the reload reported 0, which is B's count (0), the same for 2-vs-1, and assert 'completed' != 'completed' for the rescan. To be straight with you, the other three reference
_rule_cache, which doesn't exist on the old code, so there they fail at their
precondition rather than on the defect; they're structure guards for the future, not
independent proof.

_load_rules() keeps its single-value signature, so the existing
monkeypatch.setattr(static_yara, "_load_rules", ...) doubles are untouched, and the
reentrant-lock transaction from the last round is unchanged — I ran
test_load_and_read_is_serialized_against_other_scans,
test_concurrent_scans_never_report_another_rule_sets_count and
test_skip_count_travels_with_the_rules_it_describes by name and they pass. Removing
_compiled_rules/_rules_hash meant updating the autouse cache-reset fixture and
TestRuleCaching; that was load-bearing, not cosmetic — left alone, the fixture would
have quietly stopped resetting the cache and leaked rules between tests.

Two trade-offs worth naming. First, clearing the entry means A is recompiled after an
intervening non-populating load instead of hitting cache. The immutable entry alone
would have fixed the counting and kept the cache, since an entry is hash-keyed and
always correct for its own hash — I kept the invalidation because you asked for it and
it closes off a class of future mistakes, and the cost is negligible in practice
because both non-populating paths require the shipped built-in rules to be missing or
entirely uncompilable. Second, rules_skipped_count()'s documented meaning changed: on
a cache hit it now returns the cached entry's count instead of zero. That's the point
of the fix, and the old docstring claiming otherwise was part of the faulty reasoning,
so I corrected it.

ruff check and ruff format --check are clean. Full unit suite: 19 failed / 4948
passed against a 20 failed / 4941 passed baseline on 6e07493 — no new failures, and
the 19 are all pre-existing and all outside static_yara. One baseline failure
(test_nine_case_contract_across_public_surfaces) doesn't appear in the fix run; I
checked rather than claiming credit, and it's flaky in my environment: it both passed
and failed on repeated isolated runs with the fix held constant, and loading the
built-in rules here skips 0 files, so this analyzer's skip accounting can't move
analysis_completeness in that test either way. Not verified on Linux or on
3.13/3.14 — everything above is Windows + CPython 3.12.

[Souptik96]

Force-pushed to add the missing DCO Signed-off-by trailer: 95572b6 is now 6abb623. The tree is identical and nothing else changed; the reply above describes this commit.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 6abb623158ebbce9e92c60ecc51766975903f443 against all three inline threads and the subsequent cache-integrity change request.

The concurrency and A→B→A cache-count defects are fixed: the load/count transaction is locked, cache entries bind rules/hash/count, and non-populating loads invalidate the entry. Rejected-file WARNING diagnostics are also addressed. Distinct work IDs fix the original fatal ledger reconciliation failure.

One related path collision remains, described inline. A benign, fully read file named yara_rules, referenced from SKILL.md, is still mistaken for the synthetic rule-load scope during finalization. With one valid condition: false custom rule and one syntactically broken custom rule, the real CLI reports a false HIGH AE1, risk score 25, and 50% file coverage. Changing only the referenced filename to normal.txt produces no findings, score 0, and 100% file coverage. Both correctly remain nonfatal partial scans because a rule was rejected. Rule-load incompleteness must not invent a file-specific analysis-evasion finding based on the file's name.

Validation on the reviewed source, macOS/Python 3.12: 102 YARA tests and 185 related ledger/finalizer/deadline/budget/report tests passed; Ruff lint and formatting passed. CLI smoke confirmed ordinary exit 0 and --fail-on-incomplete exit 1, including a cache hit. All six hosted checks pass. The full local suite was not rerun.

Please distinguish rule-set scope from artifact paths throughout reference/coverage finalization and add an end-to-end regression for the referenced yara_rules case. Approval is withheld for this remaining report-integrity defect.

[Souptik96]

Thanks, confirmed. I reproduced it through the real CLI on 6abb623 with your exact setup: a harmless SKILL.md linking to a fully read file, one condition: false rule and one broken rule. The only thing that changes between the two runs is the referenced filename:

referenced file   issues      risk  coverage  partially_inspected  status
yara_rules        AE1:HIGH    25    50.0      1                    partial
normal.txt        (none)      0     100.0     0                    partial

Fixed in 61e9821, plus a small follow-up in 4cdb77d for the Markdown and terminal reports (below). You were right that changing the label wouldn't fix it. Any relative path is also a valid file name, so no label can be guaranteed not to collide. The fix tells these rows apart by record type, not by name:

• The rule-load event now uses a new LedgerRecordType.RULE_SET instead of SYSTEM. I didn't just exclude SYSTEM rows, because SYSTEM rows describe real artifacts and AE1 depends on them. Cache-phase opaque_content on a referenced file is one example (test_resolved_partial_reference_produces_one_canonically_counted_ae1).
• _reference_coverage_findings() skips RULE_SET rows when it groups exceptional outcomes by path.
• finalize_ledger() doesn't fold planned targets that belong to RULE_SET rows into per-component coverage. It identifies them by work ID, not by path.
• The public ledger_exceptions row gets scope: "rule_set". That field is part of _merge_exception_projection's key, so the row can't merge with a real file's same-path, same-reason row. In SARIF, the notification carries scope in properties and has no physicalLocation. Before this change it pointed artifactLocation.uri at yara_rules.

The work identity is now rule_set:static rather than system:static. It's still disjoint from every analyzer work ID, so the reconciliation fix from round 2 still holds, and test_rule_load_event_does_not_collide_with_a_component_of_the_same_name still passes.

Same CLI repro after the fix:

referenced file   issues   risk  coverage  partially_inspected  status
yara_rules        (none)   0     100.0     0                    partial
normal.txt        (none)   0     100.0     0                    partial

Both runs are still nonfatal partial scans, because a rule really was dropped.

Regression tests:

• TestRuleSkipAccounting::test_referenced_file_named_like_the_rule_set_is_not_charged_with_its_dropped_rule[yara_rules|normal.txt] is end-to-end through CliRunner with --yara-rules-dir, so it covers finalization and report generation. It asserts no issues, score 0, 100% coverage, no partial or uninspected files, is_complete false with execution_successful true, exactly one scope: "rule_set" row, an exit code of 1 under --fail-on-incomplete, and a SARIF notification with no location.
• test_rule_set_row_is_excluded_from_path_keyed_accounting_by_type (finalizer level) pins each consumer separately. It includes a control: a SYSTEM row on the same path still produces AE1 and stays a separate public row.

Negative control (source reverted, tests kept): all 3 fail. Only [yara_rules] fails on the defect itself (assert ['AE1'] == []). [normal.txt] fails only because the scope marker doesn't exist on the old code, and the finalizer test fails with LedgerRecordType has no attribute RULE_SET. Those two guard the new structure; they don't independently prove the bug. The CLI before/after tables above are the behavioural evidence.

Validation for 61e9821 (Windows, CPython 3.12.13): ruff check and ruff format --check are clean. Full unit suite (-m "not integration and not provider"): 20 failed / 4950 passed / 4 errors, against a baseline on 6abb623 of 24 failed / 4943 passed / 4 errors. Nothing failed that didn't also fail at baseline. Four baseline failures (three tests) didn't recur, but I don't think the change fixed them. All three are wall-clock checks that missed by a small margin on the slower baseline run (26 min against 14 min): 2.30 < 2.0 and 2.90 < 2.0 for the two nested_printf cases, 5.51 < 5.0 for dense-directory discovery, and a 15 s MCP stdio initialize timeout. None of them goes through this code. The other 20 are pre-existing and environment-specific: test_build_context with Windows secure-open, release tooling, compare_scan_accuracy, and letter-spacing. I haven't verified on Linux/macOS or on 3.13/3.14.

Markdown and terminal reports (4cdb77d). Those two tables still listed the row under the bare path yara_rules, so it read like a file row. They now label it as the rule set: rule set yara_rules in Markdown (the path stays in code style, the label sits outside it), and rule set yara_rules in the terminal. Rows without scope render as before. test_report_labels_rule_set_exception_as_rule_set_not_file[markdown|terminal] renders a rule-set row next to a file row with the same path and checks each line. With the renderer change reverted, both cases fail on the missing label. After this commit I re-ran ruff check, ruff format --check and tests/nodes (3311 passed, 9 failed, 4 errors). All of those failures and errors were already in the 61e9821 run. I didn't re-run the full suite for this commit.

[rng1995]

[SkillSpector Review]

Hi @Souptik96, thank you for closing the last report-integrity gap in the dropped-YARA-rule fix and for the before/after CLI tables that made it easy to verify!

Value and readiness: This head delivers #554. A rejected custom rule now makes the scan a nonfatal partial result (strict exit 1, MCP safe_to_install=false), and the rejected file is named at WARNING level. The rule-set row no longer affects path-keyed coverage or AE1. The fix itself is correct and every earlier finding is resolved. One required change remains: the rebase. It is not mechanical, because GitHub reports conflicts with main in _reference_coverage_findings, the function this fix changes, so the rebase has to carry the fix into main's rewritten version (details below). No CI has run on 4cdb77d4 yet. Once the rebased head is pushed and green, I expect to approve after a short re-check of the ported loop.

Previous findings:

• Skip count read from a separate global after _load_rules (rng1995, P1): Resolved (unchanged since 6abb623). load_rules_with_skips (static_yara.py:558) holds _RULES_LOCK across both the load and the count read.
• A→B→A cache returned B's count for A's rules (rng1995): Resolved. Both non-populating paths clear the entry (static_yara.py:522, :545). A cache hit reads cached.skipped_count (:531).
• Rule-load work ID collided with a component named yara_rules (yashrajp22): Resolved. The event omits analyzer_id, so its identity is rule_set:static, which cannot match any analyzer work item.
• Rejected rules only logged at DEBUG (yashrajp22): Resolved. Decode and compile rejections log at WARNING with the filename and a reason capped at 200 characters (static_yara.py:420, :461).
• Rule-set scope leaked into path-keyed accounting and produced a false HIGH AE1 (rng1995, P2): Resolved in 61e9821 and 4cdb77d:
  • The event now uses LedgerRecordType.RULE_SET (inspection_ledger.py:43).
  • _reference_coverage_findings skips RULE_SET rows (finalize_inspection_ledger.py:38).
  • finalize_ledger excludes RULE_SET work IDs from per-component coverage (inspection_ledger.py:891-898).
  • The public row carries scope: "rule_set", which is part of the merge key (:635), so it cannot merge with a real file's row.
  • The SARIF notification has no physical location (report.py:734).
  • Markdown and terminal output label the row as "rule set".
  • The end-to-end CliRunner regression runs once with the linked file named yara_rules and once with normal.txt. Both runs must show no AE1, score 0, 100% coverage, exactly one rule_set row, strict exit 1, and a SARIF notification with no location.

Material findings

1. [Blocker: rebase] src/skillspector/nodes/finalize_inspection_ledger.py:38: main rewrote _reference_coverage_findings. It now builds events_by_path (on main, around lines 534-548), and both _has_only_format_limitations and the AE1 diagnostics read from it. When you rebase:

   • Put the record_type == LedgerRecordType.RULE_SET → continue check at the top of that loop, next to the existing reference-caveat skip. Taking either side of the conflict as-is would drop the fix or drop main's PNG handling.
   • In report.py, keep both finalize_ledger and RULE_SET_SCOPE in the import.
   • In tests/nodes/test_finalize_inspection_ledger.py, keep both main's format-only PNG tests and test_rule_set_row_is_excluded_from_path_keyed_accounting_by_type.

   I checked main's other new consumers, and none needs a further change:

   • _status_paths_with_incomplete_evidence: static_yara still reports degraded with no reason code, so no paths become invalid.
   • The fatal-row dedupe for failed inventory items in finalize_ledger: the rule-set row is nonfatal.
   • _ledger_event_is_canonical: the path normalizes to yara_rules and the work ID recomputes from rule_set:static.

2. [Non-blocking] docs/scan-completeness.md:130 lists the public exception fields but not the new scope field. Consider one sentence saying that scope: "rule_set" (JSON ledger_exceptions[], SARIF properties.scope) marks a row whose path labels an analyzer's rule set rather than a skill artifact. Consumers should not join it to file paths.

PIC tradeoffs: The PR adds a public, optional scope field to ledger exception rows and a new internal record type. The change is additive, and I found no strict schema that rejects it. Consumers that group exceptions by path alone would still need to check scope.

Verification and gaps:

• I read the full seven-file diff and the 6abb623..4cdb77d delta.
• I traced the RULE_SET row through ledger creation, finalize_ledger, AE1 generation, exception merging, SARIF, Markdown, terminal output, and transitive re-pathing in cli._source_aware_ledger (record type and rule_set:static identity are preserved).
• I ran git merge-tree against current main (2226747e) to confirm the three conflicting files, and checked main's new finalizer logic for interactions.
• The tests and the CLI repro were not executed locally, per policy.
• No CI has run on this head because of the conflict. The previous head 6abb623 had all six checks green.
• Merge needs a rebase, six green checks on the rebased head, and a re-check of the ported loop. @yashrajp22's earlier change request (two findings, both resolved in 6e07493) is still recorded and should be re-reviewed or dismissed.

---

Decision: Changes Requested (reviewed head 4cdb77d4cff86e80c4f31b08185d861c3ec4922e)

[rng1995]

[SkillSpector Review]

Hi @Souptik96, a quick heads-up so nothing gets lost: to unblock this PR, a maintainer merged current main into your branch as e8c505c (a merge commit, no force-push). Please pull it before pushing anything else.

What the merge does:

• finalize_inspection_ledger.py: keeps main's rewritten _reference_coverage_findings (the events_by_path loop) and puts your RULE_SET skip at the top of that loop, next to the existing reference-caveat skip, so a rule-set label still never feeds path-keyed AE1 or format-limit checks.
• report.py: imports RULE_SET_SCOPE alongside main's finalize_ledger and sanitize_llm_provenance.
• Tests: keeps main's new finalizer and report tests and adds your two rule-set tests unchanged.

The source change against main matches your PR's own change line for line, apart from the import block main had already restructured. I'll re-review once CI finishes on e8c505c.

[rng1995]

[SkillSpector Review]

Hi @Souptik96, thank you for your patience while this was brought up to date with main!

Value and readiness: The only blocker from my 2026-10-02 review was the rebase onto main's rewritten _reference_coverage_findings. That is now done in merge commit e8c505c (a maintainer merge of main, no force-push), and all six CI checks pass on it. The fix for #554 is unchanged: a rejected custom rule makes the scan a nonfatal partial result, the rejected file is named at WARNING level, and the rule-set row stays out of path-keyed coverage and AE1. This is ready to merge once the remaining earlier change request from another reviewer is cleared.

Previous findings:

• Rebase required; the fix had to be ported into main's rewritten _reference_coverage_findings (Blocker): Resolved. finalize_inspection_ledger.py keeps main's events_by_path loop and skips LedgerRecordType.RULE_SET rows at its top, next to the reference-caveat skip. report.py imports RULE_SET_SCOPE alongside main's finalize_ledger and sanitize_llm_provenance. Your two rule-set tests were added unchanged next to main's new finalizer and report tests. The PR's src/ change against main matches the reviewed change line for line, apart from the import block main had already restructured.
• Skip count read from a separate global (concurrency) and rule-set path accounting (earlier rounds): Resolved, as recorded in the 2026-10-02 review. Resolving those two remaining threads now.
• docs/scan-completeness.md does not mention the new scope: "rule_set" field (Non-blocking): still open and optional.

Material findings
None.

PIC tradeoffs: Unchanged: the PR adds an optional public scope field to ledger exception rows. It is additive, and no strict schema rejects it.

Verification and gaps: I compared the merged head against the reviewed change. The PR's source diff against main is identical to the reviewed one apart from the import block. All six hosted CI checks passed on e8c505c. The merge resolution was written by a maintainer, not by the PR author. Tests were not run locally, per policy; CI ran the full suite on the merged tree. yashrajp22's earlier change request (both points verified fixed, with evidence in the resolved threads) still needs their re-review or a maintainer dismissal before this can merge.

---

Decision: Approved (reviewed head e8c505c38ebac51d1701ab47a3689597e76c9252)

[yashrajp22]

The earlier collision, skip-count race, and warning-detail issues are addressed. Fresh source and wheel checks reproduce two remaining issues below; both selected HEAD pytest runs passed 35 tests.

[Souptik96]

@yashrajp22 thank you for the review and the two repros. @rng1995 thank you for merging main in as e8c505c. I pulled it first. The two new commits (b508b64, e2d5ce9) sit on top of it as a fast-forward, with no rebase or force-push.

1. Rule-load lock ignored the caller's deadline (static_yara.py:572) — b508b64

• load_rules_with_skips() and _load_rules() now take _RULES_LOCK through a new _RulesLockWithinDeadline. When _RULE_LOAD_DEADLINE is set, it waits at most the workflow wall-clock time left in that budget. On timeout it raises the existing RUNTIME_LIMIT _YaraRuleResourceLimitError, so node() returns the same partial runtime_limit result it already returns for other rule-load deadlines. No new outcome was added.
• With no deadline set (direct callers outside node()), it blocks as before. A thread that already holds the reentrant lock gets it back at once, which covers the nested _load_rules() call.
• The snapshot is still atomic: rules and skip count are read inside one hold of the lock, or not at all.
• I wrote it as a small class rather than a contextlib.contextmanager. The generator form re-raises by assigning __traceback__, and the frozen, slotted _YaraRuleResourceLimitError rejects that with a TypeError. The full suite caught this through test_rule_discovery_limit_marks_every_component_partial.
• Tests:
  • TestRuleSkipAccounting::test_rule_lock_wait_honours_the_callers_deadline. Scan A is held inside the real _read_rule_bytes_cache path on a threading.Event, and scan B runs with a 1.2 s budget. B must return within its budget plus 0.5 s with runtime_limit on every component, and A must still get its own rules and skip count.
  • test_rule_lock_reentry_does_not_wait_on_an_expired_deadline.
• Measured locally, using your shape (A paused 3 s, B with a 1.5 s budget): B returned in 1.500 s and 1.516 s with runtime_limit. Uncontended, B took 15 to 32 ms. With the source fix reverted, the regression test fails because B waits 5.0 s, the test's safety release.

2. Rule-set identity lost in transitive status scoping (static_yara.py:1267 / cli.py) — e2d5ce9

• _source_aware_ledger() and _source_aware_status_events() now build the scoped work ID through one shared helper, _source_scoped_work_id(). The status path takes the identity behind each planned target from the child ledger: _ledger_work_identities() maps each child work ID to _ledger_work_identity(row). So the rule-set target is re-scoped with rule_set:static, the same identity its ledger row uses. The analyzer ID is now only the fallback for targets that have no child ledger row.
• Test: TestRuleSkipAccounting::test_rule_set_work_survives_transitive_status_scoping. It runs the real CLI and the real graph on a root plus two children (each child URL mapped to a local directory), with a rejected rule in each scope.
• Result: 3 rule_set exceptions, and the static_yara counts are 6 planned / 3 partial, with 0 unaccounted and every scope degraded. With the source fix reverted, the test reproduces your numbers exactly: 4 planned / 1 partial.

Verification

• Full suite on Windows, before and after: the same environmental failures (no bash/WSL, Windows paths, environment-variable length), plus the 3 new tests passing. Two wall-clock tests flake on this machine with and without the change: test_dense_directory_discovery_and_cache_complete_with_modest_real_elapsed_time and test_preparation_obeys_artifact_runtime_limit. I confirmed both by re-running them on the unmodified head.
• ruff check and ruff format --check pass on the touched files. mypy reports the same 8 existing cli.py errors before and after.

Not verified

• I did not run a wheel build or the Linux CI matrix locally. I'm relying on CI for those.
• I did not test a long-lived MCP server process directly. The concurrency test drives node() from two threads in one process.
• The optional docs/scan-completeness.md note about scope: "rule_set" is still not done. Happy to add it here or in a follow-up, whichever you prefer.

[rng1995]

Re-reviewed commit 7215857c179cf0db5bcbd6abc2d1abaf8ddf9408. All substantive prior review findings are addressed, and all six inline review threads are resolved.

• Compiled rules and skipped-rule metadata remain an atomic, coherent cache snapshot, including non-populating loads.
• Rejected rules have bounded WARNING diagnostics. Rule-set events retain distinct work identities and stay out of file coverage and AE1 accounting, with appropriate JSON, SARIF, Markdown, and terminal rendering.
• Contended rule-lock acquisition honors the caller's workflow deadline while preserving reentrant locking and the existing runtime_limit result.
• Transitive status scoping preserves each child rule-set identity; the root-plus-two-child regression reports 6 planned / 3 partial items with no unaccounted work.

Merged current main (a0d489e6) into the branch without rebasing or force-pushing. The Markdown conflict resolution preserves main's escaping helper and the rule-set label; regression expectations were updated for the escaped text. Independent review found no remaining issues.

Validation: DCO, change detection, lint/formatting, TypeScript tests, and Docker smoke tests passed. The full hosted unit suite is still running; this approval records the completed code review, and merge will wait for its successful result. Local review covered the focused concurrency, cache, collision, finalization, reporting, and transitive regressions. The merged-tree run had 979 passing tests; after adapting the Markdown expectations to main's escaping, all 31 focused Markdown/rule-set report tests passed. The remaining local CLI help startup timeout also reproduces on unchanged main; its timeout was not changed.

The optional documentation note for scope: "rule_set" remains a non-blocking follow-up. Approved.