fix(baseline): fingerprint every occurrence so repeated findings stay suppressed

[deepujain]

Fixes #630. Also covers the context-bearing repetition in #633.

Generating a baseline should suppress every finding from that scan. Previously the command fingerprinted the display-deduplicated findings, so only one occurrence was covered. Expanding that display record still loses each occurrence's original context and confidence, leaving findings on an unchanged rescan.

The report now retains the exact kept findings before display deduplication, and the baseline command fingerprints that list. Report presentation and filtering remain unchanged.

Validation: a real baseline then scan --baseline --format json round trip with AR2 findings in two files fails with the previous CLI call site and passes after repair. It checks that no issues remain and that both files have suppression fingerprints. uv run pytest tests/unit/test_cli.py tests/unit/test_suppression.py tests/nodes/test_report.py -q: 423 passed. Ruff lint and format checks pass. The optional full local suite was interrupted for host memory pressure after 4,983 tests passed; this is not a full-suite pass. Hosted unit CI is running on the repair head. No live-provider execution is claimed.

[rng1995]

[SkillSpector Review]

Hi @deepujain, thank you for your contribution to SkillSpector — we really appreciate the time you put into this! A few items need attention before it can be merged; details below.

Makes skillspector baseline expand the deduplicated effective_findings() with _expand_occurrences() before fingerprinting, so each folded occurrence gets its own entry. I traced how build_baseline_dict hashes the command's findings and how Baseline.reason_for/partition_findings hashes each occurrence on the next scan. The expanded entries only match when occurrences share every hashed field except location. That fixes the RP1 demo in #630 but not repeats of context-bearing rules (the #633 case). Competing PR #637 fingerprints the exact pre-deduplication list the report node partitions and covers both issues. #643 uses the same expansion approach as this PR.

Findings

1. [Blocking] src/skillspector/cli.py:3190: the expanded occurrences don't reproduce the findings the next scan checks.

   • finding_fingerprint hashes confidence, context and code_snippet (suppression.py:236,247-248; docs/SUPPRESSION.md:97-101 lists context and confidence as bound fields).
   • deduplicate() deliberately leaves confidence and location-specific context out of the compaction key and keeps only the representative's values (deduplicate.py:84-86,179,232-237).
   • _expand_occurrences copies those values onto every occurrence and changes only file, lines, columns and provenance (report.py:255-275).
   • Static-pattern analyzers attach a ±3-line context window to each match (for example static_patterns_anti_refusal.py:411 via static_runner.py:620,626). Every non-representative occurrence therefore gets a hash that partition_findings never computes.

   With this head, #633's repro (AR2 at SKILL.md:8 and references/notes.md:3) still reports notes.md:3 after the baseline is regenerated. Same-file repeats of any context-bearing rule behave the same way. The leftover findings still count toward the risk score, JSON issues, SARIF results and --fail-on-findings. The #630 demo passes only because RP1 (mcp_rug_pull.py:227-248) sets no context or snippet and uses a constant confidence. Expected: fingerprint the exact list the report node partitions (the kept findings before deduplication), as the #630 reporter suggested. #637 does this by returning active_findings from the report node.

