fix: accept max and ultra reasoning efforts for task --effort #454
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1092,6 +1092,88 @@ export async function importExternalAgentSession(cwd, options = {}) { | |
| }); | ||
| } | ||
|
|
||
| const MODEL_LIST_TIMEOUT_MS = 3000; | ||
| const MODEL_LIST_MAX_PAGES = 5; | ||
|
|
||
| function emitEffortValidationSkip(onProgress, effort, reason) { | ||
| emitProgress( | ||
| onProgress, | ||
| `Warning: skipping reasoning-effort validation (${reason}); dispatching effort "${effort}" as requested.`, | ||
| "starting" | ||
| ); | ||
| } | ||
|
|
||
| async function fetchModelCatalog(client) { | ||
| // includeHidden: the default model/list only returns picker-visible models, so a | ||
| // hidden model passed via --model would otherwise dodge validation. The catalog is | ||
| // paginated; follow nextCursor under ONE shared deadline. A partial catalog can only | ||
| // cause a fail-open skip for models on unfetched pages — never a false rejection. | ||
| const deadline = Date.now() + MODEL_LIST_TIMEOUT_MS; | ||
| const entries = []; | ||
| let cursor = null; | ||
| for (let page = 0; page < MODEL_LIST_MAX_PAGES; page += 1) { | ||
| const remaining = deadline - Date.now(); | ||
| if (remaining <= 0) { | ||
| break; | ||
| } | ||
| const requestPromise = client.request("model/list", cursor ? { includeHidden: true, cursor } : { includeHidden: true }); | ||
| requestPromise.catch(() => {}); | ||
| let timer; | ||
| let response = null; | ||
| try { | ||
| response = await Promise.race([ | ||
| requestPromise, | ||
| new Promise((resolve) => { | ||
| timer = setTimeout(() => resolve(null), remaining); | ||
| }) | ||
| ]); | ||
| } catch { | ||
| response = null; | ||
| } finally { | ||
| clearTimeout(timer); | ||
| } | ||
| if (!response || !Array.isArray(response.data)) { | ||
| break; | ||
| } | ||
| entries.push(...response.data); | ||
| cursor = response.nextCursor ?? null; | ||
| if (!cursor) { | ||
| break; | ||
| } | ||
| } | ||
| return entries.length > 0 ? entries : null; | ||
| } | ||
|
|
||
| function assertCatalogSupportsEffort(entries, resolvedModel, effort, onProgress) { | ||
| if (!entries) { | ||
| emitEffortValidationSkip(onProgress, effort, "model catalog unavailable or malformed"); | ||
| return; | ||
| } | ||
| if (!resolvedModel) { | ||
| emitEffortValidationSkip(onProgress, effort, "could not resolve the target model"); | ||
| return; | ||
| } | ||
| const entry = entries.find((item) => item && (item.id === resolvedModel || item.model === resolvedModel)); | ||
| if (!entry) { | ||
| emitEffortValidationSkip(onProgress, effort, `model "${resolvedModel}" is not in the model catalog`); | ||
| return; | ||
| } | ||
| const supported = Array.isArray(entry.supportedReasoningEfforts) | ||
| ? entry.supportedReasoningEfforts | ||
| .map((item) => item?.reasoningEffort) | ||
| .filter((value) => typeof value === "string" && value) | ||
| : []; | ||
| if (supported.length === 0) { | ||
| emitEffortValidationSkip(onProgress, effort, `model "${resolvedModel}" reports no reasoning-effort catalog`); | ||
| return; | ||
| } | ||
| if (!supported.includes(effort)) { | ||
| throw new Error( | ||
| `Model "${resolvedModel}" does not support reasoning effort "${effort}". Supported: ${supported.join(", ")}.` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| export async function runAppServerTurn(cwd, options = {}) { | ||
| const availability = getCodexAvailability(cwd); | ||
| if (!availability.available) { | ||
|
|
@@ -1100,8 +1182,21 @@ export async function runAppServerTurn(cwd, options = {}) { | |
|
|
||
| return withAppServer(cwd, async (client) => { | ||
| let threadId; | ||
| // Validate only an explicit --model/--effort pair. Without --model the target is | ||
| // 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 Useful? React with 👍 / 👎.
Author
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
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 |
||
| const catalogEntries = shouldValidateEffort ? await fetchModelCatalog(client) : null; | ||
|
|
||
| if (options.resumeThreadId) { | ||
| // Validate the model that turn/start will actually receive, before thread/resume | ||
| // has any side effect. The resume response's model is NOT that model (it reports | ||
| // the config default / prior thread state), so checking it can both falsely reject | ||
| // a valid requested pair and approve an unsupported one. | ||
| if (shouldValidateEffort) { | ||
| assertCatalogSupportsEffort(catalogEntries, options.model, options.effort, options.onProgress); | ||
| } | ||
| emitProgress(options.onProgress, `Resuming thread ${options.resumeThreadId}.`, "starting"); | ||
| const response = await resumeThread(client, options.resumeThreadId, cwd, { | ||
| model: options.model, | ||
|
|
@@ -1110,6 +1205,9 @@ export async function runAppServerTurn(cwd, options = {}) { | |
| }); | ||
| threadId = response.thread.id; | ||
| } else { | ||
| if (shouldValidateEffort) { | ||
| assertCatalogSupportsEffort(catalogEntries, options.model, options.effort, options.onProgress); | ||
| } | ||
| emitProgress(options.onProgress, "Starting Codex task thread.", "starting"); | ||
| const response = await startThread(client, cwd, { | ||
| model: options.model, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
model/listis available, this catalog check rejects--effort noneand--effort minimalfor the current OpenAI model entries because theirsupportedReasoningEffortslist starts atlow, while this same change still accepts and advertisesnone/minimalinnormalizeReasoningEffortand the task usage. Those values were valid plugin inputs before this commit, so users selecting a documented effort now fail beforeturn/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ef1d739, though not via either suggested option — both have a sharper edge than the mismatch they fix:
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.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.