From 1f2c2272decb1abe354ee859da3c26bce10b2a28 Mon Sep 17 00:00:00 2001 From: Xavier Hamel Date: Fri, 7 Aug 2026 13:42:53 -0400 Subject: [PATCH] fix(overwatch): Fix when a coding agent hangs for a long time --- overwatch/package.json | 2 +- overwatch/src/agents.test.ts | 86 +++++++++++++++++++++++++++++++++++ overwatch/src/agents.ts | 38 ++++++++++++++-- overwatch/src/control-loop.ts | 9 ++-- overwatch/src/sandbox.test.ts | 50 +++++++++++++++++++- overwatch/src/sandbox.ts | 33 ++++++++++++++ 6 files changed, 207 insertions(+), 11 deletions(-) create mode 100644 overwatch/src/agents.test.ts diff --git a/overwatch/package.json b/overwatch/package.json index 7c905d28..c9a12502 100644 --- a/overwatch/package.json +++ b/overwatch/package.json @@ -1,6 +1,6 @@ { "name": "@bpinternal/overwatch", - "version": "0.3.6", + "version": "0.3.7", "description": "Headless library for building control loops: scheduled bots that scan a repo for a class of anomaly, fix a few instances with a coding agent, and open a PR — no server, UI, or persistent process.", "keywords": [ "control-loop", diff --git a/overwatch/src/agents.test.ts b/overwatch/src/agents.test.ts new file mode 100644 index 00000000..7f7b44f1 --- /dev/null +++ b/overwatch/src/agents.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, test } from "vitest"; +import { Claude, Codex, type AgentContext } from "./agents"; + +type ExecCall = { command: string; env?: Record; timeoutSec?: number }; + +/** + * AgentContext stand-in that records every exec. `logOutput` stands in for what the + * agent left in the redirect file, so a test can tell the log apart from whatever the + * exec itself reported. + */ +function fakeCtx(over: { exitCode?: number; logOutput?: string; logExitCode?: number } = {}): { + ctx: AgentContext; + calls: ExecCall[]; + files: Map; +} { + const calls: ExecCall[] = []; + const files = new Map(); + const ctx: AgentContext = { + exec: async (command, options) => { + calls.push({ command, env: options?.env, timeoutSec: options?.timeoutSec }); + if (command.startsWith("cat ")) { + return { exitCode: over.logExitCode ?? 0, output: over.logOutput ?? "" }; + } + return { exitCode: over.exitCode ?? 0, output: "exec-side output" }; + }, + writeFile: async (path, content) => { + files.set(path, content); + }, + }; + return { ctx, calls, files }; +} + +const agentCall = (calls: ExecCall[]): ExecCall => calls.find((c) => !c.command.startsWith("cat "))!; + +describe.each([ + { + name: "Claude", + make: () => new Claude({ apiKey: "k", timeoutSec: 120 }), + cli: "claude -p", + envKey: "ANTHROPIC_API_KEY", + }, + { + name: "Codex", + make: () => new Codex({ apiKey: "k", timeoutSec: 120 }), + cli: "codex exec", + envKey: "OPENAI_API_KEY", + }, +])("$name.executeAgent", ({ make, cli, envKey }) => { + test("detaches all three streams from the exec's pipes", async () => { + const { ctx, calls } = fakeCtx(); + await make().executeAgent("fix it", ctx); + const command = agentCall(calls).command; + expect(command).toContain(cli); + expect(command).toContain(".control-loop/agent.log 2>&1"); + }); + + test("stages the prompt rather than passing it as a shell argument", async () => { + const { ctx, files } = fakeCtx(); + await make().executeAgent("fix it; rm -rf /", ctx); + expect(files.get(".control-loop/prompt.txt")).toBe("fix it; rm -rf /"); + }); + + test("passes the api key and timeout to the agent command", async () => { + const { ctx, calls } = fakeCtx(); + await make().executeAgent("fix it", ctx); + const call = agentCall(calls); + expect(call.env?.[envKey]).toBe("k"); + expect(call.timeoutSec).toBe(120); + }); + + test("reports the redirected log when the agent fails", async () => { + const { ctx } = fakeCtx({ exitCode: 2, logOutput: "agent stack trace" }); + await expect(make().executeAgent("fix it", ctx)).rejects.toThrow(/exited 2: agent stack trace/); + }); + + test("falls back to the exec's own output when the log is missing", async () => { + const { ctx } = fakeCtx({ exitCode: 2, logExitCode: 1 }); + await expect(make().executeAgent("fix it", ctx)).rejects.toThrow(/exited 2: exec-side output/); + }); + + test("resolves without reading the log as an error when the agent succeeds", async () => { + const { ctx } = fakeCtx({ exitCode: 0, logOutput: "all good" }); + await expect(make().executeAgent("fix it", ctx)).resolves.toBeUndefined(); + }); +}); diff --git a/overwatch/src/agents.ts b/overwatch/src/agents.ts index f269d7e0..cb3f3251 100644 --- a/overwatch/src/agents.ts +++ b/overwatch/src/agents.ts @@ -26,6 +26,7 @@ export abstract class Agent { } const PROMPT_FILE = ".control-loop/prompt.txt"; +const LOG_FILE = ".control-loop/agent.log"; /** * Writes the prompt to a scratch file and returns a shell fragment that expands to its @@ -37,6 +38,33 @@ async function stagePrompt(instructions: string, ctx: AgentContext): Promise; timeoutSec?: number }, +): Promise<{ exitCode: number; output: string }> { + const run = await ctx.exec(`${command} ${LOG_FILE} 2>&1`, options); + const log = await ctx.exec(`cat ${LOG_FILE}`); + // A missing log means the redirect itself never happened; the exec's own output is + // then the only account of what went wrong. + return { exitCode: run.exitCode, output: log.exitCode === 0 ? log.output : run.output }; +} + export type ClaudeProps = { apiKey: string; /** e.g. "claude-sonnet-5". Defaults to the CLI's default model. */ @@ -62,7 +90,7 @@ export class Claude extends Agent { override async executeAgent(instructions: string, ctx: AgentContext): Promise { const prompt = await stagePrompt(instructions, ctx); const model = this.props.model ? ` --model ${this.props.model}` : ""; - const result = await ctx.exec(`claude -p ${prompt}${model} --dangerously-skip-permissions`, { + const result = await runAgentCli(`claude -p ${prompt}${model} --dangerously-skip-permissions`, ctx, { env: { ANTHROPIC_API_KEY: this.props.apiKey }, timeoutSec: this.props.timeoutSec ?? 900, }); @@ -113,10 +141,10 @@ export class Codex extends Agent { const model = this.props.model ? ` --model ${this.props.model}` : ""; // Codex's own sandbox (--sandbox workspace-write) relies on Landlock/seccomp, which // fails inside containers — and the Daytona sandbox already isolates everything, so - // bypassing it is what Codex documents for CI/container use. { function makeAgentContext(sandbox: Sandbox): AgentContext { return { exec: async (command, options) => { - const response = await sandbox.process.executeCommand( + const timeoutSec = options?.timeoutSec ?? 300; + const response = await withDeadline( + sandbox.process.executeCommand(command, REPO_PATH, options?.env, timeoutSec), + timeoutSec, command, - REPO_PATH, - options?.env, - options?.timeoutSec ?? 300, ); return { exitCode: response.exitCode, output: response.result }; }, diff --git a/overwatch/src/sandbox.test.ts b/overwatch/src/sandbox.test.ts index e30e452e..74d1e09c 100644 --- a/overwatch/src/sandbox.test.ts +++ b/overwatch/src/sandbox.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, test } from "vitest"; +import { afterEach, describe, expect, test, vi } from "vitest"; import { appendMemory, commitAll, @@ -6,6 +6,7 @@ import { memoryPath, readMemory, runConfiguredCommand, + withDeadline, } from "./sandbox"; import type { RunLog } from "./log"; import type { AgentContext } from "./agents"; @@ -37,6 +38,53 @@ function fakeCtx(execImpl?: (command: string) => { exitCode: number; output: str const silentLog = () => ({ step: async (_label: string, fn: () => Promise) => fn() }) as unknown as RunLog; +describe("withDeadline", () => { + afterEach(() => { + vi.useRealTimers(); + }); + + test("passes through a result that arrives in time", async () => { + await expect(withDeadline(Promise.resolve("ok"), 900, "ls")).resolves.toBe("ok"); + }); + + test("passes through a rejection instead of waiting for the deadline", async () => { + await expect(withDeadline(Promise.reject(new Error("boom")), 900, "ls")).rejects.toThrow("boom"); + }); + + test("rejects with the command once the grace window past the timeout elapses", async () => { + vi.useFakeTimers(); + const hung = withDeadline(new Promise(() => {}), 900, "codex exec"); + const asserted = expect(hung).rejects.toThrow(/no response within 960s: codex exec/); + await vi.advanceTimersByTimeAsync(960_000); + await asserted; + }); + + test("holds on past the command's own timeout, leaving room for a server-side error", async () => { + vi.useFakeTimers(); + let settled = false; + const hung = withDeadline(new Promise(() => {}), 900, "codex exec").catch(() => { + settled = true; + }); + await vi.advanceTimersByTimeAsync(900_000); + expect(settled).toBe(false); + await vi.advanceTimersByTimeAsync(60_000); + await hung; + expect(settled).toBe(true); + }); + + test("treats a non-positive timeout as no timeout", async () => { + vi.useFakeTimers(); + let settled = false; + const forever = withDeadline(new Promise(() => {}), 0, "sleep").then(() => { + settled = true; + }); + await vi.advanceTimersByTimeAsync(3_600_000); + expect(settled).toBe(false); + expect(vi.getTimerCount()).toBe(0); + void forever; + }); +}); + describe("memoryPath", () => { test("namespaces the memory file by loop label", () => { expect(memoryPath("my-loop")).toBe(".github/control-loop/memory-my-loop.md"); diff --git a/overwatch/src/sandbox.ts b/overwatch/src/sandbox.ts index 10116f68..26d9c611 100644 --- a/overwatch/src/sandbox.ts +++ b/overwatch/src/sandbox.ts @@ -30,6 +30,39 @@ function shellQuote(value: string): string { return `'${value.replace(/'/g, `'\\''`)}'`; } +/** + * Extra seconds allowed past a command's own timeout before the client gives up on it. + * Wide enough that a server-side timeout, which produces a real error mentioning the + * command's output, always wins the race over this comparatively blind one. + */ +const DEADLINE_GRACE_SEC = 60; + +/** + * Arms a client-side deadline on a sandbox command. + * + * The sandbox SDK sends `timeout` as a field in the request body and never arms one + * locally — no axios timeout, no abort signal — so the promise settles only when the + * server sends a response. When it doesn't (a connection dropped mid-command, a process + * that never releases the exec) the run hangs indefinitely with no way back. Racing the + * call against a timer turns that into an ordinary failure the loop can report. + * + * A non-positive `timeoutSec` means "no timeout" and is passed through untouched. + */ +export function withDeadline(work: Promise, timeoutSec: number, command: string): Promise { + if (timeoutSec <= 0) return work; + const limit = timeoutSec + DEADLINE_GRACE_SEC; + let timer: ReturnType | undefined; + const deadline = new Promise((_resolve, reject) => { + timer = setTimeout(() => { + reject(new Error(`command produced no response within ${limit}s: ${command}`)); + }, limit * 1000); + }); + // The losing promise keeps running; unref'ing the timer keeps a pending deadline from + // holding the process open once the run itself is done. + timer?.unref?.(); + return Promise.race([work, deadline]).finally(() => clearTimeout(timer)); +} + /** * Runs one of the configured lifecycle hooks (`config.hooks`) from the repo root, * as a logged step. A missing hook is a no-op; a non-zero exit aborts the run.