diff --git a/devlog/_plan/260911_l4_service_cli/030_wp3_client_catalog.md b/devlog/_plan/260911_l4_service_cli/030_wp3_client_catalog.md new file mode 100644 index 0000000000..be2d964772 --- /dev/null +++ b/devlog/_plan/260911_l4_service_cli/030_wp3_client_catalog.md @@ -0,0 +1,66 @@ +# wp3 — #4207: connected catalog reports success while the local Codex CLI rejects it + +Work-phase 3 of the L4 lane, stacked on wp2. No carried PR: this issue had none. + +## The gap + +Subagent Bernoulli mapped the client path. `connectClient` downloads at `connect.ts:542` and +writes the hub's bytes verbatim at `:545-549`; `syncConnectedClient` writes the same way at +`:667`. The only validation in between is `validateRemoteCatalog` +(`hub-client.ts:145`), which checks JSON shape — object, `models` array, unique non-empty +slugs — and nothing about reasoning levels. `src/client` never imports the effort clamp. + +So the connection state proves the hub is reachable and the credential works, and is then +reported as readiness. The reporter's Codex CLI 0.135.0 exited before its first request on +`unknown variant \`max\``, while `ocx connect status --json` said `connected` with the catalog +present. `ocx status` even reported an active effort clamp for that same older runtime: the +local machinery already knew the ladder, and the client path simply never consulted it. + +## Decision + +The packet records it: **fail closed — block local readiness rather than reporting success.** +Not a locally clamped projection, which would make the client silently disagree with hub truth. + +## Shape + +- `catalogEffortCompatibility(models, supported)` in `src/codex/catalog/effort.ts` — pure, no + mutation, reports the rejected efforts and the models carrying them. It sits beside + `clampCatalogModelsToObservedCodexSupport`, which mutates; that is correct for a file this + process owns and wrong for one that must keep matching the hub. +- `src/client/catalog-compatibility.ts` — assesses a downloaded body against the observed local + ladder and throws `ClientCatalogIncompatibleError` when it cannot be consumed. +- Both hub-download writes are gated **before** the write. Refusing before the write is stronger + than writing and restoring: there is no window in which an unparseable catalog exists on disk, + and `writtenCatalogFingerprint` stays null so the existing rollback correctly does nothing. +- The two restore paths (`connect.ts:126`, `:729`) are deliberately **not** gated. Refusing to + restore a catalog this machine already accepted would strand the client with none at all. + +## Decisions I had to make + +**An unobservable ladder does not block.** `codexSupportedReasoningEfforts` returns null when +`codex debug models --bundled` cannot be observed. That is not evidence of incompatibility, and a +client machine may legitimately have no Codex CLI. I read the issue's *"preserve the prior +known-good catalog if compatibility cannot be established"* as the incompatible branch — the +alternative to the compatible-projection branch offered in the same sentence — not as the +inconclusive one. Recorded in the PR body too, because the other reading is defensible. + +**An invented command was caught before it shipped.** The first draft of the refusal recommended +`ocx codex-runtime`, which does not exist. `AGENTS.md` records this exact failure mode — a +documented `ocx request-history` that never existed — so every command in the message was +checked against the CLI registry. It now names `CODEX_CLI_PATH` and `ocx sync`, with `ocx doctor` +for diagnosis, matching `doctor.ts`'s existing advice. + +## Audit + +Bohr reviewed the diff adversarially and returned `SAFE_TO_PUSH`: no strict-tsc failure on the +new `src/` lines (checked by hand, since typecheck was NOT RUN), no import cycle into +`src/client` and no module-load side effect, Lab boundary untouched, all four +`atomicWriteFile(DEFAULT_CATALOG_PATH, …)` sites classified, and the gate proven to precede +`commitClientConnection`. One nit folded: a test title claimed write ordering that only the +source-scan test actually asserts, and was renamed. + +## Not run + +`bun test`, `bun run test`, `bun run test:changed`, `bun run typecheck`, `bun run build:gui` +and `bun install` are NOT RUN by operator instruction. Hosted CI on the exact pushed head is +the only product evidence this round accepts. diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 135aeaba5c..83057fdd24 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -360,6 +360,7 @@ "cli-transport-honesty.test.ts": "cli", "cli-usage-report.test.ts": "cli", "cli-version-skew.test.ts": "cli", + "client-catalog-compatibility.test.ts": "clients", "client-config-export-new-clients.test.ts": "config", "client-config-export.test.ts": "config", "client-config-new-clients.test.ts": "config", diff --git a/src/client/catalog-compatibility.ts b/src/client/catalog-compatibility.ts new file mode 100644 index 0000000000..8b4b9011cf --- /dev/null +++ b/src/client/catalog-compatibility.ts @@ -0,0 +1,107 @@ +/** + * #4207: a connected client reported `connected` with a present, freshly synced catalog while + * its installed Codex CLI exited before making a request, because the hub's catalog contained a + * reasoning level that CLI does not know: + * + * failed to parse model_catalog_json ... unknown variant `max`, + * expected one of `none`, `minimal`, `low`, `medium`, `high`, `xhigh` + * + * The connection state answered a different question from the one the operator was asking. It + * proved the hub was reachable and the credential worked; it never proved the selected local + * runtime could consume what was downloaded. This module supplies the missing half, and the + * connect path fails closed on it: an incompatible catalog is refused before it is written, so + * the previous known-good file survives and no success is reported. + * + * What it deliberately does not do: rewrite the hub's catalog into a locally compatible + * projection (the client would then silently disagree with hub truth) and terminate running + * Codex processes. Both are ruled out by the issue. + */ +import { catalogEffortCompatibility, codexSupportedReasoningEfforts } from "../codex/catalog/effort"; +import type { RawEntry } from "../codex/catalog/parsing"; + +export type ClientCatalogCompatibility = + | { kind: "compatible" } + /** The runtime ladder could not be observed, so incompatibility cannot be established. */ + | { kind: "unverified"; reason: string } + | { + kind: "incompatible"; + unsupportedEfforts: readonly string[]; + affectedModels: readonly string[]; + }; + +export interface CatalogCompatibilityDeps { + /** Injected in tests; defaults to observing the selected local Codex runtime. */ + supportedEfforts?: () => ReadonlySet | null; +} + +function parseModels(body: string): RawEntry[] | null { + try { + const parsed = JSON.parse(body) as { models?: unknown }; + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return null; + return Array.isArray(parsed.models) ? parsed.models as RawEntry[] : []; + } catch { + return null; + } +} + +/** + * Assess a downloaded catalog against the reasoning efforts the selected local Codex runtime + * accepts. Unreadable bytes are reported as unverified rather than incompatible: the hub + * client already rejects a malformed body, and inventing a second cause for it here would + * repeat the mistake #4169 was filed for. + */ +export function assessClientCatalogCompatibility( + body: string, + deps: CatalogCompatibilityDeps = {}, +): ClientCatalogCompatibility { + const models = parseModels(body); + if (!models) return { kind: "unverified", reason: "the downloaded catalog could not be read" }; + const supported = (deps.supportedEfforts ?? (() => codexSupportedReasoningEfforts()))(); + if (!supported) { + return { + kind: "unverified", + reason: "the selected local Codex runtime did not report the reasoning levels it supports", + }; + } + const result = catalogEffortCompatibility(models, supported); + if (result.compatible) return { kind: "compatible" }; + return { + kind: "incompatible", + unsupportedEfforts: result.unsupportedEfforts, + affectedModels: result.affectedModels, + }; +} + +/** + * Raised instead of writing an incompatible catalog. It names both remedies the issue asks + * for, because the operator cannot act on "incompatible" alone, and it never suggests editing + * the hub. + */ +export class ClientCatalogIncompatibleError extends Error { + readonly unsupportedEfforts: readonly string[]; + readonly affectedModels: readonly string[]; + + constructor(unsupportedEfforts: readonly string[], affectedModels: readonly string[]) { + const efforts = unsupportedEfforts.join(", "); + const models = affectedModels.length > 3 + ? `${affectedModels.slice(0, 3).join(", ")} and ${affectedModels.length - 3} more` + : affectedModels.join(", "); + super( + `catalog_incompatible: the hub catalog uses reasoning ${unsupportedEfforts.length === 1 ? "level" : "levels"} ` + + `${efforts}, which the selected local Codex CLI rejects${models ? ` (${models})` : ""}. ` + + "The previous catalog was kept and nothing was changed. Upgrade the Codex CLI to a " + + "version that supports those levels, or point CODEX_CLI_PATH at one that does and run " + + "`ocx sync`, then retry. `ocx doctor` reports which runtime is selected.", + ); + this.name = "ClientCatalogIncompatibleError"; + this.unsupportedEfforts = unsupportedEfforts; + this.affectedModels = affectedModels; + } +} + +/** Fail closed: refuse an incompatible catalog before anything is written. */ +export function assertClientCatalogCompatible(body: string, deps: CatalogCompatibilityDeps = {}): void { + const assessment = assessClientCatalogCompatibility(body, deps); + if (assessment.kind !== "incompatible") return; + throw new ClientCatalogIncompatibleError(assessment.unsupportedEfforts, assessment.affectedModels); +} diff --git a/src/client/connect.ts b/src/client/connect.ts index 2b0e8efe08..3569f83c83 100644 --- a/src/client/connect.ts +++ b/src/client/connect.ts @@ -67,6 +67,7 @@ import { readClientConnectionState, assertNoClientDisconnectPending, assertClientConnectionUnchanged, sameClientConnectionOwner, } from "./state"; +import { assertClientCatalogCompatible, type CatalogCompatibilityDeps } from "./catalog-compatibility"; class RotationRecoveryRequiredError extends Error { constructor(message: string, options?: ErrorOptions) { @@ -89,6 +90,7 @@ export interface ClientConnectDeps { fetchImpl?: typeof fetch; now?: () => Date; lifecycleLockDeps?: ClientLifecycleLockDeps; + catalogCompatibility?: CatalogCompatibilityDeps; } export interface RotateClientOptions { @@ -543,6 +545,12 @@ export async function connectClient( fetchImpl: deps.fetchImpl, timeoutMs: options.catalogTimeoutMs, }); + // Fail closed BEFORE the write (#4207). The hub being reachable and the credential working + // does not mean the selected local Codex runtime can consume what arrived: an older CLI + // exits on an unknown reasoning level before making a single request, while connect + // reports success. Refusing here leaves the previous catalog in place untouched, rather + // than writing one and restoring it afterwards. + assertClientCatalogCompatible(catalog.body, deps.catalogCompatibility); writtenCatalogFingerprint = withClientLifecycleSync(() => withConfigMutationLockSync(() => { assertConnectingState(persisted.fingerprint); atomicWriteFile(DEFAULT_CATALOG_PATH, catalog.body); @@ -659,6 +667,10 @@ export async function syncConnectedClient( if (!transient) throw error; stale = true; } + // Same gate as connect (#4207): a sync must never replace a catalog the local CLI can parse + // with one it cannot. Refusing leaves the connection and the existing catalog exactly as + // they were, which is the known-good state. + if (downloaded) assertClientCatalogCompatible(downloaded.body, deps.catalogCompatibility); const next = withClientLifecycleSync(() => withConfigMutationLockSync(() => { assertClientConnectionUnchanged(initial.connection); const token = readServiceApiTokenState(); diff --git a/src/codex/catalog/effort.ts b/src/codex/catalog/effort.ts index 491518cda6..ba324f12a3 100644 --- a/src/codex/catalog/effort.ts +++ b/src/codex/catalog/effort.ts @@ -389,6 +389,47 @@ export interface ObservedCatalogEffortClamp { readonly affectedModels: readonly string[]; } +export interface CatalogEffortCompatibility { + readonly compatible: boolean; + readonly unsupportedEfforts: readonly string[]; + readonly affectedModels: readonly string[]; +} + +/** + * Report which reasoning efforts in a catalog the local Codex runtime would reject, without + * changing anything. + * + * The clamp above is mutate-and-continue, which is right when this process owns the file it + * is about to write. It is wrong for a catalog downloaded from a hub: rewriting it locally + * would make the client disagree with hub truth, and #4207 asks for the opposite — establish + * compatibility first, and refuse rather than materialise a catalog the local CLI cannot + * parse. `supported` of null means the runtime ladder could not be observed, which is not + * evidence of incompatibility, so nothing is reported. + */ +export function catalogEffortCompatibility( + models: readonly RawEntry[], + supported: ReadonlySet | null, +): CatalogEffortCompatibility { + if (!supported) return { compatible: true, unsupportedEfforts: [], affectedModels: [] }; + const unsupported = new Set(); + const affected: string[] = []; + for (const entry of models) { + const rejected = catalogEntryEfforts(entry).filter(effort => !supported.has(effort)); + const fallback = typeof entry.default_reasoning_level === "string" + && !supported.has(entry.default_reasoning_level) + ? [entry.default_reasoning_level] + : []; + if (rejected.length === 0 && fallback.length === 0) continue; + for (const effort of [...rejected, ...fallback]) unsupported.add(effort); + if (typeof entry.slug === "string") affected.push(entry.slug); + } + return { + compatible: unsupported.size === 0, + unsupportedEfforts: [...unsupported].sort(), + affectedModels: affected, + }; +} + /** Apply an already-observed runtime ladder without probing, logging, or writing diagnostics. */ export function clampCatalogModelsToObservedCodexSupport( models: RawEntry[], diff --git a/tests/clients/client-catalog-compatibility.test.ts b/tests/clients/client-catalog-compatibility.test.ts new file mode 100644 index 0000000000..f01f43d071 --- /dev/null +++ b/tests/clients/client-catalog-compatibility.test.ts @@ -0,0 +1,139 @@ +/** + * #4207: `ocx connect status` reported connected, catalog present and freshly synced, while + * the installed Codex CLI exited before its first request because the downloaded catalog used + * a reasoning level it does not know. Connection state proved the hub and the credential; it + * never proved the selected local runtime could consume what was written. + * + * The gate fails closed: an incompatible catalog is refused before the write, so the previous + * known-good file survives. It does not rewrite the hub's catalog into a local projection and + * it does not touch running Codex processes. + */ +import { describe, expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { catalogEffortCompatibility } from "../../src/codex/catalog/effort"; +import { + assertClientCatalogCompatible, + assessClientCatalogCompatibility, + ClientCatalogIncompatibleError, +} from "../../src/client/catalog-compatibility"; +import { repoPath } from "../helpers/repo-root"; + +/** The shape the hub publishes: a model row with a reasoning ladder. */ +function catalogBody(levels: string[], slug = "gpt-5.6-sol", defaultLevel?: string): string { + return JSON.stringify({ + models: [{ + slug, + supported_reasoning_levels: levels.map(effort => ({ effort })), + ...(defaultLevel ? { default_reasoning_level: defaultLevel } : {}), + }], + }); +} + +/** Codex CLI 0.135.0's ladder, verbatim from the parse error in the issue. */ +const OLD_CLI = new Set(["none", "minimal", "low", "medium", "high", "xhigh"]); +const NEW_CLI = new Set([...OLD_CLI, "max", "ultra"]); + +describe("#4207 catalog effort compatibility", () => { + test("reports the levels an older runtime would reject, without changing the catalog", () => { + const models = JSON.parse(catalogBody(["low", "high", "max"])).models; + const before = JSON.stringify(models); + + const result = catalogEffortCompatibility(models, OLD_CLI); + + expect(result.compatible).toBe(false); + expect(result.unsupportedEfforts).toEqual(["max"]); + expect(result.affectedModels).toEqual(["gpt-5.6-sol"]); + // The clamp beside it mutates; this one must not, or the client would silently disagree + // with hub truth. + expect(JSON.stringify(models)).toBe(before); + }); + + test("a default level the runtime does not know is an incompatibility too", () => { + // The CLI parses default_reasoning_level with the same enum, so a ladder that survives + // the filter can still fail on the default alone. + const models = JSON.parse(catalogBody(["low", "high"], "gpt-5.6-sol", "ultra")).models; + const result = catalogEffortCompatibility(models, OLD_CLI); + expect(result.compatible).toBe(false); + expect(result.unsupportedEfforts).toEqual(["ultra"]); + }); + + test("a catalog the runtime fully supports is compatible", () => { + const models = JSON.parse(catalogBody(["low", "high", "max"])).models; + expect(catalogEffortCompatibility(models, NEW_CLI)).toEqual({ + compatible: true, + unsupportedEfforts: [], + affectedModels: [], + }); + }); + + test("an unobservable runtime ladder is not evidence of incompatibility", () => { + const models = JSON.parse(catalogBody(["low", "max"])).models; + expect(catalogEffortCompatibility(models, null).compatible).toBe(true); + }); +}); + +describe("#4207 client catalog gate", () => { + test("the exact ladder from the report is refused", () => { + const body = catalogBody(["low", "medium", "high", "xhigh", "max"]); + + expect(() => assertClientCatalogCompatible(body, { supportedEfforts: () => OLD_CLI })) + .toThrow(ClientCatalogIncompatibleError); + }); + + test("the refusal names the level, both remedies, and what was preserved", () => { + let thrown: unknown; + try { + assertClientCatalogCompatible(catalogBody(["low", "max"]), { supportedEfforts: () => OLD_CLI }); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(ClientCatalogIncompatibleError); + const message = (thrown as Error).message; + expect(message).toContain("max"); + // "Incompatible" alone is not actionable: the operator has to know which way out exists. + expect(message).toContain("Upgrade the Codex CLI"); + expect(message).toContain("CODEX_CLI_PATH"); + expect(message).toContain("The previous catalog was kept"); + expect((thrown as ClientCatalogIncompatibleError).unsupportedEfforts).toEqual(["max"]); + }); + + test("a compatible catalog passes the gate", () => { + expect(() => assertClientCatalogCompatible( + catalogBody(["low", "high", "max"]), + { supportedEfforts: () => NEW_CLI }, + )).not.toThrow(); + }); + + test("an unverifiable runtime does not block the connection", () => { + // A client machine may legitimately have no Codex CLI to observe. Refusing then would + // block a working configuration on absent evidence rather than on an incompatibility. + const assessment = assessClientCatalogCompatibility(catalogBody(["max"]), { supportedEfforts: () => null }); + expect(assessment.kind).toBe("unverified"); + expect(() => assertClientCatalogCompatible(catalogBody(["max"]), { supportedEfforts: () => null })) + .not.toThrow(); + }); + + test("an unreadable body is unverified, not blamed on the runtime", () => { + // The hub client already rejects a malformed body with its own cause. Inventing a second + // one here would repeat #4169, where a refusal named a cause the server never reported. + const assessment = assessClientCatalogCompatibility("not json", { supportedEfforts: () => OLD_CLI }); + expect(assessment.kind).toBe("unverified"); + }); + + test("both catalog downloads are gated, and the restore paths are not", () => { + // connect and sync each write the hub's bytes to the same path; a gate on only one of them + // still lets a sync replace a parseable catalog with an unparseable one. + const source = readFileSync(repoPath("src", "client", "connect.ts"), "utf8"); + for (const written of ["catalog.body", "downloaded.body"]) { + const write = source.indexOf(`atomicWriteFile(DEFAULT_CATALOG_PATH, ${written})`); + expect(write).toBeGreaterThan(0); + const gate = source.indexOf(`assertClientCatalogCompatible(${written}`); + expect(gate).toBeGreaterThan(0); + expect(gate).toBeLessThan(write); + } + // Restoring a catalog this machine previously accepted must not be gated on a runtime that + // may since have changed — that would strand the client with no catalog at all. + expect(source).toContain("atomicWriteFile(DEFAULT_CATALOG_PATH, snapshot.body)"); + expect(source.match(/assertClientCatalogCompatible\(/g)).toHaveLength(2); + }); +}); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index a9784f67e1..5ecabf4665 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -195,6 +195,7 @@ "cli-transport-honesty.test.ts": "cli", "cli-usage-report.test.ts": "cli", "cli-version-skew.test.ts": "cli", + "client-catalog-compatibility.test.ts": "clients", "client-config-export-new-clients.test.ts": "config", "client-config-export.test.ts": "config", "client-config-new-clients.test.ts": "config",