fix(ai): stop text events from corrupting tool-call input - #1020
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis change repairs tool-call input when text events interleave with argument chunks. Inferred completions can reopen for later arguments, authoritative completion re-parses the full input, and partial JSON is not exposed as parsed input. ChangesTool-call completion repair
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change prevents interleaved text events from corrupting tool-call input and avoids exposing fabricated partial JSON; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant TextStream
participant StreamProcessor
participant InternalToolCallState
participant ToolCallPart
TextStream->>StreamProcessor: TEXT_MESSAGE_CONTENT
StreamProcessor->>InternalToolCallState: mark inferredComplete
TextStream->>StreamProcessor: TOOL_CALL_ARGS
StreamProcessor->>InternalToolCallState: clear inferredComplete
TextStream->>StreamProcessor: TOOL_CALL_END
StreamProcessor->>InternalToolCallState: complete with accumulated arguments
StreamProcessor->>ToolCallPart: set input after strict JSON.parse
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ai/src/activities/chat/stream/processor.ts (2)
2089-2153: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
inferredCompleteis stamped before the errored/awaiting-user-action guards, enabling a downstream UI regression.Line 2099 sets
toolCall.inferredComplete = opts?.inferred === trueunconditionally, before theisToolCallPartErrored(2119) /isToolCallPartAwaitingUserAction(2126) early returns that intentionally prevent the rendered part from being downgraded. Those guards protect the message part write, but the internalinferredCompleteflag still gets set totruefor a call whose rendered part is terminally 'error' or awaiting approval.handleToolCallArgsEvent's new revert logic then trusts that flag on a later strayTOOL_CALL_ARGSand overwrites the rendered part back toinput-streaming(see comment on that segment), undoing the terminal state these guards were meant to protect.Consider only marking
inferredComplete = truewhen the call isn't already in a guarded terminal UI state (or gate the revert on the consumer side as suggested there).🤖 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 `@packages/ai/src/activities/chat/stream/processor.ts` around lines 2089 - 2153, Update completeToolCall so inferredComplete is not set to true when the tool call’s rendered part is already errored or awaiting user action. Move or condition the assignment using isToolCallPartErrored and isToolCallPartAwaitingUserAction before the existing guarded early returns, while preserving normal inferred completion behavior for unguarded calls.
1316-1368: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReverting
inferredCompleteunconditionally can clobber a terminal error/approval state.This block trusts
existingToolCall.inferredCompleteand always reverts toinput-streaming, then the trailingupdateToolCallPartcall (lines 1353-1358) writes that state onto the rendered part with no check againstisToolCallPartErrored/isToolCallPartAwaitingUserAction. If the call reached a terminal 'error' or approval state whileinferredCompletewas stilltrueinternally (see the companion comment oncompleteToolCall), a strayTOOL_CALL_ARGShere would silently downgrade the rendered part back toinput-streaming, hiding the error/approval state from the UI.🛡️ Proposed guard
if (existingToolCall.inferredComplete) { - existingToolCall.state = 'input-streaming' - existingToolCall.inferredComplete = false + existingToolCall.inferredComplete = false + if ( + !this.isToolCallPartErrored(existingToolCall.id) && + !this.isToolCallPartAwaitingUserAction(existingToolCall.id) + ) { + existingToolCall.state = 'input-streaming' + } }🤖 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 `@packages/ai/src/activities/chat/stream/processor.ts` around lines 1316 - 1368, Guard the inferredComplete recovery in handleToolCallArgsEvent so it does not overwrite terminal error or user-approval states. Before resetting existingToolCall.state to input-streaming, check the existing tool-call state or the rendered part with isToolCallPartErrored/isToolCallPartAwaitingUserAction; preserve terminal states and prevent updateToolCallPart from downgrading them.
🤖 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.
Outside diff comments:
In `@packages/ai/src/activities/chat/stream/processor.ts`:
- Around line 2089-2153: Update completeToolCall so inferredComplete is not set
to true when the tool call’s rendered part is already errored or awaiting user
action. Move or condition the assignment using isToolCallPartErrored and
isToolCallPartAwaitingUserAction before the existing guarded early returns,
while preserving normal inferred completion behavior for unguarded calls.
- Around line 1316-1368: Guard the inferredComplete recovery in
handleToolCallArgsEvent so it does not overwrite terminal error or user-approval
states. Before resetting existingToolCall.state to input-streaming, check the
existing tool-call state or the rendered part with
isToolCallPartErrored/isToolCallPartAwaitingUserAction; preserve terminal states
and prevent updateToolCallPart from downgrading them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9ec1b1b-ecb9-4cc8-b854-3fc85b888a5f
📒 Files selected for processing (8)
.changeset/fix-interleaved-text-tool-call-input.mdpackages/ai/docs/chat-architecture.mdpackages/ai/src/activities/chat/stream/processor.tspackages/ai/src/activities/chat/stream/types.tspackages/ai/tests/stream-processor.test.tstesting/e2e/src/lib/tools-test-tools.tstesting/e2e/src/routes/api.tools-test.tstesting/e2e/tests/tools-test/interleaved-args.spec.ts
|
@coderabbitai Both findings addressed in 7fb0058:
Added a regression test covering the out-of-order |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline --decorate --no-renames 7fb0058
git show --format=fuller --no-ext-diff --unified=80 7fb0058 -- \
packages/ai/src/activities/chat/stream/processor.ts \
packages/ai/tests/stream-processor.test.ts
ast-grep outline packages/ai/src/activities/chat/stream/processor.ts --items all --match 'completeToolCall|handleToolCallArgsEvent'
ast-grep outline packages/ai/tests/stream-processor.test.ts --items all --match 'out.of.order|output-error|stray'Length of output: 24352
✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
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 `@packages/ai/src/activities/chat/stream/processor.ts`:
- Around line 1332-1342: Update the existingToolCall handling in processor.ts to
preserve the rendered terminal error or approval state during later argument
updates; do not reset it to input-streaming or allow the internal input-complete
state to overwrite it. In stream-processor.test.ts, strengthen the regression
assertion to require the exact error state, ensuring the test detects accidental
rewriting to input-complete.
🪄 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 Plus
Run ID: 44877f42-64cc-4cda-8b9d-3be0bd237bc8
📒 Files selected for processing (2)
packages/ai/src/activities/chat/stream/processor.tspackages/ai/tests/stream-processor.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ai/tests/stream-processor.test.ts (1)
879-900: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that approval input is preserved too.
The regression only checks
state; it would still pass ifTOOL_CALL_END.inputoverwrote the approval part’sinputwhile leavingapproval-requestedintact.Suggested assertion
expect(toolCallPart(processor)?.state).toBe('approval-requested') + expect(toolCallPart(processor)?.input).toEqual(FULL_INPUT)🤖 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 `@packages/ai/tests/stream-processor.test.ts` around lines 879 - 900, Extend the test for TOOL_CALL_END handling around toolCallPart(processor) to also assert that the approval-requested part retains its original input, FULL_INPUT, after processing the tool end event. Keep the existing state assertion and verify input preservation separately.
🤖 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 `@packages/ai/tests/stream-processor.test.ts`:
- Around line 879-900: Extend the test for TOOL_CALL_END handling around
toolCallPart(processor) to also assert that the approval-requested part retains
its original input, FULL_INPUT, after processing the tool end event. Keep the
existing state assertion and verify input preservation separately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6939925-a8bc-4e63-b941-bd8f981d5cfb
📒 Files selected for processing (2)
packages/ai/src/activities/chat/stream/processor.tspackages/ai/tests/stream-processor.test.ts
08c4861 to
472af70
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
472af70 to
c77d222
Compare
|
View your CI Pipeline Execution ↗ for commit 77ee8d3
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit c77d222
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-acp
@tanstack/ai-angular
@tanstack/ai-anthropic
@tanstack/ai-bedrock
@tanstack/ai-byteplus
@tanstack/ai-claude-code
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-snippets
@tanstack/ai-codex
@tanstack/ai-cohere
@tanstack/ai-devtools-core
@tanstack/ai-durable-stream
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-grok-build
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-daytona
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-isolate-quickjs-bun
@tanstack/ai-llmgateway
@tanstack/ai-mcp
@tanstack/ai-memory
@tanstack/ai-mistral
@tanstack/ai-octane
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-opencode
@tanstack/ai-openrouter
@tanstack/ai-perplexity
@tanstack/ai-persistence
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-sandbox
@tanstack/ai-sandbox-cloudflare
@tanstack/ai-sandbox-daytona
@tanstack/ai-sandbox-docker
@tanstack/ai-sandbox-local-process
@tanstack/ai-sandbox-sprites
@tanstack/ai-sandbox-vercel
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-utils
@tanstack/ai-vercel-gateway
@tanstack/ai-vertex
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/openai-base
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
|
Thanks for the PR, @season179! 🙌 @AlemTuzlak will take a look. Automated pre-review checks
Automated triage — a human review follows. |
github-actions
Bot
added
the
waiting-on: author
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-actions
Bot
added
waiting-on: maintainer
f7f3ce3 to
77e3606
Compare
github-actions
Bot
added
merge-conflicts
77e3606 to
c2a02d9
Compare
github-actions
Bot
added
waiting-on: maintainer
f321456 to
3f05389
Compare
github-actions
Bot
added
waiting-on: author
AlemTuzlak
left a comment
AlemTuzlak
left a comment
There was a problem hiding this comment.
The bug is real. I ran the issue #1017 sequence on current main. After one TEXT_MESSAGE_CONTENT between two TOOL_CALL_ARGS deltas, arguments holds the full JSON and input is { templateIds: ["mock-gsk-e"] }. That is silent data corruption. Consumers that read part.input cannot detect it from state.
This PR does not earn its keep in this shape.
Root cause
handleTextMessageContentEvent and handleTextMessageEndEvent call completeAllToolCallsForMessage(). That guess says: text arrived, so tool args are done. completeToolCall then writes input from the lenient partial-JSON parser, which closes unterminated strings. handleToolCallEndEvent no-ops when state is already input-complete, so the later authoritative END cannot repair it.
The adapter contract already names TOOL_CALL_END as the complete signal. RUN_FINISHED / finalizeStream already force-complete as the safety net. The text heuristic is extra, and it is what creates the bug.
Why this PR is too large
This change keeps that heuristic, then adds a machine to undo it:
inferredCompleteon internal tool-call state- reopen on later
TOOL_CALL_ARGS - re-complete on
TOOL_CALL_END - settled-part guards so reopen does not smash error / approval states
- a residual edge the PR itself leaves open (
TOOL_CALL_RESULTbeforeTOOL_CALL_ENDwhile inferred-complete)
The new tests also lock the heuristic in. They assert state === 'input-complete' right after the interleaved text. That makes the guess part of the public contract.
Smaller fix that covers the bug
- Stop calling
completeAllToolCallsForMessagefrom text events (TEXT_MESSAGE_CONTENTandTEXT_MESSAGE_END). - In
completeToolCall, setinputonly whenJSON.parseof the accumulated arguments succeeds. If parse fails, leaveinputunset. The rawargumentsstring stays the documented fallback.
Then TOOL_CALL_END still runs, because the call is not already input-complete. An aborted stream with truncated args does not publish a fake object.
That is about 10–15 lines. No flag. No reopen. No extra settled-part path.
Other open PRs for #1017
- #1019: 8-line reopen of any
input-completecall when more args arrive. Fixes the reported sequence. Still publishes truncatedinputif the stream dies. Can reopen after a realTOOL_CALL_END. - #1183: strict parse only. Stops the fake object at force-complete time, but
TOOL_CALL_ENDstill no-ops, so finalinputstays unset even after the full JSON arrived. That does not match the issue's primary ask.
Tests
I added two unit tests for the issue sequence on current main. Both fail today with the exact truncated input. They assert the user-visible contract (input equals the full object, or stays unset). They do not require text to complete tool calls.
If we land the smaller fix, keep a regression like that, plus the e2e interleaved-args scenario from this PR. Drop tests that require inferred completion.
AlemTuzlak
changed the title
|
Addressed the requested-changes review by shrinking the fix. The Proof: the three |
A TEXT_MESSAGE_CONTENT delta between TOOL_CALL_ARGS deltas force-completed the call from a lenient partial-JSON parse. TOOL_CALL_END then no-oped, so input stayed truncated while arguments held the full JSON. Text events no longer complete tool calls. completeToolCall sets input only when JSON.parse of the arguments succeeds. Fixes TanStack#1017
5b1dfe9 to
8775570
Compare
AlemTuzlak
changed the title
Replaced the inferredComplete machine with the smaller fix: text events no longer complete tool calls, and input is set only after JSON.parse succeeds.
|
Reshaped this PR to the smaller fix from review. The bug is real. The inferredComplete flag, reopen path, and settled-part guards are gone. Completing tool calls on text was the cause. TOOL_CALL_END and RUN_FINISHED already complete them. What landed:
@tanstack/ai unit tests: 1589 passed. |
Add coverage for END.input after interleaved text, two parallel calls, and a full TEXT_START/CONTENT/END block between arg deltas. Retarget toolCallToMessage when TEXT_MESSAGE_START remaps the assistant message id, so the later ARGS deltas still accumulate.
|
Closed the two overlapping PRs as duplicates of this one:
#1020 is the one that fixes #1017: text no longer completes tool calls, and input is set only after JSON.parse succeeds. |
2 participants
A
TEXT_MESSAGE_CONTENTevent between twoTOOL_CALL_ARGSdeltas permanently corrupted tool-callinput. The processor guessed that text meant args were done, filledinputfrom a lenient partial-JSON parse of the truncated string, then skipped the laterTOOL_CALL_END.argumentsheld the full JSON.inputheld a truncated fake object.statesaidinput-complete.This PR stops that guess. Text events no longer complete tool calls.
completeToolCallsetsinputonly whenJSON.parseof the accumulated arguments succeeds. Incomplete JSON leavesinputunset. The rawargumentsstring is the fallback.Changes
completeAllToolCallsForMessagefromTEXT_MESSAGE_CONTENTandTEXT_MESSAGE_END.JSON.parsewhen completing a tool call.inputwith a partial-JSON parse #1017 sequence.interleaved-argsscenario that streams text between arg deltas.The earlier
inferredCompletestate machine is gone. Completing on text was the bug.TOOL_CALL_ENDandRUN_FINISHEDalready cover completion.Checklist
pnpm run test:pr.packages/ai/docs/chat-architecture.mdfor this change. No publicdocs/page covers this processor contract.pnpm changeset), or this PR does not change a published package.Release Impact
Testing
Commands run:
pnpm exec vitest runinpackages/ai: 1589 passed.pnpm exec oxlint src --type-awareinpackages/ai: 0 errors.pnpm exec tsc --noEmitinpackages/ai: passed.interleaved-args.spec.tsdid not run locally. This worktree has no built workspace packages for the Playwright app. CI will run it.Manual test:
TOOL_CALL_ARGSwith truncated JSON, then oneTEXT_MESSAGE_CONTENT, then the rest of the args, thenTOOL_CALL_END.part.inputequals the full object, not a truncated parse.inputunset.How this PR makes testing easy: three unit tests in
stream-processor.test.tsplustesting/e2e/tests/tools-test/interleaved-args.spec.ts.Linked issues
Fixes #1017
Risk / rollback
Low. Tool completion now waits for
TOOL_CALL_ENDorRUN_FINISHED, which the adapter contract already requires. Revert the PR if a client depended oninput-completeat the first text delta.Public API change
Callers still read
ToolCallPart.input. The value is now the strict parse of complete arguments, or unset. No new exports.