Skip to content

fix(codex): turns run at the effort the picker shows - #14440

Closed
incognitojam wants to merge 1 commit into
pingdotgg:mainfrom
incognitojam:t3code/fix-codex-effort-default
Closed

incognitojam wants to merge 1 commit into
pingdotgg:mainfrom
incognitojam:t3code/fix-codex-effort-default

Conversation

@incognitojam

@incognitojam incognitojam commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Problem

On GPT-6.1-Sol, the effort picker shows Low as the default, but a turn sent without changing it runs at Medium.

The picker marks the default from Codex's model list, which is Low for GPT-6.1-Sol. When the user hasn't chosen an effort, the composer doesn't send one, and the server falls back to "medium" in buildCodexTurnInstructions.

Fix

When a turn names no effort, the Codex session runtime now sends the effort the picker shows as the model's default. It reads it from the same provider model list the picker uses, so the two agree for every model, including GPT-6-Astra, which the picker already shows at Medium. The "medium" fallback now only applies to models without effort options.

This changes what untouched turns run at. On models whose default in Codex's model list is Low, such as GPT-6.1-Sol, they now run at Low instead of Medium. Plan mode turns follow the same rule. An effort the user picks is sent as before.

Verification

  • Tests for defaultCodexReasoningEffort check that it returns the effort the picker marks as default (Low for a Sol model, Medium for GPT-6-Astra), and nothing for a model without effort options.
  • Live run with Codex CLI 0.159.1, sending turns without touching the effort picker and reading the effort from Codex's session log:
    • On main: GPT-6.1-Sol showed Low and Codex recorded medium.
    • With this change: GPT-6.1-Sol showed Low and Codex recorded low. GPT-6-Astra showed Medium and Codex recorded medium.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes (not applicable, the UI is unchanged)
  • I included a video for animation/interaction changes (not applicable)

Written by an agent (Claude Code, claude-opus-5-5).

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Sep 30, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The change synchronizes the Codex effort picker with the existing Medium execution fallback and includes targeted coverage. It nevertheless changes the user-facing product default from the model catalog’s Low to Medium for supported models.

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

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 81459509-653a-401a-be36-cb1d707fa83b

📥 Commits

Reviewing files that changed from the base of the PR and between 6401680 and 99b9380.

📒 Files selected for processing (2)
  • apps/server/src/provider/Layers/CodexSessionRuntime.test.ts
  • apps/server/src/provider/Layers/CodexSessionRuntime.ts

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


📝 Walkthrough

Walkthrough

CodexSessionRuntime now resolves the selected model’s default reasoning effort when a turn does not specify one. Tests cover model defaults and cases where no default is available.

Changes

Codex reasoning effort defaults

Layer / File(s) Summary
Resolve and apply model reasoning effort
apps/server/src/provider/Layers/CodexSessionRuntime.ts, apps/server/src/provider/Layers/CodexSessionRuntime.test.ts
Adds a helper that reads a model’s reasoning effort. sendTurn uses the explicit effort when present, or the model’s default otherwise. Tests cover two model defaults and models without a default.

Priority: ➖ Normal

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

Change: Bug fix

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to 99b93

Turns use the selected model’s displayed default when effort is omitted, while explicit choices and the existing fallback remain intact. No material merge risk was found.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 64016

The shared default aligns the displayed choice with existing execution behavior. The reviewed changes do not expand access, permissions, or execution authority.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated change is limited to default capability metadata and the source of an unchanged turn fallback. The reviewed diff provides no new path to process spawning, credentials, approval authority, or sandbox configuration.

Trust Boundaries and Controls

  • observed — The capability mapper checks the advertised supported efforts before selecting the shared default. This constrains the displayed default to the catalog's supported options; it is not an authorization control or runtime validation guarantee.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: Codex turns now use the effort shown by the picker.
Description check ✅ Passed The description clearly explains the problem, fix, scope, verification, and checklist status. It uses different headings from the template, but it provides the required change rationale and confirms t…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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

@incognitojam
incognitojam marked this pull request as draft September 30, 2026 14:01
A Codex turn with no effort picked ran at the collaboration mode's
medium fallback, while the effort picker showed the model's default
from Codex's model list, which is Low for the Sol models. When a turn
names no effort, send the one the picker shows as the model's default.
The medium fallback now only applies to models without effort options.
@incognitojam
incognitojam force-pushed the t3code/fix-codex-effort-default branch from 6401680 to 99b9380 Compare September 30, 2026 14:05
@incognitojam incognitojam changed the title fix(codex): effort picker no longer shows Low for Medium turns fix(codex): turns run at the effort the picker shows Sep 30, 2026
@incognitojam
incognitojam marked this pull request as ready for review September 30, 2026 14:08

Copy link
Copy Markdown
Member

Note

This comment is posted by Julius' dot

Could a maintainer confirm that untouched turns should adopt the model picker's default, rather than keeping Medium and correcting the display? The current diff changes GPT-6.1-Sol execution from Medium to Low, including plan mode, and the live log comparison demonstrates that choice. The prior-approval rule asks maintainers to settle intentional product-default changes. Please link that direction before review handoff.

@juliusmarminge

Copy link
Copy Markdown
Member

Thanks for working on this. We merged the orchestrator V2 rewrite in #2829, and we are closing this PR as part of that transition.

This change touches CodexSessionRuntime.ts, which the V2 merge removed. Codex execution now uses CodexAdapterV2 and the generated app-server integration.

Sorry for the extra work this creates. If the change is still needed on V2, please rebuild it on current main, verify it there, and open a new PR linking back here. We're closing the current implementation without assuming the underlying request is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants