diff --git a/lib/agents-runner.ts b/lib/agents-runner.ts index 206af2c8e..d48c260ce 100644 --- a/lib/agents-runner.ts +++ b/lib/agents-runner.ts @@ -1,5 +1,7 @@ import { isSessionChangeEvidence, type SessionChangeEvidence } from "./session-changes.ts"; -import { existsSync } from "node:fs"; +import { chmodSync, existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import type { Duplex, Readable, Writable } from "node:stream"; import { stripVTControlCharacters } from "node:util"; import { withoutInteractiveHost } from "./rpc-host.ts"; @@ -184,6 +186,8 @@ interface LiveTask { processGroup: number | undefined; terminal: { status: TaskRecord["status"]; error: string | null } | undefined; childExit: number | null | undefined; + childExitSignal?: string | null; + instructionsTransportDir?: string; cleanupDeadlineAt: number | undefined; quarantined: boolean; nextId: number; @@ -204,6 +208,11 @@ interface LiveTask { } const STDERR_TAIL_MAX = 512; +const DIR_MODE = 0o700; +const FILE_MODE = 0o600; +// UTF-8 bytes, not characters: argv size is what kills the child on macOS. +export const MAX_INLINE_INSTRUCTIONS_BYTES = 1000; +const MAX_TRANSPORT_PREFIX_CHARS = 64; const CHILD_MARKER = "GENTLE_PI_AGENTS_CHILD"; const IPC_MARKER = "GENTLE_PI_AGENTS_OWNED_IPC"; const PARENT_NOTIFICATION_TOOL = "subagent_parent_message"; @@ -238,9 +247,15 @@ function queryRejection(error: unknown): string { return "parent rejected query"; } +export function formatChildExit(code: number | null | undefined, signal?: string | null): string { + if (typeof code === "number") return `code ${code}`; + if (signal) return `signal ${signal}`; + return `code ${code ?? "unknown"}`; +} + const hostProcess: ProcessControl = { platform: process.platform, kill: (pid, signal) => process.kill(pid, signal) }; -export function childArguments(request: TaskRequest): string[] { +export function childArguments(request: TaskRequest, instructionsPath?: string): string[] { const args = ["--mode", "rpc", "--session-dir", request.sessionDir]; for (const path of request.extensionPaths ?? []) args.push("--extension", path); if (request.resumeSessionPath) args.push("--session", request.resumeSessionPath); @@ -248,7 +263,11 @@ export function childArguments(request: TaskRequest): string[] { else if (request.thinking) args.push("--thinking", request.thinking); const tools = request.agent.tools.length > 0 ? [...new Set([...request.agent.tools, PARENT_NOTIFICATION_TOOL])] : DEFAULT_TOOLS; if (tools.length > 0) args.push("--tools", tools.join(",")); - if (request.agent.instructions.length > 0) args.push("--append-system-prompt", request.agent.instructions); + if (instructionsPath) { + args.push("--append-system-prompt", instructionsPath); + } else if (request.agent.instructions.length > 0) { + args.push("--append-system-prompt", request.agent.instructions); + } return args; } @@ -458,22 +477,44 @@ export class AgentRunner { // Do not forward stale legacy child selection or authorization. delete env.GENTLE_PI_SDD_REMEDIATION_PLAN; delete env.GENTLE_PI_RESEARCH_SELECTION; + let instructionsTransportDir: string | undefined; + let instructionsTransportPath: string | undefined; + if (Buffer.byteLength(request.agent.instructions, "utf8") > MAX_INLINE_INSTRUCTIONS_BYTES) { + try { + const prefix = (request.agent.name || "instructions").replace(/[^a-zA-Z0-9._-]/g, "_").slice(0, MAX_TRANSPORT_PREFIX_CHARS); + instructionsTransportDir = mkdtempSync(join(tmpdir(), `gentle-pi-subagent-${prefix}-`)); + try { chmodSync(instructionsTransportDir, DIR_MODE); } catch { /* best effort */ } + instructionsTransportPath = join(instructionsTransportDir, "instructions.md"); + writeFileSync(instructionsTransportPath, request.agent.instructions, { mode: FILE_MODE, encoding: "utf8" }); + try { chmodSync(instructionsTransportPath, FILE_MODE); } catch { /* best effort */ } + } catch (error) { + if (instructionsTransportDir) { + try { rmSync(instructionsTransportDir, { recursive: true, force: true }); } catch { /* best effort */ } + } + this.store.update(id, { status: TASK_STATUS.RUNNING, startedAt: this.deps.now(), lastStep: "starting" }); + this.finish(id, TASK_STATUS.FAILED, `could not write agent instructions: ${error instanceof Error ? error.message : String(error)}`); + return; + } + } let child: ChildLike; try { const pi = this.deps.resolvePi?.() ?? this.deps.pi; - child = this.deps.spawn(pi.command, [...pi.args, ...childArguments(request)], { + child = this.deps.spawn(pi.command, [...pi.args, ...childArguments(request, instructionsTransportPath)], { cwd: request.cwd, env, detached, stdio: hasParentPermissionChannel ? ["pipe", "pipe", "pipe", permissionChannelStdio, "ipc"] : ["pipe", "pipe", "pipe", "ipc"], }); } catch (error) { + if (instructionsTransportDir) { + try { rmSync(instructionsTransportDir, { recursive: true, force: true }); } catch { /* best effort */ } + } this.store.update(id, { status: TASK_STATUS.RUNNING, startedAt: this.deps.now(), lastStep: "starting" }); this.finish(id, TASK_STATUS.FAILED, `could not start pi: ${error instanceof Error ? error.message : String(error)}`); return; } const processGroup = detached && typeof child.pid === "number" && child.pid > 0 ? child.pid : undefined; - const live: LiveTask = { child, sawRunEvent: false, mutationStarts: new Map(), inFlightTools: new Map(), argumentProgress: new ToolArgumentProgress(), pending: new Map(), queries: new Map(), replies: new Map(), cancelStall: () => {}, cancelGrace: () => {}, processGroup, terminal: undefined, childExit: undefined, cleanupDeadlineAt: undefined, quarantined: false, nextId: 0, ipcClosed: false, acknowledgedIpcIds: new Set(), acknowledgedIpcOrder: [], stderrTail: "" }; + const live: LiveTask = { child, sawRunEvent: false, mutationStarts: new Map(), inFlightTools: new Map(), argumentProgress: new ToolArgumentProgress(), pending: new Map(), queries: new Map(), replies: new Map(), cancelStall: () => {}, cancelGrace: () => {}, processGroup, terminal: undefined, childExit: undefined, childExitSignal: undefined, instructionsTransportDir, cleanupDeadlineAt: undefined, quarantined: false, nextId: 0, ipcClosed: false, acknowledgedIpcIds: new Set(), acknowledgedIpcOrder: [], stderrTail: "" }; if (request.prepareResponseObservations) { let ready = false; live.observationPreparation = () => ready; @@ -515,7 +556,7 @@ export class AgentRunner { const tail = live.stderrTail + chunk; live.stderrTail = tail.length > STDERR_TAIL_MAX ? tail.slice(-STDERR_TAIL_MAX) : tail; }); - child.on("exit", (code) => this.exited(id, code)); + child.on("exit", (code, signal) => this.exited(id, code, signal)); void this.send(id, { type: "get_state" }).then((response) => { const data = response.data as { sessionFile?: unknown; model?: { provider?: unknown; id?: unknown } | null; thinkingLevel?: unknown } | undefined; if (response.success !== true || live.terminal || this.live.get(id) !== live || !data) return; @@ -853,6 +894,7 @@ export class AgentRunner { if (this.deps.now() >= (live.cleanupDeadlineAt ?? 0)) { live.cancelGrace(); live.quarantined = true; + this.cleanupLive(live); this.finish(id, TASK_STATUS.FAILED, `process cleanup unconfirmed after ${GROUP_CONFIRM_DEADLINE_MS}ms; capacity quarantined`, live); return; } @@ -877,12 +919,24 @@ export class AgentRunner { if (this.deps.now() >= (live.cleanupDeadlineAt ?? 0)) { live.cancelGrace(); live.quarantined = true; + this.cleanupLive(live); this.finish(id, TASK_STATUS.FAILED, `child exit unconfirmed after ${GROUP_CONFIRM_DEADLINE_MS}ms; capacity quarantined`, live); return; } live.cancelGrace = this.deps.schedule(() => this.confirmGroupExit(id, live), GROUP_CONFIRM_MS); } + private cleanupLive(live: LiveTask): void { + live.permissionBroker?.close(); + this.closeIpc(live); + live.cancelStall(); + live.cancelGrace(); + if (live.instructionsTransportDir) { + try { rmSync(live.instructionsTransportDir, { recursive: true, force: true }); } catch { /* best effort */ } + live.instructionsTransportDir = undefined; + } + } + private childError(id: string, error: Error): void { const live = this.live.get(id); if (!live) return; @@ -892,30 +946,26 @@ export class AgentRunner { this.requestStop(id, TASK_STATUS.FAILED, `pi process error: ${error.message}`); return; } - live.permissionBroker?.close(); - this.closeIpc(live); - live.cancelStall(); - live.cancelGrace(); + this.cleanupLive(live); this.live.delete(id); this.finish(id, TASK_STATUS.FAILED, `could not start pi: ${error.message}`, live); } - private exited(id: string, code: number | null): void { + private exited(id: string, code: number | null, signal?: NodeJS.Signals | string | null): void { const live = this.live.get(id); if (!live) return; live.childExit = code; + live.childExitSignal = signal ?? null; + const exitDesc = formatChildExit(code, signal); if (this.groupExists(live)) { - if (!live.terminal) this.requestStop(id, TASK_STATUS.FAILED, `pi exited with code ${code ?? "unknown"} before agent_settled${this.stderrSuffix(live)}`); + if (!live.terminal) this.requestStop(id, TASK_STATUS.FAILED, `pi exited with ${exitDesc} before agent_settled${this.stderrSuffix(live)}`); return; } this.completeExit(id, live); } private completeExit(id: string, live: LiveTask): void { - live.permissionBroker?.close(); - this.closeIpc(live); - live.cancelStall(); - live.cancelGrace(); + this.cleanupLive(live); this.live.delete(id); // Quarantine already notified completion, but its retained slot is now free. if (live.quarantined) { @@ -923,7 +973,8 @@ export class AgentRunner { return; } const terminal = live.terminal; - this.finish(id, terminal ? terminal.status : TASK_STATUS.FAILED, terminal ? terminal.error : `pi exited with code ${live.childExit ?? "unknown"} before agent_settled${this.stderrSuffix(live)}`, live); + const exitDesc = formatChildExit(live.childExit, live.childExitSignal); + this.finish(id, terminal ? terminal.status : TASK_STATUS.FAILED, terminal ? terminal.error : `pi exited with ${exitDesc} before agent_settled${this.stderrSuffix(live)}`, live); } private finish(id: string, status: TaskRecord["status"], error: string | null, live?: LiveTask): void { diff --git a/odd/tasks/pr-1382-review-fixes.md b/odd/tasks/pr-1382-review-fixes.md new file mode 100644 index 000000000..4c5e0159c --- /dev/null +++ b/odd/tasks/pr-1382-review-fixes.md @@ -0,0 +1,29 @@ +# PR #1382 Review Fixes + +## Objective + +Close the verified CodeRabbit review findings on PR #1382 (`#1373` large child instruction transport) by adding regression tests for failure paths during temporary file transport and child startup. + +## Problem + +PR #1382 added owner-only temporary file transport for large agent instructions and cleanup routines in `lib/agents-runner.ts`. While the happy path and synchronous spawn throw path were tested, coverage was missing for: +1. Instruction write failure after the transport directory is created on disk. +2. Early child error before a PID is assigned (`pid === undefined`). + +## Scope + +- Add regression tests in `tests/agents-runner.test.ts` covering both failure paths. +- Assert that in each case the task fails with an informative error and the temporary transport directory and file are completely cleaned up. +- Verify test suite and typecheck. + +## Constraints + +- Keep the patch minimal and limited to PR #1382 review findings. +- Technical artifacts remain in English. +- Do not commit, push, or merge without explicit user direction. + +## Tasks + +- [x] **T1 — Test transport directory cleanup on instruction write failure.** Simulate write failure after directory creation and assert task failure and directory removal. +- [x] **T2 — Test transport directory cleanup on early child error pre-PID.** Simulate early error before child PID and assert task failure and directory removal. +- [x] **T3 — Full verification.** Run test suite and typecheck. diff --git a/tests/agents-fake-child.ts b/tests/agents-fake-child.ts index 6decea2bf..28c561137 100644 --- a/tests/agents-fake-child.ts +++ b/tests/agents-fake-child.ts @@ -9,7 +9,7 @@ export interface FakeChild { child: ChildLike; written: Array>; emit(event: Record): void; - exit(code: number): void; + exit(code: number | null, signal?: string | null): void; fail(message: string): void; killed: string[]; sent: Array>; @@ -62,5 +62,5 @@ export function fakeChild(options: { exitOnKill?: boolean; pid?: number } = {}): return child; }, }; - return { child, written, killed, sent, get disconnects() { return disconnects; }, message: (event) => emitter.emit("message", event), emit: (event) => stdout.write(`${JSON.stringify(event)}\n`), exit: (code) => emitter.emit("exit", code, null), fail: (message) => emitter.emit("error", new Error(message)) }; + return { child, written, killed, sent, get disconnects() { return disconnects; }, message: (event) => emitter.emit("message", event), emit: (event) => stdout.write(`${JSON.stringify(event)}\n`), exit: (code, signal = null) => emitter.emit("exit", code, signal), fail: (message) => emitter.emit("error", new Error(message)) }; } diff --git a/tests/agents-runner.test.ts b/tests/agents-runner.test.ts index 1a1b19711..b9dbdd96a 100644 --- a/tests/agents-runner.test.ts +++ b/tests/agents-runner.test.ts @@ -1,5 +1,8 @@ import assert from "node:assert/strict"; import test from "node:test"; +import fs, { existsSync, readFileSync, statSync } from "node:fs"; +import { syncBuiltinESMExports } from "node:module"; +import { basename, dirname } from "node:path"; import { PassThrough } from "node:stream"; import { AGENT_MODE, parseAgentsConfig, resolveAgentProfile, type AgentDefinition } from "../lib/agents-config.ts"; import { TASK_STATUS, TaskStore, type TaskRecord } from "../lib/agents-protocol.ts"; @@ -1464,3 +1467,229 @@ test("ordinary tasks never inherit orphaned SDD launch metadata", async () => { h.runner.cancel(task.id); assert.equal((await h.runner.waitFor(task.id)).status, TASK_STATUS.CANCELLED); }); + +test("AgentRunner preserves the terminating signal when child exits with null code before settlement", async () => { + const { runner, children, store } = harness(); + const task = runner.run(request()); + await tick(); + assert.equal(children.length, 1); + children[0].exit(null, "SIGKILL"); + const finished = await runner.waitFor(task.id); + assert.equal(finished.status, TASK_STATUS.FAILED); + assert.equal(finished.error, "pi exited with signal SIGKILL before agent_settled"); + assert.equal(store.get(task.id)?.error, "pi exited with signal SIGKILL before agent_settled"); +}); + +test("large agent instructions are transported via owner-only temporary file rather than inline argv", async () => { + const largeInstructions = "Instructions header:\n" + "x".repeat(2500); + const largeAgent: AgentDefinition = { ...explorer, instructions: largeInstructions }; + const launches: Array<{ command: string; args: string[]; options: Parameters[2] }> = []; + const fake = fakeChild(); + let clock = 1000; + const deps: RunnerDeps = { + spawn: (command, args, options) => { + launches.push({ command, args, options }); + return fake.child; + }, + now: () => (clock += 1), + schedule: (_fn, _ms) => () => {}, + pi: { command: "pi", args: [] }, + }; + const store = new TaskStore(); + const runner = new AgentRunner(store, { maxConcurrency: 1, stallTimeoutMs: 10_000 }, deps, { + askUser: async () => ({ value: "yes" }), + }); + const task = runner.run(request({ agent: largeAgent })); + await tick(); + + assert.equal(launches.length, 1); + const promptArgIndex = launches[0].args.indexOf("--append-system-prompt"); + assert.ok(promptArgIndex !== -1, "--append-system-prompt must be present"); + const promptValue = launches[0].args[promptArgIndex + 1]; + assert.notEqual(promptValue, largeInstructions, "large instructions must not be passed inline in argv"); + assert.ok(existsSync(promptValue), "temporary instructions transport file must exist on disk"); + assert.equal(readFileSync(promptValue, "utf8"), largeInstructions, "transport file must contain the exact instructions"); + + if (process.platform !== "win32") { + const fileStat = statSync(promptValue); + assert.equal(fileStat.mode & 0o777, 0o600, "transport file must be owner-only (0o600)"); + const dirStat = statSync(dirname(promptValue)); + assert.equal(dirStat.mode & 0o777, 0o700, "transport directory must be owner-only (0o700)"); + } + + fake.exit(0); + await runner.waitFor(task.id); + assert.ok(!existsSync(promptValue), "temporary transport file must be cleaned up on child exit"); + assert.ok(!existsSync(dirname(promptValue)), "temporary transport directory must be cleaned up on child exit"); +}); + +test("temporary instructions transport file is cleaned up if spawn throws synchronously", async () => { + const largeInstructions = "Instructions header:\n" + "x".repeat(2500); + const largeAgent: AgentDefinition = { ...explorer, instructions: largeInstructions }; + let capturedPromptPath: string | undefined; + let clock = 1000; + const deps: RunnerDeps = { + spawn: (_command, args) => { + const idx = args.indexOf("--append-system-prompt"); + if (idx !== -1) capturedPromptPath = args[idx + 1]; + throw new Error("spawn failed intentionally"); + }, + now: () => (clock += 1), + schedule: (_fn, _ms) => () => {}, + pi: { command: "pi", args: [] }, + }; + const store = new TaskStore(); + const runner = new AgentRunner(store, { maxConcurrency: 1, stallTimeoutMs: 10_000 }, deps, { + askUser: async () => ({ value: "yes" }), + }); + const task = runner.run(request({ agent: largeAgent })); + await tick(); + + const finished = await runner.waitFor(task.id); + assert.equal(finished.status, TASK_STATUS.FAILED); + assert.ok(capturedPromptPath, "should have captured a transport file path"); + assert.ok(!existsSync(capturedPromptPath), "temporary transport file must be cleaned up even when spawn throws"); + assert.ok(!existsSync(dirname(capturedPromptPath)), "temporary transport directory must be cleaned up even when spawn throws"); +}); + +test("agent instructions over the byte threshold are transported via file even when under the character threshold", async () => { + const multibyteInstructions = "界".repeat(400); + assert.ok(multibyteInstructions.length < 1000 && Buffer.byteLength(multibyteInstructions, "utf8") > 1000); + const launches: string[][] = []; + const fake = fakeChild(); + let clock = 1000; + const deps: RunnerDeps = { + spawn: (_command, args) => { + launches.push(args); + return fake.child; + }, + now: () => (clock += 1), + schedule: (_fn, _ms) => () => {}, + pi: { command: "pi", args: [] }, + }; + const runner = new AgentRunner(new TaskStore(), { maxConcurrency: 1, stallTimeoutMs: 10_000 }, deps, { + askUser: async () => ({ value: "yes" }), + }); + const task = runner.run(request({ agent: { ...explorer, instructions: multibyteInstructions } })); + await tick(); + + assert.equal(launches.length, 1); + const promptValue = launches[0][launches[0].indexOf("--append-system-prompt") + 1]; + assert.notEqual(promptValue, multibyteInstructions, "multibyte instructions over the byte threshold must not be passed inline"); + assert.ok(existsSync(promptValue), "temporary instructions transport file must exist on disk"); + assert.equal(readFileSync(promptValue, "utf8"), multibyteInstructions); + + fake.exit(0); + await runner.waitFor(task.id); + assert.ok(!existsSync(dirname(promptValue)), "temporary transport directory must be cleaned up on child exit"); +}); + +test("long agent names are truncated in the instructions transport directory name", async () => { + const largeInstructions = "Instructions header:\n" + "x".repeat(2500); + const launches: string[][] = []; + const fake = fakeChild(); + let clock = 1000; + const deps: RunnerDeps = { + spawn: (_command, args) => { + launches.push(args); + return fake.child; + }, + now: () => (clock += 1), + schedule: (_fn, _ms) => () => {}, + pi: { command: "pi", args: [] }, + }; + const runner = new AgentRunner(new TaskStore(), { maxConcurrency: 1, stallTimeoutMs: 10_000 }, deps, { + askUser: async () => ({ value: "yes" }), + }); + const task = runner.run(request({ agent: { ...explorer, name: "a".repeat(300), instructions: largeInstructions } })); + await tick(); + + assert.equal(launches.length, 1, "launch must succeed despite a long agent name"); + const promptValue = launches[0][launches[0].indexOf("--append-system-prompt") + 1]; + assert.equal(readFileSync(promptValue, "utf8"), largeInstructions); + const dirName = basename(dirname(promptValue)); + assert.ok(dirName.startsWith(`gentle-pi-subagent-${"a".repeat(64)}-`), dirName); + assert.ok(dirName.length <= "gentle-pi-subagent-".length + 64 + 1 + 6, `directory name too long: ${dirName.length}`); + + fake.exit(0); + await runner.waitFor(task.id); + assert.ok(!existsSync(dirname(promptValue))); +}); + +test("temporary instructions transport directory is cleaned up if writing instructions fails", async (t) => { + // Fail only the transport write; the ESM named import is refreshed via syncBuiltinESMExports. + const originalWriteFileSync = fs.writeFileSync; + let transportDir: string | undefined; + t.mock.method(fs, "writeFileSync", (...args: Parameters) => { + const [target] = args; + if (typeof target === "string" && basename(target) === "instructions.md" && basename(dirname(target)).startsWith("gentle-pi-subagent-")) { + transportDir = dirname(target); + throw new Error("EACCES: simulated write failure"); + } + return originalWriteFileSync(...args); + }); + syncBuiltinESMExports(); + t.after(() => { + t.mock.restoreAll(); + syncBuiltinESMExports(); + }); + const failingAgent: AgentDefinition = { + ...explorer, + instructions: "Instructions header:\n" + "x".repeat(2500), + }; + let clock = 1000; + const deps: RunnerDeps = { + spawn: () => { + throw new Error("spawn should not be called when writing instructions fails"); + }, + now: () => (clock += 1), + schedule: (_fn, _ms) => () => {}, + pi: { command: "pi", args: [] }, + }; + const store = new TaskStore(); + const runner = new AgentRunner(store, { maxConcurrency: 1, stallTimeoutMs: 10_000 }, deps, { + askUser: async () => ({ value: "yes" }), + }); + const task = runner.run(request({ agent: failingAgent })); + await tick(); + + const finished = await runner.waitFor(task.id); + assert.equal(finished.status, TASK_STATUS.FAILED); + assert.match(finished.error ?? "", /could not write agent instructions: EACCES: simulated write failure/); + assert.ok(transportDir, "transport directory must have been created before the write failed"); + assert.ok(!existsSync(transportDir), "transport directory must be cleaned up on write failure"); +}); + +test("temporary instructions transport file is cleaned up if child emits an early error before PID", async () => { + const largeInstructions = "Instructions header:\n" + "x".repeat(2500); + const largeAgent: AgentDefinition = { ...explorer, instructions: largeInstructions }; + let capturedPromptPath: string | undefined; + const fake = fakeChild({ pid: undefined }); + let clock = 1000; + const deps: RunnerDeps = { + spawn: (_command, args) => { + const idx = args.indexOf("--append-system-prompt"); + if (idx !== -1) capturedPromptPath = args[idx + 1]; + queueMicrotask(() => { + fake.fail("spawn ENOENT"); + }); + return fake.child; + }, + now: () => (clock += 1), + schedule: (_fn, _ms) => () => {}, + pi: { command: "pi", args: [] }, + }; + const store = new TaskStore(); + const runner = new AgentRunner(store, { maxConcurrency: 1, stallTimeoutMs: 10_000 }, deps, { + askUser: async () => ({ value: "yes" }), + }); + const task = runner.run(request({ agent: largeAgent })); + await tick(); + + const finished = await runner.waitFor(task.id); + assert.equal(finished.status, TASK_STATUS.FAILED); + assert.match(finished.error ?? "", /could not start pi: spawn ENOENT/); + assert.ok(capturedPromptPath, "should have captured a transport file path"); + assert.ok(!existsSync(capturedPromptPath), "temporary transport file must be cleaned up on early child error"); + assert.ok(!existsSync(dirname(capturedPromptPath)), "temporary transport directory must be cleaned up on early child error"); +});