Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 104 additions & 11 deletions src/server/responses-self-named-namespace-scrub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,104 @@ function isPlainObject(value: unknown): value is Record<string, unknown> {
return !!value && typeof value === "object" && !Array.isArray(value);
}

export interface SelfNamedNamespaceScrubAuthorization {
customToolCallNames: ReadonlySet<string>;
functionCallNames: ReadonlySet<string>;
}

function collectBareCustomToolSpecs(
bareNames: Set<string>,
sameNameNamespacedCustomNames: Set<string>,
sameNameNamespacedFunctionNames: Set<string>,
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) {
if (!isPlainObject(inner) || typeof inner.name !== "string") continue;
if (namespace === inner.name) {
if (inner.type === "custom") sameNameNamespacedCustomNames.add(inner.name);
else if (inner.type === "function") sameNameNamespacedFunctionNames.add(inner.name);
}
if (inner.type === "custom" && namespace === "functions") bareNames.add(inner.name);
Comment thread
alex-jordan547 marked this conversation as resolved.
Resolved
Comment thread
alex-jordan547 marked this conversation as resolved.
Resolved
}
continue;
}
if (typeof spec.name !== "string") continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Recognize nested function declarations in scrub authorization

For a supported Chat-shaped declaration such as {type:"function", function:{name:"wait"}}, parseRequest unwraps the nested function and buildToolBridgeMaps authorizes bare wait, but this collector requires spec.name and therefore records nothing. If the upstream returns the malformed {type:"function_call", name:"wait", namespace:"wait"} shape, the namespace remains and Codex resolves it as waitwait, recreating the retry loop this scrub is intended to stop. Read spec.function.name here as the parser and undeclared-tool collector already do, and cover both top-level and additional_tools catalogs.

AGENTS.md reference: src/AGENTS.md:L19-L19

Useful? React with 👍 / 👎.

const namespace = typeof spec.namespace === "string" ? spec.namespace : undefined;
if (namespace === spec.name) {
if (spec.type === "custom") sameNameNamespacedCustomNames.add(spec.name);
else if (spec.type === "function") sameNameNamespacedFunctionNames.add(spec.name);
}
if (spec.type === "custom" && (!namespace || namespace === "functions")) bareNames.add(spec.name);
}
}

/** Bare custom tools authorized by this turn, scoped to each response call type. */
export function collectSelfNamedNamespaceScrubAuthorization(
body: unknown,
authorizedBareCustomToolNames: ReadonlySet<string>,
): SelfNamedNamespaceScrubAuthorization {
const bareNames = new Set<string>();
const sameNameNamespacedCustomNames = new Set<string>();
const sameNameNamespacedFunctionNames = new Set<string>();
if (isPlainObject(body)) {
collectBareCustomToolSpecs(
bareNames,
sameNameNamespacedCustomNames,
sameNameNamespacedFunctionNames,
body.tools,
);
if (Array.isArray(body.input)) {
for (const item of body.input) {
if (
isPlainObject(item)
&& (item.type === "additional_tools" || item.type === "tool_search_output")
) {
collectBareCustomToolSpecs(
bareNames,
sameNameNamespacedCustomNames,
sameNameNamespacedFunctionNames,
item.tools,
);
}
}
}
}
const authorizedNames = [...bareNames].filter(name => authorizedBareCustomToolNames.has(name));
return {
customToolCallNames: new Set(
authorizedNames.filter(name => !sameNameNamespacedCustomNames.has(name)),
),
functionCallNames: new Set(
authorizedNames.filter(name => !sameNameNamespacedFunctionNames.has(name)),
),
};
}

