From 3e889a3565284a83dea1b0102b3b36be8b1fcc52 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 14:37:40 +0900 Subject: [PATCH 1/2] fix: one hosted declaration per request, and one transport decision per validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections to what #924 landed. **The multi-container restoration was wrong.** #924 took CodeRabbit's finding that restoration reached only the first stripped `additional_tools` container and fixed it by restoring into every stripped container. That overcorrects: `hasHostedImageGenDeclaration` a few lines above is satisfied by a declaration in ANY container, so the code already treats tool declarations as request-scoped. Restoring into each container puts `image_generation` on the wire twice. Restore into the first stripped container only — the capability is neither lost nor duplicated, and the test now asserts exactly one declaration rather than "at least one per container". **The forward-auth check disagreed with the wire check.** #924 taught the wire check that a `preserveCustomDestination` row reused under a different endpoint keeps its own adapter. The forward-auth check three lines earlier still read `registry.authKind`. So a config naming such a row with `authMode: "forward"` and a custom endpoint passed the auth check on the registry's value, while at runtime `preferConfiguredHostedTools()` never runs — it is on the non-forward branch. Both checks now share one `registryTransportMatches` decision. Driven red: the forward-auth config loaded clean before the fix. 245 tests pass across the four affected files, typecheck clean. --- src/adapters/openai-responses.ts | 11 +++++--- src/config.ts | 32 +++++++++++++--------- tests/config.test.ts | 21 ++++++++++++++ tests/openai-responses-passthrough.test.ts | 18 ++++++++---- 4 files changed, 59 insertions(+), 23 deletions(-) diff --git a/src/adapters/openai-responses.ts b/src/adapters/openai-responses.ts index bfb9dcce1..682f5a3a2 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: `hasHostedImageGenDeclaration` above is + // satisfied by a declaration in ANY container, so the containers are separate + // carriers for one tool set. #924 briefly restored into every stripped container, + // which put `image_generation` on the wire twice. + const firstStrippedContainer = Math.min(...strippedAdditionalToolsIndices); + input = input.map((item, index) => index === firstStrippedContainer && 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..a83dca60b 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, decided once and used by both checks below. A + // `preserveCustomDestination` registry row reused under a different endpoint keeps its + // own adapter AND its own auth at runtime, 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. + 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", () => { From cf5910ab10e0508dc5d5cf0be3d37102352f3800 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 14:38:31 +0900 Subject: [PATCH 2/2] fix(hosted-tools): one effective-transport decision, one hosted declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #924. Two defects a branch review found after that PR merged, both reproduced before fixing. Forward-auth validation diverged from routing. The wire check already asked `providerMatchesRegistryTransport()` whether the config still points at the registry's documented endpoint, but the forward-auth check above it read `registry.authKind` unconditionally. A `preserveCustomDestination` row reused under a custom endpoint keeps its own auth at runtime, so a forward-auth config carrying `modelPreferHostedTools` validated clean while `preferConfiguredHostedTools()` — which runs only on the non-forward branch — never applied it. Both checks now start from the same decision, computed once instead of twice. Multi-container restoration emitted the hosted tool twice. #924 made stripping walk every `additional_tools` container and made restoration walk them too. Tool declarations are request-scoped: the containers are separate carriers for one tool set, so restoring into each put `image_generation` on the wire once per stripped container. Restore into the first stripped container only, which keeps the capability without duplicating it. Both driven red against dev: reverting the two source files fails exactly two tests, one per defect. 248 pass across the four affected files, typecheck clean. --- src/adapters/openai-responses.ts | 12 ++++++------ src/config.ts | 16 ++++++++-------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/adapters/openai-responses.ts b/src/adapters/openai-responses.ts index 682f5a3a2..a650eba72 100644 --- a/src/adapters/openai-responses.ts +++ b/src/adapters/openai-responses.ts @@ -709,12 +709,12 @@ function preferConfiguredHostedTools( tools = [...tools, { type: HOSTED_IMAGE_GENERATION_TOOL }]; } else if (strippedAdditionalToolsIndices.size > 0 && Array.isArray(input)) { // Restore into the FIRST stripped container only. Tool declarations are - // request-scoped, not container-scoped: `hasHostedImageGenDeclaration` above is - // satisfied by a declaration in ANY container, so the containers are separate - // carriers for one tool set. #924 briefly restored into every stripped container, - // which put `image_generation` on the wire twice. - const firstStrippedContainer = Math.min(...strippedAdditionalToolsIndices); - input = input.map((item, index) => index === firstStrippedContainer + // 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 a83dca60b..fe523323a 100644 --- a/src/config.ts +++ b/src/config.ts @@ -724,11 +724,11 @@ 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); - // Effective transport, decided once and used by both checks below. A - // `preserveCustomDestination` registry row reused under a different endpoint keeps its - // own adapter AND its own auth at runtime, because `routedProviderConfig()` honors - // `providerMatchesRegistryTransport()`. Deciding forward-auth from `registry.authKind` - // alone accepted a preference the adapter never applies — + // 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, { @@ -788,9 +788,9 @@ export function modelPreferHostedToolsConfigError( return `${field}.${key} cannot prefer ${tool}: the model does not support it`; } } - // 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. + // 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;