fix(findings): match equivalent findings across standard and deep scans

[IvanCaceres]

Summary

Fixes #669. Standard and Deep scans of the same repository revision can author different identity.anchor values for the same vulnerability. Because findingId derives from the fingerprint over target, rule, anchor, and instance, the comparison then places the two records in separate groups and misreports the pair (for example one resolved/unknown before finding plus one new after finding) instead of one persisting finding.

With this change, when the two compared scans reviewed identical content, findings that share a vulnerability class and identical source locations keep a consistent identification across scan modes, as the issue requests.

Changes

• plugins/codex-security/scripts/workbench_scan_history.py: compare_scans now deterministically groups before/after findings when both scans reviewed identical content (equal target_revision and non-null target_snapshot_digest, non-diff modes) and the findings unambiguously share a ruleId plus an identical primary location (first root_control location, falling back to the first affected location, matching the adapter rule in references/scan-contract.md). Such groups report as persisting with the match reason "The findings share a vulnerability class and identical locations in the same reviewed content."
  • Ambiguous keys (several findings in one scan sharing rule and location, e.g. sibling instances) are skipped and left to semantic matching, per the contract's "treat ambiguous matches as unresolved".
  • Pairs already linked by saved semantic matches or stable identities are skipped, so existing match reasons and outputs are unchanged.
  • The check gates on column presence, so callers with reduced synthetic schemas (as in the SDK probe tests) behave exactly as before.
• plugins/codex-security/tests/test_workbench_scan_history.py: two new tests cover the issue's reproduction (Standard vs Deep scan of an identical synthetic repository now compares as persisting, and a saved uncertain pair cannot split a deterministic identity) and the negative controls (changed content and ambiguous sibling locations still defer to semantic matching). Existing semantic-matching tests that relied on trivially identical fixtures now move the after finding to a distinct location so they keep exercising the semantic path; the linked-worktrees test now demonstrates the deterministic match, and the matching-origins test pins distinct revisions to stay deterministic.
• sdk/typescript/README.md: documents the deterministic grouping in the scan comparison section.

No public CLI surface changes: commands, arguments, and defaults are unchanged; only the comparison classification for identical-content scans changes.

Testing

• python -m pytest tests/test_workbench_scan_history.py in plugins/codex-security: 28 passed (Windows 11, Python 3.13.7).
• Verified the reproduction on unmodified main: the new cross-mode test fails there with {'persisting': 0, 'new': 1, 'resolved': 1}, confirming the misreport described in the issue.
• python -m pytest tests/test_workbench_db.py: 103 passed, 5 failed — the same pre-existing Windows symlink-privilege failures occur on main and are unrelated.
• python -m ruff check and python -m ruff format --check (ruff 0.16.1) over the plugin sources and .github/scripts: clean.
• python .github/scripts/check_plugin_source_compatibility.py: passed.
• Ran the Python probe from sdk/typescript/tests-ts/workbench-scan-history.test.ts ("validates related pairs by confirmed group") directly against the modified module: output matches the test's expectation. I could not run the full Bun-based SDK suite in this environment; scan-matching-e2e.test.ts was reviewed by hand — its multi-finding scans hit the ambiguity guard and its assertions do not cover the affected summaries, and knownFindingGroups inputs are intentionally unchanged.

Risk and rollout

• Behavior change is limited to compare-scans/save-scan-comparison output when both scans have equal recorded revision and snapshot digest; cross-revision comparisons, diff scans, and legacy scans without a snapshot digest are unaffected.
• Deterministic grouping only merges an unambiguous one-to-one rule-and-location pair; it never splits saved semantic matches, and semantic matching inputs (matchingInputs, knownFindingGroups) are unchanged, so the SDK matcher contract is untouched.
• A theoretical over-merge exists if two semantically distinct findings in different scans share a rule family and an identical primary location while each scan reports only one of them; the same-scan sibling case is protected by the ambiguity guard, and the contract already distinguishes siblings by location or instance.
• No schema migration, no sealed-artifact changes, and comparisons remain read-only except through the existing save path.

Public disclosure review

• No customer, partner, prospect, or user identities, data, or identifying details are included.
• No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
• I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

[sylvesterkaczmarek]

_rule_location_identities drops identity_instance, so two different findings are merged when each scan contains exactly one finding with the same rule and primary location but different instances. Include instance compatibility in the deterministic key (or reject differing instances), and add a negative regression with one finding per scan at the same rule/location and different identity_instance values.

[IvanCaceres]

_rule_location_identities drops identity_instance, so two different findings are merged when each scan contains exactly one finding with the same rule and primary location but different instances. Include instance compatibility in the deterministic key (or reject differing instances), and add a negative regression with one finding per scan at the same rule/location and different identity_instance values.

Thank you @sylvesterkaczmarek
Fixed in 3f91b5f. The deterministic key now includes identity_instance. Added the requested negative regression with one finding per scan at the same rule/location but different instances, plus coverage for missing versus present instances and matching instances. All 34 scan-history tests and required checks pass.

[sylvesterkaczmarek]

Re-reviewed the current head. identity_instance is now part of the deterministic rule/location key, so findings at the same rule and primary location no longer collapse across distinct instances. The new regressions cover differing instances, missing versus present instances, and matching instances. My earlier blocker is resolved.

[IvanCaceres]

@mldangelo-oai Hello, I think this PR is ready to take a look

[sylvesterkaczmarek]

Re-reviewed current b3493c9 after the main merges. The PR-specific matching fix remains intact: identity_instance is still part of the deterministic rule/location identity and the instance regressions remain. No blocker from me.

[sylvesterkaczmarek]

Rechecked current e5e9f87f after the latest merge from main. The PR-specific matching change is unchanged: identity_instance remains part of the deterministic rule/location key and the differing/missing/matching instance regressions remain intact. No new blocker from me.

[sylvesterkaczmarek]

Rechecked current 78ccce0d after the latest merge from main. The commits since my previous approval are dashboard changes from main; the scan-history matching change is unchanged, including identity_instance in the deterministic key and its regressions. No new blocker from me.

[IvanCaceres]

@mldangelo-oai I think this PR is ready to take another look

[sylvesterkaczmarek]

Rechecked current e9e66ff0 after the latest merges from main. The PR-specific implementation and scan-history regressions are unchanged from the version I previously approved: deterministic matching still includes identity_instance, and the differing/missing/matching instance coverage remains intact. The README differences since my prior review are upstream documentation additions, not changes to this matching fix. No new blocker from me.

[sylvesterkaczmarek]

Rechecked current aecb6be4 after the latest merge from main. None of the PR-specific files changed since my previous approval: workbench_scan_history.py, its focused regressions, and the TypeScript README are byte-unchanged across e9e66ff0..aecb6be4. The intervening changes are upstream dependency/tooling and Solidity inventory work. The deterministic matching fix still includes identity_instance and its differing/missing/matching instance coverage. No new blocker from me.

[sylvesterkaczmarek]

Rechecked current 3c2a8632 after the latest merge from main. None of the PR-specific files changed since my previous approval: plugins/codex-security/scripts/workbench_scan_history.py, plugins/codex-security/tests/test_workbench_scan_history.py, and sdk/typescript/README.md are absent from the aecb6be4..3c2a8632 change set. The intervening commits are upstream work plus the merge commit. The deterministic matching fix still includes identity_instance and its differing/missing/matching instance regressions. No new blocker from me.