/**
* Drop a tool-call `namespace` that merely repeats the call's own `name` (#3217).
*
* codex-rs resolves a client tool call as `ToolName::new(namespace, name)` and treats only
* `None | "" | "functions"` as the default namespace; anything else is concatenated into a flat
* name before routing. A backend answer of `{ name: "exec", namespace: "exec" }` therefore
* becomes `execexec`, which no client tool matches, and Codex re-issues the same call forever.
* That shape is never a legitimate identity — an MCP namespace is a server name, not the tool —
* so it is safe to scrub without consulting the declared catalog. The adapter fix that stops
* provoking the answer lives in `stripSparkCompatibility`; this is the belt to that suspender.
* The malformed Spark shape is scrubbed only when the current turn authorized a bare custom tool
* with that name. A genuine namespaced tool may intentionally use the same namespace and name.
* The adapter fix that stops provoking the answer lives in `stripSparkCompatibility`; this is the
* belt to that suspender.
*/
export function scrubSelfNamedToolCallNamespace(value: unknown): { value: unknown; changed: boolean } {
export function scrubSelfNamedToolCallNamespace(
value: unknown,
authorization: SelfNamedNamespaceScrubAuthorization,
): { value: unknown; changed: boolean } {
if (Array.isArray(value)) {
let changed = false;
const out = value.map(entry => {
const result = scrubSelfNamedToolCallNamespace(entry);
const result = scrubSelfNamedToolCallNamespace(entry, authorization);
changed ||= result.changed;
return result.value;
});
Expand All @@ -29,35 +111,46 @@ export function scrubSelfNamedToolCallNamespace(value: unknown): { value: unknow
let changed = false;
const out: Record<string, unknown> = {};
for (const [key, entry] of Object.entries(value)) {
const result = scrubSelfNamedToolCallNamespace(entry);
const result = scrubSelfNamedToolCallNamespace(entry, authorization);
out[key] = result.value;
changed ||= result.changed;
}
const authorizedNames = value.type === "custom_tool_call"
? authorization.customToolCallNames
: value.type === "function_call"
? authorization.functionCallNames
: undefined;
if (
(value.type === "custom_tool_call" || value.type === "function_call")
authorizedNames
&& typeof value.name === "string"
&& value.name.length > 0
&& value.namespace === value.name
&& authorizedNames.has(value.name)
) {
delete out.namespace;
changed = true;
}
return changed ? { value: out, changed: true } : { value, changed: false };
}

export function scrubSelfNamedToolCallNamespaceInJson(text: string): string {
export function scrubSelfNamedToolCallNamespaceInJson(
text: string,
authorization: SelfNamedNamespaceScrubAuthorization,
): string {
if (!text.includes("\"namespace\"")) return text;
let payload: unknown;
try {
payload = JSON.parse(text);
} catch {
return text;
}
const result = scrubSelfNamedToolCallNamespace(payload);
const result = scrubSelfNamedToolCallNamespace(payload, authorization);
return result.changed ? JSON.stringify(result.value) : text;
}

export function createSelfNamedToolCallNamespaceScrubRewrite(): SsePayloadRewrite {
return payload => scrubSelfNamedToolCallNamespaceInJson(payload);
export function createSelfNamedToolCallNamespaceScrubRewrite(
authorization: SelfNamedNamespaceScrubAuthorization,
): SsePayloadRewrite {
return payload => scrubSelfNamedToolCallNamespaceInJson(payload, authorization);
}

15 changes: 14 additions & 1 deletion src/server/responses/collaboration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,14 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato
/** Declared parameter schema per request-visible tool name (#1611 integer repair). */
toolParameterSchemas: Map<string, Record<string, unknown>>;
freeformToolNames: Set<string>;
bareCustomToolNames: Set<string>;
toolSearchToolNames: Set<string>;
} {
const toolNsMap = new Map<string, { namespace: string; name: string; freeform?: true }>();
const declaredToolNames = new Set<string>();
const toolParameterSchemas = new Map<string, Record<string, unknown>>();
const freeformToolNames = new Set<string>();
const bareCustomToolNames = new Set<string>();
const toolSearchToolNames = new Set<string>();
const requestedTools = parsed.context.tools ?? [];
const toolAllowed = toolChoiceToolPredicate(parsed.options.toolChoice, requestedTools);
Expand All @@ -133,6 +135,10 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato
if (t.freeform) {
budget?.chargeRetained(new TextEncoder().encode(t.name).byteLength, { kind: "retained_collectors" });
freeformToolNames.add(t.name);
if (!t.namespace || t.namespace === "functions") {
budget?.chargeRetained(new TextEncoder().encode(t.name).byteLength, { kind: "retained_collectors" });
bareCustomToolNames.add(t.name);
}
}
if (t.toolSearch) {
budget?.chargeRetained(new TextEncoder().encode(t.name).byteLength, { kind: "retained_collectors" });
Expand Down Expand Up @@ -162,7 +168,14 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato
toolParameterSchemas.set(t.name, t.parameters);
}
}
return { toolNsMap, declaredToolNames, toolParameterSchemas, freeformToolNames, toolSearchToolNames };
return {
toolNsMap,
declaredToolNames,
toolParameterSchemas,
freeformToolNames,
bareCustomToolNames,
toolSearchToolNames,
};
}


Expand Down
12 changes: 10 additions & 2 deletions src/server/responses/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ import {
} from "../responses-image-gen-repair";
import { createResponsesModelPayloadRewrite, rewriteResponsesModelJson } from "../responses-model-rewrite";
import {
collectSelfNamedNamespaceScrubAuthorization,
createSelfNamedToolCallNamespaceScrubRewrite,
scrubSelfNamedToolCallNamespaceInJson,
} from "../responses-self-named-namespace-scrub";
Expand Down Expand Up @@ -3661,6 +3662,10 @@ async function handleResponsesInner(
parsed._rawBody,
replayedInputPrefixLength,
);
const selfNamedNamespaceScrubAuthorization = collectSelfNamedNamespaceScrubAuthorization(
clientToolAuthorizationBody,
toolBridgeMaps.bareCustomToolNames,
);
const clientExplicitWireToolCatalog = hasExplicitWireToolCatalog(clientToolAuthorizationBody);
const clientDeclaredWireToolNames = collectDeclaredWireToolNames(clientToolAuthorizationBody);
const clientDeclaredNamelessCallTypes = collectDeclaredNamelessClientCallTypes(
Expand Down Expand Up @@ -4646,7 +4651,7 @@ async function handleResponsesInner(
const payloadRewrites = [
createImageGenCallRestoreRewrite(imageGenCallAliases),
// #3217: a call whose namespace repeats its own name is unroutable in codex-rs.
createSelfNamedToolCallNamespaceScrubRewrite(),
createSelfNamedToolCallNamespaceScrubRewrite(selfNamedNamespaceScrubAuthorization),
routedNamespaceToolAliases.size > 0
? createRoutedNamespaceCallRestoreRewrite(routedNamespaceToolAliases)
: undefined,
Expand Down Expand Up @@ -4879,7 +4884,10 @@ async function handleResponsesInner(
inspectResponseLogJson(logCtx, text);
const clientJson = (() => {
const restoredNamespace = restoreRoutedNamespaceCallsInJson(
scrubSelfNamedToolCallNamespaceInJson(restoreImageGenCallsInJson(text, imageGenCallAliases)),
scrubSelfNamedToolCallNamespaceInJson(
restoreImageGenCallsInJson(text, imageGenCallAliases),
selfNamedNamespaceScrubAuthorization,
),
routedNamespaceToolAliases,
);
const restoredAuthorizedBareNamespace = restoreRoutedNamespaceCallsInJson(
Expand Down
122 changes: 117 additions & 5 deletions tests/responses-self-named-namespace-scrub.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,19 @@ function sseFrom(items: Array<Record<string, unknown>>): string {
return events.map(e => `event: ${e.type}\ndata: ${JSON.stringify(e)}\n\n`).join("");
}

function request(): Request {
function request(body: Record<string, unknown> = requestBody): Request {
return new Request("http://localhost/v1/responses", {
method: "POST",
headers: { "content-type": "application/json", authorization: "Bearer test", "chatgpt-account-id": "acct" },
body: JSON.stringify(requestBody),
body: JSON.stringify(body),
});
}

function scrubAuthorization(names: string[]) {
const authorizedNames = new Set(names);
return { customToolCallNames: authorizedNames, functionCallNames: authorizedNames };
}

test("a self-named namespace on a passthrough custom_tool_call is scrubbed before the client (#3217)", async () => {
const call = { type: "custom_tool_call", id: "ctc_1", call_id: "call_1", name: "exec", namespace: "exec", input: "pwd", status: "completed" };
globalThis.fetch = (async () => new Response(sseFrom([call]), {
Expand All @@ -91,6 +96,113 @@ test("a self-named namespace on a passthrough custom_tool_call is scrubbed befor
}
});

test("a self-named namespace declared by the current turn is preserved", async () => {
const call = { type: "custom_tool_call", id: "ctc_1", call_id: "call_1", name: "exec", namespace: "exec", input: "pwd", status: "completed" };
globalThis.fetch = (async () => new Response(sseFrom([call]), {
status: 200, headers: { "content-type": "text/event-stream" },
})) as typeof fetch;
const body = {
...requestBody,
input: [
{
type: "additional_tools",
role: "developer",
tools: [{
type: "namespace",
name: "exec",
tools: [{ type: "custom", name: "exec", description: "shell" }],
}],
},
requestBody.input[1],
],
};

const res = await handleResponses(request(body), forwardConfig(), { model: "", provider: "" });
expect(res.status).toBe(200);
const text = await res.text();
expect(text).toContain('\"name\":\"exec\",\"namespace\":\"exec\"');
});

test("tool_choice for a namespaced custom tool cannot authorize a colliding bare scrub", async () => {
const call = { type: "custom_tool_call", id: "ctc_1", call_id: "call_1", name: "exec", namespace: "exec", input: "pwd", status: "completed" };
globalThis.fetch = (async () => new Response(sseFrom([call]), {
status: 200, headers: { "content-type": "text/event-stream" },
})) as typeof fetch;
const body = {
...requestBody,
tool_choice: { type: "custom", name: "remote__exec" },
input: [
{
type: "additional_tools",
role: "developer",
tools: [
{
type: "namespace",
name: "functions",
tools: [{ type: "custom", name: "exec", description: "local shell" }],
},
{
type: "namespace",
name: "remote",
tools: [{ type: "custom", name: "exec", description: "remote shell" }],
},
],
},
requestBody.input[1],
],
};

const res = await handleResponses(request(body), forwardConfig(), { model: "", provider: "" });
expect(res.status).toBe(200);
const text = await res.text();
expect(text).toContain('\"name\":\"exec\",\"namespace\":\"exec\"');
});

test("mixed custom and function collisions are scoped to the response item type", async () => {
const call = { type: "function_call", id: "fc_1", call_id: "call_1", name: "exec", namespace: "exec", arguments: "{}", status: "completed" };
globalThis.fetch = (async () => new Response(sseFrom([call]), {
status: 200, headers: { "content-type": "text/event-stream" },
})) as typeof fetch;
const body = {
...requestBody,
input: [
{
type: "additional_tools",
role: "developer",
tools: [
{
type: "namespace",
name: "functions",
tools: [{ type: "custom", name: "exec", description: "shell" }],
},
{
type: "namespace",
name: "exec",
tools: [{ type: "function", name: "exec", parameters: { type: "object", properties: {} } }],
},
],
},
requestBody.input[1],
],
};

const res = await handleResponses(request(body), forwardConfig(), { model: "", provider: "" });
expect(res.status).toBe(200);
const text = await res.text();
expect(text).toContain('\"type\":\"function_call\"');
expect(text).toContain('\"name\":\"exec\",\"namespace\":\"exec\"');

const bareCall = { type: "custom_tool_call", id: "ctc_1", call_id: "call_2", name: "exec", namespace: "exec", input: "pwd", status: "completed" };
globalThis.fetch = (async () => new Response(sseFrom([bareCall]), {
status: 200, headers: { "content-type": "text/event-stream" },
})) as typeof fetch;
const bareRes = await handleResponses(request(body), forwardConfig(), { model: "", provider: "" });
expect(bareRes.status).toBe(200);
const bareText = await bareRes.text();
expect(bareText).toContain('\"type\":\"custom_tool_call\"');
expect(bareText).not.toContain('\"namespace\":\"exec\"');
});

test("a genuine MCP namespace on a passthrough call is left alone", async () => {
const call = { type: "function_call", id: "fc_1", call_id: "call_1", name: "search", namespace: "mcp__docs", arguments: "{}", status: "completed" };
globalThis.fetch = (async () => new Response(sseFrom([call]), {
Expand Down Expand Up @@ -122,11 +234,11 @@ test("the bounded JSON (stream:false) passthrough path scrubs the same shape (#3

test("scrub is recursive, shape-preserving, and a no-op on clean payloads", () => {
const clean = { type: "response.completed", response: { output: [{ type: "custom_tool_call", name: "exec", input: "" }] } };
expect(scrubSelfNamedToolCallNamespace(clean)).toEqual({ value: clean, changed: false });
expect(scrubSelfNamedToolCallNamespace(clean, scrubAuthorization(["exec"]))).toEqual({ value: clean, changed: false });
const dirty = { response: { output: [{ type: "function_call", name: "wait", namespace: "wait", arguments: "{}" }, { type: "message" }] } };
const result = scrubSelfNamedToolCallNamespace(dirty);
const result = scrubSelfNamedToolCallNamespace(dirty, scrubAuthorization(["wait"]));
expect(result.changed).toBe(true);
expect(result.value).toEqual({ response: { output: [{ type: "function_call", name: "wait", arguments: "{}" }, { type: "message" }] } });
// An empty name never matches: a namespace equal to "" is not the self-named shape.
expect(scrubSelfNamedToolCallNamespace({ type: "custom_tool_call", name: "", namespace: "" }).changed).toBe(false);
expect(scrubSelfNamedToolCallNamespace({ type: "custom_tool_call", name: "", namespace: "" }, scrubAuthorization([])).changed).toBe(false);
});
Loading