[PR #55] [MERGED] feat(groom): opt-in auto-builder — top-N CONFIRMED findings become review-gated PRs (BE-4003) #65

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-4003-groom-auto-builder

📝 Commits (3)

8173fc2 feat(groom): opt-in auto-builder — top-N CONFIRMED findings become review-gated PRs (BE-4003)
2052e70 fix(groom): least-privilege bot tokens, normalize pr_size_limit, doc issues:write (BE-4003)
08ad004 fix(groom): harden auto-builder against review-panel findings (BE-4003)

📊 Changes

7 files changed (+816 additions, -74 deletions)

View changed files

📝 .github/groom/README.md (+24 -5)
➕ .github/groom/builder.md (+16 -0)
📝 .github/groom/ledger.py (+73 -24)
📝 .github/groom/tests/test_ledger.py (+108 -5)
📝 .github/workflows/groom.yml (+582 -31)
📝 AGENTS.md (+12 -8)
📝 README.md (+1 -1)

📄 Description

ELI-5

The groomer finds tech-debt and files GitHub issues, but the team reviews at the PR level and never works the issues — so they're a dead end. This adds an opt-in auto-builder (split C of the groom epic): when a repo turns it on, the top few confirmed findings each run are turned into real, reviewable PRs (throttled to 5), while everything else still files as issues. The PRs run full CI + cursor-review and are never auto-merged — a human still presses the button. Off by default, so nothing changes unless you opt in.

What changed

groom.yml — three new jobs behind builder: true (default false):
- build_select — the single dedup + throttle authority (no agent). Dedups verified findings against the durable ledger, then splits them: top max_prs (default 5) CONFIRM, non-security findings → build queue; the rest (DOWNGRADE, security, beyond-cap) → issue remainder. In builder-off mode it's a pass-through, so the issue path is byte-identical to before.
- build — the credential-free builder (matrix, one cell per queued finding). permissions: contents: read, only ANTHROPIC_API_KEY. An agent writes the code change into its own fresh checkout; a deterministic step captures the working-tree diff as a patch artifact and enforces the pr_size_limit bail-out. Mirrors the finder/verifier isolation exactly.
- build_pr — the credentialed PR job (no agent). Mints the bot token, applies the patch on a branch, opens a branded, review-gated PR (labels groom + groom-pr + cursor-review, a loud "NOT auto-merged" banner, the verifier rationale, and the signature marker). On a bail/oversized/non-applying patch it files a groom issue instead so nothing is dropped. Honors dry_run (preview, opens nothing).
ledger.py — PR-state added. The /issues listing already returns groom-labeled PRs; builder PRs carry the same signature marker in their body, so the ledger now classifies them: open → pr-open, merged → merged, closed-unmerged → pr-closed. All suppress re-proposing → a merged/closed finding is never re-proposed (RFC point 3). The markerless-record skip still guards against poisoning. (BE-3874 skipped all PRs because groom filed only issues then — cited in the code.)
builder.md — new phase-3 brief (scope-disciplined, keep-it-green, never-touch-security, patch-size bail).
Tests + README/AGENTS.md updated.

Acceptance criteria

≤5 verifier-top findings → PRs; remainder not built — build_select caps to_build at max_prs.
Builder holds no creds, emits only a patch artifact; PR creation in a separate job as the bot — build (contents: read) → artifact → build_pr (bot token).
Ledger prevents re-proposing a merged/closed/rejected finding — PR-state in ledger.py, regression-tested (test_{open,merged,closed}_builder_pr_* in test_ledger.py).
Builder PRs labeled/branded, run CI + cursor-review — groom/groom-pr/cursor-review labels + bot author; opening as the bot App (required) triggers CI + the review panel.
[~] Excluded from merger auto-merge — done as far as this repo can: builder PRs are bot-authored and carry groom-pr (NOT agent-coded), so the agent-work merger (which lives in a different repo and keys on author == operator && agent-coded) already skips them. The actual merger-side skip rule can't be enforced from here — please confirm the merger excludes author == bot / label == groom-pr before enabling live on a repo the merger watches.
Groomer-only (issues) remains the default; builder opt-in via builder input.

Judgment calls (open items from the ticket — resolvable in review)

5 per run vs 1–2/day: implemented 5/run per Matt's 2026-07-21 decision (the ticket flags this supersedes the epic's earlier "1–2/day"). It's the max_prs input — trivially tunable.
Beyond-cap findings: chose hybrid — remainder (DOWNGRADE, security, beyond-cap CONFIRM) is still filed as groom issues, so nothing is silently dropped (security findings especially). The alternative "defer to next run" is a one-line change if preferred.
Ranking signal: used the verifier's emission order (the finder ranks ~6–12; the verifier preserves it) — zero brief change, lowest risk. A future explicit rank/score field in verifier.md can refine it.
Rollback / test-coverage (Hunter/Wei Hai): not addressed here (out of scope for the plumbing) — left for the pilot; builder PRs run full CI so an under-tested refactor is caught at review, and the builder brief tells the agent to update touched tests.
Patch-size bail-out: implemented — pr_size_limit (default 400 changed lines); an oversized or non-applying patch files an issue instead of a giant PR.

