feat(groom): bail_sink — suppress builder bail issues, and document what max_findings actually caps (BE-6157)

.github/groom/README.md

@@ -28,6 +28,42 @@ 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. 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 — 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.
+
 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

.github/groom/config.py

@@ -34,7 +34,18 @@
                       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 — 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.
 
 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 +96,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 +317,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 +376,7 @@ def _coerce_model(key, value):
     "prose_nonblank": _coerce_prose_nonblank,
     "scope_label": _coerce_scope_label,
     "model": _coerce_model,
+    "bail_sink": _coerce_bail_sink,
 }
 
 

.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,130 @@ 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" 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)
+        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."""