feat(groom): bail_sink — suppress builder bail issues, and document what max_findings actually caps (BE-6157) - #109
feat(groom): bail_sink — suppress builder bail issues, and document what max_findings actually caps (BE-6157)#109mattmillerai wants to merge 2 commits into
Conversation
…hat max_findings actually caps (BE-6157) max_findings is documented as a cap on "NEW issues opened in one run", but it only slices the file job's findings list. Builder BAIL issues — a CONFIRMED finding whose patch exceeded pr_size_limit or touched a CI-privileged path — are opened by build_pr and were never capped, so an operator who set max_findings: 0 expecting silence still got an issue. - Amend the max_findings description (plus config.py and both READMEs) to name its scope and disclaim the bail path. - Add a bail_sink input, default `issue` (byte-identical behavior). `none` files nothing and instead emits a ::warning:: naming the finding, its bail reason and its signature, plus a run-summary line, so a suppressed bail is visible in the run rather than invisible. - bail_sink is an _OPERATIONAL_KEY, not a locked one: it can only make groom quieter, so GROOM_CONFIG can set it with no PR. pr_size_limit stays locked. - config.py owns the allowlist; build_pr imports normalize_bail_sink so the two cannot disagree, and any unrecognized value (including the reserved `linear`) resolves back to `issue` — never to silent suppression.
mattmillerai
added
cursor-review
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…old from bail_sink=none (BE-6157) Review-panel follow-ups on the bail_sink knob: - The dedup `signature` is as model-authored as `title`/`reason` (the verifier schema does not require it, and `normalize_signature` only strips SURROUNDING whitespace), so an interior newline in it ended the `::warning::` and let the remainder pose as a fresh workflow command in a job holding a write-scoped bot token. It now goes through `oneline` in both the annotation and the run summary, as do the remaining raw `title`/`reason` emissions on the dry-run and built-PR paths — the same class, one sink away. - `bail_sink: none` no longer suppresses the pre-publish secret-scan withhold. `bail_sink` is classified OPERATIONAL (settable from vars.GROOM_CONFIG with no PR) on the grounds that it can only make groom quieter, never grant it privilege — and erasing the durable record of a possible key-exfil attempt, leaving only an `::error::` in expiring run logs, is where "quieter" stopped being merely quieter. The carve-out rides on a `"withheld": true` machine field in result.json, not a substring match on the prose reason, so rewording the bail message cannot silently disarm it. - The `if not sig:` producer-error guard moves back ahead of the sink branch: a signatureless finding is a schema failure, not an operator's suppression. - Docs: `none` starves the build queue, not just tokens — a DETERMINISTIC bail re-bails every run and permanently holds a `max_prs` slot (at `max_prs: 1` nothing else is ever built). And an unrecognized value falls back to the CALLER's value, not unconditionally to `issue`. Tests pin all three new invariants (withhold exemption end to end, guard ordering, signature sanitization).
2 participants
ELI-5
Groom has a knob called
max_findingsthat says it caps "the number of NEW issues opened in one run". Someone set it to0on a repo, expecting no issues — and got one anyway. The knob wasn't broken: it caps the findings thefilejob 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 addsbail_sink— the knob that actually silences those builder bail issues, settable live fromGROOM_CONFIGwith no PR.What changed
1. Documentation (the trap itself).
max_findings' description ingroom.ymlnow names its scope (thefilejob's findings issues, after dedup) and explicitly disclaims builder bail issues, pointing atbail_sink. Mirrored in.github/groom/config.py's_OPERATIONAL_KEYScomment,.github/groom/README.md(new "Builder bail-outs" section) and the root README catalog row.2. New
bail_sinkinput, defaultissue— today's behavior, unchanged for every current caller.issue(default) — a bail files agroom-labeled issue, exactly as now.none— files nothing. Insteadbuild_premits::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.linearis reserved for the siblingsink: linearticket and is not implemented here. It is refused by name, with its own warning, and falls back toissue.3. Placement:
_OPERATIONAL_KEYS, not_LOCKED_KEYS— per the ticket.bail_sinkcan only make groom quieter, never grant it privilege, so a repo can set it viavars.GROOM_CONFIGwithout a PR. That is the whole point: the operator who setmax_findings: 0can get real silence the same way.pr_size_limitstays 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.pyownsBAIL_SINKSand exportsnormalize_bail_sink, whichbuild_pr's inline Python imports rather than re-deriving the list. Everything not provablynone— empty, unknown,linear, a dropped-by-coercion value — resolves toissue. 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 forbail_sink). Also ran the cursor-review and agents-md suites,shellcheckon bump-callers, andcheck_agents_md.py --root .— all green (the CODEOWNERS warning is pre-existing).actionlintclean ongroom.ymlandtest-groom-scripts.yml; the workflow parses and all six inline Python blocks compile.build_pr's bail branch is inline Python inside a YAMLrun:block, so it cannot be imported by the unit suite. I extracted it into a scratch harness with a fake decision/result and a stubghon PATH, and ran it four ways:BAIL_SINK=none→ warning + summary line, noghinvocation, 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.normalize_bail_sink's fail-safe defaulting, and — via a newTestBailSinkWiringclass that readsgroom.yml— the four wiring points a silent regression would live in (the input + itsissuedefault, the defaults-layer entry, theBAIL_SINK:env read, theif bail_sink == "none":branch and its warning).test-groom-scripts.yml's path filter now also watches.github/workflows/groom.ymlso a workflow-only edit that unhooks one of those actually runs the suite.Judgment calls / corrections worth reading
bail()helper in thebuildjob truncates/tmp/out/patch.diffbefore 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 forbail_sink: noneis 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.nonesuppresses 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 inresult.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.nonemeans 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 thelinearsink phase is for. Documented on the input so nobody discovers it from a bill.from config import normalize_bail_sinkcouplesbuild_prto theworkflows_refassets. A caller that pinsuses:to this SHA butworkflows_ref:to an older one gets an ImportError. This is a pre-existing coupling, not a new one — the same job already importsbuilder_pr_bodyfromledger.pyat that path — the bumper moves both pins together, and the import runs before anygit push, so it cannot orphan a branch.GITHUB_STEP_SUMMARYwrites inbuild_prwere folded into thesummarize()helper the new path needed. Behavior-identical (verified in the harness above); it avoids a third copy of the same four lines.