From c7de2121ed6457fb2c556f7361490bbfa9d95d3e Mon Sep 17 00:00:00 2001 From: agentHits <140916359+agentHits@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:29:33 -0400 Subject: [PATCH 1/2] fix(cli): update restart tolerates an installed-but-unloaded service --- src/cli/update-restart-home.ts | 13 +++++++++++-- src/cli/update-restart.ts | 19 ++++++++++++++++++- structure/runtime.md | 2 +- tests/cli/cli-update-restart-home.test.ts | 8 ++++++++ tests/cli/cli-update-restart.test.ts | 9 ++++++++- 5 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/cli/update-restart-home.ts b/src/cli/update-restart-home.ts index bec52b47ef5..68f7a9ac6bb 100644 --- a/src/cli/update-restart-home.ts +++ b/src/cli/update-restart-home.ts @@ -17,11 +17,20 @@ function physicalDirectory(path: string): UpdateRestartHome["config"] { return { path: canonical, dev: stat.dev, ino: stat.ino }; } -/** Only an unclaimed standalone home is eligible for this narrow update path. */ +/** + * Only an unclaimed standalone home is eligible for this narrow update path. + * + * A present-but-unowned service record does not block: liveness is decided at + * eligibility by the standalone classifier, which refuses a live supervisor. + * An ownership claim or an unreadable record still fails closed, and any + * mid-flight record change is caught by assertUpdateRestartHome comparing the + * full snapshot. + */ export function readUpdateRestartHome(): UpdateRestartHome { const state = resolveServiceState(); const owner = resolveServiceOwnership(); - if (state.kind !== "none" || owner.kind !== "none") throw new Error("update_restart_owner_unverified"); + if (owner.kind !== "none") throw new Error("update_restart_owner_unverified"); + if (state.kind === "unknown") throw new Error("update_restart_owner_unverified"); return { config: physicalDirectory(getConfigDir()), codex: physicalDirectory(currentCodexHome()), revision: owner.revision }; } diff --git a/src/cli/update-restart.ts b/src/cli/update-restart.ts index 1a59308dc84..d3e50ece0a4 100644 --- a/src/cli/update-restart.ts +++ b/src/cli/update-restart.ts @@ -180,6 +180,23 @@ export function describeUpdateRestartFailure(code: string, reason?: UpdateRestar } } +/** + * A service blocks the update path only while it is actually supervising. + * An installed-but-unloaded record cannot race the stop/launch sequence; + * refusing on mere presence forces a downtime stop/start for a proxy the + * service does not manage. A failed probe fails closed (blocks). + */ +export function serviceSupervisionBlocksRestart( + diagnose: () => { installed: boolean; running: boolean } = diagnoseService, +): boolean { + try { + const service = diagnose(); + return service.installed && service.running; + } catch { + return true; + } +} + function standalone(target: UpdateRestartCandidate["target"]): boolean { try { assertUpdateRestartConfiguration(target.hostname ?? ""); } catch { throw new UpdateRestartEligibilityError("configuration_changed"); } @@ -192,7 +209,7 @@ function standalone(target: UpdateRestartCandidate["target"]): boolean { if (!isIP(host) || target.source !== "runtime") throw new UpdateRestartEligibilityError("unverifiable_ancestry"); const command = readProcessCommandLine(target.pid); if (!command || !isOcxStartCommandLine(command)) throw new UpdateRestartEligibilityError("unverifiable_ancestry"); - if (diagnoseService().installed) throw new UpdateRestartEligibilityError("service"); + if (serviceSupervisionBlocksRestart()) throw new UpdateRestartEligibilityError("service"); const manager = inspectGuardedManagerTarget(target.pid, target.port); if (manager.kind !== "absent") throw new UpdateRestartEligibilityError(manager.kind === "bound" ? "service" : "unverifiable_ancestry"); let parent: string; diff --git a/structure/runtime.md b/structure/runtime.md index a4ba9577f82..091ab8dab81 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -142,7 +142,7 @@ does not perform OAuth, and runtime credential resolution rereads the owned sour | `src/providers/api-key-selection-capture.ts` | Pure request-owned snapshot of the configured key entry, reference, and revision. The router and stateful selection module share this leaf with type-only dependencies; `api-key-selection.ts` retains the compatibility export and owns persisted selection changes and route resolution. | | `src/types.ts` | Shared config, parsed request, adapter, and event types. | | `src/reasoning-effort.ts` | Codex reasoning-level definitions (`low`/`medium`/`high`/`xhigh`), per-model effort mapping, and catalog effort sanitization. | -| `src/cli/tray-proxy.ts` | Proxy restart coordination. Replacement re-observation captures one clock value for both its remaining budget and absolute end, so the observation window never extends the caller deadline. CLI proxy update restart: `src/cli/system-restart-client.ts` preserves a private newer-CLI candidate with its original runtime record and physical home snapshot. `src/cli/update-restart.ts` accepts only known versions and positively classified standalone POSIX targets: PID-1 parent, no installed/active manager, no ownership record, no client/sibling role. It holds the existing ownership mutation lease through stop and spawn and revalidates the captured target before stop. `src/cli/update-restart-transport.ts` proves identity and sends stop on one direct TCP connection, refuses reconnects and requires complete successful teardown acknowledgement. It never signals a PID or reclaims a port. Confirmed predecessor exit, runtime-record removal, port availability and definitive endpoint absence precede the one launch. The executable that launch spawns must pass the shared `REAL_BUN_MIN_BYTES` gate: an incomplete runtime refuses before any stop byte, and after a confirmed stop the transaction waits for it within the deadline, then rechecks deadline and physical home synchronously before spawning. `src/cli/update-restart-child.ts` strips inherited lease delegation, acquires its own lease before CLI preflight and checks the frozen physical homes, ownership revision, CLI version, endpoint and deadline before startup, each bind and publication. It keeps custody through rollback; its deadline ends an unfinished child. `src/cli/update-restart-home.ts` owns physical-home and unclaimed-state checks. Success requires that launched child PID, fresh attestation and the exact known version; all incomplete outcomes terminate without generic restart recovery. Supervised, Windows and uncertain cases refuse this update path. | +| `src/cli/tray-proxy.ts` | Proxy restart coordination. Replacement re-observation captures one clock value for both its remaining budget and absolute end, so the observation window never extends the caller deadline. CLI proxy update restart: `src/cli/system-restart-client.ts` preserves a private newer-CLI candidate with its original runtime record and physical home snapshot. `src/cli/update-restart.ts` accepts only known versions and positively classified standalone POSIX targets: PID-1 parent, no running manager (an installed-but-unloaded service record does not block), no ownership claim, no client/sibling role. It holds the existing ownership mutation lease through stop and spawn and revalidates the captured target before stop. `src/cli/update-restart-transport.ts` proves identity and sends stop on one direct TCP connection, refuses reconnects and requires complete successful teardown acknowledgement. It never signals a PID or reclaims a port. Confirmed predecessor exit, runtime-record removal, port availability and definitive endpoint absence precede the one launch. The executable that launch spawns must pass the shared `REAL_BUN_MIN_BYTES` gate: an incomplete runtime refuses before any stop byte, and after a confirmed stop the transaction waits for it within the deadline, then rechecks deadline and physical home synchronously before spawning. `src/cli/update-restart-child.ts` strips inherited lease delegation, acquires its own lease before CLI preflight and checks the frozen physical homes, ownership revision, CLI version, endpoint and deadline before startup, each bind and publication. It keeps custody through rollback; its deadline ends an unfinished child. `src/cli/update-restart-home.ts` owns physical-home and unclaimed-state checks. Success requires that launched child PID, fresh attestation and the exact known version; all incomplete outcomes terminate without generic restart recovery. Supervised, Windows and uncertain cases refuse this update path. | | `src/codex/shim.ts` | Codex autostart facade: dispatches Unix private PATH overlays to `src/codex/shim-overlay.ts` and retains Windows in-place script wrapping. `src/codex/shim-migration.ts` owns explicit legacy Unix restoration; `src/codex/shim-diagnostics.ts` owns diagnosis/status, distinguishes runnable from active overlays and states the unhealthy verdict for a damaged in-place (Windows) wrapper. `src/codex/shim-templates.ts` uses `src/lib/self-launch-argv.ts` to invoke standalone executables with `ensure` directly in Unix, CMD and PowerShell wrappers, without a virtual source entrypoint; management subcommands bypass startup even after value-taking global flags. Discovery resolves fnm’s temporary `fnm_multishells` directory to the durable installation directory through `src/codex/shim-path-resolution.ts`, taking the directory realpath while retaining the command basename; unresolved temporary entries refuse installation. The injected path flavour governs join and separator. The shared PATH scanner follows npm/fnm symlinks and POSIX target execute access, skips non-executable and special-file entries, and reads only the first 16 KiB of regular files through a descriptor with nonblocking POSIX open and a post-open type check. An unreadable executable still shadows later commands. Overlay activation requires the selected executable to identify the recorded private wrapper or the same file through an alias; another marker-bearing shim does not prove activation. Shell functions/aliases and empty PATH components are outside this scanner’s guarantee. `src/cli/codex-shim-readiness.ts` retains connect’s advisory exit policy. `tests/codex-integration/codex-shim-path-readiness.test.ts` covers POSIX shell lookup parity, FIFO replacement, symlinks and bounded reads. Windows retains PATHEXT lookup without POSIX execute-access checks. | | `src/service.ts` | OS service manager (macOS launchd, Linux systemd, Windows schtasks and native WinSW): always-on proxy with crash restart. The scheduler wrapper logs transactional-backup recovery without interpolating backup names. Facade over the `src/service/` leaves — `src/service/launchd.ts`, `src/service/systemd.ts`, `src/service/windows-ops.ts`, `src/service/windows-scheduler.ts`, `src/service/windows-taskxml.ts`, `src/service/state.ts`, `src/service/guards.ts`, `src/service/health.ts`, `src/service/repair.ts`, `src/service/orchestration.ts`, `src/service/diagnostics.ts`, `src/service/cli.ts`. Service definitions filter shell-local multishell PATH entries, including WinSW XML generated during install and repair, and recorded temporary launchers are replaced during repair; changed launchd definitions are reloaded. Codex-home ownership accepts either the recorded path or the same existing physical directory so path aliases remain compatible across upgrades. Elevated Task Scheduler repair stages bounded payloads; the unelevated launcher pins every namespace ancestor and payload with non-reparse handles that deny write/delete sharing on the payload and delete sharing on each ancestor until UAC processing exits. On Windows, `src/service/windows-process-priority.ts` raises the process that binds the proxy to ABOVE_NORMAL just before `startServer` (best-effort; `OCX_DISABLE_PRIORITY_BOOST=1` opts out), because the NORMAL task priority (`4`) still let a saturated host hold `/healthz` past the 750 ms CLI liveness ceiling; spawned children fall back to NORMAL since Windows does not inherit ABOVE_NORMAL. The Task Scheduler batch wrapper (`src/service/windows-taskxml.ts`) checks bundled Bun against `REAL_BUN_MIN_BYTES` before each launch: npm's placeholder, or a file that vanished before its size was read, is logged and re-checked every five seconds instead of executed, while a Bun path missing at the exist check keeps the backup-restore then exit-3 path ([wrapper contract](ops/docs-and-release.md#windows-service-wrapper-and-incomplete-updates)). | diff --git a/tests/cli/cli-update-restart-home.test.ts b/tests/cli/cli-update-restart-home.test.ts index 5f73747fd7a..ef8f897da2a 100644 --- a/tests/cli/cli-update-restart-home.test.ts +++ b/tests/cli/cli-update-restart-home.test.ts @@ -35,6 +35,14 @@ test("unreadable ownership state fails closed", () => { const h = home(); writeFileSync(join(h.config, "service-state.json"), "{bad"); expect(() => readUpdateRestartHome()).toThrow(); }); +test("a present-but-unowned service record does not block the home snapshot", () => { + const h = home(); + writeFileSync(join(h.config, "service-state.json"), JSON.stringify({ + version: 2, codexHome: h.codex, opencodexHome: h.config, backend: "scheduler", revision: 8, + })); + const captured = readUpdateRestartHome(); + assertUpdateRestartHome(captured); +}); test("busy child lease leaves config bytes, permissions and files unchanged", () => { const h = home(); const configPath = join(h.config, "config.json"); diff --git a/tests/cli/cli-update-restart.test.ts b/tests/cli/cli-update-restart.test.ts index 257466c0532..88e12e36966 100644 --- a/tests/cli/cli-update-restart.test.ts +++ b/tests/cli/cli-update-restart.test.ts @@ -1,5 +1,5 @@ import { describe, expect, spyOn, test } from "bun:test"; -import { describeUpdateRestartFailure, runUpdateRestart, UpdateRestartEligibilityError, type UpdateRestartIo } from "../../src/cli/update-restart"; +import { describeUpdateRestartFailure, runUpdateRestart, serviceSupervisionBlocksRestart, UpdateRestartEligibilityError, type UpdateRestartIo } from "../../src/cli/update-restart"; import type { UpdateRestartCandidate } from "../../src/cli/update-restart-candidate"; import { reportRestartFailure } from "../../src/cli/restart-failure"; import type { LiveProxy } from "../../src/server/proxy-liveness"; @@ -226,3 +226,10 @@ test("an unverified update home names status and the owning service recovery", ( expect(text).toContain("ocx status"); expect(text).toContain("ocx service restart"); expect(text).toContain("No changes were made"); } finally { errors.mockRestore(); } }); + +test("service supervision blocks only a live supervisor, never a stale record", () => { + expect(serviceSupervisionBlocksRestart(() => ({ installed: true, running: true }))).toBe(true); + expect(serviceSupervisionBlocksRestart(() => ({ installed: true, running: false }))).toBe(false); + expect(serviceSupervisionBlocksRestart(() => ({ installed: false, running: false }))).toBe(false); + expect(serviceSupervisionBlocksRestart(() => { throw new Error("probe down"); })).toBe(true); +}); From 7c60bf9b04bebbaae96b11a65abfd3d6605aab2b Mon Sep 17 00:00:00 2001 From: agentHits <140916359+agentHits@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:29:37 -0400 Subject: [PATCH 2/2] fix(codex): reconcile warns only when journal restore rewrites state --- src/codex/journal.ts | 28 +++++++++++++++---- tests/codex-integration/codex-journal.test.ts | 24 ++++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/codex/journal.ts b/src/codex/journal.ts index b05f10476e9..c6f4dbc84f0 100644 --- a/src/codex/journal.ts +++ b/src/codex/journal.ts @@ -85,6 +85,9 @@ export interface RestoreJournalResult { ownershipRefusal?: CodexHomeOwnerRefusalReason; configRestored: boolean; profileRestored: boolean; + /** True only when restore actually rewrote bytes (not when state was already original). */ + configRewritten: boolean; + profileRewritten: boolean; configChanged: boolean; profileChanged: boolean; complete: boolean; @@ -299,12 +302,13 @@ export function restoreJournalState(): RestoreJournalResult { try { assertCodexHomeOwner(CODEX_HOME); } catch (error) { if (!(error instanceof CodexHomeOwnerRefusal)) throw error; - return { configRestored: false, profileRestored: false, configChanged: false, profileChanged: false, - complete: false, unverified: true, ownershipRefusal: error.reason }; + return { configRestored: false, profileRestored: false, configRewritten: false, profileRewritten: false, + configChanged: false, profileChanged: false, complete: false, unverified: true, ownershipRefusal: error.reason }; } const journal = readJournal(); if (!journal) { - return { configRestored: false, profileRestored: false, configChanged: false, profileChanged: false, complete: false, unverified: false }; + return { configRestored: false, profileRestored: false, configRewritten: false, profileRewritten: false, + configChanged: false, profileChanged: false, complete: false, unverified: false }; } const currentConfig = existsSync(CODEX_CONFIG_PATH) ? readFileSync(CODEX_CONFIG_PATH, "utf-8") : null; const currentProfile = existsSync(CODEX_PROFILE_PATH) ? readFileSync(CODEX_PROFILE_PATH, "utf-8") : null; @@ -316,6 +320,8 @@ export function restoreJournalState(): RestoreJournalResult { return { configRestored: comparison.configAlreadyOriginal, profileRestored: comparison.profileAlreadyOriginal, + configRewritten: false, + profileRewritten: false, configChanged: !configUnchanged, profileChanged: !profileUnchanged, complete: false, @@ -326,14 +332,18 @@ export function restoreJournalState(): RestoreJournalResult { assertCodexHomeOwner(CODEX_HOME); let configRestored = comparison.configAlreadyOriginal; let profileRestored = comparison.profileAlreadyOriginal; + let configRewritten = false; + let profileRewritten = false; if (configUnchanged && !configRestored) { atomicWriteFile(CODEX_CONFIG_PATH, comparison.originalConfig); configRestored = true; + configRewritten = true; } if (profileUnchanged && !profileRestored) { if (comparison.originalProfile !== null) { atomicWriteFile(CODEX_PROFILE_PATH, comparison.originalProfile); profileRestored = true; + profileRewritten = true; } else if (existsSync(CODEX_PROFILE_PATH)) { // "There was no profile before, so remove the one we generated." Claiming success // without checking is how a caller ends up deleting the journal, reporting a clean @@ -343,6 +353,7 @@ export function restoreJournalState(): RestoreJournalResult { try { unlinkSync(CODEX_PROFILE_PATH); profileRestored = true; + profileRewritten = true; } catch (error) { profileRestored = (error as NodeJS.ErrnoException).code === "ENOENT"; } @@ -355,6 +366,8 @@ export function restoreJournalState(): RestoreJournalResult { return { configRestored, profileRestored, + configRewritten, + profileRewritten, configChanged: !configUnchanged, profileChanged: !profileUnchanged, complete, @@ -388,7 +401,9 @@ export function reconcileJournal(options: ReconcileJournalOptions = {}): boolean console.error("⚠️ Codex journal recovery was not verified; current configuration files and the journal were preserved."); return false; } - if (!restored.configRestored && !restored.profileRestored) return false; + // Warn only when restore actually rewrote state; an already-original + // snapshot still cleans itself up but has nothing to report. + if (!restored.configRewritten && !restored.profileRewritten) return false; console.error(`⚠️ Uncommitted or mismatched client routing (${owner.apiKeyId}) was restored from the Codex journal.`); return true; } @@ -406,7 +421,10 @@ export function reconcileJournal(options: ReconcileJournalOptions = {}): boolean console.error("⚠️ Codex journal recovery was not verified; current configuration files and the journal were preserved."); return false; } - if (!restored.configRestored && !restored.profileRestored) return false; + // Warn only when restore actually rewrote state. A stale journal whose + // artifacts are already original still cleans itself up (complete removes + // it) but has nothing to report — claiming a restore then cries wolf. + if (!restored.configRewritten && !restored.profileRewritten) return false; console.error(`⚠️ Previous session (PID ${pid}) did not shut down cleanly. Codex state restored from journal.`); return true; } diff --git a/tests/codex-integration/codex-journal.test.ts b/tests/codex-integration/codex-journal.test.ts index c19e999e976..c7f5b7382ff 100644 --- a/tests/codex-integration/codex-journal.test.ts +++ b/tests/codex-integration/codex-journal.test.ts @@ -304,6 +304,30 @@ describe("codex-journal", () => { expect(existsSync(journalPath)).toBe(false); }); + test("reconcileJournal stays silent when a dead-owner journal needs no rewrite", () => { + const journalPath = join(testDir, "opencodex-journal.json"); + const original = "# original config\nmodel_provider = \"openai\"\n"; + writeFileSync(join(testDir, "config.toml"), original, "utf8"); + writeFileSync(journalPath, JSON.stringify({ + version: 1, + originalConfig: Buffer.from(original).toString("base64"), + originalProfile: null, + pid: 999999, + timestamp: new Date().toISOString(), + }), "utf8"); + + const r = runScript(testDir, ` + const { reconcileJournal } = require("./src/codex/journal"); + const result = reconcileJournal(); + console.log(JSON.stringify({ restored: result })); + `); + expect(r.status).toBe(0); + expect(JSON.parse(r.stdout).restored).toBe(false); + expect(readFileSync(join(testDir, "config.toml"), "utf8")).toBe(original); + expect(existsSync(journalPath)).toBe(false); + expect(r.stderr).not.toContain("did not shut down cleanly"); + }); + test("reconcileJournal preserves corrupt JSON without authorizing recovery", () => { const journalPath = join(testDir, "opencodex-journal.json"); writeFileSync(journalPath, "NOT VALID JSON{{{", "utf8");