Uh oh!
There was an error while loading. Please reload this page.
fix: preserve signed thinking order around provider tools - #931
Conversation
Warning Review limit reached
Next review available in:16 minutes Limit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 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)
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 |
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
asyncfunctionstreamChatToJsonResponse(options: Parameters<typeofchat>[0],): Promise<Response>{constchunks: Array<unknown>=[]try{forawait(constchunkofchat(options)){chunks.push(chunk)}}catch(error){returnnewResponse(JSON.stringify({ chunks,error: errorinstanceofError ? error.message : String(error),}),{status: 200,headers: {'Content-Type': 'application/json'}},)}returnnewResponse(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
View your CI Pipeline Execution ↗ for commit 9a6b74e
☁️ 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-mcp@tanstack/ai-memory@tanstack/ai-mistral@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-vue@tanstack/ai-vue-ui@tanstack/openai-base@tanstack/preact-ai-devtools@tanstack/react-ai-devtools@tanstack/solid-ai-devtoolscommit: |
Thanks for the PR, @ekkoitac! 🙌 @AlemTuzlak will take a look. Automated pre-review checks
Automated triage — a human review follows. |
c81fc67 to
5091b5bComparetombeckenham
commented
Aug 20, 2026
Maintainer sweep: rebased onto |
5091b5b to
9a6b74eComparetombeckenham
commented
Aug 20, 2026
Maintainer sweep: rebased onto |
🎯 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