fix: accept max and ultra reasoning efforts for task --effort#454
fix: accept max and ultra reasoning efforts for task --effort#454Caleb0796 wants to merge 7 commits into
Conversation
Codex CLI 0.142.x model catalogs advertise max and ultra as supported reasoning levels (e.g. gpt-5.6-sol: low/medium/high/xhigh/max/ultra, per 'codex debug models'), but the companion's hardcoded VALID_REASONING_EFFORTS still stops at xhigh, so 'task --effort max' and 'task --effort ultra' are rejected before dispatch: Unsupported reasoning effort "max". Use one of: none, minimal, low, medium, high, xhigh. Users cannot request the top reasoning tiers through the plugin at all; the only workaround is setting model_reasoning_effort in ~/.codex/config.toml and omitting --effort. - add max and ultra to VALID_REASONING_EFFORTS - derive the error message from the set so the list can no longer drift - update the usage line, /codex:rescue argument-hint, and the codex-cli-runtime skill's accepted-values line - runtime tests: forward max/ultra to turn/start via the fake codex, and keep rejecting unknown values before a job starts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… catalog Adversarial review of the first commit found that a bare allowlist extension leaves a severe failure mode reachable: Codex app-server accepts turn/start for model+effort pairs the model catalog does not list (e.g. gpt-5.4-mini + ultra) and then hangs with zero events and no timeout (reported upstream as openai/codex#31552). max/ultra are 5.6-family tiers, so the flags this PR adds would have walked users straight into that hang on older models. This commit adds model-aware validation on the task dispatch path: - capture the resolved model that thread/start / thread/resume already return (previously discarded), so effort-only dispatches are checked against the configured default model too; - before turn/start, when an explicit --effort is present, issue a bounded (3s) model/list request on the existing app-server connection and reject unsupported pairs eagerly, listing that model's supported tiers; - fail open with an explicit warning when the catalog is unavailable, malformed, or does not contain the resolved model (custom providers, older CLIs) — never block dispatch on catalog problems; - fake-codex fixture gains a model/list handler plus model-list-unsupported / model-list-malformed behaviors, and five new runtime tests cover: explicit unsupported pair, effort-only against the resolved default, catalog unavailable, catalog malformed, and unknown-model pass-through (spark alias). Suite: 98/98. Live check: gpt-5.4-mini --effort ultra now fails in ~4s with the actionable message instead of hanging indefinitely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Update — second commit addresses "not all models support max/ultra". An adversarial review of the first commit raised a valid gap: The second commit adds model-aware validation on the dispatch path:
Suite: 98/98. Live before/after on the same machine: silent indefinite hang → clear error in ~4s with no turn started. 🤖 Generated with Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 917e058eec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!supported.includes(effort)) { | ||
| throw new Error( | ||
| `Model "${resolvedModel}" does not support reasoning effort "${effort}". Supported: ${supported.join(", ")}.` | ||
| ); |
There was a problem hiding this comment.
When model/list is available, this catalog check rejects --effort none and --effort minimal for the current OpenAI model entries because their supportedReasoningEfforts list starts at low, while this same change still accepts and advertises none/minimal in normalizeReasoningEffort and the task usage. Those values were valid plugin inputs before this commit, so users selecting a documented effort now fail before turn/start; either exempt these legacy values from the per-model catalog check or remove them from the accepted set/docs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ef1d739, though not via either suggested option — both have a sharper edge than the mismatch they fix:
- Exempting
none/minimalfrom the catalog check reopens the silent turn/start hang (app-server: turn/start accepts a model+reasoningEffort pair the model catalog says is unsupported, then hangs with zero events codex#31552) for exactly those values — the failure mode this PR exists to remove. - Removing them from the accepted set breaks older CLIs whose catalogs do list them: the check is dynamic, so on those installs
minimalpasses today and would become a hard parse error instead.
What changed: the values stay in the syntactic allowlist, the catalog check still applies to them (a rejected pair fails with that model's real tier list, which is strictly more actionable than the pre-PR dispatch-and-hang), and the runtime-skill doc now states that supported tiers vary by model and catalog-rejected pairs fail before dispatch.
| if (options.effort) { | ||
| await validateReasoningEffortForModel(client, resolvedModel, options.effort, options.onProgress); | ||
| } |
There was a problem hiding this comment.
For an unsupported model/effort pair, this validation runs after startThread has emitted Thread ready, so the progress updater records a threadId; when the validation throws before any turn/start, runTrackedJob stores a failed task with that threadId, and findLatestResumableTaskJob treats any non-running task with a threadId as resumable. After this failure, the next --resume/rescue continuation can resume the empty validation-failed thread instead of the last useful task; validate before creating the persistent thread or avoid marking pre-turn validation failures as resumable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — findLatestResumableTaskJob only requires threadId + non-active status, so a validation-failed job with a recorded threadId would have hijacked the next --resume. Fixed in ef1d739 by moving the check before thread creation on the fresh path: effort-only dispatches resolve the target from the catalog's isDefault entry, so a rejected pair now creates no thread at all (also no orphan ephemeral thread server-side), and the rejection tests assert threads.length === 0 in the fake state. Resume dispatches validate the resumed thread's reported model against the same single model/list fetch — a failure there leaves the pre-existing thread as the resume candidate, which is the correct target for that path.
|
Maintainer note: if you'd prefer to land the minimal fix first, I'm happy to split this — commit 1 ( 🤖 Generated with Claude Code |
…ance Review follow-up (PR bot, two P2s): 1. Pre-turn validation failures no longer pollute --resume: for fresh dispatches the catalog check now runs BEFORE thread/start (effort-only dispatches resolve the target via the catalog's isDefault entry), so a rejected pair creates no thread at all — nothing for findLatestResumableTaskJob to latch onto, and no orphan ephemeral thread server-side. Resume dispatches validate against the resumed thread's reported model, reusing the same single model/list fetch; a failed check there leaves the pre-existing thread as the resume target, which is the correct candidate anyway. 2. none/minimal vs catalog: kept in the syntactic allowlist (older CLIs whose catalogs list them still pass the dynamic check; removing them would break those users), and NOT exempted from the catalog check (exempting reopens the silent turn/start hang for exactly those values). The runtime-skill doc now states that supported tiers vary by model and that catalog-rejected pairs fail before dispatch. Tests: rejection cases now also assert zero threads were created; docs assertion added. Suite 98/98. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef1d739aad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } else { | ||
| if (options.effort) { | ||
| const targetModel = options.model ?? catalogDefaultModel(catalogEntries); |
There was a problem hiding this comment.
When a user relies on a user/project config.toml default model and supplies only --effort, this chooses the catalog's global isDefault entry instead of the model that thread/start would resolve from config (the start request still passes model: null). For a configured default such as a 5.6-family model that supports ultra, task --effort ultra is rejected against the global default before Codex can apply the config, breaking the documented config-default flow; start the thread first and validate the returned model, or read the configured default before this check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the catalog's global isDefault entry is not the model thread/start resolves from config.toml, so effort-only dispatches could be falsely rejected. Fixed in f7f47ee by narrowing scope instead of adding a second resolution mechanism: validation now applies only when BOTH --model and --effort are explicit. Effort-only dispatches pass through unvalidated — exactly the pre-PR behavior for that path — and catalogDefaultModel() is removed. The same explicit-pair rule covers resume (resuming without --model continues on the thread's own model, which the config default also misrepresents).
Independent review of the previous commit also surfaced a broker-transport hazard armed by the 3s validation timeout: a model/list that fails after the client stopped waiting hit the broker's generic catch, which cleared activeStreamSocket for the in-flight turn on the same socket — its turn/completed was then dropped and the client hung indefinitely, for fully supported pairs. Same commit removes that clearing for non-streaming failures (streaming-failure behavior unchanged) and adds a model-list-slow-error fixture + no-hang regression test.
Three regression tests were verified red on ef1d739 before the fixes; suite is 100/100.
…nership on request failures Three bug fixes, no behavior changes beyond them: 1. Effort validation now applies ONLY when both --model and --effort are explicit. The previous commit resolved effort-only dispatches via the catalog's global isDefault entry, but thread/start sends model:null and the server applies the config.toml default — a different model, so documented config-default flows could be falsely rejected (review comment on ef1d739). Effort-only dispatches now pass through unvalidated, exactly as before this PR. The same explicit-pair rule covers resume: resuming without --model continues on the thread's own model, and validating the config default against it falsely rejected e.g. resume-at-ultra after a successful ultra run. 2. The broker no longer clears activeStreamSocket when a NON-streaming request fails. A model/list that failed later than the client's 3s validation timeout stripped stream ownership from the in-flight turn on the same socket; its turn/completed notifications were then dropped and the client hung forever — reproducing openai/codex#31552 for fully supported pairs. Streaming-failure behavior is unchanged (the block never fired for streaming requests). 3. catalogDefaultModel() (the global-default guess) is removed. Regression tests (all red on the previous commit, green now): - effort-only dispatch forwards with no guessed model injected - resume-last with an explicit effort no longer validates against the config default - a slow-failing model/list no longer kills the turn's event stream on the broker transport (fixture gains model-list-slow-error; test run helper gains a timeout so a regression fails instead of hanging CI) Suite: 100/100. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7f47ee955
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // whatever the server resolves (the config.toml default on fresh dispatches, the | ||
| // thread's own model on resume) — the catalog's global isDefault entry is NOT that | ||
| // model, so guessing here falsely rejects documented flows. | ||
| const shouldValidateEffort = Boolean(options.effort) && Boolean(options.model); |
There was a problem hiding this comment.
For task --resume/--resume-last --effort ultra without --model, shouldValidateEffort is false, so the resume path skips the catalog check even though thread/resume returns the resolved model before turn/start. Resuming an older gpt-5.4/mini thread with max or ultra will therefore send an unsupported effort to turn/start, reintroducing the same pre-turn hang/error this validation was added to avoid; the fresh-dispatch config-default case needs special handling, but the resumed model is available here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked against the live app-server (codex-cli 0.142.4) before acting, because this hinges on what thread/resume actually reports. Probe: started a thread on gpt-5.4-mini (config default is a 5.6-family model), ran one turn, then thread/resume with model: null:
RESUME model:null -> response.model = "gpt-5.6-sol" // config default
(thread ran on gpt-5.4-mini)
thread/resume reports the config default, not the model the thread runs on — the test fixture models this correctly. Validating an effort-only resume against response.model therefore falsely rejects the headline flow (dispatch --model gpt-5.6-sol --effort ultra, then --resume-last --effort ultra fails with Model "gpt-5.4" does not support… whenever the config default is older) — that exact regression was reproduced end-to-end during review of ef1d739 and is pinned by a test.
Since the protocol does not expose the resumed thread's own model pre-turn, there is nothing correct to validate against here; the residual risk (resuming an old small-model thread with max/ultra) is unchanged from pre-PR behavior and belongs to the server-side validation gap tracked in openai/codex#31552. Leaving the resume path as explicit-pair-only.
Found by a pre-push adversarial review: the resume path validated response.model ?? options.model, but turn/start dispatches options.model. When the two diverge (a loaded shared-broker thread reporting prior/config state), validation could falsely reject a valid requested pair or approve an unsupported one, and rejection landed only after thread/resume side effects. Validation now runs before thread/resume against exactly the model the turn will use — symmetric with the fresh path. Regression test drives thread/resume through a fixture behavior that reports a different model than requested (both directions). Red on the previous commit, green now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…efforts A second pre-push review (independent, probe-verified) found that the abandoned model/list request also races the broker's single-slot activeRequestSocket: every completion — success or error — cleared the slot, so a model/list settling while turn/start was in flight on the same socket released ownership early. Turn notifications arriving in the same stdout chunk as the turn/start response are dispatched before the microtask that sets activeStreamSocket, so with the request slot already cleared they were dropped entirely: turn/completed never reached the client (permanent hang, no timeout), a second socket could pass the busy check mid-turn (cross-session serialization bypass), and a dropped turn/started left cancel unable to interrupt the server-side turn. Request ownership is now refcounted per socket and released only when the last in-flight request completes; socket close still clears everything. Regression test: a fixture behavior delays turn/start's response past the late model/list error and flushes the response plus all turn events as one stdout chunk — red (hang, killed by the run timeout) on the previous commit, green now. Docs: three independent reviewers tripped over none/minimal being advertised yet rejected for every cataloged model; the runtime skill now states they are legacy values that current catalogs do not list and will be rejected with an explicit --model. Behavior unchanged. Suite: 102/102. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review Two pre-push review passes (one adversarial, one independent deep review with probes) were run against this branch before pushing the latest two commits; their verified findings are fixed in c13e106 (resume validates the requested model before thread/resume) and 77193b1 (broker request-ownership refcount — the request-slot sibling of the earlier stream-slot fix, plus legacy none/minimal documentation). Each fix carries a regression test proven red on its parent commit. Suite: 102/102. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77193b11b1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| async function fetchModelCatalog(client) { | ||
| const requestPromise = client.request("model/list", {}); |
There was a problem hiding this comment.
When validating an explicit --model/--effort pair, this calls model/list with default params. The Codex app-server docs show model/list returns picker-visible models by default and says to set includeHidden: true for the full list; the response is also paginated with nextCursor (https://developers.openai.com/codex/app-server). For a user who passes a hidden or later-page model with an unsupported effort, we skip validation as “not in the model catalog” and still send turn/start, which reintroduces the unsupported-pair hang this change is trying to prevent. Please request the full catalog and page until the requested model is found or the catalog is exhausted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed against the live app-server before changing anything: default model/list returns 6 of 7 models (the hidden codex-auto-review entry is absent), includeHidden: true returns all 7, and nextCursor is null in every case today (pageSize hints are ignored) — so the hidden-model hole is real now and pagination is future-proofing. Fixed in the latest commit: fetchModelCatalog sends includeHidden: true and follows nextCursor up to 5 pages under the same single 3s deadline; a partial catalog can only fail open (skip + warn) for models on unfetched pages, never falsely reject. Regression test drives a paginated fixture catalog with the target model on page two — red on the parent commit, green now. Suite 103/103.
…fort validation Review follow-up, verified against the live app-server first: default model/list returns only picker-visible models (6 of 7 on codex-cli 0.142.4 — the hidden codex-auto-review entry is absent) and the response carries nextCursor. A hidden or later-page model passed via --model therefore dodged validation as 'not in the model catalog' and the unsupported pair was dispatched anyway. fetchModelCatalog now requests includeHidden: true and follows nextCursor up to 5 pages under the SAME single 3s deadline. A partial catalog can only produce a fail-open skip for models on unfetched pages — never a false rejection. Live probe also confirms today's server returns everything in one page (nextCursor: null), so the loop is a no-op there. Regression test: a paginated fixture catalog puts gpt-5.4-mini on page two; the unsupported pair is now rejected pre-dispatch. Red on the previous commit, green now. Suite 103/103. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Problem
task --effort max/--effort ultrawere rejected by a hardcoded allowlist even thoughcodex debug models(codex-cli 0.142.x) lists both as supported reasoning levels for 5.6-family models. Users could not request the top tiers through the plugin at all; the only workaround wasmodel_reasoning_effortin~/.codex/config.tomlwith--effortomitted. Same drift as #99 (xhigh).Dispatching an unsupported pair is worse than a parse error: the app-server accepts
turn/startand then hangs with zero events (openai/codex#31552) — so validation, not just allowlist extension, is required.What this PR does (7 commits, each with red-first regression tests)
max/ultrain the allowlist; error message derives from the set so it can't drift again; usage/docs updated.--model+--effortpairs, before any thread is created: bounded (3s)model/listwithincludeHidden: true+nextCursorpagination (live default hides models — verified: 6/7 returned without it). Unsupported pairs fail fast with that model's real tiers:Model "gpt-5.4-mini" does not support reasoning effort "ultra". Supported: low, medium, high, xhigh.sparkalias), effort-only dispatches, and resumes without--modelall pass through with a warning — the config default / thread model is resolved server-side andthread/start/thread/resumeresponses report the config default, not the target (verified against the live server), so guessing would falsely reject documented flows. A partial catalog can only skip, never falsely reject.turn/completed→ permanent hang);model/listcompletion can no longer release the slot mid-turn/start(same-chunk notifications were dropped pre-microtask → hang; a second socket could bypass cross-session serialization; a droppedturn/startedbroke cancel).turn/startreceives), beforethread/resumeside effects.none/minimalare legacy values current catalogs don't list and are rejected with an explicit--model.Testing
npm test: 103/103 (clean env). Every behavioral fix has a regression test proven red on its parent commit. Live checks on codex-cli 0.142.4: mini+ultra fails in ~4s with the actionable message (previously: silent indefinite hang); sol+max/ultra dispatch correctly.All automated review threads are resolved (5 fixed, 1 declined with live-protocol evidence in-thread); latest Codex review of
b8134d7reports no issues.🤖 Generated with Claude Code