-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(responses): carry bare tool-name echo acceptance (#4729) #4792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4fe64a0
c47cbda
f80cc60
0d169c4
c4a196a
56a0c38
adee043
39d0649
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,6 +151,38 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato | |
| dottedAliasOwners.set(t.name, null); | ||
| } | ||
| } | ||
| // Bare echo alias (`name` with no namespace spelling, #4679): some providers — observed | ||
| // on the muse family via Command Code — echo a namespaced tool by its bare name. The | ||
| // 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): admitting bare | ||
| // `exec` into the declared set would authorize the unrelated helper normalization that | ||
| // the CODE_MODE_EXEC exception exists to contain. | ||
| const BARE_ECHO_EXCLUDED_NAMES = new Set([ | ||
| "exec", "exec_command", "shell_command", "write_stdin", "apply_patch", "view_image", | ||
| ]); | ||
| const bareAliasOwners = new Map<string, string | null>(); | ||
| for (const t of authorizedTools) { | ||
| // Bare (no-namespace) declarations participate as owners too: a namespaced tool whose | ||
| // bare name equals a bare-declared function must not gain the bare alias, mirroring how | ||
| // the tool_choice bare path refuses ambiguous owners across the whole request catalog. | ||
| const identity = JSON.stringify([t.namespace ?? null, t.name]); | ||
| const owner = bareAliasOwners.get(t.name); | ||
| if (owner === undefined) bareAliasOwners.set(t.name, identity); | ||
| else if (owner !== identity) bareAliasOwners.set(t.name, null); | ||
| } | ||
| for (const t of authorizedTools) { | ||
| const canonical = namespacedToolName(t.namespace, t.name); | ||
| const owner = bareAliasOwners.get(canonical); | ||
| if (owner !== undefined && owner !== JSON.stringify([t.namespace, t.name])) { | ||
| bareAliasOwners.set(canonical, null); | ||
| } | ||
| const dotted = dottedToolName(t.namespace, t.name); | ||
| const dottedOwner = bareAliasOwners.get(dotted); | ||
| if (dottedOwner !== undefined && dottedOwner !== JSON.stringify([t.namespace, t.name])) { | ||
| bareAliasOwners.set(dotted, null); | ||
| } | ||
| } | ||
| for (const t of authorizedTools) { | ||
| // Upstream output is untrusted: only restore calls for tools the caller authorized. | ||
| const wireName = namespacedToolName(t.namespace, t.name); | ||
|
|
@@ -174,6 +206,24 @@ export function buildToolBridgeMaps(parsed: OcxParsedRequest, budget?: Translato | |
| toolNsMap.set(dottedName, { namespace: t.namespace, name: t.name, ...(t.freeform ? { freeform: true } : {}) }); | ||
| if (t.parameters && typeof t.parameters === "object") toolParameterSchemas.set(dottedName, t.parameters); | ||
| } | ||
| // Bare echo alias (`name` with no namespace spelling, #4679): same tool identity as | ||
| // the flattened wire name, so a provider that drops the namespace prefix still | ||
| // restores against this entry. Ambiguous bare names were resolved to null above; | ||
| // skipping them falls back to the spellings every provider can still echo. | ||
| // Code-mode helper spellings on the collaboration surface never gain a bare alias: | ||
| // admitting bare `exec` there would let normalizeDeclaredToolName authorize unrelated | ||
| // helper names. A namespaced custom `exec` from another catalog (for example | ||
| // `mcp__functions.exec`) remains an ordinary caller-declared tool. | ||
| if ( | ||
| bareAliasOwners.get(t.name) === JSON.stringify([t.namespace, t.name]) | ||
| && !(t.namespace === "collaboration" && BARE_ECHO_EXCLUDED_NAMES.has(t.name)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an adapter-routed request declares only a namespaced Useful? React with 👍 / 👎. |
||
| ) { | ||
| 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") toolParameterSchemas.set(t.name, t.parameters); | ||
| } | ||
| } | ||
| if (t.freeform) { | ||
| budget?.chargeRetained(new TextEncoder().encode(t.name).byteLength, { kind: "retained_collectors" }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The commit message explicitly identifies this change as a carry and supersession of #4729, but the commit object contains no AGENTS.md reference: AGENTS.md:L288-L292 Useful? React with 👍 / 👎. |
||
| import { parseRequest } from "../../src/responses/parser"; | ||
| import { buildToolBridgeMaps } from "../../src/server/responses"; | ||
|
|
||
| function collabRequest(bareName: string) { | ||
| return parseRequest({ | ||
| model: "meta/muse-spark-1.3-contributor", | ||
| input: [ | ||
| { type: "additional_tools", role: "developer", tools: [ | ||
| { type: "namespace", name: "collaboration", tools: [ | ||
| { type: "function", name: bareName, description: bareName, strict: false, parameters: { type: "object", properties: {}, required: [] } }, | ||
| ] }, | ||
| ] }, | ||
| { type: "message", role: "user", content: [{ type: "input_text", text: "run it" }] }, | ||
| ], | ||
| } as any); | ||
| } | ||
|
|
||
| describe("bare echo alias for namespaced tools (#4679)", () => { | ||
| test("an unambiguous bare name is declared and restores to the namespaced identity", () => { | ||
| const maps = buildToolBridgeMaps(collabRequest("list_agents") as any); | ||
| expect(maps.declaredToolNames.has("list_agents")).toBe(true); | ||
| expect(maps.toolNsMap.get("list_agents")).toEqual({ namespace: "collaboration", name: "list_agents" }); | ||
| }); | ||
|
|
||
| test("Code Mode helper names never gain a bare alias", () => { | ||
| const maps = buildToolBridgeMaps(collabRequest("exec") as any); // justified: parsed fixture matches the request wire shape | ||
| expect(maps.declaredToolNames.has("collaboration__exec")).toBe(true); | ||
| expect(maps.declaredToolNames.has("exec")).toBe(false); | ||
| expect(maps.toolNsMap.has("exec")).toBe(false); | ||
| }); | ||
|
|
||
| test("a bare name claimed by two namespaces stays undeclared (no hijack)", () => { | ||
| const parsed = parseRequest({ | ||
| model: "meta/muse-spark-1.3-contributor", | ||
| input: [ | ||
| { type: "additional_tools", role: "developer", tools: [ | ||
| { type: "namespace", name: "collaboration", tools: [ | ||
| { type: "function", name: "list_agents", description: "a", strict: false, parameters: { type: "object", properties: {}, required: [] } }, | ||
| ] }, | ||
| { type: "namespace", name: "other__ns", tools: [ | ||
| { type: "function", name: "list_agents", description: "b", strict: false, parameters: { type: "object", properties: {}, required: [] } }, | ||
| ] }, | ||
| ] }, | ||
| { type: "message", role: "user", content: [{ type: "input_text", text: "run it" }] }, | ||
| ], | ||
| } as any); | ||
| const maps = buildToolBridgeMaps(parsed as any); | ||
| expect(maps.declaredToolNames.has("list_agents")).toBe(false); | ||
| expect(maps.toolNsMap.has("list_agents")).toBe(false); | ||
| // Both canonical spellings remain declared. | ||
| expect(maps.declaredToolNames.has("collaboration__list_agents")).toBe(true); | ||
| expect(maps.declaredToolNames.has("other__ns__list_agents")).toBe(true); | ||
| }); | ||
|
|
||
| test("a bare name that equals another tool's dotted spelling stays undeclared", () => { | ||
| const parsed = parseRequest({ | ||
| model: "meta/muse-spark-1.3-contributor", | ||
| input: [ | ||
| { type: "additional_tools", role: "developer", tools: [ | ||
| { type: "namespace", name: "collaboration", tools: [ | ||
| { type: "function", name: "list_agents", description: "a", strict: false, parameters: { type: "object", properties: {}, required: [] } }, | ||
| ] }, | ||
| { type: "namespace", name: "mcp__x", tools: [ | ||
| { type: "function", name: "collaboration.list_agents", description: "b", strict: false, parameters: { type: "object", properties: {}, required: [] } }, | ||
| ] }, | ||
| ] }, | ||
| { type: "message", role: "user", content: [{ type: "input_text", text: "run it" }] }, | ||
| ], | ||
| } as any); | ||
| const maps = buildToolBridgeMaps(parsed as any); | ||
| // Tool B's bare name ("collaboration.list_agents") collides with tool A's dotted | ||
| // spelling, so that bare alias is poisoned; tool A's dotted spelling is poisoned in | ||
| // return by the pre-existing dotted rule. Tool B's own distinct dotted alias does not | ||
| // collide with anything and stays declared, as do both canonical spellings. | ||
| expect(maps.declaredToolNames.has("collaboration.list_agents")).toBe(false); | ||
| expect(maps.toolNsMap.has("collaboration.list_agents")).toBe(false); | ||
| expect(maps.declaredToolNames.has("mcp__x.collaboration.list_agents")).toBe(true); | ||
| expect(maps.toolNsMap.get("mcp__x.collaboration.list_agents")).toEqual({ namespace: "mcp__x", name: "collaboration.list_agents" }); | ||
| expect(maps.declaredToolNames.has("collaboration__list_agents")).toBe(true); | ||
| expect(maps.declaredToolNames.has("mcp__x__collaboration.list_agents")).toBe(true); | ||
| }); | ||
|
|
||
| test("a bare name that equals another tool's canonical spelling stays undeclared", () => { | ||
| const parsed = parseRequest({ | ||
| model: "meta/muse-spark-1.3-contributor", | ||
| input: [ | ||
| { type: "additional_tools", role: "developer", tools: [ | ||
| { type: "namespace", name: "collaboration", tools: [ | ||
| { type: "function", name: "list_agents", description: "a", strict: false, parameters: { type: "object", properties: {}, required: [] } }, | ||
| ] }, | ||
| { type: "namespace", name: "mcp__x", tools: [ | ||
| { type: "function", name: "collaboration__list_agents", description: "b", strict: false, parameters: { type: "object", properties: {}, required: [] } }, | ||
| ] }, | ||
| ] }, | ||
| { type: "message", role: "user", content: [{ type: "input_text", text: "run it" }] }, | ||
| ], | ||
| } as any); | ||
| const maps = buildToolBridgeMaps(parsed as any); | ||
| // Tool B's bare name ("collaboration__list_agents") is also tool A's declared canonical | ||
| // spelling, so the bare alias is poisoned. The canonical spelling stays declared — but as | ||
| // tool A's wire name, never as an alias of tool B — so assert the identity via toolNsMap. | ||
| // Tool B's canonical and dotted spellings remain declared. | ||
| expect(maps.toolNsMap.get("collaboration__list_agents")).toEqual({ namespace: "collaboration", name: "list_agents" }); | ||
| expect(maps.declaredToolNames.has("mcp__x__collaboration__list_agents")).toBe(true); | ||
| expect(maps.declaredToolNames.has("mcp__x.collaboration__list_agents")).toBe(true); | ||
| expect(maps.toolNsMap.get("mcp__x.collaboration__list_agents")).toEqual({ namespace: "mcp__x", name: "collaboration__list_agents" }); | ||
| }); | ||
|
|
||
| test("a bare-declared function owns its name and blocks the namespaced tool's bare alias", () => { | ||
| const parsed = parseRequest({ | ||
| model: "meta/muse-spark-1.3-contributor", | ||
| input: [ | ||
| { type: "additional_tools", role: "developer", tools: [ | ||
| { type: "namespace", name: "collaboration", tools: [ | ||
| { type: "function", name: "list_agents", description: "a", strict: false, parameters: { type: "object", properties: {}, required: [] } }, | ||
| ] }, | ||
| { type: "function", name: "list_agents", description: "b", strict: false, parameters: { type: "object", properties: {}, required: [] } }, | ||
| ] }, | ||
| { type: "message", role: "user", content: [{ type: "input_text", text: "run it" }] }, | ||
| ], | ||
| } as any); | ||
| const maps = buildToolBridgeMaps(parsed as any); | ||
| // The bare-declared (no-namespace) function participates as an owner of "list_agents", | ||
| // mirroring the tool_choice bare path's whole-catalog counting, so the namespaced tool | ||
| // must not gain it as an echo alias. "list_agents" stays in declaredToolNames because the | ||
| // bare function's own wire name IS that spelling; the alias check is toolNsMap, which | ||
| // must never map the bare name to the namespaced identity. | ||
| expect(maps.toolNsMap.has("list_agents")).toBe(false); | ||
| expect(maps.declaredToolNames.has("collaboration__list_agents")).toBe(true); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes shared Responses authorization and restoration semantics by accepting another provider-emitted tool spelling, but the commit does not update the owning structure documentation. Record the bare-alias collision and helper-exclusion invariant in
structure/transports/responses.mdso the architecture contract stays synchronized with the implementation.AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.