From df3b281c41381df140bb9f2c54ff50fb70cddda9 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 2 Aug 2026 14:50:16 -0700 Subject: [PATCH 1/2] =?UTF-8?q?feat(groom):=20bail=5Fsink=20=E2=80=94=20su?= =?UTF-8?q?ppress=20builder=20bail=20issues,=20and=20document=20what=20max?= =?UTF-8?q?=5Ffindings=20actually=20caps=20(BE-6157)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/groom/README.md | 31 +++++++ .github/groom/config.py | 62 +++++++++++++- .github/groom/tests/test_config.py | 95 +++++++++++++++++++++ .github/workflows/groom.yml | 102 ++++++++++++++++++++--- .github/workflows/test-groom-scripts.yml | 6 ++ README.md | 2 +- 6 files changed, 285 insertions(+), 13 deletions(-) diff --git a/.github/groom/README.md b/.github/groom/README.md index bda8ae3..7053831 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -28,6 +28,37 @@ ledger's PR-state stops that finding from being re-proposed. The builder holds n credentials — it can only produce a *patch*, never push. Default off: the finds-only groomer (issues) stays the default. +### Builder bail-outs, and what `max_findings` does *not* cover (BE-6157) + +A build that cannot become a PR **bails**: the builder produced no patch, the +patch exceeds `pr_size_limit`, the patch touches a CI-privileged path +(`.github/workflows|actions/`, build/test config), the patch does not apply, or +the pre-publish secret scan withheld it. By default the bail is filed as a `groom` +issue, so a CONFIRMED finding the builder already spent tokens on is handed to a +human rather than discarded. + +Two things follow that are easy to get wrong: + +- **`max_findings` does not govern bail issues.** It caps the NEW **findings** + issues the `file` job opens after dedup — a flood backstop, nothing more. Bail + issues are opened by the separate `build_pr` job, so `max_findings: 0` silences + the findings path and a bail issue can still appear. That is deliberate (losing + paid-for work is worse than one extra issue), and it was surprising enough in + practice to be worth stating twice. +- **`bail_sink` is the knob for the bail path.** `issue` (default) keeps the + behavior above; `none` files nothing and instead emits a `::warning::` naming + the finding, its bail reason and its signature, plus a run-summary line — so the + bail is visible in the run rather than invisible. `none` suppresses *every* + bail, including the secret-scan withhold (which still prints its own + `::error::`), and because no issue is filed, no signature marker is recorded, + so a later run may re-propose and re-bail on that finding. + +`bail_sink` is an **operational** knob (`vars.GROOM_CONFIG` can set it with no +PR), unlike `sink` / `pr_size_limit` / `builder`, which stay in the reviewed +workflow file. If bails are frequent because well-scoped patches keep landing +just over the line, the real fix is usually raising `pr_size_limit` **in the +caller** — a reviewed commit, by design — not suppressing the signal. + These two files are the **single source of truth** for the groom prompts, the same way [`.github/cursor-review/`](../cursor-review) is for the review panel. The core thesis of the groom initiative is *collaborate on the prompt, not the diff --git a/.github/groom/config.py b/.github/groom/config.py index 00834fe..d97334d 100644 --- a/.github/groom/config.py +++ b/.github/groom/config.py @@ -34,7 +34,14 @@ reproducibility contract, not an operational dial. Everything else (`_OPERATIONAL_KEYS`) is fair game: it can only make groom -scan less, propose less, or file less — never grant it more privilege. +scan less, propose less, or file less — never grant it more privilege. That is +why `bail_sink` (BE-6157) is operational even though its sibling `sink` is +locked: `sink` picks the credentialed backend every finding is filed through, +while `bail_sink` only chooses whether a builder BAIL becomes an issue or just a +warning. Note the corollary for `pr_size_limit`, which is the knob an operator +usually reaches for after a near-miss bail: it stays LOCKED (a reviewed commit in +the caller) because it is the unreviewably-large-patch backstop — suppressing the +bail is not a reason to unlock raising the ceiling. The lock applies to the VARIABLE and `config` layers, not to the caller-defaults layer, which is the reviewed workflow file itself — the thing the lock protects. @@ -85,7 +92,15 @@ # authority for max_prs (see the input's description in groom.yml), and # duplicating the clamp here would give two places to disagree. "max_prs": "numeric_string", + # Caps the `file` job's FINDINGS issues only. It does NOT govern the + # auto-builder's bail issues, which `build_pr` files for a CONFIRMED finding + # whose patch was too large or CI-privileged (BE-6157) — so `max_findings: 0` + # is not "open no issues". `bail_sink` is the knob for those. "max_findings": "nonneg_int", + # Operational, NOT locked, on purpose: it can only make groom quieter, never + # grant it privilege, so the operator who set `max_findings: 0` can get real + # silence through the same variable instead of a PR (BE-6157). + "bail_sink": "bail_sink", # Stay strings: interval.py is the single normalization authority, and it # deliberately degrades blank/garbage/negative to 7 rather than failing. "interval_days": "numeric_string", @@ -298,6 +313,50 @@ def _coerce_scope_label(key, value): return cleaned +# The bail sinks that are actually IMPLEMENTED (BE-6157). `linear` is deliberately +# absent: it is the sibling `sink: linear` phase's job, and accepting it here would +# resolve to a sink nothing implements — i.e. silent suppression under a name that +# promises filing. Keep this list and `build_pr`'s behavior in lockstep; the +# workflow imports `normalize_bail_sink` rather than re-deriving the allowlist. +BAIL_SINKS = ("issue", "none") +DEFAULT_BAIL_SINK = "issue" + + +def normalize_bail_sink(raw): + """Map a resolved (or absent) `bail_sink` to an implemented sink. + + Called from `build_pr`'s inline Python with the env value, which is EMPTY + whenever the coercer below dropped a bad value — an unrecognized value must + resolve to `issue`, never to `none`: filing a redundant issue is recoverable, + silently discarding a CONFIRMED finding is not. + """ + value = str(raw or "").strip().lower() + return value if value in BAIL_SINKS else DEFAULT_BAIL_SINK + + +def _coerce_bail_sink(key, value): + """One of `BAIL_SINKS`, case- and whitespace-insensitive. + + `linear` gets its own warning rather than the generic one: it is a documented + later phase, so an operator who sets it is making a reasonable mistake and + deserves to be told which, not just that the value was refused. + """ + text = value.strip().lower() if isinstance(value, str) else "" + if text in BAIL_SINKS: + return text + if text == "linear": + _warn( + f"{key}='linear' is reserved for the Linear sink phase and is not implemented — " + "ignoring it (using the caller's value)." + ) + return None + _warn( + f"{key}={_shown(value)} is not one of {'/'.join(BAIL_SINKS)} — ignoring it " + "(using the caller's value)." + ) + return None + + def _coerce_model(key, value): if not isinstance(value, str) or not _MODEL_RE.match(value.strip()): _warn(f"{key}={_shown(value)} is not a valid model id — ignoring it.") @@ -313,6 +372,7 @@ def _coerce_model(key, value): "prose_nonblank": _coerce_prose_nonblank, "scope_label": _coerce_scope_label, "model": _coerce_model, + "bail_sink": _coerce_bail_sink, } diff --git a/.github/groom/tests/test_config.py b/.github/groom/tests/test_config.py index e815cb2..d616325 100644 --- a/.github/groom/tests/test_config.py +++ b/.github/groom/tests/test_config.py @@ -9,6 +9,7 @@ import io import json import os +import re import sys import unittest import unittest.mock @@ -285,6 +286,100 @@ def test_invalid_shape_refused(self): self.assertNotIn("scope_label", resolve({}, '{"scope_label": "has spaces!"}')) +class TestBailSink(unittest.TestCase): + """bail_sink (BE-6157) — the knob `max_findings: 0` never was. + + Two halves: the ALLOWLIST coercion here, and `normalize_bail_sink`, which is + what `build_pr`'s inline Python actually calls to decide whether to suppress. + """ + + def test_operational_not_locked(self): + """The whole point: a repo can get silence via the variable, no PR.""" + self.assertIn("bail_sink", config._OPERATIONAL_KEYS) + self.assertNotIn("bail_sink", config._LOCKED_KEYS) + + def test_variable_can_suppress_bail_issues(self): + got = resolve({"bail_sink": "issue"}, '{"bail_sink": "none"}') + self.assertEqual(got["bail_sink"], "none") + + def test_case_and_whitespace_insensitive(self): + for raw in ('{"bail_sink": "NONE"}', '{"bail_sink": " none "}'): + self.assertEqual(resolve({}, raw)["bail_sink"], "none", raw) + + def test_linear_is_refused_by_name(self): + """Reserved for the sibling sink phase — must not resolve to silence.""" + self.assertEqual(resolve({"bail_sink": "issue"}, '{"bail_sink": "linear"}')["bail_sink"], + "issue") + self.assertIn("not implemented", warnings_from({}, '{"bail_sink": "linear"}')) + + def test_unknown_value_keeps_the_callers_value(self): + for bad in ('{"bail_sink": "silent"}', '{"bail_sink": 0}', '{"bail_sink": true}'): + self.assertEqual(resolve({"bail_sink": "issue"}, bad)["bail_sink"], "issue", bad) + + def test_normalize_defaults_to_issue(self): + """Every not-provably-`none` input must file — losing a CONFIRMED + finding silently is the one failure mode worth engineering against.""" + for raw in (None, "", " ", "linear", "silent", "issues", 0, False): + self.assertEqual(config.normalize_bail_sink(raw), "issue", repr(raw)) + + def test_normalize_suppresses_only_on_none(self): + for raw in ("none", "NONE", " None "): + self.assertEqual(config.normalize_bail_sink(raw), "none", repr(raw)) + + def test_dropped_value_reaches_build_pr_as_issue(self): + """End-to-end of the fail-safe: a typo'd variable drops the key from + `resolved`, so `fromJSON(...).bail_sink` renders EMPTY in the workflow — + and empty must mean `issue`, not `none`.""" + resolved = resolve({}, '{"bail_sink": "nonw"}') + self.assertNotIn("bail_sink", resolved) + self.assertEqual(config.normalize_bail_sink(resolved.get("bail_sink")), "issue") + + +class TestBailSinkWiring(unittest.TestCase): + """The half of the suppress path that lives in groom.yml (BE-6157). + + `build_pr`'s bail branch is inline Python inside a YAML `run:` block, so it + cannot be imported and unit-tested directly. What CAN be pinned is the wiring + it depends on — an input, a defaults-layer entry, the env read, and the early + return — because every one of them is a silent failure if it goes missing: a + dropped `BAIL_SINK:` line leaves `bail_sink: none` accepted by config.py and + ignored by the job, which reads as "the knob does nothing". + """ + + @classmethod + def setUpClass(cls): + path = os.path.join(os.path.dirname(__file__), "..", "..", "workflows", "groom.yml") + with open(path, encoding="utf-8") as f: + cls.wf = f.read() + + def test_input_exists_and_defaults_to_issue(self): + """Default `issue` is the back-compat promise for every current caller.""" + self.assertRegex(self.wf, r"(?s)\n bail_sink:\n.*?\n default: issue\n") + + def test_input_is_in_the_defaults_layer(self): + """Without this the reviewed `with:` value never reaches config.py.""" + self.assertRegex(self.wf, r'"bail_sink":\s*\$\{\{\s*toJSON\(inputs\.bail_sink\)\s*\}\}') + + def test_build_pr_reads_the_resolved_value(self): + self.assertRegex( + self.wf, + r"BAIL_SINK:\s*\$\{\{\s*fromJSON\(needs\.gate\.outputs\.resolved\)\.bail_sink\s*\}\}", + ) + + def test_build_pr_suppresses_and_warns(self): + self.assertIn("from config import normalize_bail_sink", self.wf) + self.assertIn('bail_sink = normalize_bail_sink(os.environ.get("BAIL_SINK"))', self.wf) + self.assertIn('if bail_sink == "none":', self.wf) + # Suppressed must still be VISIBLE — the annotation is the recovery path. + self.assertIn("::warning::bail_sink=none", self.wf) + + def test_max_findings_description_disclaims_bail_issues(self): + """The documentation half of the ticket, kept from silently rotting.""" + block = re.search(r"(?s)\n max_findings:\n(.*?)\n type:", self.wf).group(1) + self.assertIn("bail_sink: none", block) + self.assertIn("does not govern", block.lower()) + + class TestCliOutputShape(unittest.TestCase): def test_stdout_is_exactly_one_line_of_json(self): """It is written verbatim as one $GITHUB_OUTPUT line.""" diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index a956994..169501a 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -115,6 +115,12 @@ name: Groom (reusable) # # No fromJSON() and no allowlist re-check are needed — max_prs is a # # `string` input and the reusable does the parse/clamp (see its # # description). That is the whole reason it is not `type: number`. +# # A build that can't become a PR (patch over `pr_size_limit`, patch +# # touching CI-privileged paths, …) files a `groom` issue instead so the +# # work isn't lost — that path is NOT capped by `max_findings`. To make +# # a builder run open PRs and nothing else, add `bail_sink: none` (it is +# # also settable live via vars.GROOM_CONFIG — see the input). +# # bail_sink: none # secrets: # ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} # BOT_APP_PRIVATE_KEY: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} @@ -213,9 +219,17 @@ on: default: github max_findings: description: >- - Cap on the number of NEW issues opened in one run (after dedup). A - backstop against a surprise flood; the finder brief already targets - ~6-12 high-precision findings. + Cap on the number of NEW **findings** issues opened in one run (after + dedup) — i.e. the `file` job's output. A backstop against a surprise + flood; the finder brief already targets ~6-12 high-precision findings. + + + It does NOT govern the auto-builder's BAIL issues (BE-6157). Those come + from a different job (`build_pr`): a CONFIRMED finding whose patch + exceeds `pr_size_limit` or touches a CI-privileged path still files an + issue, so work the builder already paid for is not silently discarded. + So `max_findings: 0` means "file no FINDINGS", not "open no issues" — + set `bail_sink: none` to suppress the bail issues too. type: number required: false default: 12 @@ -345,6 +359,39 @@ on: type: number required: false default: 400 + bail_sink: + description: >- + Where the auto-builder's BAIL issues go (BE-6157). A CONFIRMED finding + the builder could not turn into a PR — no patch produced, patch over + `pr_size_limit`, patch touching a CI-privileged path, patch that would + not apply, or the pre-publish secret scan withholding it — is filed as a + `groom` issue so a completed, already-paid-for audit is not thrown away. + That path lives in `build_pr`, NOT the `file` job, so `max_findings` + does not cap it and `max_findings: 0` alone does not silence it. + + + `issue` (the default) is that behavior, unchanged. `none` files + nothing: the bail is announced as a `::warning::` naming the finding, + its bail reason and its dedup signature, and written to the run summary, + so it is recoverable from the run rather than lost silently. Two + consequences of `none` worth knowing before setting it: nothing records + the signature in the ledger, so the finding can be re-proposed — and + re-bail, re-spending builder tokens — on a later run; and it suppresses + EVERY bail, including the prompt-injection secret-scan withhold, which + still prints its `::error::` on the run page but files no issue. + `linear` is reserved for the Linear-sink phase and is NOT implemented + here. + + + Unlike `sink`, this is an OPERATIONAL knob, so `vars.GROOM_CONFIG` may + set it with no PR: it can only make groom quieter, never grant it + privilege. An unrecognized value (including `linear`) is dropped with a + warning and the run falls back to `issue` — the fail-open direction, so + a typo can never silently discard a finding. Ignored unless `builder` + is true. + type: string + required: false + default: issue secrets: ANTHROPIC_API_KEY: description: Anthropic API key the finder + verifier agents bill through. @@ -454,6 +501,7 @@ jobs: "volume_gate": ${{ toJSON(inputs.volume_gate) }}, "max_prs": ${{ toJSON(inputs.max_prs) }}, "max_findings": ${{ toJSON(inputs.max_findings) }}, + "bail_sink": ${{ toJSON(inputs.bail_sink) }}, "interval_days": ${{ toJSON(inputs.interval_days) }}, "cadence": ${{ toJSON(inputs.cadence) }}, "themes": ${{ toJSON(inputs.themes) }}, @@ -2024,6 +2072,11 @@ jobs: RUN_ID: ${{ github.run_id }} DRY_RUN: ${{ fromJSON(needs.gate.outputs.resolved).dry_run }} DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + # Resolved (BE-6157): where a BAIL goes. Reads empty when config.py + # dropped an unrecognized value — `normalize_bail_sink` maps that (and + # any other unknown) back to `issue`, so the fallback is today's + # behavior rather than silent suppression. + BAIL_SINK: ${{ fromJSON(needs.gate.outputs.resolved).bail_sink }} run: | set -euo pipefail python3 - <<'PY' @@ -2033,6 +2086,9 @@ jobs: # this PR (or bail issue) and never re-proposes the finding. sys.path.insert(0, os.path.join(os.environ["GITHUB_WORKSPACE"], "_groom_assets", ".github", "groom")) from ledger import signature_marker, builder_pr_body + # config.py owns the bail_sink allowlist (BE-6157) so the workflow and + # the GROOM_CONFIG coercion can never disagree about what `none` means. + from config import normalize_bail_sink repo = os.environ["REPO"] idx = int(os.environ["IDX"]) @@ -2052,6 +2108,7 @@ jobs: # still gets the `groom-security` label / scrutiny banner. is_security = str(finding.get("security")).strip().lower() != "false" dry_run = os.environ.get("DRY_RUN", "false").lower() == "true" + bail_sink = normalize_bail_sink(os.environ.get("BAIL_SINK")) def sh(cmd, check=True, **kw): r = subprocess.run(cmd, text=True, capture_output=True, **kw) @@ -2059,10 +2116,39 @@ jobs: raise RuntimeError(f"{' '.join(cmd)} failed: {(r.stderr or r.stdout).strip()}") return r + def oneline(value, limit): + """Model-authored text, flattened to one bounded line. + + An embedded newline in a `::warning::` ends the annotation early and + lets the remainder pose as its own workflow command, so anything + that reaches an annotation goes through here first. + """ + return " ".join(str(value).split())[:limit] + + def summarize(line): + path = os.environ.get("GITHUB_STEP_SUMMARY") + if path: + with open(path, "a", encoding="utf-8") as s: + s.write(line) + def file_issue(reason): # Bail fallback (agent bailed / patch too large / apply failed): # file a `groom` issue so the finding is not silently dropped AND # the ledger records it (no re-loop next run). + if bail_sink == "none": + # BE-6157: the operator asked for silence — file nothing, but + # never SILENTLY nothing. This annotation plus the summary line + # ARE the recovery path, because no issue means no signature + # marker, which means the ledger does not remember this finding + # and a later run may re-propose (and re-bail on) it. The title + # and (on the agent-bail path) the reason are model-authored, so + # both go through `oneline` before reaching the annotation. + print(f"::warning::bail_sink=none — NOT filing a bail issue for " + f"'{oneline(title, 120)}' (idx={idx}, signature={sig or 'missing'}): " + f"{oneline(reason, 300)}") + summarize(f"- 🔇 bail suppressed (`bail_sink: none`) for **{oneline(title, 120)}** " + f"(`{sig or 'no signature'}`) — {oneline(reason, 300)}\n") + return if not sig: print(f"::warning::bailed finding idx={idx} has no signature — cannot file, skipping.") return @@ -2116,10 +2202,7 @@ jobs: changed = result.get("changed", "?") print(f"[dry-run] WOULD open PR '[groom] {title}' on branch {branch} " f"(base {base}, {changed} line(s) changed).") - summary = os.environ.get("GITHUB_STEP_SUMMARY") - if summary: - with open(summary, "a", encoding="utf-8") as s: - s.write(f"- [dry-run] would build PR for **{title}** ({changed} lines)\n") + summarize(f"- [dry-run] would build PR for **{title}** ({changed} lines)\n") sys.exit(0) sh(["git", "-C", "repo", "commit", "-m", f"groom: {title}\n\nAuto-built groom refactor ({sig}).\nReview required — do not auto-merge."]) @@ -2160,8 +2243,5 @@ jobs: pr_url = out.stdout.strip() print(f"Opened builder PR: {pr_url}") - summary = os.environ.get("GITHUB_STEP_SUMMARY") - if summary: - with open(summary, "a", encoding="utf-8") as s: - s.write(f"- 🤖 Built PR for **{title}**: {pr_url}\n") + summarize(f"- 🤖 Built PR for **{title}**: {pr_url}\n") PY diff --git a/.github/workflows/test-groom-scripts.yml b/.github/workflows/test-groom-scripts.yml index 745d77f..0263a3e 100644 --- a/.github/workflows/test-groom-scripts.yml +++ b/.github/workflows/test-groom-scripts.yml @@ -10,11 +10,17 @@ on: pull_request: paths: - '.github/groom/**' + # The suite also pins the groom.yml WIRING of the knobs these scripts + # resolve (BE-6157), so a workflow-only edit that unhooks one must run it. + - '.github/workflows/groom.yml' - '.github/workflows/test-groom-scripts.yml' push: branches: [main] paths: - '.github/groom/**' + # The suite also pins the groom.yml WIRING of the knobs these scripts + # resolve (BE-6157), so a workflow-only edit that unhooks one must run it. + - '.github/workflows/groom.yml' - '.github/workflows/test-groom-scripts.yml' permissions: diff --git a/README.md b/README.md index 17d83d0..4e1f758 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ This repo is **public** so any repo — public or private, inside or outside the | [`assign-prs-to-author.yml`](.github/workflows/assign-prs-to-author.yml) | Housekeeping — assigns every open PR with no assignees to its author (bot-authored PRs skipped by default). Run on a schedule from a thin caller; useful when a team tracks PR ownership via assignees. The calling job needs `pull-requests: write` and `issues: write`. | | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | -| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. | +| [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | ## Usage From d44030f88af2795e971b7fd7dce96420dce64e14 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 2 Aug 2026 15:07:28 -0700 Subject: [PATCH 2/2] fix(groom): sanitize the bail signature, exempt the secret-scan withhold from bail_sink=none (BE-6157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .github/groom/README.md | 15 +++-- .github/groom/config.py | 6 +- .github/groom/tests/test_config.py | 32 +++++++++- .github/workflows/groom.yml | 93 ++++++++++++++++++++---------- 4 files changed, 109 insertions(+), 37 deletions(-) diff --git a/.github/groom/README.md b/.github/groom/README.md index 7053831..9a009dd 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -48,14 +48,19 @@ Two things follow that are easy to get wrong: - **`bail_sink` is the knob for the bail path.** `issue` (default) keeps the behavior above; `none` files nothing and instead emits a `::warning::` naming the finding, its bail reason and its signature, plus a run-summary line — so the - bail is visible in the run rather than invisible. `none` suppresses *every* - bail, including the secret-scan withhold (which still prints its own - `::error::`), and because no issue is filed, no signature marker is recorded, - so a later run may re-propose and re-bail on that finding. + bail is visible in the run rather than invisible. Because no issue is filed, no + signature marker is recorded, so a later run re-proposes the finding; a + *deterministic* bail (a patch that always exceeds `pr_size_limit`, or always + touches a CI-privileged path) therefore re-bails on every run and permanently + holds one of the `max_prs` slots — at `max_prs: 1`, nothing else ever gets + built. The one bail `none` does **not** suppress is the pre-publish secret-scan + withhold: that issue is filed regardless, because an expiring `::error::` in the + run log is not a durable record of a possible key-exfil attempt. `bail_sink` is an **operational** knob (`vars.GROOM_CONFIG` can set it with no PR), unlike `sink` / `pr_size_limit` / `builder`, which stay in the reviewed -workflow file. If bails are frequent because well-scoped patches keep landing +workflow file — the withhold carve-out above is what keeps that classification +honest: the knob can make groom quieter, never less safe. If bails are frequent because well-scoped patches keep landing just over the line, the real fix is usually raising `pr_size_limit` **in the caller** — a reviewed commit, by design — not suppressing the signal. diff --git a/.github/groom/config.py b/.github/groom/config.py index d97334d..592b15a 100644 --- a/.github/groom/config.py +++ b/.github/groom/config.py @@ -38,7 +38,11 @@ why `bail_sink` (BE-6157) is operational even though its sibling `sink` is locked: `sink` picks the credentialed backend every finding is filed through, while `bail_sink` only chooses whether a builder BAIL becomes an issue or just a -warning. Note the corollary for `pr_size_limit`, which is the knob an operator +warning — and `build_pr` exempts the pre-publish secret-scan withhold from `none` +so the one bail with security meaning keeps its durable record either way, which +is what keeps "quieter, never less privileged" true here. + +Note the corollary for `pr_size_limit`, which is the knob an operator usually reaches for after a near-miss bail: it stays LOCKED (a reviewed commit in the caller) because it is the unreviewably-large-patch backstop — suppressing the bail is not a reason to unlock raising the ceiling. diff --git a/.github/groom/tests/test_config.py b/.github/groom/tests/test_config.py index d616325..a8b25fc 100644 --- a/.github/groom/tests/test_config.py +++ b/.github/groom/tests/test_config.py @@ -369,10 +369,40 @@ def test_build_pr_reads_the_resolved_value(self): def test_build_pr_suppresses_and_warns(self): self.assertIn("from config import normalize_bail_sink", self.wf) self.assertIn('bail_sink = normalize_bail_sink(os.environ.get("BAIL_SINK"))', self.wf) - self.assertIn('if bail_sink == "none":', self.wf) + self.assertIn('if bail_sink == "none" and not withheld:', self.wf) # Suppressed must still be VISIBLE — the annotation is the recovery path. self.assertIn("::warning::bail_sink=none", self.wf) + def test_secret_scan_withhold_is_exempt_from_suppression(self): + """`bail_sink` is an OPERATIONAL key only while it can't erase a security record. + + The exemption rides on a machine field (`"withheld": true` in + result.json), NOT a substring match on the prose reason, so rewording the + bail message can never silently disarm it. Pin all three links: the + producer's flag, the bail call that sets it, and the consumer's read. + """ + self.assertIn('printf \'{"status":"bail","reason":%s,"withheld":%s}\\n\'', self.wf) + withhold = re.search(r"\n\s*bail \"builder output withheld:.*\n", self.wf).group(0) + self.assertTrue(withhold.rstrip().endswith(" true"), withhold) + self.assertIn( + 'file_issue(result.get("reason", "not built"), withheld=bool(result.get("withheld")))', + self.wf, + ) + + def test_missing_signature_guard_precedes_the_sink_branch(self): + """A schema failure must not be reported as an operator's suppression.""" + body = self.wf[self.wf.index("def file_issue(reason, withheld=False):"):] + self.assertLess(body.index("has no signature"), body.index('bail_sink == "none"')) + + def test_suppression_annotation_sanitizes_every_model_authored_field(self): + """`signature` is model-authored too: a raw newline in it forges a workflow command.""" + branch = re.search( + r'(?s)if bail_sink == "none" and not withheld:.*?\n\s+return\n', self.wf + ).group(0) + for field in ("oneline(title, 120)", "oneline(sig, 200)", "oneline(reason, 300)"): + self.assertIn(field, branch) + self.assertNotIn("{sig or ", branch) + def test_max_findings_description_disclaims_bail_issues(self): """The documentation half of the ticket, kept from silently rotting.""" block = re.search(r"(?s)\n max_findings:\n(.*?)\n type:", self.wf).group(1) diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index 169501a..9a918b0 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -118,8 +118,10 @@ name: Groom (reusable) # # A build that can't become a PR (patch over `pr_size_limit`, patch # # touching CI-privileged paths, …) files a `groom` issue instead so the # # work isn't lost — that path is NOT capped by `max_findings`. To make -# # a builder run open PRs and nothing else, add `bail_sink: none` (it is -# # also settable live via vars.GROOM_CONFIG — see the input). +# # a builder run open PRs and (bar a secret-scan withhold) nothing else, +# # add `bail_sink: none` — but read the input first: it also stops the +# # ledger recording the bail, so a deterministic bail re-spends a +# # `max_prs` slot every run. Settable live via vars.GROOM_CONFIG. # # bail_sink: none # secrets: # ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} @@ -229,7 +231,8 @@ on: exceeds `pr_size_limit` or touches a CI-privileged path still files an issue, so work the builder already paid for is not silently discarded. So `max_findings: 0` means "file no FINDINGS", not "open no issues" — - set `bail_sink: none` to suppress the bail issues too. + set `bail_sink: none` to suppress the bail issues too (bar the + secret-scan withhold, which is filed either way). type: number required: false default: 12 @@ -373,22 +376,28 @@ on: `issue` (the default) is that behavior, unchanged. `none` files nothing: the bail is announced as a `::warning::` naming the finding, its bail reason and its dedup signature, and written to the run summary, - so it is recoverable from the run rather than lost silently. Two - consequences of `none` worth knowing before setting it: nothing records - the signature in the ledger, so the finding can be re-proposed — and - re-bail, re-spending builder tokens — on a later run; and it suppresses - EVERY bail, including the prompt-injection secret-scan withhold, which - still prints its `::error::` on the run page but files no issue. + so it is recoverable from the run rather than lost silently. Know this + before setting it: nothing records the signature in the ledger, so the + finding is re-proposed on a later run — and a DETERMINISTIC bail (a + patch that always exceeds `pr_size_limit`, or always touches a + CI-privileged path) therefore re-bails every run, permanently holding + one of the `max_prs` slots, so at the common pilot setting `max_prs: 1` + no other CONFIRMED finding is ever built again. The ONE bail `none` + does NOT suppress is the pre-publish secret-scan withhold: that issue is + filed regardless, because destroying the durable record of a possible + key-exfil attempt is where "quieter" would stop being merely quieter. `linear` is reserved for the Linear-sink phase and is NOT implemented here. Unlike `sink`, this is an OPERATIONAL knob, so `vars.GROOM_CONFIG` may set it with no PR: it can only make groom quieter, never grant it - privilege. An unrecognized value (including `linear`) is dropped with a - warning and the run falls back to `issue` — the fail-open direction, so - a typo can never silently discard a finding. Ignored unless `builder` - is true. + privilege (that is exactly what the withhold carve-out above preserves). + An unrecognized value (including `linear`) is dropped with a warning and + the run keeps the CALLER's value — `issue` unless the caller itself + pinned `none`, in which case the typo leaves the suppression the caller + already chose rather than introducing a new one. Ignored unless + `builder` is true. type: string required: false default: issue @@ -1868,8 +1877,12 @@ jobs: SUMMARY=$(jq -r '.summary // ""' "$BUILDER_OUT" 2>/dev/null || echo "") # Declared up here because the secret scan below bails through it. + # $2 is the `withheld` flag: true ONLY for the pre-publish secret-scan + # bail, which build_pr files even under `bail_sink: none` (BE-6157) — + # a machine field rather than a substring match on the prose reason, so + # rewording the message can never silently disarm that carve-out. bail() { - printf '{"status":"bail","reason":%s}\n' "$(jq -Rn --arg r "$1" '$r')" > /tmp/out/result.json + printf '{"status":"bail","reason":%s,"withheld":%s}\n' "$(jq -Rn --arg r "$1" '$r')" "${2:-false}" > /tmp/out/result.json : > /tmp/out/patch.diff rm -f /tmp/out/pr_body.md } @@ -1903,7 +1916,7 @@ jobs: || printf '%s' "$SUMMARY" | LC_ALL=C grep -qF -- "$ANTHROPIC_API_KEY" \ || { [ -s "$PR_BODY_OUT" ] && LC_ALL=C grep -qF -- "$ANTHROPIC_API_KEY" "$PR_BODY_OUT"; }; }; then echo "::error::Build $IDX output contains ANTHROPIC_API_KEY — refusing to publish the patch, summary OR PR body (possible prompt-injection exfil); filing a redacted issue instead." - bail "builder output withheld: it contained the model API key (possible prompt-injection exfil). Patch, summary and PR body discarded — a human must review this finding manually." + bail "builder output withheld: it contained the model API key (possible prompt-injection exfil). Patch, summary and PR body discarded — a human must review this finding manually." true exit 0 fi CHANGED=$(git -C repo diff --cached --numstat | awk '{a+=($1=="-"?0:$1); d+=($2=="-"?0:$2)} END{print a+d+0}') @@ -2131,30 +2144,49 @@ jobs: with open(path, "a", encoding="utf-8") as s: s.write(line) - def file_issue(reason): + def file_issue(reason, withheld=False): # Bail fallback (agent bailed / patch too large / apply failed): # file a `groom` issue so the finding is not silently dropped AND # the ledger records it (no re-loop next run). - if bail_sink == "none": + # + # This guard stays FIRST, ahead of the `bail_sink` branch: a missing + # signature is a producer error, not an operator choice, and + # reporting it as "suppressed by the operator" would hide a schema + # failure behind a config knob. + if not sig: + print(f"::warning::bailed finding idx={idx} has no signature — cannot file, skipping.") + return + if bail_sink == "none" and not withheld: # BE-6157: the operator asked for silence — file nothing, but # never SILENTLY nothing. This annotation plus the summary line # ARE the recovery path, because no issue means no signature # marker, which means the ledger does not remember this finding - # and a later run may re-propose (and re-bail on) it. The title - # and (on the agent-bail path) the reason are model-authored, so - # both go through `oneline` before reaching the annotation. + # and a later run may re-propose (and re-bail on) it. The title, + # the signature and (on the agent-bail path) the reason are ALL + # model-authored, so every one of them goes through `oneline` + # before reaching the annotation or the run summary — an + # embedded newline would otherwise end the annotation and let + # the remainder pose as its own workflow command in a job that + # holds a write-scoped bot token. print(f"::warning::bail_sink=none — NOT filing a bail issue for " - f"'{oneline(title, 120)}' (idx={idx}, signature={sig or 'missing'}): " + f"'{oneline(title, 120)}' (idx={idx}, signature={oneline(sig, 200)}): " f"{oneline(reason, 300)}") summarize(f"- 🔇 bail suppressed (`bail_sink: none`) for **{oneline(title, 120)}** " - f"(`{sig or 'no signature'}`) — {oneline(reason, 300)}\n") - return - if not sig: - print(f"::warning::bailed finding idx={idx} has no signature — cannot file, skipping.") + f"(`{oneline(sig, 200)}`) — {oneline(reason, 300)}\n") return + # `withheld` (the pre-publish secret-scan bail) is filed even under + # `bail_sink: none`. `bail_sink` is an OPERATIONAL key precisely + # because it can only make groom quieter, never grant it privilege — + # and destroying the durable record of a detected key-exfil attempt, + # leaving only an `::error::` in run logs that expire, is the one + # bail where "quieter" would cross that line. + if withheld and bail_sink == "none": + print("::warning::bail_sink=none does NOT suppress the secret-scan withhold — " + f"filing the security bail for idx={idx} anyway.") labels = ["groom", "groom-security"] if is_security else ["groom"] if dry_run: - print(f"[dry-run] WOULD file bail issue '[groom] {title}' labels={labels} ({reason})") + print(f"[dry-run] WOULD file bail issue '[groom] {oneline(title, 120)}' " + f"labels={labels} ({oneline(reason, 300)})") return header = (f"**Groom auto-builder** — {repo}: this CONFIRMED finding could not be " f"auto-built ({reason}), so it is filed for a human. · [run]({run_url})") @@ -2166,7 +2198,8 @@ jobs: print(f"Filed bail issue: {out.stdout.strip()}") if result.get("status") != "patched": - file_issue(result.get("reason", "not built")) + # `withheld` is set only by the build job's secret-scan bail. + file_issue(result.get("reason", "not built"), withheld=bool(result.get("withheld"))) sys.exit(0) # Base = the repo's DEFAULT branch from the event context — NOT @@ -2200,9 +2233,9 @@ jobs: if dry_run: # Parity preview: the patch applied cleanly, but open nothing. changed = result.get("changed", "?") - print(f"[dry-run] WOULD open PR '[groom] {title}' on branch {branch} " + print(f"[dry-run] WOULD open PR '[groom] {oneline(title, 120)}' on branch {branch} " f"(base {base}, {changed} line(s) changed).") - summarize(f"- [dry-run] would build PR for **{title}** ({changed} lines)\n") + summarize(f"- [dry-run] would build PR for **{oneline(title, 120)}** ({changed} lines)\n") sys.exit(0) sh(["git", "-C", "repo", "commit", "-m", f"groom: {title}\n\nAuto-built groom refactor ({sig}).\nReview required — do not auto-merge."]) @@ -2243,5 +2276,5 @@ jobs: pr_url = out.stdout.strip() print(f"Opened builder PR: {pr_url}") - summarize(f"- 🤖 Built PR for **{title}**: {pr_url}\n") + summarize(f"- 🤖 Built PR for **{oneline(title, 120)}**: {pr_url}\n") PY