[PR #43] [MERGED] feat(groom): durable dedup/rejection ledger backed by GitHub issue state (BE-3874) #53

New issue

Closed

opened 2026-10-26 22:35:45 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:35:45 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-3874-groom-rejection-ledger

📝 Commits (3)

5c24e7a feat(groom): durable dedup/rejection ledger backed by GitHub issue state (BE-3874)
97ade96 fix(groom): harden ledger marker + batch dedup per review (BE-3874)
e612423 Merge remote-tracking branch 'origin/main' into matt/be-3874-groom-rejection-ledger

📊 Changes

5 files changed (+856 additions, -1 deletions)

View changed files

📝 .github/groom/README.md (+73 -1)
➕ .github/groom/ledger.py (+422 -0)
➕ .github/groom/tests/test_ledger.py (+314 -0)
➕ .github/workflows/test-groom-scripts.yml (+39 -0)
📝 AGENTS.md (+8 -0)

📄 Description

ELI-5

The "groom" janitor bot files GitHub issues for code cleanups it spots. In CI it
has no memory — every scheduled run it wakes up with a blank brain. So it
would file the same cleanup again next week, and — worse — re-file things a
human already looked at and said "no thanks" to. That's how you get everyone to
mute and disable the bot.

This PR gives it a memory that survives across runs, without any new database
or secret: it just uses the GitHub issues themselves as the memory. Before
filing, it lists the cleanup issues it already opened and skips any it's seen —
including ones a human closed as "won't fix" (those stay suppressed forever).

What

.github/groom/ledger.py — a durable dedup/rejection ledger keyed on
(repo, finding_signature) -> {filed | rejected | superseded}, built by reading
live GitHub issue state.

Live GitHub state for a signature	Ledger status	Re-file?
Open `groom` issue	`filed`	no
Closed as completed	`filed`	no
Closed as not planned (GitHub "close as wontfix")	`rejected`	no — durable
Has the `groom-rejected` label (open or closed)	`rejected`	no — durable
Has the `groom-superseded` label	`superseded`	no
No `groom` issue carries it	`unknown`	yes

Filing embeds the verifier's opaque signature as an invisible HTML-comment
marker () in the issue body; the next run recovers
it. Pure logic (marker round-trip, classification, ledger build, candidate
partition) is separated from the one thin gh api I/O shell, so it's fully
unit-tested with no network (.github/groom/tests/, 29 cases, wired into CI via
test-groom-scripts.yml).

CLI the groom workflow calls right before filing:

python3 .github/groom/ledger.py --repo owner/name --candidates findings.json --out decision.json
# -> {to_file, suppressed, invalid, ledger_size}; open issues only for to_file

Why this store (design decision)

The ticket asked to prefer the GitHub-native option that needs no net-new
secret. Issue state wins on all three acceptance points at once: it needs no
separate store, the run's built-in GITHUB_TOKEN already reads issues (no new
secret), and it's inherently auditable (the record is the issues). It also
captures human rejection for free — a maintainer closing an issue as "not
planned" is the rejection signal, no extra tracking needed. I considered a
committed .ledger.json but rejected it: it duplicates state that already lives
in the issues and adds bot-commit noise/merge churn on every run.

Acceptance criteria

✅ Filed/rejected in run N not re-filed in run N+1 (same signature) — any
existing groom issue for a signature makes it known; should_file is true
only for unknown. (AcceptanceScenarioTest, LedgerDecisionTest.)
✅ Human rejection durably suppresses — close-as-not_planned or the
groom-rejected label → rejected; read live every run, so it never expires.
✅ Auditable, no net-new org secret — GitHub issues are the store; read via
gh api with the run's GITHUB_TOKEN.

Self-review notes / judgment calls

