Skip to content

[codex] Wire reasoning summary delivery configuration - #30752

Closed
alexi-openai wants to merge 1 commit into
mainfrom
codex/reasoning-summary-delivery
Closed

[codex] Wire reasoning summary delivery configuration#30752
alexi-openai wants to merge 1 commit into
mainfrom
codex/reasoning-summary-delivery

Conversation

@alexi-openai

@alexi-openai alexi-openai commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a typed reasoning_summary_delivery config option with sequential, concurrent, and concurrent_cutoff values
  • send stream_options.reasoning_summary_delivery on OpenAI Responses API requests over HTTP and WebSocket
  • expose the option through app-server thread start, resume, and fork parameters
  • preserve the override through live resume and config locking while omitting it for custom providers

Stack

Depends on #30876.

Validation

  • combined stack previously passed the full GitHub Actions matrix
  • focused codex-core HTTP, WebSocket, non-OpenAI omission, and config-lock tests
  • focused app-server typed override and live-resume tests
  • app-server protocol serialization and schema fixtures
  • scoped Clippy and formatting

@alexi-openai
alexi-openai marked this pull request as ready for review June 30, 2026 20:35
@alexi-openai
alexi-openai requested a review from a team as a code owner June 30, 2026 20:35

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate resume before mutating delivery

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>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Forward reasoning delivery through TUI thread params

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 dylan-hurd-oai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally makes sense, but I'd like to better understand why we have to have the mutex specifically for this new field

Comment thread codex-rs/core/src/client.rs Outdated
.reasoning_summary_delivery
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner) = Some(reasoning_summary_delivery);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
alexi-openai force-pushed the codex/reasoning-summary-delivery branch from 8b6a1e4 to cd47325 Compare July 1, 2026 23:02
@alexi-openai alexi-openai changed the title [codex] Add configurable reasoning summary delivery [codex] Wire reasoning summary delivery configuration Jul 1, 2026
@alexi-openai
alexi-openai changed the base branch from main to codex/interleaved-reasoning-items July 1, 2026 23:03
@alexi-openai
alexi-openai force-pushed the codex/reasoning-summary-delivery branch 2 times, most recently from afca28a to b86d1db Compare July 2, 2026 22:53
@alexi-openai
alexi-openai force-pushed the codex/interleaved-reasoning-items branch from d0da607 to 4d84452 Compare July 2, 2026 22:58
@alexi-openai
alexi-openai force-pushed the codex/reasoning-summary-delivery branch from b86d1db to 9355f80 Compare July 2, 2026 22:59
Comment thread codex-rs/exec/src/lib.rs Outdated
workspace_roots: None,
model_provider: model_provider.clone(),
service_tier: None,
reasoning_summary_delivery: None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a ConfigOverrides value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in a20fc94. reasoning_summary_delivery no longer flows through ConfigOverrides; thread start, cold resume, and fork apply the typed request field directly after loading the config.

),
])),
ConfigOverrides::default(),
ConfigOverrides {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to use ConfigOverrides for this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed and removed in a20fc94. The app-server config-loading test is back to testing only the existing ConfigOverrides behavior.

] {
let config = Config::load_from_base_config_with_overrides(
fixture.cfg.clone(),
ConfigOverrides {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test exclusively exists to test the ConfigOverrides.reasoning_summary_delivery field, so if we delete the field, we can get rid of it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in a20fc94 along with the ConfigOverrides.reasoning_summary_delivery field. The public app-server resume behavior test remains as the contract.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the right api, shouldn't we just be submitting this as part of PendingThreadResumeRequest?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Running resumes now carry the tri-state value on PendingThreadResumeRequest; the listener applies it after validation and successful subscription, before sending the resume response. This also avoids mutating the thread on a failed resume.

alexi-openai added a commit that referenced this pull request Jul 6, 2026
## Summary

- preserve reasoning item IDs on summary part and text delta events
- track streamed response items by ID so reasoning summaries can
continue after later items begin
- keep TUI output complete and deduplicated when reasoning and
final-answer events interleave

## Stack

- **1/2: this PR — interleaved item support**
- 2/2: #30752 — wire reasoning summary delivery configuration through
the CLI and app-server

## Validation

- just test -p codex-api preserves_reasoning_summary_item_ids
- just test -p codex-core
interleaved_reasoning_summary_events_keep_reasoning_item_metadata
- just test -p codex-tui
live_reasoning_summary_is_not_rendered_twice_when_item_completes
- just fix -p codex-api -p codex-core -p codex-tui
- just fmt
Base automatically changed from codex/interleaved-reasoning-items to main July 6, 2026 16:12
@alexi-openai
alexi-openai force-pushed the codex/reasoning-summary-delivery branch from 9355f80 to a20fc94 Compare July 6, 2026 16:12
@alexi-openai
alexi-openai force-pushed the codex/reasoning-summary-delivery branch from a20fc94 to 786e552 Compare July 6, 2026 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants