fix: preserve signed thinking order around provider tools#931
Conversation
📝 WalkthroughWalkthroughAssistant message conversion now preserves signed thinking order by starting a new assistant segment after provider-executed tool calls. Unit tests and an Anthropic end-to-end regression test validate provider and local tool-call ordering. ChangesSigned thinking order preservation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
testing/e2e/global-setup.tsParsing error: "parserOptions.project" has been provided for testing/e2e/tests/anthropic-server-tool.spec.tsParsing error: "parserOptions.project" has been provided for 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 (1)
packages/ai/src/activities/chat/messages.ts (1)
240-255: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve pending thinking when flushing an otherwise empty segment.
flushSegment()only emits whencontentortoolCallsexist, so the new provider-tool split can leave a trailing signedthinkingblock stranded inpendingThinkingand then dropped on the final flush. AddpendingThinking.length > 0to the guard and cover the terminal-thinking case in tests.🤖 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/messages.ts` around lines 240 - 255, The flushSegment function currently drops pending thinking when a segment has no content or tool calls. Include pendingThinking.length > 0 in its emission guard so thinking-only segments are appended and cleared correctly, and add a test covering a terminal thinking block during the final flush.
🧹 Nitpick comments (2)
packages/ai/tests/message-converters.test.ts (1)
387-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRound-trip assertion doesn't verify the thinking part survives.
The round-trip check only asserts the tool-call/tool-result parts are present; it never confirms
{ type: 'thinking', content: 'Thinking between local tool calls' }makes it throughmodelMessagesToUIMessages, despite that being the feature under test.♻️ Proposed addition
expect(roundTripped[0]?.parts).toEqual( expect.arrayContaining([ expect.objectContaining({ type: 'tool-call', id: 'tool-call-a' }), expect.objectContaining({ type: 'tool-call', id: 'tool-call-b' }), expect.objectContaining({ type: 'tool-result', toolCallId: 'tool-call-a', }), expect.objectContaining({ type: 'tool-result', toolCallId: 'tool-call-b', }), + expect.objectContaining({ + type: 'thinking', + content: 'Thinking between local tool calls', + }), ]), )🤖 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/message-converters.test.ts` around lines 387 - 404, Strengthen the round-trip assertion in the modelMessagesToUIMessages test to also verify the thinking part is preserved, asserting a part with type "thinking" and content "Thinking between local tool calls" alongside the existing tool-call and tool-result checks.testing/e2e/src/routes/api.anthropic-bug-test.ts (1)
32-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated stream-to-response boilerplate.
The
chunks/try-catch/Responsepattern at Lines 110-131 duplicates the existing pattern at Lines 143-174 almost verbatim. Given this route already hosts two bug-repro branches (#604,#910) and is likely to grow more, extracting a small shared helper (e.g.streamChatToJsonResponse(options)) would avoid drift between the branches.♻️ Suggested helper
async function streamChatToJsonResponse( options: Parameters<typeof chat>[0], ): Promise<Response> { const chunks: Array<unknown> = [] try { for await (const chunk of chat(options)) { chunks.push(chunk) } } catch (error) { return new Response( JSON.stringify({ chunks, error: error instanceof Error ? error.message : String(error), }), { status: 200, headers: { 'Content-Type': 'application/json' } }, ) } return new Response(JSON.stringify({ chunks, error: null }), { status: 200, headers: { 'Content-Type': 'application/json' }, }) }🤖 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/src/routes/api.anthropic-bug-test.ts` around lines 32 - 133, Extract the duplicated chunks collection, chat streaming, error handling, and JSON Response construction from the thinking-order branch and the other bug-repro branch into a shared streamChatToJsonResponse helper. Update both branches to pass their existing chat options through this helper, preserving the current response status, headers, chunk payload, and error serialization.
🤖 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/messages.ts`:
- Around line 240-255: The flushSegment function currently drops pending
thinking when a segment has no content or tool calls. Include
pendingThinking.length > 0 in its emission guard so thinking-only segments are
appended and cleared correctly, and add a test covering a terminal thinking
block during the final flush.
---
Nitpick comments:
In `@packages/ai/tests/message-converters.test.ts`:
- Around line 387-404: Strengthen the round-trip assertion in the
modelMessagesToUIMessages test to also verify the thinking part is preserved,
asserting a part with type "thinking" and content "Thinking between local tool
calls" alongside the existing tool-call and tool-result checks.
In `@testing/e2e/src/routes/api.anthropic-bug-test.ts`:
- Around line 32-133: Extract the duplicated chunks collection, chat streaming,
error handling, and JSON Response construction from the thinking-order branch
and the other bug-repro branch into a shared streamChatToJsonResponse helper.
Update both branches to pass their existing chat options through this helper,
preserving the current response status, headers, chunk payload, and error
serialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6e02e8c-73ff-4696-9e9c-dba6c8e6b869
📒 Files selected for processing (6)
.changeset/calm-thinkers-wait.mdpackages/ai/src/activities/chat/messages.tspackages/ai/tests/message-converters.test.tstesting/e2e/global-setup.tstesting/e2e/src/routes/api.anthropic-bug-test.tstesting/e2e/tests/anthropic-server-tool.spec.ts
1 participant
🎯 Changes
Anthropic requires each signed thinking block to remain in its original position relative to tool-use blocks. UI message replay could move a later thinking block ahead of an earlier provider-executed tool, causing Anthropic to reject the request.
This PR:
Fixes #910
✅ Checklist
pnpm run test:pr.Validation
pnpm --filter @tanstack/ai test:lib -- message-converters.test.ts— 1,138 tests passed across 63 filespnpm --filter @tanstack/ai-e2e test:e2e -- tests/anthropic-server-tool.spec.ts— 2 tests passedpnpm test:pr— 260 tasks passed across 63 projects; 687 declaration files scanned cleanpnpm --filter @tanstack/ai-e2e test:e2e— 349 passed, 1 gated live smoke skipped, no retries or flaky failures🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests