[codex] Wire reasoning summary delivery configuration#30752
[codex] Wire reasoning summary delivery configuration#30752alexi-openai wants to merge 1 commit into
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: ded89cfb27
ℹ️ 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".
| "invalid reasoning_summary_delivery override: {error}" | ||
| )) | ||
| })?; | ||
| existing_thread.set_reasoning_summary_delivery(reasoning_summary_delivery); |
There was a problem hiding this comment.
When a client resumes a running thread with both reasoning_summary_delivery and a stale path, this mutates the existing thread before the path consistency check below returns invalid_request. The failed resume can still change stream options for later turns observed by other clients; defer the setter until after the path and resume-mismatch validation succeeds.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| pub model_reasoning_effort: Option<ReasoningEffort>, | ||
| pub plan_mode_reasoning_effort: Option<ReasoningEffort>, | ||
| pub model_reasoning_summary: Option<ReasoningSummary>, | ||
| pub reasoning_summary_delivery: Option<ReasoningSummaryDelivery>, |
There was a problem hiding this comment.
When this config is set in a local TUI session connected to a remote app server, the value is parsed into Config here, but config_request_overrides_from_config in tui/src/app_server_session.rs is the path that serializes local Config into thread/start, thread/resume, and thread/fork overrides and it still omits reasoning_summary_delivery. The remote app server therefore never sees the new knob, so include it in that overrides map and its coverage.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
dylan-hurd-oai
left a comment
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
This generally makes sense, but I'd like to better understand why we have to have the mutex specifically for this new field
| .reasoning_summary_delivery | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner) = Some(reasoning_summary_delivery); | ||
| } |
There was a problem hiding this comment.
It seems like we're doing this so we can special case it and change this while a thread is "active." I don't think we have equivalent methods for other fields on this struct, is this strictly necessary?
alexi-openai
force-pushed
the
codex/reasoning-summary-delivery
branch
from
8b6a1e4 to
cd47325
Compare
alexi-openai
changed the title
alexi-openai
changed the base branch from
main
to
codex/interleaved-reasoning-items
alexi-openai
force-pushed
the
codex/reasoning-summary-delivery
branch
2 times, most recently
from
afca28a to
b86d1db
Compare
alexi-openai
force-pushed
the
codex/interleaved-reasoning-items
branch
from
d0da607 to
4d84452
Compare
alexi-openai
force-pushed
the
codex/reasoning-summary-delivery
branch
from
b86d1db to
9355f80
Compare
| workspace_roots: None, | ||
| model_provider: model_provider.clone(), | ||
| service_tier: None, | ||
| reasoning_summary_delivery: None, |
There was a problem hiding this comment.
Do we need a ConfigOverrides value?
| ), | ||
| ])), | ||
| ConfigOverrides::default(), | ||
| ConfigOverrides { |
There was a problem hiding this comment.
We don't need to use ConfigOverrides for this
| ] { | ||
| let config = Config::load_from_base_config_with_overrides( | ||
| fixture.cfg.clone(), | ||
| ConfigOverrides { |
There was a problem hiding this comment.
this test exclusively exists to test the ConfigOverrides.reasoning_summary_delivery field, so if we delete the field, we can get rid of it.
| if let Some((existing_thread_id, existing_thread, mut source_thread)) = running_thread { | ||
| if let Some(reasoning_summary_delivery) = params.reasoning_summary_delivery { | ||
| existing_thread | ||
| .set_reasoning_summary_delivery(reasoning_summary_delivery) |
There was a problem hiding this comment.
I'm not sure this is the right api, shouldn't we just be submitting this as part of PendingThreadResumeRequest?
2 participants
Summary
Stack
Depends on #30876.
Validation