From b5eed3341ff2b4ee199c3d6d802b69f32242cd97 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Thu, 7 May 2026 16:10:30 -0700 Subject: [PATCH] fix(codex-trust): harden TOML edits, status reporting, and key parsing Four review iterations surfaced correctness issues in the new Codex hook trust path. Notably: CRLF-terminated headers slipped past the upsert regex (Windows would accumulate duplicate blocks), readHookTrustEntries falsely matched headers inside multi-line strings, install() ignored trust-write failures and reported green status, and parseTrustKey accepted malformed keys because Number('') === 0. - TOML scanner: handle CRLF, escape-aware triple-quote counting, literal-string and basic-string quoted keys with embedded ']', and [[array.of.tables]] block boundaries. - escapeTomlString covers \b\f\n\r\t and unescapeTomlString preserves unknown escapes. - New exports: parseTrustKey (strict canonical-int validation), readHookTrustEntries, removeHookTrustEntries. - install(): catch trust-write failures and surface as 'error' state. - getStatus(): verify trust hash + enabled flag for the LAST matching managed group; surface unreadable config.toml as 'partial' so hooks.json health is still visible to the user. - remove(): clean up our [hooks.state."..."] blocks via parseTrustKey. Tests: 46 passing (up from 12), covering CRLF, multi-line strings, literal-string keys, array-of-tables boundaries, parseTrustKey edge cases, and the new read/remove APIs. Co-authored-by: Orca --- src/main/codex/config-toml-trust.test.ts | 569 +++++++++++++++++++++++ src/main/codex/config-toml-trust.ts | 342 +++++++++++++- src/main/codex/hook-service.ts | 140 +++++- 3 files changed, 1021 insertions(+), 30 deletions(-) diff --git a/src/main/codex/config-toml-trust.test.ts b/src/main/codex/config-toml-trust.test.ts index 0ab88d0844c..f4a80db164c 100644 --- a/src/main/codex/config-toml-trust.test.ts +++ b/src/main/codex/config-toml-trust.test.ts @@ -1,3 +1,4 @@ +/* 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' @@ -5,6 +6,9 @@ import { join } from 'path' import { computeTrustKey, computeTrustedHash, + parseTrustKey, + readHookTrustEntries, + removeHookTrustEntries, upsertHookTrustEntries, type CodexTrustEntry } from './config-toml-trust' @@ -113,6 +117,92 @@ describe('computeTrustedHash', () => { }) 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', () => { @@ -256,4 +346,483 @@ describe('upsertHookTrustEntries', () => { 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"]` + ) + }) +}) + +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('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') + }) +}) + +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) + }) +}) + +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 index 05e10ef95b0..b70f49fc47d 100644 --- a/src/main/codex/config-toml-trust.ts +++ b/src/main/codex/config-toml-trust.ts @@ -1,3 +1,4 @@ +/* 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, @@ -49,6 +50,11 @@ export type CodexTrustEntry = { 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 { @@ -95,6 +101,70 @@ 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: 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 @@ -126,8 +196,17 @@ function buildTrustBlock(key: string, hash: string): string { ].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('"', '\\"') + 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 { @@ -160,27 +239,52 @@ function upsertTrustBlock(content: string, key: string, hash: string): string { // line. 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. function buildHeaderPattern(key: string): RegExp { const escapedKey = escapeRegex(escapeTomlString(key)) - return new RegExp(`(^|\\n)\\[hooks\\.state\\."${escapedKey}"\\][ \\t]*\\n`) + return new RegExp(`(^|\\r?\\n)\\[hooks\\.state\\."${escapedKey}"\\][ \\t]*\\r?\\n`) } function escapeRegex(value: string): string { return value.replaceAll(/[.*+?^${}()|[\]\\]/g, '\\$&') } -// Why: scan for the next top-level `[name]` header. Skip array-of-tables -// `[[name]]`. We only treat lines whose first non-whitespace character is -// `[` and that close cleanly with `]` (modulo trailing comment/whitespace) -// as headers, so `[` inside string values is ignored. +// 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 line = text.slice(cursor, lineEnd).trimStart() - if (line.startsWith('[') && !line.startsWith('[[') && /^\[[^\]]*\]\s*(#.*)?$/.test(line)) { - return cursor + 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 @@ -190,6 +294,96 @@ function findNextTableHeader(text: string): number { 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 @@ -216,3 +410,133 @@ function writeConfigAtomically(configPath: string, contents: string): void { } } } + +export function removeHookTrustEntries(configPath: string, keys: readonly string[]): void { + if (!existsSync(configPath)) { + return + } + const existing = readFileSync(configPath, 'utf-8') + 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: cut from match.index (which includes the captured leading newline) + // so we don't strand a blank line where our block used to live. + const cutStart = match.index + 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 = readFileSync(configPath, 'utf-8') + // Why: walk line-by-line so `[hooks.state."..."]` inside a `"""..."""` or + // `'''...'''` multi-line string isn't mistaken for a real header. + const headerLineRegex = /^[ \t]*\[hooks\.state\."((?:[^"\\]|\\.)*)"\][ \t]*$/ + 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 16f4674f972..b698a9ee87b 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -12,8 +12,14 @@ import { type HookDefinition } from '../agent-hooks/installer-utils' import { + computeTrustKey, + computeTrustedHash, + parseTrustKey, + readHookTrustEntries, + removeHookTrustEntries, upsertHookTrustEntries, type CodexEventLabel, + type CodexHookTrustState, type CodexTrustEntry } from './config-toml-trust' @@ -130,37 +136,91 @@ 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 + definitions.forEach((definition, idx) => { + const hooks = definition.hooks ?? [] + if (hooks.some((hook) => hook.command === command)) { + foundGroupIndex = idx + } + }) + 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. + const trustInput: CodexTrustEntry = { + sourcePath: configPath, + eventLabel: CODEX_EVENT_LABEL[eventName], + groupIndex: foundGroupIndex, + handlerIndex: 0, + 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' detail = 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 } } @@ -240,11 +300,20 @@ export class CodexHookService { config.hooks = nextHooks writeManagedScript(scriptPath, getManagedScript()) writeHooksJson(configPath, config) - // Why: write trust entries AFTER hooks.json so a partial install (script - // written, hooks.json not) cannot leave a trust hash pointing at a hook - // that does not exist. The reverse order would be benign but this keeps - // the invariant clear. - upsertHookTrustEntries(getCodexConfigTomlPath(), trustEntries) + // 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() } @@ -281,6 +350,35 @@ 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 existingKeys = readHookTrustEntries(tomlPath) + // Why: only drop entries WE could have written — same configPath, + // recognized event label. parseTrustKey returns null for malformed or + // unrelated keys, keeping the shape definition centralized. + const ourKeys: string[] = [] + for (const key of existingKeys.keys()) { + const parts = parseTrustKey(key) + if (parts === null) { + continue + } + if (parts.sourcePath !== configPath) { + continue + } + ourKeys.push(key) + } + if (ourKeys.length > 0) { + removeHookTrustEntries(tomlPath, ourKeys) + } + } catch { + // Best effort — stale trust entries are harmless once hooks.json no + // longer references the hook. + } + return this.getStatus() } }