Skip to content

feat(codex): support max reasoning effort - #1683

Open
sean2077 wants to merge 2 commits into
chenhg5:mainfrom
sean2077:codex/max-reasoning-effort
Open

feat(codex): support max reasoning effort#1683
sean2077 wants to merge 2 commits into
chenhg5:mainfrom
sean2077:codex/max-reasoning-effort

Conversation

@sean2077

@sean2077 sean2077 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • accept max and maximum as Codex reasoning-effort values
  • expose max through the /reasoning selector
  • render /reasoning usage from each agent's AvailableReasoningEfforts() instead of a shared hardcoded list
  • keep Codex reasoning levels and aliases in one ordered definition
  • add an Unreleased changelog entry

Why

Codex models such as GPT-5.6 support a max reasoning effort. cc-connect previously normalized that value to an empty string, so reasoning_effort = "max" silently fell back to the Codex default and /reasoning max was rejected.

The previous shared usage string was also inaccurate across agents: Pi exposes off and minimal, while Claude Code and Codex expose different level sets. Usage text now follows the current agent capability list.

User impact

Codex projects can configure reasoning_effort = "max", and users can select it through /reasoning max. Existing reasoning levels and session-reset behavior are unchanged. Other agents now show only the reasoning levels they actually advertise.

Validation

Passed locally on Windows:

  • go test ./agent/codex -count=1
  • go test ./core -run 'TestCmdReasoning|TestReasoning' -count=1
  • go test ./core/ -run TestCUJ -count=1
  • go vet ./agent/codex ./core
  • go build ./agent/codex ./core

go build ./... still reproduces the pre-existing Windows baseline failures outside this PR: duplicate daemon.CheckLinger definitions and an unused os import in agent/pi/proc_windows.go. The full test suite also contains pre-existing Windows-only failures that assume Unix executables, paths, symlink behavior, or file modes. Race tests could not be rerun locally because this Windows Go environment has CGO disabled; the previous CI run and maintainer review race runs passed.

Compatibility notes

  • The earlier npm run build was only a repository sanity check. The localized /reasoning strings are rendered by the Go backend and are not embedded in the web bundle, so it is omitted from the focused validation above.
  • cc-connect has no per-model reasoning-effort capability metadata. As with existing model-dependent levels such as xhigh, the selected value is passed to Codex CLI/provider for validation. max is sent only when a user explicitly configures or selects it; unsupported model/provider combinations may reject it at runtime.

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Conclusion: Comment (PR is in DRAFT — convert to ready-for-review when blockers resolved)

Overall assessment:

  • This PR is a small, well-scoped change that adds max as a recognized Codex reasoning effort and updates the localized /reasoning usage string. The change is correct and minimal, but the PR is currently marked as a draft and the author flags that go build ./... is still blocked on pre-existing upstream-main issues. Two small items below should be addressed before flipping to "ready for review".

Review scope:

  • agent/codex/codex.go — added case "max", "maximum": return "max" in normalizeReasoningEffort; added "max" to AvailableReasoningEfforts().
  • agent/codex/session_test.go — added TestNormalizeReasoningEffort_AcceptsMax and updated TestAvailableReasoningEfforts_ExcludesMinimal.
  • core/i18n.go — appended |max to all five locales in MsgReasoningUsage.
  • Run: go test -race ./agent/codex/... -count=1 -timeout 60s → ok in ~2.7s. go test -race ./core/... -run 'TestReasoning|TestCUJ' -count=1 -timeout 60s → ok in ~3.9s.

✅ What looks good:

  • Aliases both "max" and "maximum" in the normalizer — matches the previous convention (xhigh/x-high/very-high).
  • Updated all 5 locales in one go; no half-translated usage string.
  • Test naming follows the existing convention (TestNormalizeReasoningEffort_*, TestAvailableReasoningEfforts_*).
  • Behavioral change is isolated to the Codex agent's normalizeReasoningEffort and AvailableReasoningEfforts; no core contract changes.

🚨/🔴 Must fix:

  • None. No correctness/security/blast-radius issue found.

