-
Notifications
You must be signed in to change notification settings - Fork 2
fix(groom): bound finder exploration and spend #261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
|
||
| 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. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||
|
|
||
| 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":"..."}]} | ||
|
|
||
| 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+\\")) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟢 Low — This suite is textual only: asserting that |
||
|
|
||
| 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() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — Both new ceilings abort inside the |
||
| # context size, so a turn cap alone is not a reliable spend boundary. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| # `git grep` is deliberately NOT allowlisted: `--open-files-in-pager=<cmd>` | ||
| # executes an arbitrary command. The Grep tool covers content search. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| # `Edit(//<abs path>)` instead of a bare `Write` is the STRUCTURAL close of | ||
|
|
@@ -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 \ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| --max-budget-usd 8 \ | ||
|
coderabbitai[bot] marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High — There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — If the ceiling trips just after a complete |
||
| --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 "" \ | ||
|
|
||
There was a problem hiding this comment.
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
GrepviaBash(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).