[MEDIUM] feat(review): group near-duplicate findings under one review unit

[elhoim]

BLUF

• Problem — an automated sweep across N hosts produces the same observation N times. Nothing deduplicates findings, so an examiner reads and decides on every near-identical copy individually.
• Change — vhir review --findings --cluster groups near-duplicate findings into one review unit. On a synthetic 36-finding sweep this is 36 items → 5 clusters; the genuinely distinct findings stay distinct.
• Presentation only — every finding keeps its own id, its own content_hash and its own row in approvals.jsonl. Nothing here reads or writes status. Cluster-level approval is deliberately not included; that is a custody decision for you, not for me.
• Deterministic — template masking plus token overlap. No model, no network, no stored state. The same case clusters identically on every machine, which a probabilistic grouping cannot promise.
• Opt-in — new flag, no existing default changes. --cluster without --findings does nothing.

Priority: MEDIUM

Needs a decision before merge. This is a feature, not a defect fix, and it implements one of four design proposals I wrote up against this repo. It is complete and tested, but you may not want it, or may want it shaped differently. The decision is written up in #60.

The problem

A triage sweep stages findings per host. Thirty hosts with the same scheduled-task persistence produce thirty findings that differ only in hostname, user, PID and timestamp. Today vhir review --findings lists all thirty; the interactive approval queue walks all thirty.

The approach

Each finding's text is reduced to a template by masking the spans that legitimately vary between hosts:

Masked	Placeholder
IPv4	<IP>
SHA-256 / SHA-1 / MD5	<SHA256> / <SHA1> / <MD5>
GUIDs	<GUID>
ISO timestamps, dates, times	<TS> / <DATE> / <TIME>
\Users\<name>\, /home/<name>/	<USER>
Host labels (WIN-4K2J1, SRV-DC01)	<HOST>
Bare integers (PIDs, ports, counts)	<N>

Findings sharing a template are the same observation. A second pass merges templates whose token overlap (Jaccard) reaches 0.85, which catches the wording drift a generative model produces when writing the same finding thirty times — "lsass.exe accessed by rundll32.exe" vs "lsass.exe was accessed by rundll32.exe".

Measured

Synthetic case: one persistence mechanism on 30 hosts, one credential-access finding written three slightly different ways, three genuinely distinct findings.

36 finding(s) in 5 cluster(s).
2 cluster(s) hold near-duplicates: 31 fewer item(s) to read.

[1] Scheduled task persistence  (30 findings)
      F-000                DRAFT      MEDIUM
      ...
[2] Credential access  (3 findings)
      G-000                DRAFT      HIGH
      G-001                DRAFT      HIGH
      G-002                DRAFT      HIGH
[3] Outbound exfiltration
      U-000                DRAFT      HIGH
[4] Firewall rule added
      U-001                DRAFT      HIGH
[5] Log clearing
      U-002                DRAFT      HIGH

Each finding keeps its own id and approval record; this view only groups them.

The three distinct findings did not merge, which is the result that matters — a clustering that over-merges is worse than none.

Why deterministic and not a model

This started as a proposal to ask a model "do these two findings describe the same activity?". Doing it in code turns out to be better here, not merely cheaper:

• Reproducible. Re-running on the same case gives the same clusters on every machine. A derived forensic view that varies by API call is a chain-of-custody problem.
• No network in the review path. Forensic workstations are frequently air-gapped.
• Inspectable. When a merge looks wrong you can print the template and see exactly why.

Complexity

Exact-template grouping is linear. The merge pass compares distinct templates, not findings — on the demo above, 5 comparisons rather than 36², because the 30 duplicates collapse before the pass runs. Above MAX_TEMPLATES_FOR_MERGE = 400 distinct templates the pass is skipped entirely and only exact matches group, so the quadratic term is bounded. (The original proposal flagged pairwise blow-up as this feature's failure mode; this is the answer to it.)

Nothing is persisted

compute_content_hash() hashes every field outside an explicit exclusion set, so writing a cluster id into a finding would change its hash and break vhir review --verify. Clusters are recomputed on read every time. That is cheap here precisely because the computation is deterministic.

Testing

tests/test_clustering.py, 17 tests: masking of each varying span, hashes not nibbled by the integer rule, identical activity across hosts collapsing to one cluster, distinct activity staying separate, wording drift merging, dissimilar text not merging, every finding appearing exactly once, findings left unmutated, deterministic ordering, empty input, and — added after a self-review caught it — that two findings differing only in their CVE or MITRE sub-technique do not merge.

• pytest tests/ — 445 passed on Python 3.10 and 3.12 (428 pre-existing, unchanged; 17 new)
• ruff check . / ruff format --check . — clean

Deliberately not in this PR

• Cluster-level approval. One keystroke approving N findings would write N HMAC-signed records — correct in principle, but it is a change to the custody path's UX and you should decide it, not me.
• Clustering the interactive queue. Same reason: vhir approve is the custody command.
• Host-label masking is a heuristic. (It bit once already: CVE-2024-1234 normalised to <HOST>-<N>, merging findings about different CVEs. Fixed in a follow-up commit on this branch, with regression tests.)
• The remaining heuristic risk. WIN-4K2J1 matches, a host named fileserver does not. This is the most likely source of a wrong merge, which is exactly why clustering only groups and never decides.

---

CI status

The checks will show action_required: GitHub gates workflow runs on PRs from first-time fork contributors until a maintainer approves them. Reproduced locally with the same steps as ci.yml:

Step	Result
pytest tests/ — Python 3.10	445 passed
pytest tests/ — Python 3.12	445 passed
ruff check .	All checks passed
ruff format --check .	clean

Branched from main @ 954a623.

Cherry-pick note. Verified against the two IOC PRs rather than assumed. Cherry-picking this onto #54 or #55 conflicts in exactly one place: the import line in review.py, because all three add an import at the same spot. Resolution is to keep both lines. Nothing else conflicts — with #55 applied on top of this, 470 passed and ruff is clean.

Full-series check. All six PRs in this series (#54, #55, #56, #57, #58, #59) were combined into one tree and gated together, not assumed compatible: 573 passed on Python 3.10 and 3.12, ruff clean, and all three new review --findings views reachable through the merged dispatch. Every conflict between them is additive ("keep both"), concentrated in the import block, the p_review flag list and the view dispatch.

🤖 Generated with Claude Code

[scriptedstatement]

Thanks. We're deferring new review-queue features to the next minor release. One note for when we pick it up: the \b\d+\b normaliser groups findings that differ only in numbers, so Event 4624 and 4625, ports 443 and 4444, and different hashes or IPs end up in one group.