fix(ai): emit TOOL_CALL_START/ARGS during continuation re-executions#372
fix(ai): emit TOOL_CALL_START/ARGS during continuation re-executions#372DiegoGBrisa wants to merge 6 commits intoTanStack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: ec66e30 The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDuring continuation re-executions, pending tool calls now emit full call chunks: an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TextEngine
participant Tool
Client->>TextEngine: Request continuation re-execution
TextEngine->>Tool: (re)execute pending tool call
Tool-->>TextEngine: Tool result
TextEngine->>Client: STREAM: TOOL_CALL_START
TextEngine->>Client: STREAM: TOOL_CALL_ARGS
TextEngine->>Client: STREAM: TOOL_CALL_END
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
View your CI Pipeline Execution ↗ for commit 94cc2d4
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-anthropic
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-skills
@tanstack/ai-devtools-core
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-openrouter
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/typescript/ai/src/activities/chat/index.ts (1)
1027-1034: Populateargson the syntheticTOOL_CALL_ARGSchunk.At continuation time you already have the full serialized arguments, so emitting only
deltamakes these synthetic chunks a little less expressive than the adapter-emitted ones. Settingargstoo keeps raw stream consumers from needing a continuation-only code path.💡 Suggested change
const args = argsMap.get(result.toolCallId) ?? '{}' chunks.push({ type: 'TOOL_CALL_ARGS', timestamp: Date.now(), model: finishEvent.model, toolCallId: result.toolCallId, delta: args, + args, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/src/activities/chat/index.ts` around lines 1027 - 1034, The synthetic TOOL_CALL_ARGS chunk currently only sets delta and omits the full args; update the chunk created in the continuation path (the block using argsMap.get(result.toolCallId) and pushing into chunks) to also set an args property with the serialized arguments (the same value you pull from argsMap, e.g., args or parsed equivalent) so consumers see the full arguments on the synthetic chunk alongside delta, model, toolCallId, and timestamp.packages/typescript/ai/tests/chat.test.ts (1)
722-801: Add one regression for the mixed pending/wait branch too.These cases cover the path where pending server tools finish and the engine proceeds to the next model call. The new
argsMapplumbing is also used when continuation pauses again forneedsClientExecutionorneedsApproval, so I’d add one assertion there thatTOOL_CALL_STARTandTOOL_CALL_ARGSare still emitted for the completed server tool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/typescript/ai/src/activities/chat/index.ts`:
- Around line 1027-1034: The synthetic TOOL_CALL_ARGS chunk currently only sets
delta and omits the full args; update the chunk created in the continuation path
(the block using argsMap.get(result.toolCallId) and pushing into chunks) to also
set an args property with the serialized arguments (the same value you pull from
argsMap, e.g., args or parsed equivalent) so consumers see the full arguments on
the synthetic chunk alongside delta, model, toolCallId, and timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09507dd7-efca-4f4a-8822-f6c0d6ef6b33
📒 Files selected for processing (3)
.changeset/fix-continuation-chunk-emission.mdpackages/typescript/ai/src/activities/chat/index.tspackages/typescript/ai/tests/chat.test.ts
Intentionally omitting args — adapter-emitted TOOL_CALL_ARGS chunks only use delta, so the synthetic chunks match the same shape. Adding a non-standard field would require consumers to handle continuation chunks differently from regular ones. |
Continuation re-executions (pending tool calls from message history) only emit TOOL_CALL_END, skipping TOOL_CALL_START and TOOL_CALL_ARGS. This causes clients to store tool calls with empty arguments, leading to infinite re-execution loops. Two tests added: - Single pending tool call emits full START → ARGS → END sequence - Batch of pending tool calls each emit full START → ARGS → END sequence
…re-executions Pending tool calls resumed from message history only emit TOOL_CALL_END, causing clients to store empty arguments and triggering infinite re-execution loops. Emit the full START → ARGS → END sequence using the arguments from the original ToolCall objects.
DiegoGBrisa
force-pushed
the
fix/continuation-chunk-emission
branch
2 times, most recently
from
8974a32 to
654fb57
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
testing/e2e/tests/tools-test/continuation-args.spec.ts (2)
162-167: Prefer Playwright output paths over raw title-derived filenames.Using
testInfo.titledirectly inpathcan produce brittle filenames.testInfo.outputPath(...)is safer and keeps artifacts scoped per test run.Suggested change
- await page.screenshot({ - path: `test-results/continuation-args-failure-${testInfo.title.replace(/\s+/g, '-')}.png`, - fullPage: true, - }) + await page.screenshot({ + path: testInfo.outputPath( + `continuation-args-failure-${testInfo.title.replace(/[^a-z0-9-]/gi, '-')}.png`, + ), + fullPage: true, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/tests/tools-test/continuation-args.spec.ts` around lines 162 - 167, The failure screenshot path uses a brittle filename derived from testInfo.title inside the test.afterEach hook; replace constructing the path manually with Playwright's testInfo.outputPath to generate the artifact path (e.g., call testInfo.outputPath('continuation-args-failure.png') or include a safe suffix) and use that value for page.screenshot so artifacts are scoped and safely named; update the screenshot path usage in the test.afterEach callback where testInfo.title is currently used.
50-159: Add an explicit mixed pending/wait continuation scenario.Current coverage validates single/sequential/parallel client-tool continuations, but I don’t see an explicit case where completed server-tool results are emitted and the same continuation still pauses for
needsClientExecutionorneedsApproval. That mixed branch is where this regression is easiest to miss again.As per coding guidelines,
testing/e2e/**/*.spec.ts: "Add E2E test coverage for every feature, bug fix, or behavior change in the testing/e2e directory".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/tests/tools-test/continuation-args.spec.ts` around lines 50 - 159, Add a new E2E test in continuation-args.spec.ts that covers the mixed pending/server-complete continuation branch by selecting a scenario (e.g., 'mixed-client-server-tools') via selectScenario(page, ...), calling runTest(page) and waitForTestComplete(page, timeout, expectedCount) as other tests do, then use getMetadata(page) and getToolCallParts(page) to assert that a server-side tool call completed (exists in toolCallParts with expected arguments), that a client-side continuation remained pending (execution-complete-count or metadata shows remaining needsClientExecution/needsApproval), and that no tool call has empty arguments; reuse existing helpers (selectScenario, runTest, waitForTestComplete, getToolCallParts, getMetadata) and mirror assertion patterns from the 'sequential client tool' and 'parallel client tool' tests to validate both completed server results and the paused client continuation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@testing/e2e/tests/tools-test/continuation-args.spec.ts`:
- Around line 162-167: The failure screenshot path uses a brittle filename
derived from testInfo.title inside the test.afterEach hook; replace constructing
the path manually with Playwright's testInfo.outputPath to generate the artifact
path (e.g., call testInfo.outputPath('continuation-args-failure.png') or include
a safe suffix) and use that value for page.screenshot so artifacts are scoped
and safely named; update the screenshot path usage in the test.afterEach
callback where testInfo.title is currently used.
- Around line 50-159: Add a new E2E test in continuation-args.spec.ts that
covers the mixed pending/server-complete continuation branch by selecting a
scenario (e.g., 'mixed-client-server-tools') via selectScenario(page, ...),
calling runTest(page) and waitForTestComplete(page, timeout, expectedCount) as
other tests do, then use getMetadata(page) and getToolCallParts(page) to assert
that a server-side tool call completed (exists in toolCallParts with expected
arguments), that a client-side continuation remained pending
(execution-complete-count or metadata shows remaining
needsClientExecution/needsApproval), and that no tool call has empty arguments;
reuse existing helpers (selectScenario, runTest, waitForTestComplete,
getToolCallParts, getMetadata) and mirror assertion patterns from the
'sequential client tool' and 'parallel client tool' tests to validate both
completed server results and the paused client continuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cf5bcd7-44a3-4c4d-98d5-66b0663825dc
📒 Files selected for processing (1)
testing/e2e/tests/tools-test/continuation-args.spec.ts
DiegoGBrisa
force-pushed
the
fix/continuation-chunk-emission
branch
from
654fb57 to
41a47c7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
testing/e2e/tests/tools-test/helpers.ts (1)
193-211: Avoid all-or-nothing failure when one tool-call argument is malformed.At Line 203, a single bad
part.argumentsJSON currently falls into the outercatchand returns[], which hides other valid tool-call parts. Prefer per-part parsing with a safe{}fallback.Proposed refactor
- try { - const messages = JSON.parse(el.textContent || '[]') + try { + const messages = JSON.parse(el.textContent || '[]') const parts: Array<{ name: string; arguments: Record<string, unknown> }> = [] - for (const msg of messages) { - for (const part of msg.parts || []) { + for (const msg of Array.isArray(messages) ? messages : []) { + for (const part of msg?.parts || []) { if (part.type === 'tool-call') { + let parsedArgs: Record<string, unknown> = {} + if (typeof part.arguments === 'string') { + try { + const value = JSON.parse(part.arguments) + if (value && typeof value === 'object' && !Array.isArray(value)) { + parsedArgs = value as Record<string, unknown> + } + } catch { + parsedArgs = {} + } + } else if ( + part.arguments && + typeof part.arguments === 'object' && + !Array.isArray(part.arguments) + ) { + parsedArgs = part.arguments as Record<string, unknown> + } parts.push({ name: part.name, - arguments: - typeof part.arguments === 'string' - ? JSON.parse(part.arguments) - : part.arguments, + arguments: parsedArgs, }) } } } return parts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/tests/tools-test/helpers.ts` around lines 193 - 211, The current loop that builds parts treats any JSON.parse error on one part.arguments as fatal (the outer catch returns []), so change the per-part parsing to be resilient: inside the loop over messages and msg.parts, when part.type === 'tool-call' parse part.arguments with a small try/catch (e.g., let args = typeof part.arguments === 'string' ? try JSON.parse(part.arguments) catch { {} } : part.arguments) and push { name: part.name, arguments: args } so a single malformed argument falls back to {} and other tool-call parts are preserved; keep the outer try/catch only for the initial JSON.parse(el.textContent || '[]') error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@testing/e2e/tests/tools-test/helpers.ts`:
- Around line 193-211: The current loop that builds parts treats any JSON.parse
error on one part.arguments as fatal (the outer catch returns []), so change the
per-part parsing to be resilient: inside the loop over messages and msg.parts,
when part.type === 'tool-call' parse part.arguments with a small try/catch
(e.g., let args = typeof part.arguments === 'string' ? try
JSON.parse(part.arguments) catch { {} } : part.arguments) and push { name:
part.name, arguments: args } so a single malformed argument falls back to {} and
other tool-call parts are preserved; keep the outer try/catch only for the
initial JSON.parse(el.textContent || '[]') error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c655db9c-0cc8-41a8-9ab2-720a5f291ce4
📒 Files selected for processing (2)
testing/e2e/tests/tools-test/continuation-args.spec.tstesting/e2e/tests/tools-test/helpers.ts
Add 4 E2E tests verifying tool call arguments are preserved during
continuation re-executions. Without TOOL_CALL_START/ARGS emission,
clients store tool calls with empty {} arguments.
Tests:
- Single client tool args preserved after continuation
- Sequential client tool args preserved across multiple continuations
- Parallel client tool args preserved in batch continuation
- Mixed server+client tool args preserved in sequence
Also populate the `args` field on synthetic TOOL_CALL_ARGS chunks
emitted during continuation, matching adapter-emitted chunks.
DiegoGBrisa
force-pushed
the
fix/continuation-chunk-emission
branch
from
41a47c7 to
94cc2d4
Compare
1 participant
Summary
Continuation re-executions (pending tool calls resumed from message history) only emit
TOOL_CALL_END, skippingTOOL_CALL_STARTandTOOL_CALL_ARGS. This causes clients to store tool calls with emptyarguments, leading to infinite re-execution loops.
buildToolResultChunksnow accepts an optionalargsMapparametercheckForPendingToolCalls), emits the fullSTART → ARGS → ENDsequence per tool call using the arguments from the originalToolCallobjectsprocessToolCalls) is unaffected — the adapter stream already emitsSTART/ARGSTest plan
START → ARGS → ENDsequenceSTART → ARGS → ENDsequenceSTART < ARGS < ENDper tool callSummary by CodeRabbit
Bug Fixes
Tests
Chores