From a982f06b0b2a2ed1d493e549247a63c283e58501 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Thu, 10 Sep 2026 17:33:01 +0900 Subject: [PATCH 1/8] feat(codex): plan and apply a Codex CLI update from bound evidence Phase 1 answers who owns a Codex CLI installation and stops there: no registry query, no process enumeration, no writes. This adds the three inputs it left out and the two verbs that expose them. 'plan' resolves the channel to one exact version with its sha512 integrity, reads the process table, and returns a decision. Its plan id is a SHA-256 over exactly the evidence the decision rests on -- provenance, installed version, redacted location, channel, target, integrity and shim eligibility -- so there is no plan state on disk to expire, collide or clean up. Session blockers are deliberately outside the digest: a session that starts or ends between dry-run and apply must not expire a plan the operator read correctly, and it is re-read at apply time where it can only refuse. 'apply --plan ' recomputes the plan and refuses unless the id still matches, then runs exactly one command, npm install -g @openai/codex at the pinned version. The outcome is classified from a fresh inspection, never from the installer exit code, and is never retried or rolled back automatically. Nothing is stopped, restarted or signalled. Three fail-closed choices differ from the surrounding code on purpose: - listCodexAppServerProcesses maps enumeration failure to an empty list because its kill contract must never signal a process it could not verify. The update contract is the opposite, so scanCodexAppServerProcesses reports 'unavailable' and the plan defers. - OpenCodex's self-update treats a failed integrity query as skipped and proceeds best-effort. That trade is defensible for a package we publish; for a foreign package installed on the operator's behalf it refuses instead. - An advisory runtime version is what a binary said about itself. Only package-manifest evidence can be compared with a registry version or read back after an install, so anything else refuses. Windows stays inert: phase 1 performs no candidate filesystem I/O there and answers windows_inspection_deferred, so the plan is inapplicable for the same reason rather than pretending to ownership evidence it does not have. The shim is repaired only when the pre-update inspection reported a matched shim. codex-shim-autorestore already excluded this namespace from ambient repair with the note that a later apply must own its preflight; it now does. The launcher needed no change -- isCodexCliUpdateInspectionArgv already covers the namespace by argv position -- but the phase-1 scope guards asserting that this surface never advertises a dry-run are updated deliberately, while the 'check' capability keeps its stricter read-only assertions unchanged. --- scripts/test-layout/layout.json | 1 + src/cli/capabilities.ts | 34 ++ src/cli/codex-cli-update.ts | 210 ++++++-- src/cli/registry.ts | 7 +- src/cli/system-command.ts | 2 + src/codex/app-server-processes.ts | 39 ++ src/codex/cli-update-plan.ts | 472 ++++++++++++++++++ tests/cli/cli-codex-cli-update.test.ts | 90 +++- tests/cli/cli-registry.test.ts | 7 +- .../codex-cli-update-plan.test.ts | 461 +++++++++++++++++ .../codex-cli-update-zero-effect.test.ts | 2 +- tests/fixtures/test-layout-expected.json | 1 + 12 files changed, 1277 insertions(+), 49 deletions(-) create mode 100644 src/codex/cli-update-plan.ts create mode 100644 tests/codex-integration/codex-cli-update-plan.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 543c53a5773..edbddcd64ff 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -491,6 +491,7 @@ "codex-cli-installation-targets.test.ts": "codex-integration", "codex-cli-windows-installation-files.test.ts": "codex-integration", "codex-cli-update-launcher-policy.test.ts": "codex-integration", + "codex-cli-update-plan.test.ts": "codex-integration", "codex-cli-update-zero-effect.test.ts": "codex-integration", "codex-composed-acceptance.test.ts": "codex-integration", "codex-config-generation.test.ts": "codex-integration", diff --git a/src/cli/capabilities.ts b/src/cli/capabilities.ts index cb117bcba05..af2425f7eae 100644 --- a/src/cli/capabilities.ts +++ b/src/cli/capabilities.ts @@ -779,6 +779,40 @@ export const CAPABILITIES: readonly Capability[] = [ "Does not run the named Codex/npm/Node files, query a registry, install software, control processes or persist state.", ], }, + { + command: ["system", "codex-cli-update", "plan"], + summary: "Dry-run a Codex CLI update and print the plan id that authorizes applying it.", + routes: [], + flags: [ + { name: "--channel", value: "string", summary: "Registry channel to resolve. Only the stable latest channel is offered." }, + { name: "--json", value: "boolean", summary: "Emit the plan as JSON." }, + ], + mutates: false, + json: "envelope", + details: [ + "Adds the three inputs check leaves out: an exact registry version with its sha512 integrity, a fail-closed process-table read, and a decision.", + "Writes nothing and installs nothing. A refusal is a normal dry-run answer and still exits 0.", + "The plan id is a digest of the evidence the decision rests on, not a stored job. There is no plan state on disk to expire, collide or clean up.", + "An unreadable process table refuses rather than reading as no live session.", + ], + }, + { + command: ["system", "codex-cli-update", "apply"], + summary: "Install the exact Codex CLI version bound into a plan id from a dry-run.", + routes: [], + flags: [ + { name: "--plan", value: "string", required: true, summary: "Required: the plan id printed by a dry-run the operator read." }, + { name: "--json", value: "boolean", summary: "Emit the apply result as JSON." }, + ], + mutates: true, + json: "envelope", + details: [ + "--plan is mandatory because the operator must approve a target they have read. The plan is recomputed from live evidence and refused unless the id still matches.", + "Packs the exact resolved @openai/codex version, verifies the tarball sha512 against the plan-bound integrity, and installs only that verified file; never stops, restarts or signals Codex, the app-server, the desktop app or the tray.", + "The outcome is classified from a fresh inspection rather than the installer exit code, and is never retried or rolled back automatically.", + "Repairs the shim only when this installation owned a matched shim before the update.", + ], + }, { command: ["system", "codex-restart"], summary: "Restart the Codex desktop app and app-servers.", diff --git a/src/cli/codex-cli-update.ts b/src/cli/codex-cli-update.ts index 19a7f9d02ad..3fbd459aeff 100644 --- a/src/cli/codex-cli-update.ts +++ b/src/cli/codex-cli-update.ts @@ -11,20 +11,32 @@ import type { CodexCliInstallationSnapshot, CodexCliInstallationTargetDerivation, } from "../codex/cli-installation-targets"; +import { + applyCodexCliUpdatePlan, + createCodexCliUpdatePlan, + type CodexCliUpdateApplyDeps, + type CodexCliUpdateApplyResult, + type CodexCliUpdateChannel, + type CodexCliUpdatePlan, + type CodexCliUpdatePlanDeps, +} from "../codex/cli-update-plan"; import { CliUsageError, isJsonOption, printData, runCliAction } from "./runtime-api"; import { trustedNodeLauncherContext } from "./launcher-context"; export const CODEX_CLI_UPDATE_USAGE = `Usage: ocx system codex-cli-update check [--json] ocx system codex-cli-update attest [--json] - ocx system codex-cli-update attest --candidate --npm-prefix --npm-cli --node [--json]`; + ocx system codex-cli-update attest --candidate --npm-prefix --npm-cli --node [--json] + ocx system codex-cli-update plan [--channel latest] [--json] + ocx system codex-cli-update apply --plan [--json]`; + +const PLAN_ID_RE = /^[0-9a-f]{32}$/; -export type ParsedCodexCliUpdateArgs = Readonly<{ - json: boolean; -}> | Readonly<{ - json: boolean; - attest: CodexCliInstallationIdentityInput | "selected"; -}>; +export type ParsedCodexCliUpdateArgs = + | Readonly<{ action: "check"; json: boolean }> + | Readonly<{ action: "plan"; json: boolean; channel: CodexCliUpdateChannel }> + | Readonly<{ action: "apply"; json: boolean; planId: string }> + | Readonly<{ json: boolean; attest: CodexCliInstallationIdentityInput | "selected" }>; export interface CodexCliUpdateCommandDeps { readonly inspectInstall?: (deps: CodexCliInstallProvenanceDeps) => Promise; @@ -32,6 +44,8 @@ export interface CodexCliUpdateCommandDeps { readonly deriveInstallationInput?: ( snapshot: CodexCliInstallationSnapshot, ) => CodexCliInstallationTargetDerivation; + readonly createPlan?: (deps: CodexCliUpdatePlanDeps) => Promise; + readonly applyPlan?: (planId: string, deps: CodexCliUpdateApplyDeps) => Promise; } function identitySummary(report: CodexCliInstallationIdentityReport): string[] { @@ -68,24 +82,71 @@ function installSummary(report: CodexCliInstallReport): string[] { ]; } +function planSummary(plan: CodexCliUpdatePlan): string[] { + const lines = [ + `applicable: ${plan.applicable ? "yes" : "no"}`, + `reason: ${plan.refusal ?? "none"}`, + `provenance: ${plan.provenance}`, + `installed-version: ${plan.installedVersion ?? "unavailable"}`, + `version-evidence: ${plan.versionEvidence}`, + `channel: ${plan.channel}`, + `target-version: ${plan.targetVersion ?? "unresolved"}`, + `target-integrity: ${plan.targetIntegrity ?? "unresolved"}`, + `session: ${plan.session.state}${plan.session.matches === null ? "" : ` (${plan.session.matches})`}`, + ]; + if (plan.planId) lines.push(`plan: ${plan.planId}`); + if (plan.command) lines.push(`command: ${plan.command.join(" ")} (indicative — apply verifies the packed tarball and installs the verified file)`); + return lines; +} + +function applySummary(result: CodexCliUpdateApplyResult): string[] { + return [ + `status: ${result.status}`, + `reason: ${result.refusal ?? "none"}`, + `plan: ${result.planId ?? "unavailable"}`, + `target-version: ${result.targetVersion ?? "unavailable"}`, + `installed-before: ${result.installedVersionBefore ?? "unavailable"}`, + `installed-after: ${result.installedVersionAfter ?? "unavailable"}`, + `installer-exit: ${result.installerExitCode ?? "unavailable"}`, + ]; +} + +/** Read `--name value` and `--name=value` alike; both spellings reach this CLI. */ +function optionValue(tokens: readonly string[], index: number, name: string): { value: string; next: number } { + const token = tokens[index]!; + const inline = `--${name}=`; + if (token.startsWith(inline)) { + const value = token.slice(inline.length); + if (!value) throw new CliUsageError(`--${name} requires a value`, CODEX_CLI_UPDATE_USAGE); + return { value, next: index + 1 }; + } + const value = tokens[index + 1]; + if (value === undefined) throw new CliUsageError(`--${name} requires a value`, CODEX_CLI_UPDATE_USAGE); + return { value, next: index + 2 }; +} + +function isOption(token: string, name: string): boolean { + return token === `--${name}` || token.startsWith(`--${name}=`); +} + export function parseCodexCliUpdateArgs(argv: readonly string[]): ParsedCodexCliUpdateArgs { // `--json` is accepted in any argv position CLI-wide, so remove it before positional - // validation. Requiring `check` at index 0 first would reject `--json check`, which + // validation. Requiring the action at index 0 first would reject `--json check`, which // automation that puts output flags ahead of the subcommand legitimately produces. let json = false; - const positional: string[] = []; + const rest: string[] = []; for (const token of argv) { if (isJsonOption(token)) { if (json) throw new CliUsageError("--json may be specified only once", CODEX_CLI_UPDATE_USAGE); json = true; continue; } - positional.push(token); + rest.push(token); } - if (positional[0] === "attest") { + if (rest[0] === "attest") { // No options: attest the selected candidate identified from the proof-bound // launcher snapshot. The four explicit paths remain all-or-none. - if (positional.length === 1) { + if (rest.length === 1) { return Object.freeze({ json, attest: "selected" as const }); } const options = new Map([ @@ -93,12 +154,12 @@ export function parseCodexCliUpdateArgs(argv: readonly string[]): ParsedCodexCli ["--npm-cli", "npmCli"], ["--node", "node"], ]); const input: Partial> = {}; - for (let index = 1; index < positional.length; index += 2) { - const key = options.get(positional[index]!); + for (let index = 1; index < rest.length; index += 2) { + const key = options.get(rest[index]!); if (!key || input[key] !== undefined) { throw new CliUsageError("unsupported or duplicate attest option", CODEX_CLI_UPDATE_USAGE); } - const value = positional[index + 1]; + const value = rest[index + 1]; if (!value || !value.trim() || /[\0\r\n]/.test(value) || !(value.startsWith("/") || /^[a-z]:[\\/]/i.test(value))) { throw new CliUsageError("attest options require explicit absolute paths", CODEX_CLI_UPDATE_USAGE); @@ -112,13 +173,76 @@ export function parseCodexCliUpdateArgs(argv: readonly string[]): ParsedCodexCli candidate: input.candidate, npmPrefix: input.npmPrefix, npmCli: input.npmCli, node: input.node, }) }); } - if (positional[0] !== "check") { - throw new CliUsageError("codex-cli-update action must be check or attest", CODEX_CLI_UPDATE_USAGE); + const action = rest[0]; + if (action !== "check" && action !== "plan" && action !== "apply") { + throw new CliUsageError("codex-cli-update action must be check, attest, plan or apply", CODEX_CLI_UPDATE_USAGE); } - if (positional.length > 1) { - throw new CliUsageError("unsupported codex-cli-update argument", CODEX_CLI_UPDATE_USAGE); + + if (action === "check") { + if (rest.length > 1) throw new CliUsageError("unsupported codex-cli-update argument", CODEX_CLI_UPDATE_USAGE); + return Object.freeze({ action, json }); } - return Object.freeze({ json }); + + if (action === "plan") { + let channel: CodexCliUpdateChannel = "latest"; + let seen = false; + let index = 1; + while (index < rest.length) { + const token = rest[index]!; + if (!isOption(token, "channel")) { + throw new CliUsageError("unsupported codex-cli-update argument", CODEX_CLI_UPDATE_USAGE); + } + if (seen) throw new CliUsageError("--channel may be specified only once", CODEX_CLI_UPDATE_USAGE); + const read = optionValue(rest, index, "channel"); + // Only the stable channel is offered. A preview channel would need its own + // provenance story before it may install anything on the operator's behalf. + if (read.value !== "latest") throw new CliUsageError("--channel must be latest", CODEX_CLI_UPDATE_USAGE); + channel = read.value; + seen = true; + index = read.next; + } + return Object.freeze({ action, json, channel }); + } + + let planId: string | null = null; + let index = 1; + while (index < rest.length) { + const token = rest[index]!; + if (!isOption(token, "plan")) { + throw new CliUsageError("unsupported codex-cli-update argument", CODEX_CLI_UPDATE_USAGE); + } + if (planId !== null) throw new CliUsageError("--plan may be specified only once", CODEX_CLI_UPDATE_USAGE); + const read = optionValue(rest, index, "plan"); + if (!PLAN_ID_RE.test(read.value)) { + throw new CliUsageError("--plan must be a plan id from a dry-run", CODEX_CLI_UPDATE_USAGE); + } + planId = read.value; + index = read.next; + } + // Apply is never implicit: the operator quotes a plan id they read in a dry-run. + if (planId === null) throw new CliUsageError("apply requires --plan ", CODEX_CLI_UPDATE_USAGE); + return Object.freeze({ action, json, planId }); +} + +/** + * Inspection inputs for this one-shot CLI process. + * + * The published Node launcher supplies a proof-bound snapshot of configured candidate + * evidence, not selected-runtime admission. A direct Bun or source launch has no such + * proof, so nothing ambient or persisted is inspected at all. + */ +function inspectionDeps(): CodexCliInstallProvenanceDeps { + const trusted = trustedNodeLauncherContext()?.codexCliInspectionEnv; + if (!trusted || trusted.managerRoots === null) return { env: { PATH: "" }, configDir: "." }; + return { + env: { + ...trusted.managerRoots, + CODEX_CLI_PATH: trusted.codexCliPath ?? undefined, + PATH: trusted.path ?? undefined, + PATHEXT: trusted.pathExt ?? undefined, + }, + configDir: trusted.configDir, + }; } export async function handleCodexCliUpdateCommand( @@ -136,7 +260,10 @@ export async function handleCodexCliUpdateCommand( } throw error; } - return runCliAction(async () => { + // A refusal or an unapplied update is a legitimate answer rather than a crash, so the + // outcome exit code is decided here and only a thrown error is left to runCliAction. + let outcome = 0; + const code = await runCliAction(async () => { if ("attest" in parsed) { let report: CodexCliInstallationIdentityReport; try { @@ -167,25 +294,26 @@ export async function handleCodexCliUpdateCommand( printData(report, parsed.json, identitySummary(report)); return; } - const trustedInspectionEnv = trustedNodeLauncherContext()?.codexCliInspectionEnv; - const inspectionDeps: CodexCliInstallProvenanceDeps = trustedInspectionEnv - && trustedInspectionEnv.managerRoots !== null ? { - env: { - ...trustedInspectionEnv.managerRoots, - CODEX_CLI_PATH: trustedInspectionEnv.codexCliPath ?? undefined, - PATH: trustedInspectionEnv.path ?? undefined, - PATHEXT: trustedInspectionEnv.pathExt ?? undefined, - }, - configDir: trustedInspectionEnv.configDir, - // This is a fresh one-shot CLI process. Its proof-bound launcher snapshot - // supplies configured candidate evidence, not selected-runtime admission. - } : { - // Direct Bun/source launches have no pre-dotenv proof. Do not inspect - // ambient or persisted candidate state at all. - env: { PATH: "" }, - configDir: ".", - }; - const report = await (deps.inspectInstall ?? inspectCodexCliInstall)(inspectionDeps); - printData(report, parsed.json, installSummary(report)); + if (parsed.action === "check") { + const report = await (deps.inspectInstall ?? inspectCodexCliInstall)(inspectionDeps()); + printData(report, parsed.json, installSummary(report)); + return; + } + if (parsed.action === "plan") { + const plan = await (deps.createPlan ?? createCodexCliUpdatePlan)({ + channel: parsed.channel, + inspectionDeps: inspectionDeps(), + inspect: deps.inspectInstall, + }); + printData(plan, parsed.json, planSummary(plan)); + return; + } + const result = await (deps.applyPlan ?? applyCodexCliUpdatePlan)(parsed.planId, { + inspectionDeps: inspectionDeps(), + inspect: deps.inspectInstall, + }); + printData(result, parsed.json, applySummary(result)); + outcome = result.status === "applied" ? 0 : 1; }); + return code === 0 ? outcome : code; } diff --git a/src/cli/registry.ts b/src/cli/registry.ts index 6e4756c1664..6d0e970e4f2 100644 --- a/src/cli/registry.ts +++ b/src/cli/registry.ts @@ -419,13 +419,16 @@ export const CLI_COMMANDS: CliCommandEntry[] = [ { name: "system", usage: "ocx system ...", - summary: "Manage headless runtime settings, startup, sync, diagnostics, OpenCodex updates, and read-only Codex CLI inspection.", + summary: "Manage headless runtime settings, startup, sync, diagnostics, OpenCodex updates, and the Codex CLI update manager.", details: [ "system update manages OpenCodex itself.", "ocx system codex-cli-update check [--json]", "ocx system codex-cli-update attest [--json]", "ocx system codex-cli-update attest --candidate --npm-prefix --npm-cli --node [--json]", - "The Codex CLI inspection command makes no package-registry request, does not execute Codex or npm, install or repair software, control a process, or write configuration or cache state.", + "ocx system codex-cli-update plan [--channel latest] [--json]", + "ocx system codex-cli-update apply --plan [--json]", + "check and attest make no package-registry request, do not execute Codex or npm, and write nothing. plan adds a registry query and a process-table read and still writes nothing.", + "apply installs exactly the version bound into the plan id it is given, refuses a plan that no longer matches live evidence, and never stops or restarts Codex.", ], }, { diff --git a/src/cli/system-command.ts b/src/cli/system-command.ts index 4e08fcbc49c..fc5ddf955d6 100644 --- a/src/cli/system-command.ts +++ b/src/cli/system-command.ts @@ -23,6 +23,8 @@ const USAGE = `Usage: ocx system codex-cli-update check [--json] ocx system codex-cli-update attest [--json] ocx system codex-cli-update attest --candidate --npm-prefix --npm-cli --node [--json] + ocx system codex-cli-update plan [--channel latest] [--json] + ocx system codex-cli-update apply --plan [--json] ocx system update check [--channel ] [--json] ocx system update run [--channel ] [--restart ] --yes [--json] ocx system update status [--json] diff --git a/src/codex/app-server-processes.ts b/src/codex/app-server-processes.ts index 355beaa9e50..1b2acdcd131 100644 --- a/src/codex/app-server-processes.ts +++ b/src/codex/app-server-processes.ts @@ -581,6 +581,45 @@ export function listCodexAppServerProcesses(io: CodexAppServerProcessIo = {}): C return matched; } +/** + * Enumeration outcome for callers that must DEFER on an unreadable process table. + * + * {@link listCodexAppServerProcesses} maps enumeration failure to an empty list because its + * kill contract must never signal a process it could not verify. The Codex CLI update + * workflow needs the opposite reading: "no matches" and "could not look" lead to different + * decisions, and only the first one may allow an install to proceed. + */ +export type CodexAppServerProcessScan = + | Readonly<{ kind: "observed"; processes: readonly CodexAppServerProcess[] }> + | Readonly<{ kind: "unavailable" }>; + +/** Same matcher and snapshot sources as {@link listCodexAppServerProcesses}, failing closed. */ +export function scanCodexAppServerProcesses(io: CodexAppServerProcessIo = {}): CodexAppServerProcessScan { + const platform = io.platform ?? process.platform; + const getuid = io.getuid ?? (() => { + try { + return typeof process.getuid === "function" ? process.getuid() : undefined; + } catch { + return undefined; + } + }); + let snapshots: ProcessSnapshot[]; + try { + snapshots = io.listSnapshots ? io.listSnapshots() : defaultListSnapshots(platform, getuid); + } catch { + return Object.freeze({ kind: "unavailable" as const }); + } + const seen = new Set(); + const matched: CodexAppServerProcess[] = []; + for (const snapshot of snapshots) { + if (seen.has(snapshot.pid)) continue; + if (!isCodexAppServerCommandLine(snapshot.commandLine, snapshot.executable)) continue; + seen.add(snapshot.pid); + matched.push({ pid: snapshot.pid, commandLine: snapshot.commandLine }); + } + return Object.freeze({ kind: "observed" as const, processes: Object.freeze(matched) }); +} + export function formatStaleCodexAppServerWarning( processes: readonly { pid: number }[], ): string { diff --git a/src/codex/cli-update-plan.ts b/src/codex/cli-update-plan.ts new file mode 100644 index 00000000000..4bac5452dc8 --- /dev/null +++ b/src/codex/cli-update-plan.ts @@ -0,0 +1,472 @@ +import { spawnSync } from "node:child_process"; +import { createHash } from "node:crypto"; + +import { parseStrictSemver } from "../lib/strict-semver"; +import { npmInvocation } from "../update/npm-invocation.mjs"; +import { + inspectCodexCliInstall, + type CodexCliInstallKind, + type CodexCliInstallProvenanceDeps, + type CodexCliInstallReport, +} from "./cli-install-provenance"; +import { + scanCodexAppServerProcesses, + type CodexAppServerProcessIo, + type CodexAppServerProcessScan, +} from "./app-server-processes"; + +/** + * Phase 2 of the Codex CLI update manager: a deterministic dry-run plan and an explicit + * apply engine. + * + * Phase 1 ({@link inspectCodexCliInstall}) answers who owns the installation. It never + * queries a registry, never enumerates processes and never writes. This module adds the + * three inputs it deliberately left out — an exact resolved target with registry + * integrity, live session blockers, and a decision — and nothing else. + * + * Two invariants shape the whole module: + * + * - No persisted state. The plan is not stored anywhere; {@link CodexCliUpdatePlan.planId} + * is a digest over the evidence the decision rests on, and apply recomputes it from + * live evidence. A stale plan is refused, never silently regenerated. + * - Nothing is terminated or restarted. A live Codex session refuses the plan; it is + * never signalled, killed or waited on. + */ + +export const CODEX_CLI_PACKAGE = "@openai/codex"; +export const CODEX_CLI_UPDATE_SCHEMA_VERSION = 1 as const; + +/** Only the stable channel is exposed; a moving dist-tag is resolved before it is used. */ +export type CodexCliUpdateChannel = "latest"; + +export type CodexCliUpdateRefusal = + | "windows_inspection_deferred" + | "not_managed" + | "installed_version_unverified" + | "target_unresolved" + | "already_current" + | "blocked_active_session" + | "blocked_process_state_unknown"; + +/** + * `not-evaluated` is not a weaker `unknown`: it records that the plan was already refused + * on ownership or target grounds, so the process table was never read. Only `unknown` + * means an enumeration attempt failed. + */ +export type CodexCliUpdateSessionState = "none" | "active" | "unknown" | "not-evaluated"; + +export interface CodexCliUpdateSession { + readonly state: CodexCliUpdateSessionState; + /** Match count only. Command lines and paths never leave the process scanner. */ + readonly matches: number | null; +} + +export type CodexCliUpdateTarget = + | Readonly<{ kind: "resolved"; version: string; integrity: string }> + | Readonly<{ kind: "unresolved"; reason: string }>; + +export interface CodexCliUpdatePlan { + readonly schemaVersion: typeof CODEX_CLI_UPDATE_SCHEMA_VERSION; + readonly package: typeof CODEX_CLI_PACKAGE; + readonly channel: CodexCliUpdateChannel; + readonly applicable: boolean; + readonly refusal: CodexCliUpdateRefusal | null; + /** Present only for an applicable plan; there is nothing to quote for a refusal. */ + readonly planId: string | null; + readonly provenance: CodexCliInstallKind; + readonly managed: boolean; + readonly installedVersion: string | null; + readonly versionEvidence: CodexCliInstallReport["versionEvidence"]["kind"]; + readonly location: string | null; + readonly targetVersion: string | null; + readonly targetIntegrity: string | null; + /** True when the pre-update shim was `matched`, i.e. this install owns a live shim. */ + readonly shimEligible: boolean; + readonly session: CodexCliUpdateSession; + /** Exactly the argv apply would run, for the operator to read before approving it. */ + readonly command: readonly string[] | null; +} + +export type CodexCliUpdateApplyStatus = + | "applied" + | "not_applied" + | "applied_shim_repair_required" + | "ambiguous" + | "refused"; + +export type CodexCliUpdateApplyRefusal = CodexCliUpdateRefusal | "plan_stale" | "plan_unknown"; + +export interface CodexCliUpdateApplyResult { + readonly schemaVersion: typeof CODEX_CLI_UPDATE_SCHEMA_VERSION; + readonly status: CodexCliUpdateApplyStatus; + readonly refusal: CodexCliUpdateApplyRefusal | null; + readonly planId: string | null; + readonly targetVersion: string | null; + readonly installedVersionBefore: string | null; + readonly installedVersionAfter: string | null; + /** Evidence only. The readback classifies the outcome; the exit code never does. */ + readonly installerExitCode: number | null; + readonly shim: Readonly<{ attempted: boolean; restored: boolean; status: string | null }>; +} + +export interface CodexCliUpdateInstallerResult { + /** `null` for a spawn failure or timeout, mirroring `spawnSync` status. */ + readonly exitCode: number | null; +} + +export interface CodexCliUpdateShimRestoreResult { + readonly status: string; +} + +export interface CodexCliUpdatePlanDeps { + readonly inspect?: (deps: CodexCliInstallProvenanceDeps) => Promise; + readonly inspectionDeps?: CodexCliInstallProvenanceDeps; + readonly platform?: NodeJS.Platform; + readonly channel?: CodexCliUpdateChannel; + readonly scanProcesses?: (io?: CodexAppServerProcessIo) => CodexAppServerProcessScan; + readonly processIo?: CodexAppServerProcessIo; + readonly resolveTarget?: (channel: CodexCliUpdateChannel) => CodexCliUpdateTarget; +} + +export interface CodexCliUpdateApplyDeps extends CodexCliUpdatePlanDeps { + readonly runInstaller?: (version: string) => CodexCliUpdateInstallerResult; + readonly restoreShim?: () => Promise; +} + +const PLAN_ID_LENGTH = 32; +const PLAN_ID_RE = /^[0-9a-f]{32}$/; +const REGISTRY_TIMEOUT_MS = 12_000; +const INSTALL_TIMEOUT_MS = 300_000; +const SHA512_INTEGRITY_RE = /^sha512-[A-Za-z0-9+/=]+$/; + +/** The one command apply is allowed to run, in argv form. */ +export function codexCliUpdateCommand(version: string): readonly string[] { + return Object.freeze(["npm", "install", "-g", `${CODEX_CLI_PACKAGE}@${version}`]); +} + +/** + * Digest over exactly the evidence the decision rests on. + * + * Session blockers are deliberately absent: a session that starts or ends between dry-run + * and apply must not invalidate an otherwise identical plan, and it is re-read at apply + * time where it can only ever refuse. Everything else — ownership, installed version, + * location, resolved target, integrity, shim eligibility — is bound, so any drift produces + * a different id and `apply` refuses instead of installing something the operator did not + * read. + */ +export function codexCliUpdatePlanId(bound: { + readonly platform: NodeJS.Platform; + readonly provenance: CodexCliInstallKind; + readonly installedVersion: string; + readonly location: string | null; + readonly channel: CodexCliUpdateChannel; + readonly targetVersion: string; + readonly targetIntegrity: string; + readonly shimEligible: boolean; +}): string { + // Ordered pairs, not object key order: the digest must not depend on how a caller + // happened to build the record. + const fields: readonly (readonly [string, string])[] = [ + ["schemaVersion", String(CODEX_CLI_UPDATE_SCHEMA_VERSION)], + ["package", CODEX_CLI_PACKAGE], + ["platform", bound.platform], + ["provenance", bound.provenance], + ["installedVersion", bound.installedVersion], + ["location", bound.location ?? ""], + ["channel", bound.channel], + ["targetVersion", bound.targetVersion], + ["targetIntegrity", bound.targetIntegrity], + ["shimEligible", bound.shimEligible ? "1" : "0"], + ]; + const hash = createHash("sha256"); + for (const [key, value] of fields) hash.update(`${key}=${value}\n`); + return hash.digest("hex").slice(0, PLAN_ID_LENGTH); +} + +function npmTarget(args: readonly string[]): { bin: string; args: string[]; options: { windowsVerbatimArguments?: boolean } } | null { + const invocation = npmInvocation(args); + if (!invocation) return null; + return { bin: invocation.file, args: invocation.args, options: invocation.options }; +} + +/** + * Resolve the channel to one exact version plus its sha512 integrity token. + * + * OpenCodex's own updater treats a failed integrity query as `skipped` and proceeds + * best-effort. That is a defensible trade for the package we publish ourselves; it is not + * acceptable for a foreign package we are about to install on the operator's behalf, so + * every failure lane here is a refusal. + */ +export function resolveCodexCliUpdateTarget( + channel: CodexCliUpdateChannel, + spawn: typeof spawnSync = spawnSync, +): CodexCliUpdateTarget { + const versionTarget = npmTarget(["view", `${CODEX_CLI_PACKAGE}@${channel}`, "version"]); + if (!versionTarget) return Object.freeze({ kind: "unresolved" as const, reason: "npm executable was not found on a trusted PATH entry" }); + const versionRun = spawn(versionTarget.bin, versionTarget.args, { + encoding: "utf8", + timeout: REGISTRY_TIMEOUT_MS, + windowsHide: true, + ...versionTarget.options, + }); + // status === null covers a timeout as well as a spawn failure. + if (versionRun.status !== 0) { + return Object.freeze({ kind: "unresolved" as const, reason: `registry version query failed (status ${versionRun.status ?? "timeout"})` }); + } + const version = (versionRun.stdout ?? "").trim(); + if (!parseStrictSemver(version)) { + return Object.freeze({ kind: "unresolved" as const, reason: "registry returned no exact version" }); + } + + const integrityTarget = npmTarget(["view", `${CODEX_CLI_PACKAGE}@${version}`, "dist.integrity"]); + if (!integrityTarget) return Object.freeze({ kind: "unresolved" as const, reason: "npm executable was not found on a trusted PATH entry" }); + const integrityRun = spawn(integrityTarget.bin, integrityTarget.args, { + encoding: "utf8", + timeout: REGISTRY_TIMEOUT_MS, + windowsHide: true, + ...integrityTarget.options, + }); + if (integrityRun.status !== 0) { + return Object.freeze({ kind: "unresolved" as const, reason: `registry integrity query failed (status ${integrityRun.status ?? "timeout"})` }); + } + // `dist.integrity` may arrive quoted or as a space-separated multi-hash list. + const tokens = (integrityRun.stdout ?? "").replace(/["']/g, "").trim().split(/\s+/).filter(Boolean); + const integrity = tokens.find(token => SHA512_INTEGRITY_RE.test(token)); + if (!integrity) { + return Object.freeze({ kind: "unresolved" as const, reason: "registry returned no sha512 integrity token" }); + } + return Object.freeze({ kind: "resolved" as const, version, integrity }); +} + +function defaultRunInstaller(version: string): CodexCliUpdateInstallerResult { + const target = npmTarget(["install", "-g", `${CODEX_CLI_PACKAGE}@${version}`]); + if (!target) return Object.freeze({ exitCode: null }); + const run = spawnSync(target.bin, target.args, { + encoding: "utf8", + timeout: INSTALL_TIMEOUT_MS, + windowsHide: true, + stdio: "inherit", + ...target.options, + }); + return Object.freeze({ exitCode: run.status }); +} + +async function defaultRestoreShim(): Promise { + // Imported lazily: the plan engine stays a pure module that tests can drive without + // pulling in the shim state store and the config directory graph. + const { autoRestoreCodexShim } = await import("./shim"); + const result = autoRestoreCodexShim({ enabled: () => true }); + return Object.freeze({ status: result.status }); +} + +function refusedPlan( + refusal: CodexCliUpdateRefusal, + report: CodexCliInstallReport, + channel: CodexCliUpdateChannel, + target: CodexCliUpdateTarget | null, + session: CodexCliUpdateSession, +): CodexCliUpdatePlan { + return Object.freeze({ + schemaVersion: CODEX_CLI_UPDATE_SCHEMA_VERSION, + package: CODEX_CLI_PACKAGE, + channel, + applicable: false, + refusal, + planId: null, + provenance: report.provenance, + managed: report.managed, + installedVersion: report.packageVersion, + versionEvidence: report.versionEvidence.kind, + location: report.location, + targetVersion: target?.kind === "resolved" ? target.version : null, + targetIntegrity: target?.kind === "resolved" ? target.integrity : null, + shimEligible: report.shim.status === "matched", + session, + command: null, + }); +} + +const NOT_EVALUATED: CodexCliUpdateSession = Object.freeze({ state: "not-evaluated" as const, matches: null }); + +/** + * Build the dry-run plan. Reads only; nothing here writes, signals or installs. + * + * The refusal order is deliberate. Ownership and target questions are settled before the + * process table is read, so a machine that can never be updated by this workflow does not + * pay for an enumeration, and the reported refusal is the decisive one rather than + * whichever check happened to run first. + */ +export async function createCodexCliUpdatePlan(deps: CodexCliUpdatePlanDeps = {}): Promise { + const channel: CodexCliUpdateChannel = deps.channel ?? "latest"; + const platform = deps.platform ?? process.platform; + const inspect = deps.inspect ?? inspectCodexCliInstall; + const report = await inspect(deps.inspectionDeps ?? {}); + + // Phase 1 performs no candidate filesystem I/O at all on Windows, so there is no + // ownership evidence to build a plan on. This stays dormant until the handle-bound + // provenance layer exists; pretending otherwise would require exactly the reads phase 1 + // refused to perform. + if (platform === "win32" || report.reason === "windows_inspection_deferred") { + return refusedPlan("windows_inspection_deferred", report, channel, null, NOT_EVALUATED); + } + if (!report.managed || report.provenance !== "npm-global") { + return refusedPlan("not_managed", report, channel, null, NOT_EVALUATED); + } + // An advisory runtime string is what a candidate binary said about itself. Only + // package-manifest evidence states what is installed on disk, and only that can be + // compared with a registry version or read back after an install. + const installedVersion = report.versionEvidence.kind === "package-manifest" ? report.packageVersion : null; + if (!installedVersion || !parseStrictSemver(installedVersion)) { + return refusedPlan("installed_version_unverified", report, channel, null, NOT_EVALUATED); + } + + const resolveTarget = deps.resolveTarget ?? (ch => resolveCodexCliUpdateTarget(ch)); + const target = resolveTarget(channel); + if (target.kind !== "resolved") { + return refusedPlan("target_unresolved", report, channel, target, NOT_EVALUATED); + } + if (target.version === installedVersion) { + return refusedPlan("already_current", report, channel, target, NOT_EVALUATED); + } + + const scan = (deps.scanProcesses ?? scanCodexAppServerProcesses)(deps.processIo ?? {}); + if (scan.kind !== "observed") { + // An unreadable process table is not "no sessions". `listCodexAppServerProcesses` + // maps that failure to an empty list because its kill contract must never signal a + // process it could not verify; the update contract has to defer instead. + return refusedPlan("blocked_process_state_unknown", report, channel, target, Object.freeze({ state: "unknown" as const, matches: null })); + } + const matches = scan.processes.length; + if (matches > 0) { + return refusedPlan("blocked_active_session", report, channel, target, Object.freeze({ state: "active" as const, matches })); + } + + const shimEligible = report.shim.status === "matched"; + return Object.freeze({ + schemaVersion: CODEX_CLI_UPDATE_SCHEMA_VERSION, + package: CODEX_CLI_PACKAGE, + channel, + applicable: true, + refusal: null, + planId: codexCliUpdatePlanId({ + platform, + provenance: report.provenance, + installedVersion, + location: report.location, + channel, + targetVersion: target.version, + targetIntegrity: target.integrity, + shimEligible, + }), + provenance: report.provenance, + managed: report.managed, + installedVersion, + versionEvidence: report.versionEvidence.kind, + location: report.location, + targetVersion: target.version, + targetIntegrity: target.integrity, + shimEligible, + session: Object.freeze({ state: "none" as const, matches: 0 }), + command: codexCliUpdateCommand(target.version), + }); +} + +function refusedApply( + refusal: CodexCliUpdateApplyRefusal, + plan: CodexCliUpdatePlan | null, +): CodexCliUpdateApplyResult { + return Object.freeze({ + schemaVersion: CODEX_CLI_UPDATE_SCHEMA_VERSION, + status: "refused" as const, + refusal, + planId: plan?.planId ?? null, + targetVersion: plan?.targetVersion ?? null, + installedVersionBefore: plan?.installedVersion ?? null, + installedVersionAfter: null, + installerExitCode: null, + shim: Object.freeze({ attempted: false, restored: false, status: null }), + }); +} + +/** + * Apply a plan the operator has read. + * + * The plan is recomputed from live evidence and the id must match, so the operator + * approves the exact target that is about to be installed. The install itself is one + * command against an exact version; the outcome is classified from a fresh inspection, + * never from the installer exit code, and nothing is retried or rolled back automatically. + */ +export async function applyCodexCliUpdatePlan( + planId: string, + deps: CodexCliUpdateApplyDeps = {}, +): Promise { + if (!PLAN_ID_RE.test(planId)) return refusedApply("plan_unknown", null); + + const plan = await createCodexCliUpdatePlan(deps); + if (!plan.applicable || !plan.planId || !plan.targetVersion || !plan.installedVersion) { + return refusedApply(plan.refusal ?? "plan_unknown", plan); + } + // Any drift in ownership, installed version, location, target or shim eligibility + // changes the id. Refuse rather than regenerate: the operator would otherwise approve + // one plan and install another. + if (plan.planId !== planId) return refusedApply("plan_stale", plan); + + const targetVersion = plan.targetVersion; + const before = plan.installedVersion; + const installer = (deps.runInstaller ?? defaultRunInstaller)(targetVersion); + + const inspect = deps.inspect ?? inspectCodexCliInstall; + let readback: CodexCliInstallReport | null = null; + try { + readback = await inspect(deps.inspectionDeps ?? {}); + } catch { + readback = null; + } + + const after = readback + && readback.provenance === "npm-global" + && readback.versionEvidence.kind === "package-manifest" + ? readback.packageVersion + : null; + + const result = ( + status: CodexCliUpdateApplyStatus, + shim: { attempted: boolean; restored: boolean; status: string | null }, + ): CodexCliUpdateApplyResult => Object.freeze({ + schemaVersion: CODEX_CLI_UPDATE_SCHEMA_VERSION, + status, + refusal: null, + planId: plan.planId, + targetVersion, + installedVersionBefore: before, + installedVersionAfter: after, + installerExitCode: installer.exitCode, + shim: Object.freeze({ ...shim }), + }); + + const noShim = { attempted: false, restored: false, status: null }; + if (after !== targetVersion) { + // A failed readback, an unchanged version and a third version are all reported + // as-is. None of them is retried, and none of them is rolled back. + if (after !== null && after === before) return result("not_applied", noShim); + return result("ambiguous", noShim); + } + + // The shim is touched only when this installation owned a matched shim before the + // update and npm replaced it. A shim that was never tracked stays untracked. + if (!plan.shimEligible || readback?.shim.status === "matched") { + return result("applied", noShim); + } + let restore: CodexCliUpdateShimRestoreResult; + try { + restore = await (deps.restoreShim ?? defaultRestoreShim)(); + } catch { + restore = Object.freeze({ status: "failed" }); + } + const restored = restore.status === "restored" || restore.status === "healthy"; + return result(restored ? "applied" : "applied_shim_repair_required", { + attempted: true, + restored, + status: restore.status, + }); +} diff --git a/tests/cli/cli-codex-cli-update.test.ts b/tests/cli/cli-codex-cli-update.test.ts index e31c675a32b..6ecccb386c3 100644 --- a/tests/cli/cli-codex-cli-update.test.ts +++ b/tests/cli/cli-codex-cli-update.test.ts @@ -198,9 +198,9 @@ describe("Codex CLI update CLI", () => { }); test("parses the shared JSON flag spellings within the exact check grammar", () => { - expect(parseCodexCliUpdateArgs(["check"])).toEqual({ json: false }); + expect(parseCodexCliUpdateArgs(["check"])).toEqual({ action: "check", json: false }); for (const flag of ["--json", "--json=true", "-json", "—json"]) { - expect(parseCodexCliUpdateArgs(["check", flag])).toEqual({ json: true }); + expect(parseCodexCliUpdateArgs(["check", flag])).toEqual({ action: "check", json: true }); } for (const args of [ ["check", "--channel", "latest"], @@ -217,7 +217,7 @@ describe("Codex CLI update CLI", () => { */ test("the JSON flag is accepted before the check action", () => { for (const flag of ["--json", "--json=true", "-json", "—json"]) { - expect(parseCodexCliUpdateArgs([flag, "check"])).toEqual({ json: true }); + expect(parseCodexCliUpdateArgs([flag, "check"])).toEqual({ action: "check", json: true }); } // Duplicate detection and positional validation still hold in that order. expect(() => parseCodexCliUpdateArgs(["--json", "check", "--json"])).toThrow(); @@ -376,3 +376,87 @@ describe("Codex CLI update CLI", () => { } }); }); + +/** + * Phase 2 verbs. The parser is the authorization boundary here: apply must never be + * reachable without a plan id the operator read in a dry-run. + */ +describe("Codex CLI update plan and apply grammar", () => { + const PLAN_ID = "0123456789abcdef0123456789abcdef"; + + test("plan defaults to the stable channel and accepts both option spellings", () => { + expect(parseCodexCliUpdateArgs(["plan"])).toEqual({ action: "plan", json: false, channel: "latest" }); + expect(parseCodexCliUpdateArgs(["plan", "--channel", "latest"])).toEqual({ action: "plan", json: false, channel: "latest" }); + expect(parseCodexCliUpdateArgs(["plan", "--channel=latest", "--json"])).toEqual({ action: "plan", json: true, channel: "latest" }); + }); + + test("apply requires a well-formed plan id", () => { + expect(parseCodexCliUpdateArgs(["apply", "--plan", PLAN_ID])).toEqual({ action: "apply", json: false, planId: PLAN_ID }); + expect(parseCodexCliUpdateArgs(["--json", "apply", "--plan=" + PLAN_ID])).toEqual({ action: "apply", json: true, planId: PLAN_ID }); + for (const args of [ + ["apply"], + ["apply", "--plan"], + ["apply", "--plan", "short"], + ["apply", "--plan", PLAN_ID.toUpperCase()], + ["apply", "--plan", PLAN_ID, "--plan", PLAN_ID], + ["apply", "--channel", "latest"], + ["plan", "--plan", PLAN_ID], + ["plan", "--channel", "preview"], + ["plan", "extra"], + ]) expect(() => parseCodexCliUpdateArgs(args)).toThrow(); + }); + + test("a refused dry-run is a normal answer and still exits 0", async () => { + let inspected = 0; + const code = await handleCodexCliUpdateCommand(["plan", "--json"], { + inspectInstall: async () => { inspected += 1; return report; }, + createPlan: async () => ({ + schemaVersion: 1, package: "@openai/codex", channel: "latest", + applicable: false, refusal: "not_managed", planId: null, + provenance: "app-bundle", managed: false, installedVersion: null, + versionEvidence: "unavailable", location: null, + targetVersion: null, targetIntegrity: null, shimEligible: false, + session: { state: "not-evaluated", matches: null }, command: null, + }), + }); + expect(code).toBe(0); + // The plan engine owns inspection; the command must not run a second one. + expect(inspected).toBe(0); + }); + + test("apply passes the operator's plan id through and reports a refusal as nonzero", async () => { + const seen: string[] = []; + const code = await handleCodexCliUpdateCommand(["apply", "--plan", PLAN_ID, "--json"], { + applyPlan: async planId => { + seen.push(planId); + return { + schemaVersion: 1, status: "refused", refusal: "plan_stale", planId: null, + targetVersion: null, installedVersionBefore: null, installedVersionAfter: null, + installerExitCode: null, shim: { attempted: false, restored: false, status: null }, + }; + }, + }); + expect(seen).toEqual([PLAN_ID]); + expect(code).toBe(1); + }); + + test("a completed apply exits 0", async () => { + const code = await handleCodexCliUpdateCommand(["apply", "--plan", PLAN_ID], { + applyPlan: async () => ({ + schemaVersion: 1, status: "applied", refusal: null, planId: PLAN_ID, + targetVersion: "1.1.0", installedVersionBefore: "1.0.0", installedVersionAfter: "1.1.0", + installerExitCode: 0, shim: { attempted: false, restored: false, status: null }, + }), + }); + expect(code).toBe(0); + }); + + test("a malformed plan id is rejected before anything is applied", async () => { + let applies = 0; + const code = await handleCodexCliUpdateCommand(["apply", "--plan", "nope"], { + applyPlan: async () => { applies += 1; throw new Error("unreachable"); }, + }); + expect(code).toBe(2); + expect(applies).toBe(0); + }); +}); diff --git a/tests/cli/cli-registry.test.ts b/tests/cli/cli-registry.test.ts index 27084cecae9..c8d4c83a33c 100644 --- a/tests/cli/cli-registry.test.ts +++ b/tests/cli/cli-registry.test.ts @@ -99,10 +99,13 @@ describe("CLI command registry parity", () => { expect(new Set(names).size).toBe(names.length); }); - test("system help exposes the exact Codex CLI inspection grammar", () => { + test("system help exposes the exact Codex CLI update grammar", () => { const details = findCommand("system")?.details ?? []; expect(details).toContain("ocx system codex-cli-update check [--json]"); - expect(details.some(line => line.includes("dry-run"))).toBe(false); + expect(details).toContain("ocx system codex-cli-update plan [--channel latest] [--json]"); + // apply must never render as an argument-free verb: the plan id IS the authorization. + expect(details).toContain("ocx system codex-cli-update apply --plan [--json]"); + expect(details.some(line => line.startsWith("ocx system codex-cli-update apply") && !line.includes("--plan"))).toBe(false); }); test("GUI registry usage documents explicit-origin single-use pairing", () => { diff --git a/tests/codex-integration/codex-cli-update-plan.test.ts b/tests/codex-integration/codex-cli-update-plan.test.ts new file mode 100644 index 00000000000..1fd7f269f80 --- /dev/null +++ b/tests/codex-integration/codex-cli-update-plan.test.ts @@ -0,0 +1,461 @@ +import { describe, expect, test } from "bun:test"; + +import { scanCodexAppServerProcesses } from "../../src/codex/app-server-processes"; +import type { CodexCliInstallReport } from "../../src/codex/cli-install-provenance"; +import { + applyCodexCliUpdatePlan, + codexCliUpdatePlanId, + createCodexCliUpdatePlan, + resolveCodexCliUpdateTarget, + type CodexCliUpdateApplyDeps, + type CodexCliUpdatePlan, + type CodexCliUpdatePlanDeps, + type CodexCliUpdateTarget, +} from "../../src/codex/cli-update-plan"; + +/** + * Phase 2 of the Codex CLI update manager. + * + * Every case here is about one of three refusals to be casual: adopting an installation + * we do not own, installing a target the registry did not pin, and reading an unreadable + * process table as "nothing is running". + */ + +const MANAGED: CodexCliInstallReport = Object.freeze({ + schemaVersion: 1, + candidateAvailable: true, + candidateVersion: "1.0.0", + candidateSource: "environment", + selectionAttested: true, + versionEvidence: { kind: "package-manifest" }, + provenance: "npm-global", + managed: true, + reason: "npm_global_unverified", + location: "/@openai/codex", + packageVersion: "1.0.0", + shim: { status: "not-tracked", backingKind: null }, + evidence: ["package_manifest", "global_npm_layout"], +}); + +function report(overrides: Partial = {}): CodexCliInstallReport { + return { ...MANAGED, ...overrides } as CodexCliInstallReport; +} + +const RESOLVED: CodexCliUpdateTarget = Object.freeze({ + kind: "resolved", + version: "1.1.0", + integrity: "sha512-AAAA", +}); + +function planDeps(overrides: Partial = {}): CodexCliUpdatePlanDeps { + return { + platform: "linux", + inspect: async () => report(), + resolveTarget: () => RESOLVED, + scanProcesses: () => ({ kind: "observed", processes: [] }), + ...overrides, + }; +} + +async function applicablePlan(overrides: Partial = {}): Promise { + const plan = await createCodexCliUpdatePlan(planDeps(overrides)); + expect(plan.applicable).toBe(true); + return plan; +} + +describe("Codex CLI update dry-run plan", () => { + test("an applicable plan pins the exact resolved version and quotes the command it would run", async () => { + const plan = await applicablePlan(); + expect(plan.refusal).toBeNull(); + expect(plan.targetVersion).toBe("1.1.0"); + expect(plan.targetIntegrity).toBe("sha512-AAAA"); + expect(plan.installedVersion).toBe("1.0.0"); + expect(plan.session).toEqual({ state: "none", matches: 0 }); + // The dist-tag is resolved once and bound; the install can never widen back to it. + expect(plan.command).toEqual(["npm", "install", "-g", "@openai/codex@1.1.0"]); + expect(plan.planId).toMatch(/^[0-9a-f]{32}$/); + }); + + test("Windows defers without querying the registry or the process table", async () => { + let queries = 0; + let scans = 0; + const plan = await createCodexCliUpdatePlan(planDeps({ + platform: "win32", + inspect: async () => report({ reason: "windows_inspection_deferred", managed: false, provenance: "unknown" }), + resolveTarget: () => { queries += 1; return RESOLVED; }, + scanProcesses: () => { scans += 1; return { kind: "observed", processes: [] }; }, + })); + expect(plan.applicable).toBe(false); + expect(plan.refusal).toBe("windows_inspection_deferred"); + // Phase 1 reads nothing on Windows, so there is no ownership evidence to spend a + // registry request or a process enumeration on. + expect(queries).toBe(0); + expect(scans).toBe(0); + expect(plan.session.state).toBe("not-evaluated"); + }); + + test("an installation we do not own is never adopted", async () => { + for (const owned of [ + report({ managed: false, provenance: "app-bundle", reason: "app_bundle" }), + report({ managed: false, provenance: "version-manager", reason: "version_manager_owned" }), + report({ managed: false, provenance: "standalone-unverified", reason: "unverified_standalone" }), + report({ managed: true, provenance: "version-manager", reason: "version_manager_owned" }), + ]) { + const plan = await createCodexCliUpdatePlan(planDeps({ inspect: async () => owned })); + expect(plan.refusal).toBe("not_managed"); + expect(plan.planId).toBeNull(); + expect(plan.command).toBeNull(); + } + }); + + test("an advisory runtime version is not evidence of what is installed", async () => { + // A candidate binary reporting its own version cannot be compared with a registry + // version or read back after an install; only the package manifest can. + const plan = await createCodexCliUpdatePlan(planDeps({ + inspect: async () => report({ versionEvidence: { kind: "advisory-runtime" } }), + })); + expect(plan.refusal).toBe("installed_version_unverified"); + }); + + test("a version the registry did not pin with integrity is refused, not installed best-effort", async () => { + const plan = await createCodexCliUpdatePlan(planDeps({ + resolveTarget: () => ({ kind: "unresolved", reason: "registry integrity query failed (status timeout)" }), + })); + expect(plan.refusal).toBe("target_unresolved"); + expect(plan.targetVersion).toBeNull(); + expect(plan.command).toBeNull(); + }); + + test("an already current installation has nothing to apply", async () => { + const plan = await createCodexCliUpdatePlan(planDeps({ + resolveTarget: () => ({ kind: "resolved", version: "1.0.0", integrity: "sha512-AAAA" }), + })); + expect(plan.refusal).toBe("already_current"); + }); + + test("an unreadable process table defers instead of reading as no live session", async () => { + const plan = await createCodexCliUpdatePlan(planDeps({ scanProcesses: () => ({ kind: "unavailable" }) })); + expect(plan.refusal).toBe("blocked_process_state_unknown"); + expect(plan.session).toEqual({ state: "unknown", matches: null }); + }); + + test("a live Codex session refuses the plan and is never signalled", async () => { + const plan = await createCodexCliUpdatePlan(planDeps({ + scanProcesses: () => ({ kind: "observed", processes: [{ pid: 4242, commandLine: "node app-server --secret" }] }), + })); + expect(plan.refusal).toBe("blocked_active_session"); + expect(plan.session).toEqual({ state: "active", matches: 1 }); + // Only the count crosses the boundary. Command lines carry paths and arguments. + expect(JSON.stringify(plan)).not.toContain("secret"); + expect(JSON.stringify(plan)).not.toContain("4242"); + }); +}); + +describe("Codex CLI update plan identity", () => { + test("every bound field changes the id", async () => { + const base = await applicablePlan(); + const variants: Partial[] = [ + { resolveTarget: () => ({ kind: "resolved", version: "1.2.0", integrity: "sha512-AAAA" }) }, + { resolveTarget: () => ({ kind: "resolved", version: "1.1.0", integrity: "sha512-BBBB" }) }, + { inspect: async () => report({ packageVersion: "1.0.1", candidateVersion: "1.0.1" }) }, + { inspect: async () => report({ location: "/other/@openai/codex" }) }, + { inspect: async () => report({ shim: { status: "matched", backingKind: "backup" } }) }, + ]; + for (const variant of variants) { + const plan = await applicablePlan(variant); + expect(plan.planId).not.toBe(base.planId); + } + }); + + test("a session starting or ending between dry-run and apply does not invalidate the plan", async () => { + // Blockers are re-read at apply time where they can only refuse. Binding them into + // the id would expire a plan the operator read correctly, for a reason that cannot + // make the install wrong. + const first = await applicablePlan(); + const second = await applicablePlan({ + scanProcesses: () => ({ kind: "observed", processes: [] }), + }); + expect(second.planId).toBe(first.planId); + }); + + test("the id is a digest of the bound evidence, not a random handle", () => { + const bound = { + platform: "linux" as NodeJS.Platform, + provenance: "npm-global" as const, + installedVersion: "1.0.0", + location: "/@openai/codex", + channel: "latest" as const, + targetVersion: "1.1.0", + targetIntegrity: "sha512-AAAA", + shimEligible: false, + }; + expect(codexCliUpdatePlanId(bound)).toBe(codexCliUpdatePlanId(bound)); + expect(codexCliUpdatePlanId({ ...bound, shimEligible: true })).not.toBe(codexCliUpdatePlanId(bound)); + }); +}); + +function applyDeps( + overrides: Partial = {}, + installs: string[] = [], +): CodexCliUpdateApplyDeps { + return { + ...planDeps(), + runInstaller: version => { installs.push(version); return { exitCode: 0 }; }, + restoreShim: async () => ({ status: "restored" }), + ...overrides, + }; +} + +describe("Codex CLI update apply", () => { + test("a stale or unknown plan id installs nothing", async () => { + const installs: string[] = []; + const plan = await applicablePlan(); + + const unknown = await applyCodexCliUpdatePlan("not-a-plan-id", applyDeps({}, installs)); + expect(unknown.status).toBe("refused"); + expect(unknown.refusal).toBe("plan_unknown"); + + const stale = await applyCodexCliUpdatePlan("0".repeat(32), applyDeps({}, installs)); + expect(stale.status).toBe("refused"); + expect(stale.refusal).toBe("plan_stale"); + expect(stale.planId).toBe(plan.planId); + + expect(installs).toEqual([]); + }); + + test("drift between dry-run and apply refuses rather than regenerating the plan", async () => { + const installs: string[] = []; + const plan = await applicablePlan(); + // The operator read a plan for 1.1.0; the registry has since moved on. + const drifted = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + resolveTarget: () => ({ kind: "resolved", version: "1.3.0", integrity: "sha512-CCCC" }), + }, installs)); + expect(drifted.status).toBe("refused"); + expect(drifted.refusal).toBe("plan_stale"); + expect(installs).toEqual([]); + }); + + test("a session that appeared after the dry-run refuses the apply", async () => { + const installs: string[] = []; + const plan = await applicablePlan(); + const blocked = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + scanProcesses: () => ({ kind: "observed", processes: [{ pid: 7, commandLine: "codex app-server" }] }), + }, installs)); + expect(blocked.status).toBe("refused"); + expect(blocked.refusal).toBe("blocked_active_session"); + expect(installs).toEqual([]); + }); + + test("the readback classifies the result, and installs exactly the pinned version", async () => { + const installs: string[] = []; + const plan = await applicablePlan(); + let inspections = 0; + const result = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + inspect: async () => { + inspections += 1; + return inspections === 1 ? report() : report({ packageVersion: "1.1.0", candidateVersion: "1.1.0" }); + }, + }, installs)); + expect(installs).toEqual(["1.1.0"]); + expect(result.status).toBe("applied"); + expect(result.installedVersionBefore).toBe("1.0.0"); + expect(result.installedVersionAfter).toBe("1.1.0"); + expect(result.shim).toEqual({ attempted: false, restored: false, status: null }); + }); + + test("a nonzero installer exit never overrides a readback that shows the target", async () => { + const plan = await applicablePlan(); + let inspections = 0; + const result = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + runInstaller: () => ({ exitCode: 1 }), + inspect: async () => { + inspections += 1; + return inspections === 1 ? report() : report({ packageVersion: "1.1.0" }); + }, + })); + expect(result.status).toBe("applied"); + expect(result.installerExitCode).toBe(1); + }); + + test("an unchanged version is not applied, and is not retried", async () => { + const installs: string[] = []; + const plan = await applicablePlan(); + const result = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + runInstaller: version => { installs.push(version); return { exitCode: 1 }; }, + }, installs)); + expect(result.status).toBe("not_applied"); + expect(result.installedVersionAfter).toBe("1.0.0"); + expect(installs).toEqual(["1.1.0"]); + }); + + test("a failed readback, a third version or changed provenance is ambiguous", async () => { + const plan = await applicablePlan(); + const cases: [Partial, string | null][] = [ + [{ inspect: async () => { throw new Error("readback failed"); } }, null], + [{ inspect: async () => report({ packageVersion: "9.9.9" }) }, "9.9.9"], + [{ inspect: async () => report({ provenance: "version-manager" }) }, null], + [{ inspect: async () => report({ versionEvidence: { kind: "advisory-runtime" } }) }, null], + ]; + for (const [override, after] of cases) { + // The first inspection builds the plan, so these must still produce a plan id; + // drive them through a plan whose deps differ only in the readback. + const first = { ...applyDeps(), ...override } as CodexCliUpdateApplyDeps; + let calls = 0; + const result = await applyCodexCliUpdatePlan(plan.planId!, { + ...first, + inspect: async deps => { + calls += 1; + if (calls === 1) return report(); + return await (override.inspect ?? (async () => report()))(deps); + }, + }); + expect(result.status).toBe("ambiguous"); + expect(result.installedVersionAfter).toBe(after); + } + }); +}); + +describe("Codex CLI update shim repair", () => { + async function applyWithShim( + preShim: CodexCliInstallReport["shim"], + postShim: CodexCliInstallReport["shim"], + restore: () => Promise<{ status: string }>, + ) { + const deps = planDeps({ inspect: async () => report({ shim: preShim }) }); + const plan = await createCodexCliUpdatePlan(deps); + let calls = 0; + return await applyCodexCliUpdatePlan(plan.planId!, { + ...deps, + runInstaller: () => ({ exitCode: 0 }), + restoreShim: restore, + inspect: async () => { + calls += 1; + return calls === 1 + ? report({ shim: preShim }) + : report({ packageVersion: "1.1.0", shim: postShim }); + }, + }); + } + + test("an untracked shim is left alone", async () => { + let restores = 0; + const result = await applyWithShim( + { status: "not-tracked", backingKind: null }, + { status: "not-tracked", backingKind: null }, + async () => { restores += 1; return { status: "restored" }; }, + ); + expect(result.status).toBe("applied"); + expect(restores).toBe(0); + expect(result.shim.attempted).toBe(false); + }); + + test("a shim that was matched before the update is restored after it", async () => { + const result = await applyWithShim( + { status: "matched", backingKind: "backup" }, + { status: "not-tracked", backingKind: null }, + async () => ({ status: "restored" }), + ); + expect(result.status).toBe("applied"); + expect(result.shim).toEqual({ attempted: true, restored: true, status: "restored" }); + }); + + test("a shim that cannot be restored is reported, not silently swallowed", async () => { + for (const status of ["ineligible", "deferred", "disabled"]) { + const result = await applyWithShim( + { status: "matched", backingKind: "backup" }, + { status: "unknown", backingKind: null }, + async () => ({ status }), + ); + // The update itself succeeded; the operator still has a broken launcher to fix. + expect(result.status).toBe("applied_shim_repair_required"); + expect(result.installedVersionAfter).toBe("1.1.0"); + expect(result.shim.status).toBe(status); + } + }); + + test("a throwing restore does not turn a completed update into a crash", async () => { + const result = await applyWithShim( + { status: "matched", backingKind: "backup" }, + { status: "unknown", backingKind: null }, + async () => { throw new Error("lock held"); }, + ); + expect(result.status).toBe("applied_shim_repair_required"); + expect(result.shim).toEqual({ attempted: true, restored: false, status: "failed" }); + }); + + test("a shim that survived the update needs no repair", async () => { + let restores = 0; + const result = await applyWithShim( + { status: "matched", backingKind: "backup" }, + { status: "matched", backingKind: "backup" }, + async () => { restores += 1; return { status: "restored" }; }, + ); + expect(result.status).toBe("applied"); + expect(restores).toBe(0); + }); +}); + +describe("strict Codex app-server process scan", () => { + test("an enumeration failure is unavailable, not an empty list", () => { + const scan = scanCodexAppServerProcesses({ + platform: "linux", + listSnapshots: () => { throw new Error("procfs unreadable"); }, + }); + expect(scan).toEqual({ kind: "unavailable" }); + }); + + test("a readable but empty process table is observed with no matches", () => { + const scan = scanCodexAppServerProcesses({ platform: "linux", listSnapshots: () => [] }); + expect(scan).toEqual({ kind: "observed", processes: [] }); + }); + + test("the same matcher and de-duplication as the kill-path lister", () => { + const snapshot = { pid: 11, commandLine: "codex app-server" }; + const scan = scanCodexAppServerProcesses({ + platform: "linux", + listSnapshots: () => [snapshot, { ...snapshot }, { pid: 12, commandLine: "vim notes.txt" }], + }); + expect(scan.kind).toBe("observed"); + if (scan.kind !== "observed") return; + expect(scan.processes.map(p => p.pid)).toEqual([11]); + }); +}); + +describe("registry target resolution", () => { + function spawnStub(outputs: { status: number | null; stdout: string }[]) { + let call = 0; + return (() => { + const next = outputs[call++] ?? { status: 1, stdout: "" }; + return { status: next.status, stdout: next.stdout, stderr: "" }; + }) as never; + } + + test("an exact version with a sha512 token resolves", () => { + const target = resolveCodexCliUpdateTarget("latest", spawnStub([ + { status: 0, stdout: "1.4.2\n" }, + { status: 0, stdout: "'sha512-abc/DEF+123=' sha1-old\n" }, + ])); + expect(target).toEqual({ kind: "resolved", version: "1.4.2", integrity: "sha512-abc/DEF+123=" }); + }); + + test("a missing integrity token refuses instead of proceeding best-effort", () => { + const target = resolveCodexCliUpdateTarget("latest", spawnStub([ + { status: 0, stdout: "1.4.2\n" }, + { status: 0, stdout: "sha1-onlythis\n" }, + ])); + expect(target.kind).toBe("unresolved"); + }); + + test("a registry timeout refuses", () => { + const target = resolveCodexCliUpdateTarget("latest", spawnStub([{ status: null, stdout: "" }])); + expect(target.kind).toBe("unresolved"); + }); + + test("a non-version answer is not treated as a version", () => { + const target = resolveCodexCliUpdateTarget("latest", spawnStub([ + { status: 0, stdout: "latest\n" }, + ])); + expect(target.kind).toBe("unresolved"); + }); +}); + diff --git a/tests/codex-integration/codex-cli-update-zero-effect.test.ts b/tests/codex-integration/codex-cli-update-zero-effect.test.ts index 11ca23c8add..821ae070696 100644 --- a/tests/codex-integration/codex-cli-update-zero-effect.test.ts +++ b/tests/codex-integration/codex-cli-update-zero-effect.test.ts @@ -195,7 +195,7 @@ describe("Codex CLI updater zero-effect boundary", () => { }); expect(result.error).toBeUndefined(); expect(result.status).toBe(2); - expect(result.stderr).toContain("codex-cli-update action must be check"); + expect(result.stderr).toContain("codex-cli-update action must be check, plan or apply"); expect(readFileSync(statePath)).toEqual(before); expect(existsSync(marker)).toBe(false); }); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index a257ad0551d..df126a64148 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -322,6 +322,7 @@ "codex-cli-installation-targets.test.ts": "codex-integration", "codex-cli-windows-installation-files.test.ts": "codex-integration", "codex-cli-update-launcher-policy.test.ts": "codex-integration", + "codex-cli-update-plan.test.ts": "codex-integration", "codex-cli-update-zero-effect.test.ts": "codex-integration", "codex-composed-acceptance.test.ts": "codex-integration", "codex-config-generation.test.ts": "codex-integration", From d63a4d494f31ef1a158d1503ce85f2bf92642bce Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:23:51 +0900 Subject: [PATCH 2/8] fix(codex): verify the fetched CLI tarball against the plan-bound integrity before apply --- src/codex/cli-update-plan.ts | 75 +++++++++++++++---- .../codex-cli-update-plan.test.ts | 20 ++++- .../codex-cli-update-zero-effect.test.ts | 2 +- 3 files changed, 81 insertions(+), 16 deletions(-) diff --git a/src/codex/cli-update-plan.ts b/src/codex/cli-update-plan.ts index 4bac5452dc8..2d13e82c2a8 100644 --- a/src/codex/cli-update-plan.ts +++ b/src/codex/cli-update-plan.ts @@ -1,5 +1,8 @@ import { spawnSync } from "node:child_process"; import { createHash } from "node:crypto"; +import { mkdtempSync, readdirSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { parseStrictSemver } from "../lib/strict-semver"; import { npmInvocation } from "../update/npm-invocation.mjs"; @@ -94,7 +97,7 @@ export type CodexCliUpdateApplyStatus = | "ambiguous" | "refused"; -export type CodexCliUpdateApplyRefusal = CodexCliUpdateRefusal | "plan_stale" | "plan_unknown"; +export type CodexCliUpdateApplyRefusal = CodexCliUpdateRefusal | "plan_stale" | "plan_unknown" | "integrity_mismatch"; export interface CodexCliUpdateApplyResult { readonly schemaVersion: typeof CODEX_CLI_UPDATE_SCHEMA_VERSION; @@ -112,6 +115,8 @@ export interface CodexCliUpdateApplyResult { export interface CodexCliUpdateInstallerResult { /** `null` for a spawn failure or timeout, mirroring `spawnSync` status. */ readonly exitCode: number | null; + /** True when the fetched artifact's sha512 did not match the plan-bound integrity. */ + readonly integrityMismatch?: boolean; } export interface CodexCliUpdateShimRestoreResult { @@ -129,7 +134,7 @@ export interface CodexCliUpdatePlanDeps { } export interface CodexCliUpdateApplyDeps extends CodexCliUpdatePlanDeps { - readonly runInstaller?: (version: string) => CodexCliUpdateInstallerResult; + readonly runInstaller?: (version: string, expectedIntegrity: string | null) => CodexCliUpdateInstallerResult; readonly restoreShim?: () => Promise; } @@ -238,17 +243,57 @@ export function resolveCodexCliUpdateTarget( return Object.freeze({ kind: "resolved" as const, version, integrity }); } -function defaultRunInstaller(version: string): CodexCliUpdateInstallerResult { - const target = npmTarget(["install", "-g", `${CODEX_CLI_PACKAGE}@${version}`]); - if (!target) return Object.freeze({ exitCode: null }); - const run = spawnSync(target.bin, target.args, { - encoding: "utf8", - timeout: INSTALL_TIMEOUT_MS, - windowsHide: true, - stdio: "inherit", - ...target.options, - }); - return Object.freeze({ exitCode: run.status }); +/** + * Install exactly `version`, verifying the fetched tarball against the plan-bound + * sha512 SRI first. `npm install @` re-resolves registry metadata at + * install time, so a registry or proxy answering differently after the plan check + * would go unnoticed. `npm pack` fetches the same tarball the install would use, the + * digest is compared with the planned integrity, and only the verified local file is + * installed. A mismatch fails closed before anything is written. + */ +function defaultRunInstaller(version: string, expectedIntegrity: string | null): CodexCliUpdateInstallerResult { + if (!expectedIntegrity) { + const target = npmTarget(["install", "-g", `${CODEX_CLI_PACKAGE}@${version}`]); + if (!target) return Object.freeze({ exitCode: null }); + const run = spawnSync(target.bin, target.args, { + encoding: "utf8", + timeout: INSTALL_TIMEOUT_MS, + windowsHide: true, + stdio: "inherit", + ...target.options, + }); + return Object.freeze({ exitCode: run.status }); + } + const stage = mkdtempSync(join(tmpdir(), "ocx-codex-cli-update-")); + try { + const pack = npmTarget(["pack", `${CODEX_CLI_PACKAGE}@${version}`, "--pack-destination", stage]); + if (!pack) return Object.freeze({ exitCode: null }); + const packRun = spawnSync(pack.bin, pack.args, { + encoding: "utf8", + timeout: INSTALL_TIMEOUT_MS, + windowsHide: true, + stdio: "inherit", + ...pack.options, + }); + if (packRun.status !== 0) return Object.freeze({ exitCode: packRun.status }); + const tarballs = readdirSync(stage).filter(name => name.endsWith(".tgz")); + if (tarballs.length !== 1) return Object.freeze({ exitCode: null }); + const tarball = join(stage, tarballs[0]!); + const actual = `sha512-${createHash("sha512").update(readFileSync(tarball)).digest("base64")}`; + if (actual !== expectedIntegrity) return Object.freeze({ exitCode: null, integrityMismatch: true }); + const install = npmTarget(["install", "-g", tarball]); + if (!install) return Object.freeze({ exitCode: null }); + const run = spawnSync(install.bin, install.args, { + encoding: "utf8", + timeout: INSTALL_TIMEOUT_MS, + windowsHide: true, + stdio: "inherit", + ...install.options, + }); + return Object.freeze({ exitCode: run.status }); + } finally { + rmSync(stage, { recursive: true, force: true }); + } } async function defaultRestoreShim(): Promise { @@ -413,7 +458,9 @@ export async function applyCodexCliUpdatePlan( const targetVersion = plan.targetVersion; const before = plan.installedVersion; - const installer = (deps.runInstaller ?? defaultRunInstaller)(targetVersion); + const installer = (deps.runInstaller ?? defaultRunInstaller)(targetVersion, plan.targetIntegrity); + // The fetched artifact failed the plan-bound digest; nothing was installed. + if (installer.integrityMismatch) return refusedApply("integrity_mismatch", plan); const inspect = deps.inspect ?? inspectCodexCliInstall; let readback: CodexCliInstallReport | null = null; diff --git a/tests/codex-integration/codex-cli-update-plan.test.ts b/tests/codex-integration/codex-cli-update-plan.test.ts index 1fd7f269f80..129c2cc9e16 100644 --- a/tests/codex-integration/codex-cli-update-plan.test.ts +++ b/tests/codex-integration/codex-cli-update-plan.test.ts @@ -246,6 +246,25 @@ describe("Codex CLI update apply", () => { expect(installs).toEqual([]); }); + test("an artifact digest mismatch fails closed before install", async () => { + const installs: string[] = []; + const plan = await applicablePlan(); + let seenIntegrity: string | null | undefined; + const result = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + runInstaller: (version, expectedIntegrity) => { + installs.push(version); + seenIntegrity = expectedIntegrity; + return { exitCode: null, integrityMismatch: true }; + }, + }, installs)); + // The plan-bound sha512 reaches the installer seam, and a mismatch refuses + // rather than reporting an ambiguous post-install state. + expect(seenIntegrity).toBe(plan.targetIntegrity); + expect(result.status).toBe("refused"); + expect(result.refusal).toBe("integrity_mismatch"); + expect(result.installerExitCode).toBeNull(); + }); + test("the readback classifies the result, and installs exactly the pinned version", async () => { const installs: string[] = []; const plan = await applicablePlan(); @@ -458,4 +477,3 @@ describe("registry target resolution", () => { expect(target.kind).toBe("unresolved"); }); }); - diff --git a/tests/codex-integration/codex-cli-update-zero-effect.test.ts b/tests/codex-integration/codex-cli-update-zero-effect.test.ts index 821ae070696..49ba8c44581 100644 --- a/tests/codex-integration/codex-cli-update-zero-effect.test.ts +++ b/tests/codex-integration/codex-cli-update-zero-effect.test.ts @@ -195,7 +195,7 @@ describe("Codex CLI updater zero-effect boundary", () => { }); expect(result.error).toBeUndefined(); expect(result.status).toBe(2); - expect(result.stderr).toContain("codex-cli-update action must be check, plan or apply"); + expect(result.stderr).toContain("codex-cli-update action must be check, attest, plan or apply"); expect(readFileSync(statePath)).toEqual(before); expect(existsSync(marker)).toBe(false); }); From 5eeb967150cdb5e9a76727ce811957a35153e51e Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Sat, 19 Sep 2026 06:52:50 +0900 Subject: [PATCH 3/8] fix(codex): label the plan command as indicative of the verified install CodeRabbit merge-risk: the displayed command (npm install -g pkg@version) could lead an operator to run a different, unverified install than the update workflow, which packs the tarball, verifies the bound sha512, and installs the verified file. Label the plan output as indicative and correct the field contract. --- src/codex/cli-update-plan.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/codex/cli-update-plan.ts b/src/codex/cli-update-plan.ts index 2d13e82c2a8..ad6450c887b 100644 --- a/src/codex/cli-update-plan.ts +++ b/src/codex/cli-update-plan.ts @@ -86,7 +86,7 @@ export interface CodexCliUpdatePlan { /** True when the pre-update shim was `matched`, i.e. this install owns a live shim. */ readonly shimEligible: boolean; readonly session: CodexCliUpdateSession; - /** Exactly the argv apply would run, for the operator to read before approving it. */ + /** Indicative install argv for the operator to read before approving; apply actually runs `npm pack`, verifies the bound sha512, and installs the verified tarball. */ readonly command: readonly string[] | null; } From 4f8db29732ae5f433a99ce6faadcd7186a4537a2 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Sat, 19 Sep 2026 16:00:13 +0900 Subject: [PATCH 4/8] fix(codex): refuse non-advancing cli update targets and fail closed on missing integrity - compare resolved target against the installed version by semver precedence; equal stays already_current and lower is refused as target_not_newer instead of being applied as a silent downgrade - defaultRunInstaller returns integrityMismatch when no expected digest is supplied, so an injected caller cannot trigger an unverified install - share the semver precedence comparator through strict-semver and reuse it in version-skew - describe the apply capability as pack, sha512 verify, then install the verified file --- src/cli/version-skew.ts | 23 ++----------- src/codex/cli-update-plan.ts | 32 +++++++++++-------- src/lib/strict-semver.ts | 24 ++++++++++++++ .../codex-cli-update-plan.test.ts | 8 +++++ 4 files changed, 53 insertions(+), 34 deletions(-) diff --git a/src/cli/version-skew.ts b/src/cli/version-skew.ts index 7acd5177b3c..c568959a502 100644 --- a/src/cli/version-skew.ts +++ b/src/cli/version-skew.ts @@ -9,7 +9,7 @@ * comparison instead of reimplementing it -- two diagnostics disagreeing about whether an * install is stale would be worse than neither reporting it. */ -import { parseStrictSemver, type StrictSemver } from "../lib/strict-semver"; +import { compareStrictSemver, parseStrictSemver } from "../lib/strict-semver"; /** Placeholder versions that mean "unknown", not "different". */ const PLACEHOLDERS = new Set(["unknown", "0.0.0"]); @@ -28,25 +28,6 @@ export function isConfirmedVersionMatch(skew: VersionSkew): boolean { return skew.proxyVersion === skew.cliVersion && !PLACEHOLDERS.has(skew.cliVersion); } -/** SemVer precedence ignores build metadata; raw equality is handled separately. */ -function compareVersions(cli: StrictSemver, proxy: StrictSemver): number { - for (let i = 0; i < cli.core.length; i++) { - if (cli.core[i]! !== proxy.core[i]!) return cli.core[i]! > proxy.core[i]! ? 1 : -1; - } - if (cli.prerelease.length === 0) return proxy.prerelease.length === 0 ? 0 : 1; - if (proxy.prerelease.length === 0) return -1; - for (let i = 0; i < Math.max(cli.prerelease.length, proxy.prerelease.length); i++) { - const left = cli.prerelease[i]; - const right = proxy.prerelease[i]; - if (left === right) continue; - if (left === undefined) return -1; - if (right === undefined) return 1; - if (typeof left !== typeof right) return typeof left === "bigint" ? -1 : 1; - return left > right ? 1 : -1; - } - return 0; -} - /** * Compare the running CLI against the live proxy. * @@ -63,7 +44,7 @@ export function computeVersionSkew(cliVersion: string, proxyVersion: string | un } const cliSemver = parseStrictSemver(cliVersion); const proxySemver = parseStrictSemver(proxy); - const order = cliSemver && proxySemver ? compareVersions(cliSemver, proxySemver) : 0; + const order = cliSemver && proxySemver ? compareStrictSemver(cliSemver, proxySemver) : 0; const advice = order > 0 ? "the running proxy is older than this CLI. Restart the proxy using the intended current installation. " // `restart`, not `repair`: a version skew leaves the service DEFINITION unchanged, and diff --git a/src/codex/cli-update-plan.ts b/src/codex/cli-update-plan.ts index ad6450c887b..531f8495d06 100644 --- a/src/codex/cli-update-plan.ts +++ b/src/codex/cli-update-plan.ts @@ -4,7 +4,7 @@ import { mkdtempSync, readdirSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { parseStrictSemver } from "../lib/strict-semver"; +import { compareStrictSemver, parseStrictSemver } from "../lib/strict-semver"; import { npmInvocation } from "../update/npm-invocation.mjs"; import { inspectCodexCliInstall, @@ -48,6 +48,7 @@ export type CodexCliUpdateRefusal = | "installed_version_unverified" | "target_unresolved" | "already_current" + | "target_not_newer" | "blocked_active_session" | "blocked_process_state_unknown"; @@ -253,16 +254,9 @@ export function resolveCodexCliUpdateTarget( */ function defaultRunInstaller(version: string, expectedIntegrity: string | null): CodexCliUpdateInstallerResult { if (!expectedIntegrity) { - const target = npmTarget(["install", "-g", `${CODEX_CLI_PACKAGE}@${version}`]); - if (!target) return Object.freeze({ exitCode: null }); - const run = spawnSync(target.bin, target.args, { - encoding: "utf8", - timeout: INSTALL_TIMEOUT_MS, - windowsHide: true, - stdio: "inherit", - ...target.options, - }); - return Object.freeze({ exitCode: run.status }); + // Fail closed: a caller that cannot state the expected digest gets no + // unverified install, even though production plans always carry one. + return Object.freeze({ exitCode: null, integrityMismatch: true }); } const stage = mkdtempSync(join(tmpdir(), "ocx-codex-cli-update-")); try { @@ -361,7 +355,8 @@ export async function createCodexCliUpdatePlan(deps: CodexCliUpdatePlanDeps = {} // package-manifest evidence states what is installed on disk, and only that can be // compared with a registry version or read back after an install. const installedVersion = report.versionEvidence.kind === "package-manifest" ? report.packageVersion : null; - if (!installedVersion || !parseStrictSemver(installedVersion)) { + const installedSemver = installedVersion ? parseStrictSemver(installedVersion) : null; + if (!installedVersion || !installedSemver) { return refusedPlan("installed_version_unverified", report, channel, null, NOT_EVALUATED); } @@ -370,9 +365,20 @@ export async function createCodexCliUpdatePlan(deps: CodexCliUpdatePlanDeps = {} if (target.kind !== "resolved") { return refusedPlan("target_unresolved", report, channel, target, NOT_EVALUATED); } - if (target.version === installedVersion) { + // Raw equality is not the gate: a resolved target must advance the installed + // version by semver precedence. Equal versions are already current and lower + // ones are refused rather than applied as a silent downgrade. + const targetSemver = parseStrictSemver(target.version); + if (!targetSemver) { + return refusedPlan("target_unresolved", report, channel, target, NOT_EVALUATED); + } + const versionOrder = compareStrictSemver(targetSemver, installedSemver); + if (versionOrder === 0) { return refusedPlan("already_current", report, channel, target, NOT_EVALUATED); } + if (versionOrder < 0) { + return refusedPlan("target_not_newer", report, channel, target, NOT_EVALUATED); + } const scan = (deps.scanProcesses ?? scanCodexAppServerProcesses)(deps.processIo ?? {}); if (scan.kind !== "observed") { diff --git a/src/lib/strict-semver.ts b/src/lib/strict-semver.ts index 75e45b5fcbf..08b56da4072 100644 --- a/src/lib/strict-semver.ts +++ b/src/lib/strict-semver.ts @@ -45,3 +45,27 @@ export function parseStrictSemver(value: unknown, maxLength = 128): StrictSemver prerelease: Object.freeze(prereleaseParts.map(part => /^\d+$/.test(part) ? BigInt(part) : part)), }); } + +/** + * SemVer precedence per semver.org: the numeric core first, then prerelease + * identifiers (numeric identifiers sort below alphanumeric ones, and a shorter + * identifier set below a longer one sharing its prefix). Build metadata is not + * part of precedence, which is why StrictSemver does not retain it. + */ +export function compareStrictSemver(a: StrictSemver, b: StrictSemver): number { + for (let i = 0; i < a.core.length; i++) { + if (a.core[i]! !== b.core[i]!) return a.core[i]! > b.core[i]! ? 1 : -1; + } + if (a.prerelease.length === 0) return b.prerelease.length === 0 ? 0 : 1; + if (b.prerelease.length === 0) return -1; + for (let i = 0; i < Math.max(a.prerelease.length, b.prerelease.length); i++) { + const left = a.prerelease[i]; + const right = b.prerelease[i]; + if (left === right) continue; + if (left === undefined) return -1; + if (right === undefined) return 1; + if (typeof left !== typeof right) return typeof left === "bigint" ? -1 : 1; + return left > right ? 1 : -1; + } + return 0; +} diff --git a/tests/codex-integration/codex-cli-update-plan.test.ts b/tests/codex-integration/codex-cli-update-plan.test.ts index 129c2cc9e16..b59aa27c969 100644 --- a/tests/codex-integration/codex-cli-update-plan.test.ts +++ b/tests/codex-integration/codex-cli-update-plan.test.ts @@ -133,6 +133,14 @@ describe("Codex CLI update dry-run plan", () => { expect(plan.refusal).toBe("already_current"); }); + test("a resolved target lower than the install is refused, not applied as a downgrade", async () => { + const plan = await createCodexCliUpdatePlan(planDeps({ + resolveTarget: () => ({ kind: "resolved", version: "0.9.0", integrity: "sha512-AAAA" }), + })); + expect(plan.refusal).toBe("target_not_newer"); + expect(plan.planId).toBeNull(); + }); + test("an unreadable process table defers instead of reading as no live session", async () => { const plan = await createCodexCliUpdatePlan(planDeps({ scanProcesses: () => ({ kind: "unavailable" }) })); expect(plan.refusal).toBe("blocked_process_state_unknown"); From 410be373eaaa0ad9d8364be53d2f1783ac1ab706 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Sat, 19 Sep 2026 21:20:04 +0900 Subject: [PATCH 5/8] fix(codex): drop the unreachable shim-repair lane from cli update apply A matched shim is always reported standalone-unverified and unmanaged, so a plan built from real evidence can never reach the shim-repair path. Remove the dead lane and its capability claims (shimEligible, applied_shim_repair_required, restoreShim) until ownership can be classified through a shim backing. --- src/codex/cli-update-plan.ts | 62 +++----------- tests/cli/cli-codex-cli-update.test.ts | 6 +- .../codex-cli-update-plan.test.ts | 85 ------------------- 3 files changed, 13 insertions(+), 140 deletions(-) diff --git a/src/codex/cli-update-plan.ts b/src/codex/cli-update-plan.ts index 531f8495d06..eb6d257385a 100644 --- a/src/codex/cli-update-plan.ts +++ b/src/codex/cli-update-plan.ts @@ -84,8 +84,6 @@ export interface CodexCliUpdatePlan { readonly location: string | null; readonly targetVersion: string | null; readonly targetIntegrity: string | null; - /** True when the pre-update shim was `matched`, i.e. this install owns a live shim. */ - readonly shimEligible: boolean; readonly session: CodexCliUpdateSession; /** Indicative install argv for the operator to read before approving; apply actually runs `npm pack`, verifies the bound sha512, and installs the verified tarball. */ readonly command: readonly string[] | null; @@ -94,7 +92,6 @@ export interface CodexCliUpdatePlan { export type CodexCliUpdateApplyStatus = | "applied" | "not_applied" - | "applied_shim_repair_required" | "ambiguous" | "refused"; @@ -110,7 +107,6 @@ export interface CodexCliUpdateApplyResult { readonly installedVersionAfter: string | null; /** Evidence only. The readback classifies the outcome; the exit code never does. */ readonly installerExitCode: number | null; - readonly shim: Readonly<{ attempted: boolean; restored: boolean; status: string | null }>; } export interface CodexCliUpdateInstallerResult { @@ -120,10 +116,6 @@ export interface CodexCliUpdateInstallerResult { readonly integrityMismatch?: boolean; } -export interface CodexCliUpdateShimRestoreResult { - readonly status: string; -} - export interface CodexCliUpdatePlanDeps { readonly inspect?: (deps: CodexCliInstallProvenanceDeps) => Promise; readonly inspectionDeps?: CodexCliInstallProvenanceDeps; @@ -136,7 +128,6 @@ export interface CodexCliUpdatePlanDeps { export interface CodexCliUpdateApplyDeps extends CodexCliUpdatePlanDeps { readonly runInstaller?: (version: string, expectedIntegrity: string | null) => CodexCliUpdateInstallerResult; - readonly restoreShim?: () => Promise; } const PLAN_ID_LENGTH = 32; @@ -156,7 +147,7 @@ export function codexCliUpdateCommand(version: string): readonly string[] { * Session blockers are deliberately absent: a session that starts or ends between dry-run * and apply must not invalidate an otherwise identical plan, and it is re-read at apply * time where it can only ever refuse. Everything else — ownership, installed version, - * location, resolved target, integrity, shim eligibility — is bound, so any drift produces + * location, resolved target, integrity — is bound, so any drift produces * a different id and `apply` refuses instead of installing something the operator did not * read. */ @@ -168,7 +159,6 @@ export function codexCliUpdatePlanId(bound: { readonly channel: CodexCliUpdateChannel; readonly targetVersion: string; readonly targetIntegrity: string; - readonly shimEligible: boolean; }): string { // Ordered pairs, not object key order: the digest must not depend on how a caller // happened to build the record. @@ -182,7 +172,6 @@ export function codexCliUpdatePlanId(bound: { ["channel", bound.channel], ["targetVersion", bound.targetVersion], ["targetIntegrity", bound.targetIntegrity], - ["shimEligible", bound.shimEligible ? "1" : "0"], ]; const hash = createHash("sha256"); for (const [key, value] of fields) hash.update(`${key}=${value}\n`); @@ -290,14 +279,6 @@ function defaultRunInstaller(version: string, expectedIntegrity: string | null): } } -async function defaultRestoreShim(): Promise { - // Imported lazily: the plan engine stays a pure module that tests can drive without - // pulling in the shim state store and the config directory graph. - const { autoRestoreCodexShim } = await import("./shim"); - const result = autoRestoreCodexShim({ enabled: () => true }); - return Object.freeze({ status: result.status }); -} - function refusedPlan( refusal: CodexCliUpdateRefusal, report: CodexCliInstallReport, @@ -319,7 +300,6 @@ function refusedPlan( location: report.location, targetVersion: target?.kind === "resolved" ? target.version : null, targetIntegrity: target?.kind === "resolved" ? target.integrity : null, - shimEligible: report.shim.status === "matched", session, command: null, }); @@ -392,7 +372,6 @@ export async function createCodexCliUpdatePlan(deps: CodexCliUpdatePlanDeps = {} return refusedPlan("blocked_active_session", report, channel, target, Object.freeze({ state: "active" as const, matches })); } - const shimEligible = report.shim.status === "matched"; return Object.freeze({ schemaVersion: CODEX_CLI_UPDATE_SCHEMA_VERSION, package: CODEX_CLI_PACKAGE, @@ -407,7 +386,6 @@ export async function createCodexCliUpdatePlan(deps: CodexCliUpdatePlanDeps = {} channel, targetVersion: target.version, targetIntegrity: target.integrity, - shimEligible, }), provenance: report.provenance, managed: report.managed, @@ -416,7 +394,6 @@ export async function createCodexCliUpdatePlan(deps: CodexCliUpdatePlanDeps = {} location: report.location, targetVersion: target.version, targetIntegrity: target.integrity, - shimEligible, session: Object.freeze({ state: "none" as const, matches: 0 }), command: codexCliUpdateCommand(target.version), }); @@ -435,7 +412,6 @@ function refusedApply( installedVersionBefore: plan?.installedVersion ?? null, installedVersionAfter: null, installerExitCode: null, - shim: Object.freeze({ attempted: false, restored: false, status: null }), }); } @@ -457,8 +433,8 @@ export async function applyCodexCliUpdatePlan( if (!plan.applicable || !plan.planId || !plan.targetVersion || !plan.installedVersion) { return refusedApply(plan.refusal ?? "plan_unknown", plan); } - // Any drift in ownership, installed version, location, target or shim eligibility - // changes the id. Refuse rather than regenerate: the operator would otherwise approve + // Any drift in ownership, installed version, location or target changes the id. + // Refuse rather than regenerate: the operator would otherwise approve // one plan and install another. if (plan.planId !== planId) return refusedApply("plan_stale", plan); @@ -482,10 +458,7 @@ export async function applyCodexCliUpdatePlan( ? readback.packageVersion : null; - const result = ( - status: CodexCliUpdateApplyStatus, - shim: { attempted: boolean; restored: boolean; status: string | null }, - ): CodexCliUpdateApplyResult => Object.freeze({ + const result = (status: CodexCliUpdateApplyStatus): CodexCliUpdateApplyResult => Object.freeze({ schemaVersion: CODEX_CLI_UPDATE_SCHEMA_VERSION, status, refusal: null, @@ -494,32 +467,17 @@ export async function applyCodexCliUpdatePlan( installedVersionBefore: before, installedVersionAfter: after, installerExitCode: installer.exitCode, - shim: Object.freeze({ ...shim }), }); - const noShim = { attempted: false, restored: false, status: null }; if (after !== targetVersion) { // A failed readback, an unchanged version and a third version are all reported // as-is. None of them is retried, and none of them is rolled back. - if (after !== null && after === before) return result("not_applied", noShim); - return result("ambiguous", noShim); + if (after !== null && after === before) return result("not_applied"); + return result("ambiguous"); } - // The shim is touched only when this installation owned a matched shim before the - // update and npm replaced it. A shim that was never tracked stays untracked. - if (!plan.shimEligible || readback?.shim.status === "matched") { - return result("applied", noShim); - } - let restore: CodexCliUpdateShimRestoreResult; - try { - restore = await (deps.restoreShim ?? defaultRestoreShim)(); - } catch { - restore = Object.freeze({ status: "failed" }); - } - const restored = restore.status === "restored" || restore.status === "healthy"; - return result(restored ? "applied" : "applied_shim_repair_required", { - attempted: true, - restored, - status: restore.status, - }); + // A matched shim never reaches this point: the inspector reports a shim-owned + // candidate as standalone-unverified and the plan refuses `not_managed`, so an + // applied update never owned a shim npm could have replaced. + return result("applied"); } diff --git a/tests/cli/cli-codex-cli-update.test.ts b/tests/cli/cli-codex-cli-update.test.ts index 6ecccb386c3..eab1b95de6a 100644 --- a/tests/cli/cli-codex-cli-update.test.ts +++ b/tests/cli/cli-codex-cli-update.test.ts @@ -415,7 +415,7 @@ describe("Codex CLI update plan and apply grammar", () => { applicable: false, refusal: "not_managed", planId: null, provenance: "app-bundle", managed: false, installedVersion: null, versionEvidence: "unavailable", location: null, - targetVersion: null, targetIntegrity: null, shimEligible: false, + targetVersion: null, targetIntegrity: null, session: { state: "not-evaluated", matches: null }, command: null, }), }); @@ -432,7 +432,7 @@ describe("Codex CLI update plan and apply grammar", () => { return { schemaVersion: 1, status: "refused", refusal: "plan_stale", planId: null, targetVersion: null, installedVersionBefore: null, installedVersionAfter: null, - installerExitCode: null, shim: { attempted: false, restored: false, status: null }, + installerExitCode: null, }; }, }); @@ -445,7 +445,7 @@ describe("Codex CLI update plan and apply grammar", () => { applyPlan: async () => ({ schemaVersion: 1, status: "applied", refusal: null, planId: PLAN_ID, targetVersion: "1.1.0", installedVersionBefore: "1.0.0", installedVersionAfter: "1.1.0", - installerExitCode: 0, shim: { attempted: false, restored: false, status: null }, + installerExitCode: 0, }), }); expect(code).toBe(0); diff --git a/tests/codex-integration/codex-cli-update-plan.test.ts b/tests/codex-integration/codex-cli-update-plan.test.ts index b59aa27c969..2aa673a10a0 100644 --- a/tests/codex-integration/codex-cli-update-plan.test.ts +++ b/tests/codex-integration/codex-cli-update-plan.test.ts @@ -167,7 +167,6 @@ describe("Codex CLI update plan identity", () => { { resolveTarget: () => ({ kind: "resolved", version: "1.1.0", integrity: "sha512-BBBB" }) }, { inspect: async () => report({ packageVersion: "1.0.1", candidateVersion: "1.0.1" }) }, { inspect: async () => report({ location: "/other/@openai/codex" }) }, - { inspect: async () => report({ shim: { status: "matched", backingKind: "backup" } }) }, ]; for (const variant of variants) { const plan = await applicablePlan(variant); @@ -195,10 +194,8 @@ describe("Codex CLI update plan identity", () => { channel: "latest" as const, targetVersion: "1.1.0", targetIntegrity: "sha512-AAAA", - shimEligible: false, }; expect(codexCliUpdatePlanId(bound)).toBe(codexCliUpdatePlanId(bound)); - expect(codexCliUpdatePlanId({ ...bound, shimEligible: true })).not.toBe(codexCliUpdatePlanId(bound)); }); }); @@ -209,7 +206,6 @@ function applyDeps( return { ...planDeps(), runInstaller: version => { installs.push(version); return { exitCode: 0 }; }, - restoreShim: async () => ({ status: "restored" }), ...overrides, }; } @@ -287,7 +283,6 @@ describe("Codex CLI update apply", () => { expect(result.status).toBe("applied"); expect(result.installedVersionBefore).toBe("1.0.0"); expect(result.installedVersionAfter).toBe("1.1.0"); - expect(result.shim).toEqual({ attempted: false, restored: false, status: null }); }); test("a nonzero installer exit never overrides a readback that shows the target", async () => { @@ -342,86 +337,6 @@ describe("Codex CLI update apply", () => { }); }); -describe("Codex CLI update shim repair", () => { - async function applyWithShim( - preShim: CodexCliInstallReport["shim"], - postShim: CodexCliInstallReport["shim"], - restore: () => Promise<{ status: string }>, - ) { - const deps = planDeps({ inspect: async () => report({ shim: preShim }) }); - const plan = await createCodexCliUpdatePlan(deps); - let calls = 0; - return await applyCodexCliUpdatePlan(plan.planId!, { - ...deps, - runInstaller: () => ({ exitCode: 0 }), - restoreShim: restore, - inspect: async () => { - calls += 1; - return calls === 1 - ? report({ shim: preShim }) - : report({ packageVersion: "1.1.0", shim: postShim }); - }, - }); - } - - test("an untracked shim is left alone", async () => { - let restores = 0; - const result = await applyWithShim( - { status: "not-tracked", backingKind: null }, - { status: "not-tracked", backingKind: null }, - async () => { restores += 1; return { status: "restored" }; }, - ); - expect(result.status).toBe("applied"); - expect(restores).toBe(0); - expect(result.shim.attempted).toBe(false); - }); - - test("a shim that was matched before the update is restored after it", async () => { - const result = await applyWithShim( - { status: "matched", backingKind: "backup" }, - { status: "not-tracked", backingKind: null }, - async () => ({ status: "restored" }), - ); - expect(result.status).toBe("applied"); - expect(result.shim).toEqual({ attempted: true, restored: true, status: "restored" }); - }); - - test("a shim that cannot be restored is reported, not silently swallowed", async () => { - for (const status of ["ineligible", "deferred", "disabled"]) { - const result = await applyWithShim( - { status: "matched", backingKind: "backup" }, - { status: "unknown", backingKind: null }, - async () => ({ status }), - ); - // The update itself succeeded; the operator still has a broken launcher to fix. - expect(result.status).toBe("applied_shim_repair_required"); - expect(result.installedVersionAfter).toBe("1.1.0"); - expect(result.shim.status).toBe(status); - } - }); - - test("a throwing restore does not turn a completed update into a crash", async () => { - const result = await applyWithShim( - { status: "matched", backingKind: "backup" }, - { status: "unknown", backingKind: null }, - async () => { throw new Error("lock held"); }, - ); - expect(result.status).toBe("applied_shim_repair_required"); - expect(result.shim).toEqual({ attempted: true, restored: false, status: "failed" }); - }); - - test("a shim that survived the update needs no repair", async () => { - let restores = 0; - const result = await applyWithShim( - { status: "matched", backingKind: "backup" }, - { status: "matched", backingKind: "backup" }, - async () => { restores += 1; return { status: "restored" }; }, - ); - expect(result.status).toBe("applied"); - expect(restores).toBe(0); - }); -}); - describe("strict Codex app-server process scan", () => { test("an enumeration failure is unavailable, not an empty list", () => { const scan = scanCodexAppServerProcesses({ From 3e0d13c866ff85754ebfb9bcb3a23800f24d17b2 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Sat, 19 Sep 2026 21:45:58 +0900 Subject: [PATCH 6/8] fix(codex): bind apply readback to the planned install location --- src/codex/cli-update-plan.ts | 1 + tests/codex-integration/codex-cli-update-plan.test.ts | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/codex/cli-update-plan.ts b/src/codex/cli-update-plan.ts index eb6d257385a..ec7e47872fd 100644 --- a/src/codex/cli-update-plan.ts +++ b/src/codex/cli-update-plan.ts @@ -454,6 +454,7 @@ export async function applyCodexCliUpdatePlan( const after = readback && readback.provenance === "npm-global" + && readback.location === plan.location && readback.versionEvidence.kind === "package-manifest" ? readback.packageVersion : null; diff --git a/tests/codex-integration/codex-cli-update-plan.test.ts b/tests/codex-integration/codex-cli-update-plan.test.ts index 2aa673a10a0..7420f9b3910 100644 --- a/tests/codex-integration/codex-cli-update-plan.test.ts +++ b/tests/codex-integration/codex-cli-update-plan.test.ts @@ -317,6 +317,8 @@ describe("Codex CLI update apply", () => { [{ inspect: async () => report({ packageVersion: "9.9.9" }) }, "9.9.9"], [{ inspect: async () => report({ provenance: "version-manager" }) }, null], [{ inspect: async () => report({ versionEvidence: { kind: "advisory-runtime" } }) }, null], + // A different npm-global candidate at the target version is not the planned install. + [{ inspect: async () => report({ location: "/other/@openai/codex", packageVersion: "1.1.0" }) }, null], ]; for (const [override, after] of cases) { // The first inspection builds the plan, so these must still produce a plan id; From dd5fc5bd0e17bd6788f4398322ff139e3f62b8e3 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Mon, 21 Sep 2026 02:51:58 +0900 Subject: [PATCH 7/8] fix(codex): pin the cli update to the official registry and a cross-process lease npm view/pack/install now run with --registry=https://registry.npmjs.org, an empty controlled userconfig/globalconfig, a sentinel-anchored cwd, and npm_config_* env stripped, so a hostile project or user npm configuration cannot redirect the metadata, the integrity token, or the tarball. The resolved dist.tarball origin is validated against the pinned registry before the target can resolve. apply acquires one cross-process update lease before the final process scan and holds it through install and readback, so a second apply is refused and Codex startup paths (remote workspace app-server spawn, desktop-app relaunch) observe the lease and wait or refuse instead of loading a half-replaced install. --- .../ocx/references/01_management_surface.md | 42 ++- src/cli/capabilities.ts | 4 +- src/codex/cli-update-lease.ts | 231 +++++++++++++ src/codex/cli-update-plan.ts | 326 +++++++++++++----- src/codex/desktop-app-restart.ts | 15 +- src/remote-control/workspace-codex-runtime.ts | 16 + tests/clients/desktop-app-restart.test.ts | 20 +- .../remote-workspace-codex-runtime.test.ts | 65 +++- .../codex-cli-update-plan.test.ts | 261 ++++++++++++++ 9 files changed, 886 insertions(+), 94 deletions(-) create mode 100644 src/codex/cli-update-lease.ts diff --git a/skills/ocx/references/01_management_surface.md b/skills/ocx/references/01_management_surface.md index 5682c76ac71..905d8da8056 100644 --- a/skills/ocx/references/01_management_surface.md +++ b/skills/ocx/references/01_management_surface.md @@ -421,6 +421,25 @@ JSON mode: `envelope`. - selectionAttested, managed and applyAllowed remain false. The digest is an observation, not a durable update permit. - Does not run the named Codex/npm/Node files, query a registry, install software, control processes or persist state. +### `ocx system codex-cli-update plan` + +Dry-run a Codex CLI update and print the plan id that authorizes applying it. + +Drives no management route. + +| Flag | Value | Meaning | +|---|---|---| +| `--channel` | string | Registry channel to resolve. Only the stable latest channel is offered. | +| `--json` | boolean | Emit the plan as JSON. | + +JSON mode: `envelope`. + +- Adds the three inputs check leaves out: an exact registry version with its sha512 integrity, a fail-closed process-table read, and a decision. +- The registry evidence is pinned to the official npm registry with project/user npm configuration isolated, so a redirected .npmrc cannot supply the answer. +- Writes nothing and installs nothing. A refusal is a normal dry-run answer and still exits 0. +- The plan id is a digest of the evidence the decision rests on, not a stored job. There is no plan state on disk to expire, collide or clean up. +- An unreadable process table refuses rather than reading as no live session. + ### `ocx claude desktop status` Applied-vs-desired Claude Desktop state, including staleness, drift, and health. @@ -824,6 +843,25 @@ JSON mode: `payload`. - `policy set` never enables implicitly: omitting `--enabled` keeps the stored value. - `policy run` forces a run regardless of schedule, so it needs `--yes`. +### `ocx system codex-cli-update apply` + +Install the exact Codex CLI version bound into a plan id from a dry-run. + +Drives no management route. + +| Flag | Value | Meaning | +|---|---|---| +| `--plan` | string | Required: the plan id printed by a dry-run the operator read. | +| `--json` | boolean | Emit the apply result as JSON. | + +JSON mode: `envelope`. + +- --plan is mandatory because the operator must approve a target they have read. The plan is recomputed from live evidence and refused unless the id still matches. +- Resolves and packs only from the pinned official npm registry with project/user npm configuration isolated, verifies the tarball sha512 against the plan-bound integrity, and installs only that verified file; never stops, restarts or signals Codex, the app-server, the desktop app or the tray. +- Holds one cross-process update lease from the final session scan through the install and readback; a concurrent apply is refused, and Codex startup paths that observe the lease wait or refuse rather than load a half-replaced install. +- The outcome is classified from a fresh inspection rather than the installer exit code, and is never retried or rolled back automatically. +- Repairs the shim only when this installation owned a matched shim before the update. + ### `ocx system codex-restart` Restart the Codex desktop app and app-servers. @@ -930,6 +968,6 @@ JSON mode: `payload`. ## Counts -- declared capabilities: 50 -- of those, state-changing: 25 +- declared capabilities: 52 +- of those, state-changing: 26 - head-resolved invocations: 2 diff --git a/src/cli/capabilities.ts b/src/cli/capabilities.ts index af2425f7eae..eadf1305c9d 100644 --- a/src/cli/capabilities.ts +++ b/src/cli/capabilities.ts @@ -791,6 +791,7 @@ export const CAPABILITIES: readonly Capability[] = [ json: "envelope", details: [ "Adds the three inputs check leaves out: an exact registry version with its sha512 integrity, a fail-closed process-table read, and a decision.", + "The registry evidence is pinned to the official npm registry with project/user npm configuration isolated, so a redirected .npmrc cannot supply the answer.", "Writes nothing and installs nothing. A refusal is a normal dry-run answer and still exits 0.", "The plan id is a digest of the evidence the decision rests on, not a stored job. There is no plan state on disk to expire, collide or clean up.", "An unreadable process table refuses rather than reading as no live session.", @@ -808,7 +809,8 @@ export const CAPABILITIES: readonly Capability[] = [ json: "envelope", details: [ "--plan is mandatory because the operator must approve a target they have read. The plan is recomputed from live evidence and refused unless the id still matches.", - "Packs the exact resolved @openai/codex version, verifies the tarball sha512 against the plan-bound integrity, and installs only that verified file; never stops, restarts or signals Codex, the app-server, the desktop app or the tray.", + "Resolves and packs only from the pinned official npm registry with project/user npm configuration isolated, verifies the tarball sha512 against the plan-bound integrity, and installs only that verified file; never stops, restarts or signals Codex, the app-server, the desktop app or the tray.", + "Holds one cross-process update lease from the final session scan through the install and readback; a concurrent apply is refused, and Codex startup paths that observe the lease wait or refuse rather than load a half-replaced install.", "The outcome is classified from a fresh inspection rather than the installer exit code, and is never retried or rolled back automatically.", "Repairs the shim only when this installation owned a matched shim before the update.", ], diff --git a/src/codex/cli-update-lease.ts b/src/codex/cli-update-lease.ts new file mode 100644 index 00000000000..5f116ae5a46 --- /dev/null +++ b/src/codex/cli-update-lease.ts @@ -0,0 +1,231 @@ +/** + * The one cross-process lease behind the Codex CLI update manager. + * + * A process scan is a snapshot, not mutual exclusion: an app-server can start after + * the plan reads the table and before "npm install -g" mutates the global prefix, and + * two apply commands can pass the same plan concurrently. This lockfile closes that + * window — apply holds it from before the final scan through the post-install + * readback, and the Codex startup paths this codebase controls (remote workspace + * app-server spawn, desktop-app relaunch) observe it and refuse or wait. + * + * The mechanics deliberately mirror desktop-app/lock.ts: exclusive O_EXCL create on + * the contended path, owner-pid liveness first and an age bound second for staleness, + * and compare-and-delete release so a late holder can never unlink a successor's + * lease. + * + * Deliberately NOT own-pid reentrant: two applies in one process are still two + * contenders for one global install, and the second must see the lease as held. + */ +import { closeSync, mkdirSync, openSync, readFileSync, unlinkSync, writeSync } from "node:fs"; +import { dirname, join } from "node:path"; + +import { getConfigDir } from "../config/paths"; +import { isProcessAlive } from "../lib/process-control"; + +/** + * A lease older than this is stale regardless of what its owner pid says. The bound + * exists for a live pid that no longer names an updater (a recycled pid, or a wedged + * holder); it is comfortably longer than the pack + install timeouts a real apply can + * legitimately hold the lease for. + */ +export const CODEX_CLI_UPDATE_LEASE_MAX_AGE_MS = 15 * 60_000; + +/** Startup paths poll a held lease for this long before refusing the launch. */ +export const CODEX_CLI_UPDATE_LEASE_WAIT_MS = 5_000; + +export interface CodexCliUpdateLeaseRecord { + readonly version: 1; + readonly ownerPid: number; + readonly createdAtMs: number; + /** The plan id being applied. Diagnostics only; exclusion never reads it. */ + readonly planId: string | null; +} + +export interface CodexCliUpdateLeaseIo { + lockPath?: string; + isAlive?: (pid: number) => boolean; + now?: () => number; + pid?: number; +} + +export interface CodexCliUpdateLeaseWaitIo extends CodexCliUpdateLeaseIo { + sleep?: (ms: number) => Promise; +} + +export type CodexCliUpdateLeaseAcquisition = + | { acquired: true; record: CodexCliUpdateLeaseRecord } + /** A live owner inside the age bound holds the lease. */ + | { acquired: false; reason: "held"; heldBy: number } + /** The lock path could not be created or observed at all. */ + | { acquired: false; reason: "unavailable" }; + +export function defaultCodexCliUpdateLeasePath(): string { + // getConfigDir owns OPENCODEX_HOME resolution; the lease sits beside + // desktop-restart.lock so every Codex lifecycle guard shares one directory. + return join(getConfigDir(), "codex-cli-update.lock"); +} + +function defaultIsAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function readRecord(path: string): CodexCliUpdateLeaseRecord | null { + try { + const parsed: unknown = JSON.parse(readFileSync(path, "utf-8")); + if (typeof parsed !== "object" || parsed === null) return null; + const view = parsed as Record; + const ownerPid = view.ownerPid; + const createdAtMs = view.createdAtMs; + if (view.version !== 1) return null; + if (typeof ownerPid !== "number" || !Number.isSafeInteger(ownerPid) || ownerPid <= 0) return null; + if (typeof createdAtMs !== "number" || !Number.isFinite(createdAtMs)) return null; + const planId = view.planId; + return { + version: 1, + ownerPid, + createdAtMs, + planId: typeof planId === "string" ? planId : null, + }; + } catch { + return null; + } +} + +/** Exclusive create on the contended path — the whole mutual exclusion. */ +function tryCreateExclusive(path: string, record: CodexCliUpdateLeaseRecord): boolean { + mkdirSync(dirname(path), { recursive: true }); + let fd: number; + try { + fd = openSync(path, "wx", 0o600); + } catch { + return false; + } + try { + writeSync(fd, JSON.stringify(record)); + } finally { + closeSync(fd); + } + return true; +} + +function holderIsLive(record: CodexCliUpdateLeaseRecord, io: CodexCliUpdateLeaseIo): boolean { + const isAlive = io.isAlive ?? defaultIsAlive; + const now = io.now ?? Date.now; + return isAlive(record.ownerPid) && now() - record.createdAtMs <= CODEX_CLI_UPDATE_LEASE_MAX_AGE_MS; +} + +/** + * Take the update lease, or report why it could not be taken. + * + * Contended callers do not queue: a second apply reports "held" and refuses, which is + * the honest answer — queueing would run two global installs back to back against a + * plan the second caller read before the first one mutated the installation. + */ +export function acquireCodexCliUpdateLease( + io: CodexCliUpdateLeaseIo & { planId?: string } = {}, +): CodexCliUpdateLeaseAcquisition { + const path = io.lockPath ?? defaultCodexCliUpdateLeasePath(); + const now = io.now ?? Date.now; + const self = io.pid ?? process.pid; + + const existing = readRecord(path); + if (existing) { + if (holderIsLive(existing, io)) { + return { acquired: false, reason: "held", heldBy: existing.ownerPid }; + } + // Stale. Clear it and then compete for the exclusive create like anyone else: + // two processes can observe the same stale lease, and only O_EXCL decides which + // one actually holds it. + try { + unlinkSync(path); + } catch { + /* somebody else cleared it first — the create below still decides */ + } + } + + const record: CodexCliUpdateLeaseRecord = { + version: 1, + ownerPid: self, + createdAtMs: now(), + planId: io.planId ?? null, + }; + if (tryCreateExclusive(path, record)) return { acquired: true, record }; + + const winner = readRecord(path); + if (winner) { + return holderIsLive(winner, io) + ? { acquired: false, reason: "held", heldBy: winner.ownerPid } + : { acquired: false, reason: "unavailable" }; + } + + // The file exists but names nobody: truncated, corrupt, or left by a writer that + // died between create and write. Remove it and make exactly one more attempt so a + // lock nobody holds cannot wedge every future update. + try { + unlinkSync(path); + } catch { + /* somebody else cleared it first */ + } + if (tryCreateExclusive(path, record)) return { acquired: true, record }; + const successor = readRecord(path); + return successor && holderIsLive(successor, io) + ? { acquired: false, reason: "held", heldBy: successor.ownerPid } + : { acquired: false, reason: "unavailable" }; +} + +/** Compare-and-delete. Never removes a lease owned by another process. */ +export function releaseCodexCliUpdateLease(io: CodexCliUpdateLeaseIo = {}): void { + const path = io.lockPath ?? defaultCodexCliUpdateLeasePath(); + const self = io.pid ?? process.pid; + const existing = readRecord(path); + if (!existing || existing.ownerPid !== self) return; + try { + unlinkSync(path); + } catch { + /* already gone */ + } +} + +export interface CodexCliUpdateLeaseObservation { + /** True only while a live owner inside the age bound holds the lease. */ + readonly held: boolean; + readonly ownerPid: number | null; +} + +/** + * Read-only observation for Codex startup paths. A stale or corrupt record reads as + * free — reclaiming it is the acquirer's job, and a dead file must not block Codex + * from ever starting again. + */ +export function observeCodexCliUpdateLease( + io: CodexCliUpdateLeaseIo = {}, +): CodexCliUpdateLeaseObservation { + const existing = readRecord(io.lockPath ?? defaultCodexCliUpdateLeasePath()); + if (!existing || !holderIsLive(existing, io)) return { held: false, ownerPid: null }; + return { held: true, ownerPid: existing.ownerPid }; +} + +const WAIT_POLL_MS = 250; + +/** + * Bounded wait for the lease to clear, for startup paths that would rather wait out + * the tail of an install than refuse outright. Resolves true once the lease is free, + * false when the deadline passed with it still held. + */ +export async function waitForCodexCliUpdateLeaseRelease( + io: CodexCliUpdateLeaseWaitIo & { timeoutMs?: number } = {}, +): Promise { + const now = io.now ?? Date.now; + const sleep = io.sleep ?? (ms => new Promise(done => setTimeout(done, ms))); + const deadline = now() + (io.timeoutMs ?? CODEX_CLI_UPDATE_LEASE_WAIT_MS); + for (;;) { + if (!observeCodexCliUpdateLease(io).held) return true; + if (now() >= deadline) return false; + await sleep(Math.min(WAIT_POLL_MS, Math.max(1, deadline - now()))); + } +} diff --git a/src/codex/cli-update-plan.ts b/src/codex/cli-update-plan.ts index ec7e47872fd..19b145c3e29 100644 --- a/src/codex/cli-update-plan.ts +++ b/src/codex/cli-update-plan.ts @@ -1,6 +1,6 @@ -import { spawnSync } from "node:child_process"; +import { spawnSync, type SpawnSyncReturns } from "node:child_process"; import { createHash } from "node:crypto"; -import { mkdtempSync, readdirSync, readFileSync, rmSync } from "node:fs"; +import { mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -17,6 +17,11 @@ import { type CodexAppServerProcessIo, type CodexAppServerProcessScan, } from "./app-server-processes"; +import { + acquireCodexCliUpdateLease, + releaseCodexCliUpdateLease, + type CodexCliUpdateLeaseIo, +} from "./cli-update-lease"; /** * Phase 2 of the Codex CLI update manager: a deterministic dry-run plan and an explicit @@ -39,6 +44,18 @@ import { export const CODEX_CLI_PACKAGE = "@openai/codex"; export const CODEX_CLI_UPDATE_SCHEMA_VERSION = 1 as const; +/** + * The one registry this workflow is allowed to resolve, pack and install from. + * + * npm view/pack inherit the operator's npm configuration by default, so a project or + * user .npmrc — or an npm_config_* env var — can redirect "@openai/codex" to another + * registry whose version query, integrity query and tarball all agree with each + * other. Every npm call below pins this registry explicitly AND runs with the + * registry-affecting configuration isolated, so the pinned origin is the only source + * the evidence can come from. + */ +export const CODEX_CLI_REGISTRY = "https://registry.npmjs.org"; + /** Only the stable channel is exposed; a moving dist-tag is resolved before it is used. */ export type CodexCliUpdateChannel = "latest"; @@ -95,7 +112,12 @@ export type CodexCliUpdateApplyStatus = | "ambiguous" | "refused"; -export type CodexCliUpdateApplyRefusal = CodexCliUpdateRefusal | "plan_stale" | "plan_unknown" | "integrity_mismatch"; +export type CodexCliUpdateApplyRefusal = CodexCliUpdateRefusal + | "plan_stale" + | "plan_unknown" + | "integrity_mismatch" + | "update_in_progress" + | "update_lease_unavailable"; export interface CodexCliUpdateApplyResult { readonly schemaVersion: typeof CODEX_CLI_UPDATE_SCHEMA_VERSION; @@ -128,6 +150,8 @@ export interface CodexCliUpdatePlanDeps { export interface CodexCliUpdateApplyDeps extends CodexCliUpdatePlanDeps { readonly runInstaller?: (version: string, expectedIntegrity: string | null) => CodexCliUpdateInstallerResult; + /** Lease seam: a temp lockPath plus fake liveness drives the exclusion tests. */ + readonly leaseIo?: CodexCliUpdateLeaseIo; } const PLAN_ID_LENGTH = 32; @@ -178,12 +202,86 @@ export function codexCliUpdatePlanId(bound: { return hash.digest("hex").slice(0, PLAN_ID_LENGTH); } -function npmTarget(args: readonly string[]): { bin: string; args: string[]; options: { windowsVerbatimArguments?: boolean } } | null { +interface NpmTarget { + readonly bin: string; + readonly args: string[]; + readonly options: { + readonly windowsVerbatimArguments?: boolean; + readonly cwd?: string; + readonly env?: NodeJS.ProcessEnv; + }; +} + +function npmTarget(args: readonly string[]): NpmTarget | null { const invocation = npmInvocation(args); if (!invocation) return null; return { bin: invocation.file, args: invocation.args, options: invocation.options }; } +/** + * The npm environment with every registry-affecting config channel removed. + * + * npm maps any npm_config_* env var into its configuration, so NPM_CONFIG_REGISTRY or + * npm_config_@openai:registry would defeat the pinned --registry flag (a scoped + * registry beats the default for that scope). They are all stripped. The settings + * this codebase intentionally supports survive untouched: the standard proxy + * variables npm itself honors (HTTP_PROXY/HTTPS_PROXY/ALL_PROXY/NO_PROXY, any case) + * and NODE_EXTRA_CA_CERTS, which npm's own Node runtime reads for TLS. + */ +function codexCliUpdateNpmEnv(): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { ...process.env }; + for (const key of Object.keys(env)) { + if (key.toLowerCase().startsWith("npm_config_")) delete env[key]; + } + return env; +} + +interface NpmConfigIsolation { + /** Controlled cwd: contains the sentinel package.json, so npm's project-config walk stops here. */ + readonly dir: string; + /** Empty file substituted for both the user and the global npmrc. */ + readonly npmrc: string; + readonly env: NodeJS.ProcessEnv; +} + +/** + * A directory npm cannot read hostile configuration through. + * + * npm resolves project config from the nearest ancestor containing package.json (or + * node_modules/.git), so a bare temp dir is NOT enough — the walk would continue into + * the operator's real ancestors. The sentinel package.json anchors the walk here, + * where no .npmrc exists. --userconfig/--globalconfig replace the two file configs, + * and the env filter removes the env-var channel. What remains is exactly the pinned + * --registry flag plus deliberately supported proxy/CA env. + */ +function createNpmConfigIsolation(dir?: string): NpmConfigIsolation { + const root = dir ?? mkdtempSync(join(tmpdir(), "ocx-codex-cli-meta-")); + writeFileSync(join(root, "package.json"), "{}\n"); + const npmrc = join(root, "ocx-update.npmrc"); + writeFileSync(npmrc, ""); + return { dir: root, npmrc, env: codexCliUpdateNpmEnv() }; +} + +/** Registry-pinned, config-isolated argv for one npm call. */ +function isolatedNpmArgs(args: readonly string[], isolation: NpmConfigIsolation): readonly string[] { + return [ + ...args, + "--registry=" + CODEX_CLI_REGISTRY, + "--userconfig=" + isolation.npmrc, + "--globalconfig=" + isolation.npmrc, + ]; +} + +function isolatedNpmTarget(args: readonly string[], isolation: NpmConfigIsolation): NpmTarget | null { + const target = npmTarget(isolatedNpmArgs(args, isolation)); + if (!target) return null; + return { + bin: target.bin, + args: target.args, + options: { ...target.options, cwd: isolation.dir, env: isolation.env }, + }; +} + /** * Resolve the channel to one exact version plus its sha512 integrity token. * @@ -196,41 +294,67 @@ export function resolveCodexCliUpdateTarget( channel: CodexCliUpdateChannel, spawn: typeof spawnSync = spawnSync, ): CodexCliUpdateTarget { - const versionTarget = npmTarget(["view", `${CODEX_CLI_PACKAGE}@${channel}`, "version"]); - if (!versionTarget) return Object.freeze({ kind: "unresolved" as const, reason: "npm executable was not found on a trusted PATH entry" }); - const versionRun = spawn(versionTarget.bin, versionTarget.args, { - encoding: "utf8", - timeout: REGISTRY_TIMEOUT_MS, - windowsHide: true, - ...versionTarget.options, - }); - // status === null covers a timeout as well as a spawn failure. - if (versionRun.status !== 0) { - return Object.freeze({ kind: "unresolved" as const, reason: `registry version query failed (status ${versionRun.status ?? "timeout"})` }); - } - const version = (versionRun.stdout ?? "").trim(); - if (!parseStrictSemver(version)) { - return Object.freeze({ kind: "unresolved" as const, reason: "registry returned no exact version" }); - } - - const integrityTarget = npmTarget(["view", `${CODEX_CLI_PACKAGE}@${version}`, "dist.integrity"]); - if (!integrityTarget) return Object.freeze({ kind: "unresolved" as const, reason: "npm executable was not found on a trusted PATH entry" }); - const integrityRun = spawn(integrityTarget.bin, integrityTarget.args, { - encoding: "utf8", - timeout: REGISTRY_TIMEOUT_MS, - windowsHide: true, - ...integrityTarget.options, - }); - if (integrityRun.status !== 0) { - return Object.freeze({ kind: "unresolved" as const, reason: `registry integrity query failed (status ${integrityRun.status ?? "timeout"})` }); - } - // `dist.integrity` may arrive quoted or as a space-separated multi-hash list. - const tokens = (integrityRun.stdout ?? "").replace(/["']/g, "").trim().split(/\s+/).filter(Boolean); - const integrity = tokens.find(token => SHA512_INTEGRITY_RE.test(token)); - if (!integrity) { - return Object.freeze({ kind: "unresolved" as const, reason: "registry returned no sha512 integrity token" }); + // The pinned registry and the isolation directory together are the boundary: the + // version, the integrity token and the tarball URL all come from npmjs or the + // target is unresolved — a redirected answer can never be the evidence. + const isolation = createNpmConfigIsolation(); + try { + const runView = (field: string, spec: string): SpawnSyncReturns | null => { + const target = isolatedNpmTarget(["view", spec, field], isolation); + if (!target) return null; + return spawn(target.bin, target.args, { + encoding: "utf8", + timeout: REGISTRY_TIMEOUT_MS, + windowsHide: true, + ...target.options, + }); + }; + + const versionRun = runView("version", `${CODEX_CLI_PACKAGE}@${channel}`); + if (!versionRun) return Object.freeze({ kind: "unresolved" as const, reason: "npm executable was not found on a trusted PATH entry" }); + // status === null covers a timeout as well as a spawn failure. + if (versionRun.status !== 0) { + return Object.freeze({ kind: "unresolved" as const, reason: `registry version query failed (status ${versionRun.status ?? "timeout"})` }); + } + const version = (versionRun.stdout ?? "").trim(); + if (!parseStrictSemver(version)) { + return Object.freeze({ kind: "unresolved" as const, reason: "registry returned no exact version" }); + } + + const integrityRun = runView("dist.integrity", `${CODEX_CLI_PACKAGE}@${version}`); + if (!integrityRun) return Object.freeze({ kind: "unresolved" as const, reason: "npm executable was not found on a trusted PATH entry" }); + if (integrityRun.status !== 0) { + return Object.freeze({ kind: "unresolved" as const, reason: `registry integrity query failed (status ${integrityRun.status ?? "timeout"})` }); + } + // `dist.integrity` may arrive quoted or as a space-separated multi-hash list. + const tokens = (integrityRun.stdout ?? "").replace(/["']/g, "").trim().split(/\s+/).filter(Boolean); + const integrity = tokens.find(token => SHA512_INTEGRITY_RE.test(token)); + if (!integrity) { + return Object.freeze({ kind: "unresolved" as const, reason: "registry returned no sha512 integrity token" }); + } + + // The digest binds the tarball's CONTENT; this binds its ORIGIN. A registry + // answer whose tarball lives outside the pinned registry is not the artifact + // the plan approved, even if its sha512 matched. + const tarballRun = runView("dist.tarball", `${CODEX_CLI_PACKAGE}@${version}`); + if (!tarballRun) return Object.freeze({ kind: "unresolved" as const, reason: "npm executable was not found on a trusted PATH entry" }); + if (tarballRun.status !== 0) { + return Object.freeze({ kind: "unresolved" as const, reason: `registry tarball query failed (status ${tarballRun.status ?? "timeout"})` }); + } + const tarball = (tarballRun.stdout ?? "").replace(/["']/g, "").trim(); + let tarballOrigin: string | null = null; + try { + tarballOrigin = new URL(tarball).origin; + } catch { + tarballOrigin = null; + } + if (tarballOrigin !== CODEX_CLI_REGISTRY) { + return Object.freeze({ kind: "unresolved" as const, reason: "registry returned a tarball outside the official registry origin" }); + } + return Object.freeze({ kind: "resolved" as const, version, integrity }); + } finally { + rmSync(isolation.dir, { recursive: true, force: true }); } - return Object.freeze({ kind: "resolved" as const, version, integrity }); } /** @@ -249,7 +373,11 @@ function defaultRunInstaller(version: string, expectedIntegrity: string | null): } const stage = mkdtempSync(join(tmpdir(), "ocx-codex-cli-update-")); try { - const pack = npmTarget(["pack", `${CODEX_CLI_PACKAGE}@${version}`, "--pack-destination", stage]); + // The same pinned registry + isolated config as the resolve: the pack must fetch + // from the origin the plan bound, and the install's dependency resolution must + // not consult a redirected registry either. + const isolation = createNpmConfigIsolation(stage); + const pack = isolatedNpmTarget(["pack", `${CODEX_CLI_PACKAGE}@${version}`, "--pack-destination", stage], isolation); if (!pack) return Object.freeze({ exitCode: null }); const packRun = spawnSync(pack.bin, pack.args, { encoding: "utf8", @@ -264,7 +392,7 @@ function defaultRunInstaller(version: string, expectedIntegrity: string | null): const tarball = join(stage, tarballs[0]!); const actual = `sha512-${createHash("sha512").update(readFileSync(tarball)).digest("base64")}`; if (actual !== expectedIntegrity) return Object.freeze({ exitCode: null, integrityMismatch: true }); - const install = npmTarget(["install", "-g", tarball]); + const install = isolatedNpmTarget(["install", "-g", tarball], isolation); if (!install) return Object.freeze({ exitCode: null }); const run = spawnSync(install.bin, install.args, { encoding: "utf8", @@ -422,6 +550,13 @@ function refusedApply( * approves the exact target that is about to be installed. The install itself is one * command against an exact version; the outcome is classified from a fresh inspection, * never from the installer exit code, and nothing is retried or rolled back automatically. + * + * The update lease is acquired BEFORE the recomputation: its process scan is the final + * scan this install will ever take, and a scan without the lease is a snapshot, not + * mutual exclusion. The lease is then held through the install and the readback, so a + * Codex startup that observes it cannot begin loading the package while it is being + * replaced — and a second apply of the same plan is refused rather than running a + * concurrent global install against the same prefix. */ export async function applyCodexCliUpdatePlan( planId: string, @@ -429,56 +564,71 @@ export async function applyCodexCliUpdatePlan( ): Promise { if (!PLAN_ID_RE.test(planId)) return refusedApply("plan_unknown", null); - const plan = await createCodexCliUpdatePlan(deps); - if (!plan.applicable || !plan.planId || !plan.targetVersion || !plan.installedVersion) { - return refusedApply(plan.refusal ?? "plan_unknown", plan); + const leaseIo = deps.leaseIo ?? {}; + const acquisition = acquireCodexCliUpdateLease({ ...leaseIo, planId }); + if (!acquisition.acquired) { + // Held by a live updater, or the lock could not be created at all — both refuse + // before any evidence is gathered. A lease that cannot be taken is not a softer + // "unknown": installing without it would reintroduce the race it exists to close. + return refusedApply( + acquisition.reason === "held" ? "update_in_progress" : "update_lease_unavailable", + null, + ); } - // Any drift in ownership, installed version, location or target changes the id. - // Refuse rather than regenerate: the operator would otherwise approve - // one plan and install another. - if (plan.planId !== planId) return refusedApply("plan_stale", plan); - - const targetVersion = plan.targetVersion; - const before = plan.installedVersion; - const installer = (deps.runInstaller ?? defaultRunInstaller)(targetVersion, plan.targetIntegrity); - // The fetched artifact failed the plan-bound digest; nothing was installed. - if (installer.integrityMismatch) return refusedApply("integrity_mismatch", plan); - - const inspect = deps.inspect ?? inspectCodexCliInstall; - let readback: CodexCliInstallReport | null = null; try { - readback = await inspect(deps.inspectionDeps ?? {}); - } catch { - readback = null; - } - - const after = readback - && readback.provenance === "npm-global" - && readback.location === plan.location - && readback.versionEvidence.kind === "package-manifest" - ? readback.packageVersion - : null; - - const result = (status: CodexCliUpdateApplyStatus): CodexCliUpdateApplyResult => Object.freeze({ - schemaVersion: CODEX_CLI_UPDATE_SCHEMA_VERSION, - status, - refusal: null, - planId: plan.planId, - targetVersion, - installedVersionBefore: before, - installedVersionAfter: after, - installerExitCode: installer.exitCode, - }); + const plan = await createCodexCliUpdatePlan(deps); + if (!plan.applicable || !plan.planId || !plan.targetVersion || !plan.installedVersion) { + return refusedApply(plan.refusal ?? "plan_unknown", plan); + } + // Any drift in ownership, installed version, location or target changes the id. + // Refuse rather than regenerate: the operator would otherwise approve + // one plan and install another. + if (plan.planId !== planId) return refusedApply("plan_stale", plan); + + const targetVersion = plan.targetVersion; + const before = plan.installedVersion; + const installer = (deps.runInstaller ?? defaultRunInstaller)(targetVersion, plan.targetIntegrity); + // The fetched artifact failed the plan-bound digest; nothing was installed. + if (installer.integrityMismatch) return refusedApply("integrity_mismatch", plan); + + const inspect = deps.inspect ?? inspectCodexCliInstall; + let readback: CodexCliInstallReport | null = null; + try { + readback = await inspect(deps.inspectionDeps ?? {}); + } catch { + readback = null; + } + + const after = readback + && readback.provenance === "npm-global" + && readback.location === plan.location + && readback.versionEvidence.kind === "package-manifest" + ? readback.packageVersion + : null; + + const result = (status: CodexCliUpdateApplyStatus): CodexCliUpdateApplyResult => Object.freeze({ + schemaVersion: CODEX_CLI_UPDATE_SCHEMA_VERSION, + status, + refusal: null, + planId: plan.planId, + targetVersion, + installedVersionBefore: before, + installedVersionAfter: after, + installerExitCode: installer.exitCode, + }); - if (after !== targetVersion) { - // A failed readback, an unchanged version and a third version are all reported - // as-is. None of them is retried, and none of them is rolled back. - if (after !== null && after === before) return result("not_applied"); - return result("ambiguous"); + if (after !== targetVersion) { + // A failed readback, an unchanged version and a third version are all reported + // as-is. None of them is retried, and none of them is rolled back. + if (after !== null && after === before) return result("not_applied"); + return result("ambiguous"); + } + + // A matched shim never reaches this point: the inspector reports a shim-owned + // candidate as standalone-unverified and the plan refuses `not_managed`, so an + // applied update never owned a shim npm could have replaced. + return result("applied"); + } finally { + releaseCodexCliUpdateLease(leaseIo); } - - // A matched shim never reaches this point: the inspector reports a shim-owned - // candidate as standalone-unverified and the plan refuses `not_managed`, so an - // applied update never owned a shim npm could have replaced. - return result("applied"); } diff --git a/src/codex/desktop-app-restart.ts b/src/codex/desktop-app-restart.ts index 17bf83ed7de..b04df1ba35b 100644 --- a/src/codex/desktop-app-restart.ts +++ b/src/codex/desktop-app-restart.ts @@ -36,6 +36,7 @@ import { releaseDesktopRestartLock, type DesktopRestartLockIo, } from "./desktop-app/lock"; +import { observeCodexCliUpdateLease, type CodexCliUpdateLeaseIo } from "./cli-update-lease"; import { rootShells, type DesktopAppAdapter, type DesktopExec, type DesktopProcess } from "./desktop-app/types"; import { darwinDesktopAppAdapter, darwinDefaultExec } from "./desktop-app/darwin"; import { linuxDesktopAppAdapter, linuxDefaultExec } from "./desktop-app/linux"; @@ -77,6 +78,12 @@ export interface DesktopAppRestartIo { * after telling the operator it had been handed off. */ allowHandoff?: boolean; + /** + * Codex CLI update lease seam. While an apply holds it, relaunching the desktop app + * would start app-servers against a global install that is being replaced, so the + * restart skips rather than racing it. + */ + updateLease?: CodexCliUpdateLeaseIo; } export type DesktopAppRestartReason = @@ -86,6 +93,7 @@ export type DesktopAppRestartReason = | "no_targets" | "self_ancestry" | "restart_in_flight" + | "update_in_progress" | "handoff_started" | "targets_survived" | "relaunch_failed"; @@ -214,6 +222,12 @@ export function restartCodexDesktopApp(io: DesktopAppRestartIo = {}): DesktopApp const exec = io.execFile ?? selected?.exec; if (!adapter || !exec) return skipped("unsupported_platform"); + // Step -1. A Codex CLI update holds its lease across the install and readback; a + // relaunch under it would start app-servers against a half-replaced global + // install. Skip before touching the restart lock or signalling anything — the + // operator retries once the update finishes. + if (observeCodexCliUpdateLease(io.updateLease ?? {}).held) return skipped("update_in_progress"); + // Step 0. Two restarts at once are destructive rather than merely wasteful: the // first quits and relaunches, the second sees the freshly started shell as a target // and kills it. Own-pid reentrancy means the wp5 helper runs this same step and @@ -337,4 +351,3 @@ export function restartCodexDesktopApp(io: DesktopAppRestartIo = {}): DesktopApp if (!handedOff) releaseDesktopRestartLock(io.lock); } } - diff --git a/src/remote-control/workspace-codex-runtime.ts b/src/remote-control/workspace-codex-runtime.ts index b064e3c9b8c..ea29e39e22a 100644 --- a/src/remote-control/workspace-codex-runtime.ts +++ b/src/remote-control/workspace-codex-runtime.ts @@ -2,6 +2,10 @@ import { chmodSync, linkSync, mkdirSync, mkdtempSync, realpathSync, symlinkSync import { tmpdir } from "node:os"; import { dirname, isAbsolute, join } from "node:path"; import { resolveCodexRuntime } from "../codex/runtime"; +import { + waitForCodexCliUpdateLeaseRelease, + type CodexCliUpdateLeaseWaitIo, +} from "../codex/cli-update-lease"; import { remoteWorkspaceThreadStartParams } from "./workspace-coordinator"; import { startRemoteWorkspaceToolBridge } from "./workspace-tool-bridge"; import { truncateRemoteWorkspaceUtf8 } from "./workspace-utf8"; @@ -288,6 +292,12 @@ export interface CodexRemoteWorkspaceRuntimeOptions { command?: readonly string[]; env?: Record; version?: string; + /** + * Codex CLI update lease seam. Production reads the real lockfile; while an apply + * holds it, the npm-global Codex this spawn would load is being replaced, so the + * start waits briefly for the tail of the install and then refuses. + */ + updateLease?: CodexCliUpdateLeaseWaitIo & { timeoutMs?: number }; } export class CodexRemoteWorkspaceRuntimeFactory implements RemoteWorkspaceRuntimeFactory { @@ -308,6 +318,12 @@ export class CodexRemoteWorkspaceRuntimeFactory implements RemoteWorkspaceRuntim } async start(options: Parameters[0]): Promise { + // The update lease is the mutual exclusion the process scan cannot give: without + // it an apply could be mid-install while this app-server loads the package. + const leaseFree = await waitForCodexCliUpdateLeaseRelease(this.options.updateLease ?? {}); + if (!leaseFree) { + throw new Error("a Codex CLI update is in progress; refusing to start a Codex app-server mid-install"); + } const command = this.options.command ? [...this.options.command] : [resolveCodexRuntime().runtime.command]; diff --git a/tests/clients/desktop-app-restart.test.ts b/tests/clients/desktop-app-restart.test.ts index fd7ae597dcd..4f638d97e94 100644 --- a/tests/clients/desktop-app-restart.test.ts +++ b/tests/clients/desktop-app-restart.test.ts @@ -4,6 +4,7 @@ import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { restartCodexDesktopApp, type DesktopAppRestartIo } from "../../src/codex/desktop-app-restart"; +import { acquireCodexCliUpdateLease } from "../../src/codex/cli-update-lease"; import { windowsDesktopAppAdapter } from "../../src/codex/desktop-app/windows"; import { setTrustedWindowsElevationExecutablesForTests } from "../../src/lib/windows-elevation"; @@ -158,6 +159,24 @@ describe("Codex desktop app restart (#2292)", () => { expect(calls).toEqual([]); }); + test("a held Codex CLI update lease skips the restart before probing anything", () => { + // Relaunching the desktop app mid-install would start app-servers against the + // half-replaced global package; the lease makes the restart defer instead. + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + expect(acquireCodexCliUpdateLease({ lockPath, pid: 4_242 }).acquired).toBe(true); + const calls: Call[] = []; + const result = restartCodexDesktopApp({ + ...scriptedIo({ discovery: DISCOVERY, calls }), + updateLease: { lockPath, isAlive: () => true }, + }); + expect(result).toEqual({ + attempted: false, stopped: [], surviving: [], relaunch: "skipped", reason: "update_in_progress", + }); + // The lease check precedes discovery and the restart lock: nothing ran. + expect(calls).toEqual([]); + }); + test("fails closed when the package cannot be identified, killing nothing", () => { const calls: Call[] = []; const result = withTrustedExes(() => restartCodexDesktopApp(scriptedIo({ discovery: "MISS", calls }))); @@ -447,4 +466,3 @@ describe("#2557 a failed probe is not an absent app", () => { expect(script).not.toContain("SilentlyContinue' $root"); }); }); - diff --git a/tests/clients/remote-workspace-codex-runtime.test.ts b/tests/clients/remote-workspace-codex-runtime.test.ts index b9d259c2ebb..cd96856ddca 100644 --- a/tests/clients/remote-workspace-codex-runtime.test.ts +++ b/tests/clients/remote-workspace-codex-runtime.test.ts @@ -1,6 +1,9 @@ import { repoPath } from "../helpers/repo-root"; import { expect, test } from "bun:test"; -import { resolve } from "node:path"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { acquireCodexCliUpdateLease } from "../../src/codex/cli-update-lease"; import { CodexRemoteWorkspaceRuntimeFactory, RemoteWorkspaceCoordinator, @@ -8,6 +11,66 @@ import { type RemoteWorkspaceTransport, } from "../../src/remote-control"; +function startOptions(coordinator: RemoteWorkspaceCoordinator) { + return { + sessionId: "session-lease", + deviceId: "device-2", + deviceName: "Computer 2", + rootId: "root-2", + rootLabel: "Project", + capabilities: ["workspace.read"], + tools: ["list_directory", "read_file"], + coordinator, + emit: () => {}, + }; +} + +function leaseCoordinator(): RemoteWorkspaceCoordinator { + return new RemoteWorkspaceCoordinator({ + isOnline: () => true, + async invoke() { return { ok: true }; }, + }); +} + +test("a held Codex CLI update lease refuses the app-server start before spawn", async () => { + // The lease is the mutual exclusion the process scan cannot give: an app-server + // that starts while an apply holds it would load a half-replaced global install. + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + const held = acquireCodexCliUpdateLease({ lockPath, pid: 4_242, planId: "a".repeat(32) }); + expect(held.acquired).toBe(true); + + const factory = new CodexRemoteWorkspaceRuntimeFactory({ + command: [process.execPath, repoPath("tests", "fake-codex-server.ts")], + updateLease: { lockPath, isAlive: () => true, timeoutMs: 0 }, + }); + await expect(factory.start(startOptions(leaseCoordinator()))) + .rejects.toThrow("Codex CLI update is in progress"); +}); + +test("the app-server start waits out the lease and proceeds once it clears", async () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + const held = acquireCodexCliUpdateLease({ lockPath, pid: 4_242, planId: "a".repeat(32) }); + expect(held.acquired).toBe(true); + + // The owner is alive on the first observation and gone by the second — the tail + // of an install releasing, which a startup may wait out rather than refuse. + let livenessChecks = 0; + const factory = new CodexRemoteWorkspaceRuntimeFactory({ + command: [process.execPath, repoPath("tests", "fake-codex-server.ts")], + env: { FAKE_CODEX_SCRIPT: JSON.stringify({ turns: [] }) }, + updateLease: { + lockPath, + isAlive: () => livenessChecks++ === 0, + sleep: async () => {}, + timeoutMs: 1_000, + }, + }); + const handle = await factory.start(startOptions(leaseCoordinator())); + await handle.stop(); +}); + test("Codex Remote Workspace runtime owns the model process on the Hub", async () => { const events: Array<{ type: RemoteWorkspaceSessionEvent["type"]; text: string }> = []; const transport: RemoteWorkspaceTransport = { diff --git a/tests/codex-integration/codex-cli-update-plan.test.ts b/tests/codex-integration/codex-cli-update-plan.test.ts index 7420f9b3910..fe92bda6220 100644 --- a/tests/codex-integration/codex-cli-update-plan.test.ts +++ b/tests/codex-integration/codex-cli-update-plan.test.ts @@ -1,9 +1,19 @@ import { describe, expect, test } from "bun:test"; +import { existsSync, mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + import { scanCodexAppServerProcesses } from "../../src/codex/app-server-processes"; import type { CodexCliInstallReport } from "../../src/codex/cli-install-provenance"; +import { + acquireCodexCliUpdateLease, + observeCodexCliUpdateLease, + type CodexCliUpdateLeaseIo, +} from "../../src/codex/cli-update-lease"; import { applyCodexCliUpdatePlan, + CODEX_CLI_REGISTRY, codexCliUpdatePlanId, createCodexCliUpdatePlan, resolveCodexCliUpdateTarget, @@ -378,6 +388,7 @@ describe("registry target resolution", () => { const target = resolveCodexCliUpdateTarget("latest", spawnStub([ { status: 0, stdout: "1.4.2\n" }, { status: 0, stdout: "'sha512-abc/DEF+123=' sha1-old\n" }, + { status: 0, stdout: "https://registry.npmjs.org/@openai/codex/-/codex-1.4.2.tgz\n" }, ])); expect(target).toEqual({ kind: "resolved", version: "1.4.2", integrity: "sha512-abc/DEF+123=" }); }); @@ -401,4 +412,254 @@ describe("registry target resolution", () => { ])); expect(target.kind).toBe("unresolved"); }); + + test("a tarball outside the pinned registry origin is refused even with a valid digest", () => { + const target = resolveCodexCliUpdateTarget("latest", spawnStub([ + { status: 0, stdout: "1.4.2\n" }, + { status: 0, stdout: "sha512-abc/DEF+123=\n" }, + { status: 0, stdout: "https://evil.invalid/@openai/codex/-/codex-1.4.2.tgz\n" }, + ])); + expect(target.kind).toBe("unresolved"); + }); + + test("a tarball answer that is not a URL at all is refused", () => { + const target = resolveCodexCliUpdateTarget("latest", spawnStub([ + { status: 0, stdout: "1.4.2\n" }, + { status: 0, stdout: "sha512-abc/DEF+123=\n" }, + { status: 0, stdout: "not-a-url\n" }, + ])); + expect(target.kind).toBe("unresolved"); + }); +}); + +describe("registry configuration isolation", () => { + interface CapturedCall { + bin: string; + args: string[]; + options: Record; + /** Snapshot taken while the call was live; the isolation dir is gone by return. */ + cwdHadSentinel: boolean; + cwdHadNpmrc: boolean; + } + + /** The npm argv, whether it reached spawn bare (POSIX) or inside a cmd /c line (Windows). */ + function argvLine(call: CapturedCall): string { + return call.args.join(" "); + } + + /** Which registry field this invocation queried, read off the argv line. */ + function queriedField(call: CapturedCall): string { + const line = argvLine(call); + if (line.includes("dist.tarball")) return "dist.tarball"; + if (line.includes("dist.integrity")) return "dist.integrity"; + return "version"; + } + + function capturingSpawn(outputs: Record): { calls: CapturedCall[]; spawn: never } { + const calls: CapturedCall[] = []; + const spawn = ((bin: string, args: string[], options: Record) => { + const cwd = options.cwd as string; + const call: CapturedCall = { + bin, + args, + options, + cwdHadSentinel: existsSync(join(cwd, "package.json")), + cwdHadNpmrc: existsSync(join(cwd, "ocx-update.npmrc")), + }; + calls.push(call); + const field = queriedField(call); + return { status: 0, stdout: outputs[field] ?? "", stderr: "" }; + }) as never; + return { calls, spawn }; + } + + const RESOLVE_OUTPUTS = { + version: "1.4.2\n", + "dist.integrity": "sha512-abc/DEF+123=\n", + "dist.tarball": "https://registry.npmjs.org/@openai/codex/-/codex-1.4.2.tgz\n", + }; + + test("every query pins the official registry and substitutes a controlled npmrc", () => { + const { calls, spawn } = capturingSpawn(RESOLVE_OUTPUTS); + const target = resolveCodexCliUpdateTarget("latest", spawn); + expect(target.kind).toBe("resolved"); + expect(calls.length).toBe(3); + for (const call of calls) { + expect(argvLine(call)).toContain("--registry=" + CODEX_CLI_REGISTRY); + // Both file configs are substituted with the controlled empty npmrc inside the + // isolation dir — a user ~/.npmrc or a global $PREFIX/etc/npmrc cannot answer. + expect(argvLine(call)).toContain("--userconfig="); + expect(argvLine(call)).toContain("--globalconfig="); + expect(argvLine(call)).toContain("ocx-update.npmrc"); + expect(call.cwdHadSentinel).toBe(true); + expect(call.cwdHadNpmrc).toBe(true); + } + // The isolation directory is cleaned up after the resolve. + expect(existsSync(calls[0]!.options.cwd as string)).toBe(false); + }); + + test("a hostile npm_config_* env cannot reach the spawned npm", () => { + const prior = process.env.npm_config_registry; + const priorScoped = process.env["npm_config_@openai:registry"]; + process.env.npm_config_registry = "https://evil.invalid"; + process.env["npm_config_@openai:registry"] = "https://evil.invalid/"; + try { + const { calls, spawn } = capturingSpawn(RESOLVE_OUTPUTS); + const target = resolveCodexCliUpdateTarget("latest", spawn); + expect(target.kind).toBe("resolved"); + for (const call of calls) { + const env = call.options.env as NodeJS.ProcessEnv; + expect(Object.keys(env).filter(key => key.toLowerCase().startsWith("npm_config_"))).toEqual([]); + } + } finally { + if (prior === undefined) delete process.env.npm_config_registry; + else process.env.npm_config_registry = prior; + if (priorScoped === undefined) delete process.env["npm_config_@openai:registry"]; + else process.env["npm_config_@openai:registry"] = priorScoped; + } + }); + + test("the controlled cwd never is the operator's project directory", () => { + // A project .npmrc in the launch directory would otherwise redirect the scope; + // the query must run from the sentinel-anchored isolation dir instead. + const { calls, spawn } = capturingSpawn(RESOLVE_OUTPUTS); + resolveCodexCliUpdateTarget("latest", spawn); + for (const call of calls) { + const cwd = call.options.cwd as string; + expect(cwd).not.toBe(process.cwd()); + expect(cwd.startsWith(tmpdir())).toBe(true); + } + }); +}); + +describe("Codex CLI update lease", () => { + function leaseIo(dir: string, pid: number, alive: ReadonlySet): CodexCliUpdateLeaseIo { + return { + lockPath: join(dir, "codex-cli-update.lock"), + pid, + isAlive: candidate => alive.has(candidate), + }; + } + + function heldLease(dir: string, holderPid: number): CodexCliUpdateLeaseIo { + const io = leaseIo(dir, holderPid, new Set([holderPid])); + const acquisition = acquireCodexCliUpdateLease({ ...io, planId: "a".repeat(32) }); + expect(acquisition.acquired).toBe(true); + return io; + } + + test("a live holder refuses a second apply before any evidence is gathered", async () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const holder = heldLease(dir, 4_242); + const installs: string[] = []; + let inspected = 0; + const result = await applyCodexCliUpdatePlan("b".repeat(32), applyDeps({ + leaseIo: { lockPath: holder.lockPath, isAlive: pid => pid === 4_242 }, + inspect: async () => { inspected += 1; return report(); }, + }, installs)); + expect(result.status).toBe("refused"); + expect(result.refusal).toBe("update_in_progress"); + // The refusal happens before the plan is recomputed: no inspection, no install. + expect(inspected).toBe(0); + expect(installs).toEqual([]); + }); + + test("two concurrent applies cannot pass the same plan", async () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + const installs: string[] = []; + const plan = await applicablePlan(); + + // The first apply holds the lease while its inspection is still in flight. + let releaseInspection: (value: CodexCliInstallReport) => void = () => {}; + const gate = new Promise(done => { releaseInspection = done; }); + let firstCalls = 0; + const first = applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + leaseIo: { lockPath }, + inspect: async () => { + firstCalls += 1; + return firstCalls === 1 ? await gate : report({ packageVersion: "1.1.0" }); + }, + }, installs)); + // Let the first apply reach its gated inspection while holding the lease. + await new Promise(done => setTimeout(done, 10)); + + const second = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + leaseIo: { lockPath }, + }, installs)); + expect(second.status).toBe("refused"); + expect(second.refusal).toBe("update_in_progress"); + + releaseInspection(report()); + const firstResult = await first; + expect(firstResult.status).toBe("applied"); + expect(installs).toEqual(["1.1.0"]); + // The holder released the lease when it finished. + expect(existsSync(lockPath)).toBe(false); + }); + + test("a dead holder's lease is reclaimed instead of blocking forever", async () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + // A holder that died without releasing: dead pid, stale or not. + const stale = acquireCodexCliUpdateLease({ lockPath, pid: 9_999, planId: "c".repeat(32) }); + expect(stale.acquired).toBe(true); + + const installs: string[] = []; + const plan = await applicablePlan(); + let inspections = 0; + const result = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + leaseIo: { lockPath, isAlive: () => false }, + inspect: async () => { + inspections += 1; + return inspections === 1 ? report() : report({ packageVersion: "1.1.0" }); + }, + }, installs)); + expect(result.status).toBe("applied"); + expect(installs).toEqual(["1.1.0"]); + expect(existsSync(lockPath)).toBe(false); + }); + + test("a refused apply still releases the lease it took", async () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + const plan = await applicablePlan(); + const result = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + leaseIo: { lockPath }, + // Drift the target so the recomputed plan is refused as stale. + resolveTarget: () => Object.freeze({ kind: "resolved" as const, version: "9.9.9", integrity: "sha512-ZZZZ" }), + })); + expect(result.status).toBe("refused"); + expect(result.refusal).toBe("plan_stale"); + expect(existsSync(lockPath)).toBe(false); + }); + + test("a corrupt leftover lease cannot wedge every future update", async () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + writeFileSync(lockPath, "{not json"); + const installs: string[] = []; + const plan = await applicablePlan(); + let inspections = 0; + const result = await applyCodexCliUpdatePlan(plan.planId!, applyDeps({ + leaseIo: { lockPath }, + inspect: async () => { + inspections += 1; + return inspections === 1 ? report() : report({ packageVersion: "1.1.0" }); + }, + }, installs)); + expect(result.status).toBe("applied"); + expect(existsSync(lockPath)).toBe(false); + }); + + test("startup observation reads a held lease and ignores a stale one", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + const held = acquireCodexCliUpdateLease({ lockPath, pid: 5_555 }); + expect(held.acquired).toBe(true); + // Observation is read-only and liveness-bound: the same file is "held" while the + // owner lives and "free" once it is gone, without the observer mutating anything. + expect(observeCodexCliUpdateLease({ lockPath, isAlive: () => true }).held).toBe(true); + expect(observeCodexCliUpdateLease({ lockPath, isAlive: () => false }).held).toBe(false); + }); }); From a1da7370507b0a7739a841a7afb86228b86369b2 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 09:43:27 +0000 Subject: [PATCH 8/8] fix(codex): self-identifying token lease with atomic publish and heartbeat The update lease had four mutual-exclusion holes: a stale-record unlink could delete a successor's fresh lease, the O_EXCL create-before-write window let a contender steal the publish, release compared only ownerPid, and a live holder could be reaped on age alone. Records now carry a random token verified inside a link-based compare-and-delete, publication goes through a hardlinked staging file (with a publish-grace fallback), release requires the holder's token, and a heartbeat keeps a live holder past the age bound. Co-Authored-By: Epinephrine --- src/codex/cli-update-lease.ts | 323 ++++++++++++++---- src/codex/cli-update-plan.ts | 7 +- .../codex-cli-update-plan.test.ts | 70 +++- 3 files changed, 341 insertions(+), 59 deletions(-) diff --git a/src/codex/cli-update-lease.ts b/src/codex/cli-update-lease.ts index 5f116ae5a46..4388383bc53 100644 --- a/src/codex/cli-update-lease.ts +++ b/src/codex/cli-update-lease.ts @@ -8,35 +8,74 @@ * readback, and the Codex startup paths this codebase controls (remote workspace * app-server spawn, desktop-app relaunch) observe it and refuse or wait. * - * The mechanics deliberately mirror desktop-app/lock.ts: exclusive O_EXCL create on - * the contended path, owner-pid liveness first and an age bound second for staleness, - * and compare-and-delete release so a late holder can never unlink a successor's - * lease. + * The record is self-identifying: every holder publishes a random `token`, and every + * mutating transition (stale reclaim, release, heartbeat) compares that token against + * what is actually on disk via a compare-and-delete that links the current file to a + * tombstone before unlinking — a contender that observed record A can never unlink + * record B, and a late release can never delete a successor that recycled the path. + * + * Publication is atomic where the filesystem allows it: the record is written to a + * staging sibling and `linkSync`'d onto the contended path, so the lock file never + * exists in the empty/half-written state that `openSync("wx")` + `writeSync` leaves. + * Filesystems without hardlinks fall back to the O_EXCL create, and a corrupt record + * younger than PUBLISH_GRACE_MS reads as held rather than deletable, so even there a + * contender cannot steal the write window of an in-flight publisher. + * + * A live owner is never reaped by age alone: staleness needs a dead pid OR a + * heartbeat older than the bound (`startCodexCliUpdateLeaseHeartbeat` keeps it + * fresh for the length of a real install). A wedged holder whose heartbeats stopped + * is still reclaimable; one that is merely slow is not. * * Deliberately NOT own-pid reentrant: two applies in one process are still two * contenders for one global install, and the second must see the lease as held. */ -import { closeSync, mkdirSync, openSync, readFileSync, unlinkSync, writeSync } from "node:fs"; +import { randomBytes } from "node:crypto"; +import { + closeSync, + linkSync, + mkdirSync, + openSync, + readFileSync, + renameSync, + statSync, + unlinkSync, + writeSync, +} from "node:fs"; import { dirname, join } from "node:path"; import { getConfigDir } from "../config/paths"; -import { isProcessAlive } from "../lib/process-control"; /** - * A lease older than this is stale regardless of what its owner pid says. The bound - * exists for a live pid that no longer names an updater (a recycled pid, or a wedged - * holder); it is comfortably longer than the pack + install timeouts a real apply can - * legitimately hold the lease for. + * A holder whose heartbeat is older than this is stale regardless of what its owner + * pid says. The bound exists for a live pid that no longer names an updater (a + * recycled pid, or a wedged holder); it is comfortably longer than the pack + + * install timeouts a real apply can legitimately hold the lease for, and a healthy + * holder heartbeats far below it. */ export const CODEX_CLI_UPDATE_LEASE_MAX_AGE_MS = 15 * 60_000; /** Startup paths poll a held lease for this long before refusing the launch. */ export const CODEX_CLI_UPDATE_LEASE_WAIT_MS = 5_000; +/** + * A record that exists on disk but fails to parse is treated as held for this long + * after its mtime. On filesystems without hardlinks the O_EXCL fallback leaves a + * brief empty window; deleting inside it would hand a second contender the lease + * while the first is still writing. + */ +const PUBLISH_GRACE_MS = 10_000; + +/** Heartbeat interval: comfortably below the staleness bound. */ +const HEARTBEAT_INTERVAL_MS = 60_000; + export interface CodexCliUpdateLeaseRecord { readonly version: 1; readonly ownerPid: number; readonly createdAtMs: number; + /** Self-identifying holder token; every mutating transition verifies it. */ + readonly token: string; + /** Last heartbeat; older than the staleness bound means wedged even when pid lives. */ + readonly heartbeatAtMs: number; /** The plan id being applied. Diagnostics only; exclusion never reads it. */ readonly planId: string | null; } @@ -46,6 +85,8 @@ export interface CodexCliUpdateLeaseIo { isAlive?: (pid: number) => boolean; now?: () => number; pid?: number; + /** Test seam: force the O_EXCL publication fallback (hardlink-less filesystems). */ + forceExclusiveCreateFallback?: boolean; } export interface CodexCliUpdateLeaseWaitIo extends CodexCliUpdateLeaseIo { @@ -54,7 +95,7 @@ export interface CodexCliUpdateLeaseWaitIo extends CodexCliUpdateLeaseIo { export type CodexCliUpdateLeaseAcquisition = | { acquired: true; record: CodexCliUpdateLeaseRecord } - /** A live owner inside the age bound holds the lease. */ + /** A live owner inside the staleness bound holds the lease. */ | { acquired: false; reason: "held"; heldBy: number } /** The lock path could not be created or observed at all. */ | { acquired: false; reason: "unavailable" }; @@ -74,31 +115,83 @@ function defaultIsAlive(pid: number): boolean { } } +function parseRecord(raw: string): CodexCliUpdateLeaseRecord | null { + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return null; + } + if (typeof parsed !== "object" || parsed === null) return null; + const view = parsed as Record; + const ownerPid = view.ownerPid; + const createdAtMs = view.createdAtMs; + const token = view.token; + const planId = view.planId; + if (view.version !== 1) return null; + if (typeof ownerPid !== "number" || !Number.isSafeInteger(ownerPid) || ownerPid <= 0) return null; + if (typeof createdAtMs !== "number" || !Number.isFinite(createdAtMs)) return null; + if (typeof token !== "string" || token.length === 0) return null; + const heartbeatAtMs = view.heartbeatAtMs; + return { + version: 1, + ownerPid, + createdAtMs, + token, + heartbeatAtMs: + typeof heartbeatAtMs === "number" && Number.isFinite(heartbeatAtMs) + ? heartbeatAtMs + : createdAtMs, + planId: typeof planId === "string" ? planId : null, + }; +} + function readRecord(path: string): CodexCliUpdateLeaseRecord | null { try { - const parsed: unknown = JSON.parse(readFileSync(path, "utf-8")); - if (typeof parsed !== "object" || parsed === null) return null; - const view = parsed as Record; - const ownerPid = view.ownerPid; - const createdAtMs = view.createdAtMs; - if (view.version !== 1) return null; - if (typeof ownerPid !== "number" || !Number.isSafeInteger(ownerPid) || ownerPid <= 0) return null; - if (typeof createdAtMs !== "number" || !Number.isFinite(createdAtMs)) return null; - const planId = view.planId; - return { - version: 1, - ownerPid, - createdAtMs, - planId: typeof planId === "string" ? planId : null, - }; + return parseRecord(readFileSync(path, "utf-8")); } catch { return null; } } -/** Exclusive create on the contended path — the whole mutual exclusion. */ -function tryCreateExclusive(path: string, record: CodexCliUpdateLeaseRecord): boolean { - mkdirSync(dirname(path), { recursive: true }); +function newToken(): string { + return randomBytes(16).toString("hex"); +} + +/** + * Publish the record atomically: stage the full content on a unique sibling, then + * hardlink it onto the contended path. `linkSync` fails with EEXIST if the lock + * already exists, so the file is never observable without its full record. On a + * filesystem without hardlinks the caller's O_EXCL fallback applies. + */ +function publishViaLink(path: string, record: CodexCliUpdateLeaseRecord): boolean { + const staging = `${path}.staging-${process.pid}-${record.token}`; + try { + const fd = openSync(staging, "wx", 0o600); + try { + writeSync(fd, JSON.stringify(record)); + } finally { + closeSync(fd); + } + try { + linkSync(staging, path); + return true; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code === "EEXIST") return false; + throw err; + } + } finally { + try { + unlinkSync(staging); + } catch { + /* staging already gone, or was never created */ + } + } +} + +/** O_EXCL fallback for filesystems that cannot hardlink (no atomic publish). */ +function publishViaExclusiveCreate(path: string, record: CodexCliUpdateLeaseRecord): boolean { let fd: number; try { fd = openSync(path, "wx", 0o600); @@ -113,10 +206,83 @@ function tryCreateExclusive(path: string, record: CodexCliUpdateLeaseRecord): bo return true; } -function holderIsLive(record: CodexCliUpdateLeaseRecord, io: CodexCliUpdateLeaseIo): boolean { +/** Exclusive publish on the contended path — the whole mutual exclusion. */ +function tryPublish( + path: string, + record: CodexCliUpdateLeaseRecord, + io: CodexCliUpdateLeaseIo, +): boolean { + mkdirSync(dirname(path), { recursive: true }); + if (io.forceExclusiveCreateFallback) return publishViaExclusiveCreate(path, record); + try { + return publishViaLink(path, record); + } catch { + return publishViaExclusiveCreate(path, record); + } +} + +/** Rewrite the lock in place. Only safe for the holder: it is not the contended path. */ +function rewriteRecord(path: string, record: CodexCliUpdateLeaseRecord): void { + const staging = `${path}.hb-${process.pid}-${record.token}`; + const fd = openSync(staging, "wx", 0o600); + try { + writeSync(fd, JSON.stringify(record)); + } finally { + closeSync(fd); + } + renameSync(staging, path); +} + +/** + * Compare-and-delete: unlink `path` only while it still names `expected.token`. + * + * Hardlink the current file to a tombstone first: the link binds the inode we + * verified, so an owner that rewrote its record (heartbeat) or a successor that + * never existed cannot be caught by the unlink. Without hardlinks we fall back to + * re-read + token compare immediately before unlink — the window narrows to syscall + * granularity, which is the best a lockfile can do there. + */ +function deleteIfToken(path: string, expected: CodexCliUpdateLeaseRecord): boolean { + const tombstone = `${path}.gone-${expected.token}`; + try { + linkSync(path, tombstone); + } catch { + // No link support or already gone: verify by re-read and accept the narrow race. + const current = readRecord(path); + if (!current || current.token !== expected.token) return false; + try { + unlinkSync(path); + return true; + } catch { + return false; + } + } + try { + const pathIno = statSync(path).ino; + const tombIno = statSync(tombstone).ino; + const tombRecord = readRecord(tombstone); + // A zero inode means the platform cannot express identity — use the token-only check. + const sameFile = pathIno !== 0 && tombIno !== 0 ? pathIno === tombIno : true; + if (sameFile && tombRecord && tombRecord.token === expected.token) { + unlinkSync(path); + return true; + } + return false; + } finally { + try { + unlinkSync(tombstone); + } catch { + /* tombstone already gone */ + } + } +} + +function holderIsStale(record: CodexCliUpdateLeaseRecord, io: CodexCliUpdateLeaseIo): boolean { const isAlive = io.isAlive ?? defaultIsAlive; const now = io.now ?? Date.now; - return isAlive(record.ownerPid) && now() - record.createdAtMs <= CODEX_CLI_UPDATE_LEASE_MAX_AGE_MS; + if (!isAlive(record.ownerPid)) return true; + // A live pid is held only while its heartbeat is fresh; age alone never reaps it. + return now() - record.heartbeatAtMs > CODEX_CLI_UPDATE_LEASE_MAX_AGE_MS; } /** @@ -135,16 +301,19 @@ export function acquireCodexCliUpdateLease( const existing = readRecord(path); if (existing) { - if (holderIsLive(existing, io)) { + if (!holderIsStale(existing, io)) { return { acquired: false, reason: "held", heldBy: existing.ownerPid }; } - // Stale. Clear it and then compete for the exclusive create like anyone else: - // two processes can observe the same stale lease, and only O_EXCL decides which - // one actually holds it. - try { - unlinkSync(path); - } catch { - /* somebody else cleared it first — the create below still decides */ + // Stale. Remove it only while it still names the record we observed: two + // contenders can see the same stale lease, and an unconditional unlink would + // let the slower one delete the faster one's fresh lease. After compare-delete, + // the publish below decides which of us actually holds it. + if (!deleteIfToken(path, existing)) { + const successor = readRecord(path); + if (!successor) return { acquired: false, reason: "unavailable" }; + return holderIsStale(successor, io) + ? { acquired: false, reason: "unavailable" } + : { acquired: false, reason: "held", heldBy: successor.ownerPid }; } } @@ -152,47 +321,87 @@ export function acquireCodexCliUpdateLease( version: 1, ownerPid: self, createdAtMs: now(), + token: newToken(), + heartbeatAtMs: now(), planId: io.planId ?? null, }; - if (tryCreateExclusive(path, record)) return { acquired: true, record }; + if (tryPublish(path, record, io)) return { acquired: true, record }; const winner = readRecord(path); if (winner) { - return holderIsLive(winner, io) - ? { acquired: false, reason: "held", heldBy: winner.ownerPid } - : { acquired: false, reason: "unavailable" }; + return holderIsStale(winner, io) + ? { acquired: false, reason: "unavailable" } + : { acquired: false, reason: "held", heldBy: winner.ownerPid }; } - // The file exists but names nobody: truncated, corrupt, or left by a writer that - // died between create and write. Remove it and make exactly one more attempt so a - // lock nobody holds cannot wedge every future update. + // The file exists but names nobody: truncated or corrupt. If it is younger than + // the publish grace it is a contender's in-flight write on the O_EXCL fallback — + // held, not clearable. Past the grace it is a dead writer's debris: one ownerless + // file must not wedge every future update, so clear it (compare-delete on the + // path identity) and make exactly one more attempt. + try { + const stat = statSync(path); + if (now() - stat.mtimeMs < PUBLISH_GRACE_MS) { + return { acquired: false, reason: "unavailable" }; + } + } catch { + return { acquired: false, reason: "unavailable" }; + } try { unlinkSync(path); } catch { /* somebody else cleared it first */ } - if (tryCreateExclusive(path, record)) return { acquired: true, record }; + if (tryPublish(path, record, io)) return { acquired: true, record }; const successor = readRecord(path); - return successor && holderIsLive(successor, io) + return successor && !holderIsStale(successor, io) ? { acquired: false, reason: "held", heldBy: successor.ownerPid } : { acquired: false, reason: "unavailable" }; } -/** Compare-and-delete. Never removes a lease owned by another process. */ -export function releaseCodexCliUpdateLease(io: CodexCliUpdateLeaseIo = {}): void { +/** + * Compare-and-delete release bound to the holder's token — a late release can never + * unlink a successor's lease, and a release naming only a pid (recycled or not) + * cannot unlink a successor associated with the same pid edge case. + */ +export function releaseCodexCliUpdateLease( + io: CodexCliUpdateLeaseIo & { token?: string } = {}, +): void { const path = io.lockPath ?? defaultCodexCliUpdateLeasePath(); const self = io.pid ?? process.pid; const existing = readRecord(path); if (!existing || existing.ownerPid !== self) return; - try { - unlinkSync(path); - } catch { - /* already gone */ - } + if (io.token !== undefined && existing.token !== io.token) return; + deleteIfToken(path, existing); +} + +/** + * Keep a held lease's heartbeat fresh for the duration of a long install. Returns a + * stop function; the interval is unref'd so it never keeps a process alive. The + * heartbeat re-verifies its own token before rewriting, so a holder that was already + * reaped (or superseded) simply stops updating someone else's lease. + */ +export function startCodexCliUpdateLeaseHeartbeat( + record: CodexCliUpdateLeaseRecord, + io: CodexCliUpdateLeaseIo = {}, +): () => void { + const path = io.lockPath ?? defaultCodexCliUpdateLeasePath(); + const now = io.now ?? Date.now; + const timer = setInterval(() => { + const current = readRecord(path); + if (!current || current.token !== record.token) return; + try { + rewriteRecord(path, { ...current, heartbeatAtMs: now() }); + } catch { + /* a successor or a cleared lease: the next tick re-verifies and stays inert */ + } + }, HEARTBEAT_INTERVAL_MS); + timer.unref(); + return () => clearInterval(timer); } export interface CodexCliUpdateLeaseObservation { - /** True only while a live owner inside the age bound holds the lease. */ + /** True only while a live owner with a fresh heartbeat holds the lease. */ readonly held: boolean; readonly ownerPid: number | null; } @@ -206,7 +415,7 @@ export function observeCodexCliUpdateLease( io: CodexCliUpdateLeaseIo = {}, ): CodexCliUpdateLeaseObservation { const existing = readRecord(io.lockPath ?? defaultCodexCliUpdateLeasePath()); - if (!existing || !holderIsLive(existing, io)) return { held: false, ownerPid: null }; + if (!existing || holderIsStale(existing, io)) return { held: false, ownerPid: null }; return { held: true, ownerPid: existing.ownerPid }; } diff --git a/src/codex/cli-update-plan.ts b/src/codex/cli-update-plan.ts index 19b145c3e29..26efee767c8 100644 --- a/src/codex/cli-update-plan.ts +++ b/src/codex/cli-update-plan.ts @@ -20,6 +20,7 @@ import { import { acquireCodexCliUpdateLease, releaseCodexCliUpdateLease, + startCodexCliUpdateLeaseHeartbeat, type CodexCliUpdateLeaseIo, } from "./cli-update-lease"; @@ -575,6 +576,9 @@ export async function applyCodexCliUpdatePlan( null, ); } + // A real install can outlast any fixed age bound; the heartbeat keeps the lease + // self-identifying as live so a contender cannot reap it on age alone. + const stopHeartbeat = startCodexCliUpdateLeaseHeartbeat(acquisition.record, leaseIo); try { const plan = await createCodexCliUpdatePlan(deps); if (!plan.applicable || !plan.planId || !plan.targetVersion || !plan.installedVersion) { @@ -629,6 +633,7 @@ export async function applyCodexCliUpdatePlan( // applied update never owned a shim npm could have replaced. return result("applied"); } finally { - releaseCodexCliUpdateLease(leaseIo); + stopHeartbeat(); + releaseCodexCliUpdateLease({ ...leaseIo, token: acquisition.record.token }); } } diff --git a/tests/codex-integration/codex-cli-update-plan.test.ts b/tests/codex-integration/codex-cli-update-plan.test.ts index fe92bda6220..f45602fd441 100644 --- a/tests/codex-integration/codex-cli-update-plan.test.ts +++ b/tests/codex-integration/codex-cli-update-plan.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; -import { existsSync, mkdtempSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, readFileSync, utimesSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -9,6 +9,7 @@ import type { CodexCliInstallReport } from "../../src/codex/cli-install-provenan import { acquireCodexCliUpdateLease, observeCodexCliUpdateLease, + releaseCodexCliUpdateLease, type CodexCliUpdateLeaseIo, } from "../../src/codex/cli-update-lease"; import { @@ -638,6 +639,9 @@ describe("Codex CLI update lease", () => { const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); const lockPath = join(dir, "codex-cli-update.lock"); writeFileSync(lockPath, "{not json"); + // Backdate it past the publish grace: a fresh unparseable file is a contender's + // in-flight write, not debris, and must not be cleared under it. + utimesSync(lockPath, 0, 0); const installs: string[] = []; const plan = await applicablePlan(); let inspections = 0; @@ -652,6 +656,70 @@ describe("Codex CLI update lease", () => { expect(existsSync(lockPath)).toBe(false); }); + test("a fresh unparseable record is a publish in flight, not debris", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + // The O_EXCL fallback leaves a create-before-record window: a contender that + // sees it must report contention/unavailable, never unlink the publisher's file. + writeFileSync(lockPath, ""); + const attempt = acquireCodexCliUpdateLease({ lockPath, pid: 7_777 }); + expect(attempt.acquired).toBe(false); + expect(existsSync(lockPath)).toBe(true); + // Once the file is old it is genuinely dead debris and reclaims. + utimesSync(lockPath, 0, 0); + const retry = acquireCodexCliUpdateLease({ lockPath, pid: 7_777 }); + expect(retry.acquired).toBe(true); + }); + + test("a contender that observed a stale record cannot delete the successor's lease", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + // A holds a dead-pid lease; B observes it stale and takes over. The record A + // published must never let a slower contender (or A's own late release) delete B's. + const a = acquireCodexCliUpdateLease({ lockPath, pid: 1_111, planId: "a".repeat(32) }); + expect(a.acquired).toBe(true); + if (!a.acquired) return; + const b = acquireCodexCliUpdateLease({ lockPath, pid: 2_222, isAlive: () => false }); + expect(b.acquired).toBe(true); + if (!b.acquired) return; + // The late release names the superseded token: B's live lease survives. + releaseCodexCliUpdateLease({ lockPath, pid: 1_111, token: a.record.token }); + const surviving = readFileSync(lockPath, "utf-8"); + expect(JSON.parse(surviving).token).toBe(b.record.token); + // A release naming only the pid still cannot touch a successor's record either: + // the token the caller never had is required inside compare-and-delete. + releaseCodexCliUpdateLease({ lockPath, pid: 1_111 }); + expect(JSON.parse(readFileSync(lockPath, "utf-8")).token).toBe(b.record.token); + }); + + test("a live owner is never reaped by age while its heartbeat is fresh", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); + const lockPath = join(dir, "codex-cli-update.lock"); + const held = acquireCodexCliUpdateLease({ lockPath, pid: 3_333 }); + expect(held.acquired).toBe(true); + if (!held.acquired) return; + // Age the record far past the bound but keep the heartbeat fresh: a long install + // must stay held, not be deliberately broken. + writeFileSync(lockPath, JSON.stringify({ + ...held.record, + createdAtMs: 0, + heartbeatAtMs: Date.now(), + })); + const contender = acquireCodexCliUpdateLease({ lockPath, pid: 4_444, isAlive: () => true }); + expect(contender.acquired).toBe(false); + if (!contender.acquired) { + expect(contender.reason).toBe("held"); + } + // The same live pid with a heartbeat that stopped is a wedged holder: reclaimable. + writeFileSync(lockPath, JSON.stringify({ + ...held.record, + createdAtMs: 0, + heartbeatAtMs: 0, + })); + const reaper = acquireCodexCliUpdateLease({ lockPath, pid: 4_444, isAlive: () => true }); + expect(reaper.acquired).toBe(true); + }); + test("startup observation reads a held lease and ignores a stale one", () => { const dir = mkdtempSync(join(tmpdir(), "ocx-update-lease-")); const lockPath = join(dir, "codex-cli-update.lock");