From 57072bed5178f78555d2210ffa3e8c1024e1efd4 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 7 May 2026 18:35:23 -0700 Subject: [PATCH] fix(codex): register hook trust hashes so Codex 0.129 stops gating Orca's hooks (#1547) Co-authored-by: Orca --- src/main/codex/config-toml-trust.test.ts | 973 +++++++++++++++++++++++ src/main/codex/config-toml-trust.ts | 577 ++++++++++++++ src/main/codex/hook-service.ts | 205 ++++- 3 files changed, 1739 insertions(+), 16 deletions(-) create mode 100644 src/main/codex/config-toml-trust.test.ts create mode 100644 src/main/codex/config-toml-trust.ts diff --git a/src/main/codex/config-toml-trust.test.ts b/src/main/codex/config-toml-trust.test.ts new file mode 100644 index 00000000000..45c315585d4 --- /dev/null +++ b/src/main/codex/config-toml-trust.test.ts @@ -0,0 +1,973 @@ +/* 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 { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' +import { + computeTrustKey, + computeTrustedHash, + parseTrustKey, + readHookTrustEntries, + removeHookTrustEntries, + upsertHookTrustEntries, + type CodexTrustEntry +} from './config-toml-trust' + +// Why: this hash was captured from a real Codex 0.129 `/hooks` approval. If +// Codex changes its serialization or normalization rules, this test fails +// loudly instead of silently shipping bad trust entries that put hooks back +// into the review pile. +const REAL_APPROVED_COMMAND = '/bin/sh "/tmp/orca-case-b-mCmCe6/agent-hooks/codex-hook.sh"' +const REAL_APPROVED_HASH = 'sha256:bc013489dba495431d3790fda62ee5a7d907a7c491e29ad26238c3a5d6d2b163' + +let tmpDir: string +let configPath: string + +beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), 'orca-codex-trust-test-')) + configPath = join(tmpDir, 'config.toml') +}) + +afterEach(() => { + rmSync(tmpDir, { recursive: true, force: true }) +}) + +describe('computeTrustedHash', () => { + it('reproduces the hash that Codex /hooks wrote for a real approval', () => { + expect( + computeTrustedHash({ + sourcePath: '/Users/thebr/.codex/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: REAL_APPROVED_COMMAND + }) + ).toBe(REAL_APPROVED_HASH) + }) + + it('produces a different hash when the command changes', () => { + const a = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo' + }) + const b = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'bar' + }) + expect(a).not.toBe(b) + }) + + it('produces a different hash when the event label changes', () => { + const a = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo' + }) + const b = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'post_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo' + }) + expect(a).not.toBe(b) + }) + + it('ignores groupIndex/handlerIndex (those are part of the key, not the hash)', () => { + const a = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo' + }) + const b = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 99, + handlerIndex: 99, + command: 'foo' + }) + expect(a).toBe(b) + }) + + it('hashes a missing matcher the same as no matcher field', () => { + const a = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo' + }) + const b = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo', + matcher: undefined + }) + expect(a).toBe(b) + }) + + it('produces a different hash when matcher is set', () => { + const a = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo' + }) + const b = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo', + matcher: 'foo' + }) + expect(a).not.toBe(b) + }) + + it('produces a different hash when statusMessage is set', () => { + const a = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo' + }) + const b = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo', + statusMessage: 'msg' + }) + expect(a).not.toBe(b) + }) + + it('produces a different hash when async flips from default false to true', () => { + const a = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo', + async: false + }) + const b = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo', + async: true + }) + expect(a).not.toBe(b) + }) + + it('clamps timeoutSec=0 to 1 (which differs from the unset default of 600)', () => { + const zero = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo', + timeoutSec: 0 + }) + const one = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo', + timeoutSec: 1 + }) + const unset = computeTrustedHash({ + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'foo' + }) + expect(zero).toBe(one) + expect(zero).not.toBe(unset) + }) +}) + +describe('computeTrustKey', () => { + it('joins source path, event label, group index, handler index with colons', () => { + expect( + computeTrustKey({ + sourcePath: '/Users/thebr/.codex/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'irrelevant' + }) + ).toBe('/Users/thebr/.codex/hooks.json:pre_tool_use:0:0') + }) +}) + +describe('upsertHookTrustEntries', () => { + it('creates the file with a trust block when none exists', () => { + const entry: CodexTrustEntry = { + sourcePath: '/foo/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: '/bin/echo hi' + } + upsertHookTrustEntries(configPath, [entry]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).toContain(`[hooks.state."/foo/hooks.json:pre_tool_use:0:0"]`) + expect(written).toContain('enabled = true') + expect(written).toContain(`trusted_hash = "${computeTrustedHash(entry)}"`) + }) + + it('appends to an existing config without disturbing prior content', () => { + const original = [ + 'model = "gpt-5.5"', + 'approval_policy = "never"', + '', + '[features]', + 'hooks = true', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'session_start', + groupIndex: 0, + handlerIndex: 0, + command: 'echo hello' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + expect(written.startsWith(original.trimEnd())).toBe(true) + expect(written).toContain('[hooks.state."/x/hooks.json:session_start:0:0"]') + }) + + it('replaces an existing block keyed at the same path without touching unrelated blocks', () => { + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = [ + '[features]', + 'hooks = true', + '', + `[hooks.state."${key}"]`, + 'enabled = true', + 'trusted_hash = "sha256:STALE"', + '', + '[unrelated]', + 'value = 42', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo new' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).not.toContain('STALE') + expect(written).toContain('[unrelated]') + expect(written).toContain('value = 42') + // Why: we only own the [hooks.state.""] block — the [features] + // block must be unchanged. + expect(written).toContain('[features]\nhooks = true') + }) + + it('writes a single block per entry even when called repeatedly', () => { + const entry: CodexTrustEntry = { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo' + } + upsertHookTrustEntries(configPath, [entry]) + upsertHookTrustEntries(configPath, [entry]) + upsertHookTrustEntries(configPath, [entry]) + + const written = readFileSync(configPath, 'utf-8') + const occurrences = written.match(/\[hooks\.state\./g) ?? [] + expect(occurrences).toHaveLength(1) + }) + + it('writes a .bak file before overwriting an existing config', () => { + writeFileSync(configPath, 'model = "old"\n', 'utf-8') + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo' + } + ]) + expect(existsSync(`${configPath}.bak`)).toBe(true) + expect(readFileSync(`${configPath}.bak`, 'utf-8')).toBe('model = "old"\n') + }) + + it('does not write at all when the file already has the right hash', () => { + const entry: CodexTrustEntry = { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo' + } + upsertHookTrustEntries(configPath, [entry]) + const firstWrite = readFileSync(configPath, 'utf-8') + // Why: a no-op upsert must not roll the .bak forward — repeated calls + // (e.g. from app start) would otherwise destroy the last recoverable copy. + rmSync(`${configPath}.bak`, { force: true }) + upsertHookTrustEntries(configPath, [entry]) + expect(existsSync(`${configPath}.bak`)).toBe(false) + expect(readFileSync(configPath, 'utf-8')).toBe(firstWrite) + }) + + it('replaces a stale block written with CRLF line endings without duplicating', () => { + // Why: regression — Windows-style \r\n in the existing config previously + // caused the header pattern to miss and append a duplicate block. + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = [ + '[features]', + 'hooks = true', + '', + `[hooks.state."${key}"]`, + 'enabled = true', + 'trusted_hash = "sha256:STALE"', + '' + ].join('\r\n') + writeFileSync(configPath, original, 'utf-8') + + const entry: CodexTrustEntry = { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo new' + } + upsertHookTrustEntries(configPath, [entry]) + + const written = readFileSync(configPath, 'utf-8') + const occurrences = written.match(/\[hooks\.state\./g) ?? [] + expect(occurrences).toHaveLength(1) + expect(written).not.toContain('STALE') + expect(written).toContain(`trusted_hash = "${computeTrustedHash(entry)}"`) + }) + + it('preserves an immediately-adjacent unrelated hooks.state block', () => { + const targetKey = '/x/hooks.json:pre_tool_use:0:0' + const neighborKey = '/y/hooks.json:post_tool_use:0:0' + const original = [ + `[hooks.state."${targetKey}"]`, + 'enabled = true', + 'trusted_hash = "sha256:STALE"', + `[hooks.state."${neighborKey}"]`, + 'enabled = true', + 'trusted_hash = "sha256:NEIGHBOR"', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo new' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).not.toContain('STALE') + expect(written).toContain(`[hooks.state."${neighborKey}"]`) + expect(written).toContain('trusted_hash = "sha256:NEIGHBOR"') + // Neighbor's `enabled = true` should still be paired with NEIGHBOR's hash. + const neighborIdx = written.indexOf(`[hooks.state."${neighborKey}"]`) + expect(written.slice(neighborIdx)).toMatch(/enabled = true[\s\S]*sha256:NEIGHBOR/) + }) + + it('preserves an unrelated table whose quoted key contains a `]`', () => { + const original = ['[other."a]b"]', 'foo = 1', ''].join('\n') + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).toContain('[other."a]b"]') + expect(written).toContain('foo = 1') + }) + + // Why: TOML supports both basic-string and literal-string quoted keys; + // header detection must respect `]` inside `'...'` too. + it('preserves an unrelated table whose literal-string key contains a `]`', () => { + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = [ + `[hooks.state."${key}"]`, + 'enabled = true', + 'trusted_hash = "sha256:STALE"', + "[other.'a]b']", + 'foo = 1', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo new' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).not.toContain('STALE') + expect(written).toContain("[other.'a]b']") + expect(written).toContain('foo = 1') + }) + + it('does not treat `[fake]` inside a multi-line basic string as a header', () => { + const original = [ + 'model = "gpt"', + 'description = """', + 'This text has a fake header:', + '[fake]', + 'inside it.', + '"""', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).toContain( + ['description = """', 'This text has a fake header:', '[fake]', 'inside it.', '"""'].join( + '\n' + ) + ) + }) + + it('treats `\\"""` inside a multi-line basic string as an escaped quote, not a close', () => { + // Why: a basic multi-line string with `\"` escapes must not be misread as + // closing early — content and any following real header must survive intact. + const original = [ + 'prompt = """', + 'use \\"\\"\\" carefully', + '"""', + '', + '[other]', + 'x = 1', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).toContain(['prompt = """', 'use \\"\\"\\" carefully', '"""'].join('\n')) + expect(written).toContain('[other]\nx = 1') + expect(written).toContain('[hooks.state."/x/hooks.json:pre_tool_use:0:0"]') + }) + + it('escapes literal `"` and `\\` in the source path inside the trust block header', () => { + const entry: CodexTrustEntry = { + sourcePath: '/x/with"quote\\and\\back/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo' + } + upsertHookTrustEntries(configPath, [entry]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).toContain( + `[hooks.state."/x/with\\"quote\\\\and\\\\back/hooks.json:pre_tool_use:0:0"]` + ) + }) + + it('overwrites an existing block whose header has leading whitespace (TOML allows indent)', () => { + // Why: regression — buildHeaderPattern used to require column-0 headers, + // but the reader accepts indented ones. That mismatch caused upsert to + // append a duplicate `[hooks.state.""]` block, producing invalid TOML. + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = ` [hooks.state."${key}"]\nenabled = true\ntrusted_hash = "sha256:OLD"\n` + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo hi' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + const headerCount = (written.match(/\[hooks\.state\."/g) ?? []).length + expect(headerCount).toBe(1) + expect(written).not.toContain('sha256:OLD') + }) + + it('preserves `enabled = false` when the user hand-edited it before reinstall', () => { + // Why: regression — auto-install on app start used to clobber a + // hand-disabled hook back to enabled = true, removing the only way to + // mute Orca's hook short of full uninstall. + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = `[hooks.state."${key}"]\nenabled = false\ntrusted_hash = "sha256:OLD"\n` + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo hi' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).toContain('enabled = false') + expect(written).not.toContain('enabled = true') + }) + + it('overwrites an existing block when the file ends without a trailing newline', () => { + // Why: regression — buildHeaderPattern used to require a trailing + // `\r?\n`, missing it caused the upsert path to take the no-match branch + // and append a duplicate block at EOF. + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = `[hooks.state."${key}"]\nenabled = true\ntrusted_hash = "sha256:OLD"` + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo hi' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + const headerCount = (written.match(/\[hooks\.state\."/g) ?? []).length + expect(headerCount).toBe(1) + expect(written).not.toContain('sha256:OLD') + }) + + it('overwrites an existing block whose header has an inline comment', () => { + // Why: regression — buildHeaderPattern used to require the header line + // to end at \r?\n or EOF, missing TOML-valid trailing comments. The + // upsert path then took the no-match branch and appended a duplicate + // `[hooks.state.""]` block, producing invalid TOML. + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = `[hooks.state."${key}"] # user note\nenabled = true\ntrusted_hash = "sha256:OLD"\n` + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo hi' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + const headerCount = (written.match(/\[hooks\.state\."/g) ?? []).length + expect(headerCount).toBe(1) + expect(written).not.toContain('sha256:OLD') + }) +}) + +describe('removeHookTrustEntries', () => { + it('is a no-op (creates no file) when the config does not exist', () => { + removeHookTrustEntries(configPath, ['/x/hooks.json:pre_tool_use:0:0']) + expect(existsSync(configPath)).toBe(false) + }) + + it('does not roll a .bak forward when the requested key is not present', () => { + const original = ['[features]', 'hooks = true', ''].join('\n') + writeFileSync(configPath, original, 'utf-8') + removeHookTrustEntries(configPath, ['/missing/hooks.json:pre_tool_use:0:0']) + expect(readFileSync(configPath, 'utf-8')).toBe(original) + expect(existsSync(`${configPath}.bak`)).toBe(false) + }) + + it('removes a single block while leaving unrelated tables intact', () => { + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = [ + '[features]', + 'hooks = true', + '', + `[hooks.state."${key}"]`, + 'enabled = true', + 'trusted_hash = "sha256:KEEP"', + '', + '[unrelated]', + 'value = 42', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + removeHookTrustEntries(configPath, [key]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).not.toContain(`[hooks.state."${key}"]`) + expect(written).not.toContain('sha256:KEEP') + expect(written).toContain('[features]\nhooks = true') + expect(written).toContain('[unrelated]\nvalue = 42') + }) + + it('preserves the line separator when no blank line precedes the removed block', () => { + // Why: regression — removeTrustBlock used to cut from match.index (the + // captured leading newline) and fused the previous content into the next + // header, producing invalid TOML like `a = 1[other]`. + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = [ + 'a = 1', + `[hooks.state."${key}"]`, + 'enabled = true', + 'trusted_hash = "sha256:K"', + '[other]', + 'b = 2', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + removeHookTrustEntries(configPath, [key]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).not.toContain('a = 1[other]') + expect(written).toContain('a = 1\n[other]') + }) + + it('removes multiple blocks in a single call', () => { + const keyA = '/x/hooks.json:pre_tool_use:0:0' + const keyB = '/x/hooks.json:post_tool_use:0:0' + const original = [ + `[hooks.state."${keyA}"]`, + 'enabled = true', + 'trusted_hash = "sha256:A"', + '', + `[hooks.state."${keyB}"]`, + 'enabled = true', + 'trusted_hash = "sha256:B"', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + removeHookTrustEntries(configPath, [keyA, keyB]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).not.toContain(`[hooks.state."${keyA}"]`) + expect(written).not.toContain(`[hooks.state."${keyB}"]`) + expect(written).not.toContain('sha256:A') + expect(written).not.toContain('sha256:B') + }) + + it('removes a block whose header has an inline comment', () => { + // Why: paired with the upsert regression; the same pattern mismatch + // would silently leave the dead block in place during uninstall. + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = `[hooks.state."${key}"] # user note\nenabled = true\ntrusted_hash = "sha256:K"\n` + writeFileSync(configPath, original, 'utf-8') + + removeHookTrustEntries(configPath, [key]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).not.toContain(`[hooks.state."${key}"]`) + }) +}) + +describe('readHookTrustEntries', () => { + it('returns an empty map when the file does not exist', () => { + const result = readHookTrustEntries(configPath) + expect(result.size).toBe(0) + }) + + it('returns key→hash entries for each [hooks.state.""] block', () => { + const keyA = '/x/hooks.json:pre_tool_use:0:0' + const keyB = '/y/hooks.json:post_tool_use:1:0' + const original = [ + `[hooks.state."${keyA}"]`, + 'enabled = true', + 'trusted_hash = "sha256:AAA"', + '', + `[hooks.state."${keyB}"]`, + 'enabled = true', + 'trusted_hash = "sha256:BBB"', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + const result = readHookTrustEntries(configPath) + expect(result.size).toBe(2) + expect(result.get(keyA)?.trustedHash).toBe('sha256:AAA') + expect(result.get(keyA)?.enabled).toBe(true) + expect(result.get(keyB)?.trustedHash).toBe('sha256:BBB') + expect(result.get(keyB)?.enabled).toBe(true) + }) + + it('unescapes `\\\\` in the block key', () => { + // Why: a real Windows path on disk like C:\foo gets written escaped as + // `C:\\foo` inside the TOML key — the returned Map should expose the + // original unescaped form. + const original = [ + '[hooks.state."C:\\\\foo\\\\hooks.json:pre_tool_use:0:0"]', + 'enabled = true', + 'trusted_hash = "sha256:WIN"', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + const result = readHookTrustEntries(configPath) + expect(result.get('C:\\foo\\hooks.json:pre_tool_use:0:0')?.trustedHash).toBe('sha256:WIN') + }) + + it('reads entries from a CRLF-terminated config', () => { + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = [ + `[hooks.state."${key}"]`, + 'enabled = true', + 'trusted_hash = "sha256:CRLF"', + '' + ].join('\r\n') + writeFileSync(configPath, original, 'utf-8') + + const result = readHookTrustEntries(configPath) + expect(result.get(key)?.trustedHash).toBe('sha256:CRLF') + expect(result.get(key)?.enabled).toBe(true) + }) + + it('keeps blocks that have no `trusted_hash` field so callers can see enabled-only state', () => { + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = [`[hooks.state."${key}"]`, 'enabled = false', ''].join('\n') + writeFileSync(configPath, original, 'utf-8') + + const result = readHookTrustEntries(configPath) + expect(result.size).toBe(1) + expect(result.get(key)).toEqual({ trustedHash: undefined, enabled: false }) + }) + + it('reads disabled state alongside a valid trusted hash', () => { + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = [ + `[hooks.state."${key}"]`, + 'enabled = false', + 'trusted_hash = "sha256:DISABLED"', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + const result = readHookTrustEntries(configPath) + expect(result.get(key)).toEqual({ trustedHash: 'sha256:DISABLED', enabled: false }) + }) + + it('does not extract a fake [hooks.state.""] header from inside a """ block', () => { + // Why: a header-shaped line embedded in a multi-line basic string must not + // be parsed as a real trust entry. + const original = [ + 'description = """', + '[hooks.state."fake-key"]', + 'enabled = true', + 'trusted_hash = "sha256:FAKE"', + '"""', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + const result = readHookTrustEntries(configPath) + expect(result.size).toBe(0) + }) + + it("does not extract a fake [hooks.state.\"\"] header from inside a ''' block", () => { + // Why: same false-positive guard for multi-line literal strings. + const original = [ + "description = '''", + '[hooks.state."fake-key"]', + 'enabled = true', + 'trusted_hash = "sha256:FAKE"', + "'''", + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + const result = readHookTrustEntries(configPath) + expect(result.size).toBe(0) + }) + + it('reads a block whose header has an inline comment', () => { + // Why: regression — headerLineRegex used to reject TOML-valid trailing + // comments, hiding existing trust entries from getStatus and causing + // it to misreport hooks as untrusted. + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = `[hooks.state."${key}"] # user note\nenabled = true\ntrusted_hash = "sha256:CMT"\n` + writeFileSync(configPath, original, 'utf-8') + + const result = readHookTrustEntries(configPath) + expect(result.size).toBe(1) + expect(result.get(key)?.trustedHash).toBe('sha256:CMT') + }) +}) + +describe('parseTrustKey', () => { + it('parses a typical posix-style key', () => { + expect(parseTrustKey('/Users/x/.codex/hooks.json:pre_tool_use:0:0')).toEqual({ + sourcePath: '/Users/x/.codex/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0 + }) + }) + + it('parses a Windows-style sourcePath whose drive letter contains a colon', () => { + // Why: validates the "anchor on the LAST three colons" approach so colons + // inside the sourcePath itself round-trip correctly. + expect(parseTrustKey('C:\\Users\\x\\.codex\\hooks.json:session_start:2:3')).toEqual({ + sourcePath: 'C:\\Users\\x\\.codex\\hooks.json', + eventLabel: 'session_start', + groupIndex: 2, + handlerIndex: 3 + }) + }) + + it('returns null for a non-Codex event label', () => { + expect(parseTrustKey('/x/hooks.json:not_an_event:0:0')).toBeNull() + }) + + it('returns null for a key with too few colons', () => { + expect(parseTrustKey('foo:bar')).toBeNull() + expect(parseTrustKey('foo')).toBeNull() + }) + + it('returns null when the group index is not an integer', () => { + expect(parseTrustKey('/x/hooks.json:pre_tool_use:abc:0')).toBeNull() + }) + + it('returns null when the handler index is not an integer', () => { + expect(parseTrustKey('/x/hooks.json:pre_tool_use:0:abc')).toBeNull() + }) + + it('returns null when the source path is empty', () => { + expect(parseTrustKey(':pre_tool_use:0:0')).toBeNull() + }) + + it('round-trips with computeTrustKey', () => { + const entry: CodexTrustEntry = { + sourcePath: '/Users/x/.codex/hooks.json', + eventLabel: 'post_tool_use', + groupIndex: 4, + handlerIndex: 7, + command: 'irrelevant' + } + const parsed = parseTrustKey(computeTrustKey(entry)) + expect(parsed).toEqual({ + sourcePath: entry.sourcePath, + eventLabel: entry.eventLabel, + groupIndex: entry.groupIndex, + handlerIndex: entry.handlerIndex + }) + }) + + // Why: Number('') === 0 silently passes Number.isInteger; without strict + // canonical-form validation, malformed keys would coerce into valid ones. + it('returns null for empty group/handler segments', () => { + expect(parseTrustKey('/x/hooks.json:pre_tool_use::0')).toBeNull() + expect(parseTrustKey('/x/hooks.json:pre_tool_use:0:')).toBeNull() + expect(parseTrustKey('/x/hooks.json:pre_tool_use::')).toBeNull() + }) + + it('returns null for exponent or whitespace numeric segments', () => { + expect(parseTrustKey('/x/hooks.json:pre_tool_use:1e2:0')).toBeNull() + expect(parseTrustKey('/x/hooks.json:pre_tool_use: 0:0')).toBeNull() + expect(parseTrustKey('/x/hooks.json:pre_tool_use:01:0')).toBeNull() + }) +}) + +describe('upsertHookTrustEntries with array-of-tables boundaries', () => { + // Why: findNextTableHeader must treat `[[array.of.tables]]` as a block + // boundary; otherwise an upsert/remove can consume past array entries + // into unrelated user content. + it('stops the replacement at a following [[array.of.tables]] header', () => { + const key = '/x/hooks.json:pre_tool_use:0:0' + const original = [ + `[hooks.state."${key}"]`, + 'enabled = true', + 'trusted_hash = "sha256:STALE"', + '', + '[[products]]', + 'name = "thing"', + '' + ].join('\n') + writeFileSync(configPath, original, 'utf-8') + + upsertHookTrustEntries(configPath, [ + { + sourcePath: '/x/hooks.json', + eventLabel: 'pre_tool_use', + groupIndex: 0, + handlerIndex: 0, + command: 'echo' + } + ]) + + const written = readFileSync(configPath, 'utf-8') + expect(written).not.toContain('STALE') + expect(written).toContain('[[products]]') + expect(written).toContain('name = "thing"') + }) +}) diff --git a/src/main/codex/config-toml-trust.ts b/src/main/codex/config-toml-trust.ts new file mode 100644 index 00000000000..1bd8594e8c9 --- /dev/null +++ b/src/main/codex/config-toml-trust.ts @@ -0,0 +1,577 @@ +/* eslint-disable max-lines -- Why: Codex hook trust parsing, hashing, and byte-preserving TOML edits share one fragile file-format contract; splitting would make the compatibility shim harder to audit. */ +import { + copyFileSync, + existsSync, + mkdirSync, + readFileSync, + renameSync, + unlinkSync, + writeFileSync +} from 'fs' +import { dirname, join } from 'path' +import { createHash, randomUUID } from 'crypto' + +// Why: Codex 0.129+ gates each hook on a `trusted_hash` entry in +// ~/.codex/config.toml under [hooks.state.""]. Without it the hook is in +// the "review required" pile and never fires, so the agent-status sidebar +// silently goes blank. We reproduce Codex's hash so install() can register +// trust the same way `/hooks` would. Algorithm reverse-engineered from +// codex-rs/hooks/src/engine/discovery.rs (command_hook_hash) + +// codex-rs/config/src/fingerprint.rs (version_for_toml). + +export type CodexEventLabel = + | 'pre_tool_use' + | 'permission_request' + | 'post_tool_use' + | 'pre_compact' + | 'post_compact' + | 'session_start' + | 'user_prompt_submit' + | 'stop' + +export type CodexTrustEntry = { + /** Path on disk to the hooks.json that declares the hook (the "key_source"). */ + sourcePath: string + /** Codex event label (snake_case). */ + eventLabel: CodexEventLabel + /** 0-based index of the matcher group within the event array. */ + groupIndex: number + /** 0-based index of the handler within the matcher group's `hooks` array. */ + handlerIndex: number + /** The exact `command` string written to hooks.json. */ + command: string + /** Effective timeout in seconds. When undefined, defaults to 600. + * Explicit values are clamped to a minimum of 1. */ + timeoutSec?: number + /** Whether the handler is async. Defaults to false. */ + async?: boolean + /** Optional matcher pattern (only meaningful for events that support it). */ + matcher?: string + /** Optional statusMessage field. */ + statusMessage?: string +} + +export type CodexHookTrustState = { + trustedHash?: string + enabled?: boolean +} + +// Why: matches Codex's canonical_json. Sorts object keys recursively before +// SHA-256ing; arrays preserve order. +function canonicalize(value: unknown): unknown { + if (Array.isArray(value)) { + return value.map(canonicalize) + } + if (value && typeof value === 'object') { + const sorted: Record = {} + for (const key of Object.keys(value as Record).sort()) { + sorted[key] = canonicalize((value as Record)[key]) + } + return sorted + } + return value +} + +// Why: reproduces command_hook_hash. NormalizedHookIdentity has `group: +// MatcherGroup` flattened in, so the wire shape is { event_name, matcher?, +// hooks: [] }. `matcher` is omitted (not null) when +// absent — Rust's Option=None drops through the TOML→JSON path. +// Handler is normalized to timeout=600 (or explicit, min 1) and async=false. +export function computeTrustedHash(entry: CodexTrustEntry): string { + const handler: Record = { + type: 'command', + command: entry.command, + timeout: Math.max(1, entry.timeoutSec ?? 600), + async: entry.async ?? false + } + if (entry.statusMessage !== undefined) { + handler.statusMessage = entry.statusMessage + } + const identity: Record = { + event_name: entry.eventLabel, + hooks: [handler] + } + if (entry.matcher !== undefined) { + identity.matcher = entry.matcher + } + const serialized = JSON.stringify(canonicalize(identity)) + return `sha256:${createHash('sha256').update(serialized).digest('hex')}` +} + +export function computeTrustKey(entry: CodexTrustEntry): string { + return `${entry.sourcePath}:${entry.eventLabel}:${entry.groupIndex}:${entry.handlerIndex}` +} + +export function parseTrustKey(key: string): { + sourcePath: string + eventLabel: CodexEventLabel + groupIndex: number + handlerIndex: number +} | null { + // Why: keys have shape `:::`. + // sourcePath itself may contain `:` (Windows drive letters), so anchor the + // parse at the LAST three colons rather than the first. + const lastColon = key.lastIndexOf(':') + if (lastColon === -1) { + return null + } + const handlerStr = key.slice(lastColon + 1) + if (!isCanonicalNonNegativeInt(handlerStr)) { + return null + } + const secondLast = key.lastIndexOf(':', lastColon - 1) + if (secondLast === -1) { + return null + } + const groupStr = key.slice(secondLast + 1, lastColon) + if (!isCanonicalNonNegativeInt(groupStr)) { + return null + } + const thirdLast = key.lastIndexOf(':', secondLast - 1) + if (thirdLast === -1) { + return null + } + const eventLabel = key.slice(thirdLast + 1, secondLast) + if (!isCodexEventLabel(eventLabel)) { + return null + } + const sourcePath = key.slice(0, thirdLast) + if (sourcePath.length === 0) { + return null + } + return { + sourcePath, + eventLabel, + groupIndex: Number(groupStr), + handlerIndex: Number(handlerStr) + } +} + +// Why: Number('') === 0 and Number('1e2') === 100 both pass Number.isInteger, +// so reject any non-canonical decimal form before numeric conversion. +function isCanonicalNonNegativeInt(value: string): boolean { + return /^(0|[1-9]\d*)$/.test(value) +} + +function isCodexEventLabel(value: string): value is CodexEventLabel { + return ( + value === 'pre_tool_use' || + value === 'permission_request' || + value === 'post_tool_use' || + value === 'pre_compact' || + value === 'post_compact' || + value === 'session_start' || + value === 'user_prompt_submit' || + value === 'stop' + ) +} + +// Why: TOML 1.0 forbids BOMs but real-world editors (especially on Windows) sometimes +// write them. A leading  would break header regexes anchored at `^[ \t]*\[`, so +// strip it once at the file boundary and let the rest of the parser stay simple. +function readTomlFile(configPath: string): string { + const raw = readFileSync(configPath, 'utf-8') + return raw.charCodeAt(0) === 0xfeff ? raw.slice(1) : raw +} + +// Why: regex-edit ~/.codex/config.toml rather than parse + reserialize. The +// file is hand-edited by users (and other tools) and a round-trip through +// any TOML library would lose comments, key ordering, and inline-table +// style. We only ever (a) replace an existing [hooks.state.""] block +// keyed by *our* known hook keys, or (b) append a new block at EOF. Other +// content is byte-preserved. +// Why: this is a read-modify-write with no inter-process lock. Codex CLI's +// /hooks flow also writes [hooks.state.*] blocks in this file, so two +// concurrent writers can lose each other's edits. writeConfigAtomically +// prevents partial writes but not lost updates. install() is idempotent +// (deterministic hashes), so the next install() repairs drift. +export function upsertHookTrustEntries( + configPath: string, + entries: readonly CodexTrustEntry[] +): void { + const existing = existsSync(configPath) ? readTomlFile(configPath) : '' + let updated = existing + for (const entry of entries) { + updated = upsertTrustBlock(updated, computeTrustKey(entry), computeTrustedHash(entry)) + } + if (updated === existing) { + return + } + writeConfigAtomically(configPath, updated) +} + +// Why: build the canonical block we own. The two field names mirror what +// Codex itself writes when the user approves via /hooks (HookStateToml +// fields). `enabled` is plumbed through so an existing user-set +// `enabled = false` survives reinstall. +function buildTrustBlock(key: string, hash: string, enabled: boolean): string { + return [ + `[hooks.state."${escapeTomlString(key)}"]`, + `enabled = ${enabled}`, + `trusted_hash = "${escapeTomlString(hash)}"` + ].join('\n') +} + +// Why: TOML basic strings forbid raw control chars; escape backslash first so +// later substitutions don't double-escape the inserted backslashes. +function escapeTomlString(value: string): string { + return value + .replaceAll('\\', '\\\\') + .replaceAll('"', '\\"') + .replaceAll('\b', '\\b') + .replaceAll('\f', '\\f') + .replaceAll('\n', '\\n') + .replaceAll('\r', '\\r') + .replaceAll('\t', '\\t') +} + +function upsertTrustBlock(content: string, key: string, hash: string): string { + const headerPattern = buildHeaderPattern(key) + const match = headerPattern.exec(content) + if (!match) { + const block = buildTrustBlock(key, hash, true) + if (content.length === 0) { + return `${block}\n` + } + // Why: leave one blank line before our appended block so the file stays + // readable, but don't compound separators when the file already ends in + // a blank line. + const separator = content.endsWith('\n\n') ? '' : content.endsWith('\n') ? '\n' : '\n\n' + return `${content}${separator}${block}\n` + } + const headerStart = match.index + (match[1] ? match[1].length : 0) + const headerLineEnd = match.index + match[0].length + // Why: find the next top-level table header [...] so we replace ONLY this + // block. Comments and blank lines between us and the next header are part + // of our block and get rewritten — Codex itself only writes the two known + // fields, so this is safe. + const after = content.slice(headerLineEnd) + const nextHeaderRel = findNextTableHeader(after) + const blockEnd = nextHeaderRel === -1 ? content.length : headerLineEnd + nextHeaderRel + // Why: preserve a user-set `enabled = false` so a hand-disabled hook is not + // silently re-enabled by the next auto-install on app start. + const existingBlock = content.slice(headerLineEnd, blockEnd) + const enabledMatch = /^[ \t]*enabled[ \t]*=[ \t]*(true|false)[ \t\r]*(?:#.*)?$/m.exec( + existingBlock + ) + const enabled = enabledMatch ? enabledMatch[1] === 'true' : true + const block = buildTrustBlock(key, hash, enabled) + return `${content.slice(0, headerStart)}${block}\n${content.slice(blockEnd)}` +} + +// Why: Codex emits the canonical form with the key double-quoted; we never +// share this slot with another tool, so we don't bother accepting bare +// dotted-key variants. +// Why: accept both LF and CRLF — Windows editors (and some user-edited files) +// terminate the header line with \r\n. +// Why: TOML allows leading whitespace before headers, so accept indented +// headers — the reader does too, and a column-0-only writer would otherwise +// append a duplicate block on hand-indented configs. +// Why: trailing newline is a lookahead so a header at EOS without a final +// newline still matches (otherwise we append a duplicate block). +// Why: TOML allows `# inline comment` after `]`, so accept it before the +// line-end lookahead — otherwise a user-annotated header would force the +// no-match branch and append a duplicate block. +function buildHeaderPattern(key: string): RegExp { + const escapedKey = escapeRegex(escapeTomlString(key)) + return new RegExp( + `(^|\\r?\\n)[ \\t]*\\[hooks\\.state\\."${escapedKey}"\\][ \\t]*(?:#[^\\r\\n]*)?(?=\\r?\\n|$)` + ) +} + +function escapeRegex(value: string): string { + return value.replaceAll(/[.*+?^${}()|[\]\\]/g, '\\$&') +} + +// Why: quoted keys can contain `]` (e.g. `[hooks.state."a]b"]`) and `[` lines +// inside multi-line strings aren't headers, so we need a stateful scanner — +// a flat regex misclassifies both cases. +function findNextTableHeader(text: string): number { + let cursor = 0 + let inMultilineBasic = false + let inMultilineLiteral = false + while (cursor < text.length) { + const newlineIdx = text.indexOf('\n', cursor) + const lineEnd = newlineIdx === -1 ? text.length : newlineIdx + const rawLine = text.slice(cursor, lineEnd) + const line = rawLine.replace(/\r$/, '') + if (inMultilineBasic) { + if (countUnescapedTripleQuote(line, '"""') % 2 === 1) { + inMultilineBasic = false + } + } else if (inMultilineLiteral) { + if (countTripleQuote(line, "'''") % 2 === 1) { + inMultilineLiteral = false + } + } else { + const trimmed = line.trimStart() + // Why: stop at both `[table]` and `[[array.of.tables]]` — both end our + // block. Skipping `[[ ]]` here would let our slice consume past array + // entries into unrelated user content. + if (trimmed.startsWith('[') && isCompleteTableHeader(trimmed)) { + return cursor + } + // Odd count means this line opens a multi-line string without closing it. + if (countUnescapedTripleQuote(line, '"""') % 2 === 1) { + inMultilineBasic = true + } + if (countTripleQuote(line, "'''") % 2 === 1) { + inMultilineLiteral = true + } + } + if (newlineIdx === -1) { + return -1 + } + cursor = newlineIdx + 1 + } + return -1 +} + +// Why: literal strings (`'''`) don't honor escapes, so a plain indexOf scan +// suffices. +function countTripleQuote(line: string, quote: string): number { + let count = 0 + let i = 0 + while ((i = line.indexOf(quote, i)) !== -1) { + count++ + i += 3 + } + return count +} + +// Why: basic multi-line strings honor `\"` (and `\\`) escapes. Skip past +// `\` so an escaped quote doesn't count toward triple-quote scans. +function countUnescapedTripleQuote(line: string, quote: '"""'): number { + let count = 0 + let i = 0 + while (i < line.length) { + if (line[i] === '\\' && i + 1 < line.length) { + i += 2 + continue + } + if (line.startsWith(quote, i)) { + count++ + i += 3 + continue + } + i++ + } + return count +} + +// Why: walk the header byte-by-byte so `]` inside a quoted key segment +// doesn't terminate us early. Basic strings honor `\` escapes; literal +// strings (single quotes) don't allow escapes per TOML spec. +// Accepts both `[table]` and `[[array.of.tables]]` since either ends a block. +function isCompleteTableHeader(line: string): boolean { + if (!line.startsWith('[')) { + return false + } + const isArrayHeader = line.startsWith('[[') + let i = isArrayHeader ? 2 : 1 + let inBasicQuote = false + let inLiteralQuote = false + while (i < line.length) { + const ch = line[i] + if (inBasicQuote) { + if (ch === '\\' && i + 1 < line.length) { + i += 2 + continue + } + if (ch === '"') { + inBasicQuote = false + } + i++ + continue + } + if (inLiteralQuote) { + if (ch === "'") { + inLiteralQuote = false + } + i++ + continue + } + if (ch === '"') { + inBasicQuote = true + i++ + continue + } + if (ch === "'") { + inLiteralQuote = true + i++ + continue + } + if (ch === ']') { + if (isArrayHeader) { + if (line[i + 1] !== ']') { + return false + } + const tail = line.slice(i + 2) + return /^\s*(#.*)?$/.test(tail) + } + const tail = line.slice(i + 1) + return /^\s*(#.*)?$/.test(tail) + } + i++ + } + return false +} + +// Why: same atomic-rename + .bak rotation pattern as writeHooksJson — a +// half-written config.toml can brick a user's Codex install, so write to +// tmp and rename. Random-suffix tmp name avoids cross-process races on +// rapid reinstalls. +function writeConfigAtomically(configPath: string, contents: string): void { + const dir = dirname(configPath) + mkdirSync(dir, { recursive: true }) + const tmpPath = join(dir, `.${Date.now()}-${randomUUID()}.tmp`) + let renamed = false + try { + writeFileSync(tmpPath, contents, 'utf-8') + if (existsSync(configPath)) { + copyFileSync(configPath, `${configPath}.bak`) + } + renameSync(tmpPath, configPath) + renamed = true + } finally { + if (!renamed && existsSync(tmpPath)) { + try { + unlinkSync(tmpPath) + } catch { + // best effort — surfacing the cleanup failure would mask the original write error + } + } + } +} + +export function removeHookTrustEntries(configPath: string, keys: readonly string[]): void { + if (!existsSync(configPath)) { + return + } + const existing = readTomlFile(configPath) + let updated = existing + for (const key of keys) { + updated = removeTrustBlock(updated, key) + } + if (updated === existing) { + return + } + writeConfigAtomically(configPath, updated) +} + +function removeTrustBlock(content: string, key: string): string { + const headerPattern = buildHeaderPattern(key) + const match = headerPattern.exec(content) + if (!match) { + return content + } + // Why: skip past the captured leading newline so we don't fuse the previous + // line into the next header (e.g. `a = 1[other]` — invalid TOML). + const cutStart = match.index + (match[1] ? match[1].length : 0) + const headerLineEnd = match.index + match[0].length + const after = content.slice(headerLineEnd) + const nextHeaderRel = findNextTableHeader(after) + const cutEnd = nextHeaderRel === -1 ? content.length : headerLineEnd + nextHeaderRel + return content.slice(0, cutStart) + content.slice(cutEnd) +} + +export function readHookTrustEntries(configPath: string): Map { + const result = new Map() + if (!existsSync(configPath)) { + return result + } + const content = readTomlFile(configPath) + // Why: walk line-by-line so `[hooks.state."..."]` inside a `"""..."""` or + // `'''...'''` multi-line string isn't mistaken for a real header. + // Why: accept an optional `# inline comment` after `]` — TOML permits it, + // and rejecting hides a real entry, making getStatus misreport trustMissing. + const headerLineRegex = /^[ \t]*\[hooks\.state\."((?:[^"\\]|\\.)*)"\][ \t]*(?:#[^\r\n]*)?$/ + let cursor = 0 + let inMultilineBasic = false + let inMultilineLiteral = false + while (cursor < content.length) { + const newlineIdx = content.indexOf('\n', cursor) + const lineEnd = newlineIdx === -1 ? content.length : newlineIdx + const rawLine = content.slice(cursor, lineEnd) + const line = rawLine.replace(/\r$/, '') + const nextCursor = newlineIdx === -1 ? content.length : newlineIdx + 1 + if (inMultilineBasic) { + if (countUnescapedTripleQuote(line, '"""') % 2 === 1) { + inMultilineBasic = false + } + cursor = nextCursor + continue + } + if (inMultilineLiteral) { + if (countTripleQuote(line, "'''") % 2 === 1) { + inMultilineLiteral = false + } + cursor = nextCursor + continue + } + const headerMatch = headerLineRegex.exec(line) + if (headerMatch) { + const escapedKey = headerMatch[1] + const key = unescapeTomlString(escapedKey) + // Why: block ends at the next *real* header (multi-line aware). + const after = content.slice(nextCursor) + const nextHeaderRel = findNextTableHeader(after) + const blockEnd = nextHeaderRel === -1 ? content.length : nextCursor + nextHeaderRel + const block = content.slice(nextCursor, blockEnd) + // Why: we own this block's shape (only `enabled` + `trusted_hash`), so + // a line scan beats pulling in a full TOML value parser. + const hashMatch = /^[ \t]*trusted_hash[ \t]*=[ \t]*"((?:[^"\\]|\\.)*)"/m.exec(block) + const enabledMatch = /^[ \t]*enabled[ \t]*=[ \t]*(true|false)[ \t\r]*(?:#.*)?$/m.exec(block) + result.set(key, { + trustedHash: hashMatch ? unescapeTomlString(hashMatch[1]) : undefined, + enabled: enabledMatch ? enabledMatch[1] === 'true' : undefined + }) + cursor = nextCursor + continue + } + if (countUnescapedTripleQuote(line, '"""') % 2 === 1) { + inMultilineBasic = true + } + if (countTripleQuote(line, "'''") % 2 === 1) { + inMultilineLiteral = true + } + cursor = nextCursor + } + return result +} + +function unescapeTomlString(escaped: string): string { + let result = '' + let i = 0 + while (i < escaped.length) { + const ch = escaped[i] + if (ch === '\\' && i + 1 < escaped.length) { + const next = escaped[i + 1] + if (next === 'n') { + result += '\n' + } else if (next === 'r') { + result += '\r' + } else if (next === 't') { + result += '\t' + } else if (next === 'b') { + result += '\b' + } else if (next === 'f') { + result += '\f' + } else if (next === '"') { + result += '"' + } else if (next === '\\') { + result += '\\' + } + // Why: unknown escapes round-trip — preserve the backslash so we don't + // silently drop information. + else { + result += `\\${next}` + } + i += 2 + } else { + result += ch + i++ + } + } + return result +} diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index c7ed872f32f..35627d36420 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- Why: getStatus + install + remove all share the managed-command and trust-key derivation. Splitting would hide that the three operations must agree on group index, event label, and command bytes. */ import { homedir } from 'os' import { join } from 'path' import { app } from 'electron' @@ -11,6 +12,17 @@ import { writeManagedScript, type HookDefinition } from '../agent-hooks/installer-utils' +import { + computeTrustKey, + computeTrustedHash, + parseTrustKey, + readHookTrustEntries, + removeHookTrustEntries, + upsertHookTrustEntries, + type CodexEventLabel, + type CodexHookTrustState, + type CodexTrustEntry +} from './config-toml-trust' // Why: PreToolUse/PostToolUse give the dashboard a live readout of the // in-flight tool (name + input preview) between UserPromptSubmit and Stop. @@ -31,6 +43,22 @@ function getConfigPath(): string { return join(homedir(), '.codex', 'hooks.json') } +function getCodexConfigTomlPath(): string { + return join(homedir(), '.codex', 'config.toml') +} + +// Why: Codex's hash key uses the snake_case event label (see +// codex-rs/hooks/src/lib.rs::hook_event_key_label). Our hooks.json uses the +// PascalCase serde-rename. Map between them at one place so the trust-write +// path can't drift from the install path. +const CODEX_EVENT_LABEL: Record<(typeof CODEX_EVENTS)[number], CodexEventLabel> = { + SessionStart: 'session_start', + UserPromptSubmit: 'user_prompt_submit', + PreToolUse: 'pre_tool_use', + PostToolUse: 'post_tool_use', + Stop: 'stop' +} + function getManagedScriptFileName(): string { return process.platform === 'win32' ? 'codex-hook.cmd' : 'codex-hook.sh' } @@ -109,37 +137,102 @@ export class CodexHookService { } } - // Why: Report `partial` when only some managed events are registered so the - // sidebar surfaces a degraded install rather than a false-positive - // `installed`. Each CODEX_EVENTS entry must contain the managed command for - // the integration to function end-to-end (e.g. PreToolUse is required for - // permission-prompt detection per the comment above). + // Why: Report `partial` when managed events are missing OR when their + // trust entries are missing/stale. Codex 0.129+ silently drops untrusted + // hooks, so a green status without trust verification is misleading. const command = getManagedCommand(scriptPath) + const tomlPath = getCodexConfigTomlPath() + // Why: an unreadable config.toml (EACCES/EIO) is distinct from "file + // absent" (which returns an empty Map without throwing). Hooks.json may + // still be fine, so report partial with a specific reason rather than + // collapsing to a generic error or masking it as universally-stale trust. + let trustEntries: Map + let trustReadError: string | null = null + try { + trustEntries = readHookTrustEntries(tomlPath) + } catch (error) { + trustEntries = new Map() + trustReadError = error instanceof Error ? error.message : String(error) + } + const missing: string[] = [] + const trustMissing: string[] = [] + const disabled: string[] = [] let presentCount = 0 for (const eventName of CODEX_EVENTS) { const definitions = Array.isArray(config.hooks?.[eventName]) ? config.hooks![eventName]! : [] - const hasCommand = definitions.some((definition) => - (definition.hooks ?? []).some((hook) => hook.command === command) - ) - if (hasCommand) { - presentCount += 1 - } else { + // Why: install() appends our managed definition at the end, so its + // group index is the LAST match. Picking the first match would + // misreport stale duplicates as trust-missing. + let foundGroupIndex = -1 + let foundHandlerIndex = -1 + definitions.forEach((definition, idx) => { + const hooks = definition.hooks ?? [] + // Why: mirror the LAST-match-wins rule at the group level — if a user + // merged hook arrays and ended up with our command at multiple indices + // in one group, the surviving runtime entry is the last one. + const handlerIdx = hooks.findLastIndex((hook) => hook.command === command) + if (handlerIdx !== -1) { + foundGroupIndex = idx + foundHandlerIndex = handlerIdx + } + }) + if (foundGroupIndex === -1) { missing.push(eventName) + continue + } + presentCount += 1 + // Why: a stale hash blocks firing the same as a missing entry, so + // compare against the canonical hash we would write. + // Why: capture the actual handler index — Codex's hook_key uses the + // positional handlerIndex, and a user-merged hook array can put our + // command at a non-zero slot, so hardcoding 0 would misreport trust. + const trustInput: CodexTrustEntry = { + sourcePath: configPath, + eventLabel: CODEX_EVENT_LABEL[eventName], + groupIndex: foundGroupIndex, + handlerIndex: foundHandlerIndex, + command + } + const expectedHash = computeTrustedHash(trustInput) + const actualState = trustEntries.get(computeTrustKey(trustInput)) + if (actualState?.trustedHash !== expectedHash) { + trustMissing.push(eventName) + } else if (actualState?.enabled === false) { + disabled.push(eventName) } } const managedHooksPresent = presentCount > 0 let state: AgentHookInstallState let detail: string | null - if (missing.length === 0) { - state = 'installed' - detail = null - } else if (presentCount === 0) { + if (presentCount === 0) { state = 'not_installed' + // Why: surface the trust read error even when not_installed so the user + // has actionable info if config.toml is broken. + detail = trustReadError !== null ? `Trust entries unverifiable: ${trustReadError}` : null + } else if ( + missing.length === 0 && + trustMissing.length === 0 && + disabled.length === 0 && + trustReadError === null + ) { + state = 'installed' detail = null } else { state = 'partial' - detail = `Managed hook missing for events: ${missing.join(', ')}` + const parts: string[] = [] + if (missing.length > 0) { + parts.push(`Managed hook missing for events: ${missing.join(', ')}`) + } + if (trustReadError !== null) { + parts.push(`Trust entries unverifiable: ${trustReadError}`) + } else if (trustMissing.length > 0) { + parts.push(`Trust entry missing or stale for events: ${trustMissing.join(', ')}`) + } + if (disabled.length > 0) { + parts.push(`Managed hook disabled for events: ${disabled.join(', ')}`) + } + detail = parts.join('; ') } return { agent: 'codex', state, configPath, managedHooksPresent, detail } } @@ -191,6 +284,11 @@ export class CodexHookService { } } + // Why: Codex 0.129+ requires a per-hook trust entry in config.toml or the + // hook sits in the "review required" pile. We compute the trust hash for + // each managed entry as we install it and persist it alongside hooks.json + // so the user does not have to /hooks-approve after every install. + const trustEntries: CodexTrustEntry[] = [] for (const eventName of CODEX_EVENTS) { const current = Array.isArray(nextHooks[eventName]) ? nextHooks[eventName] : [] const cleaned = removeManagedCommands(current, isManagedCommand) @@ -198,11 +296,36 @@ export class CodexHookService { hooks: [{ type: 'command', command }] } nextHooks[eventName] = [...cleaned, definition] + // Why: our managed definition is appended after `cleaned`, so its + // group index in the resulting hooks.json is `cleaned.length`. The + // handler is always the first (and only) entry in the group, so + // handler index is 0. Codex's hook_key uses these positional indices. + trustEntries.push({ + sourcePath: configPath, + eventLabel: CODEX_EVENT_LABEL[eventName], + groupIndex: cleaned.length, + handlerIndex: 0, + command + }) } config.hooks = nextHooks writeManagedScript(scriptPath, getManagedScript()) writeHooksJson(configPath, config) + // Why: trust entries write last so a half-write can't leave a hash + // pointing at a hook that doesn't exist. Surface failures — without this, + // getStatus would report green for a hook Codex won't actually fire. + try { + upsertHookTrustEntries(getCodexConfigTomlPath(), trustEntries) + } catch (error) { + return { + agent: 'codex', + state: 'error', + configPath, + managedHooksPresent: true, + detail: `Hooks installed but trust entries could not be written: ${error instanceof Error ? error.message : String(error)}. Run /hooks in Codex to approve.` + } + } return this.getStatus() } @@ -239,6 +362,56 @@ export class CodexHookService { } config.hooks = nextHooks writeHooksJson(configPath, config) + + // Why: also drop our trust entries so config.toml doesn't accumulate dead + // [hooks.state."..."] blocks across install/remove cycles. Best-effort — + // a stale entry is harmless once hooks.json no longer references it. + try { + const tomlPath = getCodexConfigTomlPath() + const existingEntries = readHookTrustEntries(tomlPath) + const scriptPath = getManagedScriptPath() + const command = getManagedCommand(scriptPath) + const managedEventLabels = new Set( + CODEX_EVENTS.map((event) => CODEX_EVENT_LABEL[event]) + ) + // Why: only drop entries WE wrote. configPath (~/.codex/hooks.json) is + // shared with Codex CLI, so user-approved trust entries for non-Orca + // commands live in the same `[hooks.state.*]` namespace. Match by hash + // equivalence to our managed command — a sourcePath-only filter would + // wipe the user's manually-approved entries. + const ourKeys: string[] = [] + for (const [key, state] of existingEntries) { + const parts = parseTrustKey(key) + if (parts === null) { + continue + } + if (parts.sourcePath !== configPath) { + continue + } + if (!managedEventLabels.has(parts.eventLabel)) { + continue + } + const expectedHash = computeTrustedHash({ + sourcePath: configPath, + eventLabel: parts.eventLabel, + groupIndex: parts.groupIndex, + handlerIndex: parts.handlerIndex, + command + }) + if (state.trustedHash !== expectedHash) { + continue + } + ourKeys.push(key) + } + if (ourKeys.length > 0) { + removeHookTrustEntries(tomlPath, ourKeys) + } + } catch (error) { + // Best effort — stale trust entries are harmless once hooks.json no + // longer references the hook. Log so a programmer error doesn't disappear silently. + console.warn('[codex-hook-service] failed to clean trust entries', error) + } + return this.getStatus() } }