From c3f86b96287677865e8798c06c7383193e925777 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 15:57:23 -0700 Subject: [PATCH 1/3] fix(groom): anchor the dedup signature to the finding's file path (BE-4460) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: `` 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. --- .github/groom/README.md | 35 +++++++- .github/groom/ledger.py | 106 ++++++++++++++++++++-- .github/groom/tests/test_ledger.py | 139 +++++++++++++++++++++++++++++ .github/groom/verifier.md | 6 +- 4 files changed, 276 insertions(+), 10 deletions(-) diff --git a/.github/groom/README.md b/.github/groom/README.md index 22de4ee..36d31bc 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -46,10 +46,18 @@ rather than buried in a runner script. `REJECT` (premature / overstated / not worth it). - **`security: true`** marks any auth/permission/security-adjacent finding — those are filed as investigations, **never** auto-implemented. -- **`signature`** is a stable dedup key (`::`) whose - `` is derived **deterministically** from the finding's core subject, so it - stays identical across re-runs of the same finding and a consumer never re-files - a finding it has already seen. +- **`signature`** is a stable dedup key, `::`, + where `` 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. + It is anchored to the **path, not the title**, because titles are re-generated + on every run: a re-worded title yields a new title-slug, the ledger's + exact-string match sees a "new" finding, and the same finding is filed twice + (observed in a consumer repo: one `src/tools.ts` finding, two issues opened by + consecutive runs). Paths survive rewordings, so the signature stays identical + across re-runs and a consumer never re-files a finding it has already seen. ## How a consumer uses these briefs @@ -119,6 +127,7 @@ Keyed on `(repo, finding_signature) → {filed | rejected | superseded}`: | Open **builder PR** for the signature (BE-4003) | `pr-open` | no | | **Builder PR merged** | `merged` | no (shipped) | | **Builder PR closed unmerged** | `pr-closed` | **no — durable** (human declined) | +| A known signature shares the candidate's `` (BE-4460) | `path-collision` | no | | No `groom` issue or PR carries the signature | `unknown` | **yes** | Only an `unknown` signature is filed/proposed. Human rejection — close-as-not-planned, @@ -140,6 +149,24 @@ issue for a `to_file` finding **must**: Skip either and the next run cannot recognize the issue and will re-file it. +### The path-token backstop (BE-4460) + +Classification treats the signature as an opaque string, with one exception: a +candidate whose exact signature is `unknown` but whose `` segment +(everything after the second `:`) is already covered — by a known signature, or +by a candidate already routed to `to_file` in the same batch — is suppressed as +**`path-collision`** rather than filed. That keeps one issue per anchoring path +when the leading segments differ (a re-scoped run, a legacy signature whose slug +coincides). Matching is **exact string equality** on the path segment — no +substring or fuzzy matching, which would silently drop real findings about +different files that share a basename (`src/index.ts` vs `lib/index.ts`). + +Consequence for the format transition: a legacy *title*-derived slug that merely +*embeds* the path (`split-tools-ts-into-focused-modules`) is **not** matched, so +such a finding can be filed once more under its new path-anchored signature — +then it is stable forever. Label the superseded legacy issue `groom-superseded` +(or close it as not planned) to retire it. + The dedup decision is a point-in-time snapshot of GitHub issue state read *before* filing, and issue creation happens in a later step. Two overlapping groom runs could therefore both classify the same signature as `unknown` and diff --git a/.github/groom/ledger.py b/.github/groom/ledger.py index 902fa5b..e6e366b 100644 --- a/.github/groom/ledger.py +++ b/.github/groom/ledger.py @@ -34,6 +34,19 @@ surrounding whitespace so a stray newline in a marker can't split one signature into two ledger keys. +One structural assumption is made on top of that opacity (BE-4460): the +signature's trailing `` segment — everything after the second `:` in +the verifier's `::` format — names the +finding's primary file/directory, and is therefore stable even when the LLM +re-words the title. `partition` uses it as a **backstop**: a candidate whose +exact signature is new but whose path segment is already covered by a known +signature is suppressed as `path-collision` instead of filed. That is what +carries dedup across the title-slug -> path-slug format change (the incident +that motivated it: one `src/tools.ts` finding filed as two issues by consecutive +runs, because a reworded title produced a new title-derived slug). +Classification is unchanged — the signature stays an opaque, exactly-matched +string everywhere else. + CLI (what the groom workflow calls right before it files): python3 .github/groom/ledger.py \ @@ -123,6 +136,14 @@ # `_PRECEDENCE`. PENDING = "pending" +# Partition-time-only status: the signature itself is UNKNOWN, but its +# `` segment is already covered by a known signature (or by a +# candidate routed to `to_file` earlier in this batch). See `path_token` — this +# is the backstop that carries dedup across the title-slug -> path-slug +# signature format change (BE-4460). Like PENDING it is not a GitHub issue +# state, so it is not in `_PRECEDENCE`. +PATH_COLLISION = "path-collision" + # Precedence when several records (issues and/or builder PRs) share one # signature (e.g. a finding filed as an issue AND later built as a PR): surface # the most decision-bearing status. A human "no" is the stickiest signal, so @@ -282,6 +303,31 @@ def normalize_signature(signature) -> str: return signature.strip() +# The verifier's signature format is `::` +# (verifier.md). Only the trailing `` segment — everything after the +# SECOND colon — is the finding's anchor; the first two segments are run +# metadata. `maxsplit=2` keeps a colon inside the path segment intact. +_SIGNATURE_SEGMENTS = 3 + + +def path_token(signature) -> str: + """The `` segment of a signature, or "" if it has no third segment. + + The path segment is what makes a signature stable: the verifier now derives + it from the finding's PRIMARY file/directory path, which survives the LLM + re-wording the title on every run (BE-4460 — the same `src/tools.ts` finding + was filed twice because its title-derived slug changed between runs). + + Lowercased for comparison only. The verifier emits lowercase slugs already, + so this only guards against a stray capital; the full signature stays + case-sensitive and opaque everywhere else (`normalize_signature`). + """ + parts = normalize_signature(signature).split(":", _SIGNATURE_SEGMENTS - 1) + if len(parts) < _SIGNATURE_SEGMENTS: + return "" + return parts[-1].strip().lower() + + def extract_signature(body): """Recover the embedded signature from an issue body, or None. @@ -390,6 +436,9 @@ class Ledger: def __init__(self, statuses: dict): self._statuses = dict(statuses) + # Path-slug index over the SAME records, for the backstop below. Built + # once here rather than scanned per candidate. + self._known_paths = {t for t in map(path_token, self._statuses) if t} def __len__(self) -> int: return len(self._statuses) @@ -401,15 +450,39 @@ def status(self, signature) -> str: def is_known(self, signature) -> bool: return self.status(signature) != UNKNOWN + def path_collides(self, signature) -> bool: + """True if some KNOWN signature already covers this signature's path-slug. + + The backstop for the title-slug -> path-slug signature format change + (BE-4460). Exact-string dedup on the WHOLE signature misses two cases + that are the same finding: an issue filed for the same path under a + different `` (a re-scoped or narrowed run), and a legacy + issue whose title-derived slug happens to equal the new path-slug. + Comparing only the trailing path segment catches both. + + Deterministic and exact — string equality on the path segment, no fuzzy + or substring matching, so it can only suppress a candidate that names + the identical path. A signature with no third segment (a malformed or + pre-format signature) has no path token and never collides. + """ + token = path_token(signature) + return bool(token) and token in self._known_paths + def should_file(self, signature) -> bool: """A finding is filed only if its signature is genuinely new. A missing/blank/non-string signature is NOT filable: it has no recoverable marker, so filing it would re-file on every subsequent run. - This mirrors `partition`, which routes such a candidate to `invalid` - rather than `to_file`. + A signature whose path-slug is already covered by a known signature is + not filable either (`path_collides`). Both mirror `partition`, which + routes such candidates to `invalid` / `path-collision` rather than + `to_file`. """ - return normalize_signature(signature) != "" and self.status(signature) == UNKNOWN + return ( + normalize_signature(signature) != "" + and self.status(signature) == UNKNOWN + and not self.path_collides(signature) + ) def partition(self, findings): """Split candidate findings into to_file / suppressed / invalid. @@ -417,9 +490,10 @@ def partition(self, findings): `to_file` — signature is UNKNOWN *and* first-seen in this batch: open an issue (remember to embed the marker + apply the `groom` label). - `suppressed` — signature is known (filed/rejected/superseded) OR was + `suppressed` — signature is known (filed/rejected/superseded), was already routed to `to_file` earlier in this same batch - (`pending`): each annotated with `ledger_status` for + (`pending`), or its `` is already covered + (`path-collision`): each annotated with `ledger_status` for auditable reporting. `invalid` — no usable signature: cannot be deduped, so it is NOT filed (filing it would risk the exact duplicate-spam this ledger @@ -430,21 +504,37 @@ def partition(self, findings): both be filed — the ledger is only refreshed from GitHub between runs, so the first opens the issue and later duplicates are suppressed as `pending` (not falsely labeled `filed`, which they are not yet). + + Path backstop (BE-4460): a candidate whose signature is UNKNOWN but + whose `` is already covered — by a known signature, or by a + candidate routed to `to_file` earlier in this batch — is suppressed as + `path-collision`. That is what keeps a re-keyed signature from opening a + SECOND issue for a file that already has one, both across the format + change and across scope labels. """ to_file, suppressed, invalid = [], [], [] filed_this_batch = set() + # Path tokens routed to `to_file` in THIS batch, checked alongside the + # ledger's own: within one run GitHub state is not yet refreshed, so + # two candidates anchored to the same path would otherwise both file. + paths_this_batch = set() for finding in findings: signature = normalize_signature(finding.get("signature")) if isinstance(finding, dict) else "" if not signature: invalid.append(finding) continue status = self.status(signature) + token = path_token(signature) if status != UNKNOWN: suppressed.append({**finding, "ledger_status": status}) elif signature in filed_this_batch: suppressed.append({**finding, "ledger_status": PENDING}) + elif token and (self.path_collides(signature) or token in paths_this_batch): + suppressed.append({**finding, "ledger_status": PATH_COLLISION}) else: filed_this_batch.add(signature) + if token: + paths_this_batch.add(token) to_file.append(finding) return to_file, suppressed, invalid @@ -555,6 +645,12 @@ def main(argv=None): print("invalid") return 1 status = ledger.status(args.check) + # Report the path backstop distinctly, exactly as `partition` does, so + # the probe never says `unknown`/exit 0 for a signature partition would + # suppress. + if status == UNKNOWN and ledger.path_collides(args.check): + print(PATH_COLLISION) + return 1 print(status) return 0 if ledger.should_file(args.check) else 1 diff --git a/.github/groom/tests/test_ledger.py b/.github/groom/tests/test_ledger.py index d300db3..6c7f191 100644 --- a/.github/groom/tests/test_ledger.py +++ b/.github/groom/tests/test_ledger.py @@ -297,6 +297,121 @@ def test_partition_dedups_within_batch(self): self.assertEqual(invalid, []) +class PathTokenTest(unittest.TestCase): + """The `` segment extraction the backstop keys on (BE-4460).""" + + def test_extracts_segment_after_second_colon(self): + self.assertEqual(ledger.path_token("cloud:whole-repo:src-tools-ts"), "src-tools-ts") + self.assertEqual(ledger.path_token("repo:services-ingest:services-ingest-main-go"), + "services-ingest-main-go") + + def test_extra_colons_stay_in_the_path_segment(self): + # Only the first two colons are structural; anything after belongs to + # the path segment (maxsplit=2), so a colon in the slug can't shear it. + self.assertEqual(ledger.path_token("cloud:whole-repo:a:b"), "a:b") + + def test_no_third_segment_has_no_token(self): + # A malformed / pre-format signature has no path anchor and must never + # collide with anything (else it would suppress unrelated findings). + self.assertEqual(ledger.path_token("cloud:whole-repo"), "") + self.assertEqual(ledger.path_token("just-a-slug"), "") + self.assertEqual(ledger.path_token(""), "") + self.assertEqual(ledger.path_token(None), "") + self.assertEqual(ledger.path_token(123), "") + + def test_token_is_trimmed_and_lowercased_for_comparison(self): + self.assertEqual(ledger.path_token(" cloud:whole-repo: SRC-Tools-TS \n"), "src-tools-ts") + + def test_empty_third_segment_has_no_token(self): + self.assertEqual(ledger.path_token("cloud:whole-repo:"), "") + self.assertEqual(ledger.path_token("cloud:whole-repo: "), "") + + +class PathCollisionTest(unittest.TestCase): + """The path-token backstop: one issue per anchoring path, not per wording. + + The incident this guards: the SAME `src/tools.ts` finding was filed twice + because its signature was a slug of the LLM-generated title, which was + re-worded between runs. Signatures are now path-anchored, and this backstop + suppresses a re-keyed candidate whose path is already covered. + """ + + def setUp(self): + # A known issue for `src/tools.ts`, filed under one scope label. + self.led = ledger.Ledger({"repo:whole-repo:src-tools-ts": ledger.FILED}) + + def test_same_path_under_a_different_signature_is_suppressed(self): + # Same path, different leading segments (a legacy signature, a re-scoped + # run) => exact-string dedup misses it; the path backstop catches it. + to_file, suppressed, invalid = self.led.partition( + [{"signature": "repo:src:src-tools-ts", "title": "tools.ts is a monolith"}] + ) + self.assertEqual(to_file, []) + self.assertEqual(suppressed[0]["ledger_status"], ledger.PATH_COLLISION) + self.assertEqual(invalid, []) + + def test_exact_signature_match_still_reports_its_ledger_status(self): + # The backstop must not shadow the real status: an identical signature + # is `filed`, not `path-collision`. + _, suppressed, _ = self.led.partition([{"signature": "repo:whole-repo:src-tools-ts"}]) + self.assertEqual(suppressed[0]["ledger_status"], ledger.FILED) + + def test_different_path_is_not_suppressed(self): + # No false suppression: a finding about another file still files, even + # when the paths share tokens (`tools-ts`) — matching is exact, never + # substring/fuzzy. + to_file, suppressed, _ = self.led.partition([ + {"signature": "repo:whole-repo:src-server-ts", "title": "other file"}, + {"signature": "repo:whole-repo:test-tools-ts", "title": "same basename, other dir"}, + ]) + self.assertEqual([f["title"] for f in to_file], ["other file", "same basename, other dir"]) + self.assertEqual(suppressed, []) + + def test_legacy_title_slug_embedding_the_path_is_not_matched(self): + # Documented limit (BE-4460): matching is EXACT on the path segment, so + # a legacy TITLE-derived slug that merely *embeds* the path + # (`split-tools-ts-into-modules`) does not block the new path-anchored + # candidate — substring matching would silently drop real findings whose + # files share a basename. Cost is bounded: at most one duplicate per + # finding during the format transition, then it is stable forever. + led = ledger.Ledger({"repo:whole-repo:split-tools-ts-into-focused-modules": ledger.FILED}) + to_file, _, _ = led.partition([{"signature": "repo:whole-repo:src-tools-ts"}]) + self.assertEqual(len(to_file), 1) + + def test_signature_without_a_path_segment_never_collides(self): + led = ledger.Ledger({"legacy-flat-signature": ledger.FILED}) + to_file, _, _ = led.partition([{"signature": "another-flat-signature"}]) + self.assertEqual(len(to_file), 1) + + def test_intra_batch_path_collision_suppresses_the_second(self): + # Within ONE run GitHub state is not refreshed, so two candidates + # anchored to the same path must not open two issues either. + led = ledger.Ledger({}) + to_file, suppressed, _ = led.partition([ + {"signature": "repo:whole-repo:src-tools-ts", "title": "first"}, + {"signature": "repo:src:src-tools-ts", "title": "second, re-keyed"}, + ]) + self.assertEqual([f["title"] for f in to_file], ["first"]) + self.assertEqual([(f["title"], f["ledger_status"]) for f in suppressed], + [("second, re-keyed", ledger.PATH_COLLISION)]) + + def test_rejected_path_stays_suppressed_across_a_rewording(self): + # A human rejection must survive the verifier re-keying the finding. + led = ledger.Ledger({"repo:whole-repo:src-tools-ts": ledger.REJECTED}) + to_file, suppressed, _ = led.partition([{"signature": "repo:whole-repo-v2:src-tools-ts"}]) + self.assertEqual(to_file, []) + self.assertEqual(suppressed[0]["ledger_status"], ledger.PATH_COLLISION) + + def test_should_file_agrees_with_partition(self): + # The single-signature probe (`--check`) must not say "file it" for a + # candidate `partition` suppresses. + self.assertFalse(self.led.should_file("repo:src:src-tools-ts")) + self.assertTrue(self.led.path_collides("repo:src:src-tools-ts")) + self.assertTrue(self.led.should_file("repo:whole-repo:src-server-ts")) + self.assertFalse(self.led.path_collides("repo:whole-repo:src-server-ts")) + self.assertFalse(self.led.path_collides("no-path-segment")) + + class AcceptanceScenarioTest(unittest.TestCase): """End-to-end run N -> run N+1 using the ledger built from prior issues.""" @@ -320,6 +435,30 @@ def test_new_finding_still_files(self): to_file, _, _ = led.partition([{"signature": "z-new"}]) self.assertEqual(len(to_file), 1) + # --- Path-anchored signatures (BE-4460) --- + + def test_same_file_reworded_next_run_is_not_refiled(self): + # THE acceptance case. Run N filed the `src/tools.ts` monolith finding; + # run N+1 describes the same file with different wording. Because the + # signature is anchored to the PATH, not the title, run N+1 produces the + # identical signature and is suppressed as already `filed` — one issue, + # not two. + sig = "repo:whole-repo:src-tools-ts" + led = ledger.Ledger(ledger.build_ledger([issue(sig, state="open")])) + to_file, suppressed, _ = led.partition( + [{"signature": sig, "title": "src/tools.ts has grown into a monolith"}] + ) + self.assertEqual(to_file, []) + self.assertEqual(suppressed[0]["ledger_status"], ledger.FILED) + + def test_path_format_signature_marker_round_trip(self): + # Marker round-trip is unchanged by the format: the signature is still + # embedded and recovered verbatim (it stays an opaque string). + sig = "repo:whole-repo:src-tools-ts" + recovered = ledger.extract_signature(issue(sig)["body"]) + self.assertEqual(recovered, sig) + self.assertEqual(ledger.path_token(recovered), "src-tools-ts") + # --- Builder auto-PR dedup (BE-4003, acceptance criterion 3) --- # Builder PRs carry the bot-applied `groom-pr` label — that's what admits diff --git a/.github/groom/verifier.md b/.github/groom/verifier.md index 5132e77..71a7841 100644 --- a/.github/groom/verifier.md +++ b/.github/groom/verifier.md @@ -3,5 +3,9 @@ You are a one-shot agent-work GROOM VERIFIER on the Mac Studio — phase 2, the For each finding: (a) is the evidence accurate and are the counts/scope honest? Probe for OVERSTATEMENT. (b) Would the abstraction couple things that should stay separate, or leak across a security boundary? (c) Real value vs churn? Then assign verdict CONFIRM | DOWNGRADE (real but narrow the scope — say how) | REJECT (premature/overstated/not worth it). Set "security":true on ANY auth/security-adjacent finding — those get filed as investigations, NEVER auto-implemented. Write VALID JSON to {{VERIFIER_OUT}}, EXACTLY this shape (JSON ONLY, no prose) — escape all string contents (quotes, backslashes, newlines), especially the multi-line `body`: -{"repo":"{{REPO}}","scope":"{{SCOPE_LABEL}}","summary":"","findings":[{"title":"","verdict":"CONFIRM|DOWNGRADE|REJECT","security":,"signature":"; derive DETERMINISTICALLY from the finding's core subject (normalized title — lowercase, alphanumerics, runs of other chars collapsed to a single hyphen) so the SAME finding always yields the SAME signature across runs and the loop never re-files it>","body":""}]} +{"repo":"{{REPO}}","scope":"{{SCOPE_LABEL}}","summary":"","findings":[{"title":"","verdict":"CONFIRM|DOWNGRADE|REJECT","security":,"signature":" — see the SIGNATURE rules below>","body":""}]} + +SIGNATURE — anchor it to the FILE PATH, never to the title. `` is the finding's PRIMARY file or directory path: the single file/dir the finding is most about, slugified by lowercasing it and collapsing every run of non-alphanumeric characters (`/`, `.`, `_`, spaces, …) to a single hyphen — e.g. `src/tools.ts` -> `src-tools-ts`, `services/ingest/` -> `services-ingest`. For a MULTI-FILE finding use the most representative single path — the one the FIRST evidence item in your `body` cites. Only for a repo-wide pattern with NO single anchoring path, fall back to a normalized SUBJECT NOUN-PHRASE (the thing the finding is about, e.g. `error-wrapping` — NOT the full title, and never a verb-phrase or a fix description). Derive it DETERMINISTICALLY: the SAME finding must yield the SAME signature on every run. + +WHY the path and not the title: your titles are re-generated from scratch on every run, so re-wording the same finding ("split tools.ts into modules" -> "tools.ts is a monolith") produces a different title-slug, the exact-string dedup ledger sees a brand-new signature, and the SAME finding is filed as a SECOND issue (this happened — the identical `src/tools.ts` monolith was filed twice in consecutive runs). File and directory paths do not change when the wording does, so a path-anchored signature is stable across re-runs. Keep `` to the path ALONE — do not append the dimension, the fix, or any adjective, or you reintroduce the instability. When {{VERIFIER_OUT}} is written, you may stop. From 5da5ba74d9d43f44ee62b876dc3dabceedd4f493 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 16:20:44 -0700 Subject: [PATCH 2/3] fix(groom): scope the path backstop so it can't bury a security finding (BE-4460) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .github/groom/README.md | 63 +++++++++-- .github/groom/ledger.py | 126 +++++++++++++++++---- .github/groom/tests/test_ledger.py | 174 ++++++++++++++++++++++++++++- .github/groom/verifier.md | 11 +- 4 files changed, 335 insertions(+), 39 deletions(-) diff --git a/.github/groom/README.md b/.github/groom/README.md index 36d31bc..bb7e2d1 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -49,9 +49,14 @@ rather than buried in a runner script. - **`signature`** is a stable dedup key, `::`, where `` 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. + hyphen, leading/trailing hyphens trimmed (`src/tools.ts` → `src-tools-ts`, + `services/ingest/` → `services-ingest`). Multi-file finding: the + **alphabetically first** of the cited paths — a mechanical rule, because "the + most representative one" is a judgment the verifier would re-make differently + next run. Only a repo-wide pattern with no single anchor falls back to a + normalized subject noun-phrase. A `security: true` finding's slug is prefixed + `sec-`, so a routine finding already filed for a file can never dedup away a + security finding about that same file. It is anchored to the **path, not the title**, because titles are re-generated on every run: a re-worded title yields a new title-slug, the ledger's exact-string match sees a "new" finding, and the same finding is filed twice @@ -127,7 +132,7 @@ Keyed on `(repo, finding_signature) → {filed | rejected | superseded}`: | Open **builder PR** for the signature (BE-4003) | `pr-open` | no | | **Builder PR merged** | `merged` | no (shipped) | | **Builder PR closed unmerged** | `pr-closed` | **no — durable** (human declined) | -| A known signature shares the candidate's `` (BE-4460) | `path-collision` | no | +| A known, non-`superseded` signature shares the candidate's ``, and the candidate is not `security: true` (BE-4460) | `path-collision` | no | | No `groom` issue or PR carries the signature | `unknown` | **yes** | Only an `unknown` signature is filed/proposed. Human rejection — close-as-not-planned, @@ -153,19 +158,47 @@ Skip either and the next run cannot recognize the issue and will re-file it. Classification treats the signature as an opaque string, with one exception: a candidate whose exact signature is `unknown` but whose `` segment -(everything after the second `:`) is already covered — by a known signature, or -by a candidate already routed to `to_file` in the same batch — is suppressed as -**`path-collision`** rather than filed. That keeps one issue per anchoring path -when the leading segments differ (a re-scoped run, a legacy signature whose slug -coincides). Matching is **exact string equality** on the path segment — no -substring or fuzzy matching, which would silently drop real findings about -different files that share a basename (`src/index.ts` vs `lib/index.ts`). +(everything after the **last** `:` — `` and `` are +colon-free by construction, so counting from the right is what keeps a scope +label that itself contains a colon, `pkg:api`, from shearing the token) is +already covered — by a known signature, or by a candidate already routed to +`to_file` in the same batch — is suppressed as **`path-collision`** rather than +filed. That keeps one issue per anchoring path when the leading segments differ +(a re-scoped run, a legacy signature whose slug coincides). Matching is **exact +string equality** on the path segment — no substring or fuzzy matching, which +would silently drop real findings about different files that share a basename +(`src/index.ts` vs `lib/index.ts`). + +Because the backstop suppresses a candidate whose *own* signature is new, it is +deliberately narrow — two carve-outs: + +- **`security: true` candidates are never suppressed by it.** A path anchors a + location, not a finding, so without this an already-filed routine finding on + `src/tools.ts` would bury a later security finding on the same file and break + the "security findings always surface as investigations" guarantee. (The + verifier's `sec-` slug prefix separates the two lanes up front; this is the + code-side guarantee for legacy and cross-scope signatures that predate it.) + Exact-signature dedup still applies, so the exemption costs at most one issue + per security finding — the next run sees it as `filed`. +- **`superseded` records are left out of the path index.** `groom-superseded` is + the documented "retire this issue so its finding can be re-filed under the + current format" signal; keeping it in the index would let the retired issue go + on suppressing the replacement by path and defeat the label a human applied. + +Known, accepted limit: slugification is lossy, so two genuinely distinct paths +can map to one slug (`src/foo/bar.ts` and `src/foo-bar.ts` both → +`src-foo-bar-ts`) and the second finding is suppressed. The blast radius is one +finding on a path shape that is rare in practice, and it is the same collapse the +format already accepts for two distinct findings about one file — the deliberate +trade for a key that is stable across re-wordings. Consequence for the format transition: a legacy *title*-derived slug that merely *embeds* the path (`split-tools-ts-into-focused-modules`) is **not** matched, so such a finding can be filed once more under its new path-anchored signature — then it is stable forever. Label the superseded legacy issue `groom-superseded` -(or close it as not planned) to retire it. +(or close it as not planned) to retire it. A security finding already filed under +an unprefixed slug re-files once for the same reason when it picks up its `sec-` +prefix — same one-per-finding, one-time transition cost, same fix. The dedup decision is a point-in-time snapshot of GitHub issue state read *before* filing, and issue creation happens in a later step. Two overlapping @@ -191,8 +224,14 @@ Single-signature probe (exit 0 = should file, 1 = suppressed): ```bash python3 .github/groom/ledger.py --repo owner/name --check "" +# ...as a security finding, which the path backstop never suppresses: +python3 .github/groom/ledger.py --repo owner/name --check "" --check-security ``` +A bare signature carries no `security` flag, so the probe answers for a routine +finding by default; pass `--check-security` to mirror `partition`'s decision for +a `security: true` candidate. + - **`tests/`** — `unittest` suite, run by [`test-groom-scripts.yml`](../workflows/test-groom-scripts.yml). diff --git a/.github/groom/ledger.py b/.github/groom/ledger.py index e6e366b..83ae226 100644 --- a/.github/groom/ledger.py +++ b/.github/groom/ledger.py @@ -47,6 +47,13 @@ Classification is unchanged — the signature stays an opaque, exactly-matched string everywhere else. +Because that backstop suppresses a candidate whose own signature is new, it is +scoped tightly: it never applies to a `security: true` candidate (a path anchors +a location, not a finding, so an already-filed routine finding on a file must +not bury a later security finding on it), and a `superseded` record — the +documented "retire this issue so the finding can be re-filed" signal — is kept +out of the path index entirely. + CLI (what the groom workflow calls right before it files): python3 .github/groom/ledger.py \ @@ -304,9 +311,9 @@ def normalize_signature(signature) -> str: # The verifier's signature format is `::` -# (verifier.md). Only the trailing `` segment — everything after the -# SECOND colon — is the finding's anchor; the first two segments are run -# metadata. `maxsplit=2` keeps a colon inside the path segment intact. +# (verifier.md). Only the trailing `` segment is the finding's +# anchor; the leading segments are run metadata. A signature needs at least +# this many segments to carry a path at all. _SIGNATURE_SEGMENTS = 3 @@ -318,14 +325,44 @@ def path_token(signature) -> str: re-wording the title on every run (BE-4460 — the same `src/tools.ts` finding was filed twice because its title-derived slug changed between runs). - Lowercased for comparison only. The verifier emits lowercase slugs already, - so this only guards against a stray capital; the full signature stays - case-sensitive and opaque everywhere else (`normalize_signature`). + Split from the LAST colon, not the second: `` and + `` are both colon-free by construction (the slug collapses every + non-alphanumeric run to a hyphen), but `` is a free-form + workflow input that may itself contain a colon (`pkg:api`). Counting from + the left would then shear the scope's tail onto the token (`api:src-tools-ts`) + and break cross-scope matching; counting from the right cannot. + + Lowercased and hyphen-trimmed for comparison only. The verifier emits + lowercase slugs already, so the case fold only guards against a stray + capital; the hyphen trim reconciles the two spellings its own slug rule can + produce for one directory (`services/ingest/` -> `services-ingest-` under a + literal reading, `services-ingest` in the worked example). The full + signature stays case-sensitive and opaque everywhere else + (`normalize_signature`). """ - parts = normalize_signature(signature).split(":", _SIGNATURE_SEGMENTS - 1) - if len(parts) < _SIGNATURE_SEGMENTS: + normalized = normalize_signature(signature) + if normalized.count(":") < _SIGNATURE_SEGMENTS - 1: return "" - return parts[-1].strip().lower() + return normalized.rsplit(":", 1)[-1].strip().lower().strip("-") + + +def is_security_finding(finding) -> bool: + """True only if the candidate is EXPLICITLY marked `security: true`. + + Same truthiness rule the filing step uses to apply the `groom-security` + label (`str(...).strip().lower() == "true"` in groom.yml), so "the finding + exempt from the path backstop" and "the finding labeled security" are the + same set — never one without the other. + + Deliberately NOT the conservative `!= "false"` reading used by the *build* + gate. There, treating an unmarked finding as security is the safe direction + (it stays an issue instead of being auto-implemented). Here the exemption + only ever *adds* filings, so `!= "false"` would exempt every candidate that + simply omits the field and disable the backstop entirely. + """ + if not isinstance(finding, dict): + return False + return str(finding.get("security")).strip().lower() == "true" def extract_signature(body): @@ -436,9 +473,25 @@ class Ledger: def __init__(self, statuses: dict): self._statuses = dict(statuses) - # Path-slug index over the SAME records, for the backstop below. Built + # Path-slug index over the same records, for the backstop below. Built # once here rather than scanned per candidate. - self._known_paths = {t for t in map(path_token, self._statuses) if t} + # + # SUPERSEDED records are excluded on purpose. `groom-superseded` is the + # documented escape hatch for retiring a stale issue so its finding can + # be re-filed under the current signature format; leaving it in the path + # index would let the retired issue keep suppressing the replacement by + # path and defeat the very label the human applied. Every other status + # (including the durable human "no" of REJECTED / PR_CLOSED) still seeds + # the index. + self._known_paths = { + token + for token in ( + path_token(signature) + for signature, status in self._statuses.items() + if status != SUPERSEDED + ) + if token + } def __len__(self) -> int: return len(self._statuses) @@ -463,25 +516,36 @@ def path_collides(self, signature) -> bool: Deterministic and exact — string equality on the path segment, no fuzzy or substring matching, so it can only suppress a candidate that names the identical path. A signature with no third segment (a malformed or - pre-format signature) has no path token and never collides. + pre-format signature) has no path token and never collides, and a + SUPERSEDED record is not in the index at all (see `__init__`). + + This is a heuristic — it deliberately suppresses a candidate whose exact + signature is new — so `partition` never applies it to a `security: true` + candidate. Callers working from a whole finding should go through + `partition` or pass `security=` to `should_file`. """ token = path_token(signature) return bool(token) and token in self._known_paths - def should_file(self, signature) -> bool: + def should_file(self, signature, *, security: bool = False) -> bool: """A finding is filed only if its signature is genuinely new. A missing/blank/non-string signature is NOT filable: it has no recoverable marker, so filing it would re-file on every subsequent run. A signature whose path-slug is already covered by a known signature is - not filable either (`path_collides`). Both mirror `partition`, which - routes such candidates to `invalid` / `path-collision` rather than - `to_file`. + not filable either (`path_collides`) — unless the candidate is a + security finding, which the path backstop never suppresses. All three + mirror `partition`, which routes such candidates to `invalid` / + `path-collision` rather than `to_file`. + + `security` defaults to False because a bare signature carries no + security flag; pass the finding's own flag (`is_security_finding`) to + match `partition` exactly. """ return ( normalize_signature(signature) != "" and self.status(signature) == UNKNOWN - and not self.path_collides(signature) + and (security or not self.path_collides(signature)) ) def partition(self, findings): @@ -511,6 +575,16 @@ def partition(self, findings): `path-collision`. That is what keeps a re-keyed signature from opening a SECOND issue for a file that already has one, both across the format change and across scope labels. + + The backstop NEVER suppresses a `security: true` candidate. It is a + heuristic on a *different* signature, and a path anchors a location, not + a finding: one routine already-filed finding on `src/tools.ts` must not + be able to bury a later security finding on the same file, which would + break the "security findings are always surfaced as investigations" + guarantee the rest of the pipeline enforces. Exact-signature dedup still + applies to security candidates, so the exemption costs at most one issue + per security finding — the next run sees that issue and suppresses it as + `filed`. """ to_file, suppressed, invalid = [], [], [] filed_this_batch = set() @@ -525,11 +599,12 @@ def partition(self, findings): continue status = self.status(signature) token = path_token(signature) + security = is_security_finding(finding) if status != UNKNOWN: suppressed.append({**finding, "ledger_status": status}) elif signature in filed_this_batch: suppressed.append({**finding, "ledger_status": PENDING}) - elif token and (self.path_collides(signature) or token in paths_this_batch): + elif token and not security and (self.path_collides(signature) or token in paths_this_batch): suppressed.append({**finding, "ledger_status": PATH_COLLISION}) else: filed_this_batch.add(signature) @@ -630,6 +705,15 @@ def main(argv=None): metavar="SIGNATURE", help="Print the ledger status of one signature and exit 0 if it should be filed, 1 if suppressed.", ) + parser.add_argument( + "--check-security", + action="store_true", + help=( + "Probe --check as a `security: true` finding, which the path-collision " + "backstop never suppresses. Without it the probe answers for a routine " + "finding and may report `path-collision` where `partition` would file." + ), + ) args = parser.parse_args(argv) if args.check is None and not args.candidates: @@ -647,12 +731,12 @@ def main(argv=None): status = ledger.status(args.check) # Report the path backstop distinctly, exactly as `partition` does, so # the probe never says `unknown`/exit 0 for a signature partition would - # suppress. - if status == UNKNOWN and ledger.path_collides(args.check): + # suppress. `--check-security` mirrors partition's exemption. + if status == UNKNOWN and not args.check_security and ledger.path_collides(args.check): print(PATH_COLLISION) return 1 print(status) - return 0 if ledger.should_file(args.check) else 1 + return 0 if ledger.should_file(args.check, security=args.check_security) else 1 with open(args.candidates, encoding="utf-8") as f: findings = json.load(f) diff --git a/.github/groom/tests/test_ledger.py b/.github/groom/tests/test_ledger.py index 6c7f191..4ac76a5 100644 --- a/.github/groom/tests/test_ledger.py +++ b/.github/groom/tests/test_ledger.py @@ -305,10 +305,28 @@ def test_extracts_segment_after_second_colon(self): self.assertEqual(ledger.path_token("repo:services-ingest:services-ingest-main-go"), "services-ingest-main-go") - def test_extra_colons_stay_in_the_path_segment(self): - # Only the first two colons are structural; anything after belongs to - # the path segment (maxsplit=2), so a colon in the slug can't shear it. - self.assertEqual(ledger.path_token("cloud:whole-repo:a:b"), "a:b") + def test_a_colon_in_the_scope_label_does_not_shear_the_token(self): + # `` is a free-form workflow input and may contain a colon; + # `` and `` cannot (the slug collapses every + # non-alphanumeric run to a hyphen). So the token is what follows the + # LAST colon — counting from the left would yield `api:src-tools-ts` + # here and break cross-scope matching for the same path. + self.assertEqual(ledger.path_token("cloud:pkg:api:src-tools-ts"), "src-tools-ts") + self.assertEqual( + ledger.path_token("cloud:pkg:api:src-tools-ts"), + ledger.path_token("cloud:whole-repo:src-tools-ts"), + ) + + def test_leading_and_trailing_hyphens_are_trimmed(self): + # The verifier's slug rule read literally turns `services/ingest/` into + # `services-ingest-`, while its worked example shows `services-ingest`. + # Trimming reconciles the two so one directory has one token. + self.assertEqual(ledger.path_token("repo:scope:services-ingest-"), "services-ingest") + self.assertEqual( + ledger.path_token("repo:scope:services-ingest-"), + ledger.path_token("repo:scope:services-ingest"), + ) + self.assertEqual(ledger.path_token("repo:scope:---"), "") def test_no_third_segment_has_no_token(self): # A malformed / pre-format signature has no path anchor and must never @@ -411,6 +429,108 @@ def test_should_file_agrees_with_partition(self): self.assertFalse(self.led.path_collides("repo:whole-repo:src-server-ts")) self.assertFalse(self.led.path_collides("no-path-segment")) + def test_superseded_record_does_not_seed_the_path_index(self): + # `groom-superseded` is the documented "retire this issue so the finding + # can be re-filed under the current signature format" signal. If the + # retired issue stayed in the path index it would keep suppressing the + # replacement by path and defeat the label the human applied. + led = ledger.Ledger({"repo:whole-repo:src-tools-ts": ledger.SUPERSEDED}) + to_file, suppressed, _ = led.partition([{"signature": "repo:src:src-tools-ts"}]) + self.assertEqual(len(to_file), 1) + self.assertEqual(suppressed, []) + + def test_superseded_exact_signature_is_still_suppressed(self): + # Only the PATH index drops superseded records; exact-signature dedup is + # untouched, so the retired issue is not re-filed under its own key. + led = ledger.Ledger({"repo:whole-repo:src-tools-ts": ledger.SUPERSEDED}) + to_file, suppressed, _ = led.partition([{"signature": "repo:whole-repo:src-tools-ts"}]) + self.assertEqual(to_file, []) + self.assertEqual(suppressed[0]["ledger_status"], ledger.SUPERSEDED) + + +class SecurityExemptionTest(unittest.TestCase): + """A path anchors a LOCATION, not a finding, so the backstop must never bury + a security finding behind an already-filed routine finding on the same file. + + The rest of the pipeline guarantees security findings always surface as + investigations (groom.yml applies `groom-security` and refuses to + auto-implement them); a heuristic that suppresses a candidate whose own + signature is NEW must not be able to break that. + """ + + def setUp(self): + # A routine finding already filed for `src/tools.ts`. + self.led = ledger.Ledger({"repo:whole-repo:src-tools-ts": ledger.FILED}) + + def test_security_candidate_is_not_suppressed_by_path_collision(self): + to_file, suppressed, _ = self.led.partition( + [{"signature": "repo:src:src-tools-ts", "security": True, "title": "authz bypass"}] + ) + self.assertEqual([f["title"] for f in to_file], ["authz bypass"]) + self.assertEqual(suppressed, []) + + def test_string_true_counts_as_security(self): + # The verifier is an LLM writing JSON; it sometimes emits the STRING + # "true". groom.yml's labeling rule accepts it, so this must too — the + # exempt set and the `groom-security`-labeled set are the same set. + to_file, _, _ = self.led.partition( + [{"signature": "repo:src:src-tools-ts", "security": "TRUE"}] + ) + self.assertEqual(len(to_file), 1) + + def test_only_an_explicit_true_exempts(self): + # Anything that is not provably `true` — false, absent, null, a stray + # string — stays subject to the backstop. Were this the conservative + # `!= "false"` reading used by the BUILD gate, every candidate that + # simply omits the field would be exempt and the backstop would be dead. + absent = object() + for security in (False, None, "false", "no", 0, "", "yes", absent): + with self.subTest(security=security): + finding = {"signature": "repo:src:src-tools-ts"} + if security is not absent: + finding["security"] = security + to_file, suppressed, _ = self.led.partition([finding]) + self.assertEqual(to_file, []) + self.assertEqual(suppressed[0]["ledger_status"], ledger.PATH_COLLISION) + + def test_exemption_does_not_bypass_exact_signature_dedup(self): + # The exemption is bounded: it only skips the path HEURISTIC. Once the + # security issue exists, its own signature is `filed` and the next run + # suppresses it — so the exemption costs at most one issue, never a + # per-run duplicate. + led = ledger.Ledger({"repo:whole-repo:sec-src-tools-ts": ledger.FILED}) + to_file, suppressed, _ = led.partition( + [{"signature": "repo:whole-repo:sec-src-tools-ts", "security": True}] + ) + self.assertEqual(to_file, []) + self.assertEqual(suppressed[0]["ledger_status"], ledger.FILED) + + def test_security_candidate_is_exempt_from_the_intra_batch_path_check(self): + # Same guarantee within ONE run: a routine finding routed to `to_file` + # first must not swallow a security finding on the same file. + led = ledger.Ledger({}) + to_file, suppressed, _ = led.partition([ + {"signature": "repo:whole-repo:src-tools-ts", "title": "routine"}, + {"signature": "repo:src:src-tools-ts", "security": True, "title": "authz bypass"}, + ]) + self.assertEqual([f["title"] for f in to_file], ["routine", "authz bypass"]) + self.assertEqual(suppressed, []) + + def test_should_file_mirrors_the_exemption(self): + sig = "repo:src:src-tools-ts" + self.assertFalse(self.led.should_file(sig)) + self.assertTrue(self.led.should_file(sig, security=True)) + # A path-anchored key that is already `filed` is not filable either way. + self.assertFalse(self.led.should_file("repo:whole-repo:src-tools-ts", security=True)) + + def test_is_security_finding_helper(self): + self.assertTrue(ledger.is_security_finding({"security": True})) + self.assertTrue(ledger.is_security_finding({"security": " True "})) + self.assertFalse(ledger.is_security_finding({"security": False})) + self.assertFalse(ledger.is_security_finding({})) + self.assertFalse(ledger.is_security_finding("not a dict")) + self.assertFalse(ledger.is_security_finding(None)) + class AcceptanceScenarioTest(unittest.TestCase): """End-to-end run N -> run N+1 using the ledger built from prior issues.""" @@ -665,5 +785,51 @@ def test_oversized_rationale_is_truncated_under_limit(self): self.assertEqual(ledger.extract_signature(out), "sig-big") +class CheckCliTest(unittest.TestCase): + """The `--check` single-signature probe, including its security variant. + + Exit 0 = "file it", 1 = suppressed; stdout is the reported status. The probe + must agree with `partition`, so a routine probe reports `path-collision` + where partition would suppress and `--check-security` reports `unknown` + where partition's exemption would file. + """ + + STATUSES = {"repo:whole-repo:src-tools-ts": "filed"} + + def setUp(self): + self._real_load = ledger.load_ledger + ledger.load_ledger = lambda repo: ledger.Ledger(dict(self.STATUSES)) + self.addCleanup(setattr, ledger, "load_ledger", self._real_load) + + def _check(self, signature, *extra): + import contextlib + import io + + out = io.StringIO() + with contextlib.redirect_stdout(out): + code = ledger.main(["--repo", "owner/name", "--check", signature, *extra]) + return code, out.getvalue().strip() + + def test_known_signature_is_suppressed(self): + self.assertEqual(self._check("repo:whole-repo:src-tools-ts"), (1, "filed")) + + def test_new_path_is_filable(self): + self.assertEqual(self._check("repo:whole-repo:src-server-ts"), (0, "unknown")) + + def test_blank_signature_is_invalid(self): + self.assertEqual(self._check(" "), (1, "invalid")) + + def test_path_collision_is_reported_distinctly(self): + self.assertEqual(self._check("repo:src:src-tools-ts"), (1, ledger.PATH_COLLISION)) + + def test_check_security_skips_the_path_backstop(self): + self.assertEqual(self._check("repo:src:src-tools-ts", "--check-security"), (0, "unknown")) + + def test_check_security_still_honors_exact_signature_dedup(self): + self.assertEqual( + self._check("repo:whole-repo:src-tools-ts", "--check-security"), (1, "filed") + ) + + if __name__ == "__main__": unittest.main() diff --git a/.github/groom/verifier.md b/.github/groom/verifier.md index 71a7841..4e36e46 100644 --- a/.github/groom/verifier.md +++ b/.github/groom/verifier.md @@ -5,7 +5,14 @@ For each finding: (a) is the evidence accurate and are the counts/scope honest? Write VALID JSON to {{VERIFIER_OUT}}, EXACTLY this shape (JSON ONLY, no prose) — escape all string contents (quotes, backslashes, newlines), especially the multi-line `body`: {"repo":"{{REPO}}","scope":"{{SCOPE_LABEL}}","summary":"","findings":[{"title":"","verdict":"CONFIRM|DOWNGRADE|REJECT","security":,"signature":" — see the SIGNATURE rules below>","body":""}]} -SIGNATURE — anchor it to the FILE PATH, never to the title. `` is the finding's PRIMARY file or directory path: the single file/dir the finding is most about, slugified by lowercasing it and collapsing every run of non-alphanumeric characters (`/`, `.`, `_`, spaces, …) to a single hyphen — e.g. `src/tools.ts` -> `src-tools-ts`, `services/ingest/` -> `services-ingest`. For a MULTI-FILE finding use the most representative single path — the one the FIRST evidence item in your `body` cites. Only for a repo-wide pattern with NO single anchoring path, fall back to a normalized SUBJECT NOUN-PHRASE (the thing the finding is about, e.g. `error-wrapping` — NOT the full title, and never a verb-phrase or a fix description). Derive it DETERMINISTICALLY: the SAME finding must yield the SAME signature on every run. +SIGNATURE — anchor it to the FILE PATH, never to the title. `` is the finding's PRIMARY file or directory path, slugified by lowercasing it, collapsing every run of non-alphanumeric characters (`/`, `.`, `_`, spaces, …) to a single hyphen, then TRIMMING any leading/trailing hyphen — e.g. `src/tools.ts` -> `src-tools-ts`, `services/ingest/` -> `services-ingest` (the trailing slash's hyphen is trimmed, so the trailing slash cannot produce a second spelling of one directory). Pick that primary path MECHANICALLY, not by judgment: +- ONE file/dir -> that path. +- MULTI-FILE -> the ALPHABETICALLY FIRST of the full paths the finding cites (compare the raw paths, before slugifying). Do NOT pick "the most representative" one and do NOT use evidence ORDER — both are judgment calls you will make differently on the next run, which re-keys the finding and re-files it. +- Repo-wide pattern with NO anchoring path -> and only then — a SUBJECT NOUN-PHRASE: the shortest noun naming the thing itself, at most 3 words, singular, no adjectives and no verbs (`error-wrapping`, `retry-budget` — NOT `inconsistent-error-wrapping`, NOT the title, NOT a fix description). Prefer a path: if the pattern has ANY natural home directory, anchor to that directory instead of a noun-phrase. -WHY the path and not the title: your titles are re-generated from scratch on every run, so re-wording the same finding ("split tools.ts into modules" -> "tools.ts is a monolith") produces a different title-slug, the exact-string dedup ledger sees a brand-new signature, and the SAME finding is filed as a SECOND issue (this happened — the identical `src/tools.ts` monolith was filed twice in consecutive runs). File and directory paths do not change when the wording does, so a path-anchored signature is stable across re-runs. Keep `` to the path ALONE — do not append the dimension, the fix, or any adjective, or you reintroduce the instability. +If `"security": true`, PREFIX the slug with `sec-` (`sec-src-tools-ts`). A path names a LOCATION, not a finding, so without this a routine finding already filed for a file would dedup away a later security finding about the SAME file — and security findings must always surface as investigations. The prefix is part of the key, so keep the flag itself stable: when in doubt whether a finding is security-adjacent, it IS (that is already this pipeline's default reading). + +Derive the whole slug DETERMINISTICALLY: the SAME finding must yield the SAME signature on every run. + +WHY the path and not the title: your titles are re-generated from scratch on every run, so re-wording the same finding ("split tools.ts into modules" -> "tools.ts is a monolith") produces a different title-slug, the exact-string dedup ledger sees a brand-new signature, and the SAME finding is filed as a SECOND issue (this happened — the identical `src/tools.ts` monolith was filed twice in consecutive runs). File and directory paths do not change when the wording does, so a path-anchored signature is stable across re-runs. Beyond the `sec-` prefix, keep `` to the path ALONE — do not append the dimension, the fix, or any adjective, or you reintroduce the instability. When {{VERIFIER_OUT}} is written, you may stop. From e91409662a07278ed7b923016c06bbc85c47803a Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 16:44:23 -0700 Subject: [PATCH 3/3] fix(groom): domain-separate the security lane, fail closed on the flag (BE-4460) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/groom/README.md | 39 ++++++++++++------ .github/groom/ledger.py | 35 ++++++++++------ .github/groom/tests/test_ledger.py | 65 ++++++++++++++++++++++++------ .github/groom/verifier.md | 4 +- 4 files changed, 102 insertions(+), 41 deletions(-) diff --git a/.github/groom/README.md b/.github/groom/README.md index bb7e2d1..32db0d2 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -55,8 +55,11 @@ rather than buried in a runner script. most representative one" is a judgment the verifier would re-make differently next run. Only a repo-wide pattern with no single anchor falls back to a normalized subject noun-phrase. A `security: true` finding's slug is prefixed - `sec-`, so a routine finding already filed for a file can never dedup away a - security finding about that same file. + `sec_` — underscore, because slugification can never produce one, so the + security lane for `auth.ts` (`sec_auth-ts`) cannot collide with a routine + finding about `sec/auth.ts` (`sec-auth-ts`). That lane is what stops a routine + finding already filed for a file from deduping away a security finding about + that same file. It is anchored to the **path, not the title**, because titles are re-generated on every run: a re-worded title yields a new title-slug, the ledger's exact-string match sees a "new" finding, and the same finding is filed twice @@ -176,28 +179,38 @@ deliberately narrow — two carve-outs: location, not a finding, so without this an already-filed routine finding on `src/tools.ts` would bury a later security finding on the same file and break the "security findings always surface as investigations" guarantee. (The - verifier's `sec-` slug prefix separates the two lanes up front; this is the - code-side guarantee for legacy and cross-scope signatures that predate it.) - Exact-signature dedup still applies, so the exemption costs at most one issue - per security finding — the next run sees it as `filed`. + verifier's `sec_` slug prefix separates the two lanes up front; this is the + code-side guarantee for legacy and cross-scope signatures that predate it, and + it fails **closed** — `is_security_finding` treats a finding whose flag the + verifier omitted or mangled as security, so a malformed flag can never be what + buries one.) Exact-signature dedup still applies, so the exemption costs at + most one extra issue — the next run sees it as `filed`. It does **not** make + security findings individually addressable: two distinct vulnerabilities in one + file share the `sec_` key and collapse, exactly as two routine findings + on one file do (see the limit below). - **`superseded` records are left out of the path index.** `groom-superseded` is the documented "retire this issue so its finding can be re-filed under the current format" signal; keeping it in the index would let the retired issue go on suppressing the replacement by path and defeat the label a human applied. -Known, accepted limit: slugification is lossy, so two genuinely distinct paths -can map to one slug (`src/foo/bar.ts` and `src/foo-bar.ts` both → -`src-foo-bar-ts`) and the second finding is suppressed. The blast radius is one -finding on a path shape that is rare in practice, and it is the same collapse the -format already accepts for two distinct findings about one file — the deliberate -trade for a key that is stable across re-wordings. +Known, accepted limit — and a **permanent** suppression, not a one-time +transition cost. A path-anchored key identifies a *location*, so every later +finding that maps to an already-covered token is dropped for good: two different +findings about one file, and two genuinely distinct paths that slugify alike +(`src/foo/bar.ts` and `src/foo-bar.ts` both → `src-foo-bar-ts`, or a file and a +same-stem directory). That is the deliberate trade for a key that survives +re-wording — one issue per anchoring path, chosen over the duplicate-per-run +spam the title-derived key produced. Widening it needs a per-finding +discriminator that is *stable across runs*, which is exactly what the LLM cannot +supply today; the `security` flag is one bit that is, which is why the security +lane is carved out of this and nothing else is. Consequence for the format transition: a legacy *title*-derived slug that merely *embeds* the path (`split-tools-ts-into-focused-modules`) is **not** matched, so such a finding can be filed once more under its new path-anchored signature — then it is stable forever. Label the superseded legacy issue `groom-superseded` (or close it as not planned) to retire it. A security finding already filed under -an unprefixed slug re-files once for the same reason when it picks up its `sec-` +an unprefixed slug re-files once for the same reason when it picks up its `sec_` prefix — same one-per-finding, one-time transition cost, same fix. The dedup decision is a point-in-time snapshot of GitHub issue state read diff --git a/.github/groom/ledger.py b/.github/groom/ledger.py index 83ae226..33fbc6f 100644 --- a/.github/groom/ledger.py +++ b/.github/groom/ledger.py @@ -347,22 +347,23 @@ def path_token(signature) -> str: def is_security_finding(finding) -> bool: - """True only if the candidate is EXPLICITLY marked `security: true`. - - Same truthiness rule the filing step uses to apply the `groom-security` - label (`str(...).strip().lower() == "true"` in groom.yml), so "the finding - exempt from the path backstop" and "the finding labeled security" are the - same set — never one without the other. - - Deliberately NOT the conservative `!= "false"` reading used by the *build* - gate. There, treating an unmarked finding as security is the safe direction - (it stays an issue instead of being auto-implemented). Here the exemption - only ever *adds* filings, so `!= "false"` would exempt every candidate that - simply omits the field and disable the backstop entirely. + """True unless the candidate is PROVABLY non-security (`security: false`). + + Fails CLOSED, the same conservative reading the build gate uses (groom.yml: + `str(...).strip().lower() != "false"`): a finding whose flag the verifier + omitted or mangled is treated as security, so the path backstop can never be + what silently buries it. The verifier's schema requires `security` on every + finding, so well-formed batches are unaffected — only malformed producer + output takes the safe branch, and there it costs at most one extra issue + (exact-signature dedup still suppresses it on the next run). + + The `== "true"` reading would be wrong here for the same reason it is wrong + at the build gate: it decides a SECURITY guarantee from an LLM-authored + field, so ambiguity has to resolve toward surfacing the finding. """ if not isinstance(finding, dict): return False - return str(finding.get("security")).strip().lower() == "true" + return str(finding.get("security")).strip().lower() != "false" def extract_signature(body): @@ -483,6 +484,14 @@ def __init__(self, statuses: dict): # path and defeat the very label the human applied. Every other status # (including the durable human "no" of REJECTED / PR_CLOSED) still seeds # the index. + # + # `statuses` is already collapsed by `_PRECEDENCE`, where SUPERSEDED + # outranks FILED/PR_OPEN, so one signature carrying BOTH a superseded + # issue and a live one reads as SUPERSEDED and leaves the index. That + # only relaxes the heuristic: the signature itself is still non-UNKNOWN, + # so an exact re-run is suppressed as before, and the most a re-scoped + # variant can cost is one duplicate — the same bound as the rest of the + # format transition. self._known_paths = { token for token in ( diff --git a/.github/groom/tests/test_ledger.py b/.github/groom/tests/test_ledger.py index 4ac76a5..bb57940 100644 --- a/.github/groom/tests/test_ledger.py +++ b/.github/groom/tests/test_ledger.py @@ -362,7 +362,7 @@ def test_same_path_under_a_different_signature_is_suppressed(self): # Same path, different leading segments (a legacy signature, a re-scoped # run) => exact-string dedup misses it; the path backstop catches it. to_file, suppressed, invalid = self.led.partition( - [{"signature": "repo:src:src-tools-ts", "title": "tools.ts is a monolith"}] + [{"signature": "repo:src:src-tools-ts", "security": False, "title": "tools.ts is a monolith"}] ) self.assertEqual(to_file, []) self.assertEqual(suppressed[0]["ledger_status"], ledger.PATH_COLLISION) @@ -406,8 +406,8 @@ def test_intra_batch_path_collision_suppresses_the_second(self): # anchored to the same path must not open two issues either. led = ledger.Ledger({}) to_file, suppressed, _ = led.partition([ - {"signature": "repo:whole-repo:src-tools-ts", "title": "first"}, - {"signature": "repo:src:src-tools-ts", "title": "second, re-keyed"}, + {"signature": "repo:whole-repo:src-tools-ts", "security": False, "title": "first"}, + {"signature": "repo:src:src-tools-ts", "security": False, "title": "second, re-keyed"}, ]) self.assertEqual([f["title"] for f in to_file], ["first"]) self.assertEqual([(f["title"], f["ledger_status"]) for f in suppressed], @@ -416,7 +416,9 @@ def test_intra_batch_path_collision_suppresses_the_second(self): def test_rejected_path_stays_suppressed_across_a_rewording(self): # A human rejection must survive the verifier re-keying the finding. led = ledger.Ledger({"repo:whole-repo:src-tools-ts": ledger.REJECTED}) - to_file, suppressed, _ = led.partition([{"signature": "repo:whole-repo-v2:src-tools-ts"}]) + to_file, suppressed, _ = led.partition( + [{"signature": "repo:whole-repo-v2:src-tools-ts", "security": False}] + ) self.assertEqual(to_file, []) self.assertEqual(suppressed[0]["ledger_status"], ledger.PATH_COLLISION) @@ -471,25 +473,37 @@ def test_security_candidate_is_not_suppressed_by_path_collision(self): def test_string_true_counts_as_security(self): # The verifier is an LLM writing JSON; it sometimes emits the STRING - # "true". groom.yml's labeling rule accepts it, so this must too — the - # exempt set and the `groom-security`-labeled set are the same set. + # "true" rather than the literal. to_file, _, _ = self.led.partition( [{"signature": "repo:src:src-tools-ts", "security": "TRUE"}] ) self.assertEqual(len(to_file), 1) - def test_only_an_explicit_true_exempts(self): - # Anything that is not provably `true` — false, absent, null, a stray - # string — stays subject to the backstop. Were this the conservative - # `!= "false"` reading used by the BUILD gate, every candidate that - # simply omits the field would be exempt and the backstop would be dead. + def test_an_unusable_flag_fails_closed(self): + # The backstop decides a SECURITY guarantee from an LLM-authored field, + # so ambiguity resolves toward surfacing the finding — same conservative + # reading the build gate uses. A flag that is absent, null or mangled + # exempts the candidate rather than risking a silently buried finding. + # Cost is one extra issue; the next run suppresses it as `filed`. absent = object() - for security in (False, None, "false", "no", 0, "", "yes", absent): + for security in (None, "no", 0, "", "yes", absent): with self.subTest(security=security): finding = {"signature": "repo:src:src-tools-ts"} if security is not absent: finding["security"] = security to_file, suppressed, _ = self.led.partition([finding]) + self.assertEqual(len(to_file), 1) + self.assertEqual(suppressed, []) + + def test_only_a_provably_false_flag_is_subject_to_the_backstop(self): + # The well-formed case: the verifier's schema requires `security` on + # every finding, so real batches always land here and the backstop is + # fully active for them. + for security in (False, "false", "FALSE", " false "): + with self.subTest(security=security): + to_file, suppressed, _ = self.led.partition( + [{"signature": "repo:src:src-tools-ts", "security": security}] + ) self.assertEqual(to_file, []) self.assertEqual(suppressed[0]["ledger_status"], ledger.PATH_COLLISION) @@ -516,6 +530,27 @@ def test_security_candidate_is_exempt_from_the_intra_batch_path_check(self): self.assertEqual([f["title"] for f in to_file], ["routine", "authz bypass"]) self.assertEqual(suppressed, []) + def test_the_security_lane_prefix_is_domain_separated(self): + # `verifier.md` prefixes a security finding's slug `sec_` — UNDERSCORE, + # because slugifying a path can never produce one (every `_` in a path + # collapses to a hyphen). A hyphen prefix would NOT be domain-separated: + # a routine finding about `sec/auth.ts` slugifies to `sec-auth-ts` and + # would share a signature with a security finding about `auth.ts`, + # silently suppressing one of them. These two must stay distinct. + routine_in_sec_dir = "repo:whole-repo:sec-auth-ts" # sec/auth.ts, routine + security_on_auth = "repo:whole-repo:sec_auth-ts" # auth.ts, security + self.assertNotEqual( + ledger.path_token(routine_in_sec_dir), ledger.path_token(security_on_auth) + ) + led = ledger.Ledger({routine_in_sec_dir: ledger.FILED}) + to_file, suppressed, _ = led.partition( + [{"signature": security_on_auth, "security": False}] + ) + # Suppression here would be the collision, not the exemption — so assert + # it with the flag OFF, where the backstop is fully armed. + self.assertEqual(len(to_file), 1) + self.assertEqual(suppressed, []) + def test_should_file_mirrors_the_exemption(self): sig = "repo:src:src-tools-ts" self.assertFalse(self.led.should_file(sig)) @@ -526,8 +561,12 @@ def test_should_file_mirrors_the_exemption(self): def test_is_security_finding_helper(self): self.assertTrue(ledger.is_security_finding({"security": True})) self.assertTrue(ledger.is_security_finding({"security": " True "})) + self.assertTrue(ledger.is_security_finding({})) # fails closed + self.assertTrue(ledger.is_security_finding({"security": None})) self.assertFalse(ledger.is_security_finding({"security": False})) - self.assertFalse(ledger.is_security_finding({})) + self.assertFalse(ledger.is_security_finding({"security": " FALSE "})) + # A non-dict is not a finding at all; `partition` routes it to `invalid` + # before ever asking, so it must not be reported as security. self.assertFalse(ledger.is_security_finding("not a dict")) self.assertFalse(ledger.is_security_finding(None)) diff --git a/.github/groom/verifier.md b/.github/groom/verifier.md index 4e36e46..01a894d 100644 --- a/.github/groom/verifier.md +++ b/.github/groom/verifier.md @@ -10,9 +10,9 @@ SIGNATURE — anchor it to the FILE PATH, never to the title. `` is t - MULTI-FILE -> the ALPHABETICALLY FIRST of the full paths the finding cites (compare the raw paths, before slugifying). Do NOT pick "the most representative" one and do NOT use evidence ORDER — both are judgment calls you will make differently on the next run, which re-keys the finding and re-files it. - Repo-wide pattern with NO anchoring path -> and only then — a SUBJECT NOUN-PHRASE: the shortest noun naming the thing itself, at most 3 words, singular, no adjectives and no verbs (`error-wrapping`, `retry-budget` — NOT `inconsistent-error-wrapping`, NOT the title, NOT a fix description). Prefer a path: if the pattern has ANY natural home directory, anchor to that directory instead of a noun-phrase. -If `"security": true`, PREFIX the slug with `sec-` (`sec-src-tools-ts`). A path names a LOCATION, not a finding, so without this a routine finding already filed for a file would dedup away a later security finding about the SAME file — and security findings must always surface as investigations. The prefix is part of the key, so keep the flag itself stable: when in doubt whether a finding is security-adjacent, it IS (that is already this pipeline's default reading). +If `"security": true`, PREFIX the finished slug with `sec_` — UNDERSCORE, not hyphen: `auth.ts` -> `sec_auth-ts`. The underscore is the whole point and is NOT a typo to normalize away: slugifying a path can never produce one (every `_` in a path collapses to a hyphen), so `sec_auth-ts` (a security finding about `auth.ts`) can never collide with `sec-auth-ts` (a routine finding about `sec/auth.ts`). A path names a LOCATION, not a finding, so without this lane a routine finding already filed for a file would dedup away a later security finding about the SAME file — and security findings must always surface as investigations. The prefix is part of the key, so keep the flag itself stable: when in doubt whether a finding is security-adjacent, it IS (that is already this pipeline's default reading). Derive the whole slug DETERMINISTICALLY: the SAME finding must yield the SAME signature on every run. -WHY the path and not the title: your titles are re-generated from scratch on every run, so re-wording the same finding ("split tools.ts into modules" -> "tools.ts is a monolith") produces a different title-slug, the exact-string dedup ledger sees a brand-new signature, and the SAME finding is filed as a SECOND issue (this happened — the identical `src/tools.ts` monolith was filed twice in consecutive runs). File and directory paths do not change when the wording does, so a path-anchored signature is stable across re-runs. Beyond the `sec-` prefix, keep `` to the path ALONE — do not append the dimension, the fix, or any adjective, or you reintroduce the instability. +WHY the path and not the title: your titles are re-generated from scratch on every run, so re-wording the same finding ("split tools.ts into modules" -> "tools.ts is a monolith") produces a different title-slug, the exact-string dedup ledger sees a brand-new signature, and the SAME finding is filed as a SECOND issue (this happened — the identical `src/tools.ts` monolith was filed twice in consecutive runs). File and directory paths do not change when the wording does, so a path-anchored signature is stable across re-runs. Beyond the `sec_` prefix, keep `` to the path ALONE — do not append the dimension, the fix, or any adjective, or you reintroduce the instability. When {{VERIFIER_OUT}} is written, you may stop.