From 23416d97491c00fbbefa695999192f72a75cccf9 Mon Sep 17 00:00:00 2001 From: bymyself Date: Fri, 4 Sep 2026 15:41:02 +0000 Subject: [PATCH 1/5] fix(groom): bound finder exploration and spend --- .github/groom/finder.md | 4 +++- .github/groom/tests/test_finder_bounds.py | 27 +++++++++++++++++++++++ .github/workflows/groom.yml | 11 ++++++--- 3 files changed, 38 insertions(+), 4 deletions(-) create mode 100644 .github/groom/tests/test_finder_bounds.py diff --git a/.github/groom/finder.md b/.github/groom/finder.md index a6ff0f70..0c3d9c59 100644 --- a/.github/groom/finder.md +++ b/.github/groom/finder.md @@ -2,7 +2,9 @@ You are a one-shot agent-work GROOM FINDER on the Mac Studio — phase 1 of 2. Y Find genuine, high-value refactor opportunities a thoughtful senior engineer would actually greenlight — NOT an exhaustive lint. Dimensions: (1) genuine duplication (same non-trivial logic ~15+ lines or a clear repeated shape across >=2 sites); (2) inconsistent patterns (one concept done N ways where converging helps — list the variants); (3) missing abstractions; (4) complexity hotspots; (5) dead/vestigial code. -HARD PRECISION BAR — this is the entire point: only things you'd stake your credibility on; ~6-12 findings MAX, ranked. EXPLICITLY AVOID premature abstraction (in Go especially, a little duplication beats the wrong abstraction; never DRY incidentally-similar-but-semantically-distinct code), bikeshedding, and anything linters/formatters already enforce. For EACH finding include a 'steelman-against' (the strongest reason NOT to do it) and DROP it if the steelman wins. +Work to a bounded inspection plan. After at most 60 inspection tool calls, stop exploring, write the best valid result supported by the evidence already gathered, and finish. Reserve enough time to write the result; broad repository coverage is NOT a completion requirement. If an inspection call is denied, do not retry it or seek a shell-command substitute — continue with the available Read, Glob, Grep, git log, git show, grep, cat, ls, head, tail, and wc tools. + +HARD PRECISION BAR — this is the entire point: only things you'd stake your credibility on; ~6-12 findings MAX, ranked. Fewer than 6 findings is valid, including zero, when that is all the bounded inspection supports. EXPLICITLY AVOID premature abstraction (in Go especially, a little duplication beats the wrong abstraction; never DRY incidentally-similar-but-semantically-distinct code), bikeshedding, and anything linters/formatters already enforce. For EACH finding include a 'steelman-against' (the strongest reason NOT to do it) and DROP it if the steelman wins. Write your result as VALID JSON to {{FINDER_OUT}}, EXACTLY this shape (JSON ONLY, no prose) — escape all string contents (quotes, backslashes, newlines): {"repo":"{{REPO}}","scope":"{{SCOPE_LABEL}}","findings":[{"title":"...","dimension":"...","sites":["file:line"],"evidence":"...","proposed":"...","value":"...","risk":"...","confidence":"high|med","steelman":"..."}]} diff --git a/.github/groom/tests/test_finder_bounds.py b/.github/groom/tests/test_finder_bounds.py new file mode 100644 index 00000000..595f282c --- /dev/null +++ b/.github/groom/tests/test_finder_bounds.py @@ -0,0 +1,27 @@ +import pathlib +import re +import unittest + + +ROOT = pathlib.Path(__file__).resolve().parents[3] +WORKFLOW = (ROOT / ".github/workflows/groom.yml").read_text(encoding="utf-8") +FINDER_BRIEF = (ROOT / ".github/groom/finder.md").read_text(encoding="utf-8") + + +class TestFinderBounds(unittest.TestCase): + def test_finder_cli_has_turn_and_dollar_caps(self): + finder_step = WORKFLOW.split("- name: Run finder", 1)[1].split( + "- name: Unlock the clone", 1 + )[0] + + self.assertRegex(finder_step, re.compile(r"--max-turns\s+100(?:\s|\\)")) + self.assertRegex(finder_step, re.compile(r"--max-budget-usd\s+8(?:\.0+)?(?:\s|\\)")) + + def test_brief_requires_an_early_result_and_no_denial_retries(self): + self.assertIn("After at most 60 inspection tool calls", FINDER_BRIEF) + self.assertIn("Fewer than 6 findings is valid", FINDER_BRIEF) + self.assertIn("If an inspection call is denied, do not retry it", FINDER_BRIEF) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index 56577c10..78cfd357 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -1380,8 +1380,12 @@ jobs: # auto-discovery (and hooks/plugins/keychain); auth stays on the # ANTHROPIC_API_KEY set above, the only credential this step has (--bare # never reads OAuth/keychain, so a missing key fails loudly, not silently). - # --max-turns 150: the whole-repo finder brief needs ~82 turns in a healthy - # environment (validated); the old 40 could not finish the brief at all. + # Two independent ceilings bound a runaway finder: 100 turns and $8. + # A healthy whole-repo run historically needed ~82 turns; the brief now + # reserves its final 40-turn margin for producing the result rather than + # treating repository coverage or a six-finding minimum as completion. + # The dollar ceiling is intentionally separate: turn cost varies with + # context size, so a turn cap alone allowed one failed run to spend $11.25. # `git grep` is deliberately NOT allowlisted: `--open-files-in-pager=` # executes an arbitrary command. The Grep tool covers content search. # `Edit(//)` instead of a bare `Write` is the STRUCTURAL close of @@ -1423,7 +1427,8 @@ jobs: GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=safe.directory GIT_CONFIG_VALUE_0='*' \ claude -p "$PROMPT" \ --model "$MODEL" \ - --max-turns 150 \ + --max-turns 100 \ + --max-budget-usd 8 \ --allowedTools "Read,Glob,Grep,Edit(//${FINDER_OUT#/}),Bash(git log:*),Bash(git show:*),Bash(grep:*),Bash(cat:*),Bash(ls:*),Bash(head:*),Bash(tail:*),Bash(wc:*)" \ --bare \ --setting-sources "" \ From 16f80459cb7c884bbdc8236c091b163227b48187 Mon Sep 17 00:00:00 2001 From: bymyself Date: Fri, 4 Sep 2026 15:51:13 +0000 Subject: [PATCH 2/5] fix(groom): reserve finder output budget Addresses https://github.com/Comfy-Org/github-workflows/pull/261#discussion_r3935631334 Addresses https://github.com/Comfy-Org/github-workflows/pull/261#discussion_r3935631345 Addresses https://github.com/Comfy-Org/github-workflows/pull/261#discussion_r3935631350 Addresses https://github.com/Comfy-Org/github-workflows/pull/261#discussion_r3935631359 --- .github/groom/finder.md | 2 +- .github/groom/tests/test_finder_bounds.py | 16 +++++++++++++--- .github/workflows/groom.yml | 7 +++---- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/.github/groom/finder.md b/.github/groom/finder.md index 0c3d9c59..f826d99f 100644 --- a/.github/groom/finder.md +++ b/.github/groom/finder.md @@ -2,7 +2,7 @@ You are a one-shot agent-work GROOM FINDER on the Mac Studio — phase 1 of 2. Y Find genuine, high-value refactor opportunities a thoughtful senior engineer would actually greenlight — NOT an exhaustive lint. Dimensions: (1) genuine duplication (same non-trivial logic ~15+ lines or a clear repeated shape across >=2 sites); (2) inconsistent patterns (one concept done N ways where converging helps — list the variants); (3) missing abstractions; (4) complexity hotspots; (5) dead/vestigial code. -Work to a bounded inspection plan. After at most 60 inspection tool calls, stop exploring, write the best valid result supported by the evidence already gathered, and finish. Reserve enough time to write the result; broad repository coverage is NOT a completion requirement. If an inspection call is denied, do not retry it or seek a shell-command substitute — continue with the available Read, Glob, Grep, git log, git show, grep, cat, ls, head, tail, and wc tools. +Work to a bounded inspection plan. The run has an $8 hard ceiling: treat $6 as the inspection budget and reserve the final $2 for synthesizing and writing the required JSON. Stop inspecting before the estimated inspection spend reaches $6; low remaining budget is a no-go for another inspection call. After at most 60 inspection tool calls, stop exploring, write the best valid result supported by the evidence already gathered, and finish. Reserve enough time to write the result; broad repository coverage is NOT a completion requirement. If an inspection call is denied, do not retry it or seek a shell-command substitute — continue with the available Read, Glob, Grep, git log, git show, grep, cat, ls, head, tail, and wc tools. HARD PRECISION BAR — this is the entire point: only things you'd stake your credibility on; ~6-12 findings MAX, ranked. Fewer than 6 findings is valid, including zero, when that is all the bounded inspection supports. EXPLICITLY AVOID premature abstraction (in Go especially, a little duplication beats the wrong abstraction; never DRY incidentally-similar-but-semantically-distinct code), bikeshedding, and anything linters/formatters already enforce. For EACH finding include a 'steelman-against' (the strongest reason NOT to do it) and DROP it if the steelman wins. diff --git a/.github/groom/tests/test_finder_bounds.py b/.github/groom/tests/test_finder_bounds.py index 595f282c..6f43aae3 100644 --- a/.github/groom/tests/test_finder_bounds.py +++ b/.github/groom/tests/test_finder_bounds.py @@ -13,14 +13,24 @@ def test_finder_cli_has_turn_and_dollar_caps(self): finder_step = WORKFLOW.split("- name: Run finder", 1)[1].split( "- name: Unlock the clone", 1 )[0] + claude_command = finder_step.split('claude -p "$PROMPT"', 1)[1].split( + "--output-format json", 1 + )[0] - self.assertRegex(finder_step, re.compile(r"--max-turns\s+100(?:\s|\\)")) - self.assertRegex(finder_step, re.compile(r"--max-budget-usd\s+8(?:\.0+)?(?:\s|\\)")) + self.assertRegex(claude_command, re.compile(r"--max-turns\s+100\s+\\")) + self.assertRegex(claude_command, re.compile(r"--max-budget-usd\s+8\s+\\")) def test_brief_requires_an_early_result_and_no_denial_retries(self): + self.assertIn("treat $6 as the inspection budget", FINDER_BRIEF) + self.assertIn("reserve the final $2", FINDER_BRIEF) + self.assertIn("low remaining budget is a no-go", FINDER_BRIEF) self.assertIn("After at most 60 inspection tool calls", FINDER_BRIEF) + self.assertIn("Reserve enough time to write the result", FINDER_BRIEF) self.assertIn("Fewer than 6 findings is valid", FINDER_BRIEF) - self.assertIn("If an inspection call is denied, do not retry it", FINDER_BRIEF) + self.assertIn( + "If an inspection call is denied, do not retry it or seek a shell-command substitute", + FINDER_BRIEF, + ) if __name__ == "__main__": diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index 78cfd357..470e0924 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -1381,11 +1381,10 @@ jobs: # ANTHROPIC_API_KEY set above, the only credential this step has (--bare # never reads OAuth/keychain, so a missing key fails loudly, not silently). # Two independent ceilings bound a runaway finder: 100 turns and $8. - # A healthy whole-repo run historically needed ~82 turns; the brief now - # reserves its final 40-turn margin for producing the result rather than - # treating repository coverage or a six-finding minimum as completion. + # The brief reserves both turn and dollar margin for producing the result + # rather than treating broad coverage or a finding minimum as completion. # The dollar ceiling is intentionally separate: turn cost varies with - # context size, so a turn cap alone allowed one failed run to spend $11.25. + # context size, so a turn cap alone is not a reliable spend boundary. # `git grep` is deliberately NOT allowlisted: `--open-files-in-pager=` # executes an arbitrary command. The Grep tool covers content search. # `Edit(//)` instead of a bare `Write` is the STRUCTURAL close of From 1f38b1440fe20df283d24526c691585c64f90242 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Sat, 12 Sep 2026 16:48:18 +0000 Subject: [PATCH 3/5] fix(groom): address finder budget review findings Restores measured turn headroom, validates the exact CLI pin's budget flag, preserves complete handoffs after late CLI failures, and records why bounded inspection ended. Addresses review threads discussion_r3958358991, discussion_r3958359002, discussion_r3958359012, discussion_r3958359017, discussion_r3958359029, discussion_r3958359049, and discussion_r3958359063. --- .github/groom/finder.md | 6 +-- .github/groom/tests/test_claude_code_pin.py | 52 +++++++++++++++++++++ .github/groom/tests/test_finder_bounds.py | 38 +++++++++++---- .github/workflows/groom.yml | 40 ++++++++++++---- 4 files changed, 115 insertions(+), 21 deletions(-) diff --git a/.github/groom/finder.md b/.github/groom/finder.md index f826d99f..bde21e44 100644 --- a/.github/groom/finder.md +++ b/.github/groom/finder.md @@ -2,10 +2,10 @@ You are a one-shot agent-work GROOM FINDER on the Mac Studio — phase 1 of 2. Y Find genuine, high-value refactor opportunities a thoughtful senior engineer would actually greenlight — NOT an exhaustive lint. Dimensions: (1) genuine duplication (same non-trivial logic ~15+ lines or a clear repeated shape across >=2 sites); (2) inconsistent patterns (one concept done N ways where converging helps — list the variants); (3) missing abstractions; (4) complexity hotspots; (5) dead/vestigial code. -Work to a bounded inspection plan. The run has an $8 hard ceiling: treat $6 as the inspection budget and reserve the final $2 for synthesizing and writing the required JSON. Stop inspecting before the estimated inspection spend reaches $6; low remaining budget is a no-go for another inspection call. After at most 60 inspection tool calls, stop exploring, write the best valid result supported by the evidence already gathered, and finish. Reserve enough time to write the result; broad repository coverage is NOT a completion requirement. If an inspection call is denied, do not retry it or seek a shell-command substitute — continue with the available Read, Glob, Grep, git log, git show, grep, cat, ls, head, tail, and wc tools. +Work to a bounded inspection plan. The run has an externally enforced $8 hard ceiling. After at most 60 inspection tool calls, stop exploring, write the best valid result supported by the evidence already gathered, and finish. Reserve enough time to write the result; broad repository coverage is NOT a completion requirement. If an inspection call is denied, do not retry that operation through another tool; move on using other evidence available from the allowed read-only tools. HARD PRECISION BAR — this is the entire point: only things you'd stake your credibility on; ~6-12 findings MAX, ranked. Fewer than 6 findings is valid, including zero, when that is all the bounded inspection supports. EXPLICITLY AVOID premature abstraction (in Go especially, a little duplication beats the wrong abstraction; never DRY incidentally-similar-but-semantically-distinct code), bikeshedding, and anything linters/formatters already enforce. For EACH finding include a 'steelman-against' (the strongest reason NOT to do it) and DROP it if the steelman wins. -Write your result as VALID JSON to {{FINDER_OUT}}, EXACTLY this shape (JSON ONLY, no prose) — escape all string contents (quotes, backslashes, newlines): -{"repo":"{{REPO}}","scope":"{{SCOPE_LABEL}}","findings":[{"title":"...","dimension":"...","sites":["file:line"],"evidence":"...","proposed":"...","value":"...","risk":"...","confidence":"high|med","steelman":"..."}]} +Write your result as VALID JSON to {{FINDER_OUT}}, EXACTLY this shape (JSON ONLY, no prose) — escape all string contents (quotes, backslashes, newlines). Set `stop_reason` to `inspection-complete` when you have enough evidence to finish, or `inspection-call-limit` when the 60-call limit ended inspection: +{"repo":"{{REPO}}","scope":"{{SCOPE_LABEL}}","stop_reason":"inspection-complete|inspection-call-limit","findings":[{"title":"...","dimension":"...","sites":["file:line"],"evidence":"...","proposed":"...","value":"...","risk":"...","confidence":"high|med","steelman":"..."}]} That file is the ONLY handoff to phase 2. When it's written, you may stop. diff --git a/.github/groom/tests/test_claude_code_pin.py b/.github/groom/tests/test_claude_code_pin.py index b71b35fd..4db2b719 100644 --- a/.github/groom/tests/test_claude_code_pin.py +++ b/.github/groom/tests/test_claude_code_pin.py @@ -32,6 +32,7 @@ import re import shutil import subprocess +import tempfile import time import unittest @@ -662,6 +663,57 @@ def _fail_or_skip(self, spec, error, unavailable): self.fail(message) self.skipTest(message) + def test_finder_budget_flag_is_supported_by_pinned_cli(self): + """Install the exact pin and ask its real argument parser for the flag. + + A textual workflow assertion cannot catch an option removed by a CLI + bump. This uses the same npm package, registry and install-script path as + groom.yml, then executes only `--help` (no auth and no API request). + """ + npm = shutil.which("npm") + if npm is None: + self._fail_or_skip(self.spec, "no `npm` on PATH", True) + try: + with tempfile.TemporaryDirectory() as prefix: + install = subprocess.run( + [ + npm, + "install", + "--prefix", + prefix, + "--no-audit", + "--no-fund", + self.spec, + *_REGISTRY_FLAGS, + ], + capture_output=True, + text=True, + timeout=120, + stdin=subprocess.DEVNULL, + ) + if install.returncode != 0: + self._fail_or_skip( + self.spec, + f"`npm install` exited {install.returncode}: {install.stderr[-500:]}", + False, + ) + cli = os.path.join(prefix, "node_modules", ".bin", "claude") + help_result = subprocess.run( + [cli, "--help"], + capture_output=True, + text=True, + timeout=30, + stdin=subprocess.DEVNULL, + ) + except (OSError, subprocess.TimeoutExpired) as exc: + self._fail_or_skip(self.spec, f"could not run pinned CLI `--help`: {exc}", False) + self.assertEqual(0, help_result.returncode, help_result.stderr[-500:]) + self.assertIn( + "--max-budget-usd ", + help_result.stdout, + f"{self.spec} does not advertise the finder budget flag in `claude --help`", + ) + def _positive_control(self): """Prove this npm/registry pair answers about the pinned spec at all. diff --git a/.github/groom/tests/test_finder_bounds.py b/.github/groom/tests/test_finder_bounds.py index 6f43aae3..8d103181 100644 --- a/.github/groom/tests/test_finder_bounds.py +++ b/.github/groom/tests/test_finder_bounds.py @@ -9,29 +9,49 @@ class TestFinderBounds(unittest.TestCase): + @staticmethod + def finder_step(): + match = re.search( + r"(?ms)^ - name: Run finder\n(?P.*?)^ - name: Unlock the clone\n", + WORKFLOW, + ) + if match is None: + raise AssertionError("groom.yml has no Run finder step followed by Unlock the clone") + return match.group("body") + def test_finder_cli_has_turn_and_dollar_caps(self): - finder_step = WORKFLOW.split("- name: Run finder", 1)[1].split( - "- name: Unlock the clone", 1 - )[0] + finder_step = self.finder_step() claude_command = finder_step.split('claude -p "$PROMPT"', 1)[1].split( "--output-format json", 1 )[0] - self.assertRegex(claude_command, re.compile(r"--max-turns\s+100\s+\\")) + self.assertRegex(claude_command, re.compile(r"--max-turns\s+150\s+\\")) self.assertRegex(claude_command, re.compile(r"--max-budget-usd\s+8\s+\\")) - def test_brief_requires_an_early_result_and_no_denial_retries(self): - self.assertIn("treat $6 as the inspection budget", FINDER_BRIEF) - self.assertIn("reserve the final $2", FINDER_BRIEF) - self.assertIn("low remaining budget is a no-go", FINDER_BRIEF) + def test_brief_requires_an_early_result_and_reports_why_it_stopped(self): + self.assertNotIn("estimated inspection spend", FINDER_BRIEF) self.assertIn("After at most 60 inspection tool calls", FINDER_BRIEF) self.assertIn("Reserve enough time to write the result", FINDER_BRIEF) self.assertIn("Fewer than 6 findings is valid", FINDER_BRIEF) + self.assertIn('"stop_reason":"inspection-complete|inspection-call-limit"', FINDER_BRIEF) self.assertIn( - "If an inspection call is denied, do not retry it or seek a shell-command substitute", + "If an inspection call is denied, do not retry that operation through another tool", FINDER_BRIEF, ) + def test_complete_handoff_survives_a_late_cli_failure(self): + finder_step = self.finder_step() + self.assertIn('if [ -s "$FINDER_OUT" ] && jq -e', finder_step) + self.assertIn('(.stop_reason | IN("inspection-complete", "inspection-call-limit"))', finder_step) + self.assertIn("preserving it for independent verification", finder_step) + self.assertIn("STATUS=0", finder_step) + + def test_zero_at_call_limit_is_not_reported_as_clean(self): + self.assertIn( + "reached its inspection-call limit with zero findings", + WORKFLOW, + ) + if __name__ == "__main__": unittest.main() diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index 470e0924..a026b71c 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -1380,9 +1380,10 @@ jobs: # auto-discovery (and hooks/plugins/keychain); auth stays on the # ANTHROPIC_API_KEY set above, the only credential this step has (--bare # never reads OAuth/keychain, so a missing key fails loudly, not silently). - # Two independent ceilings bound a runaway finder: 100 turns and $8. - # The brief reserves both turn and dollar margin for producing the result - # rather than treating broad coverage or a finding minimum as completion. + # Two independent ceilings bound a runaway finder: 150 turns and $8. + # Healthy whole-repo finder runs have measured about 82 turns, so 150 + # retains enough headroom for denied calls and synthesis while the brief's + # separately countable 60-inspection-call limit drives early completion. # The dollar ceiling is intentionally separate: turn cost varies with # context size, so a turn cap alone is not a reliable spend boundary. # `git grep` is deliberately NOT allowlisted: `--open-files-in-pager=` @@ -1426,7 +1427,7 @@ jobs: GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=safe.directory GIT_CONFIG_VALUE_0='*' \ claude -p "$PROMPT" \ --model "$MODEL" \ - --max-turns 100 \ + --max-turns 150 \ --max-budget-usd 8 \ --allowedTools "Read,Glob,Grep,Edit(//${FINDER_OUT#/}),Bash(git log:*),Bash(git show:*),Bash(grep:*),Bash(cat:*),Bash(ls:*),Bash(head:*),Bash(tail:*),Bash(wc:*)" \ --bare \ @@ -1445,12 +1446,22 @@ jobs: echo "::error::finder output contains ANTHROPIC_API_KEY — refusing to publish (possible prompt-injection exfil)." rm -f "$FINDER_OUT"; STATUS=1 fi - # Do NOT swallow a non-zero exit: a rate-limit/network/turn-exhaustion - # failure must fail the job loudly. Swallowing it made the run green and - # silently discarded the finder's real candidates. The diagnostics upload - # below is `if: always()` so it still survives this failure. + # Preserve a complete handoff even if the CLI exits non-zero after writing + # it (for example, if a ceiling lands after the final Edit). The independent + # verifier still checks every candidate. A missing/incomplete handoff keeps + # the CLI failure loud, and the diagnostics upload below survives either + # path via `if: always()`. if [ "$STATUS" -ne 0 ]; then - echo "::error::claude exited $STATUS — see the groom-finder-exec artifact for subtype/num_turns." + if [ -s "$FINDER_OUT" ] && jq -e ' + type == "object" + and (.findings | type == "array") + and (.stop_reason | IN("inspection-complete", "inspection-call-limit")) + ' "$FINDER_OUT" >/dev/null 2>&1; then + echo "::warning::claude exited $STATUS after producing a complete finder handoff; preserving it for independent verification." + STATUS=0 + else + echo "::error::claude exited $STATUS without a complete finder handoff — see the groom-finder-exec artifact for subtype/num_turns." + fi fi exit "$STATUS" @@ -1545,6 +1556,14 @@ jobs: echo "::error::Finder did not produce valid JSON at $FINDER_OUT." exit 1 fi + if ! jq -e ' + type == "object" + and (.findings | type == "array") + and (.stop_reason | IN("inspection-complete", "inspection-call-limit")) + ' "$FINDER_OUT" >/dev/null; then + echo "::error::Finder output is missing a valid findings array or stop_reason." + exit 1 + fi # ENFORCE the path scope (BE-4757). The brief ASKS the finder to stay in # the directory; this is what makes it true. Runs on the finder output # because that is where the evidence `sites` live, and filtering here @@ -1562,6 +1581,9 @@ jobs: fi COUNT=$(jq '.findings | length' "$FINDER_OUT") echo "Finder candidates: $COUNT" + if [ "$COUNT" -eq 0 ] && [ "$(jq -r '.stop_reason' "$FINDER_OUT")" = "inspection-call-limit" ]; then + echo "::warning::Finder reached its inspection-call limit with zero findings; this was a bounded run, not a clean-repository claim." + fi if [ "$COUNT" -gt 0 ]; then echo "have_candidates=true" >> "$GITHUB_OUTPUT" else From 2f43bc86761ee39926f10945491eee6b4c25bada Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Sat, 12 Sep 2026 18:15:51 +0000 Subject: [PATCH 4/5] fix(groom): bound builder feasibility exploration --- .github/groom/builder.md | 2 ++ .github/groom/tests/test_builder_bounds.py | 42 ++++++++++++++++++++++ .github/workflows/groom.yml | 16 ++++++++- 3 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 .github/groom/tests/test_builder_bounds.py diff --git a/.github/groom/builder.md b/.github/groom/builder.md index 0b5ee750..ba5c1272 100644 --- a/.github/groom/builder.md +++ b/.github/groom/builder.md @@ -7,6 +7,8 @@ Rules: 2. **Keep it green.** Match the repo's conventions (read its AGENTS.md/CLAUDE.md/README). If the finding is a refactor, preserve behavior exactly. If the repo has tests for the touched area, update them; do NOT delete a test to make a change "pass". 3. **NEVER touch security/auth-adjacent code.** Those findings are filed as investigations, never auto-built — you should not have received one, but if the finding turns out to touch auth, permissions, secrets, or a trust boundary, BAIL (see below) instead of guessing. 4. **Patch-size bail-out.** If a faithful implementation balloons (many files, a large or risky diff, or it needs a design decision you can't make blindly), do NOT force a giant or speculative change. BAIL: it will be filed as an issue for a human instead. +5. **Decide early.** After at most 20 inspection tool calls, make one feasibility decision before editing: implement the finding or BAIL. Only choose `patched` when the exact bounded diff is clear and confidently finishable in this run. A request to restructure a whole subsystem, combine several independent concerns, or invent a new contract belongs in an issue, not an automatic patch. Reserve enough time to write the control file (and PR body after a patch); do not spend the full run exploring. +6. **Use the granted tools.** Prefer Read, Glob, and Grep for inspection. If a tool call is denied, do not retry that operation or seek a shell-command substitute; use a directly granted tool instead, or BAIL if the missing operation is necessary. When done, write a small control file to {{BUILDER_OUT}} — VALID JSON, EXACTLY this shape (JSON ONLY, no prose): {"status":"patched|bail","summary":""} diff --git a/.github/groom/tests/test_builder_bounds.py b/.github/groom/tests/test_builder_bounds.py new file mode 100644 index 00000000..be07a4d0 --- /dev/null +++ b/.github/groom/tests/test_builder_bounds.py @@ -0,0 +1,42 @@ +import pathlib +import re +import unittest + + +ROOT = pathlib.Path(__file__).resolve().parents[3] +WORKFLOW = (ROOT / ".github/workflows/groom.yml").read_text(encoding="utf-8") +BUILDER_BRIEF = (ROOT / ".github/groom/builder.md").read_text(encoding="utf-8") + + +class TestBuilderBounds(unittest.TestCase): + @staticmethod + def builder_step(): + match = re.search( + r"(?ms)^ - name: Run builder\n(?P.*?)^ - name: Unlock the clone's \.git\n", + WORKFLOW, + ) + if match is None: + raise AssertionError("groom.yml has no Run builder step followed by Unlock the clone's .git") + return match.group("body") + + def test_brief_requires_a_bounded_feasibility_decision_before_edits(self): + self.assertIn("After at most 20 inspection tool calls", BUILDER_BRIEF) + self.assertIn("make one feasibility decision before editing", BUILDER_BRIEF) + self.assertIn("Only choose `patched` when the exact bounded diff is clear", BUILDER_BRIEF) + self.assertIn("Reserve enough time to write the control file", BUILDER_BRIEF) + self.assertIn( + "If a tool call is denied, do not retry that operation or seek a shell-command substitute", + BUILDER_BRIEF, + ) + + def test_complete_clean_bail_survives_a_late_cli_failure(self): + builder_step = self.builder_step() + self.assertIn('if [ -s "$BUILDER_OUT" ] && jq -e', builder_step) + self.assertIn('.status == "bail"', builder_step) + self.assertIn('test -z "$(git status --short)"', builder_step) + self.assertIn("preserving the clean bail-out for issue filing", builder_step) + self.assertIn("STATUS=0", builder_step) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index a026b71c..0a44bfeb 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -2686,7 +2686,21 @@ jobs: # bank a half-finished (or empty) patch as a legitimate result. # `fail-fast: false` keeps the other findings building. if [ "$STATUS" -ne 0 ]; then - echo "::error::claude exited $STATUS — see the groom-builder-exec-$IDX artifact." + # A ceiling can land just after the builder completed a clean bail-out. + # Preserve that bounded handoff so build_pr can file the verified finding + # as an issue. Never preserve `patched` here: a non-zero exit can leave a + # half-written patch, and the normal successful path must validate it. + if [ -s "$BUILDER_OUT" ] && jq -e ' + type == "object" + and .status == "bail" + and (.summary | type == "string" and length > 0) + ' "$BUILDER_OUT" >/dev/null 2>&1 \ + && test -z "$(git status --short)"; then + echo "::warning::claude exited $STATUS after producing a complete control file; preserving the clean bail-out for issue filing." + STATUS=0 + else + echo "::error::claude exited $STATUS — see the groom-builder-exec-$IDX artifact." + fi fi exit "$STATUS" From d56a284ac587d8076ec51776a451a313a127dcd7 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Sat, 12 Sep 2026 18:19:33 +0000 Subject: [PATCH 5/5] test(groom): anchor clean-bail recovery assertion Addresses https://github.com/Comfy-Org/github-workflows/pull/261#discussion_r3997122480 --- .github/groom/tests/test_builder_bounds.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/groom/tests/test_builder_bounds.py b/.github/groom/tests/test_builder_bounds.py index be07a4d0..ea416541 100644 --- a/.github/groom/tests/test_builder_bounds.py +++ b/.github/groom/tests/test_builder_bounds.py @@ -35,7 +35,10 @@ def test_complete_clean_bail_survives_a_late_cli_failure(self): self.assertIn('.status == "bail"', builder_step) self.assertIn('test -z "$(git status --short)"', builder_step) self.assertIn("preserving the clean bail-out for issue filing", builder_step) - self.assertIn("STATUS=0", builder_step) + self.assertRegex( + builder_step, + r'(?s)preserving the clean bail-out for issue filing\."\n\s+STATUS=0\n\s+else', + ) if __name__ == "__main__":