diff --git a/config/tsconfig.cli.json b/config/tsconfig.cli.json index 3ebd71aa41f..11020ab0206 100644 --- a/config/tsconfig.cli.json +++ b/config/tsconfig.cli.json @@ -5,6 +5,7 @@ "../src/shared/**/*", "../src/main/agent-hooks/hook-stdin-contract.ts", "../src/main/agent-hooks/hook-config-write-path.ts", + "../src/main/agent-hooks/hooks-json-read.ts", "../src/main/agent-hooks/installer-utils.ts", "../src/main/agent-hooks/installer-utils-remote.ts", "../src/main/agent-hooks/managed-agent-hook-controls.ts", diff --git a/src/main/agent-hooks/hooks-json-read.ts b/src/main/agent-hooks/hooks-json-read.ts new file mode 100644 index 00000000000..c8536fad0b4 --- /dev/null +++ b/src/main/agent-hooks/hooks-json-read.ts @@ -0,0 +1,37 @@ +import { existsSync, readFileSync } from 'node:fs' +import type { HooksConfig } from './installer-utils' + +export function isPlainObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +export type HooksJsonSnapshot = { + /** null when the file does not exist or could not be read. */ + raw: string | null + config: HooksConfig | null +} + +// Why: generation guards abort a mutation when the file no longer matches the +// bytes it was derived from; the raw snapshot and the parse must come from one +// read or a concurrent save can slip between them unnoticed. +export function readHooksJsonWithRaw(configPath: string): HooksJsonSnapshot { + if (!existsSync(configPath)) { + return { raw: null, config: {} } + } + let raw: string + try { + raw = readFileSync(configPath, 'utf-8') + } catch { + return { raw: null, config: null } + } + try { + const parsed = JSON.parse(raw) + return { raw, config: isPlainObject(parsed) ? parsed : null } + } catch { + return { raw, config: null } + } +} + +export function readHooksJson(configPath: string): HooksConfig | null { + return readHooksJsonWithRaw(configPath).config +} diff --git a/src/main/agent-hooks/installer-utils.test.ts b/src/main/agent-hooks/installer-utils.test.ts index 7fac297ae86..466792009c7 100644 --- a/src/main/agent-hooks/installer-utils.test.ts +++ b/src/main/agent-hooks/installer-utils.test.ts @@ -25,6 +25,7 @@ import { wrapPosixHookCommand, wrapWindowsCmdHookCommand, wrapWindowsGitBashHookCommand, + readHooksJsonWithRaw, wrapWindowsHookCommand, writeManagedScript, writeHooksJson, @@ -43,6 +44,28 @@ afterEach(() => { rmSync(tmpDir, { recursive: true, force: true }) }) +describe('readHooksJsonWithRaw', () => { + it('returns the parsed config together with the exact bytes it came from', () => { + const contents = '{"hooks": {"Stop": []}, "custom": 1}\n' + writeFileSync(configPath, contents, 'utf-8') + + expect(readHooksJsonWithRaw(configPath)).toEqual({ + raw: contents, + config: { hooks: { Stop: [] }, custom: 1 } + }) + }) + + it('reports a missing file as an empty config with no raw bytes', () => { + expect(readHooksJsonWithRaw(configPath)).toEqual({ raw: null, config: {} }) + }) + + it('keeps the raw bytes when the contents are not a JSON object', () => { + writeFileSync(configPath, 'not json\n', 'utf-8') + + expect(readHooksJsonWithRaw(configPath)).toEqual({ raw: 'not json\n', config: null }) + }) +}) + describe('writeHooksJson', () => { it('updates a symlink target without replacing the hook config link', () => { const targetPath = join(tmpDir, 'dotfiles-hooks.json') diff --git a/src/main/agent-hooks/installer-utils.ts b/src/main/agent-hooks/installer-utils.ts index 63cadabe534..c73e1536e4f 100644 --- a/src/main/agent-hooks/installer-utils.ts +++ b/src/main/agent-hooks/installer-utils.ts @@ -63,22 +63,12 @@ export function buildManagedCommandDefinition(command: string): HookDefinition { return { command, timeout: MANAGED_HOOK_TIMEOUT_SECONDS } } -export function isPlainObject(value: unknown): value is Record { - return typeof value === 'object' && value !== null && !Array.isArray(value) -} - -export function readHooksJson(configPath: string): HooksConfig | null { - if (!existsSync(configPath)) { - return {} - } - - try { - const parsed = JSON.parse(readFileSync(configPath, 'utf-8')) - return isPlainObject(parsed) ? parsed : null - } catch { - return null - } -} +export { + isPlainObject, + readHooksJson, + readHooksJsonWithRaw, + type HooksJsonSnapshot +} from './hooks-json-read' // Why: callers in install/remove need to match not just the exact current // managed command, but also stale entries pointing at old script paths — e.g. 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 6fdf852bf6b..d20730a4d51 100644 --- a/src/main/codex/codex-real-home-hook-install.test.ts +++ b/src/main/codex/codex-real-home-hook-install.test.ts @@ -88,6 +88,7 @@ beforeEach(() => { afterEach(() => { rebaseInternals.setSessionRunnerSync(null) + rebaseInternals.resetRetryState() rmSync(fakeHomeDir, { recursive: true, force: true }) rmSync(userDataDir, { recursive: true, force: true }) if (previousUserDataPath === undefined) { @@ -405,6 +406,46 @@ describe('ensureRealHomeCodexHookState (opt-out sweep)', () => { expect(readRealHooksJson().hooks?.Stop).toEqual([{ hooks: [before] }, { hooks: [after] }]) }) + it('aborts without writing when hooks.json changes during the trust inspection', () => { + grantSucceeds() + const before = { type: 'command', command: 'before.sh' } + writeFileSync( + getRealHooksJsonPath(), + `${JSON.stringify({ hooks: { Stop: [{ hooks: [before] }] } }, null, 2)}\n` + ) + ensureRealHomeCodexHookState({ hooksEnabled: true, userDataPath: userDataDir }) + const installed = readRealHooksJson() + const after = { type: 'command', command: 'after.sh' } + installed.hooks!.Stop!.push({ hooks: [after] }) + writeFileSync(getRealHooksJsonPath(), `${JSON.stringify(installed, null, 2)}\n`) + const userTrustToml = '[hooks.state."x:stop:0:0"]\ntrusted_hash = "user"\n' + writeFileSync(getRealConfigTomlPath(), userTrustToml, 'utf-8') + const concurrentSave = `${JSON.stringify({ hooks: { Stop: [{ hooks: [before] }] } }, null, 2)}\n` + const operations: string[] = [] + rebaseInternals.setSessionRunnerSync((request) => { + operations.push(request.operation) + // A user save (or a second Orca instance) lands while the RPC runs. + writeFileSync(getRealHooksJsonPath(), concurrentSave, 'utf-8') + return { + outcome: 'inspected', + moves: request.moves.map((move) => ({ + ...move, + reportedOldKey: move.oldKey, + wasTrusted: true, + enabled: true + })) + } + }) + + expect(ensureRealHomeCodexHookState({ hooksEnabled: false, userDataPath: userDataDir })).toBe( + 'unavailable' + ) + + expect(operations).toEqual(['inspect-user-hook-trust']) + expect(readFileSync(getRealHooksJsonPath(), 'utf-8')).toBe(concurrentSave) + expect(readFileSync(getRealConfigTomlPath(), 'utf-8')).toBe(userTrustToml) + }) + it('removes only Orca entries and reports the removed lane', () => { grantSucceeds() const userStop = { diff --git a/src/main/codex/codex-real-home-hook-install.ts b/src/main/codex/codex-real-home-hook-install.ts index 30c0704734a..24e295560d2 100644 --- a/src/main/codex/codex-real-home-hook-install.ts +++ b/src/main/codex/codex-real-home-hook-install.ts @@ -5,7 +5,7 @@ import { buildManagedCommandHook, createManagedCommandMatcher, MANAGED_HOOK_TIMEOUT_SECONDS, - readHooksJson, + readHooksJsonWithRaw, removeManagedCommands, writeHooksJson, writeManagedScript, @@ -119,7 +119,10 @@ function installRealHomeCodexHook(userDataPath: string): RealHomeCodexHookLane { const material = getCodexManagedHookInstallMaterial() const hooksJsonPath = getRealHomeHooksJsonPath() const hooksWritePath = resolveHooksJsonWritePath(hooksJsonPath) - const config = readHooksJson(hooksJsonPath) + // Why: the generation guard compares against these bytes before writing; a + // separate later read would let a concurrent save land between parse and + // snapshot and be silently overwritten by the stale parse. + const { raw: previousRaw, config } = readHooksJsonWithRaw(hooksJsonPath) if (!config) { // Why: an unparseable user file must never be clobbered; without a hook // entry the managed lane keeps status working for this host. @@ -168,7 +171,6 @@ function installRealHomeCodexHook(userDataPath: string): RealHomeCodexHookLane { } } - const previousRaw = existsSync(hooksJsonPath) ? readFileSync(hooksJsonPath, 'utf-8') : null const previousMode = previousRaw === null ? undefined : statSync(hooksWritePath).mode backupRealHomeHooksJsonOnce(userDataPath, previousRaw) // Why: unknown top-level fields belong to the user (other managers' @@ -268,8 +270,10 @@ function getInstallRetryAfterMs(reason: CodexTrustGrantFallbackReason): number { function sweepRealHomeCodexHook(): RealHomeCodexHookLane { const hooksJsonPath = getRealHomeHooksJsonPath() - const config = readHooksJson(hooksJsonPath) - if (!config?.hooks) { + // Why: single read — the pre-write generation guard must compare against + // the exact bytes this sweep's parse came from. + const { raw: previousRaw, config } = readHooksJsonWithRaw(hooksJsonPath) + if (!config?.hooks || previousRaw === null) { return 'removed' } const isManagedCommand = createManagedCommandMatcher(getCodexManagedScriptFileName()) @@ -295,7 +299,6 @@ function sweepRealHomeCodexHook(): RealHomeCodexHookLane { } if (removedAny) { const hooksWritePath = resolveHooksJsonWritePath(hooksJsonPath) - const previousRaw = readFileSync(hooksJsonPath, 'utf-8') const previousMode = statSync(hooksWritePath).mode mutateRealHomeHooksPreservingUserTrust({ sourcePath: hooksJsonPath, diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index 957bfd669f1..9feec736280 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -11,6 +11,7 @@ import { hookDefinitionHasManagedCommand, MANAGED_HOOK_TIMEOUT_SECONDS, readHooksJson, + readHooksJsonWithRaw, removeManagedCommands, wrapPosixHookCommand, wrapWindowsCmdHookCommand, @@ -605,8 +606,10 @@ function cleanupLegacySystemManagedHooks(): void { const systemHomePath = getSystemCodexHomePath() const hasRecordedRealHomeGrant = readCodexTrustGrantLedgerHomeForReconciliation(systemHomePath) !== null - const config = readHooksJson(legacyConfigPath) - if (!config?.hooks) { + // Why: the pre-write guard below compares against these bytes; a separate + // later read would let a concurrent save land between parse and snapshot. + const { raw: previousRaw, config } = readHooksJsonWithRaw(legacyConfigPath) + if (!config?.hooks || previousRaw === null) { if (hasRecordedRealHomeGrant) { removeSystemManagedHookTrustEntries(systemHomePath, legacyConfigPath) } @@ -648,7 +651,6 @@ function cleanupLegacySystemManagedHooks(): void { // Why: this is the user's system hooks file, not Orca's runtime copy. // Remove only stale Orca hook entries and preserve other managers' metadata. const hooksWritePath = resolveHooksJsonWritePath(legacyConfigPath) - const previousRaw = readFileSync(legacyConfigPath, 'utf-8') const previousMode = statSync(hooksWritePath).mode mutateRealHomeHooksPreservingUserTrust({ sourcePath: legacyConfigPath,