Conversation
There was a problem hiding this comment.
Pull request overview
Adds richer streaming event typing to the SDK so consumers can correlate tool invocations with their results (including parallel/out-of-order tool execution).
Changes:
- Extended
Chunkwith atoolCallIdontool-calland introduced a newtool-resultchunk type. - Updated stream event parsing to emit
tool-callids and to surfacetool_resultSSE events as structured chunks. - Updated/added Vitest coverage to validate tool call IDs and out-of-order tool results.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/sdk/src/types.ts |
Extends the public Chunk union with toolCallId and a new tool-result variant. |
packages/sdk/src/client.ts |
Emits toolCallId for tool calls and parses tool_result events into tool-result chunks. |
packages/sdk/src/__tests__/box-agent-run.test.ts |
Updates existing stream tool test to include ids and adds a new test for parallel/out-of-order tool results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | { | ||
| type: "tool-result"; | ||
| /** Identifier of the `tool-call` chunk this result corresponds to. */ | ||
| toolCallId: string; | ||
| /** Name of the tool that produced this result, when known. */ | ||
| toolName?: string; | ||
| /** Tool output payload. Shape is tool-specific. */ | ||
| output: unknown; | ||
| /** True when the tool reported an error. */ | ||
| isError?: boolean; | ||
| } |
There was a problem hiding this comment.
The tool-result chunk docs imply toolCallId always identifies a corresponding tool-call, but the client can emit an empty string when the backend doesn’t provide an id (same as tool-call). Consider documenting that toolCallId may be empty here as well (or making it optional) so consumers don’t assume it’s always present/matchable.
| it("matches parallel tool-call and tool-result chunks by id", async () => { | ||
| const { box, fetchMock } = await createTestBox(); | ||
|
|
||
| fetchMock.mockResolvedValueOnce( | ||
| mockSSEResponse([ | ||
| { event: "run_start", data: { run_id: "r1" } }, | ||
| { event: "tool", data: { id: "toolu_a", name: "Read", input: { path: "/a" } } }, | ||
| { event: "tool", data: { id: "toolu_b", name: "Read", input: { path: "/b" } } }, | ||
| // Results arrive out of order — must still match by id. | ||
| { | ||
| event: "tool_result", | ||
| data: { tool_use_id: "toolu_b", output: "B contents", is_error: false }, | ||
| }, | ||
| { | ||
| event: "tool_result", | ||
| data: { tool_use_id: "toolu_a", output: "A contents", is_error: false }, | ||
| }, | ||
| { event: "done", data: {} }, | ||
| ]), | ||
| ); |
There was a problem hiding this comment.
The new tool_result parsing has multiple fallback branches (tool_use_id vs id, and output vs content), but the added test only covers the tool_use_id + output path. Adding a couple assertions/events to exercise the other branches would help prevent regressions if the backend payload shape varies.
packages/sdk/src/client.ts
Outdated
| const chunk: Chunk = { | ||
| type: "tool-call", | ||
| toolCallId: parsed.id ?? "", | ||
| toolName: parsed.name ?? "", | ||
| input: parsed.input ?? {}, | ||
| }; | ||
| options.onToolUse?.({ name: parsed.name ?? "", input: parsed.input ?? {} }); |
There was a problem hiding this comment.
onToolUse is invoked with only { name, input } even though tool-call chunks now include toolCallId. If callers want to correlate the callback with later tool-result chunks (especially with parallel tool calls), consider extending the callback payload type to include an optional toolCallId and passing parsed.id ?? "" here (and in the non-streaming run path for consistency).
| const chunk: Chunk = { | |
| type: "tool-call", | |
| toolCallId: parsed.id ?? "", | |
| toolName: parsed.name ?? "", | |
| input: parsed.input ?? {}, | |
| }; | |
| options.onToolUse?.({ name: parsed.name ?? "", input: parsed.input ?? {} }); | |
| const toolUsePayload = { | |
| name: parsed.name ?? "", | |
| input: parsed.input ?? {}, | |
| toolCallId: parsed.id ?? "", | |
| }; | |
| const chunk: Chunk = { | |
| type: "tool-call", | |
| toolCallId: toolUsePayload.toolCallId, | |
| toolName: toolUsePayload.name, | |
| input: toolUsePayload.input, | |
| }; | |
| options.onToolUse?.( | |
| toolUsePayload as Parameters<NonNullable<typeof options.onToolUse>>[0], | |
| ); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: parsed.name, | ||
| input: parsed.input, |
There was a problem hiding this comment.
In _executeRun's SSE handler, the onToolUse callback is invoked with name: parsed.name and input: parsed.input without the same ?? "" / ?? {} fallbacks used in the streaming path. If the backend omits either field, consumers can receive undefined even though the onToolUse type promises name: string and input: Record<string, unknown>. Consider defaulting these fields (or validating) before invoking the callback for consistency with _stream.
| name: parsed.name, | |
| input: parsed.input, | |
| name: parsed.name ?? "", | |
| input: parsed.input ?? {}, |
No description provided.