From 635a3d1238b2beb43704f834dc0249f365513643 Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 11 Sep 2026 10:14:16 +0900 Subject: [PATCH 1/4] client: report local Codex readiness instead of bare connected state A connected client reported healthy while the installed Codex CLI exited before its first request, because the catalog on disk used a reasoning level that CLI does not know. Connection state proved the hub and the credential; it never proved the selected local runtime could consume what was written. The write-time gate cannot answer this. It runs once, on bytes about to be written, so it says nothing about a catalog that predates it, one written while the runtime ladder was unverified, or a runtime swapped afterwards. inspectClientCatalogReadiness assesses the installed file, and ocx connect status, ocx status --json and ocx connect now report the verdict. Only "ready" means ready; an unobservable runtime stays "unverified" rather than becoming an incompatibility, which is the line the write-time gate already refuses to cross. The probe runs only for a connected client, so no other install pays a Codex process for it. Closes #4207 --- scripts/test-layout/layout.json | 1 + src/cli/connect.ts | 119 +++++++++-- src/cli/status.ts | 10 + src/client/catalog-compatibility.ts | 78 +++++++ tests/cli/cli-connect-readiness.test.ts | 190 ++++++++++++++++++ tests/cli/cli-status-json.test.ts | 5 + .../client-catalog-compatibility.test.ts | 61 ++++++ tests/fixtures/test-layout-expected.json | 1 + 8 files changed, 453 insertions(+), 12 deletions(-) create mode 100644 tests/cli/cli-connect-readiness.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 0c07fc9a7c..f83a0a8f49 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -335,6 +335,7 @@ "cli-codex-log-guard.test.ts": "cli", "cli-config-command.test.ts": "cli", "cli-config-show-client.test.ts": "cli", + "cli-connect-readiness.test.ts": "cli", "cli-dispatch.test.ts": "cli", "cli-dto-fidelity.test.ts": "cli", "cli-export-command.test.ts": "cli", diff --git a/src/cli/connect.ts b/src/cli/connect.ts index 3506a2fa95..c533ff358f 100644 --- a/src/cli/connect.ts +++ b/src/cli/connect.ts @@ -1,5 +1,11 @@ -import { existsSync, lstatSync } from "node:fs"; +import { existsSync, lstatSync, readFileSync } from "node:fs"; import { DEFAULT_CATALOG_PATH } from "../codex/paths"; +import { + inspectClientCatalogReadiness, + type CatalogCompatibilityDeps, + type ClientCatalogFileState, + type ClientCatalogReadiness, +} from "../client/catalog-compatibility"; import { disconnectClient, revokeConnectedClientKey, @@ -26,6 +32,12 @@ import { export interface ClientCommandDeps extends RuntimeApiDeps { lifecycleLockDeps?: ClientLifecycleLockDeps; + catalogProbeDeps?: ClientCatalogProbeDeps; +} + +export interface ClientCatalogProbeDeps extends CatalogCompatibilityDeps { + /** Injected in tests; defaults to reading the materialized client catalog off disk. */ + readCatalogBody?: () => string | null; } export const CONNECT_USAGE = `Usage: @@ -56,21 +68,71 @@ export type ClientConnectionStatus = { catalog: "present" | "missing" | "unsafe"; token: "owned" | "missing" | "changed" | "unsafe"; rotation: "clean" | "orphan-cleaned" | "recovery-required" | "unsafe"; + /** + * Whether the selected local Codex CLI can actually launch against this connection (#4207). + * + * `state: "connected"` proves the hub answered and the credential works. It never proved the + * local runtime could consume what was downloaded, which is how a connection kept reporting + * itself healthy while `codex exec` exited on `unknown variant` before its first request. + * + * Reported only while connected, and reported as its own field rather than as a fourth + * `catalog` value: the status JSON is documented additive-only, so widening an existing + * field's value domain would change what `catalog: "present"` means for every consumer that + * already reads it. + */ + readiness?: ClientCatalogReadiness["kind"]; + /** Present whenever readiness is not `ready`; names the fault and the way out. */ + readinessReason?: string; }; -export function collectClientConnectionStatus(now = Date.now(), lifecycleLockDeps?: ClientLifecycleLockDeps): ClientConnectionStatus { +function readInstalledCatalogBody(): string | null { + try { + return readFileSync(DEFAULT_CATALOG_PATH, "utf8"); + } catch { + return null; + } +} + +/** The stat half of the catalog verdict, shared by the status collector and `ocx connect`. */ +function installedCatalogFileState(): ClientCatalogFileState { + if (!existsSync(DEFAULT_CATALOG_PATH)) return "missing"; + try { + const stat = lstatSync(DEFAULT_CATALOG_PATH); + return !stat.isSymbolicLink() && stat.isFile() ? "present" : "unsafe"; + } catch { + return "unsafe"; + } +} + +/** + * Observing the runtime spawns `codex debug models`, so this runs only for a connected client — + * the one configuration that installs hub bytes the local clamp never touched. A standalone or + * hub install pays nothing for it. + * + * Never throws. A status command that dies because a Codex probe failed would replace one wrong + * answer with a worse one. + */ +function inspectInstalledCatalogReadiness( + file: ClientCatalogFileState, + deps: ClientCatalogProbeDeps, +): ClientCatalogReadiness { + try { + const read = deps.readCatalogBody ?? readInstalledCatalogBody; + return inspectClientCatalogReadiness(file, file === "present" ? read() : null, deps); + } catch { + return { kind: "unverified", reason: "the selected local Codex runtime could not be inspected" }; + } +} + +export function collectClientConnectionStatus( + now = Date.now(), + lifecycleLockDeps?: ClientLifecycleLockDeps, + catalogProbeDeps: ClientCatalogProbeDeps = {}, +): ClientConnectionStatus { const state = readClientConnectionState(); const tokenState = readServiceApiTokenState(); const rotation = inspectClientRotationRecoveryGate(state, lifecycleLockDeps).kind; - let catalog: ClientConnectionStatus["catalog"] = "missing"; - if (existsSync(DEFAULT_CATALOG_PATH)) { - try { - const stat = lstatSync(DEFAULT_CATALOG_PATH); - catalog = !stat.isSymbolicLink() && stat.isFile() ? "present" : "unsafe"; - } catch { - catalog = "unsafe"; - } - } + const catalog = installedCatalogFileState(); if (state.kind !== "connected") { return { state: state.kind, @@ -88,6 +150,7 @@ export function collectClientConnectionStatus(now = Date.now(), lifecycleLockDep : tokenState.kind === "unsafe" ? "unsafe" : tokenState.fingerprint === state.value.tokenFingerprint ? "owned" : "changed"; + const readiness = inspectInstalledCatalogReadiness(catalog, catalogProbeDeps); return { state: "connected", serverUrl: state.value.serverUrl, @@ -102,6 +165,8 @@ export function collectClientConnectionStatus(now = Date.now(), lifecycleLockDep catalog, token, rotation, + readiness: readiness.kind, + ...(readiness.kind === "ready" ? {} : { readinessReason: readiness.reason }), }; } @@ -113,12 +178,25 @@ function parseClients(raw: string | undefined): OcxConnectedClientId[] { return values as OcxConnectedClientId[]; } +/** Reads as a verdict, not a field dump: "ready" is the only word that means the client works. */ +function readinessLine(status: ClientConnectionStatus): string { + const label = status.readiness === "ready" + ? "ready" + : status.readiness === "incompatible" + ? "not ready" + : "unverified"; + return `Local Codex CLI: ${label}${status.readinessReason ? ` (${status.readinessReason})` : ""}`; +} + function statusLines(status: ClientConnectionStatus): string[] { if (status.state !== "connected") { return [`Connection: ${status.state}${status.reason ? ` (${status.reason})` : ""}`]; } return [ "Connection: connected", + // Second line on purpose. The whole of #4207 is that a reader stopped at "connected" and + // believed the client was usable, so the local verdict has to arrive before the hub detail. + readinessLine(status), `Hub: ${status.serverUrl}`, `Management: ${status.managementUrl} (${status.managementTransport})`, `Protocol: ${status.protocolVersion}`, @@ -185,6 +263,23 @@ async function runConnect(argv: string[], deps: ClientCommandDeps): Promise { @@ -204,7 +299,7 @@ export async function handleConnectCommand(argv: string[], deps: ClientCommandDe const args = argv.slice(1); const wantsJson = takeFlag(args, "--json"); rejectArgs(args, CONNECT_USAGE, { redactValues: true }); - const status = collectClientConnectionStatus(Date.now(), deps.lifecycleLockDeps); + const status = collectClientConnectionStatus(Date.now(), deps.lifecycleLockDeps, deps.catalogProbeDeps ?? {}); printData(status, wantsJson, statusLines(status)); return; } diff --git a/src/cli/status.ts b/src/cli/status.ts index 1b5d9bd508..cc1f4506bf 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -149,6 +149,14 @@ export type CliStatusJson = { catalog?: "present" | "missing" | "unsafe"; catalogAgeSeconds?: number; credentialFile: "owned" | "missing" | "changed" | "unsafe"; + /** + * #4207: whether the selected local Codex CLI can consume the catalog this client + * installed. Absent unless a client connection exists. `connected` alone proved only the + * hub and the credential, and a reader who stopped there saw a healthy connection while + * `codex exec` was exiting before its first request. + */ + readiness?: "ready" | "unverified" | "incompatible"; + readinessReason?: string; }; service: { summary: string }; codexShim: { summary: string }; @@ -678,6 +686,8 @@ export async function collectStatus(): Promise { catalog: clientConnection.catalog, ...(clientConnection.catalogAgeSeconds !== undefined ? { catalogAgeSeconds: clientConnection.catalogAgeSeconds } : {}), credentialFile: clientConnection.token, + ...(clientConnection.readiness ? { readiness: clientConnection.readiness } : {}), + ...(clientConnection.readinessReason ? { readinessReason: clientConnection.readinessReason } : {}), }, service: { summary: serviceSummary }, codexShim: { summary: codexShimSummary }, diff --git a/src/client/catalog-compatibility.ts b/src/client/catalog-compatibility.ts index 8b4b9011cf..d101a13ac3 100644 --- a/src/client/catalog-compatibility.ts +++ b/src/client/catalog-compatibility.ts @@ -34,6 +34,32 @@ export interface CatalogCompatibilityDeps { supportedEfforts?: () => ReadonlySet | null; } +/** State of the materialized client catalog file, as `ocx connect status` already reports it. */ +export type ClientCatalogFileState = "present" | "missing" | "unsafe"; + +/** + * Whether the selected local Codex runtime can consume the catalog that is *already on disk* — + * a different question from the write-time gate, and the one #4207 was actually asking. + * + * The gate runs once, on bytes about to be written. It cannot speak for a file that predates it, + * for a file written while the ladder was {@link ClientCatalogCompatibility} `unverified`, or for + * a runtime that was swapped after the write. Those are exactly the states that kept reporting + * `connected` while `codex exec` died on `unknown variant \`max\``. + * + * Only `ready` means ready. `unverified` is not `incompatible`: a client machine may legitimately + * have no observable Codex CLI, and calling that an incompatibility would condemn a working + * install on absent evidence — the same mistake the write-time gate refuses to make. + */ +export type ClientCatalogReadiness = + | { kind: "ready" } + | { kind: "unverified"; reason: string } + | { + kind: "incompatible"; + reason: string; + unsupportedEfforts: readonly string[]; + affectedModels: readonly string[]; + }; + function parseModels(body: string): RawEntry[] | null { try { const parsed = JSON.parse(body) as { models?: unknown }; @@ -105,3 +131,55 @@ export function assertClientCatalogCompatible(body: string, deps: CatalogCompati if (assessment.kind !== "incompatible") return; throw new ClientCatalogIncompatibleError(assessment.unsupportedEfforts, assessment.affectedModels); } + +/** + * Why an already-installed incompatible catalog does not reuse the refusal message above: + * nothing was kept back. The unusable bytes are the ones Codex will read on its next launch, + * so "the previous catalog was kept" would be false. The two remedies are the same, because + * the operator's options do not depend on when the file arrived. + */ +function installedCatalogRejectionReason( + unsupportedEfforts: readonly string[], + affectedModels: readonly string[], +): string { + const models = affectedModels.length > 3 + ? `${affectedModels.slice(0, 3).join(", ")} and ${affectedModels.length - 3} more` + : affectedModels.join(", "); + return `the installed catalog uses reasoning ${unsupportedEfforts.length === 1 ? "level" : "levels"} ` + + `${unsupportedEfforts.join(", ")}, which the selected local Codex CLI rejects` + + `${models ? ` (${models})` : ""}. Codex exits before its first request until the CLI is ` + + "upgraded to a version that supports those levels, or CODEX_CLI_PATH points at one that " + + "does and `ocx sync` is run. `ocx doctor` reports which runtime is selected."; +} + +/** + * Assess the catalog this machine has already installed, so a surface can stop calling a + * connection ready when the local runtime cannot launch against it. + * + * `body` is the file's bytes, or `null` when they could not be read; `file` is the state the + * caller already established by stat. Neither non-present file state is an incompatibility: an + * absent or non-regular catalog is a different fault, and this function only ever claims an + * incompatibility it has proven. + */ +export function inspectClientCatalogReadiness( + file: ClientCatalogFileState, + body: string | null, + deps: CatalogCompatibilityDeps = {}, +): ClientCatalogReadiness { + if (file === "missing") { + return { kind: "unverified", reason: "no catalog is installed for the local Codex CLI to read" }; + } + if (file === "unsafe") { + return { kind: "unverified", reason: "the catalog path is not a regular file, so its bytes were not read" }; + } + if (body === null) return { kind: "unverified", reason: "the installed catalog could not be read" }; + const assessment = assessClientCatalogCompatibility(body, deps); + if (assessment.kind === "compatible") return { kind: "ready" }; + if (assessment.kind === "unverified") return assessment; + return { + kind: "incompatible", + reason: installedCatalogRejectionReason(assessment.unsupportedEfforts, assessment.affectedModels), + unsupportedEfforts: assessment.unsupportedEfforts, + affectedModels: assessment.affectedModels, + }; +} diff --git a/tests/cli/cli-connect-readiness.test.ts b/tests/cli/cli-connect-readiness.test.ts new file mode 100644 index 0000000000..8ed26b006f --- /dev/null +++ b/tests/cli/cli-connect-readiness.test.ts @@ -0,0 +1,190 @@ +/** + * #4207: "ocx connect status" answered a different question from the one the operator asked. + * It proved the hub answered and the credential worked, then printed "connected" over a catalog + * the installed Codex CLI could not parse, so "codex exec" died on an unknown-variant error for + * the reasoning level "max" before its first request. + * + * The write-time gate added in the first round cannot close this. It runs once, on bytes about + * to be written, so it says nothing about a catalog that predates it, one written while the + * runtime ladder was unverified, or a runtime swapped after the write. These tests drive the + * status surface itself, in a real client home, with the ladder injected so no Codex process is + * spawned to observe it. + */ +import { describe, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import { createHash } from "node:crypto"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; +import { repoRoot } from "../helpers/repo-root"; + +/** Codex CLI 0.135.0's ladder, verbatim from the parse error in the issue. */ +const OLD_CLI = ["none", "minimal", "low", "medium", "high", "xhigh"]; +const NEW_CLI = [...OLD_CLI, "max", "ultra"]; + +/** A hub catalog whose top rung the reporter's CLI rejects. */ +const CATALOG_WITH_MAX = JSON.stringify({ + models: [{ slug: "gpt-5.6-sol", supported_reasoning_levels: [{ effort: "high" }, { effort: "max" }] }], +}); + +type ProbeResult = { + lines: string[]; + status: { + state: string; + catalog: string; + readiness?: string; + readinessReason?: string; + }; +}; + +/** + * Runs the real "ocx connect status" surface against a throwaway client home. The ladder is + * injected rather than observed: a spawned "codex debug models" would make the assertion depend + * on whichever Codex CLI the test machine happens to have. + */ +function runStatusProbe(options: { + connected: boolean; + ladder: string[] | null | "forbidden"; + catalog?: string; +}): ProbeResult { + const opencodexHome = mkdtempSync(join(tmpdir(), "ocx-readiness-home-")); + const codexHome = mkdtempSync(join(tmpdir(), "ocx-readiness-codex-")); + try { + const token = `ocx_data_${"f".repeat(40)}`; + const fingerprint = createHash("sha256").update(token).digest("hex"); + const catalog = options.catalog ?? CATALOG_WITH_MAX; + writeFileSync(join(opencodexHome, "config.json"), JSON.stringify(options.connected + ? { + port: 10100, + providers: {}, + defaultProvider: "openai", + runtimeRole: "client", + client: { + serverUrl: "https://hub.example.test", + managementUrl: "https://hub.example.test", + managementTransport: "direct", + selectedClients: ["codex"], + tokenEnv: "OPENCODEX_API_AUTH_TOKEN", + apiKeyId: "client-key-1", + tokenFingerprint: fingerprint, + protocolVersion: 1, + connectedAt: "2026-08-28T00:00:00.000Z", + catalogFingerprint: createHash("sha256").update(catalog).digest("base64url"), + catalogSyncedAt: "2026-08-28T00:00:00.000Z", + }, + } + : { port: 10100, providers: {}, defaultProvider: "openai" }), "utf8"); + writeFileSync(join(opencodexHome, "service-api-token"), `${token}\n`, { mode: 0o600 }); + writeFileSync(join(codexHome, "opencodex-catalog.json"), catalog, "utf8"); + + const script = ` + const { collectClientConnectionStatus, handleConnectCommand } = require("./src/cli/connect"); + const ladder = JSON.parse(process.env.FIXTURE_LADDER); + const supportedEfforts = ladder === "forbidden" + ? () => { throw new Error("the runtime was probed on a path that must not probe it"); } + : ladder === null ? () => null : () => new Set(ladder); + const lifecycleLockDeps = { lockPath: process.env.OPENCODEX_HOME + "/lifecycle.sqlite" }; + const captured = []; + const real = console.log; + (async () => { + console.log = (...parts) => captured.push(parts.join(" ")); + try { + await handleConnectCommand(["status"], { lifecycleLockDeps, catalogProbeDeps: { supportedEfforts } }); + } finally { + console.log = real; + } + const status = collectClientConnectionStatus( + Date.parse("2026-08-28T00:00:10.000Z"), + lifecycleLockDeps, + { supportedEfforts }, + ); + console.log(JSON.stringify({ lines: captured, status })); + })(); + `; + + const result = spawnSync(process.execPath, ["--eval", script], { + cwd: repoRoot(), + encoding: "utf8", + env: { + ...process.env, + OPENCODEX_HOME: opencodexHome, + CODEX_HOME: codexHome, + // Matches the existing client fixtures: no probe may reach the operator's real Claude + // Desktop configuration, even transitively. + OPENCODEX_CLAUDE_DESKTOP_CONFIG_DIR: join(opencodexHome, "desktop"), + FIXTURE_LADDER: JSON.stringify(options.ladder), + }, + }); + expect(result.status).toBe(0); + return JSON.parse(result.stdout.trim().split("\n").at(-1)!) as ProbeResult; + } finally { + removeTreeWithRetry(opencodexHome); + removeTreeWithRetry(codexHome); + } +} + +describe("#4207 connected-client readiness", () => { + test("an installed catalog the local CLI rejects is not reported as ready", () => { + const probe = runStatusProbe({ connected: true, ladder: OLD_CLI }); + + expect(probe.status.state).toBe("connected"); + // The connection is real and the file is there. Both were true in the report, and both are + // why "connected" plus "present" read as success. + expect(probe.status.catalog).toBe("present"); + expect(probe.status.readiness).toBe("incompatible"); + expect(probe.status.readinessReason).toContain("max"); + // "Incompatible" alone is not actionable; the operator needs the way out. + expect(probe.status.readinessReason).toContain("CODEX_CLI_PATH"); + // The refusal message belongs to the write-time gate, which kept a previous file. Nothing + // was kept here: the unusable bytes are the ones Codex will read next. + expect(probe.status.readinessReason).not.toContain("The previous catalog was kept"); + }); + + test("the human status states the local verdict before the hub detail", () => { + const probe = runStatusProbe({ connected: true, ladder: OLD_CLI }); + + expect(probe.lines[0]).toBe("Connection: connected"); + // Second line, not buried under Hub/Protocol/Catalog: a reader who stops at "connected" is + // exactly the failure this issue describes. + expect(probe.lines[1]).toContain("Local Codex CLI: not ready"); + expect(probe.lines.find(line => line.startsWith("Hub:"))).toBeDefined(); + }); + + test("a catalog the local CLI accepts is ready, with nothing to explain", () => { + const probe = runStatusProbe({ connected: true, ladder: NEW_CLI }); + + expect(probe.status.readiness).toBe("ready"); + expect(probe.status.readinessReason).toBeUndefined(); + expect(probe.lines[1]).toBe("Local Codex CLI: ready"); + }); + + test("an unobservable runtime is unverified, never incompatible", () => { + // A client machine may legitimately have no Codex CLI to observe. Calling that an + // incompatibility would condemn a working install on absent evidence, which is the same + // line the write-time gate refuses to cross. + const probe = runStatusProbe({ connected: true, ladder: null }); + + expect(probe.status.readiness).toBe("unverified"); + expect(probe.status.readinessReason).toContain("did not report the reasoning levels"); + }); + + test("an unreadable catalog is unverified rather than blamed on the runtime", () => { + const probe = runStatusProbe({ connected: true, ladder: OLD_CLI, catalog: "not json" }); + + expect(probe.status.readiness).toBe("unverified"); + expect(probe.status.readinessReason).toContain("could not be read"); + }); + + test("a machine with no client connection never probes the runtime", () => { + // Observing the ladder spawns a Codex process. A standalone or hub install has no client + // catalog question to answer and must not pay for one on every status call, so the injected + // probe throws if it is reached. + const probe = runStatusProbe({ connected: false, ladder: "forbidden" }); + + expect(probe.status.state).toBe("disconnected"); + expect(probe.status.readiness).toBeUndefined(); + expect(probe.status.readinessReason).toBeUndefined(); + expect(probe.lines[0]).toBe("Connection: disconnected"); + }); +}); diff --git a/tests/cli/cli-status-json.test.ts b/tests/cli/cli-status-json.test.ts index c4fe01752a..aeef546421 100644 --- a/tests/cli/cli-status-json.test.ts +++ b/tests/cli/cli-status-json.test.ts @@ -363,6 +363,11 @@ describe("CLI status JSON", () => { state: "disconnected", credentialFile: "missing", }); + // #4207 gave a connected client a local-runtime readiness verdict. Observing that runtime + // spawns a Codex process, so a machine with no client connection must not carry the field + // at all; its absence is what keeps every ordinary `ocx status` off that probe. + expect(parsed.connection).not.toHaveProperty("readiness"); + expect(parsed.connection).not.toHaveProperty("readinessReason"); const serialized = JSON.stringify(parsed).toLowerCase(); for (const forbidden of ["apikey", "sk-test-secret", "token", "refreshtoken", "authorization", "email"]) { diff --git a/tests/clients/client-catalog-compatibility.test.ts b/tests/clients/client-catalog-compatibility.test.ts index f01f43d071..454ff14c67 100644 --- a/tests/clients/client-catalog-compatibility.test.ts +++ b/tests/clients/client-catalog-compatibility.test.ts @@ -15,6 +15,7 @@ import { assertClientCatalogCompatible, assessClientCatalogCompatibility, ClientCatalogIncompatibleError, + inspectClientCatalogReadiness, } from "../../src/client/catalog-compatibility"; import { repoPath } from "../helpers/repo-root"; @@ -137,3 +138,63 @@ describe("#4207 client catalog gate", () => { expect(source.match(/assertClientCatalogCompatible\(/g)).toHaveLength(2); }); }); + +describe("#4207 installed catalog readiness", () => { + test("a catalog the local runtime accepts is ready", () => { + expect(inspectClientCatalogReadiness("present", catalogBody(["low", "max"]), { supportedEfforts: () => NEW_CLI })) + .toEqual({ kind: "ready" }); + }); + + test("a catalog already on disk that the runtime rejects is an established incompatibility", () => { + // The write-time gate never saw this file: it may predate the gate, or have been written + // while the ladder was unverified. Readiness is a question about the bytes that are there. + const readiness = inspectClientCatalogReadiness( + "present", + catalogBody(["low", "medium", "high", "xhigh", "max"]), + { supportedEfforts: () => OLD_CLI }, + ); + + expect(readiness.kind).toBe("incompatible"); + if (readiness.kind !== "incompatible") throw new Error("unreachable"); + expect(readiness.unsupportedEfforts).toEqual(["max"]); + expect(readiness.affectedModels).toEqual(["gpt-5.6-sol"]); + expect(readiness.reason).toContain("max"); + expect(readiness.reason).toContain("CODEX_CLI_PATH"); + // The gate's wording promises the previous catalog survived. Nothing survived here, so + // reusing that message would tell the operator the opposite of what happened. + expect(readiness.reason).not.toContain("The previous catalog was kept"); + }); + + test("an unobservable runtime ladder is unverified, not incompatible", () => { + const readiness = inspectClientCatalogReadiness("present", catalogBody(["max"]), { supportedEfforts: () => null }); + + expect(readiness.kind).toBe("unverified"); + }); + + test("an unreadable body is unverified", () => { + expect(inspectClientCatalogReadiness("present", "not json", { supportedEfforts: () => OLD_CLI }).kind) + .toBe("unverified"); + }); + + test("bytes that could not be read at all are unverified", () => { + expect(inspectClientCatalogReadiness("present", null, { supportedEfforts: () => OLD_CLI }).kind) + .toBe("unverified"); + }); + + test("an absent or non-regular catalog is a different fault, never an incompatibility", () => { + // Claiming an incompatibility here would name a cause nothing established -- the same + // mistake #4169 was filed for. + for (const file of ["missing", "unsafe"] as const) { + const readiness = inspectClientCatalogReadiness(file, null, { supportedEfforts: () => OLD_CLI }); + expect(readiness.kind).toBe("unverified"); + } + }); + + test("the runtime is not observed for a file state that was never read", () => { + // Only 'present' has bytes worth an opinion. Probing the local Codex CLI for a missing file + // would spend a process on a question its answer cannot change. + const probe = () => { throw new Error("the runtime was observed for a catalog that was not read"); }; + expect(inspectClientCatalogReadiness("missing", null, { supportedEfforts: probe }).kind).toBe("unverified"); + expect(inspectClientCatalogReadiness("unsafe", null, { supportedEfforts: probe }).kind).toBe("unverified"); + }); +}); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index ed625c193b..a6fef10432 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -170,6 +170,7 @@ "cli-codex-log-guard.test.ts": "cli", "cli-config-command.test.ts": "cli", "cli-config-show-client.test.ts": "cli", + "cli-connect-readiness.test.ts": "cli", "cli-dispatch.test.ts": "cli", "cli-dto-fidelity.test.ts": "cli", "cli-export-command.test.ts": "cli", From efefde42c24c4a58c305a1ed489d36d94efccc97 Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 11 Sep 2026 10:34:10 +0900 Subject: [PATCH 2/4] client: fold adversarial review into the readiness report Four things an independent read of the diff found. A diagnostics command should not start writing runtime selection state: the default observer now resolves the runtime without persisting and hands that command to the catalog read, which also avoids a second probe on a path that had already resolved it. The ocx connect decision moves into a pure connectCompletionReport, so the fail-closed exit is exercised without a hub. It prints the verdict first and withholds "Connected to" when it fails, because a caller grepping that phrase would otherwise read a broken catalog as success. A Claude-only connection is told about an old Codex CLI but not failed by it, since nothing in that connection launches Codex. connectClient now receives the same observer, so the write-time gate and the readiness check cannot disagree about the ladder inside one command. An installed catalog that is not JSON gets its own sentence instead of the gate's "downloaded" wording, and the subprocess fixture takes the same spawn budget the neighbouring client fixtures use. --- src/cli/connect.ts | 95 ++++++++++++++++++++----- src/client/catalog-compatibility.ts | 9 ++- tests/cli/cli-connect-readiness.test.ts | 63 +++++++++++++++- 3 files changed, 149 insertions(+), 18 deletions(-) diff --git a/src/cli/connect.ts b/src/cli/connect.ts index c533ff358f..22930011fc 100644 --- a/src/cli/connect.ts +++ b/src/cli/connect.ts @@ -1,5 +1,7 @@ import { existsSync, lstatSync, readFileSync } from "node:fs"; import { DEFAULT_CATALOG_PATH } from "../codex/paths"; +import { codexSupportedReasoningEfforts } from "../codex/catalog/effort"; +import { resolveCodexRuntime } from "../codex/runtime"; import { inspectClientCatalogReadiness, type CatalogCompatibilityDeps, @@ -93,6 +95,20 @@ function readInstalledCatalogBody(): string | null { } } +/** + * The ladder the selected Codex CLI accepts, observed without persisting anything. + * + * `codexSupportedReasoningEfforts()` with no deps reaches `resolveAndPersistCodexRuntime`, which + * writes codex-runtime.json. `ocx status` deliberately resolves without persisting, and a + * read-only diagnostics command should not start writing runtime selection state because a + * readiness check was added to it. Handing the already-resolved command in as the only candidate + * skips that path and reuses the resolve cache `ocx status` has usually already filled. + */ +function observeLocalCodexEffortLadder(): ReadonlySet | null { + const command = resolveCodexRuntime().runtime.command; + return codexSupportedReasoningEfforts({ commandCandidates: () => [command] }); +} + /** The stat half of the catalog verdict, shared by the status collector and `ocx connect`. */ function installedCatalogFileState(): ClientCatalogFileState { if (!existsSync(DEFAULT_CATALOG_PATH)) return "missing"; @@ -118,7 +134,9 @@ function inspectInstalledCatalogReadiness( ): ClientCatalogReadiness { try { const read = deps.readCatalogBody ?? readInstalledCatalogBody; - return inspectClientCatalogReadiness(file, file === "present" ? read() : null, deps); + return inspectClientCatalogReadiness(file, file === "present" ? read() : null, { + supportedEfforts: deps.supportedEfforts ?? observeLocalCodexEffortLadder, + }); } catch { return { kind: "unverified", reason: "the selected local Codex runtime could not be inspected" }; } @@ -188,6 +206,55 @@ function readinessLine(status: ClientConnectionStatus): string { return `Local Codex CLI: ${label}${status.readinessReason ? ` (${status.readinessReason})` : ""}`; } +export type ConnectCompletionReport = { + readonly lines: readonly string[]; + /** Non-null when `ocx connect` must exit non-zero rather than report success. */ + readonly failure: string | null; +}; + +/** + * What `ocx connect` says once the hub and the credential are settled, and whether the command + * still fails (#4207). + * + * Pure so the fail-closed decision can be exercised without a hub. Two rules it encodes: + * + * On a proven incompatibility the verdict is printed FIRST and the `Connected to …` line is + * withheld, because a caller grepping for that phrase would otherwise read a catalog the local + * CLI cannot parse as a success. The connection really was saved, so the replacement line says + * where to see it. + * + * And that failure applies only when this connection selected Codex. A Claude-only connection + * never launches the Codex CLI, so an old binary somewhere on PATH is not a reason to fail the + * operator's Claude Desktop setup — it is still worth saying, which is why the line survives + * without the exit code. + */ +export function connectCompletionReport( + connection: { serverUrl: string; apiKeyId: string }, + selectedClients: readonly OcxConnectedClientId[], + readiness: ClientCatalogReadiness, +): ConnectCompletionReport { + const connected = `Connected to ${connection.serverUrl} as key ${connection.apiKeyId}.`; + if (readiness.kind === "ready") { + return { lines: [connected, "Local Codex CLI: ready (it accepts every reasoning level in the installed catalog)."], failure: null }; + } + if (readiness.kind === "unverified") { + // Not a failure. A client with no observable Codex CLI is a working configuration, and the + // write-time gate deliberately lets it through; saying so is the honest middle report. + return { lines: [connected, `Local Codex CLI: unverified (${readiness.reason}).`], failure: null }; + } + const verdict = `Local Codex CLI: not ready (${readiness.reason})`; + if (!selectedClients.includes("codex")) { + return { + lines: [connected, `${verdict} This connection selected ${selectedClients.join(", ")}, so nothing here launches Codex.`], + failure: null, + }; + } + return { + lines: [verdict, `The connection to ${connection.serverUrl} as key ${connection.apiKeyId} was saved; run 'ocx connect status' to see it.`], + failure: `client_not_ready: ${readiness.reason}`, + }; +} + function statusLines(status: ClientConnectionStatus): string[] { if (status.state !== "connected") { return [`Connection: ${status.state}${status.reason ? ` (${status.reason})` : ""}`]; @@ -261,25 +328,21 @@ async function runConnect(argv: string[], deps: ClientCommandDeps): Promise { diff --git a/src/client/catalog-compatibility.ts b/src/client/catalog-compatibility.ts index d101a13ac3..9114f935a9 100644 --- a/src/client/catalog-compatibility.ts +++ b/src/client/catalog-compatibility.ts @@ -175,7 +175,14 @@ export function inspectClientCatalogReadiness( if (body === null) return { kind: "unverified", reason: "the installed catalog could not be read" }; const assessment = assessClientCatalogCompatibility(body, deps); if (assessment.kind === "compatible") return { kind: "ready" }; - if (assessment.kind === "unverified") return assessment; + if (assessment.kind === "unverified") { + // assessClientCatalogCompatibility words its parse failure for bytes that have just been + // downloaded. These bytes are already installed, so blaming a download would send the + // operator to the wrong place; name the file that is actually unusable. + return parseModels(body) === null + ? { kind: "unverified", reason: "the installed catalog is not readable JSON, so the local Codex CLI cannot parse it either" } + : assessment; + } return { kind: "incompatible", reason: installedCatalogRejectionReason(assessment.unsupportedEfforts, assessment.affectedModels), diff --git a/tests/cli/cli-connect-readiness.test.ts b/tests/cli/cli-connect-readiness.test.ts index 8ed26b006f..548cdb716d 100644 --- a/tests/cli/cli-connect-readiness.test.ts +++ b/tests/cli/cli-connect-readiness.test.ts @@ -18,6 +18,9 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { removeTreeWithRetry } from "../helpers/remove-tree"; import { repoRoot } from "../helpers/repo-root"; +import { INTERNAL_DEADLINE_MS } from "../helpers/test-budget"; +import { connectCompletionReport } from "../../src/cli/connect"; +import type { ClientCatalogReadiness } from "../../src/client/catalog-compatibility"; /** Codex CLI 0.135.0's ladder, verbatim from the parse error in the issue. */ const OLD_CLI = ["none", "minimal", "low", "medium", "high", "xhigh"]; @@ -106,6 +109,11 @@ function runStatusProbe(options: { const result = spawnSync(process.execPath, ["--eval", script], { cwd: repoRoot(), encoding: "utf8", + // Bun's test timeout cannot interrupt spawnSync, so a child that wedged on a lock or an + // unexpected probe would hang the worker rather than fail. Same budget the existing + // client fixtures use. + timeout: INTERNAL_DEADLINE_MS, + killSignal: "SIGKILL", env: { ...process.env, OPENCODEX_HOME: opencodexHome, @@ -173,7 +181,10 @@ describe("#4207 connected-client readiness", () => { const probe = runStatusProbe({ connected: true, ladder: OLD_CLI, catalog: "not json" }); expect(probe.status.readiness).toBe("unverified"); - expect(probe.status.readinessReason).toContain("could not be read"); + // The write-time gate says "the downloaded catalog could not be read", which points the + // operator at a download that is not the problem. These bytes are already installed. + expect(probe.status.readinessReason) + .toBe("the installed catalog is not readable JSON, so the local Codex CLI cannot parse it either"); }); test("a machine with no client connection never probes the runtime", () => { @@ -188,3 +199,53 @@ describe("#4207 connected-client readiness", () => { expect(probe.lines[0]).toBe("Connection: disconnected"); }); }); + +describe("#4207 what ocx connect reports when the local CLI cannot use the catalog", () => { + const incompatible: ClientCatalogReadiness = { + kind: "incompatible", + reason: "the installed catalog uses reasoning level max, which the selected local Codex CLI rejects", + unsupportedEfforts: ["max"], + affectedModels: ["gpt-5.6-sol"], + }; + const connection = { serverUrl: "https://hub.example.test", apiKeyId: "client-key-1" }; + + test("a ready client reports the connection and the local verdict", () => { + const report = connectCompletionReport(connection, ["codex"], { kind: "ready" }); + + expect(report.failure).toBeNull(); + expect(report.lines[0]).toContain("Connected to https://hub.example.test"); + expect(report.lines[1]).toContain("ready"); + }); + + test("an unverifiable runtime is reported but does not fail the command", () => { + // Refusing here would block a working configuration on absent evidence, which is the line + // the write-time gate already refuses to cross. + const report = connectCompletionReport(connection, ["codex"], { kind: "unverified", reason: "no Codex CLI was observed" }); + + expect(report.failure).toBeNull(); + expect(report.lines.join(" ")).toContain("unverified"); + }); + + test("a proven incompatibility fails the command and withholds the success line", () => { + const report = connectCompletionReport(connection, ["codex"], incompatible); + + expect(report.failure).toBe(`client_not_ready: ${incompatible.reason}`); + // A caller grepping for "Connected to" must not read a catalog the local CLI cannot parse + // as success, so the verdict leads and that phrase is withheld. + expect(report.lines[0]).toContain("not ready"); + expect(report.lines.join(" ")).not.toContain("Connected to"); + // The connection really was saved. Saying so is what keeps the failure from reading as a + // rollback that never happened. + expect(report.lines.join(" ")).toContain("was saved"); + }); + + test("a Claude-only connection is told, but not failed, by an old Codex CLI", () => { + // Nothing in this connection launches Codex, so a stale binary elsewhere on PATH is not a + // reason to fail an operator's Claude Desktop setup. + const report = connectCompletionReport(connection, ["claude"], incompatible); + + expect(report.failure).toBeNull(); + expect(report.lines.join(" ")).toContain("nothing here launches Codex"); + expect(report.lines[0]).toContain("Connected to"); + }); +}); From 87f5b52033e9acb5a77938b10f5ec3f7a708b3f8 Mon Sep 17 00:00:00 2001 From: JUN Date: Fri, 11 Sep 2026 10:36:32 +0900 Subject: [PATCH 3/4] client: give the write-time gate the same observer in production The previous commit only forwarded catalogCompatibility when a test had injected it, so an ordinary ocx connect still let assertClientCatalogCompatible fall back to its own default -- which persists runtime selection state and runs a second probe. One command could then act on two separately observed ladders, and the comment claiming otherwise was false. Both checks now build the observer through one helper. --- src/cli/connect.ts | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/cli/connect.ts b/src/cli/connect.ts index 22930011fc..8059cf1d74 100644 --- a/src/cli/connect.ts +++ b/src/cli/connect.ts @@ -109,6 +109,17 @@ function observeLocalCodexEffortLadder(): ReadonlySet | null { return codexSupportedReasoningEfforts({ commandCandidates: () => [command] }); } +/** + * One observer per command, for the write-time gate and the readiness check alike. Built even + * when nothing was injected, so production does not silently fall back to the default inside + * {@link assertClientCatalogCompatible} — that default persists runtime selection state and + * would run its own probe, which is how the two checks could disagree about the ladder within + * a single `ocx connect`. + */ +function catalogObserver(deps: ClientCatalogProbeDeps | undefined): CatalogCompatibilityDeps { + return { supportedEfforts: deps?.supportedEfforts ?? observeLocalCodexEffortLadder }; +} + /** The stat half of the catalog verdict, shared by the status collector and `ocx connect`. */ function installedCatalogFileState(): ClientCatalogFileState { if (!existsSync(DEFAULT_CATALOG_PATH)) return "missing"; @@ -134,9 +145,7 @@ function inspectInstalledCatalogReadiness( ): ClientCatalogReadiness { try { const read = deps.readCatalogBody ?? readInstalledCatalogBody; - return inspectClientCatalogReadiness(file, file === "present" ? read() : null, { - supportedEfforts: deps.supportedEfforts ?? observeLocalCodexEffortLadder, - }); + return inspectClientCatalogReadiness(file, file === "present" ? read() : null, catalogObserver(deps)); } catch { return { kind: "unverified", reason: "the selected local Codex runtime could not be inspected" }; } @@ -331,10 +340,10 @@ async function runConnect(argv: string[], deps: ClientCommandDeps): Promise Date: Fri, 11 Sep 2026 17:46:24 +0900 Subject: [PATCH 4/4] client: keep ocx config show out of the local Codex probe collectClientConnectionStatus observes the local ladder for a connected client, and observing it spawns codex debug models under a 45s budget. That is the point on ocx status and ocx connect status. config show is a different caller: it reads state, reason and token to answer whether the hub link is real, and it arrived on dev after this branch forked, so nothing here had declined the probe on its behalf. Declining it explicitly keeps a read-only config dump from turning into a runtime probe - the same reasoning the readiness check already applies when it refuses to persist runtime selection state. --- src/cli/config-command.ts | 11 ++++++++++- tests/cli/cli-status-hub-state.test.ts | 5 ++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/cli/config-command.ts b/src/cli/config-command.ts index f0d3c1cbae..b06ef38d15 100644 --- a/src/cli/config-command.ts +++ b/src/cli/config-command.ts @@ -171,7 +171,16 @@ export async function handleConfigCommand(argv: string[]): Promise { // Imported here rather than at module scope: `./connect` pulls the whole client lifecycle // in, and `ocx config get/set` has no use for it. const { collectClientConnectionStatus } = await import("./connect"); - const note = remoteHubConfigNote(diagnostics.config, () => collectClientConnectionStatus()); + // The readiness probe is declined explicitly. `collectClientConnectionStatus` observes the + // local Codex ladder for a connected client, and observing it spawns `codex debug models` + // under a 45s budget. `ocx config show` reads only `state`, `reason` and `token` from the + // result, so paying for a subprocess here would buy nothing and would quietly turn a + // read-only config dump into a runtime probe. Returning no ladder resolves readiness to + // `unverified`, which is the honest answer for a caller that never asked. + const note = remoteHubConfigNote( + diagnostics.config, + () => collectClientConnectionStatus(undefined, undefined, { supportedEfforts: () => null }), + ); // First key, not last: it has to be read before the empty `providers` map that misled a // reader into concluding nothing was configured anywhere. const config = note && redacted && typeof redacted === "object" && !Array.isArray(redacted) diff --git a/tests/cli/cli-status-hub-state.test.ts b/tests/cli/cli-status-hub-state.test.ts index 9875223061..e396f023e1 100644 --- a/tests/cli/cli-status-hub-state.test.ts +++ b/tests/cli/cli-status-hub-state.test.ts @@ -241,7 +241,10 @@ describe("ocx status end to end on a connected client", () => { expect(parsed.remoteHub.origin).toBe("https://hub.example.test:8443"); expect(parsed.remoteHub.subagentModels).toEqual(["xai/grok-4.6", "gpt-5.6-sol"]); expect(parsed.remoteHub.oauth).toEqual([{ provider: "xai", loggedIn: true }, { provider: "anthropic", loggedIn: false }]); - // The connection block is untouched; remoteHub describes the other end of the link. + // `remoteHub` describes the other end of the link, so it does not disturb the link's own + // state. `connection` itself is no longer untouched — a connected client also reports a + // local `readiness` verdict — so this asserts the one field `remoteHub` must not perturb + // rather than claiming the whole block is unchanged. expect(parsed.connection.state).toBe("connected"); const human = await runStatus(home, codexHome, false);