Riskiest lines & why they're safe

build_ledger now includes PRs (was an explicit skip). Safe because the marker check is unchanged: only PRs carrying a signature marker (= builder PRs) become records; a hand-labeled human PR is still ignored. History (BE-3874) cited inline — the skip predated the builder.
git diff --cached → git apply --index round-trip: verified locally against modify + add + delete; apply-failure falls back to filing an issue.
Credential boundary: the build agent job is contents: read with no GitHub token; the only job with write creds (build_pr) runs no agent and never executes repo code (it only git applys a diff). A prompt-injected builder can produce a bad patch, nothing more.

Falsification note (negative-claim check)

The one deny path added — builder=true requires bot_app_id → exit 1 — is input validation that tells the operator exactly how to enable the feature, not a capability denial (no "not supported / STOP" dead-end). The bail path preserves the capability by filing an issue. No #581-class dead-end.

Testing

python3 -m unittest discover -s .github/groom/tests → 46 pass (10 new for PR-state).
YAML validated; all 6 embedded Python heredocs compile; build_select partition + git diff/apply round-trip smoke-tested locally.
Not exercised: a live end-to-end CI run (needs the bot App + a real repo) — recommend piloting with dry_run: true first (RFC point 6).

Note on size

~700 lines, but a single cohesive feature (the three jobs are one atomic unit) and fully gated behind an opt-in flag — zero behavior change when builder is off.

