fix(findings): deduplicate cross-analyzer findings before scoring

[mimran-khan]

Summary

When multiple analyzers detect the same pattern in multiple files of a skill, each occurrence was treated as a completely independent finding. A skill that calls subprocess.run(cmd, shell=True) in 5 modules got 5 TM1 findings, each contributing full points to the risk score — even though it represents the same architectural decision observed in different places.

Changes

• nodes/deduplicate.py: New module with a pure deduplicate() function implementing two-pass deduplication:
  1. Same-file dedup: identical (rule_id, file, matched_text) → keep highest confidence
  2. Cross-file dedup: identical (rule_id, matched_text) across files → keep highest confidence representative
• nodes/report.py: Calls deduplicate() on findings before scoring and SARIF generation

Design Decisions

Decision	Rationale
Dedup key uses first 100 chars of matched_text	Prevents pathologically long matches from consuming memory while maintaining specificity
No cross-file dedup for empty matched_text	Without matched text, there's no reliable identity signal
Dedup before scoring (in report node)	Preserves raw findings for meta_analyzer LLM analysis, only deduplicates for final output
Sort output by severity then file	Stable, predictable report ordering

Behavioral Impact

Scenario	Before	After
shell=True in 5 files, same pattern	5 × TM1 HIGH findings	1 × TM1 HIGH finding
Different subprocess calls in same file	Multiple findings	Multiple (different matched_text)
Same rule_id, different patterns	All kept	All kept (different dedup keys)
Findings without matched_text	All kept	All kept (no cross-file dedup)

Test Plan

• 17 unit tests covering same-file dedup, cross-file dedup, no-matched-text preservation, edge cases (empty list, sorting, whitespace normalization, truncation, mixed scenarios, real-world simulation)
• Full test suite: 693 passed, 0 failures, 0 regressions
• ruff check and ruff format clean

Fixes #137

[mimran-khan]

PS: These issues were found while running bunch of skill evals using SkillSpector and hence raising all the issues so that i can use these with correct reference to Upstream code

[rng1995]

Review: deduplicate cross-analyzer findings before scoring

The deduplicate function itself is correct and very well tested (highest-confidence retention, whitespace normalization, no-matched-text passthrough, stable sort). My concern is how it's wired into report(), so I'm requesting changes.

Blocking: cross-file dedup removes findings from the report, not just from scoring

In report() the deduped list is assigned to filtered_findings and fed to both _compute_risk_score and _build_sarif:

findings = deduplicate(raw_findings)
filtered_findings = findings

The cross-file pass keys on (rule_id, matched_text[:100]) and ignores file, so the same matched pattern across N files collapses to a single finding with a single location. Consequences for a security scanner:

• The emitted SARIF/JSON shows one affected file; the other N-1 files are silently unrepresented. A consumer (e.g., SARIF code scanning) only annotates one location, which can leave genuinely affected files un-remediated.
• The PR title scopes this to "before scoring," but the implementation also mutates the reported findings — broader than advertised.
• "cross-analyzer" in the title implies merging duplicate reports from different analyzers, but the key is a single rule_id, so this actually collapses one rule's matches across files.

Recommendations (either is fine)

1. Apply dedup to the score computation only and keep the full findings list in the report; or
2. Aggregate collapsed instances into the surviving finding (retain all file locations / an occurrence count) instead of discarding them, so no affected-file information is lost.

Minor

• The sort uses severity_order.get(f.severity, 4) without normalizing case. A non-uppercase severity sorts last; use f.severity.upper() to match the rest of the codebase (e.g. _compute_risk_score).
• matched_text[:100] means two distinct payloads sharing a 100-char prefix collapse cross-file — acceptable as noise reduction but worth a comment given the security context.

Inter-PR note

PR #139 (per-rule diminishing returns) targets the same repeated-pattern saturation via a different mechanism and edits report.py from the same base. Once both land, dedup runs first, so #139's diminishing weights rarely trigger for identical patterns (partial redundancy). Please confirm the combined behavior is intended.

[mimran-khan]

Thanks for the thorough review! I've addressed the blocking concern:

Cross-file dedup now only affects scoring, not the report.

• deduplicate() is applied only to the findings list passed to _compute_risk_score()
• The full (non-deduped) findings list is used for SARIF, JSON, markdown, and terminal output — all affected file locations are preserved
• Added a test (test_report_dedup_affects_score_only_not_report_output) that verifies 4 files with the same pattern all appear in the report while the score remains lower than 4x base points

Also fixed the case sensitivity issue on the severity sort (f.severity.upper()).

The matched_text[:100] prefix collision is kept as-is since it's an acceptable noise reduction tradeoff — added a note in the code comment.

[rng1995]

Re-review: deduplicate cross-analyzer findings before scoring

The blocking concern is resolved — dedup no longer mutates the reported findings. Approving.

Verified

• report() now feeds the deduplicated list only to scoring (findings_for_scoring = deduplicate(raw_findings) → _compute_risk_score), while _build_sarif, JSON, markdown, and terminal all render the full filtered_findings. All affected file locations are preserved in the report/SARIF; only the score is de-duplicated.
• Severity-sort case bug fixed: severity_order.get(f.severity.upper(), 4).
• New test_report_dedup_affects_score_only_not_report_output asserts all 4 files appear in the report while the score stays below 4x base — exactly the behavior I asked for.

Non-blocking

• matched_text[:100] prefix-keying can collapse two distinct payloads that share a 100-char prefix; acceptable as noise reduction and now documented.
• #139 (per-rule diminishing returns) has merged, so dedup runs first and diminishing weights apply to what remains — the combined behavior is sensible.

[rng1995]

Re-review: deduplicate cross-analyzer findings before scoring — approving

The blocking concern is resolved: deduplication now affects scoring only, not the emitted report. Approving.

In report() the deduped list is scoped to the score computation, while every output path consumes the full filtered_findings (report.py, ~L135-171):

findings_for_scoring = deduplicate(raw_findings)
filtered_findings = raw_findings
...
risk_score, ... = _compute_risk_score(findings_for_scoring, has_executable_scripts)
sarif_report = _build_sarif(filtered_findings)

_build_sarif, JSON, markdown, and terminal all receive filtered_findings, so every affected file location is preserved in the report/SARIF — only scoring is deduped. That fully addresses the un-remediated-location risk I raised.

Also confirmed:

• Severity sort case-sensitivity fixed in deduplicate(): severity_order.get(f.severity.upper(), 4).
• New regression test test_report_dedup_affects_score_only_not_report_output asserts all four step{0..3}.py files appear in the report while risk_score < 4 * 25 — exactly locking in "report complete, score deduped."

Non-blocking: the matched_text[:100] prefix-collision tradeoff is acceptable noise reduction and is now documented in the code comment.

Inter-PR note: #139 (per-rule diminishing returns) has already merged, so dedup runs first and the diminishing weights apply afterward. The combined behavior is sensible — dedup collapses identical patterns, then diminishing returns bound any remaining repetition. Thanks for the fix.