fix(cli): bound repeated tool-call loops - #13244
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Hahaknight
left a comment
There was a problem hiding this comment.
Verified independently against main — the defect is real, the guard is wired at the right layer, and the non-streamChatResponse callers are provably unaffected.
Defect confirmed. Main's streamChatResponse is a while (true) auto-continuation loop with no bound of any kind (no loop guard, no max-turns/iteration counter anywhere in the file): a model that re-issues the same tool-call batch after each result runs until the token budget or the user's patience runs out. Same paid-balance-drain failure class as the loops this repo has hit before.
The fingerprint excludes exactly the right things. Across a repeat, what varies is the provider-generated call id and the JSON key order of arguments; what identifies the repeat is (name, arguments). getToolCallFingerprint hashes precisely the latter pair — IDs dropped, canonicalize recursively sorts object keys — so two "different" calls that do the same thing collide, and two genuinely different calls don't. The empty-batch and changed-fingerprint paths both reset the counter, so alternating patterns ([a], [b], [a], [b]…) never accumulate — correct, since those aren't loops.
Other callers are provably unaffected. blockedToolCallIds is optional; for callers that don't pass it (serve.helpers, useChat.stream.helpers, gui sessionSlice), blockedToolCalls is always empty and the return value collapses to the old constant false. The only behavioral delta is the streamChatResponse path.
Block semantics are coherent end to end. Blocked calls still get their states on the assistant message, then onToolStart → errored recordToolResult → onToolError (so the UI/history stay consistent), executable calls flow through the untouched preprocess/execute path, and the return blockedToolCalls.length > 0 rides the existing shouldReturn branch — a graceful turn end that returns the accumulated response, not an abort. The recordToolResult helper is a faithful extraction of the inline service/fallback logic it replaces (checked line against line).
One observation, non-blocking — growing batches escape. The fingerprint covers the whole batch, so the exact-repeat case ([a] → [a] → [a]) is caught, but a batch that grows each iteration ([a] → [a,b] → [a,b,c] → …) never matches its predecessor and loops unbounded. That's a rarer pathology than the exact repeat, but if you ever want to close it, tracking consecutive counts per individual (name, canonical-args) pair rather than per batch would — a batch repeats only if every call repeats, so per-call counting is strictly more sensitive without being more trigger-happy on legitimate patterns.
Second observation — the block is terminal for the turn. Because shouldReturn ends the turn immediately, the errored tool_result ("Repeated identical tool call blocked…") is recorded for the UI but never sent back to the model, so it can't react or adjust. I read that as a deliberate hard-stop choice (deterministic, no second-guess loop), and with a default of 2 allowed repeats it seems right — just flagging that it's a policy, not an accident.
Tests pin the three properties that matter: ID/key-order insensitivity, the > max boundary (2 pass, 3rd blocks), and reset on change/empty batch; the stream-level test locks the full 3-iteration shape including the blocked set arriving only on iteration 3.
Description
Fixes #12702
The CLI now bounds consecutive identical tool-call batches within one agent turn. Tool calls are fingerprinted by tool name and canonicalized arguments (provider-generated call IDs are ignored). A repeated batch may run twice; the next identical batch is recorded as an errored tool result and stopped before execution, so repeated filesystem or other side effects cannot continue without bound.
The blocked result is written through both ChatHistoryService and the fallback history path, and is surfaced through the normal CLI callbacks.
Checklist
Screen recording or screenshot
Not applicable: this is a CLI safety guard with automated coverage.
Tests