Sign up
View stargazersSign up

fix(server): honor Codex reasoning summary preferences#20410

closed6
main
GitHub fork: fix/14952-codex-summary-auto
opened24d agobyOonly21mil
34 files changed
View on GitHubTwo-way sync enabledPR last imported 15h ago

Codex forced detailed reasoning summaries on every turn, which rejects turns on endpoints such as Bedrock and overrides configured summary preferences.

Read effective Codex config for the turn's working directory. Explicit preferences, including none, win; otherwise request auto. The same path covers initial, resumed, forked, and goal starts. The replay recorder always requests auto to keep machine configuration out of recordings, and existing recordings retain a synthetic unset config response unless they contain a real recorded exchange.

Thanks to @only21mil for the original fix and replay updates in #20410. The takeover preserves those commits and adds configured-preference handling against current main.

Verification on head 676ab9ac79 (Node 24.21.0, Linux):

  • Tests before → after: 169 baseline adapter tests; seven focused cases added. Four configured-preference cases failed before the rebuild; all 176 pass afterward.
  • Codex replay integration/contract tests: 33 passed across seven suites.
  • Server typecheck, targeted lint, formatting, and whitespace checks pass. Lint retains two existing unused-binding warnings.
  • Live native Codex 0.163.0 API checks with disposable homes: unset → auto, user concise → concise, user none → none, session detailed → detailed.
  • Independent full-diff reviews: two rounds each on Codex gpt-6-sol and Claude Opus 5.5. Round one found recorder configuration capture; removed that read and trimmed unused test setup. Round two is clear on both models.

Before/after test output and native API proof. This is a nonvisual backend change, so screenshots are not applicable.

Not verified: an authenticated model turn, live Bedrock, browser UI, macOS, Windows, iOS, Android, or packaged Electron desktop. This changes the server adapter shared by all clients and connection modes; no client or wire contracts change. Native configuration and test-level proof cover the available Linux paths. Current-head CI and normal maintainer approval remain required; this PR stays unmerged.

Tests: Codex gpt-6-astra (high). Implementation: Codex gpt-6.1-sol (high). Reviews: Claude Opus 5.5 and Codex gpt-6-sol. Harnesses: Codex and Claude Code in T3 Code.

Checks

21 successful · 13 skipped
All checks passed
d7407af

Added 1 commit

  • fix(server): Codex turns stop forcing a "detailed" reasoning summaryeab9083
MmacroscopeappBot24d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — The PR changes the production default reasoning-summary setting for every Codex turn, including resumed and forked flows, so it changes runtime behavior beyond the fixture updates. The implementation is targeted and well-tested, but the default change warrants human validation across provider and model configurations.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot24d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between eab908323532e5d59cc0832ab683b286c2ab094f and d7407af55e51423ae0447ec3267a0bca264d0309.

📒 Files selected for processing: 7
  • apps/server/scripts/record-codex-app-server-replay-fixture.ts
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.testkit.ts
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts
  • apps/server/src/orchestration-v2/testkit/fixtures/plan_questions/codex_transcript.ndjson
  • apps/server/src/orchestration-v2/testkit/fixtures/proposed_plan/codex_transcript.ndjson
  • apps/server/src/provider/CodexProvider.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Codex turn-start requests now use a reasoning summary setting derived from configuration, with auto as the fallback. Replay test support, tests, the fixture recorder, and replay transcripts reflect the updated setting.

Changes: Codex summary mode

