From 1581e324a9d5778d9c9e59041ff61c8a471f5036 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 15 Jul 2026 15:07:01 -0700 Subject: [PATCH] fix(codex): preserve unicode trust RPC responses --- .../codex/codex-app-server-client.test.ts | 31 +++++++++++++++- src/main/codex/codex-app-server-client.ts | 10 +++--- src/main/codex/codex-trust-grant-host.ts | 13 ++++--- .../codex/hook-service-trust-grant.test.ts | 15 ++++++-- src/main/codex/hook-service.ts | 35 ++++++++++++++++--- 5 files changed, 88 insertions(+), 16 deletions(-) diff --git a/src/main/codex/codex-app-server-client.test.ts b/src/main/codex/codex-app-server-client.test.ts index 2e263f12384..82df78a57d2 100644 --- a/src/main/codex/codex-app-server-client.test.ts +++ b/src/main/codex/codex-app-server-client.test.ts @@ -22,7 +22,19 @@ const config = JSON.parse(process.env.STUB_CONFIG) require('node:fs').writeFileSync(config.pidFile, String(process.pid)) const trusted = new Set(config.hooks.filter(h => h.trustStatus === 'trusted').map(h => h.key)) let buffer = '' -function send(message) { process.stdout.write(JSON.stringify(message) + '\\n') } +function send(message) { + const serialized = Buffer.from(JSON.stringify(message) + '\\n') + if (config.scenario === 'split-unicode') { + const marker = Buffer.from('é') + const markerIndex = serialized.indexOf(marker) + if (markerIndex !== -1) { + process.stdout.write(serialized.subarray(0, markerIndex + 1)) + setTimeout(() => process.stdout.write(serialized.subarray(markerIndex + 1)), 5) + return + } + } + process.stdout.write(serialized) +} function listing() { return { data: [{ @@ -212,6 +224,23 @@ describe('runCodexHookTrustGrantSession', () => { expect(result.outcome).toBe('verify-failed') }) + it('decodes JSONL when a non-ASCII hook path is split across stdout chunks', async () => { + const command = "/bin/sh '/tmp/rené/codex-hook.sh'" + const key = '/home/rené/.codex/hooks.json:session_start:0:0' + const { request } = createStubRequest({ + scenario: 'split-unicode', + hooks: [{ ...managedHook(key), command }], + expectedTrustKeys: [key], + managedCommand: command, + timeoutMs: 2_000 + }) + + await expect(runCodexHookTrustGrantSession(request)).resolves.toMatchObject({ + outcome: 'granted', + entries: [{ key }] + }) + }) + it('throws the unsupported error class for unknown JSON-RPC methods', async () => { const keys = ['/home/a/.codex/hooks.json:session_start:0:0'] const { request } = createStubRequest({ diff --git a/src/main/codex/codex-app-server-client.ts b/src/main/codex/codex-app-server-client.ts index e3aa69117bc..0273893fcc8 100644 --- a/src/main/codex/codex-app-server-client.ts +++ b/src/main/codex/codex-app-server-client.ts @@ -178,8 +178,10 @@ export async function runCodexHookTrustGrantSession( child.on('close', () => { failPending(buildEarlyExitError()) }) - child.stderr.on('data', (chunk: Buffer) => { - stderrTail = (stderrTail + chunk.toString('utf8')).slice(-STDERR_TAIL_MAX_BYTES) + // Why: JSONL can contain non-ASCII hook paths. Stream decoding must retain a + // multibyte character split across pipe chunks or the response becomes invalid JSON. + child.stderr.setEncoding('utf8').on('data', (chunk: string) => { + stderrTail = (stderrTail + chunk).slice(-STDERR_TAIL_MAX_BYTES) }) // Why: a child can exit between the liveness check and stdin.write(); an // EPIPE must reject the RPC instead of becoming an unhandled stream error. @@ -188,8 +190,8 @@ export async function runCodexHookTrustGrantSession( }) let stdoutBuffer = '' - child.stdout.on('data', (chunk: Buffer) => { - stdoutBuffer += chunk.toString('utf8') + child.stdout.setEncoding('utf8').on('data', (chunk: string) => { + stdoutBuffer += chunk if (Buffer.byteLength(stdoutBuffer) > STDOUT_LINE_MAX_BYTES) { child.kill('SIGKILL') failPending(new Error('codex app-server emitted an oversized JSONL response')) diff --git a/src/main/codex/codex-trust-grant-host.ts b/src/main/codex/codex-trust-grant-host.ts index bf51c800e10..63cd81eb425 100644 --- a/src/main/codex/codex-trust-grant-host.ts +++ b/src/main/codex/codex-trust-grant-host.ts @@ -82,10 +82,15 @@ export function readCurrentCodexTrustGrantLedgerHome( host: CodexTrustGrantHost ): CodexTrustGrantLedgerHome | null { try { - return readCodexTrustGrantLedgerHomeMatchingStamp( - runtimeHomePath, - resolveCodexTrustGrantHost(host).binaryStamp - ) + const home = readCodexTrustGrantLedgerHome(runtimeHomePath) + if (!home) { + // Why: fallback-only installs have no ledger. Avoid a synchronous PATH + // and version-manager scan when there is no recorded stamp to validate. + return null + } + return binaryStampsMatch(home.binary, resolveCodexTrustGrantHost(host).binaryStamp) + ? home + : null } catch { // Why: status is diagnostic and best-effort; unreadable ledger/binary // paths must trigger conservative self-hash handling, not throw. diff --git a/src/main/codex/hook-service-trust-grant.test.ts b/src/main/codex/hook-service-trust-grant.test.ts index b317f5b3b65..4c51040fc38 100644 --- a/src/main/codex/hook-service-trust-grant.test.ts +++ b/src/main/codex/hook-service-trust-grant.test.ts @@ -18,9 +18,10 @@ import { } from './codex-trust-grant-ledger' import type { CodexHookTrustGrantRequest } from './codex-app-server-client' -const { getPathMock, homedirMock } = vi.hoisted(() => ({ +const { getPathMock, homedirMock, resolveCodexCommandMock } = vi.hoisted(() => ({ getPathMock: vi.fn<(name: string) => string>(), - homedirMock: vi.fn<() => string>() + homedirMock: vi.fn<() => string>(), + resolveCodexCommandMock: vi.fn<() => string>() })) vi.mock('electron', () => ({ app: { getPath: getPathMock } })) @@ -28,6 +29,7 @@ vi.mock('os', async (importOriginal) => { const actual = await importOriginal() return { ...actual, homedir: homedirMock } }) +vi.mock('../codex-cli/command', () => ({ resolveCodexCommand: resolveCodexCommandMock })) import { CodexHookService } from './hook-service' @@ -41,6 +43,7 @@ beforeEach(() => { previousUserDataPath = process.env.ORCA_USER_DATA_PATH process.env.ORCA_USER_DATA_PATH = userDataDir homedirMock.mockReturnValue(tmpHome) + resolveCodexCommandMock.mockReturnValue(process.execPath) getPathMock.mockImplementation((name: string) => { if (name === 'userData') { return userDataDir @@ -133,6 +136,9 @@ describe('CodexHookService app-server trust grant lane', () => { expect(service.install().state).toBe('installed') expect(runner).toHaveBeenCalledTimes(1) + // Why: each launch validates the binary stamp once; getStatus reuses the + // just-verified grant instead of repeating PATH/version-manager scans. + expect(resolveCodexCommandMock).toHaveBeenCalledTimes(2) expect(readFileSync(join(managedHome, 'config.toml'))).toEqual(firstToml) }) @@ -204,8 +210,11 @@ describe('CodexHookService app-server trust grant lane', () => { const runner = vi.fn() trustGrantInternals.setGrantSessionRunnerSync(runner) - expect(new CodexHookService().install().state).toBe('installed') + const service = new CodexHookService() + expect(service.install().state).toBe('installed') + expect(service.getStatus().state).toBe('installed') expect(runner).not.toHaveBeenCalled() + expect(resolveCodexCommandMock).not.toHaveBeenCalled() }) it('restores exact config bytes before fallback after a mutating RPC failure', () => { diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index 522c37f84e1..b1b3ac7a3b2 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -1035,6 +1035,12 @@ export class CodexHookService { } getStatus(): AgentHookInstallStatus { + return this.getStatusAfterInstall(null) + } + + private getStatusAfterInstall( + recentGrantEntries: readonly CodexTrustEntry[] | null + ): AgentHookInstallStatus { const configPath = getConfigPath() const scriptPath = getManagedScriptPath() const config = readHooksJson(configPath) @@ -1068,9 +1074,21 @@ export class CodexHookService { // Why: RPC-granted entries store Codex's own hash, which is authoritative // even when it differs from computeTrustedHash — that difference is the // drift bug class this lane exists to absorb, not a stale entry. - const ledgerHome = readCurrentCodexTrustGrantLedgerHome(getOrcaManagedCodexHomePath(), { - kind: 'native' - }) + // Why: install() already resolved the binary and either verified Codex's + // hashes or wrote fallback hashes. Re-resolving PATH here doubles sync launch work. + const ledgerHome = + recentGrantEntries === null + ? readCurrentCodexTrustGrantLedgerHome(getOrcaManagedCodexHomePath(), { kind: 'native' }) + : null + const recentGrantHashes = new Map() + for (const entry of recentGrantEntries ?? []) { + if (entry.trustedHash) { + recentGrantHashes.set(normalizeHookTrustKeyForLookup(computeTrustKey(entry)), { + signature: getCodexHookTrustSignature(entry), + trustedHash: entry.trustedHash + }) + } + } const missing: string[] = [] const trustMissing: string[] = [] @@ -1121,6 +1139,13 @@ export class CodexHookService { if (grantedHash) { validHashes.add(grantedHash) } + const recentGrant = recentGrantHashes.get(normalizeHookTrustKeyForLookup(trustKey)) + if ( + recentGrant?.signature === getCodexHookTrustSignature(trustInput) && + recentGrant.trustedHash + ) { + validHashes.add(recentGrant.trustedHash) + } const actualState = trustEntries.get(trustKey) if (!actualState?.trustedHash || !validHashes.has(actualState.trustedHash)) { trustMissing.push(eventName) @@ -1248,6 +1273,7 @@ export class CodexHookService { }) } const trustEntries: CodexTrustEntry[] = [...mirroredTrustEntries, ...managedTrustEntries] + let recentGrantEntries: readonly CodexTrustEntry[] = [] config.hooks = nextHooks writeManagedScript(scriptPath, getManagedScript()) @@ -1272,6 +1298,7 @@ export class CodexHookService { host: { kind: 'native' } }) if (grant.lane === 'rpc') { + recentGrantEntries = grant.entries upsertHookTrustEntries(tomlPath, mirroredTrustEntries) removeStaleRuntimeHookTrustEntries(tomlPath, configPath, [ ...mirroredTrustEntries, @@ -1303,7 +1330,7 @@ export class CodexHookService { } catch (error) { console.warn('[codex-hook-service] failed to clean legacy Codex hooks', error) } - return this.getStatus() + return this.getStatusAfterInstall(recentGrantEntries) } async installRemote(