fix(groomer): warn the model before its exploration rounds run out - #919
Conversation
The byte-budget path tells the model 'budget exhausted, call submit_findings with what you have' and the model reliably complies. The round limit had no equivalent: the loop simply ended, so a run that explored well but had not volunteered findings was discarded with nothing. That mattered once the byte budget stopped binding. Measured over the first runs on the larger budget, exploration went from 6.7 to 22.2 tool calls while the share of runs reporting files fell from 55% to 17%, with 4 of 6 ending on the silent round ceiling. The tight budget had been supplying deadline pressure by accident; removing it left room to explore and no reason to conclude. Also rename maxToolCalls to maxRounds. It caps model round-trips, not calls — 12 rounds produced 22 calls — matching the max_rounds / max_tool_calls split in pr-reviewer-action. DISPATCH_GROOMER_MAX_TOOL_CALLS stays as a deprecated alias. Claude-Session: https://claude.ai/code/session_01YSuDvZq9ncvyX85Uzx3cQh
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M3@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
Recommendation: Approve
This PR is a focused, internally consistent rename and behavior addition for the groomer exploration loop. The diff is small, the tests cover the new behavior at the boundary it actually changes, and nothing in the change touches filesystem path handling.
Change-by-change findings
src/lib/groomer/config.ts — rename maxToolCalls → maxRounds
- Field rename on the config interface and its default/parse site.
- Environment variable read uses
DISPATCH_GROOMER_MAX_ROUNDS ?? DISPATCH_GROOMER_MAX_TOOL_CALLSviaparseIntEnv, preserving the old name as a deprecated alias.parseIntEnvfalls back to12when both are unset, so a deployment that only sets the old name continues to work. - Verified:
run.test.tsmock config is updated tomaxRounds: 12, so existing tests compile and exercise the new field. - No new env-var handling surface introduced; this matches the precedent in the repo for backward-compatible renames (see the
MISSION_CONTROL_*→DISPATCH_*migration in.env.examplelines 198–200).
src/lib/groomer/explore.ts — add a "rounds left" nudge and rename the loop bound
- The
for (let turn = 0; turn < options.maxRounds; turn++)loop now setsroundsExhausted = turn === options.maxRounds - 1, and pushes a synthetic user-role message whenoptions.maxRounds - turn <= 2 && records.length > 0. The byte-budget path has done the same kind of nudge previously and the model complies — that's an established pattern in this file, not a new mechanism. - The synthetic message is plain prose interpolated into
messages. There is no path concatenation, file read, or shell exec here —path_handling_changesdoes not apply to this surface. Verified by reading the diff: the only string interpolation is${roundsLeft}into a literal English sentence. - Final warning text changes from "hit its tool-call budget" to "used all its rounds" so the warning matches the new naming. This is a string-only change in
warnings. roundsExhaustedis set on every iteration, not just the last, because theforloop condition exits before the next iteration. That is the intended behavior and the new test for "does not warn about rounds when the model stopped on its own" pins it.
src/lib/groomer/explore.test.ts — new round-limit warning test suite
- Three new tests cover: nudge fires with
submit_findingstext on the final rounds; nudge does not fire on the first request; warning is absent when the model stops on its own. These directly assert the new behavior at the message-payload boundary, which is the only boundary that changed. - The existing test that pinned the old warning text was updated to the new wording — required for it to keep passing.
- The byte-budget test (
maxTotalBytes: 100) is unchanged in behavior; only the field name was renamed.
src/lib/groomer/run.ts — propagate renamed field
- Passes
config.maxRoundsthrough toexploreRepository. No behavior change beyond the rename.
docs/hosted-groomer.md — env-var table and prose update
- Replaces the
DISPATCH_GROOMER_MAX_TOOL_CALLSrow withDISPATCH_GROOMER_MAX_ROUNDS, documents the old name as a deprecated alias, and adds a short paragraph explaining the two-rounds-left nudge. Matches the doc conventions in AGENTS.md ("Do not commit secrets", label conventions, env-var tables).
Required checks (must_check)
-
review for path traversal vulnerabilities: Not applicable. The diff does not introduce any filesystem read, write, join, resolve, or
..handling. The only file I/O in the changed code is insearchCode/readFile/listDirectorytool implementations, which are not modified by this PR. The new synthetic message contains only an integer (roundsLeft) interpolated into a fixed English template — there is no user-controlled path component. No new attack surface. -
test with edge-case paths (null bytes, symlinks): Not applicable for this PR. The PR does not touch the tool implementations or any path-handling code. The new tests are about LLM message payloads and warning strings. The existing explore tests continue to cover the byte-budget and tool-call-count paths, and CI is green (Tests + Typecheck + Lint + Build all success).
Standards Compliance
- Uses
parseIntEnv(existing helper) for env-var parsing — consistent with the rest of the config file. - Backward-compatibility alias pattern matches repo precedent (e.g.,
MISSION_CONTROL_URL→DISPATCH_URL). - Documentation updated alongside code change, as required by the doc-driven env-var table convention.
- No secrets, no commit of
.env/ build output. - Error handling follows existing patterns; the renamed warning string is the only user-visible message change.
Tool Harness Findings
The harness issued no tool calls and added no new evidence. Nothing to report.
CI status
All twelve CI checks (Build, Tests, Coverage, Typecheck, Lint, Database integration, npm audit, Database migrations, Docker Build, Docker Build (MCP), smoke, publish-release) reached success on commit 6f393487d1a6b55aac88de78fd3a3657c43f85b2. The PR body's claim of 2483 passing tests, clean tsc, zero lint errors, and a green build is corroborated.
Summary
maxToolCallstomaxRounds, since it caps model round-trips rather than calls.DISPATCH_GROOMER_MAX_TOOL_CALLSis kept as a deprecated alias.Why
The byte-budget path already pushes
"Context budget exhausted. Call submit_findings with what you have."back to the model, and the model complies. The round limit had no equivalent — theforloop just ended — so a run that explored well but had not volunteered findings was discarded empty.That was invisible while the byte budget bound first. After #918 raised the budget, the first runs measured:
with 4 of 6 ending on the round ceiling. The tight budget had been supplying deadline pressure by accident; removing it left the model room to explore and no reason to conclude. This restores the deadline without taking back the room.
The after-sample is 6 runs, so treat the rates as directional rather than settled. The silent ceiling is a defect regardless of where the rate lands.
Naming
12 rounds produced 22 calls, so the old name was wrong in a way that misleads when tuning. This matches the
max_rounds/max_tool_callssplit inpr-reviewer-action.Verification
npx vitest run— 2483 passed, 5 skipped (3 new)npx tsc --noEmitclean,npm run lint0 errors, build succeedshttps://claude.ai/code/session_01YSuDvZq9ncvyX85Uzx3cQh