fix: accept max and ultra reasoning efforts for task --effort #454
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1092,6 +1092,65 @@ export async function importExternalAgentSession(cwd, options = {}) { | |
| }); | ||
| } | ||
|
|
||
| const MODEL_LIST_TIMEOUT_MS = 3000; | ||
|
|
||
| function emitEffortValidationSkip(onProgress, effort, reason) { | ||
| emitProgress( | ||
| onProgress, | ||
| `Warning: skipping reasoning-effort validation (${reason}); dispatching effort "${effort}" as requested.`, | ||
| "starting" | ||
| ); | ||
| } | ||
|
|
||
| async function fetchModelCatalog(client) { | ||
| const requestPromise = client.request("model/list", {}); | ||
| requestPromise.catch(() => {}); | ||
| let timer; | ||
| try { | ||
| const response = await Promise.race([ | ||
| requestPromise, | ||
| new Promise((resolve) => { | ||
| timer = setTimeout(() => resolve(null), MODEL_LIST_TIMEOUT_MS); | ||
| }) | ||
| ]); | ||
| return Array.isArray(response?.data) ? response.data : null; | ||
| } catch { | ||
| return null; | ||
| } finally { | ||
| clearTimeout(timer); | ||
| } | ||
| } | ||
|
|
||
| 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(", ")}.` | ||
| ); | ||
|
Comment on lines
+1170
to
+1173
There was a problem hiding this comment.
When Useful? React with 👍 / 👎.
Author
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:
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. |
||
| } | ||
| } | ||
|
|
||
| export async function runAppServerTurn(cwd, options = {}) { | ||
| const availability = getCodexAvailability(cwd); | ||
| if (!availability.available) { | ||
|
|
@@ -1100,8 +1159,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 +1182,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, | ||
|
|
||
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 validating an explicit
--model/--effortpair, this callsmodel/listwith default params. The Codex app-server docs showmodel/listreturns picker-visible models by default and says to setincludeHidden: truefor the full list; the response is also paginated withnextCursor(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 sendturn/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed against the live app-server before changing anything: default
model/listreturns 6 of 7 models (the hiddencodex-auto-reviewentry is absent),includeHidden: truereturns all 7, andnextCursorisnullin every case today (pageSizehints are ignored) — so the hidden-model hole is real now and pagination is future-proofing. Fixed in the latest commit:fetchModelCatalogsendsincludeHidden: trueand followsnextCursorup 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.