diff --git a/config/tsconfig.cli.json b/config/tsconfig.cli.json index 0d278a1e854..a4df2dab4a9 100644 --- a/config/tsconfig.cli.json +++ b/config/tsconfig.cli.json @@ -21,6 +21,7 @@ "../src/main/codex/codex-hook-identity.ts", "../src/main/codex/codex-hook-trust-grant.ts", "../src/main/codex/codex-managed-trust-reconciliation.ts", + "../src/main/codex/codex-process-exit-deadline.ts", "../src/main/codex/codex-trust-config-rollback.ts", "../src/main/codex/codex-trust-grant-host.ts", "../src/main/codex/codex-trust-grant-ledger.ts", diff --git a/src/main/codex/codex-app-server-client.test.ts b/src/main/codex/codex-app-server-client.test.ts index 82df78a57d2..5bb01ae5614 100644 --- a/src/main/codex/codex-app-server-client.test.ts +++ b/src/main/codex/codex-app-server-client.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { mkdtempSync, readFileSync, rmSync, writeFileSync, existsSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -90,6 +90,7 @@ function writeFileSyncSafe(file, contents) { require('node:fs').writeFileSync(fi let tempRoots: string[] = [] afterEach(() => { + vi.restoreAllMocks() for (const root of tempRoots) { rmSync(root, { recursive: true, force: true }) } @@ -149,6 +150,8 @@ function managedHook(key: string, trustStatus = 'untrusted'): StubHook { describe('runCodexHookTrustGrantSession', () => { it('grants and verifies exactly the expected managed entries', async () => { + const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout') + const clearTimeoutSpy = vi.spyOn(globalThis, 'clearTimeout') const keys = [ '/home/a/.codex/hooks.json:session_start:0:0', '/home/a/.codex/hooks.json:stop:0:0' @@ -191,6 +194,13 @@ describe('runCodexHookTrustGrantSession', () => { expect(written.edits[0].mergeStrategy).toBe('upsert') expect(Object.keys(written.edits[0].value).sort()).toEqual([...keys].sort()) expect(written.reloadUserConfig).toBe(true) + // Why: the entry process waits on these handles after setting exitCode, so + // an uncleared grace timer adds its full delay to synchronous launch prep. + const timerHandles = setTimeoutSpy.mock.results.map(({ value }) => value) + expect(timerHandles).toHaveLength(2) + expect(clearTimeoutSpy.mock.calls.map(([handle]) => handle)).toEqual( + expect.arrayContaining(timerHandles) + ) }) it('skips config/batchWrite when every expected entry is already trusted', async () => { diff --git a/src/main/codex/codex-app-server-client.ts b/src/main/codex/codex-app-server-client.ts index 0273893fcc8..ab5dced425c 100644 --- a/src/main/codex/codex-app-server-client.ts +++ b/src/main/codex/codex-app-server-client.ts @@ -1,5 +1,6 @@ import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process' import { normalizeHookTrustKeyForLookup } from './config-toml-trust' +import { waitForProcessExitUntil } from './codex-process-exit-deadline' // Why: Codex gates hooks on a `trusted_hash` it computes from a private // canonical-JSON identity. Orca used to replicate that algorithm @@ -363,11 +364,10 @@ export async function runCodexHookTrustGrantSession( if (!exited) { // Why: the server exits promptly on stdin EOF; the grace period only // bounds a wedged child before the guaranteed SIGKILL reap. - const grace = new Promise((resolve) => setTimeout(resolve, 1500)) - await Promise.race([exitPromise, grace]) + await waitForProcessExitUntil(exitPromise, 1500) if (!exited) { child.kill('SIGKILL') - await Promise.race([exitPromise, new Promise((resolve) => setTimeout(resolve, 1000))]) + await waitForProcessExitUntil(exitPromise, 1000) } } clearTimeout(deadline) diff --git a/src/main/codex/codex-config-mirror.test.ts b/src/main/codex/codex-config-mirror.test.ts index 5022fff5cc0..4b425e2e2ef 100644 --- a/src/main/codex/codex-config-mirror.test.ts +++ b/src/main/codex/codex-config-mirror.test.ts @@ -268,6 +268,9 @@ describe('syncSystemConfigIntoManagedCodexHome', () => { [ 'model = "runtime-model"', '', + '[hooks.state]', + '# runtime-owned parent', + '', '[hooks.state."runtime-hooks:stop:0:0"]', 'enabled = false', 'trusted_hash = "sha256:runtime"', @@ -286,6 +289,9 @@ describe('syncSystemConfigIntoManagedCodexHome', () => { [ 'model = "system-model"', '', + '[hooks.state]', + '# system-owned parent', + '', '[projects."/repo"] # explicit revocation', 'trust_level = "untrusted"', '', @@ -310,6 +316,8 @@ describe('syncSystemConfigIntoManagedCodexHome', () => { expect(runtimeConfig).toContain('[projects."/system-only"]') expect(runtimeConfig).toContain('[hooks.state."runtime-hooks:stop:0:0"]') expect(runtimeConfig).not.toContain('[hooks.state."system-hooks:stop:0:0"]') + expect(runtimeConfig).toContain('# runtime-owned parent') + expect(runtimeConfig).not.toContain('# system-owned parent') expect(runtimeConfig).toContain('trust_level = "untrusted"') expect(runtimeConfig.match(/\[projects\."\/repo"\]/g)?.length).toBe(1) }) diff --git a/src/main/codex/codex-config-mirror.ts b/src/main/codex/codex-config-mirror.ts index e6ca4c0b834..26933c1ba02 100644 --- a/src/main/codex/codex-config-mirror.ts +++ b/src/main/codex/codex-config-mirror.ts @@ -275,7 +275,10 @@ function isRuntimePreservedTomlSection(header: string): boolean { } function isRuntimeHookTrustTomlSection(header: string): boolean { - return header.trimStart().startsWith('[hooks.state.') + const trimmed = header.trim() + // Why: Codex's config writer materializes the parent table on Windows. It is + // part of runtime-owned trust and must survive the next config mirror too. + return trimmed === '[hooks.state]' || trimmed.startsWith('[hooks.state.') } function isRuntimeProjectTomlSection(header: string): boolean { diff --git a/src/main/codex/codex-process-exit-deadline.ts b/src/main/codex/codex-process-exit-deadline.ts new file mode 100644 index 00000000000..d6eaf1eda2c --- /dev/null +++ b/src/main/codex/codex-process-exit-deadline.ts @@ -0,0 +1,18 @@ +export async function waitForProcessExitUntil( + exitPromise: Promise, + timeoutMs: number +): Promise { + let timer: ReturnType | undefined + const timeout = new Promise((resolve) => { + timer = setTimeout(resolve, timeoutMs) + }) + try { + await Promise.race([exitPromise, timeout]) + } finally { + // Why: this runs in a short-lived entry; a live grace timer delays the + // parent spawnSync even after the app-server process has already exited. + if (timer !== undefined) { + clearTimeout(timer) + } + } +} diff --git a/src/main/codex/hook-service-trust-grant.test.ts b/src/main/codex/hook-service-trust-grant.test.ts index 4c51040fc38..c308fde1cc0 100644 --- a/src/main/codex/hook-service-trust-grant.test.ts +++ b/src/main/codex/hook-service-trust-grant.test.ts @@ -7,6 +7,7 @@ import { wrapPosixHookCommand } from '../agent-hooks/installer-utils' import { computeTrustedHash, parseTrustKey, + readHookTrustEntries, upsertHookTrustEntries, type CodexTrustEntry } from './config-toml-trust' @@ -160,7 +161,7 @@ describe('CodexHookService app-server trust grant lane', () => { }) }) - it('upgrades self-computed trust in place without duplicate tables', () => { + it('upgrades self-computed trust in place without duplicate logical entries', () => { prepareSystemHome() const service = new CodexHookService() process.env.ORCA_DISABLE_CODEX_TRUST_RPC = '1' @@ -171,6 +172,9 @@ describe('CodexHookService app-server trust grant lane', () => { expect(service.install().state).toBe('installed') const upgraded = readFileSync(join(managedHome, 'config.toml'), 'utf-8') + // Why: the legacy Windows fallback intentionally writes slash variants; + // duplicate detection is about the normalized trust identity. + const upgradedEntries = readHookTrustEntries(join(managedHome, 'config.toml')) for (const eventLabel of [ 'session_start', 'user_prompt_submit', @@ -179,13 +183,12 @@ describe('CodexHookService app-server trust grant lane', () => { 'post_tool_use', 'stop' ]) { - const count = upgraded - .split('\n') - .filter( - (line) => line.startsWith('[hooks.state.') && line.includes(`:${eventLabel}:0:0`) - ).length - expect(count, `duplicate trust tables for ${eventLabel}`).toBe(1) + const count = [...upgradedEntries.keys()].filter((key) => + key.endsWith(`:${eventLabel}:0:0`) + ).length + expect(count, `duplicate trust entries for ${eventLabel}`).toBe(1) } + expect(upgraded).toContain('sha256:codex-session_start') }) it('leaves user trust byte-untouched while granting managed entries', () => { diff --git a/src/main/codex/hook-service-wsl-runtime.test.ts b/src/main/codex/hook-service-wsl-runtime.test.ts index cc887208f4c..c3d2ce9ca37 100644 --- a/src/main/codex/hook-service-wsl-runtime.test.ts +++ b/src/main/codex/hook-service-wsl-runtime.test.ts @@ -522,7 +522,11 @@ describe('Codex WSL runtime hook install app-server grant lane', () => { const basePlan = createTestPlan() writeFileSync(basePlan.configPath, '{"hooks":{}}\n', 'utf-8') writeFileSync(basePlan.tomlPath, '', 'utf-8') + let staleKeyExpectedRemoved: string | null = null const runner = vi.fn((request: CodexHookTrustGrantRequest) => { + if (staleKeyExpectedRemoved) { + expect(readHookTrustEntries(basePlan.tomlPath).has(staleKeyExpectedRemoved)).toBe(false) + } const entries = request.expectedTrustKeys.map((key) => { const parsed = parseTrustKey(key)! return { @@ -557,6 +561,7 @@ describe('Codex WSL runtime hook install app-server grant lane', () => { const oldKey = computeTrustKey( getManagedTrustEntry(oldPlan, expectedManagedCommand(oldPlan.commandScriptPath)) ) + staleKeyExpectedRemoved = oldKey const newPlan = { ...basePlan, diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index b1b3ac7a3b2..ab680f6edca 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -842,6 +842,13 @@ function installManagedHooksIntoWslRuntime( // Why: a successful re-grant replaces the ledger. Keep the previous // records long enough to prove ownership of stale canonical-path keys. const previousLedgerHome = readCodexTrustGrantLedgerHomeForReconciliation(runtimeHomePath) + // Why: Codex's verified RPC write must be the final config mutation. A + // host-side rewrite after verification can race or invalidate that grant. + removeStaleWslRuntimeManagedHookTrustEntries( + plan.tomlPath, + trustEntries, + previousLedgerHome ? [previousLedgerHome] : [] + ) const grant = grantManagedCodexHookTrust({ runtimeHomePath, tomlPath: plan.tomlPath, @@ -849,17 +856,10 @@ function installManagedHooksIntoWslRuntime( managedEntries: trustEntries, host: { kind: 'wsl', distro: plan.wslDistro, linuxRuntimeHome: plan.linuxRuntimeHome } }) - if (grant.lane === 'rpc') { - removeStaleWslRuntimeManagedHookTrustEntries( - plan.tomlPath, - grant.entries, - previousLedgerHome ? [previousLedgerHome] : [] - ) - } else { + if (grant.lane === 'fallback') { // Why: WSL runtime homes may carry user hook approvals we did not rebuild // here; only upsert Orca's entries instead of sweeping the whole source. upsertHookTrustEntries(plan.tomlPath, trustEntries) - removeStaleWslRuntimeManagedHookTrustEntries(plan.tomlPath, trustEntries) } } catch (error) { return {