🟠 Should improve:

  • 🟠 P2 — PR is in Draft state. The body explicitly notes that go build ./... is blocked on pre-existing upstream-main issues (daemon.CheckLinger duplicates, unused os import in agent/pi/proc_windows.go). Those are not in this PR's diff, so it is fine to merge the code change, but please either (a) confirm on the PR thread that the failures reproduce on origin/main HEAD without this branch, or (b) attach a go build ./agent/codex ./core log so reviewers can see the change itself builds cleanly.
  • 🟠 P2 — MsgReasoningUsage is shared across agents. Adding |max is correct for Codex, but if Codex is the only agent exposing max, the /reasoning usage string on non-Codex projects will now advertise a value that is rejected (reasoning_effort=max falls back to default for Claude/Copilot/etc). Consider either moving the usage string to be per-agent, or wording it as "your agent's available values" so non-Codex users don't see misleading options.
  • 🟠 P2 — AvailableReasoningEfforts hardcodes the list. Now that we have a precedent for adding reasoning levels, future additions will all touch this list. Consider a single source of truth (e.g. a package-level slice with both the level name and any aliases), so the test does not need to be edited on every change.
  • 🔵 P3 — CHANGELOG.md is untouched. Suggest adding ### Added — codex: support 'max' reasoning effort via /reasoning max (#1683) to Unreleased.
  • 🔵 P3 — Single-commit PR with 18 additions. Squash before merge is unnecessary, but the commit subject could mention the i18n impact: e.g. feat(codex): support max reasoning effort (incl. /reasoning usage string).

❓ Questions:

  • The PR description claims npm run build in web/ was run. There are no web/ changes in the diff; was this a sanity check, or did the build pick up the i18n change? Worth confirming whether the web bundle embeds the usage string.
  • For projects where Codex is the agent and the model chosen does NOT support max effort (e.g. legacy codex-mini-latest), will /reasoning max send an invalid value to the model and trigger a runtime error, or is there a per-model allowlist in the Codex agent that filters unsupported levels? If the former, this PR could regress those users.

Testing / Risk:

  • Verified: codex tests + core reasoning/CUJ tests pass with -race.
  • go build -tags no_web ./... clean.
  • Risk surface: minimal. The only behavioral change is normalizeReasoningEffort("max") returning "max" instead of "", and AvailableReasoningEfforts now contains "max". Both are additive.
  • Not covered: end-to-end test that /reasoning max actually toggles the Codex agent's reasoning setting (would require a Codex CLI fixture). Acceptable to defer.

Next step:

  • Convert to ready-for-review once the P2 items are addressed (especially confirming the go build ./... failure is pre-existing on main, not introduced by this branch). The change itself is sound and would be straightforward to approve as-is.

@sean2077

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback in 698aec0.

  • Added focused build evidence: go build ./agent/codex ./core passes on Windows. The full-build failures still reproduce in packages outside this PR (daemon duplicate CheckLinger; unused os in agent/pi).
  • Replaced the shared hardcoded usage list with values derived from the active agent's AvailableReasoningEfforts(). This also fixes the existing Pi/Claude mismatch rather than special-casing Codex in core.
  • Centralized Codex canonical levels and aliases in one ordered definition used by both normalization and AvailableReasoningEfforts().
  • Added the Unreleased changelog entry and a regression test for agent-specific usage output.

Answers to the two questions:

  • npm run build was only a broad sanity check. The /reasoning i18n text is rendered by the Go backend and is not embedded in the web bundle, so I removed it from the focused validation list.
  • There is no per-model reasoning-effort allowlist in cc-connect. The current official Codex models documentation describes Max as an opt-in reasoning setting, while the configuration reference notes that reasoning levels apply to supported models and that higher levels can be model-dependent. Neither page establishes support for legacy codex-mini-latest, so this PR does not claim that compatibility. Like existing xhigh, max is passed to Codex CLI/provider only when explicitly selected; an unsupported model/provider combination may reject it at runtime.

Focused tests, CUJ, vet, and targeted build are green locally. The updated CI run is in progress.

@sean2077
sean2077 marked this pull request as ready for review August 15, 2026 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants