fix(groom): anchor the dedup signature to the finding's file path (BE-4460)#74
fix(groom): anchor the dedup signature to the finding's file path (BE-4460)#74mattmillerai wants to merge 3 commits into
Conversation
…-4460) The verifier derived its "stable" dedup signature from a slug of the finding TITLE, but titles are re-generated by the model on every run — a rewording produced a new slug, the ledger's exact-string match saw a brand-new finding, and the SAME finding was filed as a second issue on the next run. - verifier.md: `<path-slug>` is now the finding's primary file/directory path (lowercased, non-alphanumeric runs collapsed to hyphens), with the most representative path for a multi-file finding and a subject noun-phrase only for a repo-wide pattern with no anchor. The brief states WHY. - ledger.py: path-token backstop in `partition` — a candidate whose signature is unknown but whose path segment is already covered (by a known signature or by an earlier candidate in the same batch) is suppressed as `path-collision`. Exact string equality on the path segment; no fuzzy matching. Classification and the marker round-trip are unchanged. - tests + README: path-collision suppression, no false suppression across different paths, marker round-trip, and the contract-table signature row.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 3 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
…ng (BE-4460) Review-panel follow-ups on the path-anchored dedup signature. A path anchors a LOCATION, not a finding, so the path-collision backstop — which suppresses a candidate whose own signature is new — needed narrowing: - `security: true` candidates are never suppressed by the backstop, and the verifier now prefixes their slug `sec-`. Without both, one already-filed routine finding on `src/tools.ts` silently buried a later security finding on the same file, breaking the "security findings always surface as investigations" guarantee the rest of the pipeline enforces. Exact-signature dedup still applies, so the exemption costs at most one issue per finding. - `superseded` records no longer seed the path index. `groom-superseded` is the documented "retire this issue so its finding can be re-filed" signal; keeping it indexed let the retired issue go on suppressing the replacement by path. - `path_token` splits from the LAST colon, not the second: the slug and repo basename are colon-free by construction, but `scope_label` is a free-form input, and `pkg:api` sheared the scope's tail onto the token. - `path_token` trims leading/trailing hyphens, and verifier.md now says to — its own rule read literally made `services/ingest/` -> `services-ingest-` while its worked example showed `services-ingest`, two tokens for one dir. - verifier.md picks the multi-file anchor mechanically (alphabetically first cited path) instead of "the most representative"/evidence order, both of which are judgments the LLM re-makes differently next run; the repo-wide noun-phrase fallback is tightened and de-prioritized. - `--check` gains `--check-security` so the probe still mirrors `partition`. Tests: 90 pass (was 84).
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 3 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
…g (BE-4460) Second review-panel round, on the previous commit's own fix: - The `sec-` slug prefix was NOT domain-separated: a routine finding about `sec/auth.ts` slugifies to `sec-auth-ts` and collided with the security lane for `auth.ts`, silently suppressing one of the two. The prefix is now `sec_` — an underscore can never come out of slugification (every `_` in a path collapses to a hyphen), so the two keys are provably disjoint. - `is_security_finding` now fails CLOSED (`!= "false"`, the build gate's reading) instead of requiring an explicit `true`. It decides a security guarantee from an LLM-authored field, so an omitted or mangled flag must not be what subjects a genuine security finding to the backstop. The verifier's schema requires the field, so well-formed batches are unaffected; malformed output costs at most one extra issue. - README: the path-collision suppression is PERMANENT, not a one-time transition cost — say so plainly instead of "rare in practice", and state that the `sec_` lane does not make individual security findings addressable (two vulns in one file still collapse, as two routine findings do). - Note in `Ledger.__init__` that `_PRECEDENCE` collapses a signature holding both a superseded and a live record to SUPERSEDED, dropping it from the path index — bounded to one duplicate, since exact-signature dedup still applies. Tests: 92 pass (was 90); suppression cases now pin `security: False` explicitly, which is what a well-formed verifier emits.
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
1 participant
ELI-5
The groom robot files a GitHub issue for each code-cleanup finding, and remembers what it already filed by a "signature" string. That signature used to be built from the finding's title — but the model writes a fresh title every run, so re-wording "split tools.ts into modules" into "tools.ts is a monolith" produced a different signature, the ledger thought it was a brand-new finding, and the same finding got filed a second time. This anchors the signature to the finding's file path instead, because paths don't change when the wording does, and adds a backstop in the ledger that suppresses a candidate whose path is already covered by an issue we filed before.
What changed
.github/groom/verifier.md— the signature is now<repo-basename>:<scope-label>:<path-slug>where<path-slug>is the finding's PRIMARY file or directory path (lowercased, every run of non-alphanumeric characters collapsed to a single hyphen:src/tools.ts→src-tools-ts). Multi-file finding → the most representative path (the one the body's first evidence cites). Only a repo-wide pattern with no single anchor falls back to a normalized subject noun-phrase (not the full title). The brief states explicitly WHY: titles are re-generated every run, paths are stable across rewordings..github/groom/ledger.py— newpath_token()(everything after the second:;""when there is no third segment) plus a path-token backstop inpartition(): a candidate whose exact signature isunknownbut whose path segment is already covered — by a known signature, or by a candidate already routed toto_filein the same batch — is reported undersuppressedwith the distinct statuspath-collision.Ledger.should_file()and the--checkCLI agree withpartition(they reportpath-collisionand exit 1). The signature stays an opaque string for classification — no format change is required to classify, precedence is untouched, and the marker round-trip is unchanged.PathTokenTest+PathCollisionTest(path-collision suppression, exact-signature status not shadowed, no false suppression across different paths incl. same-basename-different-directory, intra-batch collision, rejection surviving a rekey,should_file/partitionagreement) and two acceptance-scenario tests (reworded-next-run files nothing; marker round-trip with a path-format signature). 74 tests pass..github/groom/README.md: the contract-tablesignaturerow now documents the path derivation and why, apath-collisionrow in the ledger-status table, and a new "path-token backstop" section covering the exact-match rule and the transition consequence.Acceptance criteria
partitionsuppresses it withledger_status: filed, visible indecision.json. Covered bytest_same_file_reworded_next_run_is_not_refiled. (The derivation lives in an LLM prompt, so it is as reliable as the brief is followed — hence the backstop below, and the explicit "keep it to the path ALONE" instruction.)split-tools-ts-into-focused-modulesortools-ts-monolithas covering the newsrc-tools-ts— the strings genuinely differ. I deliberately did not add substring/shared-token matching:src-index-tsvslib-index-tswould then collide and a real finding would be silently dropped forever, which is a worse failure than one duplicate. What the backstop does catch: the same path under a different<scope-label>, and a legacy slug that coincides with the path slug. The transition cost is therefore bounded at one extra issue per pre-existing legacy finding, after which it is stable forever; retire the legacy issue withgroom-superseded(or close as not planned). Pinned bytest_legacy_title_slug_embedding_the_path_is_not_matchedand documented in the README.ci-groom.yml/test-groom-scripts.ymlpass —test-groom-scripts.ymlruns the unit suite on this PR (green locally: 74 tests).ci-groom.ymlnever fires on a PR (schedule +workflow_dispatchonly) and is untouched by this diff —groom.yml's interface is unchanged.Judgment calls
pendingstatus exists for exactly that reason.groom.ymluntouched. The job-summary line countssuppressedwithout breaking it out by status, sopath-collisionentries are counted there and fully itemized indecision.json— no workflow change was needed, and keeping the diff inside.github/groom/**avoids spurious consumer SHA bumps.Follow-up (operator, post-merge)
ci-groom.ymlpinsworkflows_refto a merged-main SHA (07154fb), and the briefs +ledger.pyare loaded from that pinned ref at run time — so this fix takes effect for this repo's own groom (and for external consumers) only once that pin is bumped and the floatingv1tag is moved, per the repo's versioning policy.Verification
The
--checkCLI path was exercised directly against a stubbed ledger: a colliding signature printspath-collision/ exit 1, a new oneunknown/ exit 0, an exact matchfiled/ exit 1, a blank oneinvalid/ exit 1.Self-review note on the negative-claim rule: this diff's "denial" is a dedup suppression, not a product-capability dead-end — it adds no unsupported/unavailable path. Its risk is over-suppression, which is bounded by exact string equality and pinned by explicit no-false-suppression tests.