2. [Blocking] tests/unit/test_suppression.py:1098: the new test builds both the baseline and the "per-line" findings from _expand_occurrences([folded]), so it only compares the expansion with itself. The real per-occurrence findings carry their own context. The test also never calls the CLI, so it stays green if the cli.py change is reverted. Please add a CLI round trip: baseline, then scan --baseline --format json, on a fixture where a context-bearing rule repeats across lines or files (for example #633's AR2 repro). Assert issues == [] and that every occurrence is in suppressed. Such a test fails on main and on this head.

#637 already implements the complete approach with an end-to-end round-trip test, so consolidating there may be simplest. If you'd rather continue here, switching to that approach plus the round-trip test would resolve both points. Thanks for the clear analysis and repro of #630.

Tests/CI: CI is green (lint, test-unit, DCO, docker-smoke). finding_fingerprint and the v2 schema are unchanged, so existing baselines stay compatible. The only test added is the self-referential unit test above.

---

Decision: Changes Requested (reviewed head 8e19a91f0795294ae1fb3869fcc37fac8ee30bfa)

[rng1995]

This matches the next scan only when every occurrence shares the representative's hashed fields. finding_fingerprint hashes confidence, context and code_snippet (suppression.py:236,247-248). deduplicate() intentionally leaves confidence and location-specific context out of the compaction key (deduplicate.py:84-86), and _expand_occurrences copies the representative's values onto every occurrence (report.py:255-275). For context-bearing static-pattern rules, such as AR2 with its ±3-line get_context window, the non-representative entries never match what partition_findings computes, so #633's repro still reports references/notes.md:3. RP1 in #630 only works because it carries no context. Please fingerprint the exact kept, pre-deduplication list the report node partitions, as #637 does with active_findings.

[deepujain]

The baseline now fingerprints active_findings retained before display deduplication, preserving each original context and confidence. A real CLI AR2 round trip across two files fails with the old call site and passes after repair; all423 adjacent CLI, suppression and report tests pass.

[rng1995]

Both the baseline and the partitioned findings here come from _expand_occurrences([folded]), so the test checks the expansion against itself rather than against the report node's real per-occurrence findings, which carry their own context. It also never calls the CLI, so it passes even if the cli.py change is reverted. Please add a CLI round trip (baseline, then scan --baseline --format json) with a context-bearing rule repeated across lines or files, for example #633's AR2 repro. Assert issues == [] and that every occurrence is in suppressed.

[deepujain]

Replaced the expansion-against-itself test with an actual baseline then scan --baseline --format json invocation. It checks issues are empty and both AR2 file occurrences have fingerprints; reverting the CLI call site makes it fail.

[rng1995]

[SkillSpector Review]

Hi @deepujain, thank you for reworking this to fingerprint the exact kept findings and for replacing the test with a real CLI round trip!

Value and readiness: The new head fixes the root cause. The report node now returns active_findings: the kept findings before deduplication, which is the same list partition_findings just checked. skillspector baseline fingerprints that list, so every occurrence gets an exact entry, including repeats of context-bearing rules (#633). On its merits, this is ready. It is now functionally the same change as #637, though, which was approved first, and the two conflict (same lines in cli.py, report.py and state.py). Only one should merge. I recommend merging #637 and closing this PR as a duplicate. #637 was the first complete implementation, and its test asserts the exact suppressed set. #657 also adds the same list under a different key (baseline_findings).

Previous findings:

• 1 (expanded occurrences don't reproduce the per-occurrence fingerprints): Resolved. src/skillspector/nodes/report.py:1840 returns active_findings, and src/skillspector/cli.py:3184-3190 fingerprints it. Each occurrence keeps its own context, code_snippet and confidence, so the hashes match what Baseline.reason_for computes on the next scan. _expand_occurrences is no longer used.
• 2 (self-referential unit test that never calls the CLI): Resolved. The old test is gone. tests/unit/test_cli.py:6083 runs the real graph: baseline, then scan --baseline --format json, with AR2 in SKILL.md and references/notes.md. It asserts issues == [] and that both files have AR2 fingerprints. On main it fails because only one AR2 entry is written. With the old expansion approach it fails because notes.md stays in issues.

Material findings

1. [Non-blocking] Duplicate of #637: see above. Please coordinate with the maintainers before more work here. If this PR is kept instead of #637, apply the two notes below.
2. [Non-blocking] src/skillspector/cli.py:3186-3190: if active_findings is ever missing, the code quietly falls back to effective_findings(), the incomplete path this PR replaces. The graph always sets the key, so this only matters for mocked results. Indexing it directly, as #637 does, and adding the key to the two mocked baseline tests would make a missing key fail loudly.
3. [Non-blocking] tests/unit/test_cli.py:6119: suppressed_count >= 2 would still pass if extra or wrong items were suppressed. Asserting the exact (id, file) set in suppressed would pin the behavior. Also consider adding Fixes #633 to the description.

PIC tradeoffs: Choose one of #631, #637 and #657 for the core fix. #631 and #637 are equivalent. #657 adds broader baseline-writing changes on top (see that review). As noted on #637, active_findings isn't bounded by MAX_FINDING_OUTPUT_RECORDS. A scan with more than 10,000 kept occurrences, or one that produces a baseline over 2 MiB, writes a file that load_baseline rejects (exit 2, fail closed). #657 adds pre-write validation for that case.

Verification and gaps: I read the full diff at d1536da1 (two commits on c7958a3, no merge commits). I traced report() (sanitized selected_findings -> partition_findings -> active_findings), the baseline command and build_baseline_dict -> finding_fingerprint. I compared the change line by line with #637. finding_fingerprint and the v2 schema are unchanged, so existing baselines still load. All 6 CI checks pass. Per policy, I did not run the tests locally.

---

Decision: Approved (reviewed head d1536da1037ea4c35c22d9d4310e6abdbb89f282)

[deepujain]

Closing in favor of #637, which provides the equivalent baseline fix for #630 and #633, as recommended in review. Thank you for reviewing.