From b66ec3db660edaacaf2f91566b63a4400ac3e667 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Sat, 15 Aug 2026 21:05:07 +0700 Subject: [PATCH 1/4] fix(responses): fail closed when a routed provider calls an undeclared tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bridged paths already refuse a tool call the request never declared — `declaredToolNames` in src/bridge.ts turns it into a 502 naming the tool. The native Responses passthrough had no equivalent, so the same call was relayed verbatim. Codex then received a top-level `function_call(name=apply_patch)` for which it has no handler: under code mode `apply_patch` exists only as a nested `tools.apply_patch(...)` helper declared inside the `exec` description, never as a wire tool. The turn surfaced as a bare `aborted` with the target file unchanged, no `custom_tool_call_output`, and no error anywhere to explain it. Ground truth for the guard is the OUTBOUND body rather than the parsed internal tool list. The passthrough forwards wire shapes the internal list flattens or renames — namespaced MCP groups, `additional_tools` items carried inside `input`, the routed custom-tool rewrite — so only the wire names can be compared against what the provider echoes back. Namespaced tools are accepted under either coordinate system, since Codex routes MCP calls by an explicit `namespace` field. Scope: - Only client-executed items are checked (`function_call`, `custom_tool_call`). Hosted calls are run upstream or carry no name. - Forward auth is the canonical ChatGPT backend speaking Codex's own protocol rather than a routed provider, so it stays unguarded — the same line the routed custom-tool and image-gen rewrites already draw. - An unreadable or empty catalog disables the guard rather than failing every turn. - Both the streaming relay and the bounded-JSON answer are covered. Fixes #1700 --- src/server/responses-undeclared-tool-guard.ts | 153 ++++++++ src/server/responses/core.ts | 66 +++- tests/responses-custom-tool-repair.test.ts | 11 + tests/responses-undeclared-tool-guard.test.ts | 334 ++++++++++++++++++ 4 files changed, 546 insertions(+), 18 deletions(-) create mode 100644 src/server/responses-undeclared-tool-guard.ts create mode 100644 tests/responses-undeclared-tool-guard.test.ts diff --git a/src/server/responses-undeclared-tool-guard.ts b/src/server/responses-undeclared-tool-guard.ts new file mode 100644 index 0000000000..3a263f6fb0 --- /dev/null +++ b/src/server/responses-undeclared-tool-guard.ts @@ -0,0 +1,153 @@ +import { namespacedToolName } from "../types"; +import { sseDataPayload, type SseBlockRewrite } from "./sse-payload-rewrite"; + +/** + * Item types the CLIENT executes by name. Hosted calls (`web_search_call`, + * `image_generation_call`, `local_shell_call`, `tool_search_call`, …) are run upstream or + * carry no tool name, so they are never matched against the request catalog. + */ +const CLIENT_EXECUTED_CALL_TYPES = new Set(["function_call", "custom_tool_call"]); + +/** An upstream-supplied name reaches the error message; keep it bounded. */ +const MAX_REPORTED_NAME_CHARS = 100; + +export const UNDECLARED_TOOL_CALL_ERROR_CODE = "undeclared_tool_call"; + +function isPlainObject(value: unknown): value is Record { + return !!value && typeof value === "object" && !Array.isArray(value); +} + +function addWireToolName(names: Set, tool: unknown, namespace?: string): void { + if (!isPlainObject(tool) || typeof tool.name !== "string" || tool.name.length === 0) return; + names.add(tool.name); + // Codex routes MCP calls by an explicit `namespace` field, so the same tool is reachable + // as a bare inner name or as the flattened form; accept both rather than guess which + // coordinate system this provider echoes back. + if (namespace) names.add(namespacedToolName(namespace, tool.name)); +} + +function addWireToolSpecs(names: Set, specs: unknown): void { + if (!Array.isArray(specs)) return; + for (const spec of specs) { + if (!isPlainObject(spec)) continue; + if (spec.type === "namespace" && Array.isArray(spec.tools)) { + const namespace = typeof spec.name === "string" ? spec.name : undefined; + for (const inner of spec.tools) addWireToolName(names, inner, namespace); + continue; + } + addWireToolName(names, spec); + } +} + +/** + * Tool names the OUTBOUND Responses body actually declared. + * + * This reads the body that goes upstream rather than the parsed internal tool list: the + * passthrough forwards wire shapes (namespaced MCP groups, `additional_tools` items carried + * inside `input`, routed custom-tool rewrites) that the internal list flattens or renames, and + * only the wire names can be compared against what the provider echoes back. + */ +export function collectDeclaredWireToolNames(body: unknown): Set { + const names = new Set(); + if (!isPlainObject(body)) return names; + addWireToolSpecs(names, body.tools); + if (Array.isArray(body.input)) { + for (const item of body.input) { + if (isPlainObject(item) && item.type === "additional_tools") addWireToolSpecs(names, item.tools); + } + } + return names; +} + +function undeclaredNameInItem(item: unknown, declared: ReadonlySet): string | undefined { + if (!isPlainObject(item)) return undefined; + if (typeof item.type !== "string" || !CLIENT_EXECUTED_CALL_TYPES.has(item.type)) return undefined; + const name = item.name; + if (typeof name !== "string" || name.length === 0) return undefined; + if (declared.has(name)) return undefined; + if (typeof item.namespace === "string" && declared.has(namespacedToolName(item.namespace, name))) { + return undefined; + } + return name; +} + +/** First undeclared client tool named by a Responses SSE payload, or undefined. */ +export function undeclaredToolCallName( + payload: unknown, + declared: ReadonlySet, +): string | undefined { + if (!isPlainObject(payload)) return undefined; + if (payload.type === "response.output_item.added" || payload.type === "response.output_item.done") { + return undeclaredNameInItem(payload.item, declared); + } + // Sparse gateways skip incremental items and only ever ship the terminal snapshot. + if (payload.type === "response.completed" || payload.type === "response.incomplete") { + return undeclaredToolCallNameInResponse(payload.response, declared); + } + return undefined; +} + +/** First undeclared client tool in a Responses object's `output` array, or undefined. */ +export function undeclaredToolCallNameInResponse( + response: unknown, + declared: ReadonlySet, +): string | undefined { + if (!isPlainObject(response) || !Array.isArray(response.output)) return undefined; + for (const item of response.output) { + const name = undeclaredNameInItem(item, declared); + if (name !== undefined) return name; + } + return undefined; +} + +export function undeclaredToolCallMessage(name: string): string { + const reported = name.slice(0, MAX_REPORTED_NAME_CHARS); + return `routed provider emitted undeclared client tool "${reported}"; only request-declared tools may be called`; +} + +function failedBlocks(name: string, newline: string): readonly string[] { + const failure = { + type: "upstream_error", + code: UNDECLARED_TOOL_CALL_ERROR_CODE, + message: undeclaredToolCallMessage(name), + }; + const payload = JSON.stringify({ + type: "response.failed", + response: { status: "failed", error: failure, last_error: failure }, + }); + return [`event: response.failed${newline}data: ${payload}`, "data: [DONE]"]; +} + +/** + * Fail closed when a routed provider calls a tool the request never declared (#1700). + * + * The bridged paths already refuse such a call (`declaredToolNames` in src/bridge.ts), but the + * native Responses passthrough relayed it verbatim: Codex received a `function_call` for a tool + * it has no top-level handler for — `apply_patch`, which under code mode exists only as a nested + * `tools.apply_patch(...)` helper inside `exec` — and the turn surfaced as a bare `aborted` with + * no output and no explanation. Replacing the offending event with an explicit `response.failed` + * turns that silent dead end into a compatibility error naming the tool. + * + * Everything after the trip is dropped so a later `response.completed` cannot contradict the + * terminal already sent. Non-JSON and non-item blocks pass through untouched. + */ +export function createUndeclaredToolCallGuardBlockRewrite( + declared: ReadonlySet, +): SseBlockRewrite { + let tripped = false; + return (block: string) => { + if (tripped) return []; + const payload = sseDataPayload(block); + if (payload === null || payload === "[DONE]") return [block]; + let parsed: unknown; + try { + parsed = JSON.parse(payload); + } catch { + return [block]; + } + const name = undeclaredToolCallName(parsed, declared); + if (name === undefined) return [block]; + tripped = true; + return failedBlocks(name, block.includes("\r\n") ? "\r\n" : "\n"); + }; +} diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index c6ce07c05a..103cf29fe3 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -237,6 +237,12 @@ import { } from "../sse-payload-rewrite"; import { restoreRoutedCustomCallsInJson } from "../../responses/custom-tool-compat"; import { createRoutedCustomToolRestoreBlockRewrite } from "../responses-custom-tool-repair"; +import { + collectDeclaredWireToolNames, + createUndeclaredToolCallGuardBlockRewrite, + undeclaredToolCallMessage, + undeclaredToolCallNameInResponse, +} from "../responses-undeclared-tool-guard"; import { createGithubCopilotResponsesBlockRewrite } from "../github-copilot-responses-repair"; import { responsesJsonToSseStream } from "../responses-json-events"; import { guardTerminalEventStream } from "./terminal-guard"; @@ -2267,6 +2273,25 @@ async function handleResponsesInner( if (toolBridgeMaps.freeformToolNames.has(name)) routedCustomToolNames.add(name); } } + // #1700: the bridged paths refuse a call to a tool the request never declared + // (`declaredToolNames`, src/bridge.ts). The passthrough had no equivalent, so a routed + // provider's top-level `apply_patch` — which under Codex code mode exists only as a nested + // `tools.apply_patch(...)` helper inside `exec`, never as a wire tool — reached Codex as a + // call it cannot execute, and the turn showed a bare `aborted` with the file untouched. + // Forward auth is the canonical ChatGPT backend speaking Codex's own protocol rather than a + // routed provider, so it keeps passing through unguarded, as it does for the rewrites above. + // An empty catalog disables the guard: passthrough sends the raw body upstream, so a request + // whose tools this proxy could not read is not a catalog worth policing. + const outboundRequestBody = (() => { + try { + return JSON.parse(request.body) as unknown; + } catch { + return undefined; + } + })(); + const declaredWireToolNames = route.provider.authMode === "forward" + ? new Set() + : collectDeclaredWireToolNames(outboundRequestBody); recordAdapterReasoning(logCtx, request); const actualHostKey = upstreamHostHealthKey( route.providerName, @@ -2654,13 +2679,6 @@ async function handleResponsesInner( // injection at the block level, after payload rewrites. Defaults come // from the finalized OUTBOUND body — the normalized internal tool shapes // are not the Responses wire shapes the snapshot must mirror. - const snapshotDefaultsRequest = (() => { - try { - return JSON.parse(request.body) as unknown; - } catch { - return undefined; - } - })(); const blockRewrites = [ payloadRewrites.length > 0 ? payloadRewriteAsBlockRewrite(composeSsePayloadRewrites(...payloadRewrites)) @@ -2672,7 +2690,12 @@ async function handleResponsesInner( ? createGithubCopilotResponsesBlockRewrite(translatorBudget) : undefined, snapshotRepairEnabled - ? createResponsesSnapshotBlockRewrite(snapshotDefaultsRequest, translatorBudget) + ? createResponsesSnapshotBlockRewrite(outboundRequestBody, translatorBudget) + : undefined, + // Last: every rewrite above can still rename or reshape a call item, so the guard must + // compare the names the client will actually receive against the declared catalog. + declaredWireToolNames.size > 0 + ? createUndeclaredToolCallGuardBlockRewrite(declaredWireToolNames) : undefined, ].filter((rewrite): rewrite is NonNullable => rewrite !== undefined); const clientBlockRewrite = blockRewrites.length > 0 @@ -2862,20 +2885,27 @@ async function handleResponsesInner( restoreImageGenCallsInJson(text, imageGenCallAliases), routedCustomToolNames, ); - const repaired = (() => { - if (!hasResponsesSnapshotRepair(route.provider.responsesSnapshotRepair)) return restored; - let outbound: unknown; - try { - outbound = JSON.parse(request.body); - } catch { - outbound = undefined; - } - return repairResponsesSnapshotJson(restored, outbound); - })(); + const repaired = hasResponsesSnapshotRepair(route.provider.responsesSnapshotRepair) + ? repairResponsesSnapshotJson(restored, outboundRequestBody) + : restored; return parsed._responseModelId !== undefined && parsed._responseModelId !== parsed.modelId ? rewriteResponsesModelJson(repaired, parsed._responseModelId) : repaired; })(); + // #1700: same fail-closed policy as the SSE relay above. Both the plain JSON answer and + // the reframed-SSE branch below are built from this body, so one check covers them. + if (declaredWireToolNames.size > 0) { + const undeclared = (() => { + try { + return undeclaredToolCallNameInResponse(JSON.parse(clientJson), declaredWireToolNames); + } catch { + return undefined; + } + })(); + if (undeclared !== undefined) { + return formatErrorResponse(502, "upstream_error", undeclaredToolCallMessage(undeclared)); + } + } // #875: the transport-neutral reliability policy forced a bounded JSON // upstream for a client that asked for SSE. Reframe the completed JSON // as the canonical terminal SSE sequence (created → output_item.done → diff --git a/tests/responses-custom-tool-repair.test.ts b/tests/responses-custom-tool-repair.test.ts index 952610fa55..a5fdafabee 100644 --- a/tests/responses-custom-tool-repair.test.ts +++ b/tests/responses-custom-tool-repair.test.ts @@ -742,6 +742,8 @@ describe("routed Responses custom-tool compatibility", () => { tools: Array>; toolChoice?: unknown; metadata?: unknown; + /** The upstream call names a tool this request never declared at all (#1700). */ + undeclared?: boolean; }> = [ { name: "streaming none", @@ -770,6 +772,7 @@ describe("routed Responses custom-tool compatibility", () => { stream: false, tools: [ordinaryTool], metadata: { nested: { type: "custom", name: "exec" } }, + undeclared: true, }, ]; @@ -823,6 +826,14 @@ describe("routed Responses custom-tool compatibility", () => { expect(clientSse).toContain("response.function_call_arguments.done"); expect(clientSse).not.toContain("custom_tool_call"); expect(clientSse).not.toContain("ctc_exec"); + } else if (policyCase.undeclared) { + // #1700: this request's catalog holds only `ordinary` — a metadata blob that merely + // looks like a tool declaration declares nothing — so a call to `exec` is refused + // instead of relayed. The restore contract still holds either way: it never became + // a custom_tool_call. + expect(response.status).toBe(502); + const body = await response.json() as { error: { message: string } }; + expect(body.error.message).toContain('undeclared client tool "exec"'); } else { const body = await response.json() as { output: Array> }; expect(body.output[0]).toEqual(upstreamItem); diff --git a/tests/responses-undeclared-tool-guard.test.ts b/tests/responses-undeclared-tool-guard.test.ts new file mode 100644 index 0000000000..b0af1f242a --- /dev/null +++ b/tests/responses-undeclared-tool-guard.test.ts @@ -0,0 +1,334 @@ +/** + * #1700: the native Responses passthrough relayed a routed provider's call to a tool the request + * never declared. Codex has no top-level handler for it, so the turn surfaced as a bare `aborted` + * with the target file untouched. The bridged paths already fail closed on the same condition + * (`declaredToolNames`, src/bridge.ts); these pin the passthrough's equivalent. + */ +import { describe, expect, test } from "bun:test"; +import { + collectDeclaredWireToolNames, + createUndeclaredToolCallGuardBlockRewrite, + undeclaredToolCallNameInResponse, + UNDECLARED_TOOL_CALL_ERROR_CODE, +} from "../src/server/responses-undeclared-tool-guard"; +import { relaySseWithBlockRewrite } from "../src/server/sse-payload-rewrite"; +import { handleResponses } from "../src/server/responses"; +import type { OcxConfig } from "../src/types"; +import { createTestTranslatorBudget } from "./helpers/translator-budget"; + +/** One SSE event block without its blank-line delimiter. */ +function frame(type: string, payload: Record): string { + return `event: ${type}\ndata: ${JSON.stringify({ type, ...payload })}`; +} + +/** One SSE event block including its delimiter, ready to concatenate. */ +function sse(type: string, payload: Record): string { + return `${frame(type, payload)}\n\n`; +} + +function streamFromText(text: string): ReadableStream { + const chunk = new TextEncoder().encode(text); + let sent = false; + return new ReadableStream({ + pull(controller) { + if (sent) { + controller.close(); + return; + } + sent = true; + controller.enqueue(chunk); + }, + }); +} + +async function readAll(stream: ReadableStream): Promise { + const reader = stream.getReader(); + const decoder = new TextDecoder(); + let text = ""; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + text += decoder.decode(value, { stream: true }); + } + return text; +} + +async function relay(upstream: string, declared: Iterable): Promise { + const budget = createTestTranslatorBudget(); + try { + return await readAll(relaySseWithBlockRewrite( + streamFromText(upstream), + createUndeclaredToolCallGuardBlockRewrite(new Set(declared)), + budget, + )); + } finally { + budget.dispose(); + } +} + +describe("collectDeclaredWireToolNames", () => { + test("reads function, custom, and namespaced tools off the outbound body", () => { + const names = collectDeclaredWireToolNames({ + tools: [ + { type: "function", name: "exec" }, + { type: "custom", name: "apply_patch" }, + { type: "namespace", name: "linear", tools: [{ type: "function", name: "create_issue" }] }, + { type: "web_search" }, + ], + }); + + // Namespaced MCP tools are reachable under either coordinate system, so both are accepted. + expect([...names].sort()).toEqual( + ["apply_patch", "create_issue", "exec", "linear__create_issue"], + ); + }); + + test("reads tools carried inside input as an additional_tools item", () => { + // Codex Desktop's responses_lite WS path ships the catalog there instead of body.tools. + const names = collectDeclaredWireToolNames({ + input: [ + { type: "message", role: "user", content: [] }, + { type: "additional_tools", role: "developer", tools: [{ type: "function", name: "wait" }] }, + ], + }); + + expect([...names]).toEqual(["wait"]); + }); + + test("is empty for a body this proxy could not read", () => { + expect(collectDeclaredWireToolNames(undefined).size).toBe(0); + expect(collectDeclaredWireToolNames({ tools: "nonsense" }).size).toBe(0); + }); +}); + +describe("undeclared tool call guard", () => { + const declared = ["exec", "wait", "request_user_input"]; + + test("relays a declared call untouched", async () => { + const upstream = sse("response.output_item.added", { + output_index: 0, + item: { type: "function_call", id: "fc_1", call_id: "call_1", name: "exec", arguments: "{}" }, + }) + sse("response.completed", { response: { id: "resp_1", status: "completed", output: [] } }); + + expect(await relay(upstream, declared)).toBe(upstream); + }); + + test("replaces an undeclared apply_patch with a compatibility failure", async () => { + // The reported shape: the request-visible catalog holds exec/wait/request_user_input, and + // `apply_patch` arrives anyway because code mode nests it inside the exec description. + const upstream = sse("response.output_item.added", { + output_index: 0, + item: { type: "function_call", id: "fc_1", call_id: "call_1", name: "apply_patch", arguments: "{}" }, + }); + + const out = await relay(upstream, declared); + expect(out).toContain("event: response.failed"); + expect(out).toContain(`"code":"${UNDECLARED_TOOL_CALL_ERROR_CODE}"`); + expect(out).toContain('routed provider emitted undeclared client tool \\"apply_patch\\"'); + expect(out).toEndWith("data: [DONE]\n\n"); + }); + + test("drops the rest of the turn so a later completed cannot contradict the failure", async () => { + const upstream = sse("response.output_item.added", { + output_index: 0, + item: { type: "function_call", id: "fc_1", call_id: "call_1", name: "apply_patch", arguments: "" }, + }) + + sse("response.function_call_arguments.delta", { item_id: "fc_1", delta: "{\"input\":\"" }) + + sse("response.completed", { response: { id: "resp_1", status: "completed", output: [] } }) + + "data: [DONE]\n\n"; + + const out = await relay(upstream, declared); + expect(out).not.toContain("response.completed"); + expect(out).not.toContain("function_call_arguments"); + expect(out.match(/\[DONE\]/g)).toHaveLength(1); + }); + + test("catches the snapshot-only shape, where no item event is ever streamed", async () => { + const upstream = sse("response.completed", { + response: { + id: "resp_1", + status: "completed", + output: [ + { type: "message", id: "msg_0", role: "assistant" }, + { type: "function_call", id: "fc_1", call_id: "call_1", name: "apply_patch", arguments: "{}" }, + ], + }, + }); + + const out = await relay(upstream, declared); + expect(out).toContain(`"code":"${UNDECLARED_TOOL_CALL_ERROR_CODE}"`); + expect(out).not.toContain('"status":"completed"'); + }); + + test("accepts a namespaced call echoed under its bare name", async () => { + const upstream = sse("response.output_item.added", { + output_index: 0, + item: { + type: "function_call", + id: "fc_1", + call_id: "call_1", + name: "create_issue", + namespace: "linear", + arguments: "{}", + }, + }); + + expect(await relay(upstream, ["linear__create_issue"])).toBe(upstream); + }); + + test("never blocks apply_patch when the request really declared it", async () => { + // `apply_patch` is exempt from the routed custom-tool rewrite, so it reaches upstream as + // `{type:"custom"}` and comes back as a `custom_tool_call`. A request that declares it must + // keep working — the guard exists for the case where the catalog never mentioned it. + const outbound = { tools: [{ type: "custom", name: "apply_patch" }, { type: "function", name: "exec" }] }; + const upstream = sse("response.output_item.added", { + output_index: 0, + item: { type: "custom_tool_call", id: "ctc_1", call_id: "call_1", name: "apply_patch", input: "" }, + }); + + expect(await relay(upstream, collectDeclaredWireToolNames(outbound))).toBe(upstream); + }); + + test("ignores upstream-executed calls, which are never matched against the catalog", async () => { + const upstream = sse("response.output_item.added", { + output_index: 0, + item: { type: "web_search_call", id: "ws_1", status: "completed" }, + }) + sse("response.output_item.added", { + output_index: 1, + item: { type: "tool_search_call", id: "tsc_1", status: "completed" }, + }); + + expect(await relay(upstream, declared)).toBe(upstream); + }); + + test("leaves comment frames, [DONE], and unparseable payloads alone", async () => { + const upstream = ": keep-alive\n\ndata: {not json\n\ndata: [DONE]\n\n"; + + expect(await relay(upstream, declared)).toBe(upstream); + }); + + test("bounds a hostile tool name before it reaches the error message", async () => { + const upstream = sse("response.output_item.added", { + output_index: 0, + item: { type: "function_call", id: "fc_1", call_id: "call_1", name: "x".repeat(5_000), arguments: "{}" }, + }); + + const out = await relay(upstream, declared); + expect(out).toContain(`\\"${"x".repeat(100)}\\"`); + expect(out).not.toContain("x".repeat(101)); + }); +}); + +describe("the reported turn, end to end through handleResponses", () => { + // The report's setup: provider `opencode-go`, a model pinned to the openai-responses adapter, + // and a Codex catalog of exec/wait/request_user_input with no top-level apply_patch schema. + const config = { + port: 0, + defaultProvider: "fixture", + providers: { + fixture: { + adapter: "openai-responses", + baseUrl: "https://fixture.test/v1", + authMode: "key", + apiKey: "fixture-key", + }, + }, + } as OcxConfig; + + const requestBody = (stream: boolean) => JSON.stringify({ + model: "fixture/deepseek-v4-flash", + stream, + input: [{ role: "user", content: [{ type: "input_text", text: "change v1 to v2" }] }], + tools: [ + { type: "custom", name: "exec", description: "Run JavaScript", format: { type: "grammar", syntax: "lark" } }, + { type: "function", name: "wait", parameters: { type: "object" } }, + { type: "function", name: "request_user_input", parameters: { type: "object" } }, + ], + }); + + const leakedCall = { + type: "function_call", + id: "fc_patch", + call_id: "call_patch", + name: "apply_patch", + arguments: "{\"input\":\"*** Begin Patch\"}", + status: "completed", + }; + + async function post(stream: boolean, upstream: () => Response): Promise { + const savedFetch = globalThis.fetch; + globalThis.fetch = (async () => upstream()) as typeof fetch; + try { + return await handleResponses(new Request("http://localhost/v1/responses", { + method: "POST", + headers: { "content-type": "application/json" }, + body: requestBody(stream), + }), config, { model: "", provider: "" }); + } finally { + globalThis.fetch = savedFetch; + } + } + + test("streaming: the leaked apply_patch becomes a named failure instead of a silent abort", async () => { + const response = await post(true, () => new Response([ + frame("response.output_item.added", { output_index: 0, item: { ...leakedCall, arguments: "", status: "in_progress" } }), + frame("response.output_item.done", { output_index: 0, item: leakedCall }), + frame("response.completed", { response: { id: "resp_1", status: "completed", output: [leakedCall] } }), + "data: [DONE]", + ].join("\n\n") + "\n\n", { headers: { "content-type": "text/event-stream" } })); + + const body = await response.text(); + expect(body).toContain("response.failed"); + expect(body).toContain(`"code":"${UNDECLARED_TOOL_CALL_ERROR_CODE}"`); + expect(body).toContain("apply_patch"); + // Before the guard this reached Codex as a call it has no handler for, and the turn showed + // only `aborted`. The client must not see a completed turn now. + expect(body).not.toContain("response.completed"); + }); + + test("non-streaming: the same call is refused rather than answered", async () => { + const response = await post(false, () => new Response( + JSON.stringify({ id: "resp_1", status: "completed", output: [leakedCall] }), + { headers: { "content-type": "application/json" } }, + )); + + expect(response.status).toBe(502); + const body = await response.json() as { error: { message: string } }; + expect(body.error.message).toContain('undeclared client tool "apply_patch"'); + }); + + test("a declared exec call still completes normally", async () => { + const execCall = { + type: "function_call", + id: "fc_exec", + call_id: "call_exec", + name: "exec", + arguments: "{\"input\":\"await tools.apply_patch('*** Begin Patch')\"}", + status: "completed", + }; + const response = await post(false, () => new Response( + JSON.stringify({ id: "resp_1", status: "completed", output: [execCall] }), + { headers: { "content-type": "application/json" } }, + )); + + expect(response.status).toBe(200); + const body = await response.json() as { output: Array> }; + // `exec` is declared as a custom tool, so it comes back restored to a custom_tool_call — + // the supported editing path in the report stays intact. + expect(body.output[0]).toMatchObject({ name: "exec", call_id: "call_exec" }); + }); +}); + +describe("undeclaredToolCallNameInResponse", () => { + test("names the first undeclared call in a non-streaming body", () => { + const response = { + output: [ + { type: "function_call", name: "exec" }, + { type: "custom_tool_call", name: "apply_patch", input: "*** Begin Patch" }, + ], + }; + + expect(undeclaredToolCallNameInResponse(response, new Set(["exec"]))).toBe("apply_patch"); + expect(undeclaredToolCallNameInResponse(response, new Set(["exec", "apply_patch"]))).toBeUndefined(); + }); +}); From c994d47e6cf47c5ff2698f75c95b21b96cdb0abd Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Sat, 15 Aug 2026 22:42:54 +0700 Subject: [PATCH 2/4] fix(responses): trust catalog readability, not size, and keep refused turns out of replay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on the undeclared-tool guard. An empty catalog is not an absent one. Disabling the guard whenever the declared set was empty meant a request that declares no tools authorized every client-executed call the provider cared to invent. Readability is the real question, so the parse result and the name set are now separate state: an unparseable body still stands down, a readable one enforces even at zero tools. The outbound body alone turned out to be an incomplete record of the caller's catalog. Hosted-tool preference REPLACES a client tool with its hosted form, so a request declaring `image_gen.generate` ships `{type:"image_generation"}` upstream and gets the client's own name back — which the guard would have refused. The declared set is now the union of the outbound wire names and the caller's own catalog. Widening only ever makes the guard fire less; a name declared in neither place is still refused. A refused turn must also not become continuation state. The passthrough records completed responses so a later `previous_response_id` can expand from them, and that write sat before the guard on the JSON path and on the untouched upstream stream the inspection branch reads. Both now go through a wrapper that drops a response carrying an undeclared call, and the JSON path records only after the guard passes. The wrapper inspects the payload rather than sharing a flag with the client relay, so tee ordering cannot race it. Terminal-outcome recording still reports what upstream did: the request was served and the tokens were spent, so quota and host health should account for it even though the client received a failure. --- src/server/responses/core.ts | 62 ++++-- tests/responses-undeclared-tool-guard.test.ts | 191 +++++++++++++++++- 2 files changed, 234 insertions(+), 19 deletions(-) diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 103cf29fe3..83cf599c19 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -2280,18 +2280,42 @@ async function handleResponsesInner( // call it cannot execute, and the turn showed a bare `aborted` with the file untouched. // Forward auth is the canonical ChatGPT backend speaking Codex's own protocol rather than a // routed provider, so it keeps passing through unguarded, as it does for the rewrites above. - // An empty catalog disables the guard: passthrough sends the raw body upstream, so a request - // whose tools this proxy could not read is not a catalog worth policing. + // What decides whether the catalog can be trusted is whether the body was READABLE, not how + // many tools it holds: a request that declares none authorizes none, so every client-executed + // call it gets back is undeclared. Only a body this proxy could not parse leaves the guard + // with nothing to compare against. const outboundRequestBody = (() => { try { - return JSON.parse(request.body) as unknown; + const body = JSON.parse(request.body) as unknown; + return body && typeof body === "object" && !Array.isArray(body) ? body : undefined; } catch { return undefined; } })(); - const declaredWireToolNames = route.provider.authMode === "forward" - ? new Set() - : collectDeclaredWireToolNames(outboundRequestBody); + const declaredWireToolNames = collectDeclaredWireToolNames(outboundRequestBody); + // Union with the caller's own catalog, because the outbound body is not always a complete + // record of it: hosted-tool preference REPLACES a client tool with its hosted form, so a + // request declaring `image_gen.generate` ships `{type:"image_generation"}` upstream and gets + // the client's name back. Widening only ever makes the guard fire less; a name declared in + // neither place — #1700's `apply_patch` — is still refused. + for (const name of toolBridgeMaps.declaredToolNames) declaredWireToolNames.add(name); + const undeclaredToolGuardActive = outboundRequestBody !== undefined + && route.provider.authMode !== "forward"; + // A refused turn must not seed `previous_response_id` replay. The inspection branch reads the + // untouched upstream stream, so it can still observe a `response.completed` the client never + // received; checking the payload itself rather than a flag shared with the client relay keeps + // this free of tee ordering races. + const rememberPassthroughResponseChecked = rememberPassthroughResponse + ? (response: { id?: unknown; output?: unknown; status?: unknown }) => { + if ( + undeclaredToolGuardActive + && undeclaredToolCallNameInResponse(response, declaredWireToolNames) !== undefined + ) { + return; + } + rememberPassthroughResponse(response); + } + : undefined; recordAdapterReasoning(logCtx, request); const actualHostKey = upstreamHostHealthKey( route.providerName, @@ -2694,7 +2718,7 @@ async function handleResponsesInner( : undefined, // Last: every rewrite above can still rename or reshape a call item, so the guard must // compare the names the client will actually receive against the declared catalog. - declaredWireToolNames.size > 0 + undeclaredToolGuardActive ? createUndeclaredToolCallGuardBlockRewrite(declaredWireToolNames) : undefined, ].filter((rewrite): rewrite is NonNullable => rewrite !== undefined); @@ -2747,7 +2771,7 @@ async function handleResponsesInner( const inspector = createSseInspector({ onTerminal: reportNativeTerminal, logCtx, - onCompletedResponse: rememberPassthroughResponse, + onCompletedResponse: rememberPassthroughResponseChecked, onFirstOutput: options.onFirstOutput, pinCompletedResponseIdToFirstSeen: githubCopilotRepairEnabled, }); @@ -2831,7 +2855,7 @@ async function handleResponsesInner( () => unregisterTurn(turnAc), logCtx, () => options.onNativePassthroughCancel?.(), - rememberPassthroughResponse, + rememberPassthroughResponseChecked, options.onFirstOutput, inspectionConsumerOptions, ); @@ -2841,7 +2865,7 @@ async function handleResponsesInner( logCtx, turnAc.signal, () => unregisterTurn(turnAc), - rememberPassthroughResponse, + rememberPassthroughResponseChecked, options.onFirstOutput, inspectionConsumerOptions, ); @@ -2875,11 +2899,6 @@ async function handleResponsesInner( } const text = bounded.text; inspectResponseLogJson(logCtx, text); - if (rememberPassthroughResponse) { - try { - rememberPassthroughResponse(JSON.parse(text) as { id?: unknown; output?: unknown; status?: unknown }); - } catch { /* non-JSON despite content-type; recording is best-effort */ } - } const clientJson = (() => { const restored = restoreRoutedCustomCallsInJson( restoreImageGenCallsInJson(text, imageGenCallAliases), @@ -2893,8 +2912,10 @@ async function handleResponsesInner( : repaired; })(); // #1700: same fail-closed policy as the SSE relay above. Both the plain JSON answer and - // the reframed-SSE branch below are built from this body, so one check covers them. - if (declaredWireToolNames.size > 0) { + // the reframed-SSE branch below are built from this body, so one check covers them. This + // runs BEFORE the continuation cache write below: a refused turn must not become state a + // later `previous_response_id` replay can expand from. + if (undeclaredToolGuardActive) { const undeclared = (() => { try { return undeclaredToolCallNameInResponse(JSON.parse(clientJson), declaredWireToolNames); @@ -2906,6 +2927,13 @@ async function handleResponsesInner( return formatErrorResponse(502, "upstream_error", undeclaredToolCallMessage(undeclared)); } } + if (rememberPassthroughResponseChecked) { + try { + rememberPassthroughResponseChecked( + JSON.parse(text) as { id?: unknown; output?: unknown; status?: unknown }, + ); + } catch { /* non-JSON despite content-type; recording is best-effort */ } + } // #875: the transport-neutral reliability policy forced a bounded JSON // upstream for a client that asked for SSE. Reframe the completed JSON // as the canonical terminal SSE sequence (created → output_item.done → diff --git a/tests/responses-undeclared-tool-guard.test.ts b/tests/responses-undeclared-tool-guard.test.ts index b0af1f242a..637458fdca 100644 --- a/tests/responses-undeclared-tool-guard.test.ts +++ b/tests/responses-undeclared-tool-guard.test.ts @@ -13,6 +13,7 @@ import { } from "../src/server/responses-undeclared-tool-guard"; import { relaySseWithBlockRewrite } from "../src/server/sse-payload-rewrite"; import { handleResponses } from "../src/server/responses"; +import { expandPreviousResponseInput } from "../src/responses/state"; import type { OcxConfig } from "../src/types"; import { createTestTranslatorBudget } from "./helpers/translator-budget"; @@ -99,6 +100,37 @@ describe("collectDeclaredWireToolNames", () => { expect(collectDeclaredWireToolNames(undefined).size).toBe(0); expect(collectDeclaredWireToolNames({ tools: "nonsense" }).size).toBe(0); }); + + test("is empty for a readable request that declares nothing", () => { + // Indistinguishable from the unreadable case by size alone, which is why the caller keeps + // readability as separate state: a request declaring no tools authorizes none. + expect(collectDeclaredWireToolNames({}).size).toBe(0); + expect(collectDeclaredWireToolNames({ tools: [] }).size).toBe(0); + }); + + test("ignores hosted tool entries, which carry no client-executable name", () => { + const names = collectDeclaredWireToolNames({ + tools: [{ type: "web_search" }, { type: "image_generation" }, { type: "function", name: "exec" }], + }); + + expect([...names]).toEqual(["exec"]); + }); +}); + +describe("an empty catalog still authorizes nothing", () => { + test("every client-executed call is undeclared when no tool was declared", () => { + const empty = new Set(); + const response = { output: [{ type: "function_call", name: "apply_patch" }] }; + + expect(undeclaredToolCallNameInResponse(response, empty)).toBe("apply_patch"); + }); + + test("hosted calls are still exempt under an empty catalog", () => { + const empty = new Set(); + const response = { output: [{ type: "web_search_call", id: "ws_1" }] }; + + expect(undeclaredToolCallNameInResponse(response, empty)).toBeUndefined(); + }); }); describe("undeclared tool call guard", () => { @@ -255,20 +287,25 @@ describe("the reported turn, end to end through handleResponses", () => { status: "completed", }; - async function post(stream: boolean, upstream: () => Response): Promise { + async function post( + stream: boolean, + upstream: () => Response, + body: string = requestBody(stream), + ): Promise { const savedFetch = globalThis.fetch; globalThis.fetch = (async () => upstream()) as typeof fetch; try { return await handleResponses(new Request("http://localhost/v1/responses", { method: "POST", headers: { "content-type": "application/json" }, - body: requestBody(stream), + body, }), config, { model: "", provider: "" }); } finally { globalThis.fetch = savedFetch; } } + test("streaming: the leaked apply_patch becomes a named failure instead of a silent abort", async () => { const response = await post(true, () => new Response([ frame("response.output_item.added", { output_index: 0, item: { ...leakedCall, arguments: "", status: "in_progress" } }), @@ -319,6 +356,156 @@ describe("the reported turn, end to end through handleResponses", () => { }); }); +describe("a refused turn does not become continuation state", () => { + // The guard rejects the turn for the client, so it must not also be cached as a completed + // response: a later `previous_response_id` replay would otherwise expand from a turn the + // client never accepted, reintroducing the undeclared call as history. + const config = { + port: 0, + defaultProvider: "fixture", + providers: { + fixture: { + adapter: "openai-responses", + baseUrl: "https://fixture.test/v1", + authMode: "key", + apiKey: "fixture-key", + }, + }, + } as OcxConfig; + + const declaredTools = [ + { type: "custom", name: "exec", description: "Run JavaScript", format: { type: "grammar", syntax: "lark" } }, + ]; + + async function turn(responseId: string, outputItem: Record): Promise { + const savedFetch = globalThis.fetch; + globalThis.fetch = (async () => new Response( + JSON.stringify({ id: responseId, status: "completed", output: [outputItem] }), + { headers: { "content-type": "application/json" } }, + )) as typeof fetch; + try { + return await handleResponses(new Request("http://localhost/v1/responses", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "fixture/deepseek-v4-flash", + stream: false, + input: [{ role: "user", content: [{ type: "input_text", text: "edit the file" }] }], + tools: declaredTools, + }), + }), config, { model: "", provider: "" }); + } finally { + globalThis.fetch = savedFetch; + } + } + + /** How many items a follow-up turn inherits by naming `previousId`. */ + function expandedInputLength(previousId: string): number { + const followUp = { + model: "fixture/deepseek-v4-flash", + previous_response_id: previousId, + input: [{ role: "user", content: [{ type: "input_text", text: "and again" }] }], + tools: declaredTools, + }; + const expanded = expandPreviousResponseInput(followUp) as { input?: unknown[] }; + return Array.isArray(expanded.input) ? expanded.input.length : 0; + } + + test("a completed turn is remembered, so the control is meaningful", async () => { + const accepted = await turn("resp_accepted", { + type: "function_call", id: "fc_ok", call_id: "call_ok", name: "exec", arguments: "{}", status: "completed", + }); + + expect(accepted.status).toBe(200); + expect(expandedInputLength("resp_accepted")).toBeGreaterThan(1); + }); + + test("a refused turn is not", async () => { + const refused = await turn("resp_refused", { + type: "function_call", id: "fc_bad", call_id: "call_bad", name: "apply_patch", arguments: "{}", status: "completed", + }); + + expect(refused.status).toBe(502); + // Nothing to inherit: the follow-up keeps only its own single input item. + expect(expandedInputLength("resp_refused")).toBe(1); + }); +}); + +describe("a readable request that declares no tools authorizes none", () => { + const config = { + port: 0, + defaultProvider: "fixture", + providers: { + fixture: { + adapter: "openai-responses", + baseUrl: "https://fixture.test/v1", + authMode: "key", + apiKey: "fixture-key", + }, + }, + } as OcxConfig; + + const call = { + type: "function_call", + id: "fc_1", + call_id: "call_1", + name: "apply_patch", + arguments: "{}", + status: "completed", + }; + + async function post(stream: boolean, tools: unknown[] | undefined, upstream: () => Response) { + const savedFetch = globalThis.fetch; + globalThis.fetch = (async () => upstream()) as typeof fetch; + try { + return await handleResponses(new Request("http://localhost/v1/responses", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "fixture/deepseek-v4-flash", + stream, + input: [{ role: "user", content: [{ type: "input_text", text: "hi" }] }], + ...(tools === undefined ? {} : { tools }), + }), + }), config, { model: "", provider: "" }); + } finally { + globalThis.fetch = savedFetch; + } + } + + const jsonUpstream = () => new Response( + JSON.stringify({ id: "resp_1", status: "completed", output: [call] }), + { headers: { "content-type": "application/json" } }, + ); + + const sseUpstream = () => new Response( + [ + frame("response.output_item.added", { output_index: 0, item: call }), + frame("response.completed", { response: { id: "resp_1", status: "completed", output: [call] } }), + "data: [DONE]", + ].join("\n\n") + "\n\n", + { headers: { "content-type": "text/event-stream" } }, + ); + + for (const [label, tools] of [["no tools field", undefined], ["tools: []", []]] as const) { + test(`non-streaming, ${label}`, async () => { + const response = await post(false, tools, jsonUpstream); + + expect(response.status).toBe(502); + const body = await response.json() as { error: { message: string } }; + expect(body.error.message).toContain('undeclared client tool "apply_patch"'); + }); + + test(`streaming, ${label}`, async () => { + const response = await post(true, tools, sseUpstream); + const body = await response.text(); + + expect(body).toContain(`"code":"${UNDECLARED_TOOL_CALL_ERROR_CODE}"`); + expect(body).not.toContain("response.completed"); + }); + } +}); + describe("undeclaredToolCallNameInResponse", () => { test("names the first undeclared call in a non-streaming body", () => { const response = { From cfbb3cdec5bc85154bfe8a672d9e34f9acceb2f6 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Sun, 16 Aug 2026 10:34:08 +0700 Subject: [PATCH 3/4] fix(responses): stand the guard down when the request carries no catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught this: `c994d47e6` made the guard enforce on a readable request with an empty tool catalog, and that broke six passthrough tests on macOS and three of four test shards. The reasoning behind that change sounded right in the abstract — a request that declares no tools authorizes none — but it is wrong for the passthrough, and the repository already had a test saying so. `github-copilot-stream-contract` sends `{model, input, stream}` with no `tools` field at all, and Copilot answers with a `custom_tool_call` for `apply_patch`. The client understands that call; the proxy simply has no catalog to check it against. Enforcing there replaced the turn with `response.failed` and the client never saw `response.completed`. The same shape broke the DeepSeek terminal-repair and item-id-repair contracts. So the activation condition goes back to "at least one declared name". An unreadable body lands in the same place, since it also yields no names, which is what the readable/empty distinction was reaching for. Kept from that commit: the union with the caller's own catalog (hosted-tool preference rewrites the outbound body, so it is not a complete record), and keeping a refused turn out of `previous_response_id` replay state. The empty-catalog cases in the guard's own test file asserted the behaviour this reverts; they now pin the opposite, on both transports, with the Copilot contract named as the reason. Verified: github-copilot-stream-contract, deepseek-inbound-wire, deepseek-responses-item-id-repair, responses-undeclared-tool-guard and responses-custom-tool-repair all pass; `bun run typecheck` clean. --- src/server/responses/core.ts | 13 ++++--- tests/responses-undeclared-tool-guard.test.ts | 35 +++++++------------ 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 83cf599c19..213d6d8821 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -2280,10 +2280,13 @@ async function handleResponsesInner( // call it cannot execute, and the turn showed a bare `aborted` with the file untouched. // Forward auth is the canonical ChatGPT backend speaking Codex's own protocol rather than a // routed provider, so it keeps passing through unguarded, as it does for the rewrites above. - // What decides whether the catalog can be trusted is whether the body was READABLE, not how - // many tools it holds: a request that declares none authorizes none, so every client-executed - // call it gets back is undeclared. Only a body this proxy could not parse leaves the guard - // with nothing to compare against. + // The guard needs a catalog to compare against, so it stands down when the request carries + // none. That is not the same claim as "no tools means no tool may be called": a passthrough + // request can legitimately omit `tools` entirely and still receive a tool call the client + // understands — `tests/github-copilot-stream-contract.test.ts` sends `{model, input, stream}` + // with no tools and Copilot answers with a `custom_tool_call` for `apply_patch`. Policing an + // empty catalog truncates that turn. An unreadable body lands here too, since it yields no + // names either. const outboundRequestBody = (() => { try { const body = JSON.parse(request.body) as unknown; @@ -2299,7 +2302,7 @@ async function handleResponsesInner( // the client's name back. Widening only ever makes the guard fire less; a name declared in // neither place — #1700's `apply_patch` — is still refused. for (const name of toolBridgeMaps.declaredToolNames) declaredWireToolNames.add(name); - const undeclaredToolGuardActive = outboundRequestBody !== undefined + const undeclaredToolGuardActive = declaredWireToolNames.size > 0 && route.provider.authMode !== "forward"; // A refused turn must not seed `previous_response_id` replay. The inspection branch reads the // untouched upstream stream, so it can still observe a `response.completed` the client never diff --git a/tests/responses-undeclared-tool-guard.test.ts b/tests/responses-undeclared-tool-guard.test.ts index 637458fdca..6bb2d019dd 100644 --- a/tests/responses-undeclared-tool-guard.test.ts +++ b/tests/responses-undeclared-tool-guard.test.ts @@ -117,21 +117,6 @@ describe("collectDeclaredWireToolNames", () => { }); }); -describe("an empty catalog still authorizes nothing", () => { - test("every client-executed call is undeclared when no tool was declared", () => { - const empty = new Set(); - const response = { output: [{ type: "function_call", name: "apply_patch" }] }; - - expect(undeclaredToolCallNameInResponse(response, empty)).toBe("apply_patch"); - }); - - test("hosted calls are still exempt under an empty catalog", () => { - const empty = new Set(); - const response = { output: [{ type: "web_search_call", id: "ws_1" }] }; - - expect(undeclaredToolCallNameInResponse(response, empty)).toBeUndefined(); - }); -}); describe("undeclared tool call guard", () => { const declared = ["exec", "wait", "request_user_input"]; @@ -431,7 +416,7 @@ describe("a refused turn does not become continuation state", () => { }); }); -describe("a readable request that declares no tools authorizes none", () => { +describe("a request that declares no tools has no catalog to police", () => { const config = { port: 0, defaultProvider: "fixture", @@ -487,21 +472,25 @@ describe("a readable request that declares no tools authorizes none", () => { { headers: { "content-type": "text/event-stream" } }, ); + // A passthrough request may omit `tools` entirely and still receive a tool call the client + // understands — the Copilot contract in tests/github-copilot-stream-contract.test.ts does + // exactly that with `apply_patch`. Refusing on an empty catalog truncates those turns, so the + // guard needs at least one declared name before it has an opinion. for (const [label, tools] of [["no tools field", undefined], ["tools: []", []]] as const) { - test(`non-streaming, ${label}`, async () => { + test(`non-streaming, ${label} — relayed, not refused`, async () => { const response = await post(false, tools, jsonUpstream); - expect(response.status).toBe(502); - const body = await response.json() as { error: { message: string } }; - expect(body.error.message).toContain('undeclared client tool "apply_patch"'); + expect(response.status).toBe(200); + const body = await response.json() as { output: Array> }; + expect(body.output[0]).toMatchObject({ name: "apply_patch" }); }); - test(`streaming, ${label}`, async () => { + test(`streaming, ${label} — relayed, not refused`, async () => { const response = await post(true, tools, sseUpstream); const body = await response.text(); - expect(body).toContain(`"code":"${UNDECLARED_TOOL_CALL_ERROR_CODE}"`); - expect(body).not.toContain("response.completed"); + expect(body).not.toContain(UNDECLARED_TOOL_CALL_ERROR_CODE); + expect(body).toContain("response.completed"); }); } }); From dea7d2c2733e1e0eb05fffe5009f7ed0105b658d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 21:50:19 +0900 Subject: [PATCH 4/4] fix(responses): keep a mid-stream refusal out of continuation state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checking only the terminal snapshot left a hole. An upstream can announce the undeclared call in `response.output_item.added` — which trips the client guard and sends `response.failed` — and then close with a `response.completed` whose `output` is empty. The terminal check then sees nothing undeclared and the refused turn enters `previous_response_id` replay state anyway. Rejection is now sticky for the whole turn, set from every parsed payload on the inspection side rather than derived from the terminal snapshot. That required a seam: `SseInspectorHandlers` had no per-payload callback, so the flag could never have been set. `onParsedPayload` is added there, invoked in `scanPayload` before any terminal classification, and carried on `InspectionConsumerOptions` so both `consumeForInspection` and `consumeForResponseLogMetadata` wire it — adding it to the handler type alone would have left the tee path silently inert. The flag is gated on `undeclaredToolGuardActive`, the same condition as the guard. Without that gate a no-catalog or forward-auth stream would mark every call undeclared and stop recording continuation state for exactly the passthrough traffic the guard deliberately stands down for. Regression: a stream whose only undeclared item is in `output_item.added` and whose terminal `output` is empty must give the client `response.failed` and leave nothing for a follow-up to inherit. Driven red against the unfixed flag before landing. --- src/server/relay.ts | 16 +++++++ src/server/responses/core.ts | 20 ++++++++ tests/responses-undeclared-tool-guard.test.ts | 46 +++++++++++++++++++ 3 files changed, 82 insertions(+) diff --git a/src/server/relay.ts b/src/server/relay.ts index 6e5d1de7cf..3cbc870a79 100644 --- a/src/server/relay.ts +++ b/src/server/relay.ts @@ -603,6 +603,13 @@ export type SseInspectorHandlers = { onTerminal?: (status: ResponsesTerminalStatus, httpStatusOverride?: number) => void; logCtx?: RequestLogContext; onCompletedResponse?: (response: { id?: unknown; output?: unknown; status?: unknown }) => void; + /** + * Every parsed SSE payload, delivered BEFORE any onCompletedResponse derived from that same + * payload. A caller that must decide on the whole turn -- not just its terminal snapshot -- + * needs to see the incremental events, because a stream can announce an item and then close + * with an empty `output`. + */ + onParsedPayload?: (payload: unknown) => void; onFirstOutput?: () => void; /** * Provider-scoped compatibility: persist the completed snapshot under the @@ -786,6 +793,11 @@ export function createSseInspector(handlers: SseInspectorHandlers): SseInspector if (!reported && handlers.logCtx) { inspectResponseLogSsePayloadParsed(handlers.logCtx, payload, parsed); } + // Before any terminal handling: a consumer deciding on the whole turn must observe this + // payload even when the terminal snapshot that follows no longer mentions it. + if (handlers.onParsedPayload && parsed !== undefined) { + try { handlers.onParsedPayload(parsed); } catch { /* inspection must never throw into the pump */ } + } reportFirstOutput.parsed(parsed); const status = terminalStatusFromParsed(parsed); if (status) sawTerminal = true; @@ -953,6 +965,8 @@ export type InspectionConsumerOptions = { now?: () => number; /** Forward provider-scoped response-id pinning to the owned inspector. */ pinCompletedResponseIdToFirstSeen?: boolean; + /** Observe every parsed SSE payload on the inspection side; see SseInspectorHandlers. */ + onParsedPayload?: (payload: unknown) => void; /** Test seam for proving both public consumers dispose their owned inspector. */ inspectorFactory?: (handlers: SseInspectorHandlers) => SseInspector; }; @@ -1108,6 +1122,7 @@ export function consumeForInspection( onTerminal, logCtx, onCompletedResponse, + onParsedPayload: options?.onParsedPayload, onFirstOutput, pinCompletedResponseIdToFirstSeen: options?.pinCompletedResponseIdToFirstSeen, }); @@ -1158,6 +1173,7 @@ export function consumeForResponseLogMetadata( const inspector = (options?.inspectorFactory ?? createSseInspector)({ logCtx, onCompletedResponse, + onParsedPayload: options?.onParsedPayload, onFirstOutput, pinCompletedResponseIdToFirstSeen: options?.pinCompletedResponseIdToFirstSeen, }); diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 213d6d8821..5c39e90a02 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -241,6 +241,7 @@ import { collectDeclaredWireToolNames, createUndeclaredToolCallGuardBlockRewrite, undeclaredToolCallMessage, + undeclaredToolCallName, undeclaredToolCallNameInResponse, } from "../responses-undeclared-tool-guard"; import { createGithubCopilotResponsesBlockRewrite } from "../github-copilot-responses-repair"; @@ -2308,8 +2309,25 @@ async function handleResponsesInner( // untouched upstream stream, so it can still observe a `response.completed` the client never // received; checking the payload itself rather than a flag shared with the client relay keeps // this free of tee ordering races. + // + // Checking only the terminal snapshot is not enough. An upstream can announce the undeclared + // call in `response.output_item.added`, which trips the client guard, and then close with a + // `response.completed` whose `output` is empty. The client gets `response.failed`, the terminal + // check sees nothing undeclared, and the refused turn enters continuation state anyway. So the + // rejection is sticky for the whole turn, set from every parsed payload on the inspection side. + let inspectionSawUndeclaredTool = false; + const noteInspectedPayload = (payload: unknown) => { + // Gated on the same flag as the guard itself: with no readable catalog (or a forward-auth + // provider) every name looks undeclared, and flipping this would stop recording continuation + // state for exactly the passthrough traffic the guard deliberately stands down for. + if (!undeclaredToolGuardActive || inspectionSawUndeclaredTool) return; + if (undeclaredToolCallName(payload, declaredWireToolNames) !== undefined) { + inspectionSawUndeclaredTool = true; + } + }; const rememberPassthroughResponseChecked = rememberPassthroughResponse ? (response: { id?: unknown; output?: unknown; status?: unknown }) => { + if (inspectionSawUndeclaredTool) return; if ( undeclaredToolGuardActive && undeclaredToolCallNameInResponse(response, declaredWireToolNames) !== undefined @@ -2775,6 +2793,7 @@ async function handleResponsesInner( onTerminal: reportNativeTerminal, logCtx, onCompletedResponse: rememberPassthroughResponseChecked, + onParsedPayload: noteInspectedPayload, onFirstOutput: options.onFirstOutput, pinCompletedResponseIdToFirstSeen: githubCopilotRepairEnabled, }); @@ -2825,6 +2844,7 @@ async function handleResponsesInner( drainBounds: { ms: 15_000, bytes: 32 * 1024 * 1024 }, upstream, pinCompletedResponseIdToFirstSeen: githubCopilotRepairEnabled, + onParsedPayload: noteInspectedPayload, }; if (recordTerminalOutcomes) { // A real terminal was parsed from the (teed) inspection stream — record it as the outcome diff --git a/tests/responses-undeclared-tool-guard.test.ts b/tests/responses-undeclared-tool-guard.test.ts index 6bb2d019dd..3cf8a08e17 100644 --- a/tests/responses-undeclared-tool-guard.test.ts +++ b/tests/responses-undeclared-tool-guard.test.ts @@ -414,6 +414,52 @@ describe("a refused turn does not become continuation state", () => { // Nothing to inherit: the follow-up keeps only its own single input item. expect(expandedInputLength("resp_refused")).toBe(1); }); + + test("a streamed turn refused mid-stream is not remembered, even when the terminal snapshot is empty", async () => { + // The terminal-snapshot check alone misses this shape: the undeclared call is announced in + // `response.output_item.added` (which trips the client guard) and the stream then closes with + // a `response.completed` carrying an EMPTY output. The client sees `response.failed`, the + // terminal check sees nothing undeclared, and the refused turn would enter continuation state. + const responseId = "resp_stream_refused"; + const sse = [ + `data: ${JSON.stringify({ type: "response.created", response: { id: responseId, status: "in_progress" } })}\n\n`, + `data: ${JSON.stringify({ + type: "response.output_item.added", + output_index: 0, + item: { type: "function_call", id: "fc_bad", call_id: "call_bad", name: "apply_patch", arguments: "{}" }, + })}\n\n`, + `data: ${JSON.stringify({ type: "response.completed", response: { id: responseId, status: "completed", output: [] } })}\n\n`, + "data: [DONE]\n\n", + ].join(""); + + const savedFetch = globalThis.fetch; + globalThis.fetch = (async () => new Response(sse, { + headers: { "content-type": "text/event-stream" }, + })) as typeof fetch; + let response: Response; + try { + response = await handleResponses(new Request("http://localhost/v1/responses", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "fixture/deepseek-v4-flash", + stream: true, + input: [{ role: "user", content: [{ type: "input_text", text: "edit the file" }] }], + tools: declaredTools, + }), + }), config, { model: "", provider: "" }); + } finally { + globalThis.fetch = savedFetch; + } + + // Drain so the inspection side observes the whole stream before we assert on its effect. + const clientStream = await response.text(); + expect(clientStream).toContain("response.failed"); + await Bun.sleep(50); + + // Nothing to inherit: the follow-up keeps only its own single input item. + expect(expandedInputLength(responseId)).toBe(1); + }); }); describe("a request that declares no tools has no catalog to police", () => {