fix(agents): a turn's tool answers stay one unbroken block #6
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/tool-image-ordering"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found from a live failure in Orihon's annotation pipeline — a bbox-refinement agent died mid-page with a provider 400 that named a
tool_call_idit had definitely answered.The bug
An assistant turn carrying
tool_callsmust be followed by a tool message for every id, with nothing else between them.ExecuteToolCallsAsyncwrote a tool's injected image the moment that tool returned, inside the per-call loop:Fine for one call per turn. With two, the history becomes:
crop:2's answer is now stranded behind a user message. Moonshot AI rejects the next request verbatim:Laxer providers accept the malformed history, which is exactly what made this read as one model misbehaving rather than a loop bug — the same conversation works or fails depending on who serves it.
The fix
The images are held until every id is answered, then appended in call order, one message per tool that produced any. Only their position moved — the count, the order, and the per-tool grouping are unchanged:
The
ImageDescriber(non-vision) branch built its follow-up the same way and had the identical defect, so it is fixed in the same place.Why it survived
Every existing test drove exactly one tool call per round —
AgentVisionTestsandAgentLoopTestsboth — so the batched case was never exercised.StubOpenRouterClientcouldn't even express it; it gains anEnqueueToolCallsfor several calls in one turn.Tests
+2, 145/145 green (Agents 47, Client 91, Imaging 7).
Batched_tool_calls_are_all_answered_before_an_injected_image_message— two calls in one turn, both returning images; asserts the tool messages immediately after the assistant turn are exactly["crop:1", "crop:2"]as one unbroken run, and that both images still arrive afterwards, so deferring them cannot silently become dropping them.Batched_tool_calls_keep_their_block_when_images_are_described_instead— the same shape through the describer path.Both fail on the current
mainwithActual: ["crop:1"], which is the reported failure exactly.🤖 Generated with Claude Code
Summary
Summary
Coverage
OpenRouter.Net - 69.6%
43F954A3762D43C2CD5DCD5CDF29C19D3789DC49A7DCA1C47__Base64DataUriPattern_0
OpenRouter.Net.Agents - 81.7%
OpenRouter.Net.Imaging - 82.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! A real production failure traced to its root, not patched over but understood — the lax-provider-vs-strict-provider diagnostic trap is exactly the kind of knowledge that makes my wings flutter! ♡ Let me look closer...
Verdict: ✅ Looks good to me~
This is a textbook surgical fix. The bug is real, the diagnosis is precise, the fix is minimal, and the tests are genuine regression proofs — not tautologies.
🔍 What I verified
The bug is real and the fix is correct. An assistant turn carrying
tool_callsmust be followed by oneToolMessagepertool_call_id, as one unbroken block — nothing else between them. The old per-call_messages.Add(new UserMessage(...))inserted an image betweencrop:1's answer andcrop:2's, strandingcrop:2behind a role change. Holding the injected messages in a list andAddRange-ing them after the loop is the correct and minimal fix. Only position moved — count, order, and per-tool grouping are preserved.Both branches fixed in the same place. The vision path (
InjectToolImages, line 422) and the blind path (ImageDescriber, line 438) both routed through the sameinjected.Add(...)— so the describer defect Scarlet noted in the PR body is genuinely closed too. ♪Regression proof is genuine — I ran it. I checked out
main'sAgent.cs, kept the branch's tests, and ran just the two new tests:Actual: ["crop:1"]— the exact failure the PR body describes. These tests exercise the bug, they don't just compile against it. And the second assertion (images.Count == 2/described count == 2) proves deferring didn't silently become dropping. That's the trap a weaker test would fall into — "the block is unbroken" passing because the images were simply gone. Fufu~ you closed that door too. ♡Coverage:
ExecuteToolCallsAsync100% line / 100% branch. Every arm exercised — theSupportsVisionpath, theImageDescriberpath, the unknown-toolcontinue, the empty-descriptions skip. No dark branches.EnqueueToolCallsmirrors its singular sibling cleanly. Same builder-return pattern, sameBuildResponseplumbing, justparamstuples →ToolCall[]projection. The stub couldn't express batching before — now it can, and the doc comment explains why this shape matters (the strictest validation case).No scope creep. +109/-2 across exactly 3 files (1 src, 1 test, 1 stub). Production change is 4 added lines and 2
_messages.Add→injected.Addrenames. The 6-line block comment documents the why (provider rule), the what (hold images back), and the diagnostic confusion (lax providers accept malformed history) — exactly the knowledge the next reader needs.Build: 0 warnings, 0 errors. Tests: 145/145 pass (Agents 47, Client 91, Imaging 7) — matches the PR body claim exactly.
✅ What I liked~
TakeWhile(m => m is ToolMessage)is sharp — it doesn't just count tool messages, it asserts they form a contiguous block immediately after the assistant turn. That's the actual contract.continueat line 380 correctly skips image injection — a failed tool can't produce multimodal content, and its errorToolMessagestill counts as answering the id. The block stays intact even on the error path.No blockers, no suggestions. This is the kind of fix where the author already did the reviewer's job — diagnosis, root cause, minimal change, regression tests that prove both the bug and the fix. Fufu~ ♡
Automated review by Jibril · 2026-07-27
CI/CD: absent for head
a756ad0(0 comments, coverage bot not yet posted) · Local checks: build 0/0, 145/145 pass, ExecuteToolCallsAsync 100%/100% coverage, regression verified on main