From 5eb0750bf9c56b6800a1f89e4bab86691a8886c6 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 15 Jul 2026 13:59:52 -0700 Subject: [PATCH] fix(codex): harden real-home routing cleanup and retries --- .../runtime-home-service.test.ts | 29 +++++- .../codex-accounts/runtime-home-service.ts | 18 ++-- .../codex-real-home-hook-install.test.ts | 80 ++++++++++++++++ .../codex/codex-real-home-hook-install.ts | 93 ++++++++++--------- src/main/codex/codex-real-home-path.ts | 20 ++++ 5 files changed, 185 insertions(+), 55 deletions(-) create mode 100644 src/main/codex/codex-real-home-path.ts diff --git a/src/main/codex-accounts/runtime-home-service.test.ts b/src/main/codex-accounts/runtime-home-service.test.ts index 3f3b10970f8..c3fbdea2bc1 100644 --- a/src/main/codex-accounts/runtime-home-service.test.ts +++ b/src/main/codex-accounts/runtime-home-service.test.ts @@ -991,14 +991,39 @@ describe('CodexRuntimeHomeService', () => { expect(existsSync(getRuntimeCodexHomePath())).toBe(true) }) - it('routes host system default to the real home (null) when the flag is ON', async () => { + it('routes host system default to the real home when the flag is ON', async () => { const store = createStore(createSettings({ codexSystemDefaultRealHomeEnabled: true })) const { CodexRuntimeHomeService } = await import('./runtime-home-service') const service = new CodexRuntimeHomeService(store as never) expect(service.isHostSystemDefaultRealHome()).toBe(true) expect(service.prepareForCodexLaunch()).toBeNull() - expect(service.prepareForRateLimitFetch()).toBeNull() + const previousCodexHome = process.env.CODEX_HOME + const previousOrcaCodexHome = process.env.ORCA_CODEX_HOME + process.env.CODEX_HOME = getRuntimeCodexHomePath() + process.env.ORCA_CODEX_HOME = getRuntimeCodexHomePath() + try { + // Background fetchers prefer ambient CODEX_HOME when passed null, so an + // explicit path proves nested Orca launches cannot poll the managed home. + expect(service.prepareForRateLimitFetch()).toBe(getSystemCodexHomePath()) + process.env.CODEX_HOME = getSystemCodexHomePath() + delete process.env.ORCA_CODEX_HOME + expect(service.isHostSystemDefaultRealHome()).toBe(true) + process.env.CODEX_HOME = join(testState.fakeHomeDir, 'user-owned-codex-home') + expect(service.isHostSystemDefaultRealHome()).toBe(false) + expect(service.prepareForRateLimitFetch()).toBe(getRuntimeCodexHomePath()) + } finally { + if (previousCodexHome === undefined) { + delete process.env.CODEX_HOME + } else { + process.env.CODEX_HOME = previousCodexHome + } + if (previousOrcaCodexHome === undefined) { + delete process.env.ORCA_CODEX_HOME + } else { + process.env.ORCA_CODEX_HOME = previousOrcaCodexHome + } + } }) it('keeps the managed home for a host MANAGED account even when the flag is ON', async () => { diff --git a/src/main/codex-accounts/runtime-home-service.ts b/src/main/codex-accounts/runtime-home-service.ts index c9990fcf9b0..e46bcee0307 100644 --- a/src/main/codex-accounts/runtime-home-service.ts +++ b/src/main/codex-accounts/runtime-home-service.ts @@ -61,6 +61,7 @@ import { } from './runtime-selection' import { getDefaultWslDistro, getWslHome } from '../wsl' import { isCodexSystemDefaultRealHomeEnabled } from '../codex/codex-real-home-flag' +import { hasCustomCodexHomeOverride } from '../codex/codex-real-home-path' type CodexAuthIdentity = { email: string | null @@ -210,13 +211,14 @@ export class CodexRuntimeHomeService { } // Why: real-home routing applies only to the host system-default selection - // (no managed account chosen for host) with the staged flag ON. Managed host - // accounts keep the isolated runtime home for hot-swap and token persistence. + // with the staged flag ON. Managed accounts keep hot-swap isolation; custom + // CODEX_HOMEs stay managed until phase 1 can track cleanup across old homes. isHostSystemDefaultRealHomeSelected(): boolean { const settings = this.store.getSettings() return ( isCodexSystemDefaultRealHomeEnabled(settings) && - normalizeCodexRuntimeSelection(settings).host === null + normalizeCodexRuntimeSelection(settings).host === null && + !hasCustomCodexHomeOverride() ) } @@ -288,11 +290,11 @@ export class CodexRuntimeHomeService { return syncedRuntimeHomePath ?? this.getWslSystemCodexHomePath(wslTarget) } if (this.isHostSystemDefaultRealHome()) { - // Why (flag ON, system default): read usage/auth from the user's own - // ~/.codex. Returning null makes the fetcher fall back to ~/.codex and - // its auth-presence gate check the real auth.json, so the background - // poller never spawns Codex against the managed home (the #5370 auth war). - return null + // Why: null lets the fetcher fall back to the main process's inherited + // CODEX_HOME before ~/.codex. Nested Orca launches can inherit the + // managed home, restarting the background OAuth conflict (#5370), so + // pin this non-interactive lane to the native home explicitly. + return getSystemCodexHomePath() } this.syncForCurrentSelection() syncSystemCodexResourcesIntoManagedHome() diff --git a/src/main/codex/codex-real-home-hook-install.test.ts b/src/main/codex/codex-real-home-hook-install.test.ts index f6f1f3a054d..db62745f246 100644 --- a/src/main/codex/codex-real-home-hook-install.test.ts +++ b/src/main/codex/codex-real-home-hook-install.test.ts @@ -4,6 +4,12 @@ import type * as NodeOs from 'node:os' import { tmpdir } from 'node:os' import { join } from 'node:path' import type { CodexManagedTrustGrantPlan } from './codex-hook-trust-grant' +import { + computeTrustKey, + readHookTrustEntries, + upsertHookTrustEntriesInContent, + type CodexTrustEntry +} from './config-toml-trust' const { homedirMock, grantMock } = vi.hoisted(() => ({ homedirMock: vi.fn<() => string>(), @@ -16,6 +22,7 @@ vi.mock('node:os', async () => { }) vi.mock('./codex-hook-trust-grant', () => ({ + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS: 300_000, grantManagedCodexHookTrust: grantMock })) @@ -34,6 +41,10 @@ function getRealHooksJsonPath(): string { return join(fakeHomeDir, '.codex', 'hooks.json') } +function getRealConfigTomlPath(): string { + return join(fakeHomeDir, '.codex', 'config.toml') +} + function readRealHooksJson(): { hooks?: Record [key: string]: unknown @@ -148,6 +159,21 @@ describe('ensureRealHomeCodexHookState (install)', () => { expect(existsSync(getRealHooksJsonPath())).toBe(false) }) + it('does no hook-file or grant work on repeated unsupported launches', () => { + grantUnavailable() + expect(ensureRealHomeCodexHookState({ hooksEnabled: true, userDataPath: userDataDir })).toBe( + 'unavailable' + ) + expect(existsSync(getRealHooksJsonPath())).toBe(false) + + expect(ensureRealHomeCodexHookState({ hooksEnabled: true, userDataPath: userDataDir })).toBe( + 'unavailable' + ) + + expect(grantMock).toHaveBeenCalledTimes(1) + expect(existsSync(getRealHooksJsonPath())).toBe(false) + }) + it('leaves an unparseable hooks.json untouched and keeps the managed lane', () => { writeFileSync(getRealHooksJsonPath(), '{not json', 'utf-8') @@ -205,4 +231,58 @@ describe('ensureRealHomeCodexHookState (opt-out sweep)', () => { expect(lane).toBe('removed') expect(existsSync(getRealHooksJsonPath())).toBe(false) }) + + it('removes only hash-proven Orca trust from a mixed hook group', () => { + const material = getCodexManagedHookInstallMaterial() + const userCommand = 'my-user-hook.sh' + writeFileSync( + getRealHooksJsonPath(), + `${JSON.stringify( + { + hooks: { + Stop: [ + { + hooks: [ + { type: 'command', command: userCommand }, + { type: 'command', command: material.command, timeout: 10 } + ] + } + ] + } + }, + null, + 2 + )}\n`, + 'utf-8' + ) + const entries: CodexTrustEntry[] = [ + { + sourcePath: getRealHooksJsonPath(), + eventLabel: 'stop', + groupIndex: 0, + handlerIndex: 0, + command: userCommand + }, + { + sourcePath: getRealHooksJsonPath(), + eventLabel: 'stop', + groupIndex: 0, + handlerIndex: 1, + command: material.command, + timeoutSec: 10 + } + ] + writeFileSync(getRealConfigTomlPath(), upsertHookTrustEntriesInContent('', entries), 'utf-8') + + expect(ensureRealHomeCodexHookState({ hooksEnabled: false, userDataPath: userDataDir })).toBe( + 'removed' + ) + + expect(readRealHooksJson().hooks?.Stop).toEqual([ + { hooks: [{ type: 'command', command: userCommand }] } + ]) + const trust = readHookTrustEntries(getRealConfigTomlPath()) + expect(trust.has(computeTrustKey(entries[0]!))).toBe(true) + expect(trust.has(computeTrustKey(entries[1]!))).toBe(false) + }) }) diff --git a/src/main/codex/codex-real-home-hook-install.ts b/src/main/codex/codex-real-home-hook-install.ts index 9dbec577037..42a24fd1838 100644 --- a/src/main/codex/codex-real-home-hook-install.ts +++ b/src/main/codex/codex-real-home-hook-install.ts @@ -1,17 +1,9 @@ -import { - copyFileSync, - existsSync, - mkdirSync, - readFileSync, - renameSync, - unlinkSync, - writeFileSync -} from 'node:fs' +import { copyFileSync, existsSync, mkdirSync, readFileSync, unlinkSync } from 'node:fs' import { join } from 'node:path' +import { writeFileAtomically } from '../codex-accounts/fs-utils' import { buildManagedCommandHook, createManagedCommandMatcher, - hookDefinitionHasManagedCommand, MANAGED_HOOK_TIMEOUT_SECONDS, readHooksJson, removeManagedCommands, @@ -20,12 +12,16 @@ import { type HookDefinition, type HooksConfig } from '../agent-hooks/installer-utils' -import { getSystemCodexHomePath } from './codex-home-paths' import { getCodexManagedScriptFileName } from './codex-hook-identity' -import { grantManagedCodexHookTrust } from './codex-hook-trust-grant' -import { removeCodexTrustGrantLedgerHome } from './codex-trust-grant-ledger' +import { + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS, + grantManagedCodexHookTrust, + type CodexTrustGrantFallbackReason +} from './codex-hook-trust-grant' +import { removeCodexManagedHookTrustEntries } from './codex-managed-trust-reconciliation' import { getCodexManagedHookInstallMaterial } from './hook-service' -import { computeTrustKey, removeHookTrustEntries, type CodexTrustEntry } from './config-toml-trust' +import { getSystemCodexHomePath } from './codex-home-paths' +import type { CodexTrustEntry } from './config-toml-trust' /** * Real-home Codex hook lane for the system-default selection (flag ON). @@ -42,6 +38,7 @@ import { computeTrustKey, removeHookTrustEntries, type CodexTrustEntry } from '. export type RealHomeCodexHookLane = 'pending' | 'installed' | 'unavailable' | 'removed' let currentLane: RealHomeCodexHookLane = 'pending' +let installRetryAfterMs = 0 export function getRealHomeCodexHookLane(): RealHomeCodexHookLane { return currentLane @@ -80,13 +77,24 @@ export function ensureRealHomeCodexHookState(args: { hooksEnabled: boolean userDataPath: string }): RealHomeCodexHookLane { + // Why: the grant client caches failed probes, but mutating and rolling back + // hooks.json before consulting it still adds synchronous work to every pane. + if (args.hooksEnabled && currentLane === 'unavailable' && Date.now() < installRetryAfterMs) { + return currentLane + } try { currentLane = args.hooksEnabled ? installRealHomeCodexHook(args.userDataPath) : sweepRealHomeCodexHook() + if (!args.hooksEnabled || currentLane === 'installed') { + installRetryAfterMs = 0 + } } catch (error) { console.warn('[codex-real-home-hooks] ensure failed; staying on managed lane:', error) currentLane = args.hooksEnabled ? 'unavailable' : currentLane + if (args.hooksEnabled) { + installRetryAfterMs = Date.now() + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS + } } return currentLane } @@ -99,6 +107,7 @@ function installRealHomeCodexHook(userDataPath: string): RealHomeCodexHookLane { // Why: an unparseable user file must never be clobbered; without a hook // entry the managed lane keeps status working for this host. console.warn('[codex-real-home-hooks] could not parse', hooksJsonPath, '- managed lane kept') + installRetryAfterMs = Date.now() + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS return 'unavailable' } @@ -161,12 +170,19 @@ function installRealHomeCodexHook(userDataPath: string): RealHomeCodexHookLane { // bytes and keep this host on the managed-home lane; the grant client // already logged the fallback reason. restoreRealHomeHooksJson(hooksJsonPath, previousRaw) + installRetryAfterMs = getInstallRetryAfterMs(grant.reason) console.warn( `[codex-real-home-hooks] trust grant unavailable (${grant.reason}); entry rolled back, managed lane kept` ) return 'unavailable' } +function getInstallRetryAfterMs(reason: CodexTrustGrantFallbackReason): number { + return reason === 'unsupported' || reason === 'unsupported-cached' || reason === 'disabled' + ? Number.POSITIVE_INFINITY + : Date.now() + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS +} + function sweepRealHomeCodexHook(): RealHomeCodexHookLane { const hooksJsonPath = getRealHomeHooksJsonPath() const config = readHooksJson(hooksJsonPath) @@ -176,33 +192,16 @@ function sweepRealHomeCodexHook(): RealHomeCodexHookLane { const isManagedCommand = createManagedCommandMatcher(getCodexManagedScriptFileName()) const material = getCodexManagedHookInstallMaterial() const nextHooks: Record = { ...config.hooks } - const removedTrustKeys: string[] = [] let removedAny = false for (const [eventName, definitions] of Object.entries(nextHooks)) { if (!Array.isArray(definitions)) { continue } - definitions.forEach((definition, groupIndex) => { - if (!hookDefinitionHasManagedCommand(definition, isManagedCommand)) { - return - } - const eventLabel = material.eventLabel[eventName as (typeof material.events)[number]] - if (!eventLabel) { - return - } - removedTrustKeys.push( - computeTrustKey({ - sourcePath: hooksJsonPath, - eventLabel, - groupIndex, - handlerIndex: 0, - command: material.command, - timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS - }) - ) - }) const cleaned = removeManagedCommands(definitions, isManagedCommand) - if (cleaned.length !== definitions.length) { + if ( + cleaned.length !== definitions.length || + cleaned.some((definition, index) => definition !== definitions[index]) + ) { removedAny = true } if (cleaned.length === 0) { @@ -215,17 +214,20 @@ function sweepRealHomeCodexHook(): RealHomeCodexHookLane { writeHooksJson(hooksJsonPath, { ...config, hooks: nextHooks } as HooksConfig) // Why: dead [hooks.state] blocks for a removed hook are Orca-owned records; // dropping them keeps the user's config.toml from accumulating orphans. - // Their removal never shifts user trust keys because Orca appends last. + // Verify ownership by the expected hash or grant ledger: stale/mixed hook + // groups must never make Orca delete a user's trust record at the same key. try { - removeHookTrustEntries(getRealHomeConfigTomlPath(), removedTrustKeys) + removeCodexManagedHookTrustEntries({ + tomlPath: getRealHomeConfigTomlPath(), + runtimeHomePath: getSystemCodexHomePath(), + sourcePath: hooksJsonPath, + command: material.command, + managedEventLabels: new Set(Object.values(material.eventLabel)), + timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS + }) } catch (error) { console.warn('[codex-real-home-hooks] failed to drop Orca trust entries:', error) } - try { - removeCodexTrustGrantLedgerHome(getSystemCodexHomePath()) - } catch { - // Ledger cleanup is bookkeeping only; the next grant rebuilds it. - } } return 'removed' } @@ -260,9 +262,9 @@ function restoreRealHomeHooksJson(hooksJsonPath: string, previousRaw: string | n } return } - const tmpPath = `${hooksJsonPath}.${process.pid}.rollback.tmp` - writeFileSync(tmpPath, previousRaw, 'utf-8') - renameSync(tmpPath, hooksJsonPath) + // Why: rollback is part of the safety boundary. Use the shared atomic + // writer so Windows file-lock retries and failed-temp cleanup are covered. + writeFileAtomically(hooksJsonPath, previousRaw) } catch (error) { console.warn('[codex-real-home-hooks] failed to roll back hooks.json:', error) } @@ -271,5 +273,6 @@ function restoreRealHomeHooksJson(hooksJsonPath: string, previousRaw: string | n export const _internals = { setLaneForTesting(lane: RealHomeCodexHookLane): void { currentLane = lane + installRetryAfterMs = 0 } } diff --git a/src/main/codex/codex-real-home-path.ts b/src/main/codex/codex-real-home-path.ts new file mode 100644 index 00000000000..ede4983615c --- /dev/null +++ b/src/main/codex/codex-real-home-path.ts @@ -0,0 +1,20 @@ +import { resolve } from 'node:path' +import { getSystemCodexHomePath } from './codex-home-paths' + +/** True when the user points Codex outside its standard native home. */ +export function hasCustomCodexHomeOverride(env: NodeJS.ProcessEnv = process.env): boolean { + const codexHome = env.CODEX_HOME?.trim() + const orcaCodexHome = env.ORCA_CODEX_HOME?.trim() + // Why: phase 1 owns only ~/.codex and can clean that path on downgrade. A + // custom home needs cross-home ownership tracking before Orca may mutate it. + return Boolean( + codexHome && + codexHome !== orcaCodexHome && + normalizePathForComparison(codexHome) !== normalizePathForComparison(getSystemCodexHomePath()) + ) +} + +function normalizePathForComparison(value: string): string { + const normalized = resolve(value) + return process.platform === 'win32' ? normalized.toLowerCase() : normalized +}