fix(hosted-tools): one effective-transport decision, one hosted declaration - #925
Conversation
…er validation 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.
…ration 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.
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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: cf5910ab10
ℹ️ 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".
| // 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); |
There was a problem hiding this comment.
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 👍 / 👎.
1 participant
Follow-up to #924. A branch review after that PR merged found two defects in the hosted-tool preference path; both are reproduced here and fixed with red-green coverage.
Forward-auth validation diverged from routing
modelPreferHostedToolsConfigError()made two transport decisions and only one of them was correct.The wire check asked
providerMatchesRegistryTransport()whether the config still points at the registry's documented endpoint — right, because apreserveCustomDestinationrow reused under a different endpoint keeps its own adapter at runtime. The forward-auth check sitting above it readregistry.authKindunconditionally.That matters because auth follows the same rule as the adapter:
routedProviderConfig()preserves the configuredauthModefor a custom destination too. So a config for such a provider withauthMode: "forward"andmodelPreferHostedToolsvalidated clean, whilepreferConfiguredHostedTools()— which runs only on the non-forward branch ofopenai-responses.ts— never applied the preference. The user gets a config the runtime silently ignores.Both checks now start from one
registryTransportMatchesdecision, computed once instead of twice.Multi-container restoration emitted the hosted tool twice
#924 made stripping walk every
additional_toolscontainer, and made restoration walk them too. But tool declarations are request-scoped, not container-scoped — the containers are separate carriers for a single tool set. Restoring into each stripped container putimage_generationon the wire once per container.Restoring into the first stripped container only keeps the capability that #924 was fixing, without the duplication.
Evidence
devwhile keeping the new tests fails exactly two tests, one per defect. Restoring them: 174 pass / 0 fail across the two files.config,openai-responses-passthrough,management-provider-validation,openai-api-virtual-models.bun x tsc --noEmitexit 0.Attribution
The feature these fix is @Eleven-is-cool's implementation from #616, integrated by @Ingwannu in #837 and merged as #924. This PR only repairs the two defects, and the second one is a defect in the fix I asked for during that review — not in the original contribution.