diff --git a/src/adapters/command-code.ts b/src/adapters/command-code.ts index 8f504fde18..21b3c8730d 100644 --- a/src/adapters/command-code.ts +++ b/src/adapters/command-code.ts @@ -48,20 +48,79 @@ function wireImagePart(imageUrl: string): Record { return { type: "image", image: imageUrl, ...(mediaType ? { mediaType } : {}) }; } +/** + * The /alpha/generate wire pairs every assistant `tool-call` with a following `tool` message + * whose `tool-result.toolCallId` matches it. Codex history is not guaranteed to carry that + * pairing (interrupted turns, compacted threads, and multi-step tool rounds all can leave an + * assistant call with no recorded result), and the upstream rejects an unpaired call with + * `Tool result is missing for tool call `, which currently surfaces as a generic 502 + * (#1383). This builder keeps the pairing invariant: + * + * - a `toolResult` that matches a declared assistant call emits the native `tool-result`; + * - a `toolResult` with no matching declared call degrades to a text carrier so the model + * still sees the outcome without a 400-prone standalone `tool` message; + * - every declared assistant call that never received a result gets an explicit error + * `tool-result`, so the upstream never sees an unpaired call. + */ function wireMessages(messages: OcxMessage[]): Array> { const out: Array> = []; + const pendingCalls: Array<{ id: string; name: string }> = []; + // Image parts returned by a tool cannot live inside the text-only `tool-result` wire + // output. They ride a follow-up user message, but that user message must not break the + // adjacency of the assistant turn's tool results, so carriers are buffered and flushed + // only after every declared call has its native or synthesized result. + const pendingImageCarriers: Array> = []; + // The /alpha/generate wire requires every assistant tool-call to be closed by a matching + // `tool-result` immediately after the declaring assistant message. Close any declared call + // that never received a result before a non-toolResult message moves the turn forward, so a + // synthesized `tool` message never lands after a user message or a new assistant turn. + const closePendingCalls = (): void => { + for (const call of pendingCalls) { + out.push({ role: "tool", content: [{ + type: "tool-result", + toolCallId: call.id, + toolName: call.name, + output: { type: "error-text", value: "[ocx] no tool result was recorded for this tool call; execution status unknown." }, + }] }); + } + pendingCalls.length = 0; + if (pendingImageCarriers.length > 0) { + out.push(...pendingImageCarriers); + pendingImageCarriers.length = 0; + } + }; for (const message of messages) { if (message.role === "assistant") { + closePendingCalls(); const content: Array> = []; for (const part of message.content) { if (part.type === "text") content.push({ type: "text", text: part.text }); else if (part.type === "thinking") content.push({ type: "reasoning", text: part.thinking }); - else content.push({ type: "tool-call", toolCallId: part.id, toolName: namespacedToolName(part.namespace, part.name), input: part.arguments }); + else { + const wireName = namespacedToolName(part.namespace, part.name); + content.push({ type: "tool-call", toolCallId: part.id, toolName: wireName, input: part.arguments }); + pendingCalls.push({ id: part.id, name: wireName }); + } } out.push({ role: "assistant", content }); continue; } if (message.role === "toolResult") { + const callIndex = pendingCalls.findIndex(call => call.id === message.toolCallId); + const paired = callIndex >= 0; + if (paired) pendingCalls.splice(callIndex, 1); + if (!paired) { + // Pending calls from an earlier assistant turn must still be closed before any user + // message lands, or their synthesized results would follow the orphan carrier. + closePendingCalls(); + // The upstream rejects a standalone tool message whose call was never declared by an + // assistant turn. Preserve the outcome as text so the model can still act on it. + const label = message.toolName ? `${message.toolName} (${message.toolCallId})` : message.toolCallId; + const text = toolResultText(message.content); + // The orphan result cannot ride a `tool` message; carry it in a user message instead. + out.push({ role: "user", content: [{ type: "text", text: `[tool result without adjacent tool call: ${label}]\n${text}` }] }); + continue; + } out.push({ role: "tool", content: [{ type: "tool-result", toolCallId: message.toolCallId, @@ -73,10 +132,12 @@ function wireMessages(messages: OcxMessage[]): Array> { // message using the same image encoding as the user branch so the bytes reach the model. const images = typeof message.content === "string" ? [] : message.content.filter(part => part.type === "image"); if (images.length > 0) { - out.push({ role: "user", content: images.map(part => wireImagePart((part as { imageUrl: string }).imageUrl)) }); + pendingImageCarriers.push({ role: "user", content: images.map(part => wireImagePart((part as { imageUrl: string }).imageUrl)) }); } continue; } + // User/developer message: no pending tool results may follow it on the wire. + closePendingCalls(); const content: Array> = []; if (typeof message.content === "string") content.push({ type: "text", text: message.content }); else for (const part of message.content) { @@ -85,6 +146,7 @@ function wireMessages(messages: OcxMessage[]): Array> { } out.push({ role: "user", content }); } + closePendingCalls(); return out; } @@ -241,6 +303,18 @@ function eventError(value: unknown): string { return "Command Code stream error"; } +/** + * True when the upstream rejected a tool-result continuation because an assistant tool call + * had no matching result (`Tool result is missing for tool call `). The proxy now keeps + * that pairing invariant before sending, so this error is a distinct provider-side + * validation failure rather than a generic stream fault; classify it as such for the + * dashboard/logs instead of a plain upstream 502. + */ +function isMissingToolResultError(value: unknown): boolean { + const text = eventError(value).toLowerCase(); + return text.includes("tool result is missing") || text.includes("tool_result is missing"); +} + async function*ndjson(response: Response, budget: TranslatorBudget): AsyncGenerator> { if (!response.body) throw new Error("Command Code response body missing"); const reader = response.body.getReader(); @@ -441,7 +515,18 @@ export function createCommandCodeAdapter(provider: OcxProviderConfig): ProviderA yield { type: "done", usage: usage(usageValue), stopReason }; break; } - case "error": yield { type: "error", message: eventError(event.error), status: 502 }; break; + case "error": { + const message = eventError(event.error); + if (isMissingToolResultError(message)) { + // Provider-side tool-result validation: the request carried an assistant tool + // call the upstream refused to accept. This is not a network/stream stall; the + // proxy normally prevents it by pairing every call, so flag it distinctly. + yield { type: "error", message, status: 502, errorType: "upstream_error", code: "missing_tool_result" }; + } else { + yield { type: "error", message, status: 502 }; + } + break; + } } } // A stream that ends without a finish event still needs a terminal done so the diff --git a/tests/command-code-provider.test.ts b/tests/command-code-provider.test.ts index 23d5fee5bf..a2cf33abd5 100644 --- a/tests/command-code-provider.test.ts +++ b/tests/command-code-provider.test.ts @@ -162,22 +162,111 @@ describe("Command Code provider", () => { context: { ...parsed().context, messages: [{ + role: "assistant", + content: [{ type: "toolCall", id: "call_1", name: "view_image", arguments: {} }], + timestamp: 1, + }, { role: "toolResult", toolCallId: "call_1", toolName: "view_image", content: [{ type: "text", text: "screenshot:" }, { type: "image", imageUrl: image }], isError: false, - timestamp: 1, + timestamp: 2, }], }, }); const body = JSON.parse(built.body); expect(body.params.messages).toEqual([ + { role: "assistant", content: [{ type: "tool-call", toolCallId: "call_1", toolName: "view_image", input: {} }] }, { role: "tool", content: [{ type: "tool-result", toolCallId: "call_1", toolName: "view_image", output: { type: "text", value: "screenshot:[image]" } }] }, { role: "user", content: [{ type: "image", image, mediaType: "image/png" }] }, ]); }); + test("synthesizes an error result for every assistant tool call that never received a result", async () => { + const built = await builtRequest({ + ...parsed(), + context: { + ...parsed().context, + messages: [ + { role: "user", content: "run tools", timestamp: 1 }, + { + role: "assistant", + content: [ + { type: "toolCall", id: "call_1", name: "lookup", arguments: { q: "a" } }, + { type: "toolCall", id: "call_2", name: "lookup", arguments: { q: "b" } }, + ], + timestamp: 2, + }, + { role: "toolResult", toolCallId: "call_1", toolName: "lookup", content: "one", isError: false, timestamp: 3 }, + { role: "user", content: "continue", timestamp: 4 }, + ], + }, + }); + const body = JSON.parse(built.body); + const wire = body.params.messages; + expect(wire[0]).toEqual({ role: "user", content: [{ type: "text", text: "run tools" }] }); + expect(wire[1]).toMatchObject({ role: "assistant", content: [{ type: "tool-call", toolCallId: "call_1" }, { type: "tool-call", toolCallId: "call_2" }] }); + expect(wire[2]).toMatchObject({ role: "tool", content: [{ type: "tool-result", toolCallId: "call_1", output: { type: "text", value: "one" } }] }); + // call_2 never received a result: the adapter must close it with an explicit error result + // BEFORE the next user message, or the upstream rejects the unpaired call (#1383). + expect(wire[3]).toMatchObject({ + role: "tool", + content: [{ type: "tool-result", toolCallId: "call_2", toolName: "lookup", output: { type: "error-text" } }], + }); + expect(wire[4]).toEqual({ role: "user", content: [{ type: "text", text: "continue" }] }); + }); + + test("keeps tool results contiguous before buffered image carriers", async () => { + const image = "data:image/png;base64,AAAA"; + const built = await builtRequest({ + ...parsed(), + context: { + ...parsed().context, + messages: [ + { + role: "assistant", + content: [ + { type: "toolCall", id: "call_1", name: "view_image", arguments: {} }, + { type: "toolCall", id: "call_2", name: "lookup", arguments: { q: "b" } }, + ], + timestamp: 1, + }, + { role: "toolResult", toolCallId: "call_1", toolName: "view_image", content: [{ type: "text", text: "shot" }, { type: "image", imageUrl: image }], isError: false, timestamp: 2 }, + { role: "toolResult", toolCallId: "call_2", toolName: "lookup", content: "two", isError: false, timestamp: 3 }, + ], + }, + }); + const body = JSON.parse(built.body); + const wire = body.params.messages; + // Both tool results must precede the user image carrier so the assistant turn's tool + // results stay contiguous on the wire (#1383 / CodeRabbit adjacency finding). + expect(wire[1]).toMatchObject({ role: "tool", content: [{ type: "tool-result", toolCallId: "call_1" }] }); + expect(wire[2]).toMatchObject({ role: "tool", content: [{ type: "tool-result", toolCallId: "call_2" }] }); + expect(wire[3]).toEqual({ role: "user", content: [{ type: "image", image, mediaType: "image/png" }] }); + }); + + test("degrades an orphan tool result without a declared call to a text carrier", async () => { + const built = await builtRequest({ + ...parsed(), + context: { + ...parsed().context, + messages: [ + { role: "user", content: "go", timestamp: 1 }, + { role: "toolResult", toolCallId: "call_orphan", toolName: "lookup", content: "outcome", isError: false, timestamp: 2 }, + ], + }, + }); + const body = JSON.parse(built.body); + const wire = body.params.messages; + // The upstream rejects a standalone `tool` message whose call was never declared by an + // assistant turn; the outcome must ride a user text carrier instead (#1383). + expect(wire[1]).toMatchObject({ + role: "user", + content: [{ type: "text", text: expect.stringContaining("[tool result without adjacent tool call: lookup (call_orphan)]") }], + }); + }); + test("keeps the generate config to bounded workspace and git metadata", async () => { const built = await builtRequest(parsed()); const body = JSON.parse(built.body); @@ -297,6 +386,32 @@ describe("Command Code provider", () => { ]); }); + test("classifies a missing-tool-result upstream error distinctly", async () => { + const response = new Response(JSON.stringify({ + type: "error", + error: { message: "Provider stream error: Tool result is missing for tool call call_01_x." }, + })); + const events = []; + for await (const event of createCommandCodeAdapter(provider).parseStream(response, createTestTranslatorBudget())) events.push(event); + expect(events).toEqual([ + { type: "error", message: "Provider stream error: Tool result is missing for tool call call_01_x.", status: 502, errorType: "upstream_error", code: "missing_tool_result" }, + { type: "done", usage: undefined, stopReason: undefined }, + ]); + }); + + test("classifies the underscored missing-tool-result variant distinctly", async () => { + const response = new Response(JSON.stringify({ + type: "error", + error: { message: "Provider stream error: tool_result is missing for call_02_y." }, + })); + const events = []; + for await (const event of createCommandCodeAdapter(provider).parseStream(response, createTestTranslatorBudget())) events.push(event); + expect(events).toEqual([ + { type: "error", message: "Provider stream error: tool_result is missing for call_02_y.", status: 502, errorType: "upstream_error", code: "missing_tool_result" }, + { type: "done", usage: undefined, stopReason: undefined }, + ]); + }); + test("emits a fallback done when the stream ends without a finish event", async () => { const response = new Response(JSON.stringify({ type: "text-delta", text: "partial" })); const events = [];