[PR #74] [MERGED] fix(groom): anchor the dedup signature to the finding's file path (BE-4460) #85

New issue

Closed

opened 2026-10-26 22:36:03 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:03 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/74
Author: @mattmillerai
Created: 7/24/2026
Status: ✅ Merged
Merged: 7/30/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4460-groom-path-signature

📝 Commits (4)

c3f86b9 fix(groom): anchor the dedup signature to the finding's file path (BE-4460)
5da5ba7 fix(groom): scope the path backstop so it can't bury a security finding (BE-4460)
e914096 fix(groom): domain-separate the security lane, fail closed on the flag (BE-4460)
2d02574 Merge remote-tracking branch 'origin/main' into matt/be-4460-groom-path-signature

📊 Changes

4 files changed (+635 additions, -12 deletions)

View changed files

📝 .github/groom/README.md (+83 -4)
📝 .github/groom/ledger.py (+196 -7)
📝 .github/groom/tests/test_ledger.py (+344 -0)
📝 .github/groom/verifier.md (+12 -1)

📄 Description

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 — new path_token() (everything after the second :; "" when there is no third segment) plus a path-token backstop in partition(): a candidate whose exact signature is unknown but whose path segment is already covered — by a known signature, or by a candidate already routed to to_file in the same batch — is reported under suppressed with the distinct status path-collision. Ledger.should_file() and the --check CLI agree with partition (they report path-collision and 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.
Tests — 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/partition agreement) and two acceptance-scenario tests (reworded-next-run files nothing; marker round-trip with a path-format signature). 74 tests pass.
Docs — .github/groom/README.md: the contract-table signature row now documents the path derivation and why, a path-collision row in the ledger-status table, and a new "path-token backstop" section covering the exact-match rule and the transition consequence.

Acceptance criteria

Two consecutive runs describing the same file with different wording produce ONE issue — met. The signature no longer depends on the title, so run N+1 emits the identical signature and partition suppresses it with ledger_status: filed, visible in decision.json. Covered by test_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.)
Existing open groom issues with legacy signatures are not re-filed when the new-format candidate shares their path token — met only for an exact path-segment match; flagged as partially unmet. The ticket asked for exact matching ("no fuzzy matching"), and exact matching cannot recognize a legacy TITLE-derived slug such as split-tools-ts-into-focused-modules or tools-ts-monolith as covering the new src-tools-ts — the strings genuinely differ. I deliberately did not add substring/shared-token matching: src-index-ts vs lib-index-ts would 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 with groom-superseded (or close as not planned). Pinned by test_legacy_title_slug_embedding_the_path_is_not_matched and documented in the README.
ci-groom.yml / test-groom-scripts.yml pass — test-groom-scripts.yml runs the unit suite on this PR (green locally: 74 tests). ci-groom.yml never fires on a PR (schedule + workflow_dispatch only) and is untouched by this diff — groom.yml's interface is unchanged.

Judgment calls

Intra-batch path collisions are suppressed too, not just collisions against the live ledger. The ticket specified "each candidate and each known signature"; within one run GitHub state is not refreshed, so two candidates anchored to the same path would otherwise open two issues in a single run — the same failure mode, and the existing pending status exists for exactly that reason.
Path tokens are lowercased for comparison only. The full signature stays case-sensitive and opaque everywhere else; the verifier already emits lowercase slugs, so this only absorbs a stray capital.
Inherent tradeoff of path anchoring: two genuinely distinct findings about the same file now share one signature and collapse to one issue. That is a direct consequence of the ticket's design decision, not an artifact of the backstop; the repo-wide noun-phrase fallback is the escape hatch.
groom.yml untouched. The job-summary line counts suppressed without breaking it out by status, so path-collision entries are counted there and fully itemized in decision.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.yml pins workflows_ref to a merged-main SHA (07154fb), and the briefs + ledger.py are 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 floating v1 tag is moved, per the repo's versioning policy.

Verification

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'   # 74 tests, OK
python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'   # 40 tests, OK
python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py'   # 18 tests, OK
python3 .github/agents-md-integrity/check_agents_md.py --root .   # passed (pre-existing CODEOWNERS warning)

