diff --git a/packages/agent-backend/src/lib/cli-runner.ts b/packages/agent-backend/src/lib/cli-runner.ts index ba7fc8d7..5eb9d2ea 100644 --- a/packages/agent-backend/src/lib/cli-runner.ts +++ b/packages/agent-backend/src/lib/cli-runner.ts @@ -7,6 +7,7 @@ import { formatAgentCliNotFoundRemediation, } from "./agent-cli-not-found.js" import { + collectChildStderrTail, collectChildStdout, collectChildStdoutAndStderr, } from "./collect-child-stdout.js" @@ -20,6 +21,7 @@ import { AgentBackendTimeoutError, } from "./errors.js" import { killProcessTree } from "./kill-process-tree.js" +import { sanitizeAgentBackendStderrTail } from "./sanitize-exit-message.js" import type { AgentBackendDescriptor, OnSessionId } from "./types.js" /** Graceful terminate then force-kill bound for the Agent Turn process tree. */ @@ -143,8 +145,8 @@ const commandOptions = (input: { env: input.env, extendEnv: false as const, stdin: input.stdin ?? ("ignore" as const), - // Turns always ignore stderr so a chatty CLI cannot fill an undrained pipe. - // Capture probes opt in via captureStderr on runCliCapture only. + // Piped streams must be drained inside the same raced collect as exitCode. + // Never leave a piped stream undrained (an undrained pipe can deadlock). stderr: (input.captureStderr === true ? "pipe" : "ignore") as | "pipe" | "ignore", @@ -270,7 +272,8 @@ export const runCliCapture = ( /** * Run a CLI Agent Turn: stream stdout lines, observe early Session ID, fold - * ordered assistant text, require a Session ID on success. + * ordered assistant text, drain stderr into a bounded tail, require a + * Session ID on success. * * A startup-only inactivity bound fails the turn as soon as it is clear the CLI * never started (no stdout output and no successful `observeStartup` within @@ -300,8 +303,7 @@ export const runCliTurn = ( const command = ChildProcess.make( input.binary, [...input.args], - // Never pipe stderr for turns (undrained stderr can deadlock). - commandOptions({ ...input, captureStderr: false }), + commandOptions({ ...input, captureStderr: true }), ) const result = yield* Effect.scoped( @@ -443,12 +445,19 @@ export const runCliTurn = ( ), ) - const [exitOutcome, output] = yield* Effect.all( - [handle.exitCode.pipe(Effect.result), collectOutput], - { concurrency: 2 }, + const [exitOutcome, output, stderrTail] = yield* Effect.all( + [ + handle.exitCode.pipe(Effect.result), + collectOutput, + collectChildStderrTail(handle), + ], + { concurrency: 3 }, ).pipe( // raceFirst so the armed watchdog failure ends the turn immediately // instead of waiting for the silent CLI to exit on its own. + // The stderr fold is inside this race so a piped stream is never + // left undrained, a finalize kill is not blocked waiting for it, + // and interrupting the race cannot leak the fold fiber. Effect.raceFirst(startupWatchdog), ) @@ -459,6 +468,7 @@ export const runCliTurn = ( finalized: output.finalized, errorClassification: output.errorClassification, errorMessage: output.errorMessage, + stderrTail, } }), ).pipe( @@ -485,6 +495,9 @@ export const runCliTurn = ( const exitCode = Number(result.exitOutcome.success) if (exitCode !== 0) { const sessionId = result.sessionId ?? knownSessionId + const message = + result.errorMessage ?? + sanitizeAgentBackendStderrTail(result.stderrTail) return yield* AgentBackendExitError.new({ exitCode, cwd: input.cwd, @@ -492,9 +505,7 @@ export const runCliTurn = ( ...(result.errorClassification !== undefined ? { classification: result.errorClassification } : {}), - ...(result.errorMessage !== undefined - ? { message: result.errorMessage } - : {}), + ...(message !== undefined ? { message } : {}), }) } } diff --git a/packages/agent-backend/src/lib/collect-child-stdout.ts b/packages/agent-backend/src/lib/collect-child-stdout.ts index d15a6b1e..68deb624 100644 --- a/packages/agent-backend/src/lib/collect-child-stdout.ts +++ b/packages/agent-backend/src/lib/collect-child-stdout.ts @@ -23,6 +23,41 @@ export const collectChildStdout = ( } }) +/** + * Memory bound for an Agent Turn stderr fold. Only this many characters + * are retained; older output is dropped. Matches the Install Dependencies + * diagnostic tail so a chatty CLI cannot grow unbounded. + */ +const CLI_TURN_STDERR_TAIL_LIMIT = 4_000 + +const appendStderrTail = ( + tail: string, + chunk: string, + limit = CLI_TURN_STDERR_TAIL_LIMIT, +): string => { + const combined = `${tail}${chunk}` + return combined.length <= limit + ? combined + : `…${combined.slice(-(limit - 1))}` +} + +/** + * Drain stderr to EOF, keeping only the most recent `limit` characters. + * + * Must be composed into the same concurrent collect as `exitCode` (and + * stdout) whenever stderr is piped — never leave a piped stream undrained. + */ +export const collectChildStderrTail = ( + handle: ChildProcessHandle, + limit = CLI_TURN_STDERR_TAIL_LIMIT, +): Effect.Effect => + Stream.decodeText(handle.stderr).pipe( + Stream.runFold( + () => "", + (tail, chunk) => appendStderrTail(tail, chunk, limit), + ), + ) + /** * Drain stdout and stderr concurrently to EOF, then read exit code. * diff --git a/packages/agent-backend/src/lib/errors.ts b/packages/agent-backend/src/lib/errors.ts index a81d6ed7..c43281ec 100644 --- a/packages/agent-backend/src/lib/errors.ts +++ b/packages/agent-backend/src/lib/errors.ts @@ -71,8 +71,8 @@ export class AgentBackendExitError extends Schema.TaggedErrorClass + text.replace(ANSI_ESCAPE_RE, "").replace(TOKEN_SHAPED_RE, "[redacted]").trim() + export const sanitizeAgentBackendExitMessage = (text: string): string => - text - .replace(ANSI_ESCAPE_RE, "") - .replace(TOKEN_SHAPED_RE, "[redacted]") - .trim() - .slice(0, AGENT_BACKEND_EXIT_MESSAGE_MAX) + cleanOperatorFacingText(text).slice(0, AGENT_BACKEND_EXIT_MESSAGE_MAX) + +/** + * Redact the full captured tail, then keep the most recent bound so a + * token that would straddle a raw suffix cut is already gone. + */ +export const sanitizeAgentBackendStderrTail = ( + text: string, +): string | undefined => { + const cleaned = cleanOperatorFacingText(text) + if (cleaned.length === 0) { + return undefined + } + return cleaned.length <= AGENT_BACKEND_EXIT_MESSAGE_MAX + ? cleaned + : cleaned.slice(-AGENT_BACKEND_EXIT_MESSAGE_MAX) +} diff --git a/packages/agent-backend/test/cli-runner.spec.ts b/packages/agent-backend/test/cli-runner.spec.ts index c8277b4b..229c5234 100644 --- a/packages/agent-backend/test/cli-runner.spec.ts +++ b/packages/agent-backend/test/cli-runner.spec.ts @@ -4,13 +4,14 @@ import { join } from "node:path" import { BunServices } from "@effect/platform-bun" import { Deferred, Duration, Effect, Exit, Fiber } from "effect" import { systemError } from "effect/PlatformError" -import { ChildProcessSpawner } from "effect/unstable/process" +import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process" import { AgentBackendExitError, AgentBackendNotInstalledError, AgentBackendSessionIdMissingError, AgentBackendStartupTimeoutError, AgentBackendTimeoutError, + collectChildStderrTail, runCliCapture, runCliTurn, sanitizeInheritedEnvironment, @@ -963,6 +964,337 @@ describe("runCliTurn", () => { }, ) }) + + it("puts a stderr-only failure reason on AgentBackendExitError", async () => { + await withExecutable( + [ + "printf 'Error: Token has expired and refresh failed\\n' >&2", + "exit 1", + ].join("\n"), + async (binary) => { + const error = await Effect.runPromise( + withSpawner((spawner) => + runCliTurn({ + spawner, + backend: TEST_BACKEND, + binary, + args: [], + cwd: process.cwd(), + env: sanitizeInheritedEnvironment(), + timeout: Duration.seconds(2), + parseLine: parseSimpleLine, + }).pipe(Effect.flip), + ), + ) + expect(error).toEqual( + AgentBackendExitError.new({ + exitCode: 1, + cwd: process.cwd(), + message: "Error: Token has expired and refresh failed", + }), + ) + }, + ) + }) + + it("prefers an adapter-supplied reason over the stderr tail", async () => { + await withExecutable( + [ + `printf '%s\\n' '{"sessionID":"ses_reason","errorMessage":"model overloaded"}'`, + "printf 'raw stderr should lose\\n' >&2", + "exit 1", + ].join("\n"), + async (binary) => { + const error = await Effect.runPromise( + withSpawner((spawner) => + runCliTurn({ + spawner, + backend: TEST_BACKEND, + binary, + args: [], + cwd: process.cwd(), + env: sanitizeInheritedEnvironment(), + timeout: Duration.seconds(2), + parseLine: parseSimpleLine, + }).pipe(Effect.flip), + ), + ) + expect(error).toEqual( + AgentBackendExitError.new({ + exitCode: 1, + cwd: process.cwd(), + sessionId: "ses_reason", + message: "model overloaded", + }), + ) + }, + ) + }) + + it("completes a CLI that floods stderr and keeps only the most recent tail", async () => { + await withExecutable( + [ + "printf 'prefix-marker' >&2", + "printf '%05000d' 0 >&2", + "printf 'tail-marker' >&2", + "exit 1", + ].join("\n"), + async (binary) => { + const error = await Effect.runPromise( + withSpawner((spawner) => + runCliTurn({ + spawner, + backend: TEST_BACKEND, + binary, + args: [], + cwd: process.cwd(), + env: sanitizeInheritedEnvironment(), + timeout: Duration.seconds(5), + parseLine: parseSimpleLine, + }).pipe(Effect.flip), + ), + ) + expect(error).toBeInstanceOf(AgentBackendExitError) + if (error instanceof AgentBackendExitError) { + expect(error.message).toContain("tail-marker") + expect(error.message).not.toContain("prefix-marker") + expect(error.message.length).toBeLessThanOrEqual(500) + } + }, + ) + }) + + it("bounds the stderr fold to the last 4000 characters", async () => { + await withExecutable( + [ + "printf 'prefix-marker' >&2", + "printf '%05000d' 0 >&2", + "printf 'tail-marker' >&2", + ].join("\n"), + async (binary) => { + const fold = await Effect.runPromise( + withSpawner((spawner) => + Effect.scoped( + Effect.gen(function* () { + const handle = yield* spawner.spawn( + ChildProcess.make(binary, [], { + cwd: process.cwd(), + stdin: "ignore", + stderr: "pipe", + }), + ) + const [tail] = yield* Effect.all( + [collectChildStderrTail(handle), handle.exitCode], + { concurrency: 2 }, + ) + return tail + }), + ), + ), + ) + expect(fold).toContain("tail-marker") + expect(fold).not.toContain("prefix-marker") + expect(fold.length).toBeLessThanOrEqual(4_000) + }, + ) + }) + + it("sanitizes the stderr tail before it becomes the exit message", async () => { + const secret = "ghp_this_must_never_appear_in_exit_message" + const esc = String.fromCharCode(0x1b) + await withExecutable( + [ + `printf '${esc}[31mauth failed with ${secret}${esc}[0m\\n' >&2`, + "exit 1", + ].join("\n"), + async (binary) => { + const error = await Effect.runPromise( + withSpawner((spawner) => + runCliTurn({ + spawner, + backend: TEST_BACKEND, + binary, + args: [], + cwd: process.cwd(), + env: sanitizeInheritedEnvironment(), + timeout: Duration.seconds(2), + parseLine: parseSimpleLine, + }).pipe(Effect.flip), + ), + ) + expect(error).toBeInstanceOf(AgentBackendExitError) + if (error instanceof AgentBackendExitError) { + expect(error.message).not.toContain(secret) + expect(error.message).not.toMatch(/ghp_[A-Za-z0-9]+/) + expect(error.message).toContain("[redacted]") + expect(error.message.includes(`${esc}[`)).toBe(false) + expect(error.message).toContain("auth failed") + } + }, + ) + }) + + it("redacts a stderr token that would be split by the message-length cut", async () => { + const secret = "ghp_this_must_never_appear_in_exit_message" + await withExecutable( + [ + "printf '%0100d ' 0 >&2", + `printf '${secret}' >&2`, + "printf ' %0470d' 1 >&2", + "exit 1", + ].join("\n"), + async (binary) => { + const error = await Effect.runPromise( + withSpawner((spawner) => + runCliTurn({ + spawner, + backend: TEST_BACKEND, + binary, + args: [], + cwd: process.cwd(), + env: sanitizeInheritedEnvironment(), + timeout: Duration.seconds(2), + parseLine: parseSimpleLine, + }).pipe(Effect.flip), + ), + ) + expect(error).toBeInstanceOf(AgentBackendExitError) + if (error instanceof AgentBackendExitError) { + expect(error.message).toContain("[redacted]") + expect(error.message).not.toContain(secret) + expect(error.message).not.toMatch(/ghp_[A-Za-z0-9_]+/) + expect(error.message.includes("this_must_never")).toBe(false) + } + }, + ) + }) + + it("does not delay finalize tree-kill when stderr keeps flowing", async () => { + const markerDir = await mkdtemp(join(tmpdir(), "agent-backend-stderr-fin-")) + const childAlive = join(markerDir, "child-alive") + const grandPidFile = join(markerDir, "grand.pid") + try { + await withExecutable( + [ + `setsid sh -c 'echo $$ > "${grandPidFile}"; while true; do printf "noise\\n" >&2; touch "${childAlive}"; sleep 0.05; done' &`, + `while [ ! -s "${grandPidFile}" ]; do sleep 0.01; done`, + `printf '%s\\n' '{"sessionID":"ses_fin_err","finalize":"done"}'`, + "sleep 100", + ].join("\n"), + async (binary) => { + const startedAt = Date.now() + const result = await Effect.runPromise( + withSpawner((spawner) => + runCliTurn({ + spawner, + backend: TEST_BACKEND, + binary, + args: [], + cwd: process.cwd(), + env: sanitizeInheritedEnvironment(), + timeout: Duration.seconds(10), + forceKillAfter: Duration.millis(100), + parseLine: (line) => { + try { + const parsed = JSON.parse(line) as { + sessionID?: string + finalize?: string + } + if ( + typeof parsed.sessionID === "string" && + typeof parsed.finalize === "string" + ) { + return { + sessionId: parsed.sessionID, + finalizeText: parsed.finalize, + } + } + return parseSimpleLine(line) + } catch { + return {} + } + }, + }), + ), + ) + const elapsed = Date.now() - startedAt + + expect(result).toEqual({ + sessionId: "ses_fin_err", + assistantText: "done", + }) + expect(elapsed).toBeLessThan(2_000) + + await Bun.sleep(300) + const grandPid = Number( + ( + await Bun.file(grandPidFile) + .text() + .catch(() => "") + ).trim(), + ) + expect(Number.isFinite(grandPid) && grandPid > 0).toBe(true) + expect(isPidAlive(grandPid)).toBe(false) + }, + ) + } finally { + await rm(markerDir, { recursive: true, force: true }) + } + }) + + it("still hits the startup watchdog when the CLI writes only to stderr", async () => { + const markerDir = await mkdtemp(join(tmpdir(), "agent-backend-stderr-wd-")) + const grandPidFile = join(markerDir, "grand.pid") + try { + await withExecutable( + [ + `setsid sh -c 'echo $$ > "${grandPidFile}"; while true; do printf "auth noise\\n" >&2; sleep 0.05; done' &`, + `while [ ! -s "${grandPidFile}" ]; do sleep 0.01; done`, + "sleep 100", + ].join("\n"), + async (binary) => { + const startedAt = Date.now() + const error = await Effect.runPromise( + withSpawner((spawner) => + runCliTurn({ + spawner, + backend: TEST_BACKEND, + binary, + args: [], + cwd: process.cwd(), + env: sanitizeInheritedEnvironment(), + timeout: Duration.seconds(30), + startupTimeout: Duration.millis(300), + forceKillAfter: Duration.millis(100), + parseLine: parseSimpleLine, + }).pipe(Effect.flip), + ), + ) + const elapsed = Date.now() - startedAt + + expect(error).toEqual( + new AgentBackendStartupTimeoutError({ + cwd: process.cwd(), + startupTimeoutMs: 300, + }), + ) + expect(elapsed).toBeLessThan(5_000) + + const grandPid = Number( + ( + await Bun.file(grandPidFile) + .text() + .catch(() => "") + ).trim(), + ) + expect(Number.isFinite(grandPid) && grandPid > 0).toBe(true) + expect(isPidAlive(grandPid)).toBe(false) + }, + ) + } finally { + await rm(markerDir, { recursive: true, force: true }) + } + }) }) const enoentPlatformError = systemError({ diff --git a/packages/agent-backend/test/errors.spec.ts b/packages/agent-backend/test/errors.spec.ts index 3e32dc97..9d812af7 100644 --- a/packages/agent-backend/test/errors.spec.ts +++ b/packages/agent-backend/test/errors.spec.ts @@ -1,4 +1,5 @@ import { AgentBackendExitError } from "../src/lib/errors.js" +import { sanitizeAgentBackendStderrTail } from "../src/lib/sanitize-exit-message.js" import { describe, expect, it } from "bun:test" describe("AgentBackendExitError message", () => { @@ -36,3 +37,17 @@ describe("AgentBackendExitError message", () => { expect(error.message).toContain("[redacted]") }) }) + +describe("sanitizeAgentBackendStderrTail", () => { + it("redacts a token that straddles the message-length cut", () => { + const secret = "ghp_this_must_never_appear_in_exit_message" + const text = `${"0".repeat(100)} ${secret} ${"1".repeat(470)}` + const tail = sanitizeAgentBackendStderrTail(text) + expect(tail).toBeDefined() + expect(tail).toContain("[redacted]") + expect(tail).not.toContain(secret) + expect(tail).not.toMatch(/ghp_[A-Za-z0-9_]+/) + expect(tail?.includes("this_must_never")).toBe(false) + expect(tail?.length).toBeLessThanOrEqual(500) + }) +}) diff --git a/packages/work-item-lifecycle/test/implement.spec.ts b/packages/work-item-lifecycle/test/implement.spec.ts index 37f7fd88..347330e3 100644 --- a/packages/work-item-lifecycle/test/implement.spec.ts +++ b/packages/work-item-lifecycle/test/implement.spec.ts @@ -1,8 +1,9 @@ -import { mkdtemp, rm } from "node:fs/promises" +import { chmod, mkdtemp, rm, writeFile } from "node:fs/promises" import { tmpdir } from "node:os" import { join } from "node:path" import { BunServices } from "@effect/platform-bun" import { Duration, Effect, Fiber, Layer } from "effect" +import { ChildProcessSpawner } from "effect/unstable/process" import { SqlClient } from "effect/unstable/sql" import { type ActiveAgentBackend, @@ -11,9 +12,12 @@ import { AgentBackendSessionIdMissingError, AgentBackendTimeoutError, type StartInput, + runCliTurn, + sanitizeInheritedEnvironment, } from "@ready-for-agent/agent-backend" import { DatabaseTest } from "@ready-for-agent/db/test" import { DbService, DbServiceLive } from "@ready-for-agent/db-service" +import { extractCauseChain } from "@ready-for-agent/github-service" import { KeymaxxerService, type KeymaxxerServiceShape, @@ -562,6 +566,64 @@ describe("implement", () => { expect((error as ImplementOpenCodeError).worktreePath).toBe(root) })) + it("surfaces a stderr-only Agent Turn failure in the cause chain", () => + withTemp(async (root) => { + const binary = join(root, "fake-cli") + await writeFile( + binary, + "#!/bin/sh\nprintf 'Error: Token has expired and refresh failed\\n' >&2\nexit 1\n", + ) + await chmod(binary, 0o700) + + const error = await run( + Effect.gen(function* () { + const repository = yield* seedRepository(root) + return yield* implement( + baseContext(root, { repositoryId: repository.id }), + ) + }).pipe(Effect.flip), + Layer.effect( + AgentBackend, + Effect.gen(function* () { + const spawner = yield* ChildProcessSpawner.ChildProcessSpawner + return AgentBackend.of({ + startTurn: (input) => + runCliTurn({ + spawner, + backend: { id: "opencode", label: "OpenCode" }, + binary, + args: [], + cwd: input.cwd, + env: sanitizeInheritedEnvironment(), + timeout: Duration.seconds(5), + parseLine: () => ({}), + }), + continueTurn: () => + Effect.succeed({ sessionId: "unused", assistantText: "" }), + inspect: () => + Effect.succeed({ + backend: { id: "opencode" as const, label: "OpenCode" }, + models: [], + }), + }) + }), + ).pipe(Layer.provide(PlatformLayer)), + ) + + expect(error).toBeInstanceOf(ImplementOpenCodeError) + expect(extractCauseChain(error)).toEqual([ + { + name: "ImplementOpenCodeError", + message: "OpenCode failed to implement the Work Item issue", + }, + { + name: "AgentBackendExitError", + code: "1", + message: "Error: Token has expired and refresh failed", + }, + ]) + })) + it("maps OpenCode timeout failure", () => withTemp(async (root) => { const error = await run(