Skip to content

fix(groomer): warn the model before its exploration rounds run out - #919

Merged
joryirving merged 1 commit into
mainfrom
fix/groomer-round-limit-warning
Sep 2, 2026
Merged

fix(groomer): warn the model before its exploration rounds run out#919
joryirving merged 1 commit into
mainfrom
fix/groomer-round-limit-warning

Conversation

@joryirving

Copy link
Copy Markdown
Contributor

Summary

  • Tell the model it has two rounds left so it submits what it found, instead of the loop ending silently.
  • Rename maxToolCalls to maxRounds, since it caps model round-trips rather than calls. DISPATCH_GROOMER_MAX_TOOL_CALLS is 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 — the for loop 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:

before after
avg tool calls 6.7 22.2
avg bytes used 6,873 42,554
runs reporting files 55% 17%

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_calls split in pr-reviewer-action.

Verification

  • npx vitest run — 2483 passed, 5 skipped (3 new)
  • npx tsc --noEmit clean, npm run lint 0 errors, build succeeds

https://claude.ai/code/session_01YSuDvZq9ncvyX85Uzx3cQh

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
@joryirving
joryirving merged commit b5052b9 into main Sep 2, 2026
12 checks passed
@joryirving
joryirving deleted the fix/groomer-round-limit-warning branch September 2, 2026 19:27
@its-miso its-miso Bot mentioned this pull request Sep 2, 2026

@its-saffron its-saffron Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 maxToolCallsmaxRounds

  • Field rename on the config interface and its default/parse site.
  • Environment variable read uses DISPATCH_GROOMER_MAX_ROUNDS ?? DISPATCH_GROOMER_MAX_TOOL_CALLS via parseIntEnv, preserving the old name as a deprecated alias. parseIntEnv falls back to 12 when both are unset, so a deployment that only sets the old name continues to work.
  • Verified: run.test.ts mock config is updated to maxRounds: 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.example lines 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 sets roundsExhausted = turn === options.maxRounds - 1, and pushes a synthetic user-role message when options.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_changes does 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.
  • roundsExhausted is set on every iteration, not just the last, because the for loop 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_findings text 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.maxRounds through to exploreRepository. No behavior change beyond the rename.

docs/hosted-groomer.md — env-var table and prose update

  • Replaces the DISPATCH_GROOMER_MAX_TOOL_CALLS row with DISPATCH_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 in searchCode / readFile / listDirectory tool 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_URLDISPATCH_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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant