Strip unsupported params from gpt-6 models - #462
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 718c7792bb
ℹ️ 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 (@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 (@codex) address that feedback".
| "gpt-5", | ||
| &[StripTemperature, StripTopP, ForceMaxCompletionTokens], | ||
| ), | ||
| ("gpt-6", &[StripTemperature]), |
There was a problem hiding this comment.
When a route uses Bedrock's namespaced OpenAI model ID (openai.gpt-6..., consistent with crates/braintrust-llm-router/src/catalog/spec.rs:55-58), this prefix does not match. prepare_provider_request passes the catalog model as the override, transform_request replaces universal.model with it, and apply_model_transforms therefore leaves temperature intact, causing the request to reach a GPT-6 endpoint with the unsupported parameter. Normalize the supported openai. namespace before matching or add an equivalent rule.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| "gpt-5", | ||
| &[StripTemperature, StripTopP, ForceMaxCompletionTokens], | ||
| ), | ||
| ("gpt-6", &[StripTemperature]), |
There was a problem hiding this comment.
This provider-parameter behavior change adds only the capability rule: there is no corresponding case in payloads/cases/params.ts, captured transform, or even a GPT-6 entry in the existing capability test tables. Consequently, the end-to-end path that must strip temperature is unverified, contrary to the repository's mandatory workflow for provider behavior changes; add the behavior-named payload case and capture it before landing the rule.
AGENTS.md reference: AGENTS.md:L123-L131
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ededae556
ℹ️ 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 (@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 (@codex) address that feedback".
| let normalized = match lower.strip_prefix("openai.") { | ||
| Some(normalized) => normalized, | ||
| None => &lower, | ||
| }; |
There was a problem hiding this comment.
When GPT-6 is routed through an OpenAI-compatible provider using an openai/gpt-6... model ID, this normalization leaves the slash-prefixed name unchanged, so get_model_transforms does not match the new GPT-6 rule and apply_model_transforms retains the unsupported temperature. This namespace is already recognized by normalize_openai_model_name and covered by the existing openai/gpt-5.4 test; reuse or extend that normalization so both slash and Bedrock dot namespaces receive the transform.
Useful? React with 👍 / 👎.
Erin McNulty (erin2722)
changed the title
2 participants
No description provided.