fix: resolve shared AI-SDK tool-call dedup and error handling gaps (#797)#799
Draft
easonLiangWorldedtech wants to merge 4 commits into
Draft
Conversation
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR wraps OpenAI-compatible provider errors through a shared handleOpenAIError utility, extends tool-call streaming metadata with names, and updates NativeToolCallParser and Task to track streaming tool calls with compound id+name keys. It also adds tests for the new provider, parser, and streaming flows. ChangesProvider error handling and validation
Tool-call name tracking and parser coverage
Compound-key tool-call tracking in Task
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant OpenAICompatibleHandler
participant AISDK as ai
participant ErrorHandler as handleOpenAIError
Caller->>OpenAICompatibleHandler: createMessage() / completePrompt()
OpenAICompatibleHandler->>AISDK: streamText() / generateText()
AISDK-->>OpenAICompatibleHandler: throws error
OpenAICompatibleHandler->>ErrorHandler: handleOpenAIError(error, providerName)
ErrorHandler-->>OpenAICompatibleHandler: wrapped error
OpenAICompatibleHandler-->>Caller: throws wrapped error
sequenceDiagram
participant Stream as tool_call_partial events
participant Task
participant Indices as streamingToolCallIndices
Stream->>Task: tool_call_start(id, name)
Task->>Indices: insert key = id::name
Stream->>Task: tool_call_delta(id, name)
Task->>Indices: lookup key = id::name
Stream->>Task: tool_call_end(id, name)
Task->>Indices: lookup and delete key = id::name
Possibly related issues
Suggested labels: 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/core/assistant-message/NativeToolCallParser.ts (1)
53-63: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftThread
(id, name)through streaming tool calls
NativeToolCallParserstill keysstreamingToolCallsby bareid, so reused backend ids can overwrite another in-flight tool call’s accumulated JSON.processFinishReason()also dropsname, andTask.tsreadsgetStreamingToolName(event.id)after finalization, which turns the end-path dedup key intoid::undefined.
Use the compound key instartStreamingToolCall,processStreamingChunk,finalizeStreamingToolCall, andgetStreamingToolName, and includenameon everytool_call_end.🤖 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 `@src/core/assistant-message/NativeToolCallParser.ts` around lines 53 - 63, Thread the tool call name through streaming state so reused backend ids don’t collide and finalization keeps the correct lookup key. Update NativeToolCallParser’s startStreamingToolCall, processStreamingChunk, finalizeStreamingToolCall, and getStreamingToolName to key streamingToolCalls by the compound (id, name) instead of bare id, and make processFinishReason preserve name when emitting tool_call_end events. Ensure Task.ts can still resolve the streamed tool name after finalization by reading the same compound key path.
🧹 Nitpick comments (1)
src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts (1)
346-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of
finalizeRawChunks(), but the actual argument-accumulation collision isn't tested.Both new tests only exercise
processRawChunk/finalizeRawChunks(index-keyedrawChunkTracker), notstartStreamingToolCall/processStreamingChunk/finalizeStreamingToolCall(id-keyedstreamingToolCalls). Since the latter map is where the same-id-different-name collision actually corrupts data (see comment onNativeToolCallParser.ts), consider adding a test that callsstartStreamingToolCall("dup_id", "tool_a")andstartStreamingToolCall("dup_id", "tool_b")before either finalizes, then verifies both tools' accumulated arguments remain distinct and correct.As per coding guidelines, "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."
🤖 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 `@src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts` around lines 346 - 411, The new coverage is testing raw-chunk finalization, but it misses the real collision path in streaming state. Add a unit test against NativeToolCallParser’s streaming flow using startStreamingToolCall, processStreamingChunk, and finalizeStreamingToolCall to simulate two tools with the same id and different names. Verify that each streamingToolCalls entry keeps its own accumulated arguments and that finalization returns distinct, correct results for both tool names.Source: Coding guidelines
🤖 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 `@src/core/assistant-message/NativeToolCallParser.ts`:
- Around line 53-63: Thread the tool call name through streaming state so reused
backend ids don’t collide and finalization keeps the correct lookup key. Update
NativeToolCallParser’s startStreamingToolCall, processStreamingChunk,
finalizeStreamingToolCall, and getStreamingToolName to key streamingToolCalls by
the compound (id, name) instead of bare id, and make processFinishReason
preserve name when emitting tool_call_end events. Ensure Task.ts can still
resolve the streamed tool name after finalization by reading the same compound
key path.
---
Nitpick comments:
In `@src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts`:
- Around line 346-411: The new coverage is testing raw-chunk finalization, but
it misses the real collision path in streaming state. Add a unit test against
NativeToolCallParser’s streaming flow using startStreamingToolCall,
processStreamingChunk, and finalizeStreamingToolCall to simulate two tools with
the same id and different names. Verify that each streamingToolCalls entry keeps
its own accumulated arguments and that finalization returns distinct, correct
results for both tool names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 571fc390-4957-406d-a94b-30db466fc4e9
📒 Files selected for processing (7)
src/api/providers/__tests__/openai-compatible.spec.tssrc/api/providers/openai-compatible.tssrc/api/transform/stream.tssrc/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/__tests__/NativeToolCallParser.spec.tssrc/core/task/Task.tssrc/core/task/__tests__/duplicate-tool-use-ids.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/api/providers/__tests__/openai-compatible.spec.ts (1)
174-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak assertion doesn't validate tool-call chunk content.
The test only checks
chunks.lengthat Line 200, never verifying thattool-call-start/tool-call-delta/tool-call-endstream parts are actually converted into the expected chunk types with correcttoolCallId/name/argumentdata. Given this PR's core fix is tool-call name propagation and id+name compound-key dedup, this test should assert on the actual chunk shape to catch regressions in that path.As per coding guidelines, spec files should cover "state transitions ... and error handling", which this test currently doesn't exercise meaningfully.
♻️ Proposed stronger assertions
const stream = handler.createMessage(systemPrompt, messages) const chunks: any[] = [] for await (const chunk of stream) { chunks.push(chunk) } - expect(chunks.length).toBeGreaterThan(0) + const toolCallChunks = chunks.filter((chunk) => chunk.type?.startsWith("tool_call")) + expect(toolCallChunks.length).toBeGreaterThan(0) + const startChunk = toolCallChunks.find((c) => c.type === "tool_call_start") + expect(startChunk).toMatchObject({ id: "tc_1", name: "read_file" }) })🤖 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 `@src/api/providers/__tests__/openai-compatible.spec.ts` around lines 174 - 201, The stream test in openai-compatible.spec.ts is too weak because it only checks that some chunks were produced and never verifies tool-call event mapping. Update the test around handler.createMessage/mockStreamText to assert the actual chunk contents for tool-call-start, tool-call-delta, and tool-call-end, including toolCallId, propagated name, and parsed argument data. Use the existing mockFullStream and the returned chunks array to validate the expected chunk shapes instead of just checking chunks.length.Source: Coding guidelines
src/core/assistant-message/__tests__/NativeToolCallParser-additional.spec.ts (1)
75-86: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLoosened assertion defeats the purpose of the test.
The comment claims MCP tools return
"mcp_tool_use"type, but the assertion permits eithertool_useormcp_tool_use, so a regression that returns the wrong type would go undetected.🧪 Proposed fix to assert the exact expected type
- expect(result?.type).toMatch(/^(tool_use|mcp_tool_use)$/) + expect(result?.type).toBe("mcp_tool_use")🤖 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 `@src/core/assistant-message/__tests__/NativeToolCallParser-additional.spec.ts` around lines 75 - 86, The test in NativeToolCallParser-additional.spec.ts is too permissive because finalizeStreamingToolCall is allowed to return either type, which masks regressions. Tighten the assertion in the "should return final McpToolUse for MCP tools on finalize" case to expect the exact MCP-specific type returned by NativeToolCallParser.finalizeStreamingToolCall for mcp-- prefixed tool names, and keep the rest of the setup unchanged so the test verifies the intended behavior precisely.Source: Coding guidelines
🤖 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
`@src/core/assistant-message/__tests__/NativeToolCallParser-additional.spec.ts`:
- Around line 96-112: The test case is asserting too weakly and its setup
contradicts the scenario it claims to cover. Update the NativeToolCallParser
spec to exercise the actual “not started” path by passing data that does not
trigger a start event in processRawChunk, then assert the exact events returned
by finalizeRawChunks rather than only checking Array.isArray. Use
clearRawChunkState, processRawChunk, and finalizeRawChunks to verify the
intended state transition and rename the test if needed so the title matches the
behavior being validated.
In `@src/core/task/__tests__/Task.streaming-tool-calls.spec.ts`:
- Around line 553-733: The Task streaming-tool-call integration tests only check
that cline is defined, which does not verify any state transition or parsing
behavior. Update the tests in Task.streaming-tool-calls.spec to assert
observable outcomes after attemptApiRequest(0) and iterator.next(), such as
assistantMessageContent, userMessageContent, or streaming-tool-call state, using
the existing Task instance and mock stream generators. For the duplicate
tool_call_start and lifecycle cases, add assertions that the dedup/path handling
actually occurred, or spy on NativeToolCallParser / related internal methods so
the tests validate the raw-chunk routing they are meant to cover.
---
Nitpick comments:
In `@src/api/providers/__tests__/openai-compatible.spec.ts`:
- Around line 174-201: The stream test in openai-compatible.spec.ts is too weak
because it only checks that some chunks were produced and never verifies
tool-call event mapping. Update the test around
handler.createMessage/mockStreamText to assert the actual chunk contents for
tool-call-start, tool-call-delta, and tool-call-end, including toolCallId,
propagated name, and parsed argument data. Use the existing mockFullStream and
the returned chunks array to validate the expected chunk shapes instead of just
checking chunks.length.
In
`@src/core/assistant-message/__tests__/NativeToolCallParser-additional.spec.ts`:
- Around line 75-86: The test in NativeToolCallParser-additional.spec.ts is too
permissive because finalizeStreamingToolCall is allowed to return either type,
which masks regressions. Tighten the assertion in the "should return final
McpToolUse for MCP tools on finalize" case to expect the exact MCP-specific type
returned by NativeToolCallParser.finalizeStreamingToolCall for mcp-- prefixed
tool names, and keep the rest of the setup unchanged so the test verifies the
intended behavior precisely.
🪄 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: f191ed26-f5c7-4454-bb03-95535202595b
📒 Files selected for processing (3)
src/api/providers/__tests__/openai-compatible.spec.tssrc/core/assistant-message/__tests__/NativeToolCallParser-additional.spec.tssrc/core/task/__tests__/Task.streaming-tool-calls.spec.ts
Comment on lines
+96
to
+112
| it("should not emit events for tools that haven't started", () => { | ||
| NativeToolCallParser.clearRawChunkState() | ||
|
|
||
| // process a raw chunk without hasStarted (just tracking metadata) | ||
| // This simulates the case where only arguments are received without start | ||
| const result = NativeToolCallParser.processRawChunk({ | ||
| index: 1, | ||
| id: "toolu_partial", | ||
| name: "read_file", | ||
| arguments: undefined, // No arguments means no delta event, but still tracks | ||
| }) | ||
|
|
||
| // finalize should not emit for tools that haven't hasStarted | ||
| const events = NativeToolCallParser.finalizeRawChunks() | ||
| // The behavior depends on implementation - just verify it doesn't crash | ||
| expect(Array.isArray(events)).toBe(true) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test doesn't verify its own premise and assertion is non-specific.
Since name: "read_file" is passed in the chunk, per processRawChunk's logic a tool_call_start event fires immediately (tracked.name is set on creation, then !tracked.hasStarted && tracked.name triggers the start event) — the tool call has started. The title "should not emit events for tools that haven't started" doesn't match what's actually exercised, and the assertion expect(Array.isArray(events)).toBe(true) will pass regardless of the actual event count, making the test unable to catch regressions.
🧪 Proposed fix to assert actual expected behavior
- it("should not emit events for tools that haven't started", () => {
+ it("should emit an end event for a tool call that already started (name provided upfront)", () => {
NativeToolCallParser.clearRawChunkState()
-
- // process a raw chunk without hasStarted (just tracking metadata)
- // This simulates the case where only arguments are received without start
- const result = NativeToolCallParser.processRawChunk({
+ NativeToolCallParser.processRawChunk({
index: 1,
id: "toolu_partial",
name: "read_file",
- arguments: undefined, // No arguments means no delta event, but still tracks
+ arguments: undefined,
})
-
- // finalize should not emit for tools that haven't hasStarted
const events = NativeToolCallParser.finalizeRawChunks()
- // The behavior depends on implementation - just verify it doesn't crash
- expect(Array.isArray(events)).toBe(true)
+ expect(events).toEqual([{ type: "tool_call_end", id: "toolu_partial", name: "read_file" }])
})As per coding guidelines, **/*.{test,spec}.{ts,tsx,js} should test "state transitions" precisely; this test's title and assertion don't match the actual observable state transition.
📝 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.
Suggested change
| it("should not emit events for tools that haven't started", () => { | |
| NativeToolCallParser.clearRawChunkState() | |
| // process a raw chunk without hasStarted (just tracking metadata) | |
| // This simulates the case where only arguments are received without start | |
| const result = NativeToolCallParser.processRawChunk({ | |
| index: 1, | |
| id: "toolu_partial", | |
| name: "read_file", | |
| arguments: undefined, // No arguments means no delta event, but still tracks | |
| }) | |
| // finalize should not emit for tools that haven't hasStarted | |
| const events = NativeToolCallParser.finalizeRawChunks() | |
| // The behavior depends on implementation - just verify it doesn't crash | |
| expect(Array.isArray(events)).toBe(true) | |
| }) | |
| it("should emit an end event for a tool call that already started (name provided upfront)", () => { | |
| NativeToolCallParser.clearRawChunkState() | |
| NativeToolCallParser.processRawChunk({ | |
| index: 1, | |
| id: "toolu_partial", | |
| name: "read_file", | |
| arguments: undefined, | |
| }) | |
| const events = NativeToolCallParser.finalizeRawChunks() | |
| expect(events).toEqual([{ type: "tool_call_end", id: "toolu_partial", name: "read_file" }]) | |
| }) |
🤖 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 `@src/core/assistant-message/__tests__/NativeToolCallParser-additional.spec.ts`
around lines 96 - 112, The test case is asserting too weakly and its setup
contradicts the scenario it claims to cover. Update the NativeToolCallParser
spec to exercise the actual “not started” path by passing data that does not
trigger a start event in processRawChunk, then assert the exact events returned
by finalizeRawChunks rather than only checking Array.isArray. Use
clearRawChunkState, processRawChunk, and finalizeRawChunks to verify the
intended state transition and rename the test if needed so the title matches the
behavior being validated.
Source: Coding guidelines
Comment on lines
+553
to
+733
| describe("tool_call_partial chunk handling - Task integration", () => { | ||
| it("should handle tool_call_start event from raw chunk processing", async () => { | ||
| const cline = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task with tool_call_start", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| vi.spyOn(cline as any, "getSystemPrompt").mockResolvedValue("mock system prompt") | ||
|
|
||
| async function* mockStreamGenerator() { | ||
| yield { | ||
| type: "tool_call_partial" as const, | ||
| index: 0, | ||
| id: "toolu_123", | ||
| name: "read_file", | ||
| arguments: '{"path":"a.ts"}', | ||
| } | ||
| yield { type: "text" as const, text: "response" } | ||
| } | ||
|
|
||
| vi.spyOn(cline.api, "createMessage").mockReturnValue(mockStreamGenerator()) | ||
|
|
||
| const iterator = cline.attemptApiRequest(0) | ||
| await iterator.next() | ||
|
|
||
| expect(cline).toBeDefined() | ||
| }) | ||
|
|
||
| it("should handle duplicate tool_call_start events", async () => { | ||
| const cline = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task with duplicate start", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| vi.spyOn(cline as any, "getSystemPrompt").mockResolvedValue("mock system prompt") | ||
|
|
||
| async function* mockStreamGenerator() { | ||
| yield { | ||
| type: "tool_call_partial" as const, | ||
| index: 0, | ||
| id: "toolu_dup123", | ||
| name: "read_file", | ||
| arguments: '{"path":"test.ts"}', | ||
| } | ||
| yield { | ||
| type: "tool_call_partial" as const, | ||
| index: 0, | ||
| id: "toolu_dup123", | ||
| name: "read_file", | ||
| arguments: '{"path":"test.ts"}', | ||
| } | ||
| yield { type: "text" as const, text: "response" } | ||
| } | ||
|
|
||
| vi.spyOn(cline.api, "createMessage").mockReturnValue(mockStreamGenerator()) | ||
|
|
||
| const iterator = cline.attemptApiRequest(0) | ||
| await iterator.next() | ||
|
|
||
| expect(cline).toBeDefined() | ||
| }) | ||
|
|
||
| it("should handle tool_call_delta event", async () => { | ||
| const cline = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task with delta", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| vi.spyOn(cline as any, "getSystemPrompt").mockResolvedValue("mock system prompt") | ||
|
|
||
| async function* mockStreamGenerator() { | ||
| yield { type: "tool_call_delta" as const, id: "toolu_delta123", delta: '{"path":"' } | ||
| yield { type: "text" as const, text: "response" } | ||
| } | ||
|
|
||
| vi.spyOn(cline.api, "createMessage").mockReturnValue(mockStreamGenerator()) | ||
|
|
||
| const iterator = cline.attemptApiRequest(0) | ||
| await iterator.next() | ||
|
|
||
| expect(cline).toBeDefined() | ||
| }) | ||
|
|
||
| it("should handle tool_call_end event", async () => { | ||
| const cline = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task with end", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| vi.spyOn(cline as any, "getSystemPrompt").mockResolvedValue("mock system prompt") | ||
|
|
||
| async function* mockStreamGenerator() { | ||
| yield { type: "tool_call_end" as const, id: "toolu_end123" } | ||
| yield { type: "text" as const, text: "response" } | ||
| } | ||
|
|
||
| vi.spyOn(cline.api, "createMessage").mockReturnValue(mockStreamGenerator()) | ||
|
|
||
| const iterator = cline.attemptApiRequest(0) | ||
| await iterator.next() | ||
|
|
||
| expect(cline).toBeDefined() | ||
| }) | ||
|
|
||
| it("should handle complete streaming lifecycle: start -> delta -> end", async () => { | ||
| const cline = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task with full lifecycle", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| vi.spyOn(cline as any, "getSystemPrompt").mockResolvedValue("mock system prompt") | ||
|
|
||
| async function* mockStreamGenerator() { | ||
| yield { | ||
| type: "tool_call_partial" as const, | ||
| index: 0, | ||
| id: "toolu_lifecycle123", | ||
| name: "read_file", | ||
| arguments: '{"path":"test.ts"}', | ||
| } | ||
| yield { type: "tool_call_delta" as const, id: "toolu_lifecycle123", delta: "more_args" } | ||
| yield { type: "tool_call_end" as const, id: "toolu_lifecycle123" } | ||
| yield { type: "text" as const, text: "Done" } | ||
| } | ||
|
|
||
| vi.spyOn(cline.api, "createMessage").mockReturnValue(mockStreamGenerator()) | ||
|
|
||
| const iterator = cline.attemptApiRequest(0) | ||
| await iterator.next() | ||
|
|
||
| expect(cline).toBeDefined() | ||
| }) | ||
|
|
||
| it("should handle multiple sequential tool calls", async () => { | ||
| const cline = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task with multiple tools", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| vi.spyOn(cline as any, "getSystemPrompt").mockResolvedValue("mock system prompt") | ||
|
|
||
| async function* mockStreamGenerator() { | ||
| yield { | ||
| type: "tool_call_partial" as const, | ||
| index: 0, | ||
| id: "toolu_multi1", | ||
| name: "read_file", | ||
| arguments: '{"path":"file1.ts"}', | ||
| } | ||
| yield { type: "tool_call_end" as const, id: "toolu_multi1" } | ||
| yield { | ||
| type: "tool_call_partial" as const, | ||
| index: 1, | ||
| id: "toolu_multi2", | ||
| name: "write_to_file", | ||
| arguments: '{"path":"file2.ts","content":"hello"}', | ||
| } | ||
| yield { type: "tool_call_end" as const, id: "toolu_multi2" } | ||
| yield { type: "text" as const, text: "Done multiple tools" } | ||
| } | ||
|
|
||
| vi.spyOn(cline.api, "createMessage").mockReturnValue(mockStreamGenerator()) | ||
|
|
||
| const iterator = cline.attemptApiRequest(0) | ||
| await iterator.next() | ||
|
|
||
| expect(cline).toBeDefined() | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Task integration tests only assert expect(cline).toBeDefined(), not actual behavior.
Every test in this describe block (should handle tool_call_start..., duplicate tool_call_start events, tool_call_delta, tool_call_end, complete streaming lifecycle, multiple sequential tool calls) drives the stream and then only checks cline is defined — a condition already true before the stream is processed. None assert on cline.userMessageContent, assistantMessageContent, or internal streaming-tool-call state, so these tests can't catch regressions in the raw-chunk/tool-call routing they claim to cover (especially "should handle duplicate tool_call_start events", which is meant to exercise the compound-key dedup path this PR introduces).
Consider asserting on observable state after iterator.next(), e.g. the resulting assistantMessageContent/userMessageContent entries, or a spy on NativeToolCallParser/dedup-related internals, so these integration tests actually validate the streaming lifecycle instead of merely confirming the code doesn't throw.
As per coding guidelines, **/*.{test,spec}.{ts,tsx,js} should test "state transitions... and error handling" — these tests don't verify any state transition.
🤖 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 `@src/core/task/__tests__/Task.streaming-tool-calls.spec.ts` around lines 553 -
733, The Task streaming-tool-call integration tests only check that cline is
defined, which does not verify any state transition or parsing behavior. Update
the tests in Task.streaming-tool-calls.spec to assert observable outcomes after
attemptApiRequest(0) and iterator.next(), such as assistantMessageContent,
userMessageContent, or streaming-tool-call state, using the existing Task
instance and mock stream generators. For the duplicate tool_call_start and
lifecycle cases, add assertions that the dedup/path handling actually occurred,
or spy on NativeToolCallParser / related internal methods so the tests validate
the raw-chunk routing they are meant to cover.
Source: Coding guidelines
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
removed
the
awaiting-review
- Add NativeToolCallParser tests for hasActiveStreamingToolCalls() and getStreamingToolName() - Add openai-compatible tests for streamText multiple parts and usage handling - Add Task integration tests for tool_call_start/delta/end lifecycle
easonLiangWorldedtech
force-pushed
the
fix/shared-sdk-error-handle-gap
branch
from
fdd3770 to
26ffe1d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix shared AI-SDK tool-call streaming/error-handling gaps (#797)
Problem
src/api/transform/ai-sdk.ts'sprocessAiSdkStreamPartandsrc/api/providers/openai-compatible.ts(the shared base class used by Novita, Moonshot, and Poe) have two confirmed defects:1. Missing error handling in OpenAICompatibleHandler
createMessage()/completePrompt()have no try/catch, unlike 10+ sibling providers that callhandleOpenAIError/handleProviderError.status, soTask.ts'sbackoffAndAnnounce(error?.status === 429) never fires 429-aware retry/backoff for Novita/Moonshot/PoeFix: Wrap both methods in try/catch, throw via
handleOpenAIError(error, this.config.providerName)2. Tool-call dedup keyed only on toolCallId
toolCallIdwithout checkingtoolName/argumentsFix: Change all 9 usages of
streamingToolCallIndicesto use compound key(id, name)—dedupKey = ${event.id}::${event.name}Changes
src/api/providers/openai-compatible.tssrc/core/task/Task.tssrc/api/transform/stream.tsname?: stringfieldsrc/core/assistant-message/NativeToolCallParser.tsname: tracked.nameTests Added
openai-compatible.spec.ts: 7 tests covering normal streaming, 429, 500, 400/500 errors with .status and provider name verificationNativeToolCallParser.spec.ts: 2 tests verifying compound key dedup for same-id-different-payload collisionduplicate-tool-use-ids.spec.ts: 134-line regression test suiteRegression Scope
This logic is shared across every OpenAICompatibleHandler-based provider (Moonshot, Poe, Novita). Fixes apply to all of them.
Related
Summary by CodeRabbit