fix(client-js): tolerate tool-result chunks on resume streams#16385
fix(client-js): tolerate tool-result chunks on resume streams#16385machester4 wants to merge 2 commits into
Conversation
processChatResponse and processChatResponse_vNext threw `tool_result must
be preceded by a tool_call` whenever a tool-result chunk arrived without
a matching tool-call in the same stream. Resume streams (approve-tool-call,
decline-tool-call, resume-stream) legitimately start with a tool-result
whose matching tool-call was emitted in the prior turn, so this validator
fired on every HITL approve/decline. The error is caught by the outer
.catch and only logged via console.error("Error processing stream
response: ..."), so user UX is unaffected, but it pollutes logs and
masks real errors.
Fix: when toolInvocations is null at the time the tool-result chunk
arrives, initialize it to [] and synthesize a result-only invocation.
Strict mid-stream desync detection is preserved: once any tool-call has
populated the buffer, an unmatched toolCallId still throws
`tool_result must be preceded by a tool_call with the same toolCallId`.
Adds two tests:
- a tool-result arriving as the first chunk does not throw and produces
a result-only invocation.
- a tool-result with no matching toolCallId mid-stream still throws.
🦋 Changeset detectedLatest commit: e990a23 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
@machester4 is attempting to deploy a commit to the Mastra Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughResume streams now accept tool-result chunks at their start by synthesizing result-only invocations when no prior tool-call has been buffered. Existing desync validation for unmatched toolCallIds after a tool-call buffer is populated is preserved. The change spans dual sync/async handlers, documentation, and test coverage. ChangesResume Stream Tool-Result Tolerance
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Thank you for your contribution! Please ensure that your PR fixes an existing issue and that you have linked it in the description (e.g. with We use CodeRabbit for automated code reviews. Please address all feedback from CodeRabbit by either making changes to your PR or leaving a comment explaining why you disagree with the feedback. Since CodeRabbit is an AI, it may occasionally provide incorrect feedback. Addressing CodeRabbit's feedback will greatly increase the chances of your PR being merged. We appreciate your understanding and cooperation in helping us maintain high code quality standards. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.changeset/client-js-tolerate-resume-tool-result.md:
- Around line 5-9: The changeset is too implementation‑focused; rewrite it to
state the user‑facing outcome in one or two short sentences (e.g., "Resume
streams no longer log spurious 'tool-result' errors during
approve/decline/polling."), remove internal implementation details and function
names (do not mention processChatResponse, processChatResponse_vNext,
processStreamResponse, buffering, catch paths, or desync logic), and keep the
tone brief and release‑note appropriate per the guidelines.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 949462b5-33e1-444c-8527-3a47a9543963
📒 Files selected for processing (3)
.changeset/client-js-tolerate-resume-tool-result.mdclient-sdks/client-js/src/resources/agent.test.tsclient-sdks/client-js/src/resources/agent.ts
| Tolerate `tool-result` chunks at the start of resume streams (`approve-tool-call`, `decline-tool-call`, `resume-stream`). | ||
|
|
||
| Previously, `processChatResponse` and `processChatResponse_vNext` threw `tool_result must be preceded by a tool_call` whenever a `tool-result` chunk arrived without a matching `tool-call` in the same stream. Because `processStreamResponse` always passes `lastMessage: undefined`, resume streams (which legitimately start with a `tool-result` whose matching `tool-call` was emitted in the prior turn) would trip this validation on every HITL approve/decline and pollute logs with `Error processing stream response: tool_result must be preceded by a tool_call` via the outer `.catch`. | ||
|
|
||
| The fix synthesizes a result-only invocation when `toolInvocations` is null at the time the chunk arrives. Strict desync detection is preserved: once a `tool-call` has populated the buffer, an unmatched `toolCallId` still throws `tool_result must be preceded by a tool_call with the same toolCallId`. |
There was a problem hiding this comment.
Changeset text is too implementation-heavy for release notes.
Please rewrite this entry to focus on user-visible outcome (resume streams no longer log false tool-result errors) and keep internals (processChatResponse*, buffer details, catch path) out of the changelog body.
Suggested rewrite
-Tolerate `tool-result` chunks at the start of resume streams (`approve-tool-call`, `decline-tool-call`, `resume-stream`).
-
-Previously, `processChatResponse` and `processChatResponse_vNext` threw `tool_result must be preceded by a tool_call` whenever a `tool-result` chunk arrived without a matching `tool-call` in the same stream. Because `processStreamResponse` always passes `lastMessage: undefined`, resume streams (which legitimately start with a `tool-result` whose matching `tool-call` was emitted in the prior turn) would trip this validation on every HITL approve/decline and pollute logs with `Error processing stream response: tool_result must be preceded by a tool_call` via the outer `.catch`.
-
-The fix synthesizes a result-only invocation when `toolInvocations` is null at the time the chunk arrives. Strict desync detection is preserved: once a `tool-call` has populated the buffer, an unmatched `toolCallId` still throws `tool_result must be preceded by a tool_call with the same toolCallId`.
+Fixed resume streams to accept a `tool-result` chunk as the first event in `approve-tool-call`, `decline-tool-call`, and `resume-stream`.
+
+This removes false validation errors and noisy logs during resume flows while keeping mismatch validation for genuinely out-of-sync tool events.[potential_issue]
As per coding guidelines: “Highlight outcomes! … Do not focus on internal implementation details” and “Write short, direct sentences”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Tolerate `tool-result` chunks at the start of resume streams (`approve-tool-call`, `decline-tool-call`, `resume-stream`). | |
| Previously, `processChatResponse` and `processChatResponse_vNext` threw `tool_result must be preceded by a tool_call` whenever a `tool-result` chunk arrived without a matching `tool-call` in the same stream. Because `processStreamResponse` always passes `lastMessage: undefined`, resume streams (which legitimately start with a `tool-result` whose matching `tool-call` was emitted in the prior turn) would trip this validation on every HITL approve/decline and pollute logs with `Error processing stream response: tool_result must be preceded by a tool_call` via the outer `.catch`. | |
| The fix synthesizes a result-only invocation when `toolInvocations` is null at the time the chunk arrives. Strict desync detection is preserved: once a `tool-call` has populated the buffer, an unmatched `toolCallId` still throws `tool_result must be preceded by a tool_call with the same toolCallId`. | |
| '@mastra/client-js': patch | |
| --- | |
| Fixed resume streams to accept a `tool-result` chunk as the first event in `approve-tool-call`, `decline-tool-call`, and `resume-stream`. | |
| This removes false validation errors and noisy logs during resume flows while keeping mismatch validation for genuinely out-of-sync tool events. |
🤖 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 @.changeset/client-js-tolerate-resume-tool-result.md around lines 5 - 9, The
changeset is too implementation‑focused; rewrite it to state the user‑facing
outcome in one or two short sentences (e.g., "Resume streams no longer log
spurious 'tool-result' errors during approve/decline/polling."), remove internal
implementation details and function names (do not mention processChatResponse,
processChatResponse_vNext, processStreamResponse, buffering, catch paths, or
desync logic), and keep the tone brief and release‑note appropriate per the
guidelines.
Demonstrates the deeper bug fixed by tolerating a `tool-result` as the first chunk of a resume stream: when `resumeStream` is called with `clientTools` and the resume cycle emits a `tool-call` for one of those client tools after the suspended tool's result, the client tool's `execute` must run and trigger the recursive stream. Pre-fix: the leading `tool-result` chunk threw inside `processChatResponse_vNext`, the outer `.catch` ran, and `onFinish` never fired — so the recursion path was unreachable and the client tool was silently never executed (UX-level bug, not just log noise).
1 participant
Problem
processChatResponseandprocessChatResponse_vNext(inclient-sdks/client-js/src/resources/agent.ts) throwError: tool_result must be preceded by a tool_callwhenever atool-resultchunk arrives without a matchingtool-callin the same stream.Resume streams —
resume-stream,approve-tool-call, anddecline-tool-call— legitimately violate that invariant: they start with atool-resultwhose matchingtool-callwas emitted in the prior turn. Combined withprocessStreamResponsealways passinglastMessage: undefinedtoprocessChatResponse_vNext, the in-memorytoolInvocationsbuffer is always empty when the first chunk arrives, so the validator throws on every resume.The throw is caught by the outer
.catchinprocessStreamResponseand only logged viaconsole.error("Error processing stream response:", error). That makes the failure look cosmetic at first glance, but it is not.Two distinct impacts
1.
resumeStream+clientTools: real, observable bug (UX-level).When
resumeStreamis called withclientToolsand the resume cycle emits anothertool-callfor one of those client tools after the suspended tool's result, the throw aborts processing insideprocessChatResponse_vNext.onFinishnever fires, so the recursive client-tool execution path inprocessStreamResponsenever runs — the client tool'sexecuteis silently never called. From the user's perspective, the agent simply stops mid-conversation: the tool that should have run client-side just doesn't.This is what the new test
resumeStream: still runs client tools when the resume cycle starts with a tool-result (post-suspend)demonstrates and protects against.2.
approveToolCall/declineToolCall: noisy logs.These two methods don't expose
clientToolsin their public signatures, so the recursion path never applies. The resume stream still flows through to the consumer via the controller-side branch of the teed body, so users don't see any UX impact — justError processing stream response: tool_result must be preceded by a tool_callrepeated in logs on every HITL approval, masking real errors and making observability harder.Repro (minimal)
Verified the bug exists on every published version from
1.13.4through1.18.0-alpha.8and the most recent tip-of-main snapshot at the time of writing.Fix
When
toolInvocationsis null at the time thetool-resultchunk arrives, initialize it to[]and build a result-only invocation directly from the chunk payload. This is the legitimate resume-stream path.Strict mid-stream desync detection is preserved: once any
tool-callhas populated the buffer, an unmatchedtoolCallIdstill throwstool_result must be preceded by a tool_call with the same toolCallId. Only the first-chunk-of-resume case is relaxed.The same change is applied in both
processChatResponse(legacyonToolResultPart) andprocessChatResponse_vNext(case 'tool-result'), sinceprocessStreamResponseandprocessStreamResponseLegacyboth feed the resume-stream pattern.Tests
Three new tests:
agent.test.ts—should tolerate a tool-result as the first chunk (approve-tool-call / decline-tool-call resume): verifies the validator no longer throws on a leadingtool-resultand produces a result-only invocation.agent.test.ts—should still throw on mid-stream desync (tool-result without matching tool-call after another tool-call): verifies the strict desync check still fires once the buffer has been populated.agent.vnext.test.ts—resumeStream: still runs client tools when the resume cycle starts with a tool-result (post-suspend): end-to-end repro of scenario 1 above. Asserts thatexecuteSpyis called and the recursive fetch fires — both of which are unreachable on the unfixed SDK becauseonFinishnever runs.Local run on
client-sdks/client-js: 419 passing tests (3 new). The 2 unrelated failing tests (agent.test.ts > Agent.stream > should handle vNext step-finish...,agent.vnext.test.ts > Agent vNext > resumeStream: omits local seed messages...) were already failing onmainbefore this change — verified by stashing this PR's changes and re-running.Changeset
Patch bump for
@mastra/client-js. See.changeset/client-js-tolerate-resume-tool-result.md.Notes
resumeStream+clientTools+ post-suspend client tool calls now actually run; (b) absence of the spurious server-side log line onapproveToolCall/declineToolCallconsumers.toolInvocationswas previously null at the moment thetool-resultarrives, which is precisely the resume-stream scenario.ELI5
When resuming an interrupted conversation, the client sometimes saw harmless server-side errors because a tool result arrived that referenced a tool-call from the previous turn. This PR stops those noisy errors by accepting a result-only chunk when resuming, so resume flows proceed cleanly.
Summary
This PR fixes client-js handling of tool-result chunks at the start of resume streams (approve-tool-call, decline-tool-call, resume-stream). Previously, processChatResponse and processChatResponse_vNext threw "tool_result must be preceded by a tool_call" when a tool-result arrived without a matching tool-call in the same stream (the matching call was emitted in a prior turn). Because processStreamResponse passes lastMessage: undefined for resume streams, the in-memory toolInvocations buffer could be empty and the validator errored; the error was caught and logged, producing noisy server-side logs despite correct client behavior.
Fix: when a tool-result chunk arrives and toolInvocations is null, initialize toolInvocations = [] and synthesize a result-only ToolInvocation from the chunk payload. Preserve strict mid-stream desync detection: if the buffer was already populated by a prior tool-call, an unmatched toolCallId still throws. Applied fixes in both processChatResponse (legacy onToolResultPart) and processChatResponse_vNext (case 'tool-result').
Changes
client-sdks/client-js/src/resources/agent.ts (+39/-22)
client-sdks/client-js/src/resources/agent.test.ts (+67)
client-sdks/client-js/src/resources/agent.vnext.test.ts (+69)
.changeset/client-js-tolerate-resume-tool-result.md
Tests & impact