diff --git a/src/server/responses-undeclared-tool-guard.ts b/src/server/responses-undeclared-tool-guard.ts index 12c99fd233..57099b1ce4 100644 --- a/src/server/responses-undeclared-tool-guard.ts +++ b/src/server/responses-undeclared-tool-guard.ts @@ -1,8 +1,14 @@ -import { namespacedToolName, normalizeDeclaredToolName } from "../types"; +import { + CODE_MODE_EXEC_TOOL_NAME, + namespacedToolName, + normalizeDeclaredToolName, +} from "../types"; import { sseDataPayload, type SseBlockRewrite } from "./sse-payload-rewrite"; /** Item types the client executes through a request-declared wire name. */ const CLIENT_EXECUTED_CALL_TYPES = new Set(["function_call", "custom_tool_call"]); +/** Codex groups ordinary top-level tools here; unlike an MCP namespace, it has no wire prefix. */ +const BUILTIN_FUNCTIONS_NAMESPACE = "functions"; /** * Hosted declarations whose response items the PROVIDER executes, keyed by the request @@ -79,11 +85,18 @@ function addWireToolName(names: Set, tool: unknown, namespace?: string): ? nestedFunction.name : undefined; if (!name) return; - names.add(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, name)); + if (!namespace || namespace === BUILTIN_FUNCTIONS_NAMESPACE) { + names.add(name); + return; + } + names.add(namespacedToolName(namespace, name)); + // `exec` is the one name that also switches on nested-helper normalization, so a bare alias + // for a namespaced MCP tool would silently authorize `exec_command`/`shell_command`/ + // `apply_patch` the request never declared. Every other inner name keeps the bare alias. + if (name !== CODE_MODE_EXEC_TOOL_NAME) names.add(name); } /** diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index b06237fac9..e8891ae24d 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -86,7 +86,7 @@ import { import { injectionDebugLog } from "../../lib/injection-debug-log"; import { resolveClientRetryAfter } from "../../lib/retry-after"; import { enrichOpenCodeZenRateLimitMessage } from "../../providers/opencode-zen-rate-limit"; -import { modelInList, namespacedToolName } from "../../types"; +import { CODE_MODE_EXEC_TOOL_NAME, modelInList, namespacedToolName } from "../../types"; import type { AdapterEvent, OcxConfig, @@ -351,6 +351,7 @@ import { createRoutedToolSearchRestoreBlockRewrite } from "../responses-tool-sea import { createRoutedNamespaceCallRestoreRewrite, NamespaceToolCollisionError, + restoreRoutedNamespaceCalls, restoreRoutedNamespaceCallsInJson, type RoutedNamespaceToolAliases, } from "../../responses/namespace-tool-compat"; @@ -3634,6 +3635,23 @@ async function handleResponsesInner( let outboundRequestBody: Record | undefined; const declaredWireToolNames = new Set(); const declaredNamelessClientCallTypes = new Set(); + // `buildToolBridgeMaps` creates a bare alias only when the caller selected exactly one + // namespaced tool through a bare tool_choice. Restore that request-bounded identity before + // authorization checks instead of admitting the bare name into the declared set: for `exec`, + // the latter would also authorize the unrelated code-mode helper names. + const authorizedBareNamespaceToolAliases: RoutedNamespaceToolAliases = new Map( + [...toolBridgeMaps.toolNsMap].flatMap(([alias, identity]) => + alias === identity.name + ? [[alias, { + namespace: identity.namespace, + name: identity.name, + kind: identity.freeform ? "custom" as const : "function" as const, + }] as const] + : [] + ), + ); + const restoreAuthorizedBareNamespaceToolCalls = (value: unknown): unknown => + restoreRoutedNamespaceCalls(value, authorizedBareNamespaceToolAliases).value; let undeclaredToolGuardActive = false; const refreshUndeclaredToolGuard = (builtRequest: AdapterRequest): void => { outboundRequestBody = parseOutboundRequestBody(builtRequest.body); @@ -3677,7 +3695,19 @@ async function handleResponsesInner( // however, the parsed maps also contain historical catalog entries, so only the bounded // current-turn wire snapshot above may authorize a call. if (replayedInputPrefixLength === 0) { - for (const name of toolBridgeMaps.declaredToolNames) declaredWireToolNames.add(name); + for (const name of toolBridgeMaps.declaredToolNames) { + // `buildToolBridgeMaps` also aliases a namespaced tool under its bare name when the + // caller's `tool_choice` selected it unambiguously, which the bridge needs to route the + // call back. For `exec` alone that alias would also switch on nested-helper + // normalization and re-authorize `exec_command`/`shell_command`/`apply_patch`, so it is + // admitted here only when the caller's own catalog declared a bare `exec`. Selecting an + // MCP `exec` is not a declaration of the code-mode shell tool. + if ( + name === CODE_MODE_EXEC_TOOL_NAME + && !clientDeclaredWireToolNames.has(CODE_MODE_EXEC_TOOL_NAME) + ) continue; + declaredWireToolNames.add(name); + } } undeclaredToolGuardActive = ( declaredWireToolNames.size > 0 @@ -3703,7 +3733,7 @@ async function handleResponsesInner( // state for exactly the passthrough traffic the guard deliberately stands down for. if (!undeclaredToolGuardActive || inspectionSawUndeclaredTool) return; if (undeclaredToolCallName( - payload, + restoreAuthorizedBareNamespaceToolCalls(payload), declaredWireToolNames, declaredNamelessClientCallTypes, providerExecutedCallTypes, @@ -3715,7 +3745,7 @@ async function handleResponsesInner( ? (response: { id?: unknown; output?: unknown; status?: unknown }) => { if (inspectionSawUndeclaredTool) return; const restoredResponse = restoreRoutedCustomCalls( - response, + restoreAuthorizedBareNamespaceToolCalls(response), routedCustomToolNames, routedCustomToolRepairNames, declaredWireToolNames, @@ -4449,6 +4479,9 @@ async function handleResponsesInner( routedNamespaceToolAliases.size > 0 ? createRoutedNamespaceCallRestoreRewrite(routedNamespaceToolAliases) : undefined, + authorizedBareNamespaceToolAliases.size > 0 + ? createRoutedNamespaceCallRestoreRewrite(authorizedBareNamespaceToolAliases) + : undefined, hasResponsesItemIdRepair(repairConfig) ? createResponsesItemIdPayloadRewrite(repairConfig!, translatorBudget) : undefined, @@ -4678,8 +4711,12 @@ async function handleResponsesInner( restoreImageGenCallsInJson(text, imageGenCallAliases), routedNamespaceToolAliases, ); - const restored = restoreRoutedCustomCallsInJson( + const restoredAuthorizedBareNamespace = restoreRoutedNamespaceCallsInJson( restoredNamespace, + authorizedBareNamespaceToolAliases, + ); + const restored = restoreRoutedCustomCallsInJson( + restoredAuthorizedBareNamespace, routedCustomToolNames, routedCustomToolRepairNames, declaredWireToolNames, diff --git a/src/types.ts b/src/types.ts index d7acd066dd..08880878df 100644 --- a/src/types.ts +++ b/src/types.ts @@ -3,6 +3,7 @@ export type { OcxTool, OcxToolChoice } from "./types/tools"; export { + CODE_MODE_EXEC_TOOL_NAME, namespacedToolName, normalizeDeclaredToolName, toolChoiceAliases, diff --git a/src/types/tools.ts b/src/types/tools.ts index d79c1c0121..8f713be620 100644 --- a/src/types/tools.ts +++ b/src/types/tools.ts @@ -44,20 +44,30 @@ export function namespacedToolName(namespace: string | undefined, name: string): const LEGACY_SHELL_BRIDGE_TOOL_NAMES = ["exec_command", "shell_command"] as const; const CODE_MODE_HELPER_TOOL_NAMES = [...LEGACY_SHELL_BRIDGE_TOOL_NAMES, "apply_patch"] as const; +/** + * The one declared name that turns nested-helper normalization on. Declaring it is not just a + * name: it also decides whether an emitted `exec_command`/`shell_command`/`apply_patch` is + * accepted as that shell tool, so callers that build declared-name sets must add it only for a + * genuine bare declaration. + */ +export const CODE_MODE_EXEC_TOOL_NAME = "exec"; + export function normalizeDeclaredToolName( name: string, declared: ReadonlySet | undefined, ): string { - if (!declared || !declared.has("exec")) return name; + if (!declared || !declared.has(CODE_MODE_EXEC_TOOL_NAME)) return name; if (declared.has(name)) return name; - if (name === "apply_patch") return "exec"; + if (name === "apply_patch") return CODE_MODE_EXEC_TOOL_NAME; // When the catalog explicitly declares any legacy shell bridge name, the environment // genuinely exposes that tool — turn normalization off so a call is never mis-routed // to `exec`. if ((LEGACY_SHELL_BRIDGE_TOOL_NAMES as readonly string[]).some(legacy => declared.has(legacy))) { return name; } - return (CODE_MODE_HELPER_TOOL_NAMES as readonly string[]).includes(name) ? "exec" : name; + return (CODE_MODE_HELPER_TOOL_NAMES as readonly string[]).includes(name) + ? CODE_MODE_EXEC_TOOL_NAME + : name; } export function toolChoiceAliases(tool: Pick): string[] { diff --git a/tests/responses-undeclared-tool-guard.test.ts b/tests/responses-undeclared-tool-guard.test.ts index 9864a3c8cd..99a73cf8c1 100644 --- a/tests/responses-undeclared-tool-guard.test.ts +++ b/tests/responses-undeclared-tool-guard.test.ts @@ -96,6 +96,42 @@ describe("collectDeclaredWireToolNames", () => { ); }); + test("withholds the bare alias when only a namespaced exec was declared", () => { + // A bare `exec` in the declared set is not just a name: it switches on nested-helper + // normalization, so aliasing a namespaced MCP `exec` under the bare name would authorize + // `exec_command`/`shell_command`/`apply_patch` this request never declared. + const names = collectDeclaredWireToolNames({ + tools: [{ type: "namespace", name: "mcp", tools: [{ type: "function", name: "exec" }] }], + }); + + expect([...names]).toEqual(["mcp__exec"]); + }); + + test("keeps exec bare in Codex's reserved functions namespace", () => { + // Codex groups ordinary top-level tools here; this is not an MCP namespace and the parser + // deliberately lowers its children without a namespace. + const names = collectDeclaredWireToolNames({ + tools: [{ + type: "namespace", + name: "functions", + tools: [{ type: "custom", name: "exec", description: "Run a command" }], + }], + }); + + expect([...names]).toEqual(["exec"]); + }); + + test("keeps the bare alias when the request also declared a top-level exec", () => { + const names = collectDeclaredWireToolNames({ + tools: [ + { type: "custom", name: "exec" }, + { type: "namespace", name: "mcp", tools: [{ type: "function", name: "exec" }] }, + ], + }); + + expect([...names].sort()).toEqual(["exec", "mcp__exec"]); + }); + 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({ @@ -711,6 +747,7 @@ describe("empty and absent tool catalogs", () => { history: unknown[] = [], model = "fixture/deepseek-v4-flash", previousResponseId?: string, + toolChoice?: unknown, ) { const savedFetch = globalThis.fetch; globalThis.fetch = (async () => upstream()) as typeof fetch; @@ -730,6 +767,7 @@ describe("empty and absent tool catalogs", () => { : [{ type: "additional_tools", role: "developer", tools: additionalTools }]), ], ...(tools === undefined ? {} : { tools }), + ...(toolChoice === undefined ? {} : { tool_choice: toolChoice }), }), }), requestConfig, { model: "", provider: "" }); } finally { @@ -1177,6 +1215,48 @@ describe("empty and absent tool catalogs", () => { expect(currentBody.output[0]).toMatchObject({ name: "exec" }); }); + test("a replayed functions namespace still authorizes its top-level exec", async () => { + const previousId = "resp_replay_with_functions_exec"; + const prime = await post( + false, + undefined, + () => Response.json({ id: previousId, status: "completed", output: [] }), + [{ type: "function", name: "historical", parameters: { type: "object" } }], + ); + expect(prime.status).toBe(200); + await prime.arrayBuffer(); + + const response = await post( + false, + undefined, + () => Response.json({ + id: "resp_functions_exec", + status: "completed", + output: [{ + type: "custom_tool_call", + id: "ctc_functions_exec", + call_id: "call_functions_exec", + name: "exec", + input: "echo allowed", + status: "completed", + }], + }), + [{ + type: "namespace", + name: "functions", + tools: [{ type: "custom", name: "exec", description: "Run a command" }], + }], + config, + [], + "fixture/deepseek-v4-flash", + previousId, + ); + + expect(response.status).toBe(200); + const body = await response.json() as { output: Array> }; + expect(body.output[0]).toMatchObject({ name: "exec", type: "custom_tool_call" }); + }); + test("a current tool-search output still authorizes its discovered tool after replay", async () => { const previousId = "resp_replay_with_current_tool_search_output"; const prime = await post( @@ -1295,6 +1375,84 @@ describe("empty and absent tool catalogs", () => { const refusedBody = await refused.json() as { error: { message: string } }; expect(refusedBody.error.message).toContain('undeclared client tool "apply_patch"'); }); + + test("a bare tool_choice restores only its unambiguous namespaced exec", async () => { + const tools = [{ + type: "namespace", + name: "mcp__functions", + tools: [{ type: "function", name: "exec", description: "Run a command", parameters: { type: "object" } }], + }]; + const toolChoice = { type: "function", name: "exec" }; + const upstreamCall = (name: string) => () => Response.json({ + id: `resp_${name}`, + status: "completed", + output: [{ + type: "function_call", + id: `fc_${name}`, + call_id: `call_${name}`, + name, + arguments: "{}", + status: "completed", + }], + }); + + const accepted = await post( + false, + tools, + upstreamCall("exec"), + undefined, + config, + [], + "fixture/deepseek-v4-flash", + undefined, + toolChoice, + ); + expect(accepted.status).toBe(200); + const acceptedBody = await accepted.json() as { output: Array> }; + expect(acceptedBody.output[0]).toMatchObject({ + type: "function_call", + name: "exec", + namespace: "mcp__functions", + }); + + for (const name of ["apply_patch", "exec_command", "shell_command"]) { + const refused = await post( + false, + tools, + upstreamCall(name), + undefined, + config, + [], + "fixture/deepseek-v4-flash", + undefined, + toolChoice, + ); + expect(refused.status).toBe(502); + const body = await refused.json() as { error: { message: string } }; + expect(body.error.message).toContain(`undeclared client tool "${name}"`); + } + }); + + test("a request that really declares a bare exec still accepts the helper names", () => { + return post( + false, + [ + { type: "custom", name: "exec", description: "Run a command" }, + { + type: "namespace", + name: "mcp__functions", + tools: [{ type: "custom", name: "exec", description: "Run a command" }], + }, + ], + jsonUpstream, + ).then(async response => { + expect(response.status).toBe(200); + const body = await response.json() as { output: Array> }; + // Accepted and normalized onto the declared code-mode shell tool, exactly as before. + expect(body.output[0]).toMatchObject({ name: "exec", type: "custom_tool_call" }); + expect(String(body.output[0]?.input)).toContain("tools.apply_patch"); + }); + }); }); describe("undeclaredToolCallNameInResponse", () => { @@ -1360,6 +1518,31 @@ describe("undeclaredToolCallNameInResponse", () => { ); expect(undeclaredToolCallNameInResponse(namespaced, new Set(["exec", "mcp__server__exec_command"]))).toBeUndefined(); }); + + test("a namespaced-only exec declaration does not authorize the nested helper names", () => { + // End-to-end over the real collector: declaring `exec` inside an MCP namespace must not + // hand the request a bare code-mode shell tool. + const declared = collectDeclaredWireToolNames({ + tools: [{ type: "namespace", name: "mcp", tools: [{ type: "function", name: "exec" }] }], + }); + + for (const name of ["exec_command", "shell_command", "apply_patch", "exec"]) { + expect(undeclaredToolCallNameInResponse( + { output: [{ type: "function_call", name, call_id: "call_1" }] }, + declared, + )).toBe(name); + } + + // The declared tool itself still answers under either coordinate system. + expect(undeclaredToolCallNameInResponse( + { output: [{ type: "function_call", name: "exec", namespace: "mcp", call_id: "call_1" }] }, + declared, + )).toBeUndefined(); + expect(undeclaredToolCallNameInResponse( + { output: [{ type: "function_call", name: "mcp__exec", call_id: "call_1" }] }, + declared, + )).toBeUndefined(); + }); }); /**