Strip sampling params for Sonnet 5+ models#335
Conversation
![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: 48b319a813
ℹ️ 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".
| static OPUS_4_7_OR_LATER_RE: LazyLock<Regex> = LazyLock::new(|| { | ||
| Regex::new( | ||
| r"(^|[./:@])claude-(opus-(4[-.]([7-9]|[1-9]\d)|([5-9]|[1-9]\d)[-.]\d{1,2})|fable-[a-z0-9][a-z0-9.-]*)($|[-./:@])", | ||
| r"(^|[./:@])claude-(opus-(4[-.]([7-9]|[1-9]\d)|([5-9]|[1-9]\d)[-.]\d{1,2})|sonnet-([5-9]|[1-9]\d)([-.]\d{1,2})?|fable-[a-z0-9][a-z0-9.-]*)($|[-./:@])", |
There was a problem hiding this comment.
reasoning=none for Sonnet 5
By adding Sonnet 5 to this shared regex, supports_output_config_effort() becomes true, so the existing adapter use_effort branch consumes an OpenAI reasoning_effort: "none"/universal ReasoningEffort::None without emitting thinking: {type:"disabled"}. Anthropic documents Sonnet 5 as adaptive-thinking-on by default and says disabled thinking requires that field, so these cross-provider requests still run with thinking and its token cost instead of opting out; keep the disabled thinking path for None even when Sonnet 5 supports effort. Source: https://platform.claude.com/docs/en/about-claude/models/whats-new-sonnet-5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this is a good catch, fixed
Erin McNulty (erin2722)
force-pushed
the
fix/strip-temperature-claude-sonnet-5
branch
from
48b319a to
4aa8366
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4aa8366815
ℹ️ 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".
| static OPUS_4_7_OR_LATER_RE: LazyLock<Regex> = LazyLock::new(|| { | ||
| Regex::new( | ||
| r"(^|[./:@])claude-(opus-(4[-.]([7-9]|[1-9]\d)|([5-9]|[1-9]\d)[-.]\d{1,2})|fable-[a-z0-9][a-z0-9.-]*)($|[-./:@])", | ||
| r"(^|[./:@])claude-(opus-(4[-.]([7-9]|[1-9]\d)|([5-9]|[1-9]\d)[-.]\d{1,2})|sonnet-([5-9]|[1-9]\d)([-.]\d{1,2})?|fable-[a-z0-9][a-z0-9.-]*)($|[-./:@])", |
There was a problem hiding this comment.
With Sonnet 5 added to this shared regex, supports_adaptive_thinking()/supports_output_config_effort() become true for same-provider Anthropic Sonnet 5 requests. A request containing output_config: {"effort":"xhigh"} is parsed to ReasoningEffort::Xhigh, but the new adaptive path ignores the raw output_config extras and reasoning_effort_level() serializes it back as EffortLevel::Max; before this regex change the raw output_config was preserved. Anthropic's generated type accepts separate xhigh and max values, so this silently changes the requested effort for those Sonnet 5 round-trips.
Useful? React with 👍 / 👎.
…fort=none
claude-sonnet-5 rejects sampling params (temperature/top_p/top_k) and uses the
modern reasoning contract (adaptive thinking + output_config.effort, rejecting
legacy thinking.type=enabled) - verified live against the Anthropic API.
- Extend OPUS_4_7_OR_LATER_RE, which gates apply_model_transforms,
supports_adaptive_thinking, and supports_output_config_effort for Opus 4.7+
and Fable, to also cover Sonnet 5+ (and provider-wrapped Bedrock/Vertex IDs).
Add focused tests.
- Sonnet 5 (and Opus 4.7+) think by default when the `thinking` field is omitted,
so a cross-provider `reasoning_effort: "none"` was silently dropping the opt-out
and running thinking with its token cost. Emit `thinking: {type: "disabled"}`
for an explicit opt-out (effort=none / enabled=false) even when the model
supports output_config.effort. Confirmed live: thinking.disabled yields
thinking_tokens=0 on claude-sonnet-5. Add unit and cross-provider tests.
Co-Authored-By: Claude <noreply@anthropic.com>
Erin McNulty (erin2722)
force-pushed
the
fix/strip-temperature-claude-sonnet-5
branch
from
4aa8366 to
0174ea3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0174ea33b1
ℹ️ 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".
| static OPUS_4_7_OR_LATER_RE: LazyLock<Regex> = LazyLock::new(|| { | ||
| Regex::new( | ||
| r"(^|[./:@])claude-(opus-(4[-.]([7-9]|[1-9]\d)|([5-9]|[1-9]\d)[-.]\d{1,2})|fable-[a-z0-9][a-z0-9.-]*)($|[-./:@])", | ||
| r"(^|[./:@])claude-(opus-(4[-.]([7-9]|[1-9]\d)|([5-9]|[1-9]\d)[-.]\d{1,2})|sonnet-([5-9]|[1-9]\d)([-.]\d{1,2})?|fable-[a-z0-9][a-z0-9.-]*)($|[-./:@])", |
There was a problem hiding this comment.
By matching Sonnet 5 here, requests with reasoning_effort now go through the adaptive/effort path; however, the adapter's forced-tool guard only omits the thinking object while still emitting output_config.effort. For claude-sonnet-5 with tool_choice: {"type":"tool", ...}/any (including the JSON-object shim) and reasoning enabled, Anthropic treats omitted thinking as adaptive by default and documents forced tool use as incompatible with active thinking, so these requests can still 400 instead of degrading reasoning like the legacy path. See Anthropic docs: https://platform.claude.com/docs/en/build-with-claude/adaptive-thinking and https://platform.claude.com/docs/en/agents-and-tools/tool-use/define-tools.
Useful? React with 👍 / 👎.
Routing claude-sonnet-5 through the adaptive/effort path rerouted same-provider Anthropic round-trips through the reconstructed OutputConfig, which serializes the universal ReasoningEffort enum back to EffortLevel. That enum has no Max variant, so reasoning_effort_level() collapsed ReasoningEffort::Xhigh to EffortLevel::Max - silently bumping Anthropic's distinct "xhigh" and "max" effort values (verified live: both accepted on claude-sonnet-5, distinct thinking depths). - Prefer the raw Anthropic `output_config` extras verbatim on same-provider round-trips when present (the pre-Sonnet-5 behavior), only reconstructing when there is no raw output_config. - Fix reasoning_effort_level() to map Xhigh -> EffortLevel::Xhigh (not Max) so the reconstructed cross-provider path no longer bumps the requested effort. - Add unit (raw round-trip preserves xhigh/max) and cross-provider (reasoning_effort=xhigh stays xhigh) tests.
Now that claude-sonnet-5 routes through the adaptive/effort path, requests with reasoning effort and forced tool_choice (tool_choice type "tool" with a name, including the JSON-object shim) hit the forced-tool guard. The guard already drops the `thinking` object because forced tool use is incompatible with active thinking, but it still emitted output_config.effort, leaving a dangling effort that asks for reasoning the guard just disabled. Drop output_config.effort when forced_tool_choice is set on the adaptive path, both for the reconstructed OutputConfig and the raw same-provider extras round-trip, while keeping output_config.format (independent of thinking). Confirmed live against claude-sonnet-5: the post-fix payload (forced tool, no thinking, no effort) returns 200 tool_use, and output_config with format only is accepted under forced tool_choice. Add tests covering the constructed path, the auto (non-forced) regression, and the raw round-trip path. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bfe4eeca6
ℹ️ 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 effort_level = if use_effort && !forced_tool_choice { | ||
| reasoning_effort_level(reasoning_config, Some(max_tokens)) |
There was a problem hiding this comment.
any tool_choice as forced
Fresh evidence: this new effort suppression still hinges on forced_tool_choice, but is_forced_tool_choice() only returns true for ToolChoiceType::Tool with a name while Anthropic ToolChoiceType::Any round-trips to universal ToolChoiceMode::Required (crates/lingua/src/providers/anthropic/convert.rs:2212,2231). For Sonnet 5/Opus 4.7+ requests with reasoning enabled and tool_choice: {"type":"any"}, this line leaves output_config.effort (and the adaptive thinking branch also keeps thinking) even though Anthropic documents thinking with tools only supports auto/none, so those required-tool requests still 400; treat Any as forced when deciding both effort and thinking. https://platform.claude.com/docs/en/build-with-claude/extended-thinking
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fixed
The typed-boundary CI gate (make typed-boundary-check-branch) flags new direct serde_json::Value field access (.get(...)/.as_str()/.as_object(), etc.) in crates/lingua/src/providers and src/universal. The Sonnet 5 test assertions were reaching into the emitted payload Value directly; switch them to the existing test idiom of deserializing into CreateMessageParams and asserting on typed fields (ThinkingType, EffortLevel). No behavior change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6637d72c9
ℹ️ 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".
| if forced_tool_choice { | ||
| if let Some(obj) = output_config.as_object_mut() { | ||
| obj.remove("effort"); |
There was a problem hiding this comment.
When an Anthropic round-trip has raw output_config and the target model uses adaptive thinking, this branch edits the provider field by calling as_object_mut() and removing "effort" by string key. The project guide for /workspace/lingua says provider extras must be read through typed views and forbids new raw Value field-plucking for provider semantics; use OutputConfig or another typed compatibility struct for this mutation so malformed shapes are rejected at the boundary and the typed-boundary CI gate does not fail.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fixed
…d effort
is_forced_tool_choice() only matched ToolChoiceType::Tool with a name, but
Anthropic tool_choice {"type":"any"} (which round-trips to universal
ToolChoiceMode::Required) also forces tool use. The forced-tool guard
therefore left output_config.effort (and, on the adaptive path, the enabled
thinking object) in place for any/required requests, and Anthropic rejects an
enabled thinking object under forced tool use with a 400 ("Thinking may not be
enabled when tool_choice forces tool use") - verified live on claude-sonnet-5
and claude-opus-4-6.
Broaden is_forced_tool_choice() to return true for ToolChoiceType::Any so the
existing effort/thinking suppression covers any as well as tool. Add tests for
the Sonnet 5 adaptive path (effort + thinking dropped) and the legacy
enabled-thinking path (thinking omitted so it no longer 400s).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2597985cf
ℹ️ 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".
| Some( | ||
| serde_json::to_value(&Thinking { | ||
| budget_tokens: None, | ||
| display: None, | ||
| thinking_type: ThinkingType::Disabled, |
There was a problem hiding this comment.
This branch now runs for every model covered by supports_output_config_effort(), including the existing claude-fable-5 match. For a cross-provider request such as OpenAI reasoning_effort: "none", use_effort is true and the adapter now serializes thinking: {"type":"disabled"}; Anthropic's adaptive-thinking docs list Fable 5 as always-on and state that thinking: {type: "disabled"} is not supported, so these requests become invalid instead of preserving the only supported default behavior. Source: https://platform.claude.com/docs/en/build-with-claude/adaptive-thinking
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fixed
| static OPUS_4_7_OR_LATER_RE: LazyLock<Regex> = LazyLock::new(|| { | ||
| Regex::new( | ||
| r"(^|[./:@])claude-(opus-(4[-.]([7-9]|[1-9]\d)|([5-9]|[1-9]\d)[-.]\d{1,2})|fable-[a-z0-9][a-z0-9.-]*)($|[-./:@])", | ||
| r"(^|[./:@])claude-(opus-(4[-.]([7-9]|[1-9]\d)|([5-9]|[1-9]\d)[-.]\d{1,2})|sonnet-([5-9]|[1-9]\d)([-.]\d{1,2})?|fable-[a-z0-9][a-z0-9.-]*)($|[-./:@])", |
There was a problem hiding this comment.
By adding Sonnet 5 to this adaptive-thinking matcher, forced-tool requests with no explicit params.reasoning still emit no thinking field because the adapter only creates disabled thinking from an explicit ReasoningEffort::None/enabled=false. On Sonnet 5, omitting thinking means adaptive thinking is on by default, and Anthropic documents tool_choice: {"type":"any"} or a named forced tool as incompatible with thinking, so common required-tool requests to claude-sonnet-5 will still 400 unless the forced-tool path emits thinking: {"type":"disabled"} for this default-on model. Sources: https://platform.claude.com/docs/en/about-claude/models/whats-new-sonnet-5 and https://platform.claude.com/docs/en/build-with-claude/extended-thinking
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this isn't correct
The effort=none opt-out emitted thinking: {type: "disabled"} for every model in
supports_output_config_effort(), which now includes claude-fable-5. Fable 5
keeps adaptive thinking always on and rejects thinking.type=disabled with a 400
("thinking.type.disabled is not supported for this model") - verified live. The
only supported opt-out there is to omit thinking, preserving the always-on
adaptive default.
Add a supports_disabling_thinking() capability (false for Fable/Mythos always-on
models, true otherwise) and gate the disabled emission on it in the adapter, so
reasoning_effort="none" omits thinking instead of emitting disabled on those
models. Add capability and cross-provider adapter tests.
2 participants
claude-sonnet-5 returns "temperature is deprecated for this model" (and the same for top_p/top_k), and uses the modern reasoning contract (adaptive thinking + output_config.effort, rejecting legacy thinking.type=enabled) - verified live against the Anthropic API. Extend the existing OPUS_4_7_OR_LATER_RE, which already gates apply_model_transforms, supports_adaptive_thinking, and supports_output_config_effort for Opus 4.7+ and Fable, to also cover Sonnet 5+ (claude-sonnet-5, dated and provider-wrapped Bedrock/Vertex IDs). Add focused tests.