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,70 @@ 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 catalogDefaultModel(entries) { | ||
| const entry = Array.isArray(entries) ? entries.find((item) => item && item.isDefault) : null; | ||
| return entry ? entry.id ?? entry.model ?? null : 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(", ")}.` | ||
| ); | ||
|
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,6 +1164,7 @@ export async function runAppServerTurn(cwd, options = {}) { | |
|
|
||
| return withAppServer(cwd, async (client) => { | ||
| let threadId; | ||
| const catalogEntries = options.effort ? await fetchModelCatalog(client) : null; | ||
|
|
||
| if (options.resumeThreadId) { | ||
| emitProgress(options.onProgress, `Resuming thread ${options.resumeThreadId}.`, "starting"); | ||
|
|
@@ -1109,7 +1174,14 @@ export async function runAppServerTurn(cwd, options = {}) { | |
| ephemeral: false | ||
| }); | ||
| threadId = response.thread.id; | ||
| if (options.effort) { | ||
| assertCatalogSupportsEffort(catalogEntries, response.model ?? null, options.effort, options.onProgress); | ||
| } | ||
| } 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 Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Confirmed — the catalog's global Independent review of the previous commit also surfaced a broker-transport hazard armed by the 3s validation timeout: a Three regression tests were verified red on ef1d739 before the fixes; suite is 100/100. |
||
| assertCatalogSupportsEffort(catalogEntries, targetModel, 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.