fix(ai): stop emitting duplicate TOOL_CALL_END for server-executed tools#696
Open
valeriudev wants to merge 4 commits into
Open
fix(ai): stop emitting duplicate TOOL_CALL_END for server-executed tools#696valeriudev wants to merge 4 commits into
valeriudev wants to merge 4 commits into
Conversation
Adapters already stream START/ARGS/END for every tool call, but the post-execution phase in chat() pushed a second TOOL_CALL_END with no preceding TOOL_CALL_START. AG-UI-strict consumers (e.g. @ag-ui/client's verifyEvents) reject an orphan END and abort the stream. buildToolResultChunks now emits END only alongside the matching START it already gates on argsMap (the continuation re-execution path, which reconstructs a never-streamed call); the normal post-execution path contributes only TOOL_CALL_RESULT. The result/error state lives on the spec-compliant TOOL_CALL_RESULT, so nothing is lost. Fixes TanStack#519.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR fixes issue Chat Duplication Fix
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testing/e2e/tests/anthropic-server-tool.spec.ts`:
- Around line 37-38: Add a dedicated Playwright+aimock E2E test in
testing/e2e/tests/anthropic-server-tool.spec.ts that reproduces `#519` by
exercising the chat/tools-test path and asserting streaming event invariants:
simulate a server-executed tool invocation and verify exactly one TOOL_CALL_END
is emitted per server tool execution and that each TOOL_CALL_START pairs with a
subsequent TOOL_CALL_END (preserve ordering). Use the existing aimock setup in
this spec to mock the tool response, create a test case named to reference the
regression (e.g., "regression `#519`: single TOOL_CALL_END per tool"), and add
assertions that count TOOL_CALL_START and TOOL_CALL_END events and validate
start/end pairing in the streamed output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9fbd824-91a5-4756-b759-5a74efedb274
📒 Files selected for processing (5)
.changeset/chat-no-duplicate-tool-call-end.mdpackages/ai/src/activities/chat/index.tspackages/ai/tests/chat.test.tspackages/ai/tests/middleware.test.tstesting/e2e/tests/anthropic-server-tool.spec.ts
Drive chat() with a server tool through an adapter that streams its own START/ARGS/END (as real adapters do) and assert the emitted lifecycle is balanced: exactly one TOOL_CALL_END per tool call, each preceded by a matching START — the invariant @ag-ui/client's verifyEvents enforces. Reproduces TanStack#519 (the orphan duplicate END) end-to-end.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
testing/e2e/tests/tool-call-lifecycle.spec.ts (1)
44-50: ⚡ Quick winTighten END/START pairing invariant.
Line 45-50 currently checks only that a START existed previously. It should consume the START on Line 48-49 so duplicate ENDs for the same
toolCallIdfail the invariant check.Suggested diff
const open = new Set<string>() for (const c of chunks) { if (c.type === 'TOOL_CALL_START') open.add(idOf(c)) if (c.type === 'TOOL_CALL_END') { - expect(open.has(idOf(c))).toBe(true) + const id = idOf(c) + expect(open.has(id)).toBe(true) + open.delete(id) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testing/e2e/tests/tool-call-lifecycle.spec.ts` around lines 44 - 50, The invariant currently only checks that a TOOL_CALL_END has a prior TOOL_CALL_START by using open.has(idOf(c)) but does not remove the start, allowing duplicate ENDs to pass; update the loop handling of c.type === 'TOOL_CALL_END' to both assert open.has(idOf(c)) and then remove that id from the open Set (open.delete(idOf(c))) so each START is consumed and a second END for the same toolCallId will fail; reference the variables/chunk types used in this test: chunks, open (Set<string>), idOf(c), and the c.type values 'TOOL_CALL_START' / 'TOOL_CALL_END'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@testing/e2e/tests/tool-call-lifecycle.spec.ts`:
- Around line 44-50: The invariant currently only checks that a TOOL_CALL_END
has a prior TOOL_CALL_START by using open.has(idOf(c)) but does not remove the
start, allowing duplicate ENDs to pass; update the loop handling of c.type ===
'TOOL_CALL_END' to both assert open.has(idOf(c)) and then remove that id from
the open Set (open.delete(idOf(c))) so each START is consumed and a second END
for the same toolCallId will fail; reference the variables/chunk types used in
this test: chunks, open (Set<string>), idOf(c), and the c.type values
'TOOL_CALL_START' / 'TOOL_CALL_END'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66e47d63-8009-45a7-81c2-ccf096da26f5
📒 Files selected for processing (3)
testing/e2e/src/routeTree.gen.tstesting/e2e/src/routes/api.tool-call-lifecycle-wire.tstesting/e2e/tests/tool-call-lifecycle.spec.ts
✅ Files skipped from review due to trivial changes (1)
- testing/e2e/src/routeTree.gen.ts
A second TOOL_CALL_END for the same toolCallId now fails the pairing check directly, matching how @ag-ui/client's verifyEvents closes open tool calls. Addresses CodeRabbit review feedback on TanStack#696.
The devtools middleware sourced tool results from TOOL_CALL_END, which the adapter emits before execution (no result attached). With the TanStack#519 fix in @tanstack/ai, the post-execution END is no longer re-emitted, so results now travel on the spec-compliant TOOL_CALL_RESULT event. Handle it so server-executed tool output keeps reaching devtools.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #519.
What
chat()emitted a duplicateTOOL_CALL_ENDfor every server-executed tool. Adapters already streamTOOL_CALL_START/TOOL_CALL_ARGS/TOOL_CALL_ENDfor each call, but the post-execution phase pushed a secondENDwith no matchingSTART. AG-UI-strict consumers (e.g.@ag-ui/client'sverifyEvents, which CopilotKit's runtime pipes through) reject an orphanENDand abort the stream.Fix
buildToolResultChunksnow emitsENDonly alongside theSTART/ARGSit already gates onargsMap— the continuation re-execution path, which reconstructs a tool call the adapter never streamed. The normal post-execution path contributes onlyTOOL_CALL_RESULT. This also makes the triple atomic:STARTandARGSwere already conditional onargsMap; onlyENDwas leaking out unconditionally.Result and error state ride on the spec-compliant
TOOL_CALL_RESULT, so nothing is lost — verified the clientStreamProcessordelivers both viahandleToolCallResultEvent.Tests
START/END/RESULTper server-executed tool, plus the "everyENDhas a matchingSTART" invariantverifyEventsenforces.TOOL_CALL_END(matching real adapters) and moved the error-state assertion ontoTOOL_CALL_RESULT.@tanstack/aiunit suite + affected E2E specs green.Summary by CodeRabbit
New Features
Bug Fixes
Tests