Layer / File(s)Summary
Derive the turn summary setting
apps/server/src/provider/CodexProvider.ts, apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts		The provider returns the configured summary preference unless its origin is packaged defaults; otherwise, it returns auto. The adapter reads configuration for the runtime working directory and passes the result to turn-start parameters.
Test configured summary values
apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.testkit.ts, apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts		The replay testkit can supply config-read responses. Parameterized tests check summary values across first-start, resume, and fork lifecycles.
Update replay records
apps/server/scripts/record-codex-app-server-replay-fixture.ts, apps/server/src/orchestration-v2/testkit/fixtures/*/codex_transcript.ndjson		The fixture recorder and replay transcripts use auto instead of detailed in turn-start requests and matching thread settings.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant startNativeTurn
3 participant Codex config/read
4 participant codexTurnReasoningSummary
5 participant Codex turn/start
6 startNativeTurn->>Codex config/read: Read config for the runtime working directory
7 Codex config/read-->>startNativeTurn: Return the config response
8 startNativeTurn->>codexTurnReasoningSummary: Derive the summary setting
9 codexTurnReasoningSummary-->>startNativeTurn: Return the configured value or auto
10 startNativeTurn->>Codex turn/start: Send the summary setting

Suggested reviewers: t3dotgg, juliusmarminge

Merge Risk: ⚪ Minimal · up to d7407

No merge-blocking issue was established for the reasoning-summary preference change; normal checks remain appropriate.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly and concisely describes the main change: honoring configured Codex reasoning summary preferences.
Description checkPassedThe description clearly explains the problem, the implementation, affected lifecycle paths, replay behavior, verification results, limitations, and agent attribution. Although it does not use the temp…
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly explains the problem, the implementation, affected lifecycle paths, replay behavior, verification results, limitations, and agent attribution. Although it does not use the template headings, it provides the required information and identifies that maintainer approval remains pending.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

JJulius Marminge22d ago

Taking over this one as maintainer for batch codex-05. I will preserve author credit, rebuild the focused fix against current main, include configured reasoning-summary preferences, and run independent regression and adversarial reviews. This work will remain unmerged.

Added 5 commits

  • Merge remote-tracking branch 'origin/main' into fix/14952-codex-summary-auto59c6a0a
  • test(server): cover configured Codex reasoning summariesbf40908
  • fix(server): honor configured Codex reasoning summariesabd6050
  • fix(server): keep Codex replay recordings deterministic676ab9a
  • Merge upstream main into fix/14952-codex-summary-autod7407af
Ttheo2d ago

Note

🤖 Claude Opus 5.5 responding on behalf of Theo

Merged #31149 (af65a55). It does not fix #14952 or replace this PR, but it touches the same replay fixtures.

It adds "features.concurrent_reasoning_summaries": true to CODEX_THREAD_CONFIG, so every Codex thread/start, thread/resume, and thread/fork frame in testkit/fixtures/*/codex_transcript.ndjson now carries that key. A rebase of this PR needs to keep it.

The two changes work together. Codex sends non-blocking summary delivery only when a summary is requested, so a resolved auto still gets it and none turns it off.

Oonly21mil2d ago
Synced from GitHub

Note

🤖 Opus 5.5 and Sol 6.1 replying on behalf of Victor.

Re your comment: @t3dotgg @juliusmarminge Since Julius took this PR over, we haven't pushed to its branch. A reviewed merge of current main is ready at 82a19547a68eb5f698f92dc991a6886b91a7d003 on only21mil/t3code:fix/14952-codex-summary-auto-merge-main.

It keeps this PR's summary behavior (an explicit Codex model_reasoning_summary, none included, wins; packaged defaults and unset config send auto) and #31149's "features.concurrent_reasoning_summaries": true in CODEX_THREAD_CONFIG and in all 35 thread/start, thread/resume and thread/fork frames across the 28 Codex replay fixtures. vp run check:changed passes, and the Codex adapter and replay suites pass (480 tests in 14 files).

It fast-forwards from the current head:

sh
1git fetch https://github.com/only21mil/t3code fix/14952-codex-summary-auto-merge-main && git merge --ff-only FETCH_HEAD

If either of you would rather we push it, we can fast-forward the PR branch to it.

Rronharel021d ago
Synced from GitHub

I hit the same rejection and opened #33222, which was closed as a duplicate of this one. Sharing my gateway result, since the description lists an authenticated turn as unverified.

I tested this on macOS, with Codex pointed at an OpenAI-compatible gateway and no reasoning summary set in my Codex config.

  • main: every turn is rejected.
  • This PR (d7407af55e): turn/start sends summary: "auto", and a turn on gpt-6-luna completes normally :)
Oonly21mil1d ago
Synced from GitHub

Note

🤖 Opus 5.5 replying on behalf of Victor.

Thank you for testing this against a gateway and sharing the result. It covers the authenticated turn the description lists as unverified.

JJulius Marminge20h ago

Thanks, @only21mil, for the earlier fix and for testing the gateway behavior. Closing this in favor of #33222 by @ronharel02: this head is over two weeks old and main has changed its files, while the other PR is current. The approaches differ on honoring existing Codex configuration; please raise that separately if it still matters.