[PR #109] [MERGED] feat(groom): bail_sink — suppress builder bail issues, and document what max_findings actually caps (BE-6157) #120

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/109
Author: @mattmillerai
Created: 8/2/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-6157-groom-bail-sink

📝 Commits (3)

df3b281 feat(groom): bail_sink — suppress builder bail issues, and document what max_findings actually caps (BE-6157)
d44030f fix(groom): sanitize the bail signature, exempt the secret-scan withhold from bail_sink=none (BE-6157)
b3ee61e Merge remote-tracking branch 'origin/main' into matt/be-6157-groom-bail-sink

📊 Changes

5 files changed (+354 additions, -18 deletions)

View changed files

📝 .github/groom/README.md (+36 -0)
📝 .github/groom/config.py (+62 -0)
📝 .github/groom/tests/test_config.py (+125 -0)
📝 .github/workflows/groom.yml (+130 -17)
📝 README.md (+1 -1)

📄 Description

ELI-5

Groom has a knob called max_findings that says it caps "the number of NEW issues opened in one run". Someone set it to 0 on a repo, expecting no issues — and got one anyway. The knob wasn't broken: it caps the findings the file job opens, while the auto-builder files a different kind of issue from a different job when a patch is too big to open as a PR. The setting did what it documented; it did not do what it implied. This PR says so out loud in the docs, and adds bail_sink — the knob that actually silences those builder bail issues, settable live from GROOM_CONFIG with no PR.

What changed

1. Documentation (the trap itself). max_findings' description in groom.yml now names its scope (the file job's findings issues, after dedup) and explicitly disclaims builder bail issues, pointing at bail_sink. Mirrored in .github/groom/config.py's _OPERATIONAL_KEYS comment, .github/groom/README.md (new "Builder bail-outs" section) and the root README catalog row.

2. New bail_sink input, default issue — today's behavior, unchanged for every current caller.

issue (default) — a bail files a groom-labeled issue, exactly as now.
none — files nothing. Instead build_pr emits ::warning::bail_sink=none — NOT filing a bail issue for '<title>' (idx=N, signature=<sig>): <reason> and appends a run-summary line, so a suppressed bail is visible in the run, not invisible.
linear is reserved for the sibling sink: linear ticket and is not implemented here. It is refused by name, with its own warning, and falls back to issue.

3. Placement: _OPERATIONAL_KEYS, not _LOCKED_KEYS — per the ticket. bail_sink can only make groom quieter, never grant it privilege, so a repo can set it via vars.GROOM_CONFIG without a PR. That is the whole point: the operator who set max_findings: 0 can get real silence the same way. pr_size_limit stays locked (a reviewed commit in the caller) — the config.py docstring now spells out that suppressing the bail is not a reason to unlock raising the ceiling (ticket item 3).

4. Single allowlist. config.py owns BAIL_SINKS and exports normalize_bail_sink, which build_pr's inline Python imports rather than re-deriving the list. Everything not provably none — empty, unknown, linear, a dropped-by-coercion value — resolves to issue. The fail-safe direction is deliberate: a redundant issue is recoverable, a silently discarded CONFIRMED finding is not.

Verification

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' — 183 passed (was 170; +13 for bail_sink). Also ran the cursor-review and agents-md suites, shellcheck on bump-callers, and check_agents_md.py --root . — all green (the CODEOWNERS warning is pre-existing).
actionlint clean on groom.yml and test-groom-scripts.yml; the workflow parses and all six inline Python blocks compile.
The suppress path was exercised, not just asserted. build_pr's bail branch is inline Python inside a YAML run: block, so it cannot be imported by the unit suite. I extracted it into a scratch harness with a fake decision/result and a stub gh on PATH, and ran it four ways: BAIL_SINK=none → warning + summary line, no gh invocation, exit 0; unset → filed the issue exactly as before, no summary line; linear → filed (fail-safe); NONE → suppressed (case-insensitive). A model-authored title containing a newline and a literal ::error:: was flattened onto one line, as intended.
Unit tests cover the allowlist coercion, normalize_bail_sink's fail-safe defaulting, and — via a new TestBailSinkWiring class that reads groom.yml — the four wiring points a silent regression would live in (the input + its issue default, the defaults-layer entry, the BAIL_SINK: env read, the if bail_sink == "none": branch and its warning). test-groom-scripts.yml's path filter now also watches .github/workflows/groom.yml so a workflow-only edit that unhooks one of those actually runs the suite.

