[High priority] Duplicate findings re-posted on every new commit (dedup keyed on SHA + LLM wording)

[mountainowl]

Summary

Bubo re-posts the same finding as a new inline thread every time a new commit is pushed to an MR/PR. The dedup that is supposed to prevent "stacking duplicates on rebases or repeated polls" does not work across commits, so an open MR accumulates one duplicate thread per push for every still-present finding.

This is high priority: it is most visible on blocking security findings (e.g. a committed credential), where the duplicate noise erodes trust in the reviewer exactly where it matters most.

Reproduction (observed)

A newly-added gradle.properties contains a committed GitLab token. On the MR:

1. Commit A → Bubo posts: "Issue (blocking, security): private token is committed".
2. Developer pushes commit B (unrelated change elsewhere; gradle.properties untouched).
3. Bubo re-reviews at the new SHA and posts a second thread: "Issue (blocking, security): GitLab token is committed" — same file, same line, same category, different wording.

Note the two bodies differ ("private token" vs "GitLab token") — the LLM re-words the finding on each run.

Root cause

The finding identity is derived from values that both change across a re-review, so both dedup layers miss:

1. DB fingerprint — findings.finding_fingerprint() hashes {project, iid, sha, file, line, body}. It includes the commit sha and the full finding_body() render (which contains the LLM-generated title/impact/evidence/fix). A new commit changes sha; a re-run changes the wording → different hash.
2. DB seen-check — db.finding_seen() is scoped to the same sha (where project=? and iid=? and sha=? and fingerprint=?). By its own docstring it only short-circuits "re-extraction across retried worker runs at the same SHA". There is no cross-SHA dedup for inline findings.
3. Provider fallback — gitlab.find_discussion_by_body() matches the comment body verbatim (note.get("body") == body). Since the wording changes run-to-run, this exact-match never fires either.

Net: there is no SHA-independent, wording-independent identity for a finding, so re-review of the same MR re-posts.

Proposed fix

Add a stable, additive dedup key — do not touch finding_fingerprint (it keys the outcome-tracking and dispute-suppression tables and must stay stable):

• New findings.finding_dedup_key(project, iid, finding) = stable_hash of {project, iid, file, line, category, severity, type}, with the enum fields normalized (strip().lower()) and defaulted the same way finding_body defaults them (issue/blocking/correctness). SHA-independent and body/wording-independent.
• New review_findings.dedup_key column (additive ALTER) populated by record_finding.
• New db.finding_posted_on_mr(project, iid, dedup_key) → checks for an already-POSTED finding with that key across all SHAs of the MR.
• In the worker posting loop, skip-before-post when finding_posted_on_mr(...) is true (in addition to the existing same-SHA finding_seen).

Why line is included (deliberate)

The two failure directions are asymmetric. Dropping line would collapse two distinct blocking-security findings in the same file into one — the second would never post (a silently-hidden real finding). A residual duplicate is annoying and self-evident; a hidden second secret is dangerous. So we favor precision and keep line.

Known limitations (accepted, documented)

• Line shift: if a later commit inserts lines above a finding, its line changes and you get one residual duplicate. This is the safe failure direction. The robust fix (content-anchoring on the flagged line's text) is a larger change — there is no stable content field on the finding today — and is deferred as a follow-up.
• Post-deploy: existing posted rows have dedup_key = NULL, so the first re-review of an already-open MR after this ships may duplicate once, then stabilize.

Not a bug: cross-repo "learning"

Separately observed: the same finding was accepted as "won't fix / approval provided" in another repo, and the expectation was that Bubo would then suppress it here. That is working as designed and should not change:

• Dispute-driven suppression is per-project (where rf.project = ?), off by default (suppress_disputed_classes), needs dispute_suppress_min_samples (≥5) recorded outcomes, and keys on disputes / false-positives — not on a true-positive that one team chose to accept.
• Teaching a security reviewer to stop reporting committed secrets because one team accepted the risk once would be a footgun. Suppression intentionally does not generalize an accepted true-positive across repos.

This issue is scoped to the duplicate bug only.

[mountainowl]

Fixed on branch fix/duplicate-findings-cross-commit — commit f82afee (no MR opened per request).

Adds a SHA- and wording-independent finding_dedup_key (file, line, category, severity, type) plus finding_posted_on_mr, and the poller now skips a finding already live on the MR before posting. Regression test reproduces the exact reported scenario (post at sha-A → re-review at sha-B with a reworded body, same locus → one thread). Full suite green (the 4 test_project_layout README failures are pre-existing on main).

Note: the Fixes #135 trailer will auto-close this only when the branch merges to the default branch; until then it stays open. Known one-time effect: MRs already open when this ships have no dedup_key on their rows, so they may duplicate once on the next re-review, then stabilize.