fix(responses): fence helper spellings from every manufactured bare alias

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.

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);
 }
 
 /**

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<string, string | null>();
   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,20 @@ 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.
+  //
+  // 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"
@@ -269,8 +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;
-    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") {

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";

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<string> = new Set<string>([
+  CODE_MODE_EXEC_TOOL_NAME,
+  ...CODE_MODE_HELPER_TOOL_NAMES,
+]);
+
 /**
  * Normalizes provider-emitted tool names against declared tool catalogs.
  *

structure/transports/responses.md

@@ -76,6 +76,28 @@ 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 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
 web-search variant, and `defer_loading` on any declaration, which `activateDeferredTool` clears only