Load-bearing filing contract (dependency on the not-yet-written groom
workflow). The memory only works if the step that opens an issue applies
the groom label and appends signature_marker(sig) to the body. This is
the correct seam — a ledger can't remember what the filer didn't record — and
it's documented in the module docstring + README. The filing step is Phase 1/2
of BE-3870; this PR is the standalone hard piece it will call.
Closed-as-completed → suppressed (won't re-file a fixed-then-regressed
debt). Deliberate anti-annoyance default matching the acceptance wording
("filed in run N not re-filed in N+1"). If re-detecting regressions is wanted
later, treat completed as unknown — it's a one-constant change.
superseded is a defined hook with no producer in this PR — the
groom-superseded label is classified if present, part of the ticket's
{filed|rejected|superseded} model; nothing here sets it yet.
Fail-loud, not fail-empty. If gh api errors, fetch_groom_issues raises
rather than returning an empty ledger — an empty ledger would re-file
everything (the exact spam this prevents).
Negative-claim check: the diff adds no capability-denying dead-end / "not
supported" path; the invalid bucket is defensive routing of malformed
(signature-less) input, surfaced in output — not a user-facing denial. Trigger
does not fire.
Purely additive: 5 new files, +8 doc lines in AGENTS.md; no existing behavior
changed (Chesterton's Fence clean). AGENTS.md integrity checker still passes.

_{🔄 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/43 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/21/2026 **Status:** ✅ Merged **Merged:** 7/21/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-3874-groom-rejection-ledger` --- ### 📝 Commits (3) - [`5c24e7a`](https://github.com/Comfy-Org/github-workflows/commit/5c24e7aa53d041fb6bf6e46933c1bf773fb039b0) feat(groom): durable dedup/rejection ledger backed by GitHub issue state (BE-3874) - [`97ade96`](https://github.com/Comfy-Org/github-workflows/commit/97ade96c1caced3cb90e30df2178483d4f6a8621) fix(groom): harden ledger marker + batch dedup per review (BE-3874) - [`e612423`](https://github.com/Comfy-Org/github-workflows/commit/e612423f848d59c31af3a63d63ef3844adb3a032) Merge remote-tracking branch 'origin/main' into matt/be-3874-groom-rejection-ledger ### 📊 Changes **5 files changed** (+856 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+73 -1) ➕ `.github/groom/ledger.py` (+422 -0) ➕ `.github/groom/tests/test_ledger.py` (+314 -0) ➕ `.github/workflows/test-groom-scripts.yml` (+39 -0) 📝 `AGENTS.md` (+8 -0) </details> ### 📄 Description ## ELI-5 The "groom" janitor bot files GitHub issues for code cleanups it spots. In CI it has **no memory** — every scheduled run it wakes up with a blank brain. So it would file the *same* cleanup again next week, and — worse — re-file things a human already looked at and said "no thanks" to. That's how you get everyone to mute and disable the bot. This PR gives it a memory that survives across runs, **without any new database or secret**: it just uses **the GitHub issues themselves** as the memory. Before filing, it lists the cleanup issues it already opened and skips any it's seen — including ones a human closed as "won't fix" (those stay suppressed forever). ## What `.github/groom/ledger.py` — a durable dedup/rejection ledger keyed on `(repo, finding_signature) -> {filed | rejected | superseded}`, built by reading live GitHub issue state. | Live GitHub state for a signature | Ledger status | Re-file? | |---|---|---| | Open `groom` issue | `filed` | no | | Closed as **completed** | `filed` | no | | Closed as **not planned** (GitHub "close as wontfix") | `rejected` | **no — durable** | | Has the `groom-rejected` label (open or closed) | `rejected` | **no — durable** | | Has the `groom-superseded` label | `superseded` | no | | No `groom` issue carries it | `unknown` | **yes** | Filing embeds the verifier's opaque signature as an invisible HTML-comment marker (``) in the issue body; the next run recovers it. Pure logic (marker round-trip, classification, ledger build, candidate partition) is separated from the one thin `gh api` I/O shell, so it's fully unit-tested with no network (`.github/groom/tests/`, 29 cases, wired into CI via `test-groom-scripts.yml`). CLI the groom workflow calls right before filing: ```bash python3 .github/groom/ledger.py --repo owner/name --candidates findings.json --out decision.json # -> {to_file, suppressed, invalid, ledger_size}; open issues only for to_file ``` ## Why this store (design decision) The ticket asked to prefer the GitHub-native option that needs no net-new secret. **Issue state wins on all three acceptance points at once:** it needs no separate store, the run's built-in `GITHUB_TOKEN` already reads issues (no new secret), and it's inherently auditable (the record *is* the issues). It also captures human rejection *for free* — a maintainer closing an issue as "not planned" is the rejection signal, no extra tracking needed. I considered a committed `.ledger.json` but rejected it: it duplicates state that already lives in the issues and adds bot-commit noise/merge churn on every run. ## Acceptance criteria - ✅ **Filed/rejected in run N not re-filed in run N+1 (same signature)** — any existing `groom` issue for a signature makes it `known`; `should_file` is true only for `unknown`. (`AcceptanceScenarioTest`, `LedgerDecisionTest`.) - ✅ **Human rejection durably suppresses** — close-as-`not_planned` or the `groom-rejected` label → `rejected`; read live every run, so it never expires. - ✅ **Auditable, no net-new org secret** — GitHub issues are the store; read via `gh api` with the run's `GITHUB_TOKEN`. ## Self-review notes / judgment calls - **Load-bearing filing contract (dependency on the not-yet-written groom workflow).** The memory only works if the step that *opens* an issue applies the `groom` label **and** appends `signature_marker(sig)` to the body. This is the correct seam — a ledger can't remember what the filer didn't record — and it's documented in the module docstring + README. The filing step is Phase 1/2 of BE-3870; this PR is the standalone hard piece it will call. - **Closed-as-completed → suppressed (won't re-file a fixed-then-regressed debt).** Deliberate anti-annoyance default matching the acceptance wording ("filed in run N not re-filed in N+1"). If re-detecting regressions is wanted later, treat `completed` as `unknown` — it's a one-constant change. - **`superseded` is a defined hook with no producer in this PR** — the `groom-superseded` label is classified if present, part of the ticket's `{filed|rejected|superseded}` model; nothing here sets it yet. - **Fail-loud, not fail-empty.** If `gh api` errors, `fetch_groom_issues` raises rather than returning an empty ledger — an empty ledger would re-file everything (the exact spam this prevents). - **Negative-claim check:** the diff adds no capability-denying dead-end / "not supported" path; the `invalid` bucket is defensive routing of malformed (signature-less) input, surfaced in output — not a user-facing denial. Trigger does not fire. - Purely additive: 5 new files, +8 doc lines in AGENTS.md; no existing behavior changed (Chesterton's Fence clean). AGENTS.md integrity checker still passes. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:35:45 +00:00