diff --git a/src/adapters/openai-responses.ts b/src/adapters/openai-responses.ts index bfb9dcce1..a650eba72 100644 --- a/src/adapters/openai-responses.ts +++ b/src/adapters/openai-responses.ts @@ -708,10 +708,13 @@ function preferConfiguredHostedTools( if (strippedTopLevelImageGenTool && Array.isArray(tools)) { tools = [...tools, { type: HOSTED_IMAGE_GENERATION_TOOL }]; } else if (strippedAdditionalToolsIndices.size > 0 && Array.isArray(input)) { - // Restore in EVERY container we stripped, not just the first: a request carrying - // two `additional_tools` groups would otherwise leave the later ones with no image - // capability at all. Raised by the automated review on #924. - input = input.map((item, index) => strippedAdditionalToolsIndices.has(index) + // Restore into the FIRST stripped container only. Tool declarations are + // request-scoped, not container-scoped — the containers are separate carriers for + // one tool set, so a single hosted declaration covers the request. An earlier + // revision restored into every stripped container and put `image_generation` on + // the wire twice; review caught it. + const firstStripped = Math.min(...strippedAdditionalToolsIndices); + input = input.map((item, index) => index === firstStripped && isPlainObject(item) && Array.isArray(item.tools) ? { ...item, tools: [...item.tools, { type: HOSTED_IMAGE_GENERATION_TOOL }] } diff --git a/src/config.ts b/src/config.ts index 005b44ae3..fe523323a 100644 --- a/src/config.ts +++ b/src/config.ts @@ -724,7 +724,22 @@ export function modelPreferHostedToolsConfigError( if (prototype !== Object.prototype && prototype !== null) return `${field} must be a plain object with own properties`; const entries = Object.entries(value); const registry = getProviderRegistryEntry(providerName); - if (entries.length > 0 && (registry?.authKind === "forward" || (!registry && provider.authMode === "forward"))) { + // Effective transport: a `preserveCustomDestination` registry row reused under a + // different endpoint keeps its own adapter AND its own auth at runtime, because + // `routedProviderConfig()` honors `providerMatchesRegistryTransport()`. Both the + // wire check below and the forward-auth check here have to start from the same + // decision, or validation accepts a preference the adapter never applies — + // `preferConfiguredHostedTools()` runs only on the non-forward branch. + const registryTransportMatches = typeof provider.baseUrl === "string" + && providerMatchesRegistryTransport(providerName, { + baseUrl: provider.baseUrl, + adapter: provider.adapter as OcxProviderConfig["adapter"], + ...(typeof provider.authMode === "string" ? { authMode: provider.authMode as OcxProviderConfig["authMode"] } : {}), + }); + const effectiveForwardAuth = registryTransportMatches + ? registry?.authKind === "forward" + : provider.authMode === "forward"; + if (entries.length > 0 && effectiveForwardAuth) { return `${field} is not supported on forward-auth Responses providers`; } const requestedWireFor = (modelId: string): unknown => provider.modelAdapters @@ -773,18 +788,9 @@ export function modelPreferHostedToolsConfigError( return `${field}.${key} cannot prefer ${tool}: the model does not support it`; } } - // Start from the registry adapter only when this config still points at the registry's - // documented transport. A `preserveCustomDestination` row reused under a different - // endpoint keeps its own adapter at runtime (`routedProviderConfig()` honors - // `providerMatchesRegistryTransport()`), so trusting `registry.adapter` there would - // accept a preference the Responses adapter never sees. Raised by the automated - // review on #924. - const registryTransportMatches = typeof provider.baseUrl === "string" - && providerMatchesRegistryTransport(providerName, { - baseUrl: provider.baseUrl, - adapter: provider.adapter as OcxProviderConfig["adapter"], - ...(typeof provider.authMode === "string" ? { authMode: provider.authMode as OcxProviderConfig["authMode"] } : {}), - }); + // Same `registryTransportMatches` decision the forward-auth check above uses: + // start from the registry adapter only when this config still points at the + // registry's documented transport. const baseWire = registryTransportMatches ? registry?.adapter ?? provider.adapter : provider.adapter; let effectiveWire = resolveEffectiveWire(key, baseWire); const virtualWireModel = resolveOpenAiVirtualModel(providerName, key)?.wireModelId; diff --git a/tests/config.test.ts b/tests/config.test.ts index e687d3b9d..e5855aa49 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1070,6 +1070,27 @@ describe("opencodex config defaults", () => { expect(readConfigDiagnostics().source).toBe("fallback"); expect(readConfigDiagnostics().error).toContain("requires the openai-responses wire"); + // The forward-auth half of the same effective-transport question. A + // `preserveCustomDestination` registry row reused under a different endpoint keeps + // its OWN auth at runtime, not the registry's, because `routedProviderConfig()` + // honors `providerMatchesRegistryTransport()`. Deciding forward-auth from + // `registry.authKind` alone accepted a preference the adapter never applies: + // `preferConfiguredHostedTools()` runs only on the non-forward branch. + writeConfig({ + port: 12345, + providers: { + "volcengine-agent-plan": { + adapter: "openai-responses", + authMode: "forward", + baseUrl: "https://custom.example.test/v1", + modelPreferHostedTools: { "some-model": ["image_generation"] }, + }, + }, + defaultProvider: "volcengine-agent-plan", + }); + expect(readConfigDiagnostics().source).toBe("fallback"); + expect(readConfigDiagnostics().error).toContain("not supported on forward-auth"); + // Registry providers route through their registry wire, not this persisted adapter. writeConfig({ port: 12345, diff --git a/tests/openai-responses-passthrough.test.ts b/tests/openai-responses-passthrough.test.ts index 9b7040c0a..34b281842 100644 --- a/tests/openai-responses-passthrough.test.ts +++ b/tests/openai-responses-passthrough.test.ts @@ -1147,9 +1147,12 @@ describe("OpenAI Responses hosted-tool name conflicts", () => { } }); - test("every stripped additional_tools container gets hosted image generation restored", () => { - // Stripping runs over all containers, but restoration originally targeted only the - // first stripped index, so a second container lost its image capability entirely. + test("multi-container stripping restores hosted image generation exactly once", () => { + // Stripping runs over every container. Restoration must not: tool declarations are + // request-scoped, and `hasHostedImageGenDeclaration` treats a declaration in any + // container as covering the request. #924 briefly restored into each stripped + // container and put `image_generation` on the wire twice; this asserts against both + // that and the original defect of losing the capability entirely. const adapter = createResponsesPassthroughAdapter({ ...keyedProvider, modelPreferHostedTools: { "provider-image-model": ["image_generation"] }, @@ -1181,9 +1184,12 @@ describe("OpenAI Responses hosted-tool name conflicts", () => { const containers = body.input.filter(item => item.type === "additional_tools"); expect(containers).toHaveLength(2); - for (const container of containers) { - expect(container.tools).toContainEqual({ type: "image_generation" }); - } + const hostedDeclarations = containers.flatMap(container => + (container.tools ?? []).filter(tool => tool.type === "image_generation")); + // Exactly one hosted declaration on the wire, riding the first stripped container + // so the capability is neither lost nor duplicated. + expect(hostedDeclarations).toEqual([{ type: "image_generation" }]); + expect(containers[0].tools).toContainEqual({ type: "image_generation" }); }); test("configured model rewrites a custom image-gen selector", () => {