From a7ca13e4a73052ab17ef50213b02d29aa8b2c0f1 Mon Sep 17 00:00:00 2001 From: Danny Kim Date: Sat, 17 Oct 2026 23:15:39 +0900 Subject: [PATCH] Spawn child processes directly and drop the Deno.Command shim --- .agents/memory/bot-agents-child-processes.md | 39 +++++ .../memory/bot-agents-node-command-shim.md | 25 --- .agents/memory/bot-agents-tests-root-only.md | 6 +- src/auth.test.ts | 10 +- src/auth.ts | 6 +- src/child.test.ts | 35 ++++ src/child.ts | 30 ++++ src/command.ts | 160 ----------------- src/errors.ts | 3 +- src/isolation.test.ts | 68 ++++---- src/isolation.ts | 22 --- src/run.ts | 165 +++++++++--------- src/runtime.test.ts | 47 +++-- 13 files changed, 255 insertions(+), 361 deletions(-) create mode 100644 .agents/memory/bot-agents-child-processes.md delete mode 100644 .agents/memory/bot-agents-node-command-shim.md create mode 100644 src/child.test.ts create mode 100644 src/child.ts delete mode 100644 src/command.ts diff --git a/.agents/memory/bot-agents-child-processes.md b/.agents/memory/bot-agents-child-processes.md new file mode 100644 index 0000000..65bc1e2 --- /dev/null +++ b/.agents/memory/bot-agents-child-processes.md @@ -0,0 +1,39 @@ +--- +description: Child processes use plain node:child_process spawn (explicit stdio, uid/gid, whole env); src/child.ts has exitCode (128+signo) and echo; no execFile maxBuffer +--- + +Every call site calls `spawn` from `node:child_process` itself, with +everything explicit: + +- **`uid`/`gid`** of the profile for anything an agent profile runs (each + profile is its own UID, 20000+). Never drop them. +- **`env`** is the whole environment the child gets (a sanitized one for + agents); nothing from the coordinator is merged in. +- **`stdio`**: stdin is `"ignore"` so an agent CLI never waits on input + (only `codex login --with-api-key` pipes the token in). Output that is not + read is `"ignore"`; output that is read is `"pipe"` and streamed. +- No `execFile`/`exec`: their `maxBuffer` (1 MiB) kills a chatty child. Read + piped streams incrementally instead. + +`src/child.ts` holds what the call sites share: + +- **`exitCode(child)`**: call it right after `spawn`. It settles on `exit` + (not `close`, which a daemonized grandchild can hold open), maps a signal to + `128 + signo` as a shell does, and rejects on a failed start. The rejection + is pre-caught so it is not reported as unhandled before the caller awaits it. +- **`echo(stream, chunk)`**: writes and waits for `drain` when the buffer is + full, so echoing never outruns a slow reader. + +Tests use `spawnSync` for short commands. + +Other port details: + +- **`src/errors.ts`** `isNotFound` checks `ENOENT` (file) or `ESRCH` (process). +- Temporary files are an exclusive `wx` create with mode `0600`. +- `src/json.ts` holds type guards used instead of `as` casts on parsed JSON. +- `process` and `Buffer` must come from `node:` imports: + `no-restricted-globals` in `scripts/project-lint.ts` enforces it + ([[lint-parity-never-shrinks]]). + +Related: how each agent CLI reports bad credentials is in +[[agent-cli-auth-failures]]. diff --git a/.agents/memory/bot-agents-node-command-shim.md b/.agents/memory/bot-agents-node-command-shim.md deleted file mode 100644 index eb7076c..0000000 --- a/.agents/memory/bot-agents-node-command-shim.md +++ /dev/null @@ -1,25 +0,0 @@ ---- -description: src/command.ts reproduces Deno.Command on child_process.spawn (stdio defaults, clearEnv, uid/gid, 128+signo); src/errors.ts maps NotFound to ENOENT/ESRCH ---- - -The Deno code was ported one for one; two small modules keep the old shapes -so callers barely changed: - -- **`src/command.ts`** wraps `child_process.spawn` with `Deno.Command`'s API: - `output()` and `spawn()`, `clearEnv`, `uid`/`gid` (the agent isolation runs - each profile as its own UID, 20000+), and Deno's stdio defaults: stdin - inherited; stdout/stderr piped for `output()`, inherited for `spawn()`. A - signal exit maps to status `128 + signo`. Status settles on process exit, - not on stream close. Use it instead of calling `node:child_process` - directly so isolation and status handling stay uniform. -- **`src/errors.ts`** replaces `Deno.errors.NotFound` checks with an - `ENOENT`/`ESRCH` code check. -- `makeTempFile` became an exclusive `wx` create with mode `0600`. -- `src/json.ts` holds type guards used instead of `as` casts on parsed JSON. -- `process` and `Buffer` must come from `node:` imports: - `no-restricted-globals` in `scripts/project-lint.ts` maps Deno's - `no-window`/`no-process-global`/`no-node-globals` there - ([[lint-parity-never-shrinks]]). - -Related: how each agent CLI reports bad credentials is in -[[agent-cli-auth-failures]]. diff --git a/.agents/memory/bot-agents-tests-root-only.md b/.agents/memory/bot-agents-tests-root-only.md index 823c2ed..1ee2be4 100644 --- a/.agents/memory/bot-agents-tests-root-only.md +++ b/.agents/memory/bot-agents-tests-root-only.md @@ -7,10 +7,10 @@ credential cleanup. They need root and are written as `test.skipIf(!IS_ROOT)(...)` (`IS_ROOT` from `#testing`), so they show as skipped instead of silently passing. -- As a normal user `vp test` passes 56 and skips 5. In CI (self-hosted - runner, root) all 61 run. +- As a normal user `vp test` passes 61 and skips 5. In CI (self-hosted + runner, root) all 66 run. - When changing `isolation.ts`, `runtime.ts`, `credentials.ts`, `prepare.ts`, - or `command.ts`, run the suite as root in `node:24-bookworm` (e.g. a + or `child.ts`, run the suite as root in `node:24-bookworm` (e.g. a container with the repo mounted) before pushing; a green non-root run proves nothing about those paths. - The migration also ran an end-to-end smoke as root with fake diff --git a/src/auth.test.ts b/src/auth.test.ts index 1a7f334..febdb82 100644 --- a/src/auth.test.ts +++ b/src/auth.test.ts @@ -17,7 +17,7 @@ test("retries once after an invalid refresh token", async () => { const result = await retryInvalidToken( () => Promise.resolve({ - status: { success: false }, + code: 1, error: attempts++ === 0 ? "ERROR: invalid_refresh_token" : "still failed", }), () => { @@ -40,7 +40,7 @@ test("does not retry an unrelated failure", async () => { () => { attempts++; return Promise.resolve({ - status: { success: false }, + code: 1, error: "rate limited", }); }, @@ -86,7 +86,7 @@ test("re-logs in after the job 9837 revoked-session stderr", async () => { const result = await retryInvalidToken( () => Promise.resolve({ - status: { success: attempts++ > 0 }, + code: attempts++ > 0 ? 0 : 1, error: attempts === 1 ? REVOKED_CODEX.join("\n") : "", }), () => { @@ -95,9 +95,9 @@ test("re-logs in after the job 9837 revoked-session stderr", async () => { }, ); - expect({ relogins, success: result.status.success }).toStrictEqual({ + expect({ relogins, code: result.code }).toStrictEqual({ relogins: 1, - success: true, + code: 0, }); }); diff --git a/src/auth.ts b/src/auth.ts index 2cde002..d50837d 100644 --- a/src/auth.ts +++ b/src/auth.ts @@ -48,12 +48,12 @@ export function isClaudeAuthFailure(result: ClaudeFailureFields): boolean { ); } -export async function retryInvalidToken( +export async function retryInvalidToken( attempt: () => Promise, relogin: () => Promise, ): Promise { let result = await attempt(); - if (!result.status.success && isCodexAuthFailure(result.error)) { + if (result.code !== 0 && isCodexAuthFailure(result.error)) { await relogin(); result = await attempt(); } @@ -111,7 +111,7 @@ export async function stageCodexAuth( return stagedPath; } -// Creates an empty private file under a fresh name, as Deno.makeTempFile did. +// Creates an empty private file under a fresh name. async function makeTempFile(directory: string): Promise { const path = join(directory, `.tmp-${randomUUID()}`); const handle = await open(path, "wx", 0o600); diff --git a/src/child.test.ts b/src/child.test.ts new file mode 100644 index 0000000..1b576cf --- /dev/null +++ b/src/child.test.ts @@ -0,0 +1,35 @@ +import { spawn } from "node:child_process"; + +import { expect, test } from "vite-plus/test"; + +import { isNotFound } from "./errors.ts"; +import { exitCode } from "./child.ts"; + +test("reports the exit code", async () => { + expect(await exitCode(spawn("/bin/sh", ["-c", "exit 0"], { stdio: "ignore" }))).toBe(0); + expect(await exitCode(spawn("/bin/sh", ["-c", "exit 3"], { stdio: "ignore" }))).toBe(3); +}); + +test("reports a signal as 128 plus its number", async () => { + const child = spawn("/bin/sh", ["-c", "kill -KILL $$"], { stdio: "ignore" }); + expect(await exitCode(child)).toBe(137); +}); + +test("resolves on exit while a grandchild still holds the output open", async () => { + const child = spawn("/bin/sh", ["-c", "sleep 2 & exit 5"], { stdio: ["ignore", "pipe", "pipe"] }); + const started = Date.now(); + expect(await exitCode(child)).toBe(5); + expect(Date.now() - started).toBeLessThan(1500); + child.stdout.destroy(); + child.stderr.destroy(); +}); + +test("rejects when the command does not exist", async () => { + const child = spawn("/nonexistent/command", [], { stdio: "ignore" }); + const code = exitCode(child); + // The rejection must stay unhandled-safe until it is awaited. + await new Promise((resolve) => { + setTimeout(resolve, 50); + }); + await expect(code).rejects.toSatisfy(isNotFound); +}); diff --git a/src/child.ts b/src/child.ts new file mode 100644 index 0000000..7a09a09 --- /dev/null +++ b/src/child.ts @@ -0,0 +1,30 @@ +// What the child process call sites share: the exit status, read right after +// `spawn` so the exit event cannot pass unseen while the caller reads the +// streams, and echoing output without outrunning a slow reader. +import type { ChildProcess } from "node:child_process"; +import { once } from "node:events"; +import { constants } from "node:os"; +import type { Writable } from "node:stream"; + +// Resolves once the process exits, before its streams close, with its exit +// code, or with 128 plus the signal number if a signal killed it, as a shell +// reports. A process that fails to start rejects with the spawn error. +export function exitCode(child: ChildProcess): Promise { + const code = new Promise((resolve, reject) => { + child.once("error", reject); + child.once("exit", (exit, signal) => { + resolve(exit ?? (signal === null ? 1 : 128 + constants.signals[signal])); + }); + }); + // Callers await it after draining the streams; a failed start must not be + // reported as an unhandled rejection before then. + code.catch(() => {}); + return code; +} + +// Writes a chunk, waiting for the stream to drain when its buffer is full. +export async function echo(stream: Writable, chunk: string | Uint8Array): Promise { + if (!stream.write(chunk)) { + await once(stream, "drain"); + } +} diff --git a/src/command.ts b/src/command.ts deleted file mode 100644 index e45fe2d..0000000 --- a/src/command.ts +++ /dev/null @@ -1,160 +0,0 @@ -// A child process with the call shapes the runner was reviewed with: `output()` -// collects both streams, `spawn()` hands them out, and `clearEnv` passes only -// the given environment. Streams default as Deno's did: stdin is inherited, -// stdout and stderr are piped for `output()` and inherited for `spawn()`. -import { spawn as spawnProcess } from "node:child_process"; -import type { ChildProcess, StdioOptions } from "node:child_process"; -import { Buffer } from "node:buffer"; -import { constants } from "node:os"; -import process from "node:process"; -import type { Readable, Writable } from "node:stream"; -import { buffer } from "node:stream/consumers"; -import { finished } from "node:stream/promises"; - -export type StdioMode = "inherit" | "piped" | "null"; - -export interface CommandOptions { - args?: string[]; - cwd?: string; - env?: Record; - clearEnv?: boolean; - uid?: number; - gid?: number; - stdin?: StdioMode; - stdout?: StdioMode; - stderr?: StdioMode; -} - -export interface CommandStatus { - success: boolean; - code: number; -} - -export interface CommandOutput extends CommandStatus { - stdout: Uint8Array; - stderr: Uint8Array; -} - -export interface CommandChild { - status: Promise; - stdin: Writable; - stdout: Readable; - stderr: Readable; -} - -const STDIO: Record = { - inherit: "inherit", - piped: "pipe", - null: "ignore", -}; - -function signalNumber(signal: NodeJS.Signals): number { - return constants.signals[signal]; -} - -function startProcess( - command: string, - options: CommandOptions, - outputDefault: StdioMode, -): ChildProcess { - const stdio: StdioOptions = [ - STDIO[options.stdin ?? "inherit"], - STDIO[options.stdout ?? outputDefault], - STDIO[options.stderr ?? outputDefault], - ]; - const environment = options.clearEnv === true ? options.env : { ...process.env, ...options.env }; - return spawnProcess(command, options.args ?? [], { - stdio, - ...(options.cwd !== undefined && { cwd: options.cwd }), - ...(environment !== undefined && { env: environment }), - ...(options.uid !== undefined && { uid: options.uid }), - ...(options.gid !== undefined && { gid: options.gid }), - }); -} - -// Settles when the process exits, before its streams close, as Deno's did. A -// signal-killed process reports 128 plus the signal number, as a shell does. -function waitForStatus(child: ChildProcess): Promise { - return new Promise((resolve, reject) => { - child.once("error", reject); - child.once("exit", (code, signal) => { - const exitCode = code ?? (signal === null ? 1 : 128 + signalNumber(signal)); - resolve({ success: exitCode === 0, code: exitCode }); - }); - }); -} - -function piped(stream: T | null, name: string): T { - if (stream === null) { - throw new TypeError(`${name} is not piped`); - } - return stream; -} - -function collect(stream: Readable | null): Promise { - return stream === null ? Promise.resolve(new Uint8Array()) : buffer(stream); -} - -// A stream's chunks as bytes, whether or not it has an encoding set. -export async function* chunks(stream: Readable): AsyncGenerator { - for await (const chunk of stream) { - const value: unknown = chunk; - if (value instanceof Uint8Array) { - yield value; - } else if (typeof value === "string") { - yield Buffer.from(value); - } - } -} - -export function spawn(command: string, options: CommandOptions = {}): CommandChild { - const child = startProcess(command, options, "inherit"); - const status = waitForStatus(child); - // A failed start rejects `status`; the caller sees it once it awaits that. - status.catch(() => {}); - return { - status, - get stdin(): Writable { - return piped(child.stdin, "stdin"); - }, - get stdout(): Readable { - return piped(child.stdout, "stdout"); - }, - get stderr(): Readable { - return piped(child.stderr, "stderr"); - }, - }; -} - -export async function output( - command: string, - options: CommandOptions = {}, -): Promise { - const child = startProcess(command, options, "piped"); - const [status, stdout, stderr] = await Promise.all([ - waitForStatus(child), - collect(child.stdout), - collect(child.stderr), - ]); - return { ...status, stdout, stderr }; -} - -// Writes one chunk and settles once the stream has taken it, as Deno's -// `Deno.stdout.write` did. -export async function writeChunk(stream: Writable, chunk: Uint8Array): Promise { - await new Promise((resolve, reject) => { - stream.write(chunk, (error) => { - if (error) { - reject(error); - } else { - resolve(); - } - }); - }); -} - -// Writes the last chunk and settles once the stream has flushed and closed. -export async function endWith(stream: Writable, chunk: string): Promise { - stream.end(chunk); - await finished(stream); -} diff --git a/src/errors.ts b/src/errors.ts index d00da7b..bf0a25d 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -1,5 +1,4 @@ -// Node reports a missing file as ENOENT and a missing process as ESRCH, where -// Deno threw `Deno.errors.NotFound` for both. +// A missing file or a missing process: Node reports them as ENOENT and ESRCH. export function isNotFound(error: unknown): boolean { if (!(error instanceof Error) || !("code" in error)) { return false; diff --git a/src/isolation.test.ts b/src/isolation.test.ts index fbc6451..f3d54e8 100644 --- a/src/isolation.test.ts +++ b/src/isolation.test.ts @@ -1,14 +1,11 @@ +import { spawnSync } from "node:child_process"; +import type { SpawnSyncReturns } from "node:child_process"; import { expect, test } from "vite-plus/test"; import { chmod, chown, mkdir, readFile, rm, symlink, writeFile } from "node:fs/promises"; import process from "node:process"; import { isNotFound } from "./errors.ts"; -import { - agentCommand, - isRunnableProfileProcess, - killProfileProcesses, - readProfileFile, -} from "./isolation.ts"; +import { isRunnableProfileProcess, killProfileProcesses, readProfileFile } from "./isolation.ts"; import type { RuntimeProfile } from "./runtime.ts"; import { currentUid, IS_ROOT, IS_WINDOWS, makeTempDir } from "#testing"; @@ -22,6 +19,23 @@ const profile = (uid: number, home: string): RuntimeProfile => ({ tokenFile: null, }); +// Runs a command as the profile's user from the root directory. Without `env` +// it inherits this process's environment. +function runAs( + selected: RuntimeProfile, + command: string, + args: string[], + env?: Record, +): SpawnSyncReturns { + return spawnSync(command, args, { + cwd: "/", + uid: selected.uid, + gid: selected.gid, + encoding: "utf8", + ...(env !== undefined && { env }), + }); +} + test("zombie process entries are already quiescent", () => { const status = "State:\tZ (zombie)\nUid:\t20010\t20010\t20010\t20010\n"; expect(isRunnableProfileProcess(status, 20_010)).toBe(false); @@ -60,20 +74,14 @@ test.skipIf(!IS_ROOT)("a profile process can read only its own private files", a await writeFile(`${root}/reviewer`, "root", { mode: 0o600 }); const selected = profile(20_000, homeA); - const own = await agentCommand(selected, "/bin/cat", [`${homeA}/token`], { - cwd: "/", - }).output(); - const sibling = await agentCommand(selected, "/bin/cat", [`${homeB}/token`], { - cwd: "/", - }).output(); - const reviewer = await agentCommand(selected, "/bin/cat", [`${root}/reviewer`], { - cwd: "/", - }).output(); + const own = runAs(selected, "/bin/cat", [`${homeA}/token`]); + const sibling = runAs(selected, "/bin/cat", [`${homeB}/token`]); + const reviewer = runAs(selected, "/bin/cat", [`${root}/reviewer`]); - expect(new TextDecoder().decode(own.stdout)).toBe("own"); - expect(own.success).toBe(true); - expect(sibling.success).toBe(false); - expect(reviewer.success).toBe(false); + expect(own.stdout).toBe("own"); + expect(own.status).toBe(0); + expect(sibling.status).not.toBe(0); + expect(reviewer.status).not.toBe(0); } finally { await rm(root, { recursive: true }); } @@ -81,13 +89,8 @@ test.skipIf(!IS_ROOT)("a profile process can read only its own private files", a test.skipIf(!IS_ROOT)("cleanup leaves no runnable daemonized process", async () => { const selected = profile(20_010, "/tmp"); - const launched = await agentCommand( - selected, - "/bin/sh", - ["-c", "sleep 60 >/dev/null 2>&1 & echo $!"], - { cwd: "/" }, - ).output(); - const pid = Number(new TextDecoder().decode(launched.stdout).trim()); + const launched = runAs(selected, "/bin/sh", ["-c", "sleep 60 >/dev/null 2>&1 & echo $!"]); + const pid = Number(launched.stdout.trim()); expect(Number.isInteger(pid)).toBe(true); await killProfileProcesses(selected.uid); @@ -99,14 +102,11 @@ test.skipIf(!IS_ROOT)("cleanup leaves no runnable daemonized process", async () expect(status === null || !isRunnableProfileProcess(status, selected.uid)).toBe(true); }); -test.skipIf(!IS_ROOT)("the agent process cannot read its coordinator environment", async () => { +test.skipIf(!IS_ROOT)("the agent process cannot read its coordinator environment", () => { const selected = profile(20_020, "/tmp"); - const result = await agentCommand(selected, "/usr/bin/cat", [`/proc/${process.pid}/environ`], { - cwd: "/", - env: { PARENT_SECRET: "must-not-cross-boundary" }, - clearEnv: true, - stderr: "piped", - }).output(); + const result = runAs(selected, "/usr/bin/cat", [`/proc/${process.pid}/environ`], { + PARENT_SECRET: "must-not-cross-boundary", + }); - expect(result.success).toBe(false); + expect(result.status).not.toBe(0); }); diff --git a/src/isolation.ts b/src/isolation.ts index 04745b3..c096155 100644 --- a/src/isolation.ts +++ b/src/isolation.ts @@ -2,31 +2,9 @@ import { lstat, readdir, readFile } from "node:fs/promises"; import process from "node:process"; import { setTimeout as sleep } from "node:timers/promises"; -import { output, spawn } from "./command.ts"; -import type { CommandChild, CommandOptions, CommandOutput } from "./command.ts"; import { isNotFound } from "./errors.ts"; import type { RuntimeProfile } from "./runtime.ts"; -export type AgentCommandOptions = Omit; - -export interface AgentCommand { - output(): Promise; - spawn(): CommandChild; -} - -export function agentCommand( - profile: RuntimeProfile, - command: string, - args: string[], - options: AgentCommandOptions = {}, -): AgentCommand { - const full: CommandOptions = { ...options, args, uid: profile.uid, gid: profile.gid }; - return { - output: () => output(command, full), - spawn: () => spawn(command, full), - }; -} - export function isRunnableProfileProcess(status: string, uid: number): boolean { const owner = /^Uid:\s+(\d+)/mu.exec(status)?.[1]; const state = /^State:\s+(\w)/mu.exec(status)?.[1]; diff --git a/src/run.ts b/src/run.ts index ec1a0ae..69a410b 100644 --- a/src/run.ts +++ b/src/run.ts @@ -1,10 +1,12 @@ // Runs one ordered agent profile. Credential decryption and reviewer posting // live in separate processes so neither privileged secret reaches this runner. +import { spawn } from "node:child_process"; import { chmod, chown, readFile, writeFile } from "node:fs/promises"; import process from "node:process"; import { createInterface } from "node:readline"; import type { Readable } from "node:stream"; import { text as readText } from "node:stream/consumers"; +import { finished } from "node:stream/promises"; import { setTimeout as sleep } from "node:timers/promises"; import { @@ -16,9 +18,8 @@ import { stageCodexAuth, } from "./auth.ts"; import type { ClaudeFailureFields, CodexAuthentication } from "./auth.ts"; -import { chunks, endWith, output, writeChunk } from "./command.ts"; -import type { CommandStatus } from "./command.ts"; -import { agentCommand, killProfileProcesses, readProfileFile } from "./isolation.ts"; +import { echo, exitCode } from "./child.ts"; +import { killProfileProcesses, readProfileFile } from "./isolation.ts"; import { isObject } from "./json.ts"; import { collectOpenCodeOutput, @@ -98,8 +99,8 @@ async function postAuthorComment(body: string): Promise { } async function runRoot(command: string, args: string[]): Promise { - const { success, code } = await output(command, { args, env: PROCESS_ENV, clearEnv: true }); - if (!success) throw new Error(`${command} ${args[0]} exited with ${code}`); + const code = await exitCode(spawn(command, args, { env: PROCESS_ENV, stdio: "ignore" })); + if (code !== 0) throw new Error(`${command} ${args[0]} exited with ${code}`); } async function configureGitAuthor(): Promise { @@ -122,12 +123,15 @@ async function runAgentCommand( args: string[], token?: string, ): Promise { - const { success, code } = await agentCommand(profile, command, args, { + const child = spawn(command, args, { cwd: WORKSPACE, env: agentEnvironment(PROCESS_ENV, profile, token), - clearEnv: true, - }).output(); - if (!success) throw new Error(`${command} ${args[0]} exited with ${code}`); + uid: profile.uid, + gid: profile.gid, + stdio: "ignore", + }); + const code = await exitCode(child); + if (code !== 0) throw new Error(`${command} ${args[0]} exited with ${code}`); } async function prepareProfile(profile: RuntimeProfile): Promise { @@ -189,8 +193,7 @@ async function renderPrompt( } async function runClaude(profile: RuntimeProfile, token: string, prompt: string): Promise { - const claude = agentCommand( - profile, + const claude = spawn( "claude", [ "--print", @@ -208,21 +211,21 @@ async function runClaude(profile: RuntimeProfile, token: string, prompt: string) ...agentEnvironment(PROCESS_ENV, profile, token), IS_SANDBOX: "1", }, - clearEnv: true, - stdout: "piped", - stderr: "piped", + uid: profile.uid, + gid: profile.gid, + stdio: ["ignore", "pipe", "pipe"], }, - ).spawn(); + ); + const status = exitCode(claude); let result: ClaudeResult | undefined; let error = ""; const copyError = (async (): Promise => { - const decoder = new TextDecoder(); - for await (const chunk of chunks(claude.stderr)) { - const part = decoder.decode(chunk, { stream: true }); + claude.stderr.setEncoding("utf8"); + for await (const chunk of claude.stderr) { + const part = String(chunk); error += part; console.error(part); } - error += decoder.decode(); })(); for await (const line of createInterface({ input: claude.stdout, crlfDelay: Infinity })) { if (line !== "") { @@ -250,7 +253,7 @@ async function runClaude(profile: RuntimeProfile, token: string, prompt: string) } } } - const [status] = await Promise.all([claude.status, copyError]); + const [code] = await Promise.all([status, copyError]); if (result?.is_error === true) { const failure = new Error(result.result ?? `claude ended with ${result.subtype}`); if (isClaudeAuthFailure(result)) { @@ -261,8 +264,8 @@ async function runClaude(profile: RuntimeProfile, token: string, prompt: string) if (result?.result === undefined) { throw new Error(error.trim() || `claude ended with ${result?.subtype ?? "no result"}`); } - if (!status.success) { - throw new Error(error.trim() || `claude exited with ${status.code}`); + if (code !== 0) { + throw new Error(error.trim() || `claude exited with ${code}`); } return result.result; } @@ -270,7 +273,7 @@ async function runClaude(profile: RuntimeProfile, token: string, prompt: string) // Polls every 100 ms until the login prints something, failing if it exits first. async function pollForInstructions( shownText: () => string, - status: Promise, + status: Promise, ): Promise { if (shownText().trim() !== "") { return; @@ -286,29 +289,28 @@ async function codexDeviceLogin( profile: RuntimeProfile, environment: Record, ): Promise { - const login = agentCommand(profile, "codex", ["login", "--device-auth"], { + const login = spawn("codex", ["login", "--device-auth"], { cwd: WORKSPACE, env: environment, - clearEnv: true, - stdout: "piped", - stderr: "piped", - }).spawn(); + uid: profile.uid, + gid: profile.gid, + stdio: ["ignore", "pipe", "pipe"], + }); + const status = exitCode(login); let shown = ""; const collect = async (stream: Readable): Promise => { - const decoder = new TextDecoder(); - for await (const chunk of chunks(stream)) { - shown += decoder.decode(chunk, { stream: true }); + stream.setEncoding("utf8"); + for await (const chunk of stream) { + shown += String(chunk); } - shown += decoder.decode(); }; const drained = Promise.all([collect(login.stdout), collect(login.stderr)]); - const status = login.status; await sleep(3000); await pollForInstructions(() => shown, status); await postAuthorComment(withProfile(shown, profile)); - const [result] = await Promise.all([status, drained]); - if (!result.success) { - throw new AgentAuthError(`codex login exited with ${result.code}`); + const [code] = await Promise.all([status, drained]); + if (code !== 0) { + throw new AgentAuthError(`codex login exited with ${code}`); } } @@ -317,17 +319,19 @@ async function codexApiLogin( environment: Record, token: string, ): Promise { - const login = agentCommand(profile, "codex", ["login", "--with-api-key"], { + const login = spawn("codex", ["login", "--with-api-key"], { cwd: WORKSPACE, env: environment, - clearEnv: true, - stdin: "piped", - stderr: "piped", - }).spawn(); - await endWith(login.stdin, `${token}\n`); - const [status, error] = await Promise.all([login.status, readText(login.stderr)]); - if (!status.success) { - throw new Error(error.trim() || `codex login exited with ${status.code}`); + uid: profile.uid, + gid: profile.gid, + stdio: ["pipe", "inherit", "pipe"], + }); + const status = exitCode(login); + login.stdin.end(`${token}\n`); + await finished(login.stdin); + const [code, error] = await Promise.all([status, readText(login.stderr)]); + if (code !== 0) { + throw new Error(error.trim() || `codex login exited with ${code}`); } } @@ -349,17 +353,18 @@ async function runCodex( if (authentication.kind === "api-key") { await codexApiLogin(profile, environment, authentication.token); } else { - const loggedIn = await agentCommand(profile, "codex", ["login", "status"], { + const check = spawn("codex", ["login", "status"], { cwd: WORKSPACE, env: environment, - clearEnv: true, - }).output(); - if (!loggedIn.success) await codexDeviceLogin(profile, environment); + uid: profile.uid, + gid: profile.gid, + stdio: "ignore", + }); + if ((await exitCode(check)) !== 0) await codexDeviceLogin(profile, environment); } const outputPath = await makeProfileFile(profile, "result.md"); - const attempt = async (): Promise<{ status: CommandStatus; error: string }> => { - const codex = agentCommand( - profile, + const attempt = async (): Promise<{ code: number; error: string }> => { + const codex = spawn( "codex", [ "exec", @@ -373,20 +378,21 @@ async function runCodex( { cwd: WORKSPACE, env: environment, - clearEnv: true, - stdout: "inherit", - stderr: "piped", + uid: profile.uid, + gid: profile.gid, + stdio: ["ignore", "inherit", "pipe"], }, - ).spawn(); - const decoder = new TextDecoder(); + ); + const status = exitCode(codex); + codex.stderr.setEncoding("utf8"); let error = ""; - for await (const chunk of chunks(codex.stderr)) { - await writeChunk(process.stderr, chunk); - error += decoder.decode(chunk, { stream: true }); + for await (const chunk of codex.stderr) { + const part = String(chunk); + await echo(process.stderr, part); + error += part; } - error += decoder.decode(); // Codex echoes the prompt, whose comments may quote auth errors. - return { status: await codex.status, error: error.replaceAll(prompt, "") }; + return { code: await status, error: error.replaceAll(prompt, "") }; }; const result = authentication.kind === "api-key" @@ -395,8 +401,8 @@ async function runCodex( await runAgentCommand(profile, "codex", ["logout"]); await codexDeviceLogin(profile, environment); }); - if (!result.status.success) { - const failure = new Error(result.error.trim() || `codex exited with ${result.status.code}`); + if (result.code !== 0) { + const failure = new Error(result.error.trim() || `codex exited with ${result.code}`); if (isCodexAuthFailure(result.error)) { throw new AgentAuthError("codex sign-in failed", { cause: failure }); } @@ -410,29 +416,24 @@ async function runOpenCode( token: string, prompt: string, ): Promise { - const child = agentCommand(profile, "opencode", openCodeArgs(profile.model, prompt), { + const child = spawn("opencode", openCodeArgs(profile.model, prompt), { cwd: WORKSPACE, env: agentEnvironment(PROCESS_ENV, profile, token), - clearEnv: true, - stdout: "piped", - stderr: "piped", - }).spawn(); + uid: profile.uid, + gid: profile.gid, + stdio: ["ignore", "pipe", "pipe"], + }); + const status = exitCode(child); const collected = collectOpenCodeOutput( - chunks(child.stdout), - chunks(child.stderr), - async (chunk) => { - await writeChunk(process.stdout, chunk); - }, - async (chunk) => { - await writeChunk(process.stderr, chunk); - }, + child.stdout, + child.stderr, + (chunk) => echo(process.stdout, chunk), + (chunk) => echo(process.stderr, chunk), ); - const [status, streams] = await Promise.all([child.status, collected]); + const [code, streams] = await Promise.all([status, collected]); const error = streams.stderr.trim(); - if (!status.success) { - throw ( - openCodeError(streams.stdout) ?? new Error(error || `opencode exited with ${status.code}`) - ); + if (code !== 0) { + throw openCodeError(streams.stdout) ?? new Error(error || `opencode exited with ${code}`); } return parseOpenCodeEvents(streams.stdout); } diff --git a/src/runtime.test.ts b/src/runtime.test.ts index 5f5cd9d..364648a 100644 --- a/src/runtime.test.ts +++ b/src/runtime.test.ts @@ -1,10 +1,10 @@ +import { spawnSync } from "node:child_process"; import { chmod, readdir, readFile, rm, stat, writeFile } from "node:fs/promises"; import process from "node:process"; import { fileURLToPath } from "node:url"; import { assert, expect, test } from "vite-plus/test"; -import { output } from "./command.ts"; import { prepareRuntime } from "./runtime.ts"; import type { RuntimeProfile } from "./runtime.ts"; import { IS_ROOT, makeTempDir } from "#testing"; @@ -13,14 +13,9 @@ const MASTER_KEY = "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8"; const ENCRYPTED = "v1:ICEiIyQlJicoKSor:oV_FAgnsN3p1FyegJxwGVdyjfyPio6qDeSUwbA"; // Runs `command` as the profile's user, from the root directory. -async function runAs(profile: RuntimeProfile, command: string, path: string): Promise { - const result = await output(command, { - args: [path], - cwd: "/", - uid: profile.uid, - gid: profile.gid, - }); - return result.success; +function runAs(profile: RuntimeProfile, command: string, path: string): boolean { + const result = spawnSync(command, [path], { cwd: "/", uid: profile.uid, gid: profile.gid }); + return result.status === 0; } async function expectPrivate(path: string, uid: number, mode: number): Promise { @@ -58,16 +53,16 @@ test.skipIf(!IS_ROOT)("prepares root-only reviewer and isolated profile credenti expect((await stat(codex.home)).uid).toStrictEqual(codex.uid); expect(codex.tokenFile).toBe(null); - const ownToken = await output("/bin/cat", { - args: [claude.tokenFile], + const ownToken = spawnSync("/bin/cat", [claude.tokenFile], { cwd: "/", uid: claude.uid, gid: claude.gid, + encoding: "utf8", }); - expect(ownToken.success).toBe(true); - expect(new TextDecoder().decode(ownToken.stdout)).toBe("secret-token"); - expect(await runAs(claude, "/bin/cat", manifest.reviewerFile)).toBe(false); - expect(await runAs(claude, "/bin/ls", codex.home)).toBe(false); + expect(ownToken.status).toBe(0); + expect(ownToken.stdout).toBe("secret-token"); + expect(runAs(claude, "/bin/cat", manifest.reviewerFile)).toBe(false); + expect(runAs(claude, "/bin/ls", codex.home)).toBe(false); } finally { await rm(root, { recursive: true }); await rm(action, { recursive: true }); @@ -85,18 +80,20 @@ test.skipIf(!IS_ROOT)("removes prepared credentials when output publication fail profiles: [{ type: "claude", token: ENCRYPTED }], }), ); - const result = await output(process.execPath, { - args: [fileURLToPath(new URL("prepare.ts", import.meta.url))], - env: { - ACTION_PATH: action, - RUNNER_TEMP: root, - BOT_AGENTS_MASTER_KEY: MASTER_KEY, - GITHUB_OUTPUT: action, + const result = spawnSync( + process.execPath, + [fileURLToPath(new URL("prepare.ts", import.meta.url))], + { + env: { + ACTION_PATH: action, + RUNNER_TEMP: root, + BOT_AGENTS_MASTER_KEY: MASTER_KEY, + GITHUB_OUTPUT: action, + }, }, - clearEnv: true, - }); + ); - expect(result.success).toBe(false); + expect(result.status).not.toBe(0); expect(await readdir(root)).toStrictEqual([]); } finally { await rm(root, { recursive: true });