fix: harden real-home hook cleanup

This commit is contained in:
Brennan Benson
2026-07-16 01:16:23 -07:00
parent 4788d5191e
commit f5cf6ea53d
7 changed files with 82 additions and 17 deletions
@@ -119,7 +119,7 @@ describe('ensureRealHomeCodexHookState (install)', () => {
expect(plan.managedEntries.every((entry) => entry.groupIndex === 0)).toBe(true)
})
it('appends LAST and preserves user entries, unknown fields, and trust positions', () => {
it('keeps the managed lane for unknown top-level fields Codex cannot load', () => {
grantSucceeds()
const userConfig = {
hooks: {
@@ -128,27 +128,43 @@ describe('ensureRealHomeCodexHookState (install)', () => {
},
_pluginManagerMetadata: { owner: 'someone-else' }
}
writeFileSync(getRealHooksJsonPath(), `${JSON.stringify(userConfig, null, 2)}\n`, 'utf-8')
const original = `${JSON.stringify(userConfig, null, 2)}\n`
writeFileSync(getRealHooksJsonPath(), original, 'utf-8')
const lane = ensureRealHomeCodexHookState({ hooksEnabled: true, userDataPath: userDataDir })
expect(lane).toBe('installed')
expect(lane).toBe('unavailable')
expect(readFileSync(getRealHooksJsonPath(), 'utf-8')).toBe(original)
expect(grantMock).not.toHaveBeenCalled()
expect(existsSync(join(userDataDir, 'codex-real-home-hooks', 'hooks.json.pre-orca'))).toBe(
false
)
})
it('appends LAST and preserves user entries and trust positions', () => {
grantSucceeds()
const userConfig = {
hooks: {
Stop: [{ matcher: 'deploy-*', hooks: [{ type: 'command', command: 'my-stop-hook.sh' }] }],
PreCompact: [{ hooks: [{ type: 'command', command: 'my-compact-hook.sh' }] }]
}
}
const original = `${JSON.stringify(userConfig, null, 2)}\n`
writeFileSync(getRealHooksJsonPath(), original, 'utf-8')
expect(ensureRealHomeCodexHookState({ hooksEnabled: true, userDataPath: userDataDir })).toBe(
'installed'
)
const config = readRealHooksJson()
// User's Stop entry keeps position 0; Orca's entry is appended after it.
expect(config.hooks?.Stop).toHaveLength(2)
expect(config.hooks?.Stop?.[0]).toEqual(userConfig.hooks.Stop[0])
// Non-managed events Orca does not subscribe to stay untouched.
expect(config.hooks?.PreCompact).toEqual(userConfig.hooks.PreCompact)
// Unknown top-level metadata survives, unlike the managed-home writer.
expect(config._pluginManagerMetadata).toEqual(userConfig._pluginManagerMetadata)
// The appended entry's trust key uses its appended position.
const plan = grantMock.mock.calls[0]![0] as CodexManagedTrustGrantPlan
const stopEntry = plan.managedEntries.find((entry) => entry.eventLabel === 'stop')
expect(stopEntry?.groupIndex).toBe(1)
// Pristine pre-Orca backup lands under userData, not in ~/.codex.
expect(plan.managedEntries.find((entry) => entry.eventLabel === 'stop')?.groupIndex).toBe(1)
expect(
readFileSync(join(userDataDir, 'codex-real-home-hooks', 'hooks.json.pre-orca'), 'utf-8')
).toBe(`${JSON.stringify(userConfig, null, 2)}\n`)
).toBe(original)
})
it('updates a symlinked hooks.json target without replacing the symlink', () => {
@@ -127,6 +127,12 @@ function installRealHomeCodexHook(userDataPath: string): RealHomeCodexHookLane {
installRetryAfterMs = Date.now() + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS
return 'unavailable'
}
if (Object.keys(config).some((key) => key !== 'hooks')) {
// Why: Codex rejects unknown root keys instead of ignoring them. Avoid a
// transient rewrite of a user-owned file that the trust RPC cannot load.
installRetryAfterMs = Date.now() + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS
return 'unavailable'
}
// Why: the same script the managed lane maintains; deploying here too keeps
// host-connect ordering independent of the managed installer loop.
+18
View File
@@ -1,6 +1,7 @@
/* eslint-disable max-lines -- Why: this suite keeps the hash fixture, TOML edit edge cases, and trust-state parser regressions together so Codex compatibility failures are easy to audit. */
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
import {
chmodSync,
existsSync,
lstatSync,
mkdirSync,
@@ -9,6 +10,7 @@ import {
realpathSync,
rmSync,
symlinkSync,
statSync,
writeFileSync
} from 'node:fs'
import { tmpdir } from 'node:os'
@@ -1302,6 +1304,22 @@ describe('normalizeCodexProjectPathForRevocationLookup', () => {
})
describe('removeHookTrustEntries', () => {
it.skipIf(process.platform === 'win32')('preserves restrictive config permissions', () => {
const entry: CodexTrustEntry = {
sourcePath: '/x/hooks.json',
eventLabel: 'stop',
groupIndex: 0,
handlerIndex: 0,
command: 'echo trusted'
}
upsertHookTrustEntries(configPath, [entry])
chmodSync(configPath, 0o600)
removeHookTrustEntries(configPath, [computeTrustKey(entry)])
expect(statSync(configPath).mode & 0o777).toBe(0o600)
})
it('updates a symlink target without replacing config.toml', () => {
const targetPath = join(tmpDir, 'dotfiles-config.toml')
const entry: CodexTrustEntry = {
+5 -1
View File
@@ -5,6 +5,7 @@ import {
mkdirSync,
readFileSync,
realpathSync,
statSync,
unlinkSync,
writeFileSync
} from 'node:fs'
@@ -798,9 +799,12 @@ export function writeConfigAtomically(configPath: string, contents: string): voi
const dir = dirname(writePath)
mkdirSync(dir, { recursive: true })
const tmpPath = join(dir, `.${Date.now()}-${randomUUID()}.tmp`)
const existingMode = existsSync(writePath) ? statSync(writePath).mode : undefined
let renamed = false
try {
writeFileSync(tmpPath, contents, 'utf-8')
// Why: real-home trust cleanup must not widen a dotfiles-managed config's
// restrictive permissions when the atomic rename installs new bytes.
writeFileSync(tmpPath, contents, { encoding: 'utf-8', mode: existingMode })
if (existsSync(writePath)) {
copyFileWithWindowsRetry(writePath, `${writePath}.bak`)
}
@@ -1,11 +1,13 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import {
chmodSync,
lstatSync,
mkdirSync,
mkdtempSync,
readFileSync,
rmSync,
symlinkSync,
statSync,
writeFileSync
} from 'node:fs'
import { tmpdir } from 'node:os'
@@ -269,6 +271,25 @@ describe('CodexHookService app-server trust grant lane', () => {
expect(operations).toEqual(['inspect-user-hook-trust', 'repair-user-hook-trust'])
})
it.skipIf(process.platform === 'win32')(
'preserves restrictive real-home hooks permissions during flag-off cleanup',
() => {
prepareSystemHome()
const hooksPath = join(tmpHome, '.codex', 'hooks.json')
const material = getCodexManagedHookInstallMaterial()
writeFileSync(
hooksPath,
`${JSON.stringify({ hooks: { Stop: [{ hooks: [{ type: 'command', command: material.command }] }] } }, null, 2)}\n`
)
chmodSync(hooksPath, 0o600)
installCodexLikeGrantRunner()
expect(new CodexHookService().install().state).toBe('installed')
expect(statSync(hooksPath).mode & 0o777).toBe(0o600)
}
)
it('does not accept a ledger hash after the recorded Codex binary stamp changes', () => {
prepareSystemHome()
installCodexLikeGrantRunner()
+1 -1
View File
@@ -663,7 +663,7 @@ function cleanupLegacySystemManagedHooks(): void {
// never replace that newer dotfiles generation with our stale parse.
throw new Error('System Codex hooks changed during trust repair')
}
writeHooksJson(hooksWritePath, { ...config, hooks: nextHooks })
writeHooksJson(hooksWritePath, { ...config, hooks: nextHooks }, { preserveMode: true })
},
restoreHooks: () => writeFileAtomically(hooksWritePath, previousRaw, { mode: previousMode })
})
+3 -3
View File
@@ -1791,12 +1791,12 @@ app.whenReady().then(async () => {
)
// Why: while the real-home lane owns ~/.codex/hooks.json, the legacy
// system-home sweep inside managed installs would delete the entry the
// real-home installer just appended. Flag OFF or hooks off re-arms the sweep,
// which is also what removes the entry again on downgrade or opt-out.
// real-home installer just appended. Flag OFF, hooks off, or an incapable
// trust lane re-arms the sweep so downgrade, opt-out, and rollback converge.
setSystemCodexHomeHookSweepSuppressed(
() =>
codexRuntimeHome !== null &&
codexRuntimeHome.isHostSystemDefaultRealHomeSelected() &&
codexRuntimeHome.isHostSystemDefaultRealHome() &&
isAgentStatusHooksEnabled(store?.getSettings())
)
codexAccounts = new CodexAccountService(store, rateLimits, codexRuntimeHome)