Repository navigation
fix(responses): accept provider-added default namespace prefix for declared bare tools #4272
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 2 commits
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 |
|---|---|---|
|
|
@@ -430,6 +430,8 @@ import { | |
| createUndeclaredToolCallGuardBlockRewrite, | ||
| currentTurnWireToolCatalogBody, | ||
| hasExplicitWireToolCatalog, | ||
| createDefaultNamespacePrefixStripRewrite, | ||
| normalizeDefaultNamespacePrefixInJson, | ||
| undeclaredToolCallMessage, | ||
| undeclaredToolCallName, | ||
| undeclaredToolCallNameInResponse, | ||
|
|
@@ -5938,6 +5940,10 @@ async function handleResponsesInner( | |
| : undefined; | ||
| // Compose opt-in payload rewrites into one parse/stringify pass (image-gen restore first). | ||
| const payloadRewrites = [ | ||
| // Fold a provider-added default namespace ('default.view_image' for a | ||
| // declared bare 'view_image') back to bare before the undeclared-tool | ||
| // guard compares names the client will actually receive. | ||
| createDefaultNamespacePrefixStripRewrite(new Set(declaredWireToolNames)), | ||
|
Contributor
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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Normalize the response before persisting continuation state. At 🤖 Prompt for AI Agents |
||
| createImageGenCallRestoreRewrite(imageGenCallAliases), | ||
| // #3217: a call whose namespace repeats its own name is unroutable in codex-rs. | ||
| createSelfNamedToolCallNamespaceScrubRewrite(selfNamedNamespaceScrubAuthorization), | ||
|
|
@@ -6220,7 +6226,11 @@ async function handleResponsesInner( | |
| restored, | ||
| routedToolSearchNames, | ||
| ); | ||
| const repaired = normalizeFunctionCompletionJson(restoredToolSearch); | ||
| const restoredDefaultPrefix = normalizeDefaultNamespacePrefixInJson( | ||
| restoredToolSearch, | ||
| declaredWireToolNames, | ||
| ); | ||
| const repaired = normalizeFunctionCompletionJson(restoredDefaultPrefix); | ||
| const modelRewritten = parsed._responseModelId !== undefined && parsed._responseModelId !== parsed.modelId | ||
| ? rewriteResponsesModelJson(repaired, parsed._responseModelId) | ||
| : repaired; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,10 +12,13 @@ import { | |
| createUndeclaredToolCallGuardBlockRewrite, | ||
| currentTurnWireToolCatalogBody, | ||
| hasExplicitWireToolCatalog, | ||
| normalizeDefaultNamespacePrefixInJson, | ||
| undeclaredToolCallName, | ||
| undeclaredToolCallNameInResponse, | ||
| UNDECLARED_TOOL_CALL_ERROR_CODE, | ||
| type ProviderExecutedCallType, | ||
| } from "../../src/server/responses-undeclared-tool-guard"; | ||
| import { normalizeDeclaredToolName } from "../../src/types/tools"; | ||
| import { relaySseWithBlockRewrite } from "../../src/server/sse-payload-rewrite"; | ||
| import { handleResponses } from "../../src/server/responses"; | ||
| import { expandPreviousResponseInput } from "../../src/responses/state"; | ||
|
|
@@ -1840,3 +1843,100 @@ describe("xAI hosted-call authorization through handleResponses", () => { | |
| expect(body.error.message).toContain('undeclared client tool "x_keyword_search"'); | ||
| }); | ||
| }); | ||
|
|
||
| describe("provider-added default namespace prefix", () => { | ||
| const viewImageCatalog = { | ||
| tools: [{ type: "function", name: "view_image" }], | ||
| }; | ||
|
|
||
| function declaredBareViewImage(): Set<string> { | ||
| return collectDeclaredWireToolNames(viewImageCatalog); | ||
| } | ||
|
|
||
| test("dotted default prefix resolves to the declared bare tool", () => { | ||
| const declared = declaredBareViewImage(); | ||
| expect(declared.has("view_image")).toBe(true); | ||
| expect( | ||
| undeclaredToolCallName( | ||
| { | ||
| type: "response.output_item.added", | ||
| item: { type: "function_call", name: "default.view_image", call_id: "call_1" }, | ||
| }, | ||
| declared, | ||
| ), | ||
| ).toBeUndefined(); | ||
| expect(normalizeDeclaredToolName("default.view_image", declared)).toBe("view_image"); | ||
| expect(normalizeDeclaredToolName("default__view_image", declared)).toBe("view_image"); | ||
| }); | ||
|
|
||
| test("namespaced default shape resolves to the declared bare tool", () => { | ||
| const declared = declaredBareViewImage(); | ||
| expect( | ||
| undeclaredToolCallName( | ||
| { | ||
| type: "response.output_item.added", | ||
| item: { type: "function_call", name: "view_image", namespace: "default", call_id: "call_2" }, | ||
| }, | ||
| declared, | ||
| ), | ||
| ).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("genuinely undeclared default-prefixed tools still fail closed", () => { | ||
| const declared = declaredBareViewImage(); | ||
| expect( | ||
| undeclaredToolCallName( | ||
| { | ||
| type: "response.output_item.added", | ||
| item: { type: "function_call", name: "default.apply_patch", call_id: "call_3" }, | ||
| }, | ||
| declared, | ||
| ), | ||
| ).toBe("default.apply_patch"); | ||
| expect( | ||
| normalizeDefaultNamespacePrefixInJson(JSON.stringify({ name: "default.apply_patch" }), declared), | ||
| ).toBe(JSON.stringify({ name: "default.apply_patch" })); | ||
| }); | ||
|
|
||
| test("no fold while the default namespace is genuinely declared", () => { | ||
| const declared = collectDeclaredWireToolNames({ | ||
| tools: [{ type: "namespace", name: "default", tools: [{ type: "function", name: "view_image" }] }], | ||
| }); | ||
| expect(declared.has("default__view_image")).toBe(true); | ||
| expect(normalizeDeclaredToolName("default.other", declared)).toBe("default.other"); | ||
| }); | ||
|
|
||
| test("payload rewrite restores the bare name before relay", () => { | ||
| const declared = declaredBareViewImage(); | ||
| const rewritten = normalizeDefaultNamespacePrefixInJson( | ||
| JSON.stringify({ | ||
| type: "response.output_item.added", | ||
| item: { type: "function_call", name: "default.view_image", call_id: "call_1" }, | ||
| }), | ||
| declared, | ||
| ); | ||
| const parsed = JSON.parse(rewritten) as { item: { name: string } }; | ||
| expect(parsed.item.name).toBe("view_image"); | ||
| }); | ||
|
|
||
| test("guard block rewrite relays the folded call without failing", () => { | ||
| const declared = declaredBareViewImage(); | ||
| const guard = createUndeclaredToolCallGuardBlockRewrite(declared); | ||
| const blocks = guard( | ||
| frame("response.output_item.added", { | ||
| output_index: 0, | ||
| item: { | ||
| type: "function_call", | ||
| id: "fc_1", | ||
| call_id: "call_1", | ||
| name: "default.view_image", | ||
| arguments: "{}", | ||
| status: "in_progress", | ||
| }, | ||
| }), | ||
| ); | ||
| expect(blocks).toHaveLength(1); | ||
| expect(blocks[0]).toContain("response.output_item.added"); | ||
| expect(blocks[0]).not.toContain("response.failed"); | ||
| }); | ||
| }); | ||
|
Comment on lines
+1847
to
+1944
Contributor
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. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add focused coverage for the
🤖 Prompt for AI Agents |
||
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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse valid escaped JSON before deciding that normalization is unnecessary.
Line 472 checks serialized bytes. A valid payload can encode
default.view_imageas"\u0064efault.view_image"or encode thenamespacevalue the same way. This branch then returns the original payload.The undeclared-tool guard later parses that payload and accepts the declared bare tool. The client still receives the prefixed tool name, so it cannot match the tool it declared.
Remove the lexical prefix check. Return early only when
declaredis empty.Proposed fix
As per coding guidelines, adapter changes must preserve the internal event contract and tool-call behavior.
📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Coding guidelines