Judgment calls / corrections worth reading

The ticket says a suppressed bail leaves "the patch artifact still uploaded, so it is recoverable from the run." That is not quite true today, and I did not change it. The bail() helper in the build job truncates /tmp/out/patch.diff before upload — deliberately, because the same helper carries the secret-scan withhold, where publishing the patch is the thing being prevented. Preserving the patch on the size/CI-privileged bails only would change artifact contents for current callers, against the "byte-identical" criterion. So the recovery path for bail_sink: none is the ::warning:: (title + bail reason + dedup signature) and the run-summary line, not the patch bytes. The docs I wrote say exactly that; they do not claim the patch is recoverable.
none suppresses every bail, including the prompt-injection secret-scan withhold. I kept it uniform rather than carving out an exception: a carve-out needs a machine-readable bail kind in result.json, which is scope the ticket did not ask for, and the secret-scan path already prints its own ::error:: on the run page independently. Called out in the input description and both READMEs.
none means the finding is not recorded in the ledger, so a later run can re-propose and re-bail on it, re-spending builder tokens. Unavoidable without filing something — a durable record is what the linear sink phase is for. Documented on the input so nobody discovers it from a bill.
from config import normalize_bail_sink couples build_pr to the workflows_ref assets. A caller that pins uses: to this SHA but workflows_ref: to an older one gets an ImportError. This is a pre-existing coupling, not a new one — the same job already imports builder_pr_body from ledger.py at that path — the bumper moves both pins together, and the import runs before any git push, so it cannot orphan a branch.
Small drive-by: the two existing GITHUB_STEP_SUMMARY writes in build_pr were folded into the summarize() helper the new path needed. Behavior-identical (verified in the harness above); it avoids a third copy of the same four lines.

