fix: preserve hosted image tool preferences - #616
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds exact-model ChangesHosted tool preference
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ConfigurationValidator
participant OpenAIResponsesAdapter
participant OpenAIResponsesGateway
Client->>ConfigurationValidator: Configure modelPreferHostedTools
ConfigurationValidator->>ConfigurationValidator: Validate model, hosted tool, auth mode, and wire
Client->>OpenAIResponsesAdapter: Send model, tools, and tool_choice
OpenAIResponsesAdapter->>OpenAIResponsesAdapter: Remove image_gen conflicts and rewrite selectors
OpenAIResponsesAdapter->>OpenAIResponsesGateway: Send normalized image_generation request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ab30dca to
1783957
Compare
|
Please change the PR to ready for review once you are finished @Eleven-is-cool |
1783957 to
e8ed8bf
Compare
![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: e8ed8bf62d
ℹ️ 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".
![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 524-609: The tool-group traversal in preferConfiguredHostedTools
and normalizeImageGenClientTools is duplicated across top-level body.tools and
nested additional_tools entries. Extract a shared mapResponsesToolGroups helper
that applies a supplied mapper to both tool-group locations while preserving
unchanged references and rebuilding the body only when needed, then use it for
the strip and normalize flows without changing their distinct semantics.
In `@src/config.ts`:
- Around line 499-549: Update the forward-auth rejection in
modelPreferHostedToolsConfigError to check the actual provider.authMode
regardless of whether getProviderRegistryEntry(providerName) returns a registry
entry, while preserving the registry authKind fallback for providers without an
explicit forward mode. Ensure any non-empty configuration is rejected whenever
the effective provider mode is forward, matching
createResponsesPassthroughAdapter.buildRequest.
🪄 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: 095da904-3c44-44c0-9b26-f51e26545d3f
📒 Files selected for processing (16)
docs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.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
e8ed8bf to
012ab25
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs-site/src/content/docs/reference/configuration.md`:
- Line 301: Clarify modelPreferHostedTools matching using the routing-resolved
model identity: explain that OpenAI virtual models retain their public selector
while Responses eligibility and exact preference matching resolve through the
underlying wire model, and that modelAdapters overrides are not matched against
the public selector. Apply equivalent clarification in
docs-site/src/content/docs/reference/configuration.md:301-301,
docs-site/src/content/docs/ja/reference/configuration.md:175-175,
docs-site/src/content/docs/ko/reference/configuration.md:184-184,
docs-site/src/content/docs/ru/reference/configuration.md:207-207, and
docs-site/src/content/docs/zh-cn/reference/configuration.md:175-175, translated
appropriately; align structure/04_transports-and-sidecars.md:75-80 with the same
resolved-model and wire behavior.
🪄 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: a47120b1-3ee6-45c0-bb9d-439369508cac
📒 Files selected for processing (16)
docs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.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
012ab25 to
53618c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 558-609: Update preferConfiguredHostedTools so a tool_choice
rewritten to the hosted image-generation tool remains consistent with the
resulting tools and nested input.additional_tools: when client declarations are
stripped, either retain/re-add the hosted tool declaration or avoid rewriting
tool_choice unless a hosted entry remains. Add a regression test covering a
request containing only the client wrapper declaration.
🪄 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: 14d6f631-18ba-4a5d-be18-edc9eb35eba7
📒 Files selected for processing (16)
docs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.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
53618c6 to
2831925
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 574-621: Update the restoration logic around
preferConfiguredHostedTools to track every stripped declaration location, not
only the first nested additional_tools index. When toolChoiceChanged and no
hosted declaration remains, restore the hosted image-generation tool
independently in top-level tools and in every additional_tools entry that was
stripped; remove the if/else-if exclusivity while preserving unchanged requests
and existing tool ordering.
🪄 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: 390447f9-9773-4abc-b33e-e79c33acfbd0
📒 Files selected for processing (16)
docs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.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
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Useful opt-in fix for Responses-compatible gateways that reserve the hosted image namespace server-side. Not a duplicate of #492 / #588 / #90 — those handle conflict strip / namespace round-trip / hosted strip; this adds keyed modelPreferHostedTools so selected models can prefer hosted image_generation without widening tool choice for everyone. Worth landing after the gap below.
What it does
- New provider config
modelPreferHostedTools(exact model →["image_generation"]). - On keyed Responses (non-forward): strip colliding client
image_genwrappers and rewrite affectedtool_choiceto hostedimage_generation. - Validation rejects forward auth and non-Responses wires; virtual
-proIDs documented/tested. - Docs + structure notes updated.
Bugbot
| Severity | Location | Finding |
|---|---|---|
| Medium | src/adapters/openai-responses.ts ~610–620 (preferConfiguredHostedTools) |
Hosted restore is gated on toolChoiceChanged. If the request has client-only image_gen tools and a non-forcing tool_choice (auto / none / unset / unchanged string), the preference still strips the client surface but does not re-inject { type: "image_generation" }, so the wire can end with no image tool. Existing regressions only cover forced wrapper selectors (function / custom / allowed_tools) where tool_choice is rewritten. |
Suggested fix: restore a hosted declaration whenever client image_gen was stripped and no hosted declaration remains in the request-wide tool set — not only when tool_choice was rewritten. Add a regression with tool_choice: "auto" (and optionally omitted) + client-only image_gen namespace under modelPreferHostedTools.
Security review
No medium+ security issues. Forward gating holds at validation and runtime; tool_choice rewrite stays scoped to operator opt-in + existing image-gen intent; model keys are lookup-only (no injection sink).
Codex / CodeRabbit open fixes
All prior threads are resolved; author responses look substantiated:
- Codex P2 “validate configured adapter” — routing replaces registry-known adapter before wire check; accepted.
- Rabbit forward-auth / registry
authKind— verified at route boundary; accepted. - Rabbit shared mapper — withdrawn (different semantics).
- Rabbit virtual-model docs — wording fixed across locales.
- Rabbit restore hosted in same group — fixed in
28319250for forced selectors. - Rabbit multi-location restore — withdrawn after Responses Lite “one request-wide tool set” clarification.
None of those need reopen. The Bugbot auto/non-forcing strip gap above is new and not covered by the resolved threads.
CI / process notes
- Cross-platform CI was stuck on fork
action_required; workflow runs were approved and are in progress — wait for green before merge. maintainerCanModifyis false on this fork PR, so maintainers cannot push the fix into the branch. Please enable “Allow edits from maintainers” or land the Bugbot fix yourself on this PR (no follow-up PR needed).- Branch is ~11 commits behind
devbut currently mergeable; rebase only if conflicts appear.
Must-fix before merge
- Fix hosted restore when stripping client
image_genwithout atool_choicerewrite (+ regression fortool_choice: "auto"). - Green Cross-platform CI on the tip.
No other must-fix from this review.
2831925 to
1aba0e4
Compare
|
Fixed the non-forcing selector gap in 1aba0e4. Root cause: hosted restoration was incorrectly gated on Restoration now runs whenever a client wrapper was stripped and no hosted declaration remains, independent of selector rewriting. Added regressions for top-level Validated with |
2 participants
Summary
image_gendeclarations and rewrite affected selectors to hostedimage_generationwithout widening tool choice.Root cause
Some gateways reserve the image tool namespace server-side. The existing generic normalization intentionally retains an empty client namespace, so those gateways reject the request before tool execution.
Validation
bun run typecheckbun run privacy:scanbun run lint:guibun run doctor:gui:if-changedbun run buildindocs-siteFull
bun run test: 5,704 passed, 1 skipped; 28 existing environment-dependent model-catalog/network-policy failures remain outside this change.Summary by CodeRabbit
modelPreferHostedToolsto provider configuration for per-model opt-in to prefer hostedimage_generationon supported non-forwardopenai-responsesgateways.image_genselections to hosted tooling while preserving tool-choice intent.-promodel resolution behavior.