diff --git a/config/tsconfig.cli.json b/config/tsconfig.cli.json index 49c8d52b4a9..3ebd71aa41f 100644 --- a/config/tsconfig.cli.json +++ b/config/tsconfig.cli.json @@ -13,6 +13,7 @@ "../src/main/claude/hook-settings.ts", "../src/main/claude/hook-service.ts", "../src/main/codex/codex-app-server-capability-cache.ts", + "../src/main/codex/codex-app-server-capability-signal.ts", "../src/main/codex/codex-app-server-client.ts", "../src/main/codex/codex-app-server-grant-bridge.ts", "../src/main/codex/codex-app-server-grant-envelope.ts", diff --git a/src/main/codex/codex-app-server-capability-signal.ts b/src/main/codex/codex-app-server-capability-signal.ts new file mode 100644 index 00000000000..a35518f0ded --- /dev/null +++ b/src/main/codex/codex-app-server-capability-signal.ts @@ -0,0 +1,7 @@ +// Why: only an argv parse failure that names app-server proves the RPC surface +// is absent; unrelated stderr must remain transient rather than poison the cache. +export function stderrIndicatesMissingAppServer(stderrTail: string): boolean { + return /^(?=.*\bapp-server\b).*(?:unrecognized subcommand|unexpected argument|invalid subcommand)/im.test( + stderrTail + ) +} diff --git a/src/main/codex/codex-app-server-client.test.ts b/src/main/codex/codex-app-server-client.test.ts index 5bb01ae5614..804cd50ae82 100644 --- a/src/main/codex/codex-app-server-client.test.ts +++ b/src/main/codex/codex-app-server-client.test.ts @@ -49,6 +49,11 @@ if (config.scenario === 'no-subcommand') { process.stderr.write("error: unrecognized subcommand 'app-server'\\n") process.exit(2) } +if (config.scenario === 'transient-stderr') { + process.stderr.write('config loader: unexpected argument in value\\n') + process.exit(1) +} +if (config.scenario === 'silent-exit-0') process.exit(0) process.stdin.setEncoding('utf8') process.stdin.on('data', (chunk) => { buffer += chunk @@ -278,6 +283,32 @@ describe('runCodexHookTrustGrantSession', () => { expect(isCodexAppServerUnsupportedError(error)).toBe(true) }) + it('does not classify unrelated stderr as a missing app-server capability', async () => { + const { request } = createStubRequest({ + scenario: 'transient-stderr', + hooks: [], + expectedTrustKeys: ['k'], + managedCommand: MANAGED_COMMAND + }) + + const error = await runCodexHookTrustGrantSession(request).catch((caught: unknown) => caught) + expect(error).toBeInstanceOf(Error) + expect(isCodexAppServerUnsupportedError(error)).toBe(false) + }) + + it('treats a silent exit zero before initialize as transient', async () => { + const { request } = createStubRequest({ + scenario: 'silent-exit-0', + hooks: [], + expectedTrustKeys: ['k'], + managedCommand: MANAGED_COMMAND + }) + + const error = await runCodexHookTrustGrantSession(request).catch((caught: unknown) => caught) + expect(error).toBeInstanceOf(Error) + expect(isCodexAppServerUnsupportedError(error)).toBe(false) + }) + it('kills a hung server at the session deadline', async () => { const keys = ['/home/a/.codex/hooks.json:session_start:0:0'] const { request, pidFile } = createStubRequest({ diff --git a/src/main/codex/codex-app-server-client.ts b/src/main/codex/codex-app-server-client.ts index 3fd8e538a9f..4e58c8348cc 100644 --- a/src/main/codex/codex-app-server-client.ts +++ b/src/main/codex/codex-app-server-client.ts @@ -1,13 +1,7 @@ -import { spawn } from 'node:child_process' +import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process' import { normalizeHookTrustKeyForLookup } from './config-toml-trust' -import { runCodexAppServerSession, type CodexAppServerInvocation } from './codex-app-server-session' - -export { - CodexAppServerTimeoutError, - CodexAppServerUnsupportedError, - isCodexAppServerUnsupportedError -} from './codex-app-server-session' -export type { CodexAppServerInvocation } from './codex-app-server-session' +import { waitForProcessExitUntil } from './codex-process-exit-deadline' +import { stderrIndicatesMissingAppServer } from './codex-app-server-capability-signal' // Why: Codex gates hooks on a `trusted_hash` it computes from a private // canonical-JSON identity. Orca used to replicate that algorithm @@ -19,6 +13,15 @@ export type { CodexAppServerInvocation } from './codex-app-server-session' // Codex as the only hash authority. See upstream codex-rs/tui/src/hooks_rpc.rs // and codex-rs/tui/src/startup_hooks_review.rs. +export type CodexAppServerInvocation = { + command: string + args: string[] + /** Overlay applied on top of the inherited environment (e.g. CODEX_HOME). */ + env?: Record + /** Whole-session deadline. The codex child is SIGKILLed when it lapses. */ + timeoutMs: number +} + export type CodexHookTrustGrantRequest = { invocation: CodexAppServerInvocation /** cwd passed to hooks/list. Discovery of the managed CODEX_HOME's @@ -50,15 +53,49 @@ export type CodexHookTrustGrantSessionResult = } | { outcome: 'verify-failed'; reason: string } -export type CodexHookListing = { +/** Codex-side absence of the trust-grant RPC surface (old CLI without the + * app-server subcommand, or a server without hooks/list / config/batchWrite). + * This is the ONLY error class the capability cache marks unsupported. */ +export class CodexAppServerUnsupportedError extends Error { + constructor(message: string) { + super(message) + this.name = 'CodexAppServerUnsupportedError' + } +} + +export class CodexAppServerTimeoutError extends Error { + constructor(message: string) { + super(message) + this.name = 'CodexAppServerTimeoutError' + } +} + +export function isCodexAppServerUnsupportedError(error: unknown): boolean { + return error instanceof Error && error.name === 'CodexAppServerUnsupportedError' +} + +type JsonRpcResponse = { + id?: number + result?: unknown + error?: { code?: number; message?: string } +} + +type CodexHookListing = { key: string command: string | null currentHash: string trustStatus: string - enabled: boolean } -export function collectCodexHookListings(result: unknown): CodexHookListing[] { +const JSON_RPC_METHOD_NOT_FOUND = -32601 +const STDERR_TAIL_MAX_BYTES = 8192 +const STDOUT_LINE_MAX_BYTES = 1024 * 1024 + +function isMethodNotFoundError(error: { code?: number; message?: string }): boolean { + return error.code === JSON_RPC_METHOD_NOT_FOUND || /method not found/i.test(error.message ?? '') +} + +function collectHookListings(result: unknown): CodexHookListing[] { const data = result && typeof result === 'object' && Array.isArray((result as { data?: unknown }).data) ? ((result as { data: unknown[] }).data as { hooks?: unknown }[]) @@ -85,8 +122,7 @@ export function collectCodexHookListings(result: unknown): CodexHookListing[] { key: hook.key, command: typeof hook.command === 'string' ? hook.command : null, currentHash: hook.currentHash, - trustStatus: hook.trustStatus, - enabled: typeof hook.enabled === 'boolean' ? hook.enabled : true + trustStatus: hook.trustStatus }) } } @@ -103,59 +139,232 @@ export async function runCodexHookTrustGrantSession( request: CodexHookTrustGrantRequest, spawnImpl: typeof spawn = spawn ): Promise { - return runCodexAppServerSession( - request.invocation, - async (requestRpc) => { - const expectedKeys = new Set(request.expectedTrustKeys) - const matchManaged = (listing: CodexHookListing): boolean => - listing.command === request.managedCommand && - expectedKeys.has(normalizeHookTrustKeyForLookup(listing.key)) + const { invocation } = request + const child = spawnImpl(invocation.command, invocation.args, { + env: { ...process.env, ...invocation.env }, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true + }) as ChildProcessWithoutNullStreams - const listResult = await requestRpc('hooks/list', { cwds: [request.hooksListCwd] }) - const managedListings = collectCodexHookListings(listResult).filter(matchManaged) - if (managedListings.length !== expectedKeys.size) { - return { - outcome: 'verify-failed', - reason: `hooks/list reported ${managedListings.length} of ${expectedKeys.size} expected managed entries` - } - } + let stderrTail = '' + let exited = false + let nextRequestId = 1 + let timedOut = false + const pending = new Map< + number, + { resolve: (r: JsonRpcResponse) => void; reject: (e: Error) => void } + >() - const needingTrust = managedListings.filter((listing) => listing.trustStatus !== 'trusted') - if (needingTrust.length > 0) { - // Why: same wire shape as the Codex TUI "Trust all" flow — one upsert - // edit under hooks.state with each key's Codex-computed current hash. - const value: Record = {} - for (const listing of needingTrust) { - value[listing.key] = { trusted_hash: listing.currentHash } - } - await requestRpc('config/batchWrite', { - edits: [{ keyPath: 'hooks.state', value, mergeStrategy: 'upsert' }], - reloadUserConfig: true - }) - } + const exitPromise = new Promise((resolve) => { + child.on('exit', () => { + exited = true + resolve() + }) + }) + // Why: 'error' fires instead of 'exit' when the spawn itself fails + // (ENOENT); surface it to every in-flight request or they wait forever. + let spawnError: Error | null = null + child.on('error', (error) => { + spawnError = error + exited = true + failPending(error) + }) + // Why: 'close' (not 'exit') guarantees the stderr tail is complete, so an + // early death classifies correctly as missing-subcommand vs transient. + child.on('close', () => { + failPending(buildEarlyExitError()) + }) + // 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. + child.stdin.on('error', (error) => { + failPending(error) + }) - const verifyResult = await requestRpc('hooks/list', { cwds: [request.hooksListCwd] }) - const verifiedListings = collectCodexHookListings(verifyResult).filter(matchManaged) - const untrusted = verifiedListings.filter((listing) => listing.trustStatus !== 'trusted') - if (verifiedListings.length !== expectedKeys.size || untrusted.length > 0) { - return { - outcome: 'verify-failed', - reason: - untrusted.length > 0 - ? `post-grant verify left ${untrusted.length} entries ${untrusted[0].trustStatus}` - : `post-grant verify reported ${verifiedListings.length} of ${expectedKeys.size} entries` - } + let stdoutBuffer = '' + 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')) + return + } + let newlineIndex + while ((newlineIndex = stdoutBuffer.indexOf('\n')) !== -1) { + const line = stdoutBuffer.slice(0, newlineIndex).trim() + stdoutBuffer = stdoutBuffer.slice(newlineIndex + 1) + if (!line) { + continue } + let message: JsonRpcResponse + try { + message = JSON.parse(line) as JsonRpcResponse + } catch { + continue + } + if (typeof message.id === 'number' && pending.has(message.id)) { + const waiter = pending.get(message.id)! + pending.delete(message.id) + waiter.resolve(message) + } + } + }) + + function failPending(error: Error): void { + for (const waiter of pending.values()) { + waiter.reject(error) + } + pending.clear() + } + + const deadline = setTimeout(() => { + timedOut = true + child.kill('SIGKILL') + failPending( + new CodexAppServerTimeoutError( + `codex app-server session exceeded ${invocation.timeoutMs}ms (${invocation.command})` + ) + ) + }, invocation.timeoutMs) + + function sendLine(payload: Record): void { + child.stdin.write(`${JSON.stringify(payload)}\n`) + } + + async function requestRpc(method: string, params?: Record): Promise { + if (spawnError) { + throw spawnError + } + if (timedOut) { + throw new CodexAppServerTimeoutError('codex app-server session already timed out') + } + if (exited) { + throw buildEarlyExitError() + } + const id = nextRequestId++ + const response = await new Promise((resolve, reject) => { + pending.set(id, { resolve, reject }) + const payload: Record = { method, id } + if (params !== undefined) { + payload.params = params + } + try { + sendLine(payload) + } catch (error) { + pending.delete(id) + reject(error instanceof Error ? error : new Error(String(error))) + } + }) + if (response.error) { + if (isMethodNotFoundError(response.error)) { + throw new CodexAppServerUnsupportedError( + `codex app-server does not support ${method}: ${response.error.message ?? 'method not found'}` + ) + } + throw new Error( + `codex app-server ${method} failed: ${response.error.message ?? 'unknown error'}` + ) + } + return response.result + } + + function buildEarlyExitError(): Error { + if (stderrIndicatesMissingAppServer(stderrTail)) { + return new CodexAppServerUnsupportedError( + `codex CLI does not support the app-server subcommand: ${stderrTail.trim().slice(0, 400)}` + ) + } + return new Error( + `codex app-server exited before completing the session${stderrTail ? `: ${stderrTail.trim().slice(0, 400)}` : ''}` + ) + } + + try { + await requestRpc('initialize', { + clientInfo: { name: 'orca_desktop', title: 'Orca', version: '0.0.0' } + }) + sendLine({ method: 'initialized' }) + + const expectedKeys = new Set(request.expectedTrustKeys) + const matchManaged = (listing: CodexHookListing): boolean => + listing.command === request.managedCommand && + expectedKeys.has(normalizeHookTrustKeyForLookup(listing.key)) + + const listResult = await requestRpc('hooks/list', { cwds: [request.hooksListCwd] }) + const managedListings = collectHookListings(listResult).filter(matchManaged) + if (managedListings.length !== expectedKeys.size) { return { - outcome: 'granted', - wroteTrust: needingTrust.length > 0, - entries: verifiedListings.map((listing) => ({ - key: listing.key, - normalizedKey: normalizeHookTrustKeyForLookup(listing.key), - trustedHash: listing.currentHash - })) + outcome: 'verify-failed', + reason: `hooks/list reported ${managedListings.length} of ${expectedKeys.size} expected managed entries` } - }, - spawnImpl - ) + } + + const needingTrust = managedListings.filter((listing) => listing.trustStatus !== 'trusted') + if (needingTrust.length > 0) { + // Why: same wire shape as the Codex TUI "Trust all" flow — one upsert + // edit under hooks.state with each key's Codex-computed current hash. + const value: Record = {} + for (const listing of needingTrust) { + value[listing.key] = { trusted_hash: listing.currentHash } + } + await requestRpc('config/batchWrite', { + edits: [{ keyPath: 'hooks.state', value, mergeStrategy: 'upsert' }], + reloadUserConfig: true + }) + } + + const verifyResult = await requestRpc('hooks/list', { cwds: [request.hooksListCwd] }) + const verifiedListings = collectHookListings(verifyResult).filter(matchManaged) + const untrusted = verifiedListings.filter((listing) => listing.trustStatus !== 'trusted') + if (verifiedListings.length !== expectedKeys.size || untrusted.length > 0) { + return { + outcome: 'verify-failed', + reason: + untrusted.length > 0 + ? `post-grant verify left ${untrusted.length} entries ${untrusted[0].trustStatus}` + : `post-grant verify reported ${verifiedListings.length} of ${expectedKeys.size} entries` + } + } + return { + outcome: 'granted', + wroteTrust: needingTrust.length > 0, + entries: verifiedListings.map((listing) => ({ + key: listing.key, + normalizedKey: normalizeHookTrustKeyForLookup(listing.key), + trustedHash: listing.currentHash + })) + } + } catch (error) { + if ( + error instanceof Error && + !(error instanceof CodexAppServerUnsupportedError) && + !(error instanceof CodexAppServerTimeoutError) && + stderrIndicatesMissingAppServer(stderrTail) + ) { + throw new CodexAppServerUnsupportedError( + `codex CLI does not support the app-server subcommand: ${stderrTail.trim().slice(0, 400)}` + ) + } + throw error + } finally { + try { + child.stdin.end() + } catch { + // stdin may already be destroyed after a kill; reaping below still runs. + } + if (!exited) { + // Why: the server exits promptly on stdin EOF; the grace period only + // bounds a wedged child before the guaranteed SIGKILL reap. + await waitForProcessExitUntil(exitPromise, 1500) + if (!exited) { + child.kill('SIGKILL') + await waitForProcessExitUntil(exitPromise, 1000) + } + } + clearTimeout(deadline) + } } diff --git a/src/main/codex/codex-user-hook-trust-rebase-client.ts b/src/main/codex/codex-user-hook-trust-rebase-client.ts index c749946064f..ea27671e9fb 100644 --- a/src/main/codex/codex-user-hook-trust-rebase-client.ts +++ b/src/main/codex/codex-user-hook-trust-rebase-client.ts @@ -1,7 +1,49 @@ -import { collectCodexHookListings, type CodexHookListing } from './codex-app-server-client' import { runCodexAppServerSession, type CodexAppServerInvocation } from './codex-app-server-session' import { normalizeHookTrustKeyForLookup } from './config-toml-trust' +type CodexHookListing = { + key: string + command: string | null + currentHash: string + trustStatus: string + enabled: boolean +} + +function collectCodexHookListings(result: unknown): CodexHookListing[] { + const data = + result && typeof result === 'object' && Array.isArray((result as { data?: unknown }).data) + ? ((result as { data: unknown[] }).data as { hooks?: unknown }[]) + : [] + const listings: CodexHookListing[] = [] + const seenKeys = new Set() + for (const entry of data) { + const hooks = Array.isArray(entry?.hooks) ? entry.hooks : [] + for (const hook of hooks as Record[]) { + if ( + typeof hook?.key !== 'string' || + typeof hook.currentHash !== 'string' || + typeof hook.trustStatus !== 'string' + ) { + continue + } + // Why: hooks/list repeats user-scope hooks per requested cwd; trust + // rebasing must consider each key once. + if (seenKeys.has(hook.key)) { + continue + } + seenKeys.add(hook.key) + listings.push({ + key: hook.key, + command: typeof hook.command === 'string' ? hook.command : null, + currentHash: hook.currentHash, + trustStatus: hook.trustStatus, + enabled: typeof hook.enabled === 'boolean' ? hook.enabled : true + }) + } + } + return listings +} + export type CodexUserHookTrustMove = { oldKey: string newKey: string