From 320ece6871838e86e3595d4b6b47527edf96f7af Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 22:11:15 +0900 Subject: [PATCH 1/2] fix(responses): fence helper spellings from every manufactured bare alias The bare echo alias was fenced; the bare `tool_choice` selector path was not. A request that declared `mcp__remote.exec` and selected it with a bare selector -- `{name: "exec"}` or `allowedTools: ["exec"]` -- still put bare `exec` into `declaredToolNames` from the second loop, and `normalizeDeclaredToolName` (src/types/tools.ts) then rewrote an undeclared `apply_patch`, `exec_command` or `write_stdin` onto it. Bare logical selection is a supported request shape (`toolAllowedByChoice`), so this was constructible, not hypothetical. Sweeping the rest of the ingress found a third copy of the same rule in a different file. `collectDeclaredWireToolNames` builds the passthrough declared catalog and fenced exactly one name, `exec`, so a namespaced `exec_command`, `shell_command`, `write_stdin`, `apply_patch` or `view_image` still donated its bare spelling to a set that reaches `normalizeDeclaredToolName`, `resolveCodeModeHelperName` and `declaresCodeModeExec`. Bare `exec_command`/`shell_command` there switch nested-helper normalization OFF for a catalog that genuinely declared the shell; the rest are accepted as declared under a name the caller only authorized inside a namespace. Both copies drifted because each site kept its own list, so there is now one: `NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES` in src/types/tools.ts, derived from `CODE_MODE_EXEC_TOOL_NAME` and `CODE_MODE_HELPER_TOOL_NAMES` so it cannot fall out of step with the names it protects. All three sites read it. What is preserved: selection resolves the bare shorthand against the request catalog rather than this map, so a bare selector still selects and still forces the tool; canonical `ns__name` and dotted `ns.name` restoration are untouched; non-helper names keep both their #4679 echo alias and their selector alias; and a genuine namespace-free declaration of `apply_patch` keeps its bare name, since that is the caller declaring a tool rather than a namespace being discarded. A full inventory of `buildToolBridgeMaps` confirms no remaining path gives a namespaced helper a bare spelling, and that `freeformToolNames`, `bareCustomToolNames`, `bareFunctionToolNames` and `toolSearchToolNames` never reach a declared-name or code-mode argument. The invariant is recorded in structure/transports/responses.md. The `mcp__functions.exec` selector case in responses-parser.test.ts moves to a non-helper name so it still pins the bare-selector contract, and gains an explicit assertion that a helper spelling stops at canonical and dotted. --- .../041_preview_fence_test_is_structural.md | 46 +++++++ src/server/responses-undeclared-tool-guard.ts | 14 +- src/server/responses/collaboration.ts | 23 ++-- src/types.ts | 1 + src/types/tools.ts | 24 ++++ structure/transports/responses.md | 14 ++ .../responses-bare-echo-helper-fence.test.ts | 121 +++++++++++++++++- tests/responses/responses-parser.test.ts | 43 +++++-- 8 files changed, 256 insertions(+), 30 deletions(-) create mode 100644 devlog/_plan/260916_release_2570_stabilization/041_preview_fence_test_is_structural.md diff --git a/devlog/_plan/260916_release_2570_stabilization/041_preview_fence_test_is_structural.md b/devlog/_plan/260916_release_2570_stabilization/041_preview_fence_test_is_structural.md new file mode 100644 index 0000000000..27f84becf0 --- /dev/null +++ b/devlog/_plan/260916_release_2570_stabilization/041_preview_fence_test_is_structural.md @@ -0,0 +1,46 @@ +# Follow-up: the preview read-fence test asserts shape, not behaviour + +Raised by the third regression audit on the 2.57.0 candidate, deferred past the +release on purpose. + +## What the test does today + +`tests/responses/responses-preview-main-read-fence.test.ts` reads +`src/server/responses/request-prepare.ts` and `src/codex/auth-context.ts` as +text and asserts with regexes that both native-main read fences carry the +request-owned ownership term, that both preview sites validate ownership the way +`resolveCodexAuthContext` does, that no main exclusion is guarded by drain state +alone, and that `nativeMainSelectionOnly` stays derived from the drain. + +It was written that way deliberately, for the reason recorded in its own header: +driving the divergence end to end needs a `thread_spawn` whose caller bearer is +forwardable, an account-gated candidate model, and a denial cache whose only +entry is main. The sibling contract in +`tests/routing/subagent-fallback-preview-sites.test.ts` made the same call for +the same subsystem. + +## Why that is not sufficient + +A structural assertion catches the regression that has actually recurred twice -- +a fence reconstructed inline from drain state, losing the ownership half -- and +nothing else. It cannot see a fence that is present but wired to the wrong +headers, an ownership term computed against a stale route, or a consumer that +stops reading `nativeMainReadsForbidden`. Any of those is a semantic routing +regression that would keep this file green, which means the file reports more +confidence than it holds. + +## What the replacement needs + +A behavioural case that drives `prepareResponsesRequest` with a forwardable +caller bearer on a `thread_spawn` and observes that the preview performs no +credential-validating read of the physical main token and scores main the same +way final authentication does. The expensive part is the fixture, not the +assertion: an account-gated model, a populated denial cache, and an injected +entitlement resolver that records whether main was consulted. The existing pool +harness in `tests/routing/subagent-fallback-handle-responses.test.ts` already +carries most of it, but that file is at its size cap, so the work is a new file +in `tests/routing/` plus its two layout registrations. + +Keep the structural file when the behavioural one lands. They fail on different +things, and the cheap one is what catches the inline-reconstruction regression +before review. diff --git a/src/server/responses-undeclared-tool-guard.ts b/src/server/responses-undeclared-tool-guard.ts index 58b2ce727c..4cefb22784 100644 --- a/src/server/responses-undeclared-tool-guard.ts +++ b/src/server/responses-undeclared-tool-guard.ts @@ -1,7 +1,7 @@ import { collectAmbiguousDottedAliases, dottedAliasIsUnambiguous, wireToolInnerName } from "../responses/tool-name-aliases"; import { - CODE_MODE_EXEC_TOOL_NAME, dottedToolName, + NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES, namespacedToolName, normalizeDeclaredToolName, } from "../types"; @@ -104,10 +104,14 @@ function addWireToolName( if (dottedAliasIsUnambiguous(namespace, name) && !ambiguousDottedAliases?.has(dotted)) { names.add(dotted); } - // `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`/`view_image` the request never declared. Every other inner name keeps the bare alias. - if (name !== CODE_MODE_EXEC_TOOL_NAME) names.add(name); + // The code-mode helper spellings do not get a bare alias for a namespaced tool. Bare `exec` + // switches nested-helper normalization on for a catalog that never declared the shell; bare + // `exec_command`/`shell_command` switch it off for one that did; bare `write_stdin`/ + // `apply_patch`/`view_image` are simply accepted as declared under a name the caller only ever + // authorized inside a namespace. This guard named only `exec` and let the other five through, + // which is the same drift the bridge-side copy had; both now read one list + // (src/types/tools.ts). Every other inner name keeps the bare alias. + if (!NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES.has(name)) names.add(name); } /** diff --git a/src/server/responses/collaboration.ts b/src/server/responses/collaboration.ts index 0fb059faa8..de4a7f2443 100644 --- a/src/server/responses/collaboration.ts +++ b/src/server/responses/collaboration.ts @@ -30,7 +30,7 @@ import { } from "../../combos"; import { isInjectionDebugEnabled } from "../../lib/debug-settings"; import { injectionDebugLog } from "../../lib/injection-debug-log"; -import { dottedToolName, modelInList, namespacedToolName, toolChoiceToolPredicate } from "../../types"; +import { dottedToolName, modelInList, namespacedToolName, NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES, toolChoiceToolPredicate } from "../../types"; import type { AdapterEvent, OcxConfig, OcxParsedRequest, OcxProviderConfig, OcxProviderContinuationState, OcxUsage } from "../../types"; import { forceRefreshOAuthAccessSnapshot, @@ -156,12 +156,9 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato // bare spelling is only a safe alias while it names ONE tool and cannot be read as // another identity's canonical or dotted spelling. // Code-mode helper spellings never gain a bare alias (#4679 review), whatever namespace - // declares them: admitting bare `exec` into the declared set would authorize the unrelated - // helper normalization that the CODE_MODE_EXEC exception exists to contain. The namespace is - // not the safety property here — the bare spelling is — so this is a property of the NAME. - const BARE_ECHO_EXCLUDED_NAMES = new Set([ - "exec", "exec_command", "shell_command", "write_stdin", "apply_patch", "view_image", - ]); + // declares them and whatever put the alias there. The list is owned by `src/types/tools.ts`, + // beside the names it protects, because the copy that used to live here drifted to a single + // namespace and had to be widened twice. const bareAliasOwners = new Map(); for (const t of authorizedTools) { // Bare (no-namespace) declarations participate as owners too: a namespaced tool whose @@ -223,7 +220,7 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato // is withdrawn, and only for these six spellings. if ( bareAliasOwners.get(t.name) === JSON.stringify([t.namespace, t.name]) - && !BARE_ECHO_EXCLUDED_NAMES.has(t.name) + && !NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES.has(t.name) ) { budget?.chargeRetained(new TextEncoder().encode(t.name).byteLength, { kind: "retained_collectors" }); declaredToolNames.add(t.name); @@ -257,6 +254,15 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato // Some routed providers echo a bare tool_choice selector instead of the flattened catalog // name. Accept only selectors the client actually sent and only when the full request catalog // contains one tool with that logical name. + // + // Under the same helper-spelling fence as the echo path above (#4813 review). An explicit bare + // selector is a narrower request than a bare echo, but the set it writes into is the same one, + // and bare `exec` there is the same single switch: `normalizeDeclaredToolName` would rewrite + // an undeclared `apply_patch`, `exec_command` or `write_stdin` onto the selected tool + // (src/types/tools.ts). Selection itself is unaffected — `toolAllowedByChoice` resolves the + // bare shorthand against the request catalog, not against this map — so the tool is still + // chosen, still forced, and still restores under `ns__name` and `ns.name`. What a helper + // spelling loses here is only the bare RESTORE alias, exactly as it does above. const choice = parsed.options.toolChoice; const bareChoiceNames = new Set( choice && typeof choice === "object" @@ -269,6 +275,7 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato } for (const t of authorizedTools) { if (!t.namespace || !bareChoiceNames.has(t.name) || bareNameCounts.get(t.name) !== 1 || declaredToolNames.has(t.name)) continue; + if (NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES.has(t.name)) continue; budget?.chargeRetained(new TextEncoder().encode(t.name).byteLength, { kind: "retained_collectors" }); declaredToolNames.add(t.name); budget?.chargeRetained(new TextEncoder().encode(JSON.stringify([t.name, t.namespace, t.name])).byteLength, { kind: "retained_collectors" }); diff --git a/src/types.ts b/src/types.ts index c2104f9d41..9ef3936da9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -16,6 +16,7 @@ export { isAllowedToolChoice, toolChoiceToolPredicate, declaresCodeModeExec, + NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES, } from "./types/tools"; export type { UpstreamHttpVersion, ReasoningSummaryDelivery, CodexAccountMode } from "./types/wire"; diff --git a/src/types/tools.ts b/src/types/tools.ts index fd80a4b100..06fe8589e1 100644 --- a/src/types/tools.ts +++ b/src/types/tools.ts @@ -67,6 +67,30 @@ const CODE_MODE_HELPER_TOOL_NAMES = [ */ export const CODE_MODE_EXEC_TOOL_NAME = "exec"; +/** + * Spellings that may never be MANUFACTURED as a bare alias for a namespaced tool. + * + * A bare alias is an ordinary compatibility affordance -- providers echo a namespaced tool + * without its prefix, and restoring the identity needs the bare spelling registered. For these + * six it is also an authorization decision, because a declared-name set is what + * `normalizeDeclaredToolName` and `declaresCodeModeExec` read: bare `exec` turns nested-helper + * normalization on for a catalog that never declared the shell, bare `exec_command` or + * `shell_command` turns it off for one that did, and the rest are accepted as declared calls the + * caller only ever authorized under a namespace. + * + * This is a property of the SPELLING, not of the namespace that declared it and not of the reason + * the alias was being added. It lives here, beside the names it protects, because every site that + * builds a declared-name set has to apply the same list -- the two that kept their own copies each + * drifted, once to a single namespace and once to a single name. + * + * A genuine namespace-free declaration is NOT covered: that is the caller declaring the tool, not + * a namespace being discarded to synthesize a bare name. + */ +export const NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES: ReadonlySet = new Set([ + CODE_MODE_EXEC_TOOL_NAME, + ...CODE_MODE_HELPER_TOOL_NAMES, +]); + /** * Normalizes provider-emitted tool names against declared tool catalogs. * diff --git a/structure/transports/responses.md b/structure/transports/responses.md index bee5e62c77..2b3abe17f0 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -76,6 +76,20 @@ the tool surface so request-local aliases remain available for response restorat item records which tool actually ran, so re-pointing it at a same-named namespace child would rewrite that record on a coincidence rather than translate it. +A namespaced tool is registered under every coordinate a provider might echo — `ns__name`, the +dotted `ns.name`, and the bare `name` — but six spellings never get the bare one: +`exec`, `exec_command`, `shell_command`, `write_stdin`, `apply_patch`, `view_image` +(`NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES`). A declared-name set is what decides nested-helper +normalization, so bare `exec` from a namespace turns it on for a catalog that never declared the +shell, and `normalizeDeclaredToolName` then rewrites an undeclared `apply_patch` onto it. The +fence is a property of the SPELLING, not of the declaring namespace and not of why the alias was +being added — both drifted once, one to `collaboration` only and one to `exec` only, and each +drift was a live authorization widening. Every site that builds a declared-name set reads the one +list: `buildToolBridgeMaps` for the echo and `tool_choice` selector paths, and +`collectDeclaredWireToolNames` for the passthrough catalog. A genuine namespace-free declaration +is untouched: that is the caller declaring the tool, not a namespace being discarded to +manufacture a bare name. + Codex-private tool fields are removed at the same boundary from one table (`CANONICAL_ONLY_TOOL_FIELDS`) rather than one bespoke pass each: `external_web_access` on either web-search variant, and `defer_loading` on any declaration, which `activateDeferredTool` clears only diff --git a/tests/responses/responses-bare-echo-helper-fence.test.ts b/tests/responses/responses-bare-echo-helper-fence.test.ts index a21a19ae33..722fbf52b9 100644 --- a/tests/responses/responses-bare-echo-helper-fence.test.ts +++ b/tests/responses/responses-bare-echo-helper-fence.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import { parseRequest } from "../../src/responses/parser"; import { buildToolBridgeMaps } from "../../src/server/responses"; +import { collectDeclaredWireToolNames } from "../../src/server/responses-undeclared-tool-guard"; import { normalizeDeclaredToolName, declaresCodeModeExec } from "../../src/types/tools"; /** @@ -12,16 +13,21 @@ import { normalizeDeclaredToolName, declaresCodeModeExec } from "../../src/types * * The exclusion that prevents that was once scoped to the `collaboration` namespace, which made * the boundary a property of the declaring namespace rather than of the spelling, and any other - * namespace could then donate the bare name. These cases pin the exclusion to the NAME, and pin - * the half that has to keep working beside it: the namespaced tool stays reachable under the - * spellings that carry their namespace, and non-helper names keep their #4679 echo fallback. + * namespace could then donate the bare name. Fencing the echo path then left the SELECTOR path + * open one level down: a bare `tool_choice` for a namespaced helper name added the same bare + * spelling to the same set from a different loop. + * + * These cases pin the exclusion to the NAME across both paths, and pin the half that has to keep + * working beside it: the namespaced tool stays reachable under the spellings that carry their + * namespace, selection by bare shorthand still resolves, and non-helper names keep both their + * #4679 echo fallback and their bare selector alias. * * Kept out of `bare-echo-alias.test.ts` so the namespace-independence contract has a file of its * own rather than growing the file that pins the original collaboration-only behaviour. */ -function namespacedToolRequest(namespace: string, name: string) { - return parseRequest({ +function namespacedToolRequest(namespace: string, name: string, choiceNames?: string[]) { + const parsed = parseRequest({ model: "claude-opus-5", input: "run it", tools: [{ @@ -30,11 +36,16 @@ function namespacedToolRequest(namespace: string, name: string) { tools: [{ type: "function", name, parameters: { type: "object" } }], }], }); + // Assigned rather than parsed from `tool_choice`, the way the sibling selector cases in + // `responses-parser.test.ts` do it. The loop under test reads `options.toolChoice` and nothing + // else, so going through selector validation would only add a second thing that can fail. + if (choiceNames) parsed.options.toolChoice = { allowedTools: choiceNames, mode: "required" }; + return parsed; } const HELPER_SPELLINGS = ["exec", "exec_command", "shell_command", "write_stdin", "apply_patch", "view_image"]; -describe("helper spellings are fenced from the bare echo alias in every namespace", () => { +describe("helper spellings are fenced from every bare alias, in every namespace", () => { test("a foreign namespace donates no helper spelling", () => { const donated = HELPER_SPELLINGS.filter(name => { const maps = buildToolBridgeMaps(namespacedToolRequest("mcp__remote", name)); @@ -73,6 +84,59 @@ describe("helper spellings are fenced from the bare echo alias in every namespac expect(maps.toolNsMap.get("list_issues")).toMatchObject({ namespace: "mcp__remote", name: "list_issues" }); }); + test("an explicit bare tool_choice selector donates no helper spelling either", () => { + // The bypass one level down: the echo path is fenced, so the selector loop was the remaining + // way to put bare `exec` in the declared set. It writes into the same set, so it is the same + // switch -- a narrower request shape, not a narrower consequence. + const donated = HELPER_SPELLINGS.filter(name => { + const maps = buildToolBridgeMaps(namespacedToolRequest("mcp__remote", name, [name])); + return maps.declaredToolNames.has(name) || maps.toolNsMap.has(name); + }); + + expect(donated).toEqual([]); + }); + + test("a bare selector in the single-name form is fenced the same way", () => { + // `tool_choice: {name}` and `tool_choice: {allowedTools}` reach the selector loop through the + // same `bareChoiceNames` set, so both forms are pinned rather than only the one a fixture + // happened to build. + const parsed = namespacedToolRequest("mcp__remote", "exec", ["exec"]); + parsed.options.toolChoice = { name: "exec" }; + const maps = buildToolBridgeMaps(parsed); + + expect(maps.declaredToolNames.has("exec")).toBe(false); + expect(maps.toolNsMap.has("exec")).toBe(false); + expect(declaresCodeModeExec(maps.declaredToolNames)).toBe(false); + }); + + test("a bare selector still SELECTS the helper tool and restores under its own spellings", () => { + // The fence withdraws the bare restore alias, not the selection. `toolAllowedByChoice` + // resolves the bare shorthand against the request catalog, so the tool stays authorized and + // stays forced; it simply answers to the spellings that carry its namespace. + const maps = buildToolBridgeMaps(namespacedToolRequest("mcp__remote", "exec", ["exec"])); + + expect(maps.declaredToolNames.has("mcp__remote__exec")).toBe(true); + expect(maps.declaredToolNames.has("mcp__remote.exec")).toBe(true); + expect(maps.toolNsMap.get("mcp__remote.exec")).toMatchObject({ namespace: "mcp__remote", name: "exec" }); + }); + + test("canonical and dotted selectors are unaffected for a helper name", () => { + for (const selector of ["mcp__remote__exec", "mcp__remote.exec"]) { + const maps = buildToolBridgeMaps(namespacedToolRequest("mcp__remote", "exec", [selector])); + expect([selector, maps.declaredToolNames.has(selector)]).toEqual([selector, true]); + expect([selector, maps.declaredToolNames.has("exec")]).toEqual([selector, false]); + } + }); + + test("a non-helper name still gains its bare alias through the selector path", () => { + // Same narrowness check as the echo path: fencing the selector loop must not take the bare + // selector alias away from every other namespaced tool. + const maps = buildToolBridgeMaps(namespacedToolRequest("mcp__remote", "list_issues", ["list_issues"])); + + expect(maps.declaredToolNames.has("list_issues")).toBe(true); + expect(maps.toolNsMap.get("list_issues")).toMatchObject({ namespace: "mcp__remote", name: "list_issues" }); + }); + test("the withheld name is exactly what would have turned helper normalization on", () => { // The consequence, asserted against the consumer rather than restated: a declared set that // carries bare `exec` rewrites undeclared helper calls onto it. This is the set the previous @@ -88,3 +152,48 @@ describe("helper spellings are fenced from the bare echo alias in every namespac .toEqual(["apply_patch", "exec_command", "write_stdin"]); }); }); + +/** + * The passthrough guard builds its own declared-name catalog from the outbound body, and it feeds + * the same consumers: `undeclaredNameInItem` passes it to `normalizeDeclaredToolName`, and + * custom-tool restoration passes it on to `resolveCodeModeHelperName` and + * `declaresCodeModeExec`. It had the same fence written as a single name -- `exec` -- so the + * other five spellings still got a bare alias for an arbitrary namespace. Both sites now read one + * list, so these cases are the other half of the same invariant. + */ +describe("the passthrough declared-name catalog applies the same fence", () => { + test("a namespaced helper gets canonical and dotted spellings but no bare alias", () => { + const withheld = HELPER_SPELLINGS.filter(name => collectDeclaredWireToolNames({ + tools: [{ type: "namespace", name: "mcp", tools: [{ type: "function", name }] }], + }).has(name)); + + expect(withheld).toEqual([]); + }); + + test("the namespaced spellings themselves are still admitted", () => { + const names = collectDeclaredWireToolNames({ + tools: [{ type: "namespace", name: "mcp", tools: [{ type: "function", name: "apply_patch" }] }], + }); + + expect([...names]).toEqual(["mcp__apply_patch", "mcp.apply_patch"]); + }); + + test("a genuine top-level helper declaration keeps its bare name", () => { + // The line the fence must not cross. Here the caller really did declare `apply_patch` as a + // bare tool; no namespace is being discarded to synthesize the spelling, so withholding it + // would refuse a call the request plainly authorized. + const names = collectDeclaredWireToolNames({ + tools: [{ type: "custom", name: "apply_patch" }, { type: "function", name: "exec" }], + }); + + expect([...names].sort()).toEqual(["apply_patch", "exec"]); + }); + + test("a non-helper namespaced tool keeps all three spellings", () => { + const names = collectDeclaredWireToolNames({ + tools: [{ type: "namespace", name: "linear", tools: [{ type: "function", name: "create_issue" }] }], + }); + + expect([...names].sort()).toEqual(["create_issue", "linear.create_issue", "linear__create_issue"]); + }); +}); diff --git a/tests/responses/responses-parser.test.ts b/tests/responses/responses-parser.test.ts index b42e286c8d..b09001cefe 100644 --- a/tests/responses/responses-parser.test.ts +++ b/tests/responses/responses-parser.test.ts @@ -227,26 +227,26 @@ describe("Responses parser", () => { tools: [{ type: "namespace", name: "mcp__functions", - tools: [{ type: "custom", name: "exec", description: "Run a command" }], + tools: [{ type: "custom", name: "run_command", description: "Run a command" }], }], tool_choice: { type: "allowed_tools", mode: "required", - tools: [{ type: "custom", name: "exec" }], + tools: [{ type: "custom", name: "run_command" }], }, }); let maps = buildToolBridgeMaps(parsed); expect([...maps.toolNsMap]).toEqual([ - ["mcp__functions__exec", { namespace: "mcp__functions", name: "exec", freeform: true }], - ["mcp__functions.exec", { namespace: "mcp__functions", name: "exec", freeform: true }], - ["exec", { namespace: "mcp__functions", name: "exec", freeform: true }], + ["mcp__functions__run_command", { namespace: "mcp__functions", name: "run_command", freeform: true }], + ["mcp__functions.run_command", { namespace: "mcp__functions", name: "run_command", freeform: true }], + ["run_command", { namespace: "mcp__functions", name: "run_command", freeform: true }], ]); - expect([...maps.declaredToolNames]).toEqual(["mcp__functions__exec", "mcp__functions.exec", "exec"]); - expect([...maps.freeformToolNames]).toEqual(["exec"]); + expect([...maps.declaredToolNames]).toEqual(["mcp__functions__run_command", "mcp__functions.run_command", "run_command"]); + expect([...maps.freeformToolNames]).toEqual(["run_command"]); const bridged = buildResponseJSON([ - { type: "tool_call_start", id: "call_exec", name: "exec" }, + { type: "tool_call_start", id: "call_exec", name: "run_command" }, { type: "tool_call_delta", arguments: '{"input":"pwd"}' }, { type: "tool_call_end" }, { type: "done" }, @@ -255,14 +255,35 @@ describe("Responses parser", () => { expect((bridged.output as Record[])[0]).toMatchObject({ type: "custom_tool_call", call_id: "call_exec", - name: "exec", + name: "run_command", input: "pwd", status: "completed", }); - parsed.options.toolChoice = { name: "exec" }; + parsed.options.toolChoice = { name: "run_command" }; maps = buildToolBridgeMaps(parsed); - expect([...maps.toolNsMap.keys()]).toEqual(["mcp__functions__exec", "mcp__functions.exec", "exec"]); + expect([...maps.toolNsMap.keys()]).toEqual(["mcp__functions__run_command", "mcp__functions.run_command", "run_command"]); + + // A code-mode helper spelling is the exception, and it is the spelling that decides -- not the + // namespace and not the fact that the caller selected it. Bare `exec` in the declared set is + // what turns nested-helper normalization on, so the selector grants canonical and dotted + // restoration and stops there. Selection itself is unaffected; only the bare alias is. + const helperSelector = parseRequest({ + model: "claude-opus-5", + input: "run it", + tools: [{ + type: "namespace", + name: "mcp__functions", + tools: [{ type: "custom", name: "exec", description: "Run a command" }], + }], + tool_choice: { + type: "allowed_tools", + mode: "required", + tools: [{ type: "custom", name: "exec" }], + }, + }); + expect([...buildToolBridgeMaps(helperSelector).declaredToolNames]) + .toEqual(["mcp__functions__exec", "mcp__functions.exec"]); expect(() => parseRequest({ model: "claude-opus-5", From 4b6afa770580c3116e2efe9bb48848d299572938 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 22:49:20 +0900 Subject: [PATCH 2/2] fix(responses): keep the selector identity alias while fencing the declaration CI on #4819 refused a call it should have allowed: `responses-undeclared-tool-guard.test.ts:2047` posts a catalog declaring `mcp__functions.exec` with `tool_choice: {name: "exec"}`, the provider echoes bare `exec`, and the turn answered 502 instead of 200. That test encodes a legitimate shape, and it already proved the boundary holds: the same case refuses `apply_patch`, `exec_command`, `shell_command`, `write_stdin` and `view_image` from the identical request. It could do both because passthrough separates the two jobs a bare alias does. Identity RESTORATION runs first -- `authorizedBareNamespaceToolAliases` is built from `toolNsMap`, rewrites the echoed bare name to the namespaced identity, and the guard then authorizes `ns__name` -- while DECLARATION is withheld, because `declaredWireToolNames` admits bare `exec` from the bridge map only when the client's own catalog declared it. The selector fence withdrew both, so restoration stopped firing and a declared, explicitly selected tool became undeclared. Only the declaration is unsafe: a declared-name set carrying bare `exec` is what makes `normalizeDeclaredToolName` rewrite an undeclared helper name onto the selected tool. So the selector path now writes `toolNsMap` and `toolParameterSchemas` and skips `declaredToolNames` for the six spellings. The caller nominated exactly one tool by name, `bareNameCounts` proves nothing else answers to it, and restoring that identity authorizes nothing the request had not already declared. The echo path keeps withholding both. A bare echo is a guess rather than a nomination, and `tests/responses/bare-echo-alias.test.ts` has pinned the `toolNsMap` exclusion there since #4679. The bridges check the declared set before consulting `toolNsMap`, so a bare helper echo is refused there either way; that is the safe side and it is unchanged. Fixture updated to assert the split rather than a blanket absence, and the `mcp__functions.exec` case in responses-parser.test.ts now pins both halves. structure/transports/responses.md states the invariant on declaration rather than on aliasing. --- src/server/responses/collaboration.ts | 29 ++++++---- structure/transports/responses.md | 32 +++++++---- .../responses-bare-echo-helper-fence.test.ts | 57 ++++++++++++------- tests/responses/responses-parser.test.ts | 12 ++-- 4 files changed, 80 insertions(+), 50 deletions(-) diff --git a/src/server/responses/collaboration.ts b/src/server/responses/collaboration.ts index de4a7f2443..b40d3d01ec 100644 --- a/src/server/responses/collaboration.ts +++ b/src/server/responses/collaboration.ts @@ -255,14 +255,19 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato // name. Accept only selectors the client actually sent and only when the full request catalog // contains one tool with that logical name. // - // Under the same helper-spelling fence as the echo path above (#4813 review). An explicit bare - // selector is a narrower request than a bare echo, but the set it writes into is the same one, - // and bare `exec` there is the same single switch: `normalizeDeclaredToolName` would rewrite - // an undeclared `apply_patch`, `exec_command` or `write_stdin` onto the selected tool - // (src/types/tools.ts). Selection itself is unaffected — `toolAllowedByChoice` resolves the - // bare shorthand against the request catalog, not against this map — so the tool is still - // chosen, still forced, and still restores under `ns__name` and `ns.name`. What a helper - // spelling loses here is only the bare RESTORE alias, exactly as it does above. + // A helper spelling selected this way is split rather than refused (#4819). The two things a + // bare alias does are separable, and passthrough already relies on that: identity RESTORATION + // runs before authorization there, rewriting the echoed bare name to the namespaced identity + // the caller declared, and the guard then authorizes `ns__name`. DECLARATION is the part that + // is unsafe, because a declared-name set carrying bare `exec` is what makes + // `normalizeDeclaredToolName` rewrite an undeclared `apply_patch`, `exec_command` or + // `write_stdin` onto the selected tool (src/types/tools.ts). + // + // So a helper spelling gets the `toolNsMap` entry and not the `declaredToolNames` entry. The + // caller nominated exactly one tool by name, `bareNameCounts` proves nothing else answers to + // it, and restoring it authorizes nothing the request did not already declare. The echo path + // above withholds both, because a bare echo is a guess rather than a nomination and #4679 + // pinned that shape (`tests/responses/bare-echo-alias.test.ts`). const choice = parsed.options.toolChoice; const bareChoiceNames = new Set( choice && typeof choice === "object" @@ -275,9 +280,11 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato } for (const t of authorizedTools) { if (!t.namespace || !bareChoiceNames.has(t.name) || bareNameCounts.get(t.name) !== 1 || declaredToolNames.has(t.name)) continue; - if (NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES.has(t.name)) continue; - budget?.chargeRetained(new TextEncoder().encode(t.name).byteLength, { kind: "retained_collectors" }); - declaredToolNames.add(t.name); + // Restore the identity; declare the name only when it is not a helper spelling. + if (!NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES.has(t.name)) { + budget?.chargeRetained(new TextEncoder().encode(t.name).byteLength, { kind: "retained_collectors" }); + declaredToolNames.add(t.name); + } budget?.chargeRetained(new TextEncoder().encode(JSON.stringify([t.name, t.namespace, t.name])).byteLength, { kind: "retained_collectors" }); toolNsMap.set(t.name, { namespace: t.namespace, name: t.name, ...(t.freeform ? { freeform: true } : {}) }); if (t.parameters && typeof t.parameters === "object") { diff --git a/structure/transports/responses.md b/structure/transports/responses.md index 2b3abe17f0..94dc919bbe 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -77,18 +77,26 @@ item records which tool actually ran, so re-pointing it at a same-named namespac rewrite that record on a coincidence rather than translate it. A namespaced tool is registered under every coordinate a provider might echo — `ns__name`, the -dotted `ns.name`, and the bare `name` — but six spellings never get the bare one: -`exec`, `exec_command`, `shell_command`, `write_stdin`, `apply_patch`, `view_image` -(`NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES`). A declared-name set is what decides nested-helper -normalization, so bare `exec` from a namespace turns it on for a catalog that never declared the -shell, and `normalizeDeclaredToolName` then rewrites an undeclared `apply_patch` onto it. The -fence is a property of the SPELLING, not of the declaring namespace and not of why the alias was -being added — both drifted once, one to `collaboration` only and one to `exec` only, and each -drift was a live authorization widening. Every site that builds a declared-name set reads the one -list: `buildToolBridgeMaps` for the echo and `tool_choice` selector paths, and -`collectDeclaredWireToolNames` for the passthrough catalog. A genuine namespace-free declaration -is untouched: that is the caller declaring the tool, not a namespace being discarded to -manufacture a bare name. +dotted `ns.name`, and the bare `name` — but six spellings never reach a DECLARED-NAME set under +the bare one: `exec`, `exec_command`, `shell_command`, `write_stdin`, `apply_patch`, +`view_image` (`NAMESPACED_BARE_ALIAS_EXCLUDED_NAMES`). A declared-name set is what decides +nested-helper normalization, so bare `exec` from a namespace turns it on for a catalog that never +declared the shell, and `normalizeDeclaredToolName` then rewrites an undeclared `apply_patch` +onto it. The fence is a property of the SPELLING, not of the declaring namespace and not of why +the alias was being added — both copies drifted once, one to `collaboration` only and one to +`exec` only, and each drift was a live authorization widening. Every site that builds a +declared-name set reads the one list: `buildToolBridgeMaps` for the echo and `tool_choice` +selector paths, and `collectDeclaredWireToolNames` for the passthrough catalog. + +Declaration and restoration are separate, and only declaration is fenced. Passthrough rewrites an +echoed bare name to its namespaced identity before authorizing anything +(`authorizedBareNamespaceToolAliases`, built from `toolNsMap`), and the guard then authorizes +`ns__name`, so a `tool_choice` that nominates one helper tool by its bare name keeps the +`toolNsMap` entry and loses only the declaration. The echo path withholds both, because a bare +echo is a guess rather than a nomination. The bridges check the declared set before consulting +`toolNsMap`, so there a bare helper echo is refused either way. A genuine namespace-free +declaration is untouched throughout: that is the caller declaring the tool, not a namespace being +discarded to manufacture a bare name. Codex-private tool fields are removed at the same boundary from one table (`CANONICAL_ONLY_TOOL_FIELDS`) rather than one bespoke pass each: `external_web_access` on either diff --git a/tests/responses/responses-bare-echo-helper-fence.test.ts b/tests/responses/responses-bare-echo-helper-fence.test.ts index 722fbf52b9..4c4f1503d0 100644 --- a/tests/responses/responses-bare-echo-helper-fence.test.ts +++ b/tests/responses/responses-bare-echo-helper-fence.test.ts @@ -17,10 +17,14 @@ import { normalizeDeclaredToolName, declaresCodeModeExec } from "../../src/types * open one level down: a bare `tool_choice` for a namespaced helper name added the same bare * spelling to the same set from a different loop. * - * These cases pin the exclusion to the NAME across both paths, and pin the half that has to keep - * working beside it: the namespaced tool stays reachable under the spellings that carry their - * namespace, selection by bare shorthand still resolves, and non-helper names keep both their - * #4679 echo fallback and their bare selector alias. + * These cases pin the exclusion to the NAME across both paths, and pin the halves that have to + * keep working beside it. The line the fence runs along is DECLARATION, not restoration: no + * declared-name set ever gains a manufactured bare helper spelling, while the `toolNsMap` + * identity entry an explicit selector creates survives, because passthrough restores an echoed + * bare name to its namespaced identity before authorizing it and would otherwise refuse a call + * the caller had both declared and selected. The namespaced tool also stays reachable under the + * spellings that carry its namespace, selection by bare shorthand still resolves, and non-helper + * names keep both their #4679 echo fallback and their bare selector alias. * * Kept out of `bare-echo-alias.test.ts` so the namespace-independence contract has a file of its * own rather than growing the file that pins the original collaboration-only behaviour. @@ -45,7 +49,7 @@ function namespacedToolRequest(namespace: string, name: string, choiceNames?: st const HELPER_SPELLINGS = ["exec", "exec_command", "shell_command", "write_stdin", "apply_patch", "view_image"]; -describe("helper spellings are fenced from every bare alias, in every namespace", () => { +describe("helper spellings are fenced from every declared-name set, in every namespace", () => { test("a foreign namespace donates no helper spelling", () => { const donated = HELPER_SPELLINGS.filter(name => { const maps = buildToolBridgeMaps(namespacedToolRequest("mcp__remote", name)); @@ -84,35 +88,44 @@ describe("helper spellings are fenced from every bare alias, in every namespace" expect(maps.toolNsMap.get("list_issues")).toMatchObject({ namespace: "mcp__remote", name: "list_issues" }); }); - test("an explicit bare tool_choice selector donates no helper spelling either", () => { + test("an explicit bare tool_choice selector declares no helper spelling", () => { // The bypass one level down: the echo path is fenced, so the selector loop was the remaining - // way to put bare `exec` in the declared set. It writes into the same set, so it is the same - // switch -- a narrower request shape, not a narrower consequence. - const donated = HELPER_SPELLINGS.filter(name => { - const maps = buildToolBridgeMaps(namespacedToolRequest("mcp__remote", name, [name])); - return maps.declaredToolNames.has(name) || maps.toolNsMap.has(name); - }); + // way to put bare `exec` in the DECLARED set, which is the set that switches nested-helper + // normalization on. + const declared = HELPER_SPELLINGS.filter( + name => buildToolBridgeMaps(namespacedToolRequest("mcp__remote", name, [name])).declaredToolNames.has(name), + ); - expect(donated).toEqual([]); + expect(declared).toEqual([]); + }); + + test("but it does keep the identity alias, which is what restores the call", () => { + // The half that must survive. Passthrough restores an echoed bare name to the namespaced + // identity BEFORE authorizing it (`authorizedBareNamespaceToolAliases` in + // passthrough-dispatch.ts reads exactly this map), and the guard then authorizes + // `ns__name`. Withholding the map entry too refused a call the caller had declared and + // explicitly selected. + for (const name of HELPER_SPELLINGS) { + const maps = buildToolBridgeMaps(namespacedToolRequest("mcp__remote", name, [name])); + expect([name, maps.toolNsMap.get(name)]).toEqual([name, { namespace: "mcp__remote", name }]); + } }); - test("a bare selector in the single-name form is fenced the same way", () => { - // `tool_choice: {name}` and `tool_choice: {allowedTools}` reach the selector loop through the - // same `bareChoiceNames` set, so both forms are pinned rather than only the one a fixture - // happened to build. + test("both tool_choice forms behave the same way", () => { + // `{name}` and `{allowedTools}` reach the selector loop through the same `bareChoiceNames` + // set, so both forms are pinned rather than only the one a fixture happened to build. const parsed = namespacedToolRequest("mcp__remote", "exec", ["exec"]); parsed.options.toolChoice = { name: "exec" }; const maps = buildToolBridgeMaps(parsed); expect(maps.declaredToolNames.has("exec")).toBe(false); - expect(maps.toolNsMap.has("exec")).toBe(false); expect(declaresCodeModeExec(maps.declaredToolNames)).toBe(false); + expect(maps.toolNsMap.get("exec")).toEqual({ namespace: "mcp__remote", name: "exec" }); }); - test("a bare selector still SELECTS the helper tool and restores under its own spellings", () => { - // The fence withdraws the bare restore alias, not the selection. `toolAllowedByChoice` - // resolves the bare shorthand against the request catalog, so the tool stays authorized and - // stays forced; it simply answers to the spellings that carry its namespace. + test("a bare selector still SELECTS the helper tool and declares its own spellings", () => { + // `toolAllowedByChoice` resolves the bare shorthand against the request catalog rather than + // against this map, so the tool stays authorized and stays forced. const maps = buildToolBridgeMaps(namespacedToolRequest("mcp__remote", "exec", ["exec"])); expect(maps.declaredToolNames.has("mcp__remote__exec")).toBe(true); diff --git a/tests/responses/responses-parser.test.ts b/tests/responses/responses-parser.test.ts index b09001cefe..4b6605c316 100644 --- a/tests/responses/responses-parser.test.ts +++ b/tests/responses/responses-parser.test.ts @@ -265,9 +265,9 @@ describe("Responses parser", () => { expect([...maps.toolNsMap.keys()]).toEqual(["mcp__functions__run_command", "mcp__functions.run_command", "run_command"]); // A code-mode helper spelling is the exception, and it is the spelling that decides -- not the - // namespace and not the fact that the caller selected it. Bare `exec` in the declared set is - // what turns nested-helper normalization on, so the selector grants canonical and dotted - // restoration and stops there. Selection itself is unaffected; only the bare alias is. + // namespace and not the fact that the caller selected it. Bare `exec` in the DECLARED set is + // what turns nested-helper normalization on, so the selector grants the identity alias that + // restores the call without granting the declaration that would rewrite helper names onto it. const helperSelector = parseRequest({ model: "claude-opus-5", input: "run it", @@ -282,8 +282,10 @@ describe("Responses parser", () => { tools: [{ type: "custom", name: "exec" }], }, }); - expect([...buildToolBridgeMaps(helperSelector).declaredToolNames]) - .toEqual(["mcp__functions__exec", "mcp__functions.exec"]); + const helperMaps = buildToolBridgeMaps(helperSelector); + expect([...helperMaps.declaredToolNames]).toEqual(["mcp__functions__exec", "mcp__functions.exec"]); + expect(helperMaps.toolNsMap.get("exec")) + .toEqual({ namespace: "mcp__functions", name: "exec", freeform: true }); expect(() => parseRequest({ model: "claude-opus-5",