From 572b2fa7b1deea99ebec08b6efd9f5cc5e3d9f89 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 23 Aug 2026 12:00:53 -0700 Subject: [PATCH] test(codex): exercise live baseline read denial --- src/main/codex/codex-config-mirror.ts | 8 +-- src/main/codex/config-settings-baseline.ts | 10 ++-- src/main/codex/config-settings-promotion.ts | 15 ++--- src/main/codex/config-sync-stall.ts | 6 +- src/main/codex/config-toml-trust.ts | 10 ++-- ...hared-state-survives-a-failed-read.test.ts | 60 +++++++++++-------- 6 files changed, 55 insertions(+), 54 deletions(-) diff --git a/src/main/codex/codex-config-mirror.ts b/src/main/codex/codex-config-mirror.ts index dcb51c814da..f3d96a1064c 100644 --- a/src/main/codex/codex-config-mirror.ts +++ b/src/main/codex/codex-config-mirror.ts @@ -160,10 +160,10 @@ function syncSystemConfigIntoManagedCodexHomeUnsafe( ): CodexConfigMirrorResult { const systemConfigPath = join(systemHomePath, 'config.toml') const runtimeConfigPath = join(runtimeHomePath, 'config.toml') - // Why: `existsSync` answered `false` for a locked file exactly as for an absent - // one, so a held handle on the RUNTIME config read as "no runtime config yet" - // and the fresh-mirror branch below overwrote it wholesale. Neither side may - // be acted on unless it was actually observed. + // Why: `existsSync` collapses an indeterminate probe into the same `false` as + // absence, so a transiently unavailable RUNTIME config could reach the fresh + // mirror after the path recovered. Neither side may be acted on unless it was + // actually observed. const systemConfigObservation = observeAgentStateFile(systemConfigPath) if (systemConfigObservation.kind === 'indeterminate') { return { status: 'refused-indeterminate', error: systemConfigObservation.error } diff --git a/src/main/codex/config-settings-baseline.ts b/src/main/codex/config-settings-baseline.ts index cee0cc0c97c..9879720d802 100644 --- a/src/main/codex/config-settings-baseline.ts +++ b/src/main/codex/config-settings-baseline.ts @@ -24,12 +24,10 @@ type StoredSettingsBaseline = { } /** - * Why callers need three answers, not two: the baseline records what Orca last - * mirrored, so `null` drives "there is nothing to compare against, bootstrap - * one". For a file that merely could not be READ, bootstrapping overwrites the - * record of a change the user made inside Codex, and the next mirror pass then - * writes the old value back over their edit. Absent and unparseable still map - * to `absent` — rebuilding those is the intent. + * Why callers need three answers, not two: without a readable baseline, + * promotion cannot distinguish an in-Codex edit from Orca's last mirror. An + * unreadable baseline must stall that mirror; absent and unparseable still map + * to `absent` because rebuilding those is the intent. */ export type CodexSettingsBaselineObservation = | { kind: 'present'; baseline: CodexSettingsBaseline } diff --git a/src/main/codex/config-settings-promotion.ts b/src/main/codex/config-settings-promotion.ts index a8a9c2e541b..9ff7383291d 100644 --- a/src/main/codex/config-settings-promotion.ts +++ b/src/main/codex/config-settings-promotion.ts @@ -163,13 +163,6 @@ export function snapshotCodexRuntimeSettingsBaseline( runtimeHomePath = getOrcaManagedCodexHomePath(), conflicts: ReadonlyMap = new Map() ): void { - // Why: overwriting a baseline that could not be read records whatever the - // runtime config says now as "Orca wrote this", so an edit the user made - // inside Codex since the last pass is reclassified and never promoted again. - // Absent and malformed are still rebuilt — that is what this function is for. - if (observeCodexSettingsBaseline(runtimeHomePath).kind === 'indeterminate') { - return - } try { const runtimeTomlPath = join(runtimeHomePath, 'config.toml') // Why: record an empty baseline even for a missing runtime config, so Codex's first write still diffs and promotes. @@ -274,10 +267,10 @@ function promoteCodexRuntimeSettingsToSystemUnsafe( const writeTarget = resolvePromotionWriteTarget(systemTomlPath) // Why: a dangling symlink may target an unmade dir tree; create its real parent so the atomic temp write has a home. mkdirSync(dirname(writeTarget.path), { recursive: true, mode: 0o700 }) - // Why: this is the user's real ~/.codex/config.toml, and `existsSync` reading - // `false` for a locked file sent it down the reconstruct branch below, which - // replaces the canonical config with settings derived from Orca's runtime - // copy. One read replaces the old existsSync + read pair and its TOCTOU gap. + // Why: this is the user's real ~/.codex/config.toml, and an indeterminate + // existence probe sent it down the reconstruct branch below, which replaces + // the canonical config with settings derived from Orca's runtime copy. One + // read replaces the old existsSync + read pair and its TOCTOU gap. // The indeterminate arm is a backstop rather than the live guard: an // unreadable system config already refused in readPromotedSettingValues, // because `writeTarget.path` always resolves to the same file as diff --git a/src/main/codex/config-sync-stall.ts b/src/main/codex/config-sync-stall.ts index 1dad7cfb080..44e385c6471 100644 --- a/src/main/codex/config-sync-stall.ts +++ b/src/main/codex/config-sync-stall.ts @@ -27,9 +27,9 @@ export function getCodexConfigSyncStatus( const runtimeConfigPath = join(homes.runtimeHomePath, 'config.toml') // Why: a stall only withholds settings once a managed runtime config exists; // without one the mirror seeds it and there is nothing yet to fall behind. - // But `existsSync` said "no runtime config" for one that was merely locked, - // and this function then reported `synced` while the mirror was refusing — - // telling the user their edits had been applied when nothing had run. + // But `existsSync` never proved the config readable, so this function could + // report `synced` while the mirror refused its content read — telling the + // user their edits had been applied when nothing had run. const runtimeConfigObservation = observeAgentStateFile(runtimeConfigPath) if (runtimeConfigObservation.kind === 'absent') { return { state: 'synced', reason: null, systemConfigPath } diff --git a/src/main/codex/config-toml-trust.ts b/src/main/codex/config-toml-trust.ts index a50f01c3343..d641c664f24 100644 --- a/src/main/codex/config-toml-trust.ts +++ b/src/main/codex/config-toml-trust.ts @@ -338,12 +338,10 @@ export function upsertHookTrustEntries( configPath: string, entries: readonly CodexTrustEntry[] ): void { - // Why: `existsSync` answered `false` for a locked config exactly as for an - // absent one, so `existing` became '' and the upsert below rebuilt the file - // from the trust entries alone — replacing the user's model, provider, MCP - // servers, approvals and comments with a trust-only stub. Only a definitive - // absence may seed from empty; every caller in hook-service already turns a - // throw here into "trust entries could not be written. Run /hooks in Codex". + // Why: `existsSync` collapses an indeterminate probe into the same `false` as + // absence. If the path recovers before the write, rebuilding from '' replaces + // the user's model, provider, MCP servers, approvals and comments with a + // trust-only stub. Only a definitive absence may seed from empty. const observation = observe(() => readTomlFile(configPath)) if (observation.kind === 'indeterminate') { throw observation.error diff --git a/src/main/codex/sta-4823-shared-state-survives-a-failed-read.test.ts b/src/main/codex/sta-4823-shared-state-survives-a-failed-read.test.ts index 445a994ca73..e9fce52bb41 100644 --- a/src/main/codex/sta-4823-shared-state-survives-a-failed-read.test.ts +++ b/src/main/codex/sta-4823-shared-state-survives-a-failed-read.test.ts @@ -3,11 +3,12 @@ import type * as NodeFs from 'node:fs' import type * as NodeOs from 'node:os' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { syncSystemConfigIntoManagedCodexHome } from './codex-config-mirror' import type { CodexTrustEntry } from './config-toml-trust' // STA-4823: six shared Codex state files were rebuilt, erased or reported as -// healthy after a read that had merely failed. Every one is reachable on the -// host lane; the WSL-specific half of the catalogue stays in STA-4606. +// healthy after a read that had merely failed. Host permission failures drive +// four; the other guards cover transient transport and observation races. const denials = vi.hoisted(() => { const state = { @@ -18,9 +19,8 @@ const denials = vi.hoisted(() => { state.paths.add(path) }, /** - * Content reads fail but `existsSync` still says the file is there — the - * shape a Windows sharing violation takes, as distinct from `deny`, which - * models an ACL denial where `existsSync` itself reports false. + * Content reads fail but `existsSync` and metadata reads still succeed — + * the measured host permission/sharing-denial shape. */ denyReads(path: string): void { state.readOnlyPaths.add(path) @@ -33,8 +33,9 @@ const denials = vi.hoisted(() => { * produce this. `chmod 000` on macOS and `icacls /deny (R)` on Windows both * leave `existsSync` TRUE, leave `stat`/`lstat` succeeding, and fail only * the content read (EACCES / EPERM errno -4048). The shape modelled here is - * the UNC / `\\wsl$` transport, where an unreachable distro reports errno - * `UNKNOWN` at every level and `existsSync` folds that to false. + * a transient transport failure: a UNC / `\\wsl$` probe reports errno + * `UNKNOWN`, `existsSync` folds it to false, and the transport recovers + * before the following write. * * The distinction decides what the guard is worth: under a permission denial * the pre-fix code already failed safe, because `existsSync` was true so it @@ -46,6 +47,8 @@ const denials = vi.hoisted(() => { }, release(path: string): void { state.paths.delete(path) + state.readOnlyPaths.delete(path) + state.existenceDeniedPaths.delete(path) }, reset(): void { state.paths.clear() @@ -268,7 +271,7 @@ describe('STA-4823 D30 — an unreadable trust-grant ledger must not be rewritte it('skips the write instead of dropping every other home', () => { const ledgerPath = seedLedgerWithAnotherHome() const before = realFs.readFileSync(ledgerPath, 'utf-8') - denials.deny(ledgerPath) + denials.denyReads(ledgerPath) writeCodexTrustGrantLedgerHome( runtimeHomePath, @@ -317,7 +320,7 @@ describe('STA-4823 D33 — an unreadable hooks.json is not an empty hooks.json', }) }) -describe('STA-4823 D26 — an unreadable settings baseline must not be overwritten', () => { +describe('STA-4823 D26 — an unreadable settings baseline must stall the mirror', () => { function seedBaseline(): string { realFs.writeFileSync( baselinePath(), @@ -327,20 +330,29 @@ describe('STA-4823 D26 — an unreadable settings baseline must not be overwritt return realFs.readFileSync(baselinePath(), 'utf-8') } - it('leaves the record alone so the pending edit can still be promoted', () => { + it('preserves the pending runtime edit until the baseline can be read', () => { const before = seedBaseline() - realFs.writeFileSync( - join(runtimeHomePath, 'config.toml'), - 'model = "edited-in-codex"\n', - 'utf-8' - ) - denials.deny(baselinePath()) + const runtimeConfigPath = join(runtimeHomePath, 'config.toml') + realFs.writeFileSync(runtimeConfigPath, 'model = "edited-in-codex"\n', 'utf-8') + realFs.writeFileSync(join(systemHome(), 'config.toml'), 'model = "old-model"\n', 'utf-8') + denials.denyReads(baselinePath()) + + syncSystemConfigIntoManagedCodexHome({ runtimeHomePath, systemHomePath: systemHome() }) + + // Before the fix the failed baseline read became "no baseline", promotion + // returned an empty plan, and the mirror wrote the old system value over + // the pending in-Codex edit. + expect(realFs.readFileSync(runtimeConfigPath, 'utf-8')).toContain('edited-in-codex') + expect(realFs.readFileSync(baselinePath(), 'utf-8')).toBe(before) + }) + + it('does not replace a baseline after an indeterminate existence probe', () => { + const before = seedBaseline() + realFs.writeFileSync(join(runtimeHomePath, 'config.toml'), 'model = "edited"\n', 'utf-8') + denials.denyExistence(baselinePath()) snapshotCodexRuntimeSettingsBaseline(runtimeHomePath) - // Before the fix this recorded "edited-in-codex" as Orca's own write, so - // the user's in-Codex change was never promoted and the next mirror pass - // wrote the old value back over it. expect(realFs.readFileSync(baselinePath(), 'utf-8')).toBe(before) }) @@ -396,9 +408,9 @@ describe('STA-4823 D15 — the sync status must not report synced while the mirr const status = getCodexConfigSyncStatus({ runtimeHomePath, systemHomePath: systemHome() }) - // Before the fix existsSync reported the locked runtime config as absent - // and this returned `synced` — telling the user their edits had been - // applied while nothing had run. + // Before the fix existsSync collapsed this failed observation into absence + // and returned `synced` — telling the user their edits had been applied + // while nothing had run. expect(status).toMatchObject({ state: 'stalled', reason: 'managed-home-unavailable' }) }) @@ -446,7 +458,7 @@ describe('STA-4823 D31 — an unreadable pane registry must not erase every attr it('does not persist an empty registry over the real one', () => { seedTwoAttributedPanes() const before = realFs.readFileSync(registryPath(), 'utf-8') - denials.deny(registryPath()) + denials.denyReads(registryPath()) paneRegistry.recordCodexPaneAccount('pty-3', { selectionKey: 'host', @@ -461,7 +473,7 @@ describe('STA-4823 D31 — an unreadable pane registry must not erase every attr it('does not cache the erasure, so attribution returns when the file does', () => { seedTwoAttributedPanes() - denials.deny(registryPath()) + denials.denyReads(registryPath()) expect(paneRegistry.getCodexPaneAccount(PANE)).toBeNull()