diff --git a/docs-site/src/content/docs/guides/codex-app-models.md b/docs-site/src/content/docs/guides/codex-app-models.md index 209469f41d8..84913591709 100644 --- a/docs-site/src/content/docs/guides/codex-app-models.md +++ b/docs-site/src/content/docs/guides/codex-app-models.md @@ -133,6 +133,29 @@ rows from the effective catalog while compatibility aliases exist, so Desktop ca them by ignoring `visibility`. See [Codex Desktop native-allowlist compatibility](/guides/combos/#codex-desktop-native-allowlist-compatibility) for the command, disable-key semantics, and safety constraints. +### What this means for a disabled native model + +Without a native alias configured, disabling a bare native GPT slug does not remove it from the +catalog. The row stays with `visibility: "hide"`, which `/v1/models` and the dashboard both honour +— they stop listing the model — while Desktop, under the policy above, can keep showing it. So the +model can still be picked in Desktop after you disabled it, and the surfaces disagree about whether +it exists. + +Picking it is not rejected for being disabled. `disabledModels` controls catalog visibility, not +admission, so the request is routed by the ordinary rules as though the model were enabled: the turn +runs on the model you disabled, or fails on whatever path that id resolves to. Either way the +outcome is not the one the toggle implies. + +The row is retained deliberately. It holds the real upstream metadata, so re-enabling the model +restores that metadata instead of a synthesized guess. When you need the row gone outright rather +than hidden, configure a `nativeAlias` combo: while one exists, disabled bare native rows are +omitted from the effective catalog entirely. + +If Codex's `config.toml` pins a root `model` that this proxy does not expose — a disabled model +among them — every new session starts on a model opencodex does not serve. `ocx doctor` reports +that under **Codex default model exposure**, as a warning rather than a failure, and says when it +could not determine the exposed set at all. + ## Integration path `ocx init`, `ocx start`, and `ocx sync` wire the shared Codex config and catalog into the proxy; see diff --git a/src/cli/doctor.ts b/src/cli/doctor.ts index df03ab83843..42fb413e74d 100644 --- a/src/cli/doctor.ts +++ b/src/cli/doctor.ts @@ -13,7 +13,7 @@ import { dirname, join } from "node:path"; import { getConfigDir, getConfigPath, readConfigDiagnostics } from "../config"; import { readPid } from "../config/process-state"; import { probeUncleanExitState } from "./status"; -import { findLiveProxy, type LiveProxy } from "../server/proxy-liveness"; +import { findLiveProxy, probeHostname, type LiveProxy } from "../server/proxy-liveness"; import { BUN_RUNTIME_SOURCES } from "../lib/bun-runtime"; import type { BunRuntimeSource } from "../lib/bun-runtime"; import { maskAccountId } from "../lib/privacy"; @@ -27,6 +27,7 @@ import { probeNativeProfileRecoveryState, resolveNativeProfileContext } from ".. import { NativeProfileError } from "../codex/native-profile-types"; import { collectOrcaCodexHomeDiagnostic, resolveCodexHomeDir as resolveCodexHomeDirImpl, isWslRuntime, listWslWindowsCodexHomes, wslAutomountRoot, type CodexHomeDeps } from "../codex/home"; import { scanCodexAgentRolesWithTomlModelFallback } from "../codex/subagent-model-fallback"; +import { readCatalog, readCodexCatalogPath, readConfiguredDefaultModel } from "../codex/catalog/parsing"; import { diagnoseCodexShim, findCodexOnPath, isWindowsInteropDir, type CodexShimDiagnostic } from "../codex/shim"; import { providerTableString, rootTomlString } from "../codex/injected-marker"; import { countPendingOpencodexHistory } from "../codex/history-provider"; @@ -1040,6 +1041,161 @@ export function chatgptPublicEndpointHint( return "ChatGPT-family requests use the public ChatGPT endpoint through this proxy, in both Pool and Direct modes. Eligible streaming turns dial the ChatGPT websocket transport (the same responses_websockets lane Codex CLI defaults to) and fall back to SSE over HTTP when a turn is not eligible - an unsupported Bun runtime, an oversized create frame, or a proxy route that cannot carry the socket - and local provider pacing can hold a request before it is dispatched at all. This hint classifies configuration only and measures nothing, so upstream queueing is one possible contributor to a slow first output: compare actual transport, pacing, network, and provider observations before concluding. service_tier=priority is a request preference: this backend can echo service_tier \"default\" even on turns it scheduled as priority (#2558), so the echoed response tier in request logs stays an observation with confirmation \"assumed\" and cannot confirm or deny the granted tier."; } +/** + * Bound for the doctor-side `/v1/models` read (#4646). A diagnostic must not hang on a proxy + * that is listening but wedged mid-gather; when the read does not land in time the on-disk + * catalog answers instead, and if that is unreadable too the verdict is "could not determine" + * rather than a guess. + */ +const EXPOSED_MODELS_TIMEOUT_MS = 8000; + +/** + * Whether Codex's pinned default model is one this proxy exposes (#4646). + * + * Three states, not two. Reporting "not exposed" when the exposed set could not be read would + * be a fabricated failure on exactly the installs least able to check it (proxy down, catalog + * never synced), so an unreadable set is its own verdict. + */ +export type DefaultModelExposureStatus = "not_configured" | "exposed" | "not_exposed" | "undeterminable"; + +export interface DefaultModelExposure { + status: DefaultModelExposureStatus; + /** The configured pin, or null when Codex's config.toml has no root `model`. */ + model: string | null; + /** Which surface answered; null when neither could be read. */ + source: "proxy" | "catalog" | null; + detail: string; + action?: string; +} + +/** Exactly the catalog's own `RawEntry` shape, so an on-disk row needs no conversion. */ +type CatalogVisibilityRow = Record; + +export interface DefaultModelExposureDeps { + readConfiguredModelFn?: () => string | null; + /** The live proxy doctor already resolved, or null/absent when none is running. */ + live?: LiveProxy | null; + fetchFn?: typeof fetch; + readCatalogModelsFn?: () => readonly CatalogVisibilityRow[] | null; +} + +/** + * Ids the running proxy advertises, or null when the read did not produce a usable answer. + * + * Null is deliberately indistinguishable across transport failure, a non-200, and a malformed + * body, because every one of them means the same thing to the caller: this surface did not + * answer, ask the next one. The 401 case is real rather than theoretical — `/v1/models` requires + * data-plane admission on a non-loopback bind (`isApiAuthRequired`), and doctor deliberately + * holds no data-plane key, so a remote-bound proxy always falls through to the catalog. + */ +async function fetchExposedModelIds(live: LiveProxy, fetchFn: typeof fetch): Promise | null> { + try { + const res = await fetchFn(`http://${probeHostname(live.hostname)}:${live.port}/v1/models`, { + signal: AbortSignal.timeout(EXPOSED_MODELS_TIMEOUT_MS), + }); + if (!res.ok) return null; + const body = await res.json() as { data?: unknown }; + if (!Array.isArray(body?.data)) return null; + const ids = new Set(); + for (const row of body.data) { + const id = (row as { id?: unknown } | null)?.id; + if (typeof id === "string" && id.length > 0) ids.add(id); + } + return ids; + } catch { + return null; + } +} + +/** Picker-visible catalog slugs, or null when the catalog is absent or unparseable. */ +function catalogExposedModelIds(rows: readonly CatalogVisibilityRow[] | null): Set | null { + if (rows === null) return null; + const ids = new Set(); + for (const row of rows) { + // `visibility: "hide"` rows are retained on purpose (see the native-toggle contract in + // structure/catalog.md); they are exactly the rows a pin must not resolve to. + if (!row || row.visibility !== "list") continue; + const slug = row.slug; + if (typeof slug === "string" && slug.length > 0) ids.add(slug); + } + return ids; +} + +function defaultCatalogModels(): readonly CatalogVisibilityRow[] | null { + const models = readCatalog(readCodexCatalogPath())?.models; + return Array.isArray(models) ? models : null; +} + +/** + * Compare Codex's root `model` pin against the models this install actually exposes (#4646). + * + * The exposed set is read, never recomputed. Reproducing the live assembly in the CLI would mean + * duplicating an entitlements snapshot, a provider gather and account-selector expansion, and the + * duplicate would drift — the same failure `formatStartupRoutingDetail` and `computeVersionSkew` + * were extracted to prevent. So the running proxy answers when there is one, the on-disk catalog + * answers otherwise, and neither is reconstructed here. + * + * Both surfaces are consulted before any negative verdict. They name a routed row through the + * same `/` slug space, but they are built by different code at different times, so + * requiring both to disagree is what keeps an encoding or staleness difference from being + * reported to the operator as a broken pin. + */ +export async function collectDefaultModelExposure( + deps: DefaultModelExposureDeps = {}, +): Promise { + const configured = (deps.readConfiguredModelFn ?? readConfiguredDefaultModel)(); + const model = typeof configured === "string" ? configured.trim() : ""; + if (!model) { + return { + status: "not_configured", + model: null, + source: null, + detail: "Codex config.toml pins no root `model`, so Codex picks from the exposed catalog", + }; + } + + const live = deps.live ?? null; + const proxyIds = live ? await fetchExposedModelIds(live, deps.fetchFn ?? fetch) : null; + const catalogIds = catalogExposedModelIds((deps.readCatalogModelsFn ?? defaultCatalogModels)()); + if (proxyIds === null && catalogIds === null) { + return { + status: "undeterminable", + model, + source: null, + detail: `could not read the exposed model set, so Codex \`model = "${model}"\` was not checked`, + action: "Start the proxy with 'ocx start', or run 'ocx sync' to write the Codex catalog, then re-run 'ocx doctor'", + }; + } + + const source = proxyIds !== null ? "proxy" as const : "catalog" as const; + // `source` reports which surface produced the verdict, so a match names the surface that + // matched rather than the one we happened to read first. + const matched = proxyIds?.has(model) === true + ? "proxy" as const + : catalogIds?.has(model) === true ? "catalog" as const : null; + if (matched !== null) { + return { + status: "exposed", + model, + source: matched, + detail: `Codex \`model = "${model}"\` is exposed by this install`, + }; + } + // Name only the surfaces that actually answered: claiming a check that did not happen is the + // same defect as claiming an exposure verdict we could not reach. + const checked = [ + ...(proxyIds !== null ? ["the running proxy's /v1/models"] : []), + ...(catalogIds !== null ? ["the on-disk Codex catalog"] : []), + ].join(" and "); + return { + status: "not_exposed", + model, + source, + detail: `Codex \`model = "${model}"\` is NOT exposed by this install (checked ${checked}), so every new Codex session starts on a model this proxy does not serve`, + action: "Expose that model (enable it in the dashboard or drop it from 'disabledModels') and run 'ocx sync', or pin an exposed id as 'model' in CODEX_HOME/config.toml", + }; +} + export async function runDoctor(args: string[] = []): Promise { if (args.includes("--fix-codex-runtime")) { const resolved = resolveCodexRuntime(); @@ -1325,6 +1481,26 @@ export async function runDoctor(args: string[] = []): Promise { console.log(line); } + // Adjacent to the section above because both read Codex's config.toml, and an operator + // debugging "Codex config" wants the pinned model checked in the same place. + console.log("\nCodex default model exposure"); + const defaultModelExposure = await collectDefaultModelExposure({ live }); + if (defaultModelExposure.status === "not_exposed") { + console.log(` !! ${defaultModelExposure.detail}`); + console.log(` Action: ${defaultModelExposure.action}`); + } else if (defaultModelExposure.status === "undeterminable") { + // Not `!!`: nothing is known to be wrong. The one thing this must never do is report an + // unread set as a broken pin. + console.log(` -- ${defaultModelExposure.detail}`); + console.log(` Action: ${defaultModelExposure.action}`); + } else { + console.log(` ok ${defaultModelExposure.detail}`); + } + // Deliberately no `recordDoctorFailure()` and no `process.exitCode` write. A pin that is not + // exposed is a degraded install, not an unusable one — the operator can still pick another + // model in the session — and the rule above reserves FAIL for an unusable surface so a warning + // cannot break a legitimately green pipeline. + console.log("\nCodex agent role files"); const tomlFallbackRoles = scanCodexAgentRolesWithTomlModelFallback(resolveCodexHomeDirImpl()); if (tomlFallbackRoles.length === 0) { @@ -1397,6 +1573,12 @@ export async function runDoctor(args: string[] = []): Promise { hints.push(`${row.detail}. Set ${row.envName} in the shell that starts the proxy, or store a literal key in config (value hidden here).`); } if (codexEnvKeyReadiness) hints.push(`${codexEnvKeyReadiness.detail}. ${codexEnvKeyReadiness.action}.`); + // Only the negative verdict becomes a hint. "Could not determine" is usually just a proxy that + // is not running, which `proxyDownRestartHint` already reports; repeating it here would put a + // second line in the hint list for one fact. + if (defaultModelExposure.status === "not_exposed") { + hints.push(`${defaultModelExposure.detail}. ${defaultModelExposure.action}.`); + } const anyDrvfs = paths.some(p => detectFsType(p.path, mounts).isDrvfs || detectFsType(p.path, mounts).isMntDrive); const noProxy = currentProxyEnv.every(p => !p.present) && !configuredProxy.present; if (!startup.rebootSafe) { diff --git a/src/codex/catalog/parsing.ts b/src/codex/catalog/parsing.ts index e1629a0ed58..378e664ed93 100644 --- a/src/codex/catalog/parsing.ts +++ b/src/codex/catalog/parsing.ts @@ -284,6 +284,30 @@ export function readConfiguredAutoReviewModel(): string | null { return null; } +/** + * Read the root `model` pin from Codex's config.toml (issue #4646). + * + * Codex starts every new session on this id, and nothing in opencodex checks that the id is one + * the proxy actually exposes: the pin lives in Codex's config, while exposure is decided here by + * `disabledModels`, provider `selectedModels`, and account entitlements. When the two disagree + * every turn fails and no surface says why, which is what the `ocx doctor` section added for + * #4646 reports. + * + * Read-only, and deliberately the same shape and the same swallow-and-return-null error policy as + * `readConfiguredAutoReviewModel` above: a diagnostic must degrade to "unknown" on an unreadable + * or absent config rather than throw out of the surface that called it. + */ +export function readConfiguredDefaultModel(): string | null { + try { + const configPath = activeCodexConfigPath(); + if (existsSync(configPath)) { + const toml = readFileSync(configPath, "utf-8"); + return readRootTomlString(toml, "model"); + } + } catch { /* ignore */ } + return null; +} + export function parseCatalogJson(raw: string): RawCatalog | null { try { const cat = JSON.parse(raw); diff --git a/structure/catalog.md b/structure/catalog.md index 4926f349171..81285327804 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -117,6 +117,20 @@ native alias also omits disabled bare native rows from the effective catalog. Da derived from the static native set, and sync retains bundled/pristine native recovery sources so a later re-enable or alias removal restores native metadata. +Without such an alias, a disabled bare native keeps a `visibility: "hide"` row, and that retention +has an operator-visible consequence. `visibleNativeSlugs` in `src/codex/catalog/metadata.ts` drops +the slug from `/v1/models` and the dashboard while `applyNativeVisibility` keeps the catalog row, so +a renderer that ignores `visibility` can still offer a model every other surface calls disabled. +Selecting it is not refused: `disabledModels` is a catalog control, and `src/router.ts` never +consults it, so the turn resolves by the ordinary routing rules instead of failing as disabled. +Retention is the deliberate trade — it preserves real upstream metadata for a later re-enable +rather than synthesizing a guess — and a `nativeAlias` combo is the lever that omits the row +outright. + +Nothing in the catalog validates Codex's own root `model` pin against this exposed set; +`readConfiguredDefaultModel` in `src/codex/catalog/parsing.ts` reads the pin, and `ocx doctor` +reports it (see [Runtime](runtime.md)). + Provider live-model lists are cached with a configured TTL (`src/codex/model-cache.ts`). Adding, deleting, or editing a provider's shape clears that per-provider cache; a disabled-only change deliberately does not, because a disabled provider is already excluded from the catalog gather diff --git a/structure/runtime.md b/structure/runtime.md index ed2bffbf75f..7907edd4f0b 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -72,6 +72,7 @@ The prefilter is only an optimization, not final process-membership authority. | `src/config/process-state.ts` | Owns `ocx.pid`, `runtime-port.json`, cheap liveness, full command-line identity verification, and snapshot-guarded cleanup. | | `src/server/ports.ts` | Owns bind availability and ephemeral-port selection. Temporary probes dispose accepted peers and wait for listener close before reporting success. | | `src/cli/status.ts` / `src/cli/status-probes.ts` | Status snapshot assembly and the shared read-only health/stale-process probes used by status and doctor. Probe evidence keeps recorded-port choice, before/after snapshots and per-call timer cleanup together. | +| `src/cli/doctor.ts` | Read-only environment diagnostics. Sections print through `console.log`; each is a `collect*` helper above `runDoctor` so it is testable without the command. Only a `FAIL`-level condition records a doctor failure — a degraded-but-working install must not break a green pipeline. `collectDefaultModelExposure` compares Codex's root `model` pin against the exposed set, which it READS rather than recomputes: the running proxy's `/v1/models` when one answers, otherwise the on-disk catalog's `visibility: "list"` slugs. It reports exposed, not exposed, or undeterminable, and never the second when it could not read either surface. | | `src/router.ts` | Provider/model selection before adapter dispatch. Policy execution and ordinary management dry-run share effective-provider capability evidence; unresolved, missing, and disabled providers are excluded before scoring. | | `src/providers/api-key-selection-capture.ts` | Pure request-owned snapshot of the configured key entry, reference, and revision. The router and stateful selection module share this leaf with type-only dependencies; `api-key-selection.ts` retains the compatibility export and owns persisted selection changes and route resolution. | | `src/types.ts` | Shared config, parsed request, adapter, and event types. | diff --git a/tests/codex-integration/doctor.test.ts b/tests/codex-integration/doctor.test.ts index 56ad8c1a968..fe07a6a797c 100644 --- a/tests/codex-integration/doctor.test.ts +++ b/tests/codex-integration/doctor.test.ts @@ -10,6 +10,7 @@ import { collectPaths, detectFsType, collectConfiguredProxy, + collectDefaultModelExposure, collectProxyEnv, collectRunningProxyEnv, chatgptPublicEndpointHint, @@ -1013,3 +1014,95 @@ describe("doctor reports an unclean prior proxy exit", () => { expect(output).toContain("assumed"); }); }); + +// #4646: Codex pins a default model in its own config.toml, and nothing compared that pin +// against the models this install exposes. Every dependency is injected here, so these cases +// touch neither the real `CODEX_HOME` nor the network. +describe("doctor Codex default model exposure (#4646)", () => { + const live = { pid: 4321, port: 10100, source: "config" as const }; + const respondWith = (body: unknown, status = 200) => ( + (async () => new Response(JSON.stringify(body), { status })) as unknown as typeof fetch + ); + + test("no root model pin is not a finding", async () => { + const result = await collectDefaultModelExposure({ + readConfiguredModelFn: () => null, + readCatalogModelsFn: () => [{ slug: "gpt-5.6-sol", visibility: "list" }], + }); + + expect(result.status).toBe("not_configured"); + expect(result.model).toBeNull(); + expect(result.action).toBeUndefined(); + }); + + test("a pin the running proxy advertises is exposed, and names the proxy as the source", async () => { + const result = await collectDefaultModelExposure({ + readConfiguredModelFn: () => " gpt-5.6-sol ", + live, + fetchFn: respondWith({ data: [{ id: "gpt-5.6-sol" }, { id: "kiro/claude-opus-4.6" }] }), + readCatalogModelsFn: () => null, + }); + + expect(result.status).toBe("exposed"); + // Trimmed: a pin written with surrounding whitespace is the same pin. + expect(result.model).toBe("gpt-5.6-sol"); + expect(result.source).toBe("proxy"); + }); + + test("a pin missing from every readable surface is the warning, and says which surfaces it read", async () => { + const result = await collectDefaultModelExposure({ + readConfiguredModelFn: () => "kiro/claude-opus-4.6", + live, + fetchFn: respondWith({ data: [{ id: "gpt-5.6-sol" }] }), + readCatalogModelsFn: () => [{ slug: "gpt-5.6-sol", visibility: "list" }], + }); + + expect(result.status).toBe("not_exposed"); + expect(result.detail).toContain("kiro/claude-opus-4.6"); + expect(result.detail).toContain("/v1/models"); + expect(result.detail).toContain("on-disk Codex catalog"); + expect(result.action).toBeDefined(); + }); + + test("an unreadable exposed set is undeterminable, never 'not exposed'", async () => { + const result = await collectDefaultModelExposure({ + readConfiguredModelFn: () => "gpt-5.6-sol", + live: null, + readCatalogModelsFn: () => null, + }); + + expect(result.status).toBe("undeterminable"); + expect(result.source).toBeNull(); + expect(result.detail).not.toContain("NOT exposed"); + expect(result.action).toContain("ocx start"); + }); + + test("a proxy that refuses the read falls back to the catalog instead of guessing", async () => { + const result = await collectDefaultModelExposure({ + readConfiguredModelFn: () => "gpt-5.6-sol", + live, + // What a non-loopback bind returns to doctor, which holds no data-plane key. + fetchFn: respondWith({ error: "opencodex API key required" }, 401), + readCatalogModelsFn: () => [{ slug: "gpt-5.6-sol", visibility: "list" }], + }); + + expect(result.status).toBe("exposed"); + expect(result.source).toBe("catalog"); + }); + + test("a retained hide row is not exposure: the pin Desktop can still show is still reported", async () => { + const result = await collectDefaultModelExposure({ + readConfiguredModelFn: () => "gpt-5.6-terra", + live: null, + // Exactly the shape a disabled bare native leaves behind (see native-model-toggle.test.ts). + readCatalogModelsFn: () => [ + { slug: "gpt-5.6-terra", visibility: "hide" }, + { slug: "gpt-5.6-sol", visibility: "list" }, + ], + }); + + expect(result.status).toBe("not_exposed"); + expect(result.source).toBe("catalog"); + expect(result.detail).not.toContain("/v1/models"); + }); +}); diff --git a/tests/codex-integration/native-model-toggle.test.ts b/tests/codex-integration/native-model-toggle.test.ts index 7e90a6f05c8..6981a002c12 100644 --- a/tests/codex-integration/native-model-toggle.test.ts +++ b/tests/codex-integration/native-model-toggle.test.ts @@ -352,6 +352,39 @@ describe("native GPT model toggles (bare slugs in disabledModels)", () => { }))).toEqual(new Set()); }); + // #4646 asked for disabled native slugs to be omitted outright. They are retained as + // `visibility: "hide"` on purpose, and this pins the whole shape of that contract in one + // place, because it is what the operator-facing note in + // docs-site/.../codex-app-models.md and structure/catalog.md now describe: the row is gone + // from the availability list, still present in the catalog so a later re-enable restores real + // upstream metadata, and omitted outright only once a native-alias combo exists. + test("without a native alias a disabled native is hidden-but-retained; the alias is what omits it", () => { + const disabled = { disabledModels: ["gpt-5.6-terra"] }; + // Not vacuous: terra is a candidate native on both the live-catalog and fallback paths + // (NATIVE_OPENAI_MODELS and DOCUMENTED_NATIVE_OPENAI_ADDITIONS both carry it). + expect(visibleNativeSlugs({ disabledModels: [] })).toContain("gpt-5.6-terra"); + expect(visibleNativeSlugs(disabled)).not.toContain("gpt-5.6-terra"); + expect(desktopAllowlistSuppressedNativeSlugs(disabled)).toEqual(new Set()); + + const entries = [{ slug: "gpt-5.6-terra", visibility: "list" }, { slug: "gpt-5.6-sol", visibility: "list" }]; + applyNativeVisibility(entries, new Set(disabled.disabledModels)); + // Retained rather than dropped: the row survives with its upstream metadata. + expect(entries.find(entry => entry.slug === "gpt-5.6-terra")?.visibility).toBe("hide"); + expect(entries.find(entry => entry.slug === "gpt-5.6-sol")?.visibility).toBe("list"); + + expect(desktopAllowlistSuppressedNativeSlugs(makeConfig({ + ...disabled, + combos: { + nova: { + alias: "gpt-5.6-sol", + nativeAlias: true, + displayName: "Nova1 - Sol", + targets: [{ provider: "nova", model: "codex/gpt-5.6-sol" }], + }, + }, + })).has("gpt-5.6-terra")).toBe(true); + }); + test("configured public selectors replace bare picker rows with account-qualified native clones", () => { const template = nativeTemplate(); template.comp_hash = "native-compaction-hash";