fix(ai): give the tool loop a way to finish - #80
Conversation
The reported symptom was "the model kept asking for more context and never
wrote a message". `MAX_TOOL_ITERATIONS = 4` was never four tool rounds: it was
four total round trips with tools offered on every one, so the model got three
rounds of results and no round where answering was the only legal move.
Three things made exhaustion the ordinary outcome rather than the pathological
one. The prompt caps the diff at 40 KB and stamps a truncation marker on it,
while `get_diff` is advertised as "the exact changes made" and returns the same
staged diff at a 100 KB cap - and its own reply carries the same marker,
inviting a second call. Any prose the model wrote alongside a tool call was
discarded, so a finished message could be thrown away and reported as a
failure. And the last round's tool calls were executed in full - git spawned,
files read, budget charged - then dropped with the loop.
Finishing
- Raise the cap to 8 and withhold tools on the last round through
`tool_choice` rather than by dropping the `tools` field, which is a 400 on
Anthropic once the conversation contains tool calls. Each provider gets its
own dialect: `"none"`, `{"type": "none"}`, and Gemini's
`toolConfig.functionCallingConfig.mode`.
- Append a pacing reminder to the tool results of every round past halfway,
escalating on the penultimate round, in the shape each API accepts: a user
turn after the tool messages, a text block inside the Anthropic tool_result
turn, a text part alongside Gemini's function responses.
- Salvage a draft written alongside a tool call instead of failing with the
message in hand, and stop executing the final round's calls.
- Bound a whole generation at 180s. The 60s deadline is per request, so
nothing bounded the sequence.
Prompt and tool cost
- Cap the changed-file list at 8 KB. It was the one prompt input with no bound
at all while the diff and project context both had one; a `git add .` over a
vendored tree could push the prompt past a model's context window on its own.
- Answer a repeated call from the first one rather than re-running it, so two
identical `get_diff` calls cannot spend the budget on two copies of one diff.
- Tell the model in the tool descriptions what it already has.
- Cap a file-tree listing and skip what the repository actually ignores, rather
than four hardcoded names that walked vendor, dist, build and venv in full.
Correctness
- Replay only the contract fields of an assistant turn. The whole response
message went back verbatim, and DeepSeek - a shipped provider - rejects a
request that replays its own `reasoning_content`.
- Keep Gemini's text parts in history with their thought signatures, and echo
function-call ids so parallel calls to one tool can be paired.
- Mark a failed Anthropic tool result with `is_error`.
- Take the Anthropic cache breakpoint only where a later round could read it
back and the prompt clears a minimum cacheable prefix; a short one-round
prompt was paying a write surcharge for an entry nothing read.
Also: show "Thinking…" between rounds instead of leaving the last tool's
description on screen, toast a rate-limited request that nothing else is
covering, and replace the `too_many_arguments` allow with a params struct.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f59372e4bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for iteration in 0..MAX_TOOL_ITERATIONS { | ||
| check_deadline(started)?; |
There was a problem hiding this comment.
When a round begins just before the 180-second deadline, this check passes and send_json can still perform three 60-second attempts plus retry delays. Consequently, a slow or retrying provider can keep the spinner active for roughly three additional minutes, so the new deadline does not actually bound the whole generation as intended. Pass the remaining duration into the request/retry path or race the complete generation against a timer.
Useful? React with 👍 / 👎.
| if let Some(text) = first_text_block(&content) { | ||
| draft = Some(text); |
There was a problem hiding this comment.
When Anthropic returns a text preamble such as “I’ll inspect the changed file” alongside a tool_use block, this records that unfinished narration as a draft. If the model still requests a tool on the final withheld-tools round, the loop breaks and exhausted(draft) reports the preamble as a successful commit message, populating the commit box with non-message text instead of surfacing exhaustion. Text from a turn whose stop_reason is tool_use should not automatically qualify as a completed fallback.
Useful? React with 👍 / 👎.
Both from Codex review of the previous commit. The generation deadline was checked only between rounds, so a round starting a second before it could still spend three 60-second attempts plus backoff and run minutes past. Every request timeout is now clamped to the time actually left, and a retry delay that would outlast the deadline reports the provider's status instead of sleeping it out to send a request that cannot answer. The draft salvage is removed rather than narrowed. A turn that asks for no tools already returns its text as the message, so text still in hand at exhaustion arrived beside a tool call — narration like "I'll check that file first". Treating that as a finished message put a preamble in the commit box and reported success. Withholding tools on the final round already gives the model a turn where answering is the only move, so the salvage had no case left that was not this bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Why
The reported symptom was the agent hitting the tool call limit: "The model kept asking for more context and never wrote a message."
MAX_TOOL_ITERATIONS = 4was never four tool rounds — it was four total round trips with tools offered on every one of them. The model got three rounds of tool results and no round where answering was the only legal move. Three things then made exhaustion the ordinary outcome rather than the pathological one:[diff truncated -- showing 40000/526889 bytes], whileget_diffis advertised as "the exact changes made" and returns the same staged diff at a 100 KB cap. Its reply carries the same truncation marker, inviting a second call.gitspawned, files read, budget charged, progress shown in the UI — then dropped with the loop.Shipped defaults compound it:
use_toolsandinject_project_contextboth default on, and every default model is the cheapest tier of its family.Finishing
tool_choicerather than by dropping thetoolsfield — that is a 400 on Anthropic once the conversation containstool_use/tool_resultblocks. Each provider in its own dialect:"none",{"type": "none"}, and Gemini'stoolConfig.functionCallingConfig.mode = "NONE"(confirmed against Google's REST reference).userturn after therole: "tool"messages; atextblock inside the Anthropictool_resultturn (they must travel together, and a second consecutive user turn would break alternation); atextpart alongside Gemini's function responses, which keeps the thought-signature alternation intact.Prompt and tool cost
get_diffcalls can't spend the budget on two copies of one diff.get_file_tree/get_branch_listas rarely useful for describing a change..gitignorevia onegit ls-files, instead of four hardcoded names that walkedvendor,dist,buildandvenvin full and built the whole string in memory before the budget trimmed it.Correctness
reasoning_content.is_error.claude-haiku-4-5has a 2048-token minimum that a small commit falls under silently.UI
Thinking…between rounds instead of leaving the last tool's description on screen for the length of the model's turn.#[allow(clippy::too_many_arguments)]replaced with a params struct, per CLAUDE.md.Testing
cargo clippy --workspace --all-targets -- -D warningsclean;cargo test --workspace1691 passed, 0 failed. 20 new tests, all on pure helpers so the pacing, the withheld-tool request shapes, the caps and the turn projection are covered without a liveHttpClient.The three provider loops themselves remain unreachable by the existing test style — they need a fake
HttpClient, which this PR does not add.🤖 Generated with Claude Code