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/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/src/cli/connect.ts b/src/cli/connect.ts index 3506a2fa95..8059cf1d74 100644 --- a/src/cli/connect.ts +++ b/src/cli/connect.ts @@ -1,5 +1,13 @@ -import { existsSync, lstatSync } from "node:fs"; +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, + type ClientCatalogFileState, + type ClientCatalogReadiness, +} from "../client/catalog-compatibility"; import { disconnectClient, revokeConnectedClientKey, @@ -26,6 +34,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 +70,96 @@ 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 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] }); +} + +/** + * 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"; + 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, catalogObserver(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 +177,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 +192,8 @@ export function collectClientConnectionStatus(now = Date.now(), lifecycleLockDep catalog, token, rotation, + readiness: readiness.kind, + ...(readiness.kind === "ready" ? {} : { readinessReason: readiness.reason }), }; } @@ -113,12 +205,74 @@ 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})` : ""}`; +} + +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})` : ""}`]; } 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}`, @@ -183,8 +337,21 @@ async function runConnect(argv: string[], deps: ClientCommandDeps): Promise { @@ -204,7 +371,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..9114f935a9 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,62 @@ 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") { + // 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), + 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..548cdb716d --- /dev/null +++ b/tests/cli/cli-connect-readiness.test.ts @@ -0,0 +1,251 @@ +/** + * #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"; +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"]; +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", + // 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, + 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"); + // 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", () => { + // 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"); + }); +}); + +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"); + }); +}); 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); 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",