Sign up
View stargazersSign up

fix(server): Codex threads start at the selected reasoning effort#33833

open2
main
factory/issue-33823
opened19h agobyJJulius Marminge
2 files changed
View on GitHubTwo-way sync enabledPR last imported 14h ago

codexThreadRuntimeParams (used by thread/start, thread/resume, the resume before thread/revert, and thread/fork) forwarded only modelSelection.model, so a Codex thread started at the user's Codex config default effort and the selected effort arrived only with turn/start, making Codex apply a settings change at the first turn (linked by the reporter to terra/luna ending turns early). The fix extracts the existing effort resolution from buildCodexTurnStartParams (selected reasoningEffort option, else codexDefaultReasoningEffortOverride for the model) into codexSelectedReasoningEffort and has codexThreadRuntimeParams send it as config.model_reasoning_effort, so the thread and its turns always agree. No effort selected and no model override means nothing is sent, as before. No new regex or parser. One file changed (+15/-4); the repro test from factory/repro-33823 is kept unchanged.

Fixes pingdotgg/t3code#33823

Reproduction on main

Commit fa2f4ff2a2 (origin/main). Ran T3CODE_DEV_SEED=0 vp run dev in a fresh worktree with a Codex instance launched with -c model_reasoning_effort="medium" (stand-in for the reporter's config.toml default) and a logging wrapper in front of the codex binary (codex-cli 0.163.0) to capture JSON-RPC. In the web UI, picked GPT-6-Luna with reasoning effort High (the picker labels Medium as Default) and sent a 9-file, one-at-a-time task. Captured traffic: thread/start carried model: gpt-6-luna and no effort anywhere (no config.model_reasoning_effort); turn/start carried effort: high and collaborationMode.settings.reasoning_effort: high. The Codex rollout logged a thread_settings_applied event at the turn switching the thread to reasoning_effort: high (this version's form of the settings update the reporter sees as configuration_update on 0.167). With this short task the turn completed all 9 steps; the early stop itself depends on the model and the reporter's longer task and newer Codex, so I did not see it. Added a replay test ("starts a thread at the selected reasoning effort") that calls ensureThread with effort high and expects thread/start config to include model_reasoning_effort: "high"; it fails on main and passed with a throwaway fix, which I reverted.

Evidence

Being captured: before and after in the real app. This section is filled in before the pull request can merge.

Checks run

vp test run src/orchestration-v2/Adapters/CodexAdapterV2.test.ts -t "starts a thread at the selected reasoning effort" with the fix stashed: 1 failed (reproduces). vp test run src/orchestration-v2/Adapters/CodexAdapterV2.test.ts with the fix: 150/150 passed. vp run check:changed: format, lint, knip, graphql, typecheck (3 packages), test (2 files, 152 tests) all pass.

Not included

  • The thread config sends the effort unvalidated, while turn/start decodes it against the Codex ReasoningEffort schema; values come from Codex's own model catalogue, so validation was left as is.
  • FRICTION (tooling): git push to git.cafe over HTTPS failed because credential.https://git.cafe.usehttppath=true while the stored credential has no path; pushed with -c credential.https://git.cafe.usehttppath=false and did not change the global config.

AI-assisted; verified with the tests and tools listed above.

Checks

23 successful · 10 skipped
All checks passed
f2b7c2f

Added 2 commits

  • test(server): Codex thread/start carries the selected reasoning effortc4df0c1
  • fix(server): Codex threads start at the selected reasoning effortf2b7c2f
MmacroscopeappBot19h ago
Synced from GitHub

Approvability

Verdict: Approved at f2b7c2f

Macroscope's review found this PR approvable — Codex threads now initialize with the reasoning effort already used for their turns, avoiding a settings switch mid-turn. Existing selections and fallback defaults remain unchanged, with the override scoped to individual threads.

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

