Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
11 changes: 7 additions & 4 deletions src/adapters/openai-responses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

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 Avoid spreading the unbounded stripped-index set

When a request contains sufficiently many matching additional_tools containers, Math.min(...strippedAdditionalToolsIndices) passes every index as a function argument and eventually throws RangeError: Maximum call stack size exceeded before the request is dispatched. This is reachable within the Responses endpoint's 256 MiB admitted-body limit; track the first stripped index during the existing scan or obtain it through iteration instead of spreading an unbounded set.

Useful? React with 👍 / 👎.

input = input.map((item, index) => index === firstStripped
&& isPlainObject(item)
&& Array.isArray(item.tools)
? { ...item, tools: [...item.tools, { type: HOSTED_IMAGE_GENERATION_TOOL }] }
Expand Down
32 changes: 19 additions & 13 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
21 changes: 21 additions & 0 deletions tests/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
18 changes: 12 additions & 6 deletions tests/openai-responses-passthrough.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"] },
Expand Down Expand Up @@ -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", () => {
Expand Down
Loading