fix: preserve hosted image tool preferences (#837 rebased, two defects fixed) - #924
Conversation
… hosted-tool preferences Two defects the plan audit found in this change, both reproduced before fixing. Validation disagreed with routing. `modelPreferHostedTools` resolved the effective wire from `registry.adapter` and an explicit `modelAdapters` entry, then stopped. At request time `resolveModelAdapter()` consults the registry's per-model `modelWireDefaults` before falling back to the provider-wide adapter. DeepSeek routes `deepseek-v4-flash` over native Responses for a Responses inbound while its provider-wide wire stays openai-chat, so a valid preference for that model was rejected at config load with "requires the openai-responses wire" — a config the runtime would have honored. The validator now walks the same order. Inherited keys threw before dispatch. `provider.modelPreferHostedTools?. [modelId]` walks the prototype chain, so a routed model id of `constructor` or `toString` resolved to a function and threw `TypeError: ... .includes is not a function` inside `preferConfiguredHostedTools`, failing the request. Lookup is now own-property only and requires an array. Both regressions were driven red first: the DeepSeek case produced the exact rejection above, and the inherited-key case produced the exact TypeError. 247 tests pass across the four affected files, typecheck clean.
📝 WalkthroughWalkthroughThe PR adds exact-model hosted Responses tool preferences, validates effective provider routing, preserves virtual model IDs, normalizes image-generation tools, updates documentation, and adds regression tests. It also adds planning documents for issue-disposition and recovery work. ChangesHosted tool preferences
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderConfig
participant ConfigValidator
participant VirtualModelResolver
participant OpenAIResponsesAdapter
participant ResponsesGateway
ProviderConfig->>ConfigValidator: Validate modelPreferHostedTools
ConfigValidator->>VirtualModelResolver: Resolve effective model and wire
VirtualModelResolver-->>ConfigValidator: Return selected and base model IDs
ConfigValidator-->>OpenAIResponsesAdapter: Accept eligible configuration
OpenAIResponsesAdapter->>OpenAIResponsesAdapter: Normalize image tools and tool_choice
OpenAIResponsesAdapter->>ResponsesGateway: Send normalized Responses request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f8b1d64e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return `${field}.${key} cannot prefer ${tool}: the model does not support it`; | ||
| } | ||
| } | ||
| let effectiveWire = resolveEffectiveWire(key, registry?.adapter ?? provider.adapter); |
There was a problem hiding this comment.
When a provider with preserveCustomDestination reuses a registry ID (for example volcengine-agent-plan) but changes its endpoint or adapter, routedProviderConfig() deliberately preserves the configured transport because providerMatchesRegistryTransport() returns false. This validator nevertheless starts from registry.adapter, so an openai-chat custom destination can pass validation with modelPreferHostedTools; at runtime the Responses adapter is never selected and the preference is silently ignored. Use the registry adapter only when the configured provider matches that registry transport, otherwise start from provider.adapter.
Useful? React with 👍 / 👎.
…ults case The fix for registry wire defaults has a mirror the first regression did not reach. `volcengine-agent-plan` is a Responses registry row carrying `preserveCustomDestination`, so a config that reuses the id while pointing at a different endpoint keeps its own transport: `providerMatchesRegistryTransport()` returns false and `routedProviderConfig()` preserves the configured `openai-chat` adapter. Validating from `registry.adapter` unconditionally would accept a hosted-tool preference the Responses adapter never sees. Driven red against the pre-fix `src/config.ts` to confirm it is not vacuous.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/openai-responses.ts`:
- Around line 680-717: Update the additional_tools handling around the input
mapping and restoration logic to track every container whose tools were changed
by stripGroup, rather than only strippedAdditionalToolsIndex. When no hosted
image-generation declaration remains, restore the hosted tool in each stripped
additional_tools container, while preserving unchanged containers and existing
top-level behavior. Add a regression test in
openai-responses-passthrough.test.ts covering two stripped additional_tools
containers.
In `@src/config.ts`:
- Around line 731-759: Extract the shared pinned-wire, modelAdapters override,
registry-default, and fallback precedence from resolveEffectiveWire and
resolveWireProtocolOverride into a pure selectEffectiveWire helper, using
providerName, modelId, currentWire, modelAdapters, allowedWires, and inbound as
needed. Update both callers to use this helper, preserving the existing
“responses” inbound behavior and eliminating duplicated resolution logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92f08eb0-b014-4654-9da6-5c70701d373c
📒 Files selected for processing (16)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdsrc/adapters/openai-responses.tssrc/config.tssrc/providers/openai-virtual-models.tssrc/responses/hosted-tool-policy.tssrc/server/auth-cors.tssrc/types.tsstructure/04_transports-and-sidecars.mdtests/config.test.tstests/management-provider-validation.test.tstests/openai-api-virtual-models.test.tstests/openai-responses-passthrough.test.ts
| const resolveEffectiveWire = (modelId: string, currentWire: unknown): unknown => { | ||
| const pinned = pinnedWireAdapter(providerName, modelId); | ||
| if (pinned) return pinned; | ||
| const requestedWire = requestedWireFor(modelId); | ||
| if (typeof requestedWire === "string" && MODEL_ADAPTER_OVERRIDE_ALLOWED.has(requestedWire)) { | ||
| return requestedWire; | ||
| } | ||
| // No explicit override: fall back to the registry's per-model wire default before | ||
| // the provider-wide adapter, because that is the order `resolveModelAdapter()` | ||
| // uses at request time (src/server/adapter-resolve.ts:38-48). Skipping it rejected | ||
| // preferences the runtime would have honored — DeepSeek routes `deepseek-v4-flash` | ||
| // over native Responses for a Responses inbound while the provider-wide wire stays | ||
| // openai-chat. Hosted-tool preferences only apply to Responses traffic, so the | ||
| // inbound to ask about is "responses". | ||
| const registryDefault = typeof currentWire === "string" && typeof provider.baseUrl === "string" | ||
| ? providerModelWireDefault( | ||
| providerName, | ||
| { | ||
| baseUrl: provider.baseUrl, | ||
| adapter: currentWire, | ||
| ...(typeof provider.authMode === "string" ? { authMode: provider.authMode as OcxProviderConfig["authMode"] } : {}), | ||
| }, | ||
| modelId, | ||
| MODEL_ADAPTER_OVERRIDE_ALLOWED, | ||
| "responses", | ||
| ) | ||
| : undefined; | ||
| return registryDefault ?? currentWire; | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Extract the shared wire-resolution algorithm to avoid drift between validation and runtime.
resolveEffectiveWire re-implements, step by step, the same pinned-wire → modelAdapters override → registry-default → fallback order as resolveWireProtocolOverride in src/server/adapter-resolve.ts:27-55. The two implementations currently agree, as confirmed by the DeepSeek and gpt-5.6-sol-pro test cases, but they are two independently maintained copies of one selection algorithm.
The PR itself documents that a prior version of this validator drifted from the runtime order (the comment at Line 738-745 explains the fix). Duplicating the algorithm means the same class of regression can reappear the next time resolveWireProtocolOverride gains a new precedence rule (e.g., a new pinned-wire case or override condition) that is not mirrored here.
Extract a small pure function, for example selectEffectiveWire(providerName, modelId, currentWire, modelAdapters, allowedWires, inbound), that both resolveWireProtocolOverride and this validator call, so the precedence order has one source of truth.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config.ts` around lines 731 - 759, Extract the shared pinned-wire,
modelAdapters override, registry-default, and fallback precedence from
resolveEffectiveWire and resolveWireProtocolOverride into a pure
selectEffectiveWire helper, using providerName, modelId, currentWire,
modelAdapters, allowedWires, and inbound as needed. Update both callers to use
this helper, preserving the existing “responses” inbound behavior and
eliminating duplicated resolution logic.
… matches Automated review on #924 found the mirror of the defect the previous commit fixed. That one made validation too strict; this one made it too loose. A `preserveCustomDestination` registry row reused under a different endpoint keeps its own adapter at runtime — `routedProviderConfig()` honors `providerMatchesRegistryTransport()`, which returns false once the endpoint diverges. `volcengine-agent-plan` is such a row with an `openai-responses` registry adapter, so a config naming that id while pointing elsewhere with `adapter: "openai-chat"` passed validation on the registry's adapter while the Responses adapter never ran. The preference was accepted and then silently ignored. Validation now starts from `provider.adapter` unless the configured transport still matches the registry's documented one. Driven red first: the volcengine-agent-plan config loaded with source "file" before the fix and now returns "requires the openai-responses wire". 279 tests pass across the five affected files, typecheck clean.
…ped container Stripping walked all `additional_tools` containers; restoration targeted only the first stripped index. A request carrying two containers, each declaring an empty `image_gen` namespace and no hosted declaration anywhere, ended with the first container repaired and the second left with an empty tool list — no image capability at all. Track every stripped index and restore each one. Found by the automated review on #924 and driven red first: the second container came back as `[]` before the fix. Full suite 7575 pass, 0 fail.
|
Both automated reviews found real defects. Fixed, each driven red first. Codex, This is the mirror of the defect the previous commit fixed — that one made validation too strict, this one too loose. Validation now starts from CodeRabbit, CodeRabbit's second note — that Verification after both fixes: |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260803_pr_issue_sweep/000_plan.md`:
- Around line 42-51: Remove the bearer-token fragment and developer-specific Bun
path from the committed plan’s evidence block, retaining only a high-level
finding. Move the exact reproduction details to the designated private scratch
document, and rotate the credential if the fragment represents a real token.
In `@devlog/_plan/260803_pr_issue_sweep/020_phase2_hosted_tool_duplicate.md`:
- Line 17: Update the paragraph beginning with “#837” in the issue plan so the
hash is not parsed as a Markdown heading, either by prefixing the sentence with
“Issue” or by escaping the hash while preserving the existing issue references.
- Around line 15-25: Update the provenance table entry in the plan so the `#837`
relationship is described as the same substantive diff as `#616` plus one
independent schema-fixture correction, rather than “identical.” Preserve the
existing explanation of the exec_command parameter change and authorship.
In `@devlog/_plan/260803_pr_issue_sweep/030_phase3_compact_alternate.md`:
- Around line 93-101: Update the alternate-account recovery flow so B’s
provider, headers, base URL, and authentication context are fully built and
validated before recording A with promoteAccountId: B. If B construction fails,
record A without promotion and return A; only add promotion after successful B
construction. Extend the construction-failure test to verify B is not promoted.
In `@devlog/_plan/260803_transport_attribution/000_plan.md`:
- Line 113: Update the prose line beginning with “#919” in the plan document to
start with “Issue `#919` spent one round...”, preserving the issue identifier and
remaining sentence content while satisfying Markdown heading-spacing lint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b0a62668-1873-4764-a722-2b29f28e279e
📒 Files selected for processing (11)
devlog/_plan/260803_cooldown_recovery_probe/000_plan.mddevlog/_plan/260803_pr_issue_sweep/000_plan.mddevlog/_plan/260803_pr_issue_sweep/010_phase1_image_forwarding.mddevlog/_plan/260803_pr_issue_sweep/020_phase2_hosted_tool_duplicate.mddevlog/_plan/260803_pr_issue_sweep/030_phase3_compact_alternate.mddevlog/_plan/260803_pr_issue_sweep/040_phase4_backlog_disposition.mddevlog/_plan/260803_pr_issue_sweep/050_phase5_916_disposition.mddevlog/_plan/260803_sparse_snapshot_repair/000_plan.mddevlog/_plan/260803_transport_attribution/000_plan.mdsrc/config.tstests/config.test.ts
| ```text | ||
| Claude: {"baseUrl":"https://attacker.example","token":null} | ||
| Bun: {"path":"/Users/jun/.bun/bin/bun","source":"override",...} | ||
| Health: {"seen":"Bearer ocx_admin_AAAA...","source":"management-api-unavailable"} | ||
| ``` | ||
|
|
||
| An ambient `ANTHROPIC_BASE_URL` survives credential stripping and redirects | ||
| OAuth-bearing traffic; `OPENCODEX_BUN_PATH` is reread after Bun loads project | ||
| dotenv, so a repository-local file can persist the durable executable; and the | ||
| admin token is handed to any listener that answers a forgeable `/healthz`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove credential-like evidence from the committed plan.
Lines 42-51 include a bearer-token fragment in Health.seen and a developer-specific path in Bun.path. This conflicts with devlog/_plan/260803_pr_issue_sweep/050_phase5_916_disposition.md Lines 5-10, which requires reproduction details for unfixed security defects to remain in scratch space.
Keep only a high-level finding in this document. Move the exact evidence to the private scratch location. Rotate the credential if the fragment came from a real token.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260803_pr_issue_sweep/000_plan.md` around lines 42 - 51, Remove
the bearer-token fragment and developer-specific Bun path from the committed
plan’s evidence block, retaining only a high-level finding. Move the exact
reproduction details to the designated private scratch document, and rotate the
credential if the fragment represents a real token.
| | Diff | +819/−18, 16 files | identical | | ||
|
|
||
| #837 replayed #616's commit onto a newer base, preserving authorship, and adds | ||
| one independent change: | ||
|
|
||
| ```diff | ||
| - { type: "function", name: "exec_command", parameters: {} }, | ||
| + { type: "function", name: "exec_command", parameters: { type: "object" } }, | ||
| ``` | ||
|
|
||
| That is a fixture correction for schema normalization present only on the newer |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the provenance table match the fixture correction.
Line 15 calls the #837 diff identical, but Lines 17-25 state that #837 adds an independent schema-fixture change. Record the relationship as the same substantive diff plus one fixture correction.
Proposed wording
-| Diff | +819/−18, 16 files | identical |
+| Diff | +819/−18, 16 files | same substantive diff; one fixture correction |🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 17-17: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260803_pr_issue_sweep/020_phase2_hosted_tool_duplicate.md`
around lines 15 - 25, Update the provenance table entry in the plan so the `#837`
relationship is described as the same substantive diff as `#616` plus one
independent schema-fixture correction, rather than “identical.” Preserve the
existing explanation of the exec_command parameter change and authorship.
| | Committer | Eleven-is-cool | Ingwannu | | ||
| | Diff | +819/−18, 16 files | identical | | ||
|
|
||
| #837 replayed #616's commit onto a newer base, preserving authorship, and adds |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the issue reference at the start of the paragraph.
Line 17 starts with #837 without a following space. Markdownlint reports MD018. Prefix the sentence with Issue or escape the hash.
Proposed wording
-#837 replayed `#616`'s commit onto a newer base, preserving authorship, and adds
+Issue `#837` replayed `#616`'s commit onto a newer base, preserving authorship, and adds📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #837 replayed #616's commit onto a newer base, preserving authorship, and adds | |
| Issue `#837` replayed `#616`'s commit onto a newer base, preserving authorship, and adds |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 17-17: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260803_pr_issue_sweep/020_phase2_hosted_tool_duplicate.md` at
line 17, Update the paragraph beginning with “#837” in the issue plan so the
hash is not parsed as a Markdown heading, either by prefixing the sentence with
“Issue” or by escaping the hash while preserving the existing issue references.
Source: Linters/SAST tools
| 4. Alternate B exists: | ||
| - apply A's quota headers to its quota cache; | ||
| - record A's actual rejection with retry/reset metadata, scope, writer | ||
| generation, and `promoteAccountId: B`; | ||
| - **build B's provider, headers, base URL, and auth context completely | ||
| first**, then cancel A's body. If B's construction throws, A's body is | ||
| still intact and its rejection can be returned to the client. Cancelling | ||
| first would leave nothing to fall back to. | ||
| - send B with `recovery: "single"` — one network send, no transient ladder, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Build the alternate before promoting it.
The plan records A with promoteAccountId: B before B's provider, headers, base URL, and authentication context are fully constructed. If B construction throws, the request returns A but the pool state can still promote B. A later request can then select an account that failed construction.
Build and validate B first. If construction fails, record A without promoteAccountId and return A. Add promotion only after B construction succeeds. Extend the construction-failure test to assert that B was not promoted.
Also applies to: 148-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260803_pr_issue_sweep/030_phase3_compact_alternate.md` around
lines 93 - 101, Update the alternate-account recovery flow so B’s provider,
headers, base URL, and authentication context are fully built and validated
before recording A with promoteAccountId: B. If B construction fails, record A
without promotion and return A; only add promotion after successful B
construction. Extend the construction-failure test to verify B is not promoted.
|
|
||
| ## #919 — the post-200 half, and why it is not a quick fix either | ||
|
|
||
| #919 spent one round in the sweep unit as "the easy one": the synthetic/real |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep #919 as prose.
Line 113 begins with #919 without a space. markdownlint-cli2 reports MD018. Rewrite it as Issue #919 spent one round... so the issue identifier remains prose and the document passes Markdown lint.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 113-113: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260803_transport_attribution/000_plan.md` at line 113, Update
the prose line beginning with “#919” in the plan document to start with “Issue
`#919` spent one round...”, preserving the issue identifier and remaining sentence
content while satisfying Markdown heading-spacing lint.
Source: Linters/SAST tools
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/openai-responses.ts (1)
707-718: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore nested tools when top-level tools were also stripped.
When a request contains a client image tool in both
toolsandadditional_tools, Line 708 restores the top-level hosted declaration and Line 710 skips every nested container because ofelse if.Use two independent
ifblocks. Add a regression test with a stripped top-level group and at least one strippedadditional_toolsgroup.Proposed fix
- } else if (strippedAdditionalToolsIndices.size > 0 && Array.isArray(input)) { + } + if (strippedAdditionalToolsIndices.size > 0 && Array.isArray(input)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/adapters/openai-responses.ts` around lines 707 - 718, Update the restoration logic around strippedTopLevelImageGenTool and strippedAdditionalToolsIndices to use independent if blocks, so top-level tools and every stripped additional_tools container are restored when both were present. Add a regression test covering a request with stripped top-level and nested image tools, asserting both hosted declarations are restored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/adapters/openai-responses.ts`:
- Around line 707-718: Update the restoration logic around
strippedTopLevelImageGenTool and strippedAdditionalToolsIndices to use
independent if blocks, so top-level tools and every stripped additional_tools
container are restored when both were present. Add a regression test covering a
request with stripped top-level and nested image tools, asserting both hosted
declarations are restored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab1507cc-981a-4279-b4fa-158b39745c15
📒 Files selected for processing (2)
src/adapters/openai-responses.tstests/openai-responses-passthrough.test.ts
fix(hosted-tools): one effective-transport decision, one hosted declaration Follow-up to #924. Forward-auth validation now shares the same providerMatchesRegistryTransport decision as the wire check, and hosted-tool restoration targets the first stripped container only instead of duplicating image_generation once per container. Both driven red against dev; 7575 pass, typecheck and privacy scan clean.
…er validation Two corrections to what lidge-jun#924 landed. **The multi-container restoration was wrong.** lidge-jun#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.** lidge-jun#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.
…ration Follow-up to lidge-jun#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. lidge-jun#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.
3 participants
Rebases #837 onto current
devand fixes the two defects a plan audit found in it. Supersedes both #837 and #616; the substantive commit keeps @Eleven-is-cool as author.Provenance
#616 and #837 are the same implementation, not two competing ones. Git authorship shows it:
1aba0e4b89d51dbc@Ingwannu replayed @Eleven-is-cool's commit onto a newer base, preserving authorship, and added a fixture correction that only applies on that base. This branch does the same thing once more onto
fa51fce54, so the credit chain is intact:git logstill shows @Eleven-is-cool as author of the implementation.What it does
Non-forward Responses gateways that reserve
image_genserver-side reject even the empty client namespacenormalizeImageGenClientTools()preserves, so a user has no way to say "use the hosted tool for this model." This adds an exact-model opt-in:It strips colliding client
image_gendeclarations fromtoolsand nestedadditional_tools, rewrites forced selectors to{ type: "image_generation" }, and restores hosted image generation if stripping removed the only image declaration. The Spark exclusion table moves out of the adapter intosrc/responses/hosted-tool-policy.tsso config validation can consult it too.Two defects fixed on top
Both were reproduced red before fixing.
Validation disagreed with routing (
src/config.ts). The validator resolved the effective wire fromregistry.adapterplus an explicitmodelAdaptersentry, then stopped. At request timeresolveModelAdapter()consults the registry's per-modelmodelWireDefaultsfirst. DeepSeek routesdeepseek-v4-flashover native Responses for a Responses inbound while its provider-wide wire staysopenai-chat, so a valid preference for that model was rejected at config load withrequires the openai-responses wire. The validator now walks the same order the runtime does.Inherited keys threw before dispatch (
src/adapters/openai-responses.ts).provider.modelPreferHostedTools?.[modelId]walks the prototype chain, so a routed model id ofconstructorortoStringresolved to a function and threwTypeError: ... .includes is not a function, failing the request before it reached upstream. Lookup is own-property only and array-checked now.Rebase
Six conflicts, all from
devsplittingconfiguration.mdintoconfiguration/providers.mdafter this branch was written. The doc row was placed in its new home in all five locales;src/config.tswas a parallel import addition.Verification
bun x tsc --noEmit— exit 0bun run test— 7574 pass, 8 skip, 0 fail across 504 filesbun run privacy:scan— passedconfig,management-provider-validation,openai-api-virtual-models,openai-responses-passthroughDisposition
Closes #837 and #616 once merged. Thanks @Eleven-is-cool for the implementation and @Ingwannu for the first integration.
Summary by CodeRabbit
modelPreferHostedToolsconfiguration to enable hostedimage_generationfor specific compatible models.