The --check CLI path was exercised directly against a stubbed ledger: a colliding signature prints path-collision / exit 1, a new one unknown / exit 0, an exact match filed / exit 1, a blank one invalid / 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.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/74 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/24/2026 **Status:** ✅ Merged **Merged:** 7/30/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4460-groom-path-signature` --- ### 📝 Commits (4) - [`c3f86b9`](https://github.com/Comfy-Org/github-workflows/commit/c3f86b96287677865e8798c06c7383193e925777) fix(groom): anchor the dedup signature to the finding's file path (BE-4460) - [`5da5ba7`](https://github.com/Comfy-Org/github-workflows/commit/5da5ba74d9d43f44ee62b876dc3dabceedd4f493) fix(groom): scope the path backstop so it can't bury a security finding (BE-4460) - [`e914096`](https://github.com/Comfy-Org/github-workflows/commit/e91409662a07278ed7b923016c06bbc85c47803a) fix(groom): domain-separate the security lane, fail closed on the flag (BE-4460) - [`2d02574`](https://github.com/Comfy-Org/github-workflows/commit/2d02574a70a2a7b0d2d9e1c4bc963ceca56ab7a9) Merge remote-tracking branch 'origin/main' into matt/be-4460-groom-path-signature ### 📊 Changes **4 files changed** (+635 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+83 -4) 📝 `.github/groom/ledger.py` (+196 -7) 📝 `.github/groom/tests/test_ledger.py` (+344 -0) 📝 `.github/groom/verifier.md` (+12 -1) </details> ### 📄 Description ## 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`** — new `path_token()` (everything after the second `:`; `""` when there is no third segment) plus a **path-token backstop** in `partition()`: a candidate whose exact signature is `unknown` but whose path segment is already covered — by a known signature, or by a candidate already routed to `to_file` in the same batch — is reported under `suppressed` with the distinct status **`path-collision`**. `Ledger.should_file()` and the `--check` CLI agree with `partition` (they report `path-collision` and 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. - **Tests** — `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`/`partition` agreement) and two acceptance-scenario tests (reworded-next-run files nothing; marker round-trip with a path-format signature). 74 tests pass. - **Docs** — `.github/groom/README.md`: the contract-table `signature` row now documents the path derivation and why, a `path-collision` row in the ledger-status table, and a new "path-token backstop" section covering the exact-match rule and the transition consequence. ## Acceptance criteria 1. **Two consecutive runs describing the same file with different wording produce ONE issue** — met. The signature no longer depends on the title, so run N+1 emits the identical signature and `partition` suppresses it with `ledger_status: filed`, visible in `decision.json`. Covered by `test_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.) 2. **Existing open groom issues with legacy signatures are not re-filed when the new-format candidate shares their path token** — **met only for an exact path-segment match; flagged as partially unmet.** The ticket asked for exact matching ("no fuzzy matching"), and exact matching cannot recognize a legacy TITLE-derived slug such as `split-tools-ts-into-focused-modules` or `tools-ts-monolith` as covering the new `src-tools-ts` — the strings genuinely differ. I deliberately did **not** add substring/shared-token matching: `src-index-ts` vs `lib-index-ts` would 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 with `groom-superseded` (or close as not planned). Pinned by `test_legacy_title_slug_embedding_the_path_is_not_matched` and documented in the README. 3. **`ci-groom.yml` / `test-groom-scripts.yml` pass** — `test-groom-scripts.yml` runs the unit suite on this PR (green locally: 74 tests). `ci-groom.yml` never fires on a PR (schedule + `workflow_dispatch` only) and is untouched by this diff — `groom.yml`'s interface is unchanged. ## Judgment calls - **Intra-batch path collisions are suppressed too**, not just collisions against the live ledger. The ticket specified "each candidate and each known signature"; within one run GitHub state is not refreshed, so two candidates anchored to the same path would otherwise open two issues in a single run — the same failure mode, and the existing `pending` status exists for exactly that reason. - **Path tokens are lowercased for comparison only.** The full signature stays case-sensitive and opaque everywhere else; the verifier already emits lowercase slugs, so this only absorbs a stray capital. - **Inherent tradeoff of path anchoring:** two genuinely distinct findings about the same file now share one signature and collapse to one issue. That is a direct consequence of the ticket's design decision, not an artifact of the backstop; the repo-wide noun-phrase fallback is the escape hatch. - **`groom.yml` untouched.** The job-summary line counts `suppressed` without breaking it out by status, so `path-collision` entries are counted there and fully itemized in `decision.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.yml` pins `workflows_ref` to a merged-main SHA (`07154fb`), and the briefs + `ledger.py` are 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 floating `v1` tag is moved, per the repo's versioning policy. ## Verification ``` python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' # 74 tests, OK python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' # 40 tests, OK python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py' # 18 tests, OK python3 .github/agents-md-integrity/check_agents_md.py --root . # passed (pre-existing CODEOWNERS warning) ``` The `--check` CLI path was exercised directly against a stubbed ledger: a colliding signature prints `path-collision` / exit 1, a new one `unknown` / exit 0, an exact match `filed` / exit 1, a blank one `invalid` / 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:03 +00:00