Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/groom/finder.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit — "do not retry it or seek a shell-command substitute" is immediately followed by a tool list that includes the shell substitutes (grep, cat, ls, head, tail, wc), so retrying a denied Grep via Bash(grep:*) is both forbidden and apparently sanctioned by the same sentence. Reword so the no-substitute rule and the available-tools list do not contradict each other. Raised by 1 of 6 reviewers (kimi-k3-high adversarial).


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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium — Blessing zero findings turns a starved run into a silent green no-op: findings: [] sets have_candidates=false, audit_verify and everything downstream are skipped by their if:, the job is green, and the cadence clock still advances — so "gave up at the budget line" is indistinguishable from "repo is clean" for a full GROOM_INTERVAL_DAYS. That is the outcome groom.yml refuses elsewhere (it fails turn exhaustion loudly, and logs dropped counts because a silent drop looks like a clean directory). Have the agent record its stop reason in the output, or emit a warning when it stops early. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).


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":"..."}]}
Expand Down
37 changes: 37 additions & 0 deletions .github/groom/tests/test_finder_bounds.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
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]
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-budget-usd\s+8\s+\\"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low — This suite is textual only: asserting that --max-budget-usd 8 appears in the YAML stays green whether or not the pinned CLI accepts the option, so it reads as coverage for the new ceiling while proving nothing about it and would not catch the unknown-option failure mode. It also pins the brief's $6/$2 prose and the workflow's 8 with independent literals, so the two can drift apart with both tests passing, and split("- name: Run finder", 1)[1] raises IndexError on a step rename instead of failing usefully. test_claude_code_pin.py, which shells out rather than trusting a literal, is the pattern that would actually bind. Raised by 3 of 6 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial, kimi-k3-high adversarial).


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 or seek a shell-command substitute",
FINDER_BRIEF,
)


if __name__ == "__main__":
unittest.main()
10 changes: 7 additions & 3 deletions .github/workflows/groom.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1380,8 +1380,11 @@ 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.
# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium — Both new ceilings abort inside the Run finder step, and interval.py's agent_step_started counts any non-skipped start of that step as a spent audit, so a cap-induced failure still advances the GROOM_INTERVAL_DAYS clock. A repo that reliably trips either tightened cap therefore gets one loud failure followed by a full interval of silently skipped ticks rather than a retry on the next daily tick — and tightening both caps makes that path materially more reachable. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).

# context size, so a turn cap alone is not a reliable spend boundary.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium — This comment claims the brief "reserves both turn and dollar margin", but a claude -p agent has no live cumulative-spend readout, so "stop before the estimated inspection spend reaches $6" is an unverifiable self-estimate and the hard $8 cut — which can land mid-Edit of the handoff file — is the only real enforcement. $8 is also a fixed constant while model is a caller input defaulting to an Opus-class model, so per-turn spend varies by an order of magnitude across callers, and a dollar bound makes coverage a function of untrusted repo content (bulky files cut inspection short). The 60-tool-call bound is the half the agent can actually count; the dollar reserve needs enforcement outside the prompt. Raised by 5 of 6 reviewers (gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial, kimi-k3-high adversarial, kimi-k3-high edge-case).

# `git grep` is deliberately NOT allowlisted: `--open-files-in-pager=<cmd>`
# executes an arbitrary command. The Grep tool covers content search.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low — The stated rationale — turn cost varies with context size, so a turn cap alone is not a reliable spend boundary — applies identically to the verifier and builder agent steps, which this change leaves at --max-turns 150 with no dollar ceiling. Both read the same untrusted repo content that drives context size, so the runaway-spend vector closed here on the finder stays open on the rest of the pipeline. Raised by 1 of 6 reviewers (claude-opus-5-thinking-max adversarial).

# `Edit(//<abs path>)` instead of a bare `Write` is the STRUCTURAL close of
Expand Down Expand Up @@ -1423,7 +1426,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 \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High — The cap drops 150 → 100 in the same hunk that deletes the only recorded measurement: the whole-repo finder brief needs ~82 turns in a healthy environment. That leaves ~18 turns of slack instead of ~68, on a repo that keeps growing. The compensating bound is advisory prompt text in a different unit — 60 inspection tool calls are not turns, and denied calls, retries and synthesis all consume turns without consuming inspection calls — so runs that completed before can now end in error_max_turns. Keep the measurement in the comment and justify 100 against it, or keep the headroom. Raised by 3 of 6 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial, kimi-k3-high adversarial).

--max-budget-usd 8 \
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High--max-budget-usd is introduced with no evidence that the exact-pinned @​anthropic-ai/claude-code@​2.1.217 accepts it, and an unknown option aborts argv parsing before any agent turn runs — so if the flag is unsupported, every finder invocation exits non-zero having produced nothing. The neighbouring permission-rule comment records that its rules were verified empirically against the pinned 2.1.217 precisely because this surface is version-specific; verify this flag the same way before merge. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium — If the ceiling trips just after a complete $FINDER_OUT is written — the likely overrun shape now that the brief tells the agent to write the result and stop — the CLI exits non-zero, the step runs exit "$STATUS", and neither Assert finder produced JSON nor Upload finder candidates carries if: always(). A finished, valid handoff is thrown away and audit_verify is skipped after the run has billed the full audit. Validate and upload the result before propagating the exit status. Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max edge-case).

--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 "" \
Expand Down
Loading