mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 16:02:56 +00:00
* fix(crash-reporting): make the own-Chromium gate a real choke point
Round-3 review found the guard was not the choke point its own comments
claimed: six pid-addressed `taskkill /pid <pid> /t /f` families in main were
ungated and uninstrumented, so the stale-pid shape stayed producible and a
`selfInitiatedTreeKillCount: 0` could read as exculpatory when it was not.
- Gate the remaining main-process families: the git command-runner abort, the
notebook-cell and automation-precheck timeouts.
- Turn the `src/shared` seam into the gate itself (`process-tree-kill-gate`), so
the runProcess choke point, the codex app-server deadline kill and the
ephemeral-VM recipe kill ask the same decision. Those three are compiled into
the CLI/relay too and cannot import main; main installs the guard at preflight.
- Ratchet (`main-process-tree-kill-gate.test.ts`): a new pid-addressed taskkill
in main that skips the gate fails, and the allowlist entries must still exist.
- Give pid-addressed kills eviction priority in the 32-entry ring: 32 routine
`win-pty-job` teardowns from a window-close burst no longer evict the one
entry that discriminates a self-kill from an external one.
- Correct the coverage doc, which described the uninstrumented Windows sites as
POSIX `process.kill(-pid)` group kills and omitted the git and codex paths.
* fix(crash-reporting): keep a refused tree-kill from leaking the root it owns
A refusal must block the pid-addressed tree walk, not the termination. Five of
the six gated sites returned on refusal with no fallback, so a refused
`taskkill /pid /t /f` left git.exe, a timed-out notebook cell, an automation
precheck or an ephemeral-VM recipe running while the caller reported it stopped.
The root kill is addressed by the child handle, which cannot reach the recycled
pid the refusal is about, so it stays correct and required on that path.
Also fixes the ring eviction the scope preference introduced: with the ring
saturated by pid-addressed kills, the only non-pid-addressed entry is the one
just pushed, so the splice evicted itself and the detail came back `{}` --
byte-identical to the external-kill arm, in the window-close case the guard
exists for. Eviction now excludes the newest entry and falls back to FIFO.
Tests: refusal now asserts the root kill at all six sites, and the ring covers
the saturated-pid ordering as well as round 3's group-burst ordering.
* fix(crash-reporting): stop a refused tree-kill leaking the commit-message agent, and count call sites
Two round-5 blocking findings, both open on main and on both branches.
`killSourceControlAgentProcess` had no root-kill fallback on its win32 arm: the
taskkill was the only termination, so once the own-Chromium gate could refuse it
the promise resolved having killed nothing. Both callers do
`terminationComplete ??= killSourceControlAgentProcess(child)` and then release
the managed-home lock on that promise, so a refusal left the local Codex/Claude
commit-message agent running while the caller reported it stopped -- the
lock-contention failure the taskkill was added for. Same fix as the six sibling
sites: the handle-addressed root kill cannot reach the recycled pid the refusal
is about, so it stays correct and required on that path.
The ratchet was file-granular, not call-site granular: one gate mention anywhere
in a file exempted every taskkill in it, which left the six files that now ask
the gate ratchet-blind -- the inverse of what it is for. It now counts `/pid`
call sites against gate admissions per file, so a second ungated kill inside an
existing family fails. Keying on the `/pid` argument rather than a quoted
`taskkill` also catches a kill whose program name comes from a constant. The
three comments that claimed more than the old scan enforced now state the rule
and its two remaining blind spots.
Also: the recording in `admitSelfInitiatedTreeKill` is now wrapped the way the
`admitProcessTreeKill` seam already wraps it, with the refusal decision taken
before anything that can throw so a diagnostics failure cannot flip it; and
`orca-chromium-process-pids` documents the false-positive direction (a stale
`getAppMetrics()` entry plus pid reuse refuses a live unrelated child), which is
the mechanism the root-kill fallback exists to bound.
Tests: refusal now asserts the root kill at all seven sites; the ratchet asserts
call-site counting and the constant-program form.
* test(crash-reporting): run the own-Chromium gate against real Windows trees
Nothing on this branch had ever executed on Windows. The unit tests pin the
gate's decision against a mocked taskkill, which cannot show that the decision
does anything to a real process: that `/T /F` reaps a detached grandchild, that
a refusal leaves that tree standing, or that the handle-addressed root kill the
refusal path falls back to reaps the root while orphaning descendants.
Adds a win32-gated live test covering all four, registered in both the
`package_windows` CI lane and `WINDOWS_PACKAGE_TESTS` as
`win32-test-lane-registration` requires.
Also completes the coverage doc's "never instrumented" list, which omitted the
macOS keyboard-input-source probe's POSIX group kill in `ipc/app.ts`.
* fix(crash-reporting): pin the commit-message root kill on the Windows arm
The first Windows run of this branch found nine failures the macOS suite
cannot see: `commit-message-text-generation-test-harness` asserts
`expect(child.kill).not.toHaveBeenCalled()` on `process.platform === 'win32'`,
which is the contract the previous commit deliberately replaced — and it
branches on the real platform, so it is dead code everywhere CI runs today.
The harness now asserts the handle-addressed root kill on every platform. On
win32 it lands after the tree walk, so the expectation waits rather than reading
one tick early, and its ten call sites await it. Red against the pre-fix arm at
all seven sites; the production code is unchanged.
* test(crash-reporting): remove the Windows lane marker tree through the retrying helper
The new win32 spec teardown used a raw rmSync, which the windows-lane-tree-removal
boundary ratchet rejects — and which is exactly the EPERM the ratchet exists to
prevent, since this spec's marker directory is written by processes it has just
force-killed.
* fix(crash-reporting): only refuse pid-addressed tree walks, disclose the handle-less codex site
The own-Chromium gate refused the POSIX process-group arm of
signalProcessTree as well, which was new macOS/Linux behaviour: a stale
getAppMetrics() entry plus pid reuse would orphan a group that main reaps
today. A POSIX group only holds what Orca put in it, so the refusal is now
scoped to win-taskkill-tree and the POSIX arm is recorded and admitted like
the other group kills in main. That also drops the synchronous
getAppMetrics() read from every POSIX termination.
codex-turn-added-roots kills roots found by a table walk, so a refusal has
no handle to fall back to. Pin that the refusal is visible - crumb written,
turn reported as not cancelled - rather than fixing what cannot be fixed.
* test(crash-reporting): detach the Windows survival fixture and observe real spawns
369 lines
13 KiB
TypeScript
369 lines
13 KiB
TypeScript
import { spawn, type ChildProcess, type ChildProcessWithoutNullStreams } from 'node:child_process'
|
|
import { waitForProcessExitUntil } from './codex-process-exit-deadline'
|
|
import { stderrIndicatesMissingAppServer } from './codex-app-server-capability-signal'
|
|
import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution'
|
|
import { admitProcessTreeKill } from '../../shared/child-process/process-tree-kill-gate'
|
|
|
|
// Why: `codex app-server` is Orca's sanctioned RPC surface into Codex-owned
|
|
// state (hook trust hashes, the sqlite thread index). This module owns the
|
|
// stdio JSONL transport — spawn, handshake, framing, deadline, reap — so every
|
|
// RPC consumer (trust grant, session index heal) shares one hardened lifecycle.
|
|
|
|
export type CodexAppServerInvocation = {
|
|
command: string
|
|
args: string[]
|
|
/**
|
|
* The resolved CLI path, used to pair the CLI with the `node` it was installed
|
|
* against — without it a CLI resolved out of a version-manager directory runs
|
|
* under whatever node leads PATH and dies on a NODE_MODULE_VERSION mismatch
|
|
* (stablyai/orca#10932).
|
|
*
|
|
* Required, and `null` only for a guest-side launcher (wsl.exe) where the host
|
|
* path means nothing. Optional would let a native builder omit it and silently
|
|
* fall back to pairing against a cmd.exe wrapper with no type error.
|
|
*/
|
|
cliPath: string | null
|
|
/** Overlay applied on top of the inherited environment (e.g. CODEX_HOME). */
|
|
env?: Record<string, string>
|
|
/** Env keys stripped from the inherited environment before spawn (e.g. an
|
|
* inherited CODEX_HOME, so a default-home grant runs against the real ~/.codex). */
|
|
envToDelete?: readonly string[]
|
|
/** Whole-session deadline. The codex child is SIGKILLed when it lapses. */
|
|
timeoutMs: number
|
|
}
|
|
|
|
/** Codex-side absence of the requested app-server RPC surface (old CLI without
|
|
* the app-server subcommand, or a server without the called methods).
|
|
* This is the ONLY error class capability caches mark 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 }
|
|
}
|
|
|
|
export type CodexAppServerRpc = {
|
|
request: (method: string, params?: Record<string, unknown>) => Promise<unknown>
|
|
notify: (method: string, params?: Record<string, unknown>) => void
|
|
}
|
|
|
|
const JSON_RPC_METHOD_NOT_FOUND = -32601
|
|
const STDERR_TAIL_MAX_BYTES = 8192
|
|
const STDOUT_LINE_MAX_BYTES = 1024 * 1024
|
|
|
|
export function killCodexAppServerProcessTree(
|
|
child: Pick<ChildProcess, 'pid' | 'kill'>,
|
|
options: { platform?: NodeJS.Platform; spawnImpl?: typeof spawn } = {}
|
|
): void {
|
|
const platform = options.platform ?? process.platform
|
|
const spawnImpl = options.spawnImpl ?? spawn
|
|
if (platform === 'win32' && child.pid) {
|
|
if (
|
|
!admitProcessTreeKill({
|
|
pid: child.pid,
|
|
site: 'codex-app-server-session-deadline',
|
|
scope: 'win-taskkill-tree'
|
|
})
|
|
) {
|
|
// Refusal blocks the tree walk, not the termination: the root kill is
|
|
// handle-addressed, so it cannot reach the recycled pid we refused.
|
|
child.kill('SIGKILL')
|
|
return
|
|
}
|
|
try {
|
|
// Why: npm-installed Codex runs behind cmd.exe; killing only that wrapper
|
|
// leaves the app-server child alive after a timeout or failed shutdown.
|
|
const killer = spawnImpl('taskkill', ['/pid', String(child.pid), '/t', '/f'], {
|
|
stdio: 'ignore',
|
|
windowsHide: true
|
|
})
|
|
let fellBack = false
|
|
const killDirectChild = (): void => {
|
|
if (!fellBack) {
|
|
fellBack = true
|
|
child.kill('SIGKILL')
|
|
}
|
|
}
|
|
killer.on('error', killDirectChild)
|
|
killer.on('exit', (code) => {
|
|
if (code !== 0) {
|
|
killDirectChild()
|
|
}
|
|
})
|
|
killer.unref()
|
|
return
|
|
} catch {
|
|
// Fall through to the direct-child best effort when taskkill cannot start.
|
|
}
|
|
}
|
|
if (child.pid) {
|
|
try {
|
|
// npm/package-manager launchers insert a shim child on POSIX. Reap its
|
|
// direct descendants before signalling the wrapper itself.
|
|
const descendants = spawnImpl('pkill', ['-KILL', '-P', String(child.pid)], {
|
|
stdio: 'ignore'
|
|
})
|
|
// A missing pkill surfaces as an async 'error' event, and an unhandled one
|
|
// takes down the main process.
|
|
descendants.on('error', () => undefined)
|
|
descendants.unref()
|
|
} catch {
|
|
// The direct kill below remains the fallback when pkill is unavailable.
|
|
}
|
|
}
|
|
child.kill('SIGKILL')
|
|
}
|
|
|
|
/** Codex answering "no such method" is the only response that proves the RPC
|
|
* surface is absent rather than temporarily failing. */
|
|
export function isCodexMethodNotFoundError(error: unknown): boolean {
|
|
if (typeof error !== 'object' || error === null) {
|
|
return false
|
|
}
|
|
const { code, message } = error as { code?: unknown; message?: unknown }
|
|
return (
|
|
code === JSON_RPC_METHOD_NOT_FOUND ||
|
|
/method not found/i.test(typeof message === 'string' ? message : '')
|
|
)
|
|
}
|
|
|
|
/**
|
|
* Runs one short-lived `codex app-server` session over stdio JSON-RPC (JSONL):
|
|
* spawn → initialize → initialized → body(rpc) → EOF/reap. The child is reaped
|
|
* on every path; the session deadline SIGKILLs it.
|
|
*/
|
|
export async function runCodexAppServerSession<T>(
|
|
invocation: CodexAppServerInvocation,
|
|
body: (rpc: CodexAppServerRpc) => Promise<T>,
|
|
spawnImpl: typeof spawn = spawn
|
|
): Promise<T> {
|
|
// Why: a default-home grant must run against the real ~/.codex, so strip an
|
|
// inherited CODEX_HOME (envToDelete) after applying the overlay, not before.
|
|
const childEnv: NodeJS.ProcessEnv = { ...process.env, ...invocation.env }
|
|
for (const key of invocation.envToDelete ?? []) {
|
|
delete childEnv[key]
|
|
}
|
|
const pairedEnv = invocation.cliPath
|
|
? withCliRuntimeOnPath(invocation.cliPath, childEnv)
|
|
: childEnv
|
|
const child = spawnImpl(invocation.command, invocation.args, {
|
|
env: pairedEnv,
|
|
stdio: ['pipe', 'pipe', 'pipe'],
|
|
windowsHide: true
|
|
}) as ChildProcessWithoutNullStreams
|
|
|
|
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 exitPromise = new Promise<void>((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)
|
|
})
|
|
|
|
let stdoutBuffer = ''
|
|
child.stdout.setEncoding('utf8').on('data', (chunk: string) => {
|
|
stdoutBuffer += chunk
|
|
if (Buffer.byteLength(stdoutBuffer) > STDOUT_LINE_MAX_BYTES) {
|
|
// Why: Windows process-tree termination is asynchronous; stop buffered
|
|
// chunks from spawning another taskkill for the same oversized response.
|
|
child.stdout.destroy()
|
|
killCodexAppServerProcessTree(child)
|
|
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()
|
|
}
|
|
|
|
let rejectDeadline: (error: Error) => void = () => {}
|
|
const deadlinePromise = new Promise<never>((_resolve, reject) => {
|
|
rejectDeadline = reject
|
|
})
|
|
const deadline = setTimeout(() => {
|
|
timedOut = true
|
|
const error = new CodexAppServerTimeoutError(
|
|
`codex app-server session exceeded ${invocation.timeoutMs}ms (${invocation.command})`
|
|
)
|
|
killCodexAppServerProcessTree(child)
|
|
failPending(error)
|
|
rejectDeadline(error)
|
|
}, invocation.timeoutMs)
|
|
|
|
function sendLine(payload: Record<string, unknown>): void {
|
|
child.stdin.write(`${JSON.stringify(payload)}\n`)
|
|
}
|
|
|
|
function notify(method: string, params?: Record<string, unknown>): void {
|
|
const payload: Record<string, unknown> = { method }
|
|
if (params !== undefined) {
|
|
payload.params = params
|
|
}
|
|
try {
|
|
sendLine(payload)
|
|
} catch {
|
|
// Notifications are fire-and-forget; a dead child fails the next request.
|
|
}
|
|
}
|
|
|
|
async function requestRpc(method: string, params?: Record<string, unknown>): Promise<unknown> {
|
|
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<JsonRpcResponse>((resolve, reject) => {
|
|
pending.set(id, { resolve, reject })
|
|
const payload: Record<string, unknown> = { 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 (isCodexMethodNotFoundError(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 {
|
|
const session = async (): Promise<T> => {
|
|
await requestRpc('initialize', {
|
|
clientInfo: { name: 'orca_desktop', title: 'Orca', version: '0.0.0' }
|
|
})
|
|
notify('initialized')
|
|
return body({ request: requestRpc, notify })
|
|
}
|
|
// Why: the timeout owns the whole callback, including time between RPCs;
|
|
// killing the child alone cannot settle a callback awaiting unrelated work.
|
|
return await Promise.race([session(), deadlinePromise])
|
|
} 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) {
|
|
killCodexAppServerProcessTree(child)
|
|
await waitForProcessExitUntil(exitPromise, 1000)
|
|
}
|
|
}
|
|
clearTimeout(deadline)
|
|
}
|
|
}
|