_{🔄 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/55 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/22/2026 **Status:** ✅ Merged **Merged:** 7/22/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4003-groom-auto-builder` --- ### 📝 Commits (3) - [`8173fc2`](https://github.com/Comfy-Org/github-workflows/commit/8173fc2809ca362e41731db20c47d45f18bd03a7) feat(groom): opt-in auto-builder — top-N CONFIRMED findings become review-gated PRs (BE-4003) - [`2052e70`](https://github.com/Comfy-Org/github-workflows/commit/2052e70dc718056a0bffca89a02102244a73a321) fix(groom): least-privilege bot tokens, normalize pr_size_limit, doc issues:write (BE-4003) - [`08ad004`](https://github.com/Comfy-Org/github-workflows/commit/08ad0044714e80294ee52342f1233ddb226af936) fix(groom): harden auto-builder against review-panel findings (BE-4003) ### 📊 Changes **7 files changed** (+816 additions, -74 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+24 -5) ➕ `.github/groom/builder.md` (+16 -0) 📝 `.github/groom/ledger.py` (+73 -24) 📝 `.github/groom/tests/test_ledger.py` (+108 -5) 📝 `.github/workflows/groom.yml` (+582 -31) 📝 `AGENTS.md` (+12 -8) 📝 `README.md` (+1 -1) </details> ### 📄 Description ## ELI-5 The groomer finds tech-debt and files GitHub issues, but the team reviews at the **PR** level and never works the issues — so they're a dead end. This adds an **opt-in auto-builder** (split C of the groom epic): when a repo turns it on, the top few *confirmed* findings each run are turned into **real, reviewable PRs** (throttled to 5), while everything else still files as issues. The PRs run full CI + cursor-review and **are never auto-merged** — a human still presses the button. Off by default, so nothing changes unless you opt in. ## What changed - **`groom.yml` — three new jobs** behind `builder: true` (default false): - `build_select` — the single **dedup + throttle** authority (no agent). Dedups verified findings against the durable ledger, then splits them: top `max_prs` (default 5) **CONFIRM, non-security** findings → build queue; the rest (DOWNGRADE, security, beyond-cap) → issue remainder. In builder-off mode it's a pass-through, so the issue path is byte-identical to before. - `build` — the **credential-free builder** (matrix, one cell per queued finding). `permissions: contents: read`, only `ANTHROPIC_API_KEY`. An agent writes the code change into its own fresh checkout; a deterministic step captures the working-tree diff as a **patch artifact** and enforces the `pr_size_limit` bail-out. Mirrors the finder/verifier isolation exactly. - `build_pr` — the **credentialed PR job** (no agent). Mints the bot token, applies the patch on a branch, opens a **branded, review-gated PR** (labels `groom` + `groom-pr` + `cursor-review`, a loud "NOT auto-merged" banner, the verifier rationale, and the signature marker). On a bail/oversized/non-applying patch it files a `groom` **issue** instead so nothing is dropped. Honors `dry_run` (preview, opens nothing). - **`ledger.py` — PR-state added.** The `/issues` listing already returns groom-labeled PRs; builder PRs carry the same signature marker in their body, so the ledger now classifies them: open → `pr-open`, merged → `merged`, closed-unmerged → `pr-closed`. All suppress re-proposing → **a merged/closed finding is never re-proposed** (RFC point 3). The markerless-record skip still guards against poisoning. (BE-3874 skipped all PRs because groom filed only issues then — cited in the code.) - **`builder.md`** — new phase-3 brief (scope-disciplined, keep-it-green, never-touch-security, patch-size bail). - Tests + README/AGENTS.md updated. ## Acceptance criteria - [x] ≤5 verifier-top findings → PRs; remainder not built — `build_select` caps `to_build` at `max_prs`. - [x] Builder holds no creds, emits only a patch artifact; PR creation in a separate job as the bot — `build` (contents: read) → artifact → `build_pr` (bot token). - [x] Ledger prevents re-proposing a merged/closed/rejected finding — PR-state in `ledger.py`, **regression-tested** (`test_{open,merged,closed}_builder_pr_*` in `test_ledger.py`). - [x] Builder PRs labeled/branded, run CI + cursor-review — `groom`/`groom-pr`/`cursor-review` labels + bot author; opening as the bot App (required) triggers CI + the review panel. - [~] **Excluded from merger auto-merge** — done as far as *this* repo can: builder PRs are **bot-authored** and carry `groom-pr` (NOT `agent-coded`), so the agent-work merger (which lives in a **different repo** and keys on `author == operator && agent-coded`) already skips them. The actual merger-side skip rule can't be enforced from here — **please confirm the merger excludes `author == bot` / `label == groom-pr`** before enabling live on a repo the merger watches. - [x] Groomer-only (issues) remains the default; builder opt-in via `builder` input. ## Judgment calls (open items from the ticket — resolvable in review) 1. **5 per run vs 1–2/day:** implemented **5/run** per Matt's 2026-07-21 decision (the ticket flags this supersedes the epic's earlier "1–2/day"). It's the `max_prs` input — trivially tunable. 2. **Beyond-cap findings:** chose **hybrid** — remainder (DOWNGRADE, security, beyond-cap CONFIRM) is still filed as `groom` issues, so nothing is silently dropped (security findings especially). The alternative "defer to next run" is a one-line change if preferred. 3. **Ranking signal:** used the **verifier's emission order** (the finder ranks ~6–12; the verifier preserves it) — zero brief change, lowest risk. A future explicit `rank`/score field in `verifier.md` can refine it. 4. **Rollback / test-coverage (Hunter/Wei Hai):** not addressed here (out of scope for the plumbing) — left for the pilot; builder PRs run full CI so an under-tested refactor is caught at review, and the builder brief tells the agent to update touched tests. 5. **Patch-size bail-out:** implemented — `pr_size_limit` (default 400 changed lines); an oversized or non-applying patch files an issue instead of a giant PR. ## Riskiest lines & why they're safe - **`build_ledger` now includes PRs** (was an explicit skip). Safe because the **marker check** is unchanged: only PRs carrying a signature marker (= builder PRs) become records; a hand-labeled human PR is still ignored. History (BE-3874) cited inline — the skip predated the builder. - **`git diff --cached` → `git apply --index`** round-trip: verified locally against modify + add + delete; apply-failure falls back to filing an issue. - **Credential boundary:** the `build` agent job is `contents: read` with no GitHub token; the only job with write creds (`build_pr`) runs no agent and never executes repo code (it only `git apply`s a diff). A prompt-injected builder can produce a bad patch, nothing more. ## Falsification note (negative-claim check) The one deny path added — `builder=true requires bot_app_id → exit 1` — is **input validation** that tells the operator exactly how to enable the feature, not a capability denial (no "not supported / STOP" dead-end). The bail path *preserves* the capability by filing an issue. No #581-class dead-end. ## Testing - `python3 -m unittest discover -s .github/groom/tests` → **46 pass** (10 new for PR-state). - YAML validated; all 6 embedded Python heredocs compile; `build_select` partition + `git diff`/`apply` round-trip smoke-tested locally. - Not exercised: a live end-to-end CI run (needs the bot App + a real repo) — recommend piloting with `dry_run: true` first (RFC point 6). ## Note on size ~700 lines, but a single cohesive feature (the three jobs are one atomic unit) and **fully gated behind an opt-in flag** — zero behavior change when `builder` is off. --- <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:51 +00:00