From 29e669680d60cf4e0d788906d984cf92f385e93c Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 6 Oct 2026 02:57:25 -0400 Subject: [PATCH] fix(codex): never write a Codex config.toml that Codex can't load, and approve both symlink spellings (#25741) * fix(codex): never write a hook approval Codex cannot load, and approve both symlink spellings - Refuse any hooks.state write into a Codex config.toml (upsert, move, remove, mirrored enabled state, SSH installer) that would turn a loadable file into one Codex cannot load; write nothing and surface the reason. - Read approvals written as dotted keys or inline tables. - Approve and remove Orca's ~/.codex hook under both the spelled and the resolved key when ~/.codex or HOME is a symlink; move user approvals under both keys. - Stale runtime trust cleanup no longer keeps an unexpected key whose conflicting duplicate tables read as no hash. * refactor(codex): pass every hooks.json spelling as one sourcePaths list * fix(codex): sweep retired-hook approvals under every key spelling; read literal-string trusted_hash --- src/main/codex/codex-hook-legacy-cleanup.ts | 25 +- src/main/codex/codex-hook-remote-install.ts | 7 +- .../codex/codex-hook-trust-cleanup.test.ts | 50 ++++ src/main/codex/codex-hook-trust-cleanup.ts | 13 +- src/main/codex/codex-hook-user-mirroring.ts | 4 +- .../codex-managed-trust-reconciliation.ts | 30 +- .../codex/codex-real-home-hook-install.ts | 33 ++- ...codex-real-home-hook-key-spellings.test.ts | 269 ++++++++++++++++++ src/main/codex/codex-real-home-hook-sweep.ts | 6 +- .../codex/codex-real-home-hook-withdrawal.ts | 3 +- src/main/codex/codex-real-home-hooks-json.ts | 16 ++ src/main/codex/codex-trust-identity.ts | 28 +- .../codex/codex-user-hook-trust-moves.test.ts | 2 +- src/main/codex/codex-user-hook-trust-moves.ts | 7 +- src/main/codex/config-toml-hook-trust-read.ts | 52 +++- .../codex/config-toml-trust-hook-read.test.ts | 25 ++ .../config-toml-trust-loadability.test.ts | 203 +++++++++++++ src/main/codex/config-toml-trust.ts | 72 ++++- .../hook-service-managed-install.test.ts | 15 + 19 files changed, 808 insertions(+), 52 deletions(-) create mode 100644 src/main/codex/codex-hook-trust-cleanup.test.ts create mode 100644 src/main/codex/codex-real-home-hook-key-spellings.test.ts create mode 100644 src/main/codex/config-toml-trust-loadability.test.ts diff --git a/src/main/codex/codex-hook-legacy-cleanup.ts b/src/main/codex/codex-hook-legacy-cleanup.ts index 36bdff8df62..c26f61a93f8 100644 --- a/src/main/codex/codex-hook-legacy-cleanup.ts +++ b/src/main/codex/codex-hook-legacy-cleanup.ts @@ -21,6 +21,7 @@ import { removeSelfComputedMatchingTrustEntries } from './codex-hook-trust-cleanup' import { runExclusivelyForCodexTrustConfig } from './codex-trust-config-mutation-queue' +import { getRealHomeHookKeySourcePaths } from './codex-real-home-hooks-json' import { mutateRealHomeHooksPreservingUserTrust } from './codex-user-hook-trust-moves' const LEGACY_ORCA_PROFILE_NAME = 'orca-agent-status' @@ -61,6 +62,9 @@ async function sweepLegacySystemManagedHooks(): Promise { return } + // Why every spelling: with a symlinked home, Codex may have approved the + // retired hook under its resolved key too. + const sourcePaths = getRealHomeHookKeySourcePaths() const nextHooks = { ...config.hooks } const trustEntries: CodexTrustEntry[] = [] let removedManagedHook = false @@ -68,15 +72,16 @@ async function sweepLegacySystemManagedHooks(): Promise { if (!Array.isArray(definitions)) { continue } - const eventTrustEntries = collectManagedTrustEntries( - legacyConfigPath, - eventName, - definitions, - isRetiredCodexHookCommand - ) - // Why: user hook configs can be large; avoid the argument limit from push(...entries). - for (const entry of eventTrustEntries) { - trustEntries.push(entry) + for (const sourcePath of sourcePaths) { + // Why: user hook configs can be large; avoid the argument limit from push(...entries). + for (const entry of collectManagedTrustEntries( + sourcePath, + eventName, + definitions, + isRetiredCodexHookCommand + )) { + trustEntries.push(entry) + } } const cleaned = removeManagedCommands(definitions, isRetiredCodexHookCommand) removedManagedHook ||= definitions.some((definition) => @@ -95,7 +100,7 @@ async function sweepLegacySystemManagedHooks(): Promise { // Remove only retired Orca hook entries and preserve other managers' metadata. const hooksWritePath = resolveHooksJsonWritePath(legacyConfigPath) mutateRealHomeHooksPreservingUserTrust({ - sourcePath: legacyConfigPath, + sourcePaths, tomlPath: getSystemCodexConfigTomlPath(), beforeHooks: config.hooks, afterHooks: nextHooks, diff --git a/src/main/codex/codex-hook-remote-install.ts b/src/main/codex/codex-hook-remote-install.ts index f3b929db9c8..dced65f6ef1 100644 --- a/src/main/codex/codex-hook-remote-install.ts +++ b/src/main/codex/codex-hook-remote-install.ts @@ -13,7 +13,11 @@ import { writeManagedScriptRemote, writeTextFileRemoteAtomic } from '../agent-hooks/installer-utils-remote' -import { upsertHookTrustEntriesInContent, type CodexTrustEntry } from './config-toml-trust' +import { + assertLoadableHookTrustConfig, + upsertHookTrustEntriesInContent, + type CodexTrustEntry +} from './config-toml-trust' import { CODEX_EVENTS, CODEX_EVENT_LABEL, @@ -108,6 +112,7 @@ export async function installCodexHooksRemote( const existingToml = existingTomlRaw ?? '' const updatedToml = upsertHookTrustEntriesInContent(existingToml, trustEntries) if (updatedToml !== existingToml) { + assertLoadableHookTrustConfig(remoteTomlPath, existingToml, updatedToml) await writeTextFileRemoteAtomic(sftp, remoteTomlPath, updatedToml) } } catch (error) { diff --git a/src/main/codex/codex-hook-trust-cleanup.test.ts b/src/main/codex/codex-hook-trust-cleanup.test.ts new file mode 100644 index 00000000000..8796d6c48c1 --- /dev/null +++ b/src/main/codex/codex-hook-trust-cleanup.test.ts @@ -0,0 +1,50 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { + computeTrustKey, + escapeTomlString, + readHookTrustEntries, + type CodexTrustEntry +} from './config-toml-trust' +import { removeStaleRuntimeHookTrustEntries } from './codex-hook-trust-cleanup' + +let dir: string +let tomlPath: string +let hooksPath: string + +function entry(command: string, groupIndex: number): CodexTrustEntry { + return { sourcePath: hooksPath, eventLabel: 'stop', groupIndex, handlerIndex: 0, command } +} + +beforeEach(() => { + // Why realpath: runtime keys are resolved, and the temp dir may sit under a symlink. + dir = realpathSync.native(mkdtempSync(join(tmpdir(), 'orca-codex-trust-cleanup-'))) + tomlPath = join(dir, 'config.toml') + hooksPath = join(dir, 'hooks.json') +}) + +afterEach(() => { + rmSync(dir, { recursive: true, force: true }) +}) + +describe('removeStaleRuntimeHookTrustEntries', () => { + it('removes an unexpected key whose duplicate tables disagree, so read as no hash', () => { + const expected = { ...entry('orca.sh', 0), trustedHash: 'sha256:orca' } + const staleKey = escapeTomlString(computeTrustKey(entry('gone.sh', 1))) + writeFileSync( + tomlPath, + `[hooks.state."${escapeTomlString(computeTrustKey(expected))}"]\ntrusted_hash = "sha256:orca"\n\n` + + `[hooks.state."${staleKey}"]\ntrusted_hash = "sha256:a"\n\n` + + `[hooks.state."${staleKey}"]\ntrusted_hash = "sha256:b"\n` + ) + + removeStaleRuntimeHookTrustEntries(tomlPath, hooksPath, [expected]) + + expect(readFileSync(tomlPath, 'utf-8')).not.toContain(staleKey) + expect(readHookTrustEntries(tomlPath).get(computeTrustKey(expected))?.trustedHash).toBe( + 'sha256:orca' + ) + }) +}) diff --git a/src/main/codex/codex-hook-trust-cleanup.ts b/src/main/codex/codex-hook-trust-cleanup.ts index cd64594d5af..b4225151470 100644 --- a/src/main/codex/codex-hook-trust-cleanup.ts +++ b/src/main/codex/codex-hook-trust-cleanup.ts @@ -95,7 +95,10 @@ export function removeStaleRuntimeHookTrustEntries( if (!parsed || !codexHookSourcePathsEqual(parsed.sourcePath, canonicalRuntimeHooksPath)) { continue } - if (expectedHashes.get(normalizeHookTrustKeyForLookup(key)) === state.trustedHash) { + const expectedHash = expectedHashes.get(normalizeHookTrustKeyForLookup(key)) + // Why a defined hash: conflicting duplicate tables read as no hash, and an + // unexpected key must not match that and survive. + if (expectedHash !== undefined && expectedHash === state.trustedHash) { continue } staleKeys.push(key) @@ -107,12 +110,12 @@ export function removeStaleRuntimeHookTrustEntries( export function removeSystemManagedHookTrustEntries( systemHomePath: string, - hooksJsonPath: string + sourcePaths: readonly [string, ...string[]] ): void { removeCodexManagedHookTrustEntries({ tomlPath: getSystemCodexConfigTomlPath(), runtimeHomePath: systemHomePath, - sourcePath: hooksJsonPath, + sourcePaths, command: getManagedCommand(getManagedScriptPath()), managedEventLabels: CODEX_MANAGED_EVENT_LABELS, timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS @@ -124,7 +127,7 @@ export function removeRuntimeManagedHookTrustEntries(configPath: string): void { removeCodexManagedHookTrustEntries({ tomlPath: getCodexConfigTomlPath(), runtimeHomePath: getOrcaManagedCodexHomePath(), - sourcePath: configPath, + sourcePaths: [configPath], command: getManagedCommand(getManagedScriptPath()), managedEventLabels: CODEX_MANAGED_EVENT_LABELS, timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS, @@ -143,7 +146,7 @@ export function removeWslRuntimeManagedHookTrustEntries( removeCodexManagedHookTrustEntries({ tomlPath: plan.tomlPath, runtimeHomePath: pathWin32.dirname(plan.tomlPath), - sourcePath: plan.trustConfigPath, + sourcePaths: [plan.trustConfigPath], command: wrapReadablePosixHookCommand(plan.commandScriptPath), managedEventLabels: CODEX_MANAGED_EVENT_LABELS, timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS diff --git a/src/main/codex/codex-hook-user-mirroring.ts b/src/main/codex/codex-hook-user-mirroring.ts index c257819cf09..8a79213b199 100644 --- a/src/main/codex/codex-hook-user-mirroring.ts +++ b/src/main/codex/codex-hook-user-mirroring.ts @@ -9,7 +9,7 @@ import { escapeTomlString, getCodexExplicitHomeHookSourcePath, parseTrustKey, - writeConfigAtomically, + writeLoadableHookTrustConfig, type CodexTrustEntry } from './config-toml-trust' import { createCodexHookTrustEntry, getCodexHookTrustSignature } from './codex-hook-identity' @@ -202,7 +202,7 @@ export function applyMirroredRuntimeUserHookTrustStates( updated = updated.replace(pattern, `$1${enabled}`) } if (updated !== existing) { - writeConfigAtomically(tomlPath, updated) + writeLoadableHookTrustConfig(tomlPath, existing, updated) } } diff --git a/src/main/codex/codex-managed-trust-reconciliation.ts b/src/main/codex/codex-managed-trust-reconciliation.ts index 3fdf8cd5478..50dbcf5cc84 100644 --- a/src/main/codex/codex-managed-trust-reconciliation.ts +++ b/src/main/codex/codex-managed-trust-reconciliation.ts @@ -58,7 +58,8 @@ function addLedgerRecognizedHashes( type CodexManagedHookTrustOwnershipOptions = { runtimeHomePath: string - sourcePath: string + /** hooks.json first, then other spellings Codex may key it by (same hash). */ + sourcePaths: readonly [string, ...string[]] command: string managedEventLabels: ReadonlySet timeoutSec: number @@ -71,17 +72,21 @@ function getCodexManagedHookTrustEntryKeys( options: CodexManagedHookTrustOwnershipOptions ): string[] { const ledgerHome = readCodexTrustGrantLedgerHomeForReconciliation(options.runtimeHomePath) + const [sourcePath, ...aliases] = options.sourcePaths const expectedSourcePath = options.sourceUsesExplicitCodexHome - ? getCodexExplicitHomeHookSourcePath(options.sourcePath) - : normalizeCodexHookSourcePath(options.sourcePath) + ? getCodexExplicitHomeHookSourcePath(sourcePath) + : normalizeCodexHookSourcePath(sourcePath) + const aliasSourcePaths = aliases.map(normalizeCodexHookSourcePath) const ownedKeys: string[] = [] for (const [key, state] of existingEntries) { const parts = parseTrustKey(key) - if ( - !parts || - !codexHookSourcePathsEqual(parts.sourcePath, expectedSourcePath) || - !options.managedEventLabels.has(parts.eventLabel) - ) { + if (!parts || !options.managedEventLabels.has(parts.eventLabel)) { + continue + } + const isAlias = aliasSourcePaths.some((alias) => + codexHookSourcePathsEqual(parts.sourcePath, alias) + ) + if (!isAlias && !codexHookSourcePathsEqual(parts.sourcePath, expectedSourcePath)) { continue } const expectedEntry: CodexTrustEntry = { @@ -97,6 +102,15 @@ function getCodexManagedHookTrustEntryKeys( computeTrustedHash({ ...expectedEntry, timeoutSec: undefined }) ]) addLedgerRecognizedHashes(recognizedHashes, [ledgerHome], key, expectedEntry) + if (isAlias) { + // Why: the ledger records Codex's grant under the primary spelling's key only. + addLedgerRecognizedHashes( + recognizedHashes, + [ledgerHome], + computeTrustKey(expectedEntry), + expectedEntry + ) + } if (state.trustedHash && recognizedHashes.has(state.trustedHash)) { ownedKeys.push(key) } diff --git a/src/main/codex/codex-real-home-hook-install.ts b/src/main/codex/codex-real-home-hook-install.ts index eea13bb8ada..981cc829401 100644 --- a/src/main/codex/codex-real-home-hook-install.ts +++ b/src/main/codex/codex-real-home-hook-install.ts @@ -9,8 +9,10 @@ import { assertHooksJsonGeneration, backupRealHomeHooksJsonOnce, getRealHomeConfigTomlPath, + getRealHomeHookKeySourcePaths, getRealHomeHooksJsonPath } from './codex-real-home-hooks-json' +import { upsertHookTrustEntries, type CodexTrustEntry } from './config-toml-trust' import { getCodexManagedScriptFileName } from './codex-hook-identity' import { CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS, @@ -212,6 +214,9 @@ async function settleApproval( console.warn('[codex-real-home-hooks] background trust grant failed:', error) } } + if (outcome?.lane === 'rpc' && readCodexHooksEnabled()) { + approveOtherRealHomeKeySpellings(outcome.entries) + } installRetryAfterMs = recordRealHomeApprovalOutcome(outcome) approval = null // Why from the settings: hooks turned off during the session must not read as @@ -268,7 +273,7 @@ async function installRealHomeCodexHook( if (plan.changed) { backupRealHomeHooksJsonOnce(userDataPath, previousRaw) mutateRealHomeHooksPreservingUserTrust({ - sourcePath: hooksJsonPath, + sourcePaths: getRealHomeHookKeySourcePaths(), tomlPath: getRealHomeConfigTomlPath(), beforeHooks: config.hooks ?? {}, afterHooks: plan.hooks, @@ -295,7 +300,9 @@ async function installRealHomeCodexHook( useDefaultCodexHome: true, background: true } - if (await findCurrentManagedCodexHookTrust(grantPlan)) { + const current = await findCurrentManagedCodexHookTrust(grantPlan) + if (current) { + approveOtherRealHomeKeySpellings(current) return { verdict: 'installed' } } return { @@ -304,6 +311,26 @@ async function installRealHomeCodexHook( } } +/** + * Codex's grant keys ~/.codex as spelled; a pane whose CODEX_HOME names a + * symlinked home keys it resolved. Codex's hash ignores the path, so it carries. + */ +function approveOtherRealHomeKeySpellings(granted: readonly CodexTrustEntry[]): void { + const [, ...otherSpellings] = getRealHomeHookKeySourcePaths() + if (otherSpellings.length === 0 || granted.length === 0) { + return + } + try { + upsertHookTrustEntries( + getRealHomeConfigTomlPath(), + otherSpellings.flatMap((sourcePath) => granted.map((entry) => ({ ...entry, sourcePath }))) + ) + } catch (error) { + // Why not a failure: the spelled key Codex wrote still approves default-home panes. + console.warn('[codex-real-home-hooks] could not approve the resolved ~/.codex key:', error) + } +} + /** * The user's explicit opt-out: strips Orca's entry and its trust from the real * ~/.codex. Joins the system lane an opt-out caller already holds. @@ -320,7 +347,7 @@ export async function removeRealHomeCodexHookForOptOut(): Promise ({ + homedirMock: vi.fn<() => string>(), + grantMock: vi.fn(), + findCurrentMock: vi.fn() +})) + +vi.mock('node:os', async () => { + const actual = await vi.importActual('node:os') + return { ...actual, homedir: homedirMock } +}) + +vi.mock('./codex-hook-trust-grant', () => ({ + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS: 300_000, + findCurrentManagedCodexHookTrust: findCurrentMock, + grantManagedCodexHookTrust: grantMock +})) + +import { + ensureRealHomeCodexHookState, + removeRealHomeCodexHookForOptOut, + _internals +} from './codex-real-home-hook-install' +import { getRealHomeHookKeySourcePaths } from './codex-real-home-hooks-json' +import { cleanupLegacyManagedHookRepresentations } from './codex-hook-legacy-cleanup' +import { getCodexHookTrustSignature } from './codex-hook-identity' +import { getCodexManagedHookInstallMaterial } from './hook-service' +import { writeCodexTrustGrantLedgerHome } from './codex-trust-grant-ledger' + +// Why this file: Codex keys ~/.codex/hooks.json as spelled on its default home +// and resolved when CODEX_HOME names it, so a symlinked home has two keys. + +let root: string +let home: string +let userDataDir: string +let previousUserDataPath: string | undefined + +const codexHome = (): string => join(home, '.codex') +const hooksPath = (): string => join(codexHome(), 'hooks.json') +const tomlPath = (): string => join(codexHome(), 'config.toml') +const codexHash = (entry: CodexTrustEntry): string => `sha256:codex-${entry.eventLabel}` + +function stopEntry(sourcePath: string, groupIndex = 0): CodexTrustEntry { + return { + sourcePath, + eventLabel: 'stop', + groupIndex, + handlerIndex: 0, + command: getCodexManagedHookInstallMaterial().command, + timeoutSec: 10 + } +} + +/** Stands in for Codex: approves the spelled keys it was asked about, and records the ledger. */ +function grantLikeCodex(): void { + grantMock.mockImplementation((plan: CodexManagedTrustGrantPlan) => { + const entries = plan.managedEntries.map((entry) => ({ + ...entry, + trustedHash: codexHash(entry) + })) + upsertHookTrustEntries(plan.tomlPath, entries) + writeCodexTrustGrantLedgerHome(plan.runtimeHomePath, { + binary: null, + entries: Object.fromEntries( + entries.map((entry) => [ + normalizeHookTrustKeyForLookup(computeTrustKey(entry)), + { signature: getCodexHookTrustSignature(entry), trustedHash: entry.trustedHash } + ]) + ) + }) + return { lane: 'rpc', entries } + }) +} + +async function ensureSettled(): Promise { + await ensureRealHomeCodexHookState({ + hooksEnabled: true, + userDataPath: userDataDir, + writePolicy: 'add-missing-only' + }) + return _internals.settledVerdictForTesting() +} + +function linkCodexHomeToDotfiles(): string { + const target = join(home, 'dotfiles-codex') + mkdirSync(target) + symlinkSync(target, codexHome(), process.platform === 'win32' ? 'junction' : 'dir') + return join(realpathSync.native(target), 'hooks.json') +} + +beforeEach(() => { + grantMock.mockReset() + findCurrentMock.mockReset() + findCurrentMock.mockResolvedValue(null) + // Why realpath: the temp dir itself may sit under a symlink (macOS /var), which + // would give every home in this file a second spelling. + root = realpathSync.native(mkdtempSync(join(tmpdir(), 'orca-real-home-spellings-'))) + home = join(root, 'home') + mkdirSync(home) + userDataDir = join(root, 'user-data') + mkdirSync(userDataDir) + previousUserDataPath = process.env.ORCA_USER_DATA_PATH + process.env.ORCA_USER_DATA_PATH = userDataDir + homedirMock.mockReturnValue(home) + _internals.resetForTesting('pending') +}) + +afterEach(() => { + rmSync(root, { recursive: true, force: true }) + if (previousUserDataPath === undefined) { + delete process.env.ORCA_USER_DATA_PATH + } else { + process.env.ORCA_USER_DATA_PATH = previousUserDataPath + } + vi.clearAllMocks() +}) + +describe('both spellings of a symlinked ~/.codex', () => { + it('has one key when nothing on the path is a symlink', () => { + mkdirSync(codexHome()) + + expect(getRealHomeHookKeySourcePaths()).toEqual([normalizeCodexHookSourcePath(hooksPath())]) + }) + + it('resolves the key through a symlinked HOME before ~/.codex exists', () => { + const linkedHome = join(root, 'linked-home') + symlinkSync(home, linkedHome, process.platform === 'win32' ? 'junction' : 'dir') + homedirMock.mockReturnValue(linkedHome) + + expect(getRealHomeHookKeySourcePaths()).toEqual([ + normalizeCodexHookSourcePath(join(linkedHome, '.codex', 'hooks.json')), + normalizeCodexHookSourcePath(join(home, '.codex', 'hooks.json')) + ]) + expect(getCodexExplicitHomeHookSourcePath(join(linkedHome, '.codex', 'hooks.json'))).toBe( + normalizeCodexHookSourcePath(join(home, '.codex', 'hooks.json')) + ) + }) + + it("copies Codex's approval to the resolved key, and the opt-out removes both", async () => { + const resolvedHooks = linkCodexHomeToDotfiles() + grantLikeCodex() + + expect(await ensureSettled()).toBe('installed') + + const spelled = stopEntry(hooksPath()) + const resolved = stopEntry(resolvedHooks) + const trust = readHookTrustEntries(tomlPath()) + expect(trust.get(computeTrustKey(spelled))?.trustedHash).toBe(codexHash(spelled)) + expect(trust.get(computeTrustKey(resolved))?.trustedHash).toBe(codexHash(spelled)) + + expect(await removeRealHomeCodexHookForOptOut()).toBe('removed') + + const after = readHookTrustEntries(tomlPath()) + expect(after.get(computeTrustKey(spelled))).toBeUndefined() + expect(after.get(computeTrustKey(resolved))).toBeUndefined() + }) + + it('copies the approval when an earlier grant is still current, with no session', async () => { + const resolvedHooks = linkCodexHomeToDotfiles() + findCurrentMock.mockImplementation(async (plan: CodexManagedTrustGrantPlan) => + plan.managedEntries.map((entry) => ({ ...entry, trustedHash: codexHash(entry) })) + ) + + expect(await ensureSettled()).toBe('installed') + + expect(grantMock).not.toHaveBeenCalled() + expect( + readHookTrustEntries(tomlPath()).get(computeTrustKey(stopEntry(resolvedHooks)))?.trustedHash + ).toBe(codexHash(stopEntry(resolvedHooks))) + }) + + it('moves a user approval under both keys when the opt-out shifts the hook', async () => { + const resolvedHooks = linkCodexHomeToDotfiles() + grantLikeCodex() + await ensureSettled() + const installed = JSON.parse(readFileSync(hooksPath(), 'utf-8')) + installed.hooks.Stop.push({ hooks: [{ type: 'command', command: 'after.sh' }] }) + writeFileSync(hooksPath(), `${JSON.stringify(installed, null, 2)}\n`) + const afterAt = (sourcePath: string, groupIndex: number): CodexTrustEntry => ({ + sourcePath, + eventLabel: 'stop', + groupIndex, + handlerIndex: 0, + command: 'after.sh' + }) + upsertHookTrustEntries(tomlPath(), [ + { ...afterAt(hooksPath(), 1), trustedHash: 'sha256:user-spelled' }, + { ...afterAt(resolvedHooks, 1), trustedHash: 'sha256:user-resolved' } + ]) + + expect(await removeRealHomeCodexHookForOptOut()).toBe('removed') + + const trust = readHookTrustEntries(tomlPath()) + expect(trust.get(computeTrustKey(afterAt(hooksPath(), 0)))?.trustedHash).toBe( + 'sha256:user-spelled' + ) + expect(trust.get(computeTrustKey(afterAt(resolvedHooks, 0)))?.trustedHash).toBe( + 'sha256:user-resolved' + ) + expect(trust.get(computeTrustKey(afterAt(resolvedHooks, 1)))).toBeUndefined() + }) + + it("sweeps a retired hook's approval under both keys", async () => { + const resolvedHooks = linkCodexHomeToDotfiles() + const retired = `/bin/sh "${join(home, 'old-user-data', 'agent-hooks', 'codex-hook.sh')}"` + writeFileSync( + hooksPath(), + `${JSON.stringify({ hooks: { Stop: [{ hooks: [{ type: 'command', command: retired }] }] } })}\n` + ) + const retiredAt = (sourcePath: string): CodexTrustEntry => ({ + sourcePath, + eventLabel: 'stop', + groupIndex: 0, + handlerIndex: 0, + command: retired + }) + upsertHookTrustEntries(tomlPath(), [retiredAt(hooksPath()), retiredAt(resolvedHooks)]) + + await cleanupLegacyManagedHookRepresentations() + + const trust = readHookTrustEntries(tomlPath()) + expect(trust.get(computeTrustKey(retiredAt(hooksPath())))).toBeUndefined() + expect(trust.get(computeTrustKey(retiredAt(resolvedHooks)))).toBeUndefined() + }) + + it('keeps the lane and the file when the copy would break config.toml', async () => { + linkCodexHomeToDotfiles() + // Why no write: Codex keeps its own approval in this inline form, which an + // appended [hooks.state."k"] table would turn into a file Codex cannot load. + const original = 'model = "m"\nhooks = { state = {} }\n' + writeFileSync(tomlPath(), original) + grantMock.mockImplementation((plan: CodexManagedTrustGrantPlan) => ({ + lane: 'rpc', + entries: plan.managedEntries.map((entry) => ({ ...entry, trustedHash: codexHash(entry) })) + })) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + expect(await ensureSettled()).toBe('installed') + + expect(readFileSync(tomlPath(), 'utf-8')).toBe(original) + expect(warn).toHaveBeenCalledWith( + '[codex-real-home-hooks] could not approve the resolved ~/.codex key:', + expect.objectContaining({ name: 'CodexConfigTomlRefusedError' }) + ) + }) +}) diff --git a/src/main/codex/codex-real-home-hook-sweep.ts b/src/main/codex/codex-real-home-hook-sweep.ts index 71560c1329e..ae52fcf0072 100644 --- a/src/main/codex/codex-real-home-hook-sweep.ts +++ b/src/main/codex/codex-real-home-hook-sweep.ts @@ -10,6 +10,7 @@ import { resolveHooksJsonWritePath } from '../agent-hooks/hook-config-write-path import { assertHooksJsonGeneration, getRealHomeConfigTomlPath, + getRealHomeHookKeySourcePaths, getRealHomeHooksJsonPath } from './codex-real-home-hooks-json' import { getCodexManagedScriptFileName } from './codex-hook-identity' @@ -55,8 +56,9 @@ export async function sweepRealHomeCodexHook(): Promise<'removed' | 'unavailable } if (removedAny) { const hooksWritePath = resolveHooksJsonWritePath(hooksJsonPath) + const sourcePaths = getRealHomeHookKeySourcePaths() mutateRealHomeHooksPreservingUserTrust({ - sourcePath: hooksJsonPath, + sourcePaths, tomlPath: getRealHomeConfigTomlPath(), beforeHooks: config.hooks, afterHooks: nextHooks, @@ -73,7 +75,7 @@ export async function sweepRealHomeCodexHook(): Promise<'removed' | 'unavailable removeCodexManagedHookTrustEntries({ tomlPath: getRealHomeConfigTomlPath(), runtimeHomePath: getSystemCodexHomePath(), - sourcePath: hooksJsonPath, + sourcePaths, command: material.command, managedEventLabels: new Set(Object.values(material.eventLabel)), timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS diff --git a/src/main/codex/codex-real-home-hook-withdrawal.ts b/src/main/codex/codex-real-home-hook-withdrawal.ts index 6b7d17a3413..746540c4955 100644 --- a/src/main/codex/codex-real-home-hook-withdrawal.ts +++ b/src/main/codex/codex-real-home-hook-withdrawal.ts @@ -10,6 +10,7 @@ import type { RealHomeCodexHookSlotWrite } from './codex-real-home-hook-entry-pl import { assertHooksJsonGeneration, getRealHomeConfigTomlPath, + getRealHomeHookKeySourcePaths, getRealHomeHooksJsonPath } from './codex-real-home-hooks-json' import { readHookTrustEntries } from './config-toml-trust' @@ -91,7 +92,7 @@ export function withdrawUntrustedRealHomeWrites( if (withdrew > 0) { // Why: a hook appended after this entry meanwhile moves up a slot; its trust moves with it. mutateRealHomeHooksPreservingUserTrust({ - sourcePath: hooksJsonPath, + sourcePaths: getRealHomeHookKeySourcePaths(), tomlPath: getRealHomeConfigTomlPath(), beforeHooks: config.hooks, afterHooks: nextHooks, diff --git a/src/main/codex/codex-real-home-hooks-json.ts b/src/main/codex/codex-real-home-hooks-json.ts index 3549ee2da50..b334825ea17 100644 --- a/src/main/codex/codex-real-home-hooks-json.ts +++ b/src/main/codex/codex-real-home-hooks-json.ts @@ -3,6 +3,10 @@ import { join } from 'node:path' import { writeFileAtomically } from '../codex-accounts/fs-utils' import { resolveHooksJsonWritePath } from '../agent-hooks/hook-config-write-path' import { getSystemCodexHomePath } from './codex-home-paths' +import { + getCodexExplicitHomeHookSourcePath, + normalizeCodexHookSourcePath +} from './config-toml-trust' /** The user's real `~/.codex` hook files, plus the guard and pristine backup * the real-home lane needs before it is allowed to mutate them. */ @@ -10,6 +14,18 @@ export function getRealHomeHooksJsonPath(): string { return join(getSystemCodexHomePath(), 'hooks.json') } +/** + * Every key Codex may give an entry in ~/.codex/hooks.json: as spelled when it + * runs on its default home, resolved when a pane's CODEX_HOME names it. They + * differ when ~/.codex or HOME is a symlink, and Orca approves under both. + */ +export function getRealHomeHookKeySourcePaths(): [string, ...string[]] { + const hooksJsonPath = getRealHomeHooksJsonPath() + const spelled = normalizeCodexHookSourcePath(hooksJsonPath) + const resolved = getCodexExplicitHomeHookSourcePath(hooksJsonPath) + return resolved === spelled ? [spelled] : [spelled, resolved] +} + export function getRealHomeConfigTomlPath(): string { return join(getSystemCodexHomePath(), 'config.toml') } diff --git a/src/main/codex/codex-trust-identity.ts b/src/main/codex/codex-trust-identity.ts index 8bfe130c585..2bef8966322 100644 --- a/src/main/codex/codex-trust-identity.ts +++ b/src/main/codex/codex-trust-identity.ts @@ -80,13 +80,27 @@ export function getExplicitHomeCodexHookSourcePath(sourcePath: string): string { if (process.platform !== 'win32' && isUnambiguousWindowsPath(sourcePath)) { return normalizeCodexTrustSourcePath(sourcePath) } - try { - // Why: hook discovery resolves the explicit home but keeps the hooks.json leaf logical. - return normalizeCodexTrustSourcePath( - join(realpathSync.native(dirname(sourcePath)), basename(sourcePath)) - ) - } catch { - return normalizeCodexTrustSourcePath(sourcePath) + // Why: hook discovery resolves the explicit home but keeps the hooks.json leaf logical. + return normalizeCodexTrustSourcePath( + join(resolveThroughExistingAncestor(dirname(sourcePath)), basename(sourcePath)) + ) +} + +// Why the nearest existing ancestor: a home not created yet still sits under a resolved HOME. +function resolveThroughExistingAncestor(path: string): string { + const missing: string[] = [] + let current = path + for (;;) { + try { + return join(realpathSync.native(current), ...missing.toReversed()) + } catch { + const parent = dirname(current) + if (parent === current) { + return path + } + missing.push(basename(current)) + current = parent + } } } diff --git a/src/main/codex/codex-user-hook-trust-moves.test.ts b/src/main/codex/codex-user-hook-trust-moves.test.ts index 60e86e2d1ef..78b48c48871 100644 --- a/src/main/codex/codex-user-hook-trust-moves.test.ts +++ b/src/main/codex/codex-user-hook-trust-moves.test.ts @@ -48,7 +48,7 @@ function mutate( after: Record ): void { mutateRealHomeHooksPreservingUserTrust({ - sourcePath: hooksPath, + sourcePaths: [hooksPath], tomlPath: configPath, beforeHooks: before, afterHooks: after, diff --git a/src/main/codex/codex-user-hook-trust-moves.ts b/src/main/codex/codex-user-hook-trust-moves.ts index 6182de8a439..16a7475d11e 100644 --- a/src/main/codex/codex-user-hook-trust-moves.ts +++ b/src/main/codex/codex-user-hook-trust-moves.ts @@ -68,13 +68,16 @@ export function getMovedCodexUserHookTrust( * its new key, verbatim. Needs no Codex session, so no removal waits on one. */ export function mutateRealHomeHooksPreservingUserTrust(args: { - sourcePath: string + /** Every spelling Codex may key this file by (as spelled, and resolved). */ + sourcePaths: readonly string[] tomlPath: string beforeHooks: HooksByEvent afterHooks: HooksByEvent writeHooks: () => void }): void { - const moves = getMovedCodexUserHookTrust(args.sourcePath, args.beforeHooks, args.afterHooks) + const moves = args.sourcePaths.flatMap((sourcePath) => + getMovedCodexUserHookTrust(sourcePath, args.beforeHooks, args.afterHooks) + ) args.writeHooks() try { moveHookTrustEntries(args.tomlPath, moves) diff --git a/src/main/codex/config-toml-hook-trust-read.ts b/src/main/codex/config-toml-hook-trust-read.ts index 690a2a8538a..200fc89d9e5 100644 --- a/src/main/codex/config-toml-hook-trust-read.ts +++ b/src/main/codex/config-toml-hook-trust-read.ts @@ -1,3 +1,5 @@ +import { parse as parseToml } from 'smol-toml' +import { isPlainObject } from '../agent-hooks/hooks-json-read' import type { CodexHookTrustState } from './config-toml-trust' import { normalizeCodexHookTrustLookupKey } from './codex-trust-identity' import { findAllHookTrustBlocks } from './config-toml-hook-trust-blocks' @@ -27,6 +29,44 @@ export class CodexHookTrustEntryMap extends Map { } export function readHookTrustContent(content: string): Map { + const result = readHookTrustTables(content) + // Why: approvals written as dotted keys or inline tables have no [hooks.state."k"] header. + for (const [key, state] of readParsedHookTrust(content)) { + if (!result.has(key)) { + result.set(key, state) + } + } + return result +} + +function readParsedHookTrust(content: string): [string, CodexHookTrustState][] { + let parsed: unknown + try { + parsed = parseToml(content) + } catch { + return [] + } + const hooks = isPlainObject(parsed) ? parsed.hooks : undefined + const state = isPlainObject(hooks) ? hooks.state : undefined + if (!isPlainObject(state)) { + return [] + } + return Object.entries(state).flatMap(([key, value]): [string, CodexHookTrustState][] => + isPlainObject(value) + ? [ + [ + key, + { + trustedHash: typeof value.trusted_hash === 'string' ? value.trusted_hash : undefined, + enabled: typeof value.enabled === 'boolean' ? value.enabled : undefined + } + ] + ] + : [] + ) +} + +function readHookTrustTables(content: string): CodexHookTrustEntryMap { const result = new CodexHookTrustEntryMap() const conflictingTrustedHashKeys = new Set() for (const block of findAllHookTrustBlocks(content)) { @@ -69,11 +109,15 @@ function readHookTrustBlockState(block: string): { const lineEnd = newlineIndex === -1 ? block.length : newlineIndex const line = block.slice(cursor, lineEnd).replace(/\r$/, '') if (isTomlStructuralLine(scanState)) { - const hashMatch = /^[ \t]*trusted_hash[ \t]*=[ \t]*"((?:[^"\\]|\\.)*)"[ \t]*(?:#.*)?$/.exec( - line - ) + // Why both forms: a literal-string hash must not read as "no approval". + const hashMatch = + /^[ \t]*trusted_hash[ \t]*=[ \t]*(?:"((?:[^"\\]|\\.)*)"|'([^'\r\n]*)')[ \t]*(?:#.*)?$/.exec( + line + ) if (hashMatch) { - trustedHashes.add(unescapeTomlBasicString(hashMatch[1]!)) + trustedHashes.add( + hashMatch[1] !== undefined ? unescapeTomlBasicString(hashMatch[1]) : hashMatch[2]! + ) } const enabledMatch = /^[ \t]*enabled[ \t]*=[ \t]*(true|false)[ \t]*(?:#.*)?$/.exec(line) if (enabledMatch) { diff --git a/src/main/codex/config-toml-trust-hook-read.test.ts b/src/main/codex/config-toml-trust-hook-read.test.ts index 466cb77f73d..b1bb584d70f 100644 --- a/src/main/codex/config-toml-trust-hook-read.test.ts +++ b/src/main/codex/config-toml-trust-hook-read.test.ts @@ -70,6 +70,31 @@ describe('readHookTrustEntries', () => { expect(readHookTrustEntries(configPath).get(key)?.trustedHash).toBeUndefined() }) + it('reads a literal-string trusted_hash', () => { + const key = '/x/hooks.json:stop:0:0' + writeFileSync(configPath, `[hooks.state."${key}"]\ntrusted_hash = 'sha256:LITERAL'\n`, 'utf-8') + + expect(readHookTrustEntries(configPath).get(key)?.trustedHash).toBe('sha256:LITERAL') + }) + + it('fails closed when a literal and a basic hash conflict', () => { + const key = '/x/hooks.json:stop:0:0' + writeFileSync( + configPath, + [ + `[hooks.state."${key}"]`, + "trusted_hash = 'sha256:USER'", + '', + `[hooks.state.'${key}']`, + 'trusted_hash = "sha256:ORCA"', + '' + ].join('\n'), + 'utf-8' + ) + + expect(readHookTrustEntries(configPath).get(key)?.trustedHash).toBeUndefined() + }) + it('ignores trust-looking fields inside multiline strings', () => { const key = '/x/hooks.json:stop:0:0' writeFileSync( diff --git a/src/main/codex/config-toml-trust-loadability.test.ts b/src/main/codex/config-toml-trust-loadability.test.ts new file mode 100644 index 00000000000..0d0334a5793 --- /dev/null +++ b/src/main/codex/config-toml-trust-loadability.test.ts @@ -0,0 +1,203 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import type { SFTPWrapper } from 'ssh2' +import type * as SmolToml from 'smol-toml' +import type * as InstallerUtilsRemote from '../agent-hooks/installer-utils-remote' + +const mocks = vi.hoisted(() => { + const state: { + // Why: removal and enabled-flag edits cannot break real TOML, so these cases + // make the parser reject the edited bytes to prove the write is still checked. + rejectParse: ((content: string) => boolean) | null + remoteFiles: Map + } = { rejectParse: null, remoteFiles: new Map() } + return state +}) + +vi.mock('smol-toml', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + parse: (content: string) => { + if (mocks.rejectParse?.(content)) { + throw new Error('simulated unloadable TOML') + } + return actual.parse(content) + } + } +}) + +vi.mock('../agent-hooks/installer-utils-remote', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + readHooksJsonRemote: async (_sftp: SFTPWrapper, path: string) => + JSON.parse(mocks.remoteFiles.get(path) ?? '{}'), + readTextFileRemote: async (_sftp: SFTPWrapper, path: string) => + mocks.remoteFiles.get(path) ?? null, + writeHooksJsonRemote: async (_sftp: SFTPWrapper, path: string, config: unknown) => { + mocks.remoteFiles.set(path, JSON.stringify(config)) + }, + writeManagedScriptRemote: async () => {}, + writeTextFileRemoteAtomic: async (_sftp: SFTPWrapper, path: string, content: string) => { + mocks.remoteFiles.set(path, content) + } + } +}) + +import { + computeTrustKey, + escapeTomlString, + isCodexConfigTomlRefusedError, + moveHookTrustEntries, + readHookTrustEntries, + removeHookTrustEntries, + upsertHookTrustEntries, + type CodexTrustEntry +} from './config-toml-trust' +import { applyMirroredRuntimeUserHookTrustStates } from './codex-hook-user-mirroring' +import { installCodexHooksRemote } from './codex-hook-remote-install' + +let dir: string +let tomlPath: string +let hooksPath: string + +function stopEntry(groupIndex = 0): CodexTrustEntry { + return { + sourcePath: hooksPath, + eventLabel: 'stop', + groupIndex, + handlerIndex: 0, + command: 'orca-hook.sh' + } +} + +function tomlKey(entry: CodexTrustEntry): string { + return escapeTomlString(computeTrustKey(entry)) +} + +function expectRefusal(write: () => void): void { + let thrown: unknown + try { + write() + } catch (error) { + thrown = error + } + expect(isCodexConfigTomlRefusedError(thrown)).toBe(true) +} + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'orca-codex-toml-loadability-')) + tomlPath = join(dir, 'config.toml') + hooksPath = join(dir, 'hooks.json') + mocks.rejectParse = null + mocks.remoteFiles.clear() +}) + +afterEach(() => { + rmSync(dir, { recursive: true, force: true }) +}) + +describe('hook approval writes never break a config.toml Codex can load', () => { + it.each([ + ['an inline hooks.state table', '[hooks]\nstate = { "x:stop:0:0" = { trusted_hash = "u" } }\n'], + ['an inline hooks table', 'hooks = { state = {} }\n'], + [ + "a dotted approval for Orca's own key", + (): string => `hooks.state."${tomlKey(stopEntry())}".trusted_hash = "sha256:user"\n` + ] + ])('upsert refuses and leaves the file byte-identical: %s', (_case, content) => { + const original = `model = "m"\n${typeof content === 'string' ? content : content()}` + writeFileSync(tomlPath, original) + + expectRefusal(() => upsertHookTrustEntries(tomlPath, [stopEntry()])) + + expect(readFileSync(tomlPath, 'utf-8')).toBe(original) + }) + + it('upsert may still repair a file Codex already cannot load', () => { + writeFileSync(tomlPath, 'model = \n') + + upsertHookTrustEntries(tomlPath, [stopEntry()]) + + expect(readFileSync(tomlPath, 'utf-8')).toContain('trusted_hash') + }) + + it('a move refuses to land on a key the user approved as dotted keys', () => { + const original = + `hooks.state."${tomlKey(stopEntry(0))}".trusted_hash = "sha256:user"\n\n` + + `[hooks.state."${tomlKey(stopEntry(1))}"]\ntrusted_hash = "sha256:moved"\n` + writeFileSync(tomlPath, original) + + expectRefusal(() => + moveHookTrustEntries(tomlPath, [ + { oldKey: computeTrustKey(stopEntry(1)), newKey: computeTrustKey(stopEntry(0)) } + ]) + ) + + expect(readFileSync(tomlPath, 'utf-8')).toBe(original) + }) + + it('a removal is checked before it is written', () => { + upsertHookTrustEntries(tomlPath, [stopEntry(0), { ...stopEntry(1), command: 'user.sh' }]) + const original = readFileSync(tomlPath, 'utf-8') + mocks.rejectParse = (content) => !content.includes(computeTrustKey(stopEntry(0))) + + expectRefusal(() => removeHookTrustEntries(tomlPath, [computeTrustKey(stopEntry(0))])) + + expect(readFileSync(tomlPath, 'utf-8')).toBe(original) + }) + + it("mirroring a user hook's enabled state is checked before it is written", () => { + upsertHookTrustEntries(tomlPath, [{ ...stopEntry(), enabled: true }]) + const original = readFileSync(tomlPath, 'utf-8') + mocks.rejectParse = (content) => content.includes('enabled = false') + + expectRefusal(() => + applyMirroredRuntimeUserHookTrustStates(tomlPath, [{ entry: stopEntry(), enabled: false }]) + ) + + expect(readFileSync(tomlPath, 'utf-8')).toBe(original) + }) + + it('the SSH installer reports the refusal and leaves the remote config.toml untouched', async () => { + const remoteToml = '/home/u/.codex/config.toml' + const original = 'model = "m"\nhooks = { state = {} }\n' + mocks.remoteFiles.set(remoteToml, original) + mocks.remoteFiles.set('/home/u/.codex/hooks.json', '{"hooks":{}}') + + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mocked remote helpers never touch the SFTP handle. + const status = await installCodexHooksRemote({} as SFTPWrapper, '/home/u') + + expect(status).toMatchObject({ + state: 'error', + detail: expect.stringContaining('defines hook approvals in a form Orca cannot add to') + }) + expect(mocks.remoteFiles.get(remoteToml)).toBe(original) + }) +}) + +describe('reading approvals Codex or the user wrote without a table header', () => { + it('reads an approval written as dotted keys', () => { + writeFileSync(tomlPath, `hooks.state."${tomlKey(stopEntry())}".trusted_hash = "sha256:user"\n`) + + expect(readHookTrustEntries(tomlPath).get(computeTrustKey(stopEntry()))).toEqual({ + trustedHash: 'sha256:user', + enabled: undefined + }) + }) + + it('reads an approval written as an inline table', () => { + writeFileSync( + tomlPath, + `[hooks]\nstate = { "${tomlKey(stopEntry())}" = { trusted_hash = "sha256:user", enabled = false } }\n` + ) + + expect(readHookTrustEntries(tomlPath).get(computeTrustKey(stopEntry()))).toEqual({ + trustedHash: 'sha256:user', + enabled: false + }) + }) +}) diff --git a/src/main/codex/config-toml-trust.ts b/src/main/codex/config-toml-trust.ts index 181a0540b6b..87d89e1af64 100644 --- a/src/main/codex/config-toml-trust.ts +++ b/src/main/codex/config-toml-trust.ts @@ -1,4 +1,5 @@ import { existsSync, readFileSync } from 'node:fs' +import { parse as parseToml } from 'smol-toml' import { codexTrustSourcePathsEqual, computeCodexTrustedHash, @@ -103,7 +104,11 @@ export function parseTrustKey(key: string): { return parseCodexTrustKey(key) } -// Why: trust edits preserve unrelated bytes instead of reserializing the user's config. +/** + * Upserts hook approvals, preserving unrelated bytes instead of reserializing + * the user's config. Throws CodexConfigTomlRefusedError, writing nothing, when + * Codex could not load the result; see writeLoadableHookTrustConfig. + */ export function upsertHookTrustEntries( configPath: string, entries: readonly CodexTrustEntry[] @@ -111,7 +116,59 @@ export function upsertHookTrustEntries( const existing = readTomlForMutation(configPath) const updated = upsertHookTrustEntriesInContent(existing, entries) if (updated !== existing) { - writeConfigAtomically(configPath, updated) + writeLoadableHookTrustConfig(configPath, existing, updated) + } +} + +/** Thrown instead of writing a config.toml that Codex could no longer load. */ +export class CodexConfigTomlRefusedError extends Error { + constructor(message: string) { + super(message) + this.name = 'CodexConfigTomlRefusedError' + } +} + +export function isCodexConfigTomlRefusedError( + error: unknown +): error is CodexConfigTomlRefusedError { + return error instanceof Error && error.name === 'CodexConfigTomlRefusedError' +} + +/** + * Every hooks.state write goes through here. A user's inline + * `hooks.state = {...}` or dotted `hooks.state."k".trusted_hash` key cannot take + * an appended `[hooks.state."k"]` table, and Codex refuses to start with a + * config.toml it cannot load, so a write that would break a loadable file is + * refused instead. One that is already broken may still be repaired. + */ +export function writeLoadableHookTrustConfig( + configPath: string, + previous: string, + contents: string +): void { + assertLoadableHookTrustConfig(configPath, previous, contents) + writeConfigAtomically(configPath, contents) +} + +/** Throws CodexConfigTomlRefusedError when `contents` would break a `previous` Codex could load. */ +export function assertLoadableHookTrustConfig( + configPath: string, + previous: string, + contents: string +): void { + if (isLoadableToml(previous) && !isLoadableToml(contents)) { + throw new CodexConfigTomlRefusedError( + `${configPath} defines hook approvals in a form Orca cannot add to without breaking it` + ) + } +} + +function isLoadableToml(content: string): boolean { + try { + parseToml(stripLeadingBom(content)) + return true + } catch { + return false } } @@ -126,7 +183,7 @@ export function moveHookTrustEntries( const existing = readTomlForMutation(configPath) const updated = moveHookTrustContent(existing, moves) if (updated !== existing) { - writeConfigAtomically(configPath, updated) + writeLoadableHookTrustConfig(configPath, existing, updated) } } @@ -181,7 +238,7 @@ export function removeHookTrustEntries(configPath: string, keys: readonly string const existing = readTomlFile(configPath) const updated = removeHookTrustEntriesFromContent(existing, keys) if (updated !== existing) { - writeConfigAtomically(configPath, updated) + writeLoadableHookTrustConfig(configPath, existing, updated) } } @@ -212,6 +269,9 @@ function readTomlForMutation(configPath: string): string { } function readTomlFile(configPath: string): string { - const raw = readFileSync(configPath, 'utf-8') - return raw.charCodeAt(0) === 0xfeff ? raw.slice(1) : raw + return stripLeadingBom(readFileSync(configPath, 'utf-8')) +} + +function stripLeadingBom(content: string): string { + return content.charCodeAt(0) === 0xfeff ? content.slice(1) : content } diff --git a/src/main/codex/hook-service-managed-install.test.ts b/src/main/codex/hook-service-managed-install.test.ts index eedee438a65..9ec6ed8ce6f 100644 --- a/src/main/codex/hook-service-managed-install.test.ts +++ b/src/main/codex/hook-service-managed-install.test.ts @@ -129,6 +129,21 @@ describe('CodexHookService', () => { expect(trustConfig).toContain(':permission_request:0:0') }) + it('reports, instead of writing, approvals a mirrored inline hooks.state cannot take', async () => { + const systemCodexHome = join(homes.tmpHome, '.codex') + mkdirSync(systemCodexHome, { recursive: true }) + writeFileSync(join(systemCodexHome, 'config.toml'), 'hooks = { state = {} }\n', 'utf-8') + + const status = await new CodexHookService().install() + + expect(status).toMatchObject({ + state: 'error', + detail: expect.stringContaining('defines hook approvals in a form Orca cannot add to') + }) + const managedToml = join(homes.userDataDir, 'codex-runtime-home', 'home', 'config.toml') + expect(readFileSync(managedToml, 'utf-8')).not.toContain('[hooks.state.') + }) + it('installs managed hooks + trust into a per-account self-contained home, not the shared mirror', async () => { const systemCodexHome = join(homes.tmpHome, '.codex') mkdirSync(systemCodexHome, { recursive: true })