Ggithub-actionsBot19h ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ The exact PR base did not have a successful artifact. Baseline uses the latest successful main measurement shown below.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
CodexThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
CodexLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
CodexLive turn WebSocket decoded21.5 KiB21.5 KiB0 B (0.0%)24.4 KiB✅
CodexLive turn messages110 (0.0%)8✅
ClaudeTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
ClaudeThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
ClaudeLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
ClaudeLive turn WebSocket decoded21.9 KiB21.9 KiB0 B (0.0%)24.4 KiB✅
ClaudeLive turn messages110 (0.0%)8✅

Baseline: feb9c6b · PR result: f2b7c2f · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

CcoderabbitaiBot19h ago
Synced from GitHub

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The Codex adapter now includes the selected reasoning effort in thread configuration when one is defined. It also uses a shared helper to resolve reasoning effort for turn parameters. A replay test checks that thread/start sends the selected high-effort value.

Changes: Codex thread reasoning effort

Layer / File(s)Summary
Resolve selected reasoning effort
apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts		A shared helper returns the selected reasoning effort or the model-specific Codex default. Turn parameters now use this helper.
Set effort in thread configuration
apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts, apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts		Thread runtime parameters now include the selected effort in configuration when defined. A replay test checks that thread/start sends "high" when that effort is selected.

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

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

Suggested labels: bugfix, size:S

Suggested reviewers: macroscopeapp, t3dotgg

Merge Risk: External image Minimal · up to f2b7c

No actionable issue remains in the reviewed change. It is mergeable after normal checks.

Pre-merge checks: Passed 4

Check nameStatusExplanation
Title checkPassedThe title clearly summarizes the main change: Codex threads start with the selected reasoning effort.
Description checkPassedThe description explains the problem, change, reproduction, linked issue, and verification results. The scope and approval section does not state maintainer approval or explain why the fix qualifies a…
Linked Issues checkPassedIssue [#33823] asks T3 to send the selected reasoning effort in Codex thread configuration. codexSelectedReasoningEffort now uses the same option and model-default resolution as `buildCodexTurnStart…
Out of Scope Changes checkPassedThe changes are limited to Codex effort resolution, thread runtime configuration, and a regression test for issue [#33823]. Each change supports the linked issue. No unrelated changes appear in the wh…
Full details: Description check

Explanation

The description explains the problem, change, reproduction, linked issue, and verification results. The scope and approval section does not state maintainer approval or explain why the fix qualifies as an obvious bug, and the agent-use note omits the model and harness. These gaps do not make the otherwise detailed description largely incomplete.

Full details: Linked Issues check

Explanation

Issue [#33823] asks T3 to send the selected reasoning effort in Codex thread configuration. codexSelectedReasoningEffort now uses the same option and model-default resolution as buildCodexTurnStartParams. codexThreadRuntimeParams adds that effort as config.model_reasoning_effort, and the regression test checks that ensureThread sends high in thread/start. When no effort or model override resolves, the code omits the field.

Full details: Out of Scope Changes check

Explanation

The changes are limited to Codex effort resolution, thread runtime configuration, and a regression test for issue [#33823]. Each change supports the linked issue. No unrelated changes appear in the whole-PR diff.

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
External image Recent review info

External image Run configuration

External image Commits

Reviewing files that changed from the base of the PR and between 61cb845d9699d0ecce5a9b660d3b32907914df8b and f2b7c2fb8462f7ccf8e753377e374469014f0ad3.

External image Files selected for processing: 2

  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts

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

JJulius Marminge18h ago

The automated pipeline stopped on this pull request and will not merge it:

the capture step could not show the bug on the base commit, so there is nothing to prove fixed.

On each side, configured a transparent Codex logging wrapper supplying a medium default effort, selected the affected model with High effort and Standard service tier, and submitted the identical nine-file sequential shell task without follow-ups. Requested one shell call per file followed by one verification call. Both turns completed and both sets of nine files independently verified. Base did not exhibit early termination; steps were not changed to manufacture a difference.

It stays open for a person to finish or close.