_{🔄 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/109 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/2/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6157-groom-bail-sink` --- ### 📝 Commits (3) - [`df3b281`](https://github.com/Comfy-Org/github-workflows/commit/df3b281c41381df140bb9f2c54ff50fb70cddda9) feat(groom): bail_sink — suppress builder bail issues, and document what max_findings actually caps (BE-6157) - [`d44030f`](https://github.com/Comfy-Org/github-workflows/commit/d44030f88af2795e971b7fd7dce96420dce64e14) fix(groom): sanitize the bail signature, exempt the secret-scan withhold from bail_sink=none (BE-6157) - [`b3ee61e`](https://github.com/Comfy-Org/github-workflows/commit/b3ee61e380d7423d422b285baf9513b135d439a5) Merge remote-tracking branch 'origin/main' into matt/be-6157-groom-bail-sink ### 📊 Changes **5 files changed** (+354 additions, -18 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+36 -0) 📝 `.github/groom/config.py` (+62 -0) 📝 `.github/groom/tests/test_config.py` (+125 -0) 📝 `.github/workflows/groom.yml` (+130 -17) 📝 `README.md` (+1 -1) </details> ### 📄 Description ## ELI-5 Groom has a knob called `max_findings` that says it caps "the number of NEW issues opened in one run". Someone set it to `0` on a repo, expecting no issues — and got one anyway. The knob wasn't broken: it caps the *findings* the `file` job opens, while the auto-builder files a *different* kind of issue from a *different* job when a patch is too big to open as a PR. The setting did what it documented; it did not do what it implied. This PR says so out loud in the docs, and adds `bail_sink` — the knob that actually silences those builder bail issues, settable live from `GROOM_CONFIG` with no PR. ## What changed **1. Documentation (the trap itself).** `max_findings`' description in `groom.yml` now names its scope (the `file` job's *findings* issues, after dedup) and explicitly disclaims builder bail issues, pointing at `bail_sink`. Mirrored in `.github/groom/config.py`'s `_OPERATIONAL_KEYS` comment, `.github/groom/README.md` (new "Builder bail-outs" section) and the root README catalog row. **2. New `bail_sink` input**, default `issue` — today's behavior, unchanged for every current caller. - `issue` (default) — a bail files a `groom`-labeled issue, exactly as now. - `none` — files nothing. Instead `build_pr` emits `::warning::bail_sink=none — NOT filing a bail issue for '<title>' (idx=N, signature=<sig>): <reason>` and appends a run-summary line, so a suppressed bail is *visible in the run*, not invisible. - `linear` is reserved for the sibling `sink: linear` ticket and is **not** implemented here. It is refused by name, with its own warning, and falls back to `issue`. **3. Placement: `_OPERATIONAL_KEYS`, not `_LOCKED_KEYS`** — per the ticket. `bail_sink` can only make groom quieter, never grant it privilege, so a repo can set it via `vars.GROOM_CONFIG` without a PR. That is the whole point: the operator who set `max_findings: 0` can get real silence the same way. `pr_size_limit` stays **locked** (a reviewed commit in the caller) — the config.py docstring now spells out that suppressing the bail is not a reason to unlock raising the ceiling (ticket item 3). **4. Single allowlist.** `config.py` owns `BAIL_SINKS` and exports `normalize_bail_sink`, which `build_pr`'s inline Python imports rather than re-deriving the list. Everything not provably `none` — empty, unknown, `linear`, a dropped-by-coercion value — resolves to `issue`. The fail-safe direction is deliberate: a redundant issue is recoverable, a silently discarded CONFIRMED finding is not. ## Verification - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` — **183 passed** (was 170; +13 for `bail_sink`). Also ran the cursor-review and agents-md suites, `shellcheck` on bump-callers, and `check_agents_md.py --root .` — all green (the CODEOWNERS warning is pre-existing). - `actionlint` clean on `groom.yml` and `test-groom-scripts.yml`; the workflow parses and all six inline Python blocks compile. - **The suppress path was exercised, not just asserted.** `build_pr`'s bail branch is inline Python inside a YAML `run:` block, so it cannot be imported by the unit suite. I extracted it into a scratch harness with a fake decision/result and a stub `gh` on PATH, and ran it four ways: `BAIL_SINK=none` → warning + summary line, **no `gh` invocation**, exit 0; unset → filed the issue exactly as before, no summary line; `linear` → filed (fail-safe); `NONE` → suppressed (case-insensitive). A model-authored title containing a newline and a literal `::error::` was flattened onto one line, as intended. - Unit tests cover the allowlist coercion, `normalize_bail_sink`'s fail-safe defaulting, and — via a new `TestBailSinkWiring` class that reads `groom.yml` — the four wiring points a silent regression would live in (the input + its `issue` default, the defaults-layer entry, the `BAIL_SINK:` env read, the `if bail_sink == "none":` branch and its warning). `test-groom-scripts.yml`'s path filter now also watches `.github/workflows/groom.yml` so a workflow-only edit that unhooks one of those actually runs the suite. ## Judgment calls / corrections worth reading - **The ticket says a suppressed bail leaves "the patch artifact still uploaded, so it is recoverable from the run." That is not quite true today, and I did not change it.** The `bail()` helper in the `build` job truncates `/tmp/out/patch.diff` *before* upload — deliberately, because the same helper carries the secret-scan withhold, where publishing the patch is the thing being prevented. Preserving the patch on the size/CI-privileged bails only would change artifact contents for current callers, against the "byte-identical" criterion. So the recovery path for `bail_sink: none` is the `::warning::` (title + bail reason + dedup signature) and the run-summary line, not the patch bytes. The docs I wrote say exactly that; they do not claim the patch is recoverable. - **`none` suppresses *every* bail, including the prompt-injection secret-scan withhold.** I kept it uniform rather than carving out an exception: a carve-out needs a machine-readable bail *kind* in `result.json`, which is scope the ticket did not ask for, and the secret-scan path already prints its own `::error::` on the run page independently. Called out in the input description and both READMEs. - **`none` means the finding is not recorded in the ledger**, so a later run can re-propose and re-bail on it, re-spending builder tokens. Unavoidable without filing something — a durable record is what the `linear` sink phase is for. Documented on the input so nobody discovers it from a bill. - **`from config import normalize_bail_sink` couples `build_pr` to the `workflows_ref` assets.** A caller that pins `uses:` to this SHA but `workflows_ref:` to an older one gets an ImportError. This is a pre-existing coupling, not a new one — the same job already imports `builder_pr_body` from `ledger.py` at that path — the bumper moves both pins together, and the import runs before any `git push`, so it cannot orphan a branch. - **Small drive-by:** the two existing `GITHUB_STEP_SUMMARY` writes in `build_pr` were folded into the `summarize()` helper the new path needed. Behavior-identical (verified in the harness above); it avoids a third copy of the same four lines. --- <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:26 +00:00