[codex] Update reasoning effort - #29899
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aacfca13c
ℹ️ 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".
| if let Err(err) = super::multi_agents::validate_ultra_reasoning_effort( | ||
| turn_context.effective_reasoning_effort(), | ||
| &turn_context.config.features, | ||
| ) { |
There was a problem hiding this comment.
Validate Ultra defaults before committing settings
When a turn switches to a model whose catalog default effort is ultra while features.multi_agent_mode is disabled, SessionConfiguration::apply() does not reject it because the explicit effort is still None; this later effective-effort check returns BadRequest only after state.session_configuration = next.clone() has already committed the model/settings. The rejected update then sticks, so subsequent turns keep selecting the same invalid Ultra default and fail until the user manually changes settings again. Validate the effective effort before storing next, or roll back the state on this error.
Useful? React with 👍 / 👎.
9aacfca to
51d08c2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51d08c2e02
ℹ️ 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".
| if let Err(err) = super::multi_agents::validate_ultra_reasoning_effort( | ||
| turn_context.effective_reasoning_effort(), | ||
| &turn_context.config.features, | ||
| ) { |
There was a problem hiding this comment.
Validate Ultra before startup prewarm
When a session starts on a model whose explicit or catalog-default effort is ultra while features.multi_agent_mode is disabled and Responses WebSocket prewarm is enabled, the startup prewarm path builds a turn with new_startup_prewarm_turn_with_sub_id() and never reaches this validation before calling prewarm_websocket, so the backend still receives a reasoning.effort: "max" prewarm request even though the first real turn is later rejected. Apply the same Ultra gate before scheduling/issuing startup prewarm, or skip prewarm for invalid effective efforts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
we can prewarm but we should reject if ultra is not support on turn.
|
|
||
| fn reasoning_effort_for_request(effort: ReasoningEffortConfig) -> ReasoningEffortConfig { | ||
| match effort { | ||
| ReasoningEffortConfig::Ultra => ReasoningEffortConfig::Custom("max".to_string()), |
There was a problem hiding this comment.
Preserve custom providers'
ultra effort
When a non-OpenAI/custom provider or cached model catalog advertises the literal effort string ultra (or a user has model_reasoning_effort = "ultra" for that provider), deserialization now turns it into ReasoningEffort::Ultra and this helper rewrites it to max for every Responses/compact/memory request without checking the provider. Before this change, ultra was just a custom effort and was forwarded unchanged, so these integrations no longer receive the effort value they advertised; gate the aliasing to the Codex/OpenAI models that require max, or preserve the original wire value for custom providers.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
7fbc663 to
5605533
Compare
| /// Set the initial multi-agent mode for this thread. `none` leaves the | ||
| /// multi-agent tools available without injecting mode instructions. | ||
| /// Omitted defaults to `explicitRequestOnly`. | ||
| #[deprecated(note = "use Ultra reasoning effort for proactive multi-agent behavior")] |
There was a problem hiding this comment.
i think these rust deprecated tags are causing issues in ci. do we want to deprecate on the rust level or just on the app-server API spec level
646944e to
6f61655
Compare
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn model_reasoning_selection_popup_ultra_snapshot() { |
There was a problem hiding this comment.
this is just to test the reasoning level shows up? is the logic different enough from the other reasoning levels to warrant the test, or do all reasoning levels have this test?
There was a problem hiding this comment.
not much - the only diff is that we also gate this by feature.mutli_agent_mode. I can drop this.
b6ff83b to
b5816e9
Compare
| active_permission_profile, | ||
| reasoning_effort, | ||
| multi_agent_mode, | ||
| multi_agent_mode: MultiAgentMode::ExplicitRequestOnly, |
There was a problem hiding this comment.
basically all responses that contain muti_agent_mode will return explicit request only. This is not concerning because we are strongly enforcing ultra reasoning effort as the only way to go into mutl_agent_mode.
| let multi_agent_mode = match turn_context.effective_reasoning_effort() { | ||
| Some(ReasoningEffort::Ultra) => MultiAgentMode::Proactive, | ||
| _ => MultiAgentMode::ExplicitRequestOnly, | ||
| }; |
There was a problem hiding this comment.
do we need to first check that the model supports Ultra? or is that guaranteed by this point?
There was a problem hiding this comment.
it is granular enough at this point
There was a problem hiding this comment.
i think this forwards the effort unchanged, so Ultra stays Ultra? in the normal responses request path we have some logic that maps Ultra -> Max. should we reuse that?
There was a problem hiding this comment.
good point - it should be mapped to max cause ultra actually is not supported by the backend.
| preset | ||
| .supported_reasoning_efforts | ||
| .sort_by_key(|preset| preset.effort == ReasoningEffort::Ultra); | ||
| preset |
There was a problem hiding this comment.
why are we reordering the reasoning effort order from the model/list result? is this leftover from when we were filtering Ultra out by the feature flag?
the app-server guidance says we shouldnt reorder this, and i think we order them correctly from the server anyways
There was a problem hiding this comment.
yep from before - addressing it now.
b5816e9 to
7988f0f
Compare
7988f0f to
1b7ec0f
Compare
sayan-oai
left a comment
There was a problem hiding this comment.
thanks for cleaning this all up!
final nit-- can you update pr desc to describe final state?
Why
Parsing reasoning effort to enable multi agent mode conditionally.