diff --git a/commands/agent.tsx b/commands/agent.tsx index 226765b..a151979 100644 --- a/commands/agent.tsx +++ b/commands/agent.tsx @@ -11,7 +11,7 @@ import { registerCommands } from "./slash"; import { isCommandTool, summarizeToolOutput } from "../tui/src/tool-display"; import { parseApprovalMode } from "../runtime/approval"; import { createEventLog, now } from "../runtime/eventLog"; -import { IterationBudgetExhaustedError } from "../runtime/loop"; +import { BudgetExhaustedError } from "../runtime/loop"; import { enableSandbox } from "../runtime/sandbox"; import { VERSION } from "../config/version"; import { PassThrough } from "stream"; @@ -297,7 +297,7 @@ async function runHeadless( }, onError(error) { failed = true; - if (error instanceof IterationBudgetExhaustedError) budgetExhausted = true; + if (error instanceof BudgetExhaustedError) budgetExhausted = true; log.write({ type: "error", ts: now(), message: error.message }); process.stderr.write(`✖ ${error.message}\n`); }, @@ -320,7 +320,7 @@ async function runHeadless( await controller.run(prompt); } catch (error) { failed = true; - if (error instanceof IterationBudgetExhaustedError) budgetExhausted = true; + if (error instanceof BudgetExhaustedError) budgetExhausted = true; const message = error instanceof Error ? error.message : String(error); log.write({ type: "error", ts: now(), message }); process.stderr.write(`✖ ${message}\n`); @@ -333,8 +333,11 @@ async function runHeadless( process.stdout.write("\n"); // Exit codes are a contract with automated callers: // 0 - the turn completed - // 2 - the loop ran out of iterations; work may be partially done, and the - // caller should judge the result rather than treat this as a crash + // 2 - the loop ran out of budget, of iterations or of wall-clock time; work + // may be partially done, and the caller should judge the result rather + // than treat this as a crash. One code for both: a distinct one for the + // deadline would be booked as an exception by any harness not yet + // updated to know it, dropping those trials from the mean. // 1 - anything else went wrong process.exit(failed ? (budgetExhausted ? EXIT_BUDGET_EXHAUSTED : 1) : 0); } diff --git a/config/types.ts b/config/types.ts index ef2d8c2..65d59ea 100644 --- a/config/types.ts +++ b/config/types.ts @@ -291,6 +291,11 @@ export interface AgentCallbacks { * nobody is there to ask, so the loop raises `IterationBudgetExhaustedError` * as it always has. Headless runs deliberately do not implement it, which is * what keeps their exit-code contract. + * + * The step ceiling is the only budget that asks. A wall-clock deadline + * (`WOOPCODE_MAX_WALL_SEC`) never consults this and raises + * `WallBudgetExhaustedError` directly: iterations do not tick while a human + * thinks about the question, and a clock does. */ onBudgetExhausted?(info: { steps: number }): Promise; /** Reported once per completed iteration, before the next one starts. */ diff --git a/docs/adr/0001-wall-clock-budget-for-the-agent-loop.md b/docs/adr/0001-wall-clock-budget-for-the-agent-loop.md new file mode 100644 index 0000000..bcb7d6e --- /dev/null +++ b/docs/adr/0001-wall-clock-budget-for-the-agent-loop.md @@ -0,0 +1,108 @@ +--- +title: Wall-clock budget for the agent loop +type: concept +summary: Why the loop measures a second budget in seconds, what the reserve holds back, and which alternatives were rejected. +prerequisites: [] +related: + - /docs/reference/configuration +since: 1.1.0 +--- + +# Wall-clock budget for the agent loop + +Status: accepted + +The loop has only ever measured its budget in iterations. Harbor enforces a +wall clock. Running the checked-in five-task config with +`WOOPCODE_MAX_ITERATIONS=200`, every trial finished having spent between 2% and +23% of the time it was given, and `make-mips-interpreter` was killed by its own +200th iteration at 406s of 1800 — mid-work, with `exception_info: null` proving +Harbor's timeout never fired. So the loop gets a second budget, +`WOOPCODE_MAX_WALL_SEC`, and stops on whichever of the two binds first. + +## The measurement + +Per-task agent timeouts come from each task package's `task.toml` +(`~/.cache/harbor/tasks/packages/terminal-bench///task.toml`). +Against the `jobs/tb2-post-1.1` run: + +| task | timeout | wall used | unused | iterations | what stopped it | +| --- | --- | --- | --- | --- | --- | +| build-pov-ray | 12000s | 287s | 98% | 79 | model chose to | +| circuit-fibsqrt | 3600s | 400s | 89% | 165 | model chose to | +| make-mips-interpreter | 1800s | 406s | **77%** | **200** | **our ceiling** | +| overfull-hbox | 750s | 192s | 74% | 59 | model chose to | +| video-processing | 3600s | 261s | 93% | 83 | model chose to | + +CLAUDE.md's benchmarking section records the opposite rule — *"Wall clock is the +binding budget, not iterations"* — and a future reader will find it and assume +this change is a mistake. That claim was measured on `overfull-hbox`, which has +the shortest timeout in the set by 2.4×. It does not generalise to the other +four, and correcting it is part of this work. + +Iteration rate, measured: `make-mips-interpreter` averaged 2.03s of wall per +iteration (1.77s of it provider time), so its 1800s had room for roughly 885. + +## What was decided, and the alternatives + +**Both budgets stand; neither replaces the other.** With the wall bounding +spend, the iteration ceiling reverts to the role `loop.ts` already claims for it +— a guard against a pathological loop with nobody watching — and `job.yaml`'s +`max_iterations` goes 200 → 1000 so it stops binding first. + +Rejected: **raising `max_iterations` alone.** Zero code, immediately testable, +but the loop still cannot see a clock, so on `overfull-hbox`'s 750s a slow run +gets hard-killed by Harbor mid-work instead of winding down. Also rejected: +**per-task iteration budgets** derived from each timeout ÷ measured rate — honest +to the data, but it is hand-tuning benchmark config per task, which is fragile +and overfits. + +**The operator passes the whole budget; the loop subtracts its own reserve.** +`agent.py` forwards Harbor's `timeout_sec` verbatim, so a published number traces +back to `task.toml` with no arithmetic in between, and the safety margin stays +one constant in one repository. Rejected: having the caller send a pre-reduced +figure (`timeout_sec * 0.9`), which splits the reserve across two repos and +scales a fixed wind-down cost proportionally, giving a 750s task the same +fraction as a 12000s one. + +**Tool timeouts are clamped to the remaining budget.** Without it the deadline +is advisory: `run_terminal` defaults to 300s and the model may ask for more, so +one command started just inside the budget outlives it by minutes — on +`overfull-hbox`, a single default-timeout call is 40% of the entire budget. + +**The deadline lives in module state (`runtime/deadline.ts`), not on +`Tool.execute`.** `runtime/sandbox/registry.ts` argues this case in its own +docstring for the same shape: three tools need it, and threading it through +would change the `Tool` interface in `config/types.ts` and every tool signature. + +**`onBudgetExhausted` is not consulted when the wall deadline binds.** The +iteration ceiling can afford to ask because iterations do not tick while a human +thinks. A clock does — and the only path with a handler is the interactive one, +which will not have the variable set. + +**`WallBudgetExhaustedError` is a sibling of `IterationBudgetExhaustedError` +under a shared `BudgetExhaustedError`, and both exit 2.** The exit contract in +`commands/agent.tsx` already means "worked, did not finish, judge the result +rather than treat this as a crash", which is exactly what a deadline produces, +and `agent.py` already maps 2 to success. A distinct exit code 3 was rejected: +until `agent.py` was updated to match, Harbor would book those trials as +exceptions and drop them from the mean, overstating measured accuracy. + +**The wind-down converts time into steps rather than warning separately.** +Remaining time ÷ the turn's own mean wall-per-iteration gives a step count, and +the existing `REMAINING_ITERATIONS_WARNING = 5` then serves both budgets through +one message and one flag. A constant expressed in seconds was rejected as the +wrong shape across this task set — 120s is 16% of `overfull-hbox`'s budget and +1% of `build-pov-ray`'s. + +## Consequences + +- `WOOPCODE_MAX_WALL_SEC` and the exit-code behaviour become a contract with + `harbor_woopcode/agent.py`. Changing either means changing both. +- The wind-down flag replaces an equality test (`iterations === budget - 5`) + that silently never fired when the ceiling was below five. +- This fixes a loop that ends early. It makes **no claim** about benchmark + reward: with five tasks at one trial each, and three failures with three + unrelated causes, there is no power to attribute a score change to it. The + claim to verify is narrower — that the loop no longer kills itself with + budget in hand. diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index c0d658d..70cec84 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -142,6 +142,7 @@ the run. | --- | --- | --- | | `WOOPCODE_PROVIDER` | `google` | Pairs with `WOOPCODE_API_KEY` | | `WOOPCODE_MAX_ITERATIONS` | `40` | Steps a turn may take before it stops to ask whether to keep going. Interactively the ceiling is a checkpoint, so it is set to catch a stuck loop rather than to ration requests — the provider rations those itself, and answering the checkpoint grants another `40`. A headless run has nobody to ask, so this is the whole budget and exhausting it exits `2` | +| `WOOPCODE_MAX_WALL_SEC` | unset (off) | Wall-clock seconds a turn may take, counted from process start. Pass the whole budget the harness enforces; a reserve is held back internally so the last step, the final answer and the session write still land. The loop stops on whichever budget binds first, and a spent clock exits `2` like a spent ceiling. Unset interactively, where a person decides when a turn has gone on too long | | `WOOPCODE_MAX_ATTEMPTS` | `3` | Tries per provider request before the error surfaces | | `WOOPCODE_TOOL_HISTORY_BUDGET` | unset (off) | Characters of tool history to keep before older results are compacted. Off by default — see the measurements in `runtime/compaction.ts` | | `WOOPCODE_THINKING_BUDGET` | `-1` | Reasoning depth; see below | diff --git a/packages/tests/runtime/deadline.test.ts b/packages/tests/runtime/deadline.test.ts new file mode 100644 index 0000000..d6e6011 --- /dev/null +++ b/packages/tests/runtime/deadline.test.ts @@ -0,0 +1,122 @@ +import { describe, test, expect, afterEach } from "bun:test"; +import { + WALL_RESERVE_SEC, + clampToBudget, + clearDeadline, + deadlineReached, + now, + remainingMs, + setDeadline, +} from "../../../runtime/deadline"; + +/** + * The deadline is module state, and module state outlives a file. + * + * A test that installed a fake clock and did not clear it would leave every + * later test in the run measuring elapsed time against a number that never + * moves — which is exactly the class of failure the injected clock exists to + * avoid in the first place. + */ +afterEach(() => { + clearDeadline(); +}); + +/** A clock the test moves by hand. */ +function fakeClock(start = 0) { + let at = start; + return { + now: () => at, + advance(ms: number) { + at += ms; + }, + }; +} + +describe("the wall-clock deadline", () => { + test("the reserve is subtracted from the budget the operator gave", () => { + const clock = fakeClock(); + setDeadline(600, { now: clock.now, startedAt: 0 }); + + expect(remainingMs()).toBe((600 - WALL_RESERVE_SEC) * 1000); + }); + + test("it is counted from process start, so a second turn cannot restart it", () => { + const clock = fakeClock(); + setDeadline(600, { now: clock.now, startedAt: 0 }); + + // A turn's worth of time passes, then the next turn sets the same budget + // again. The harness has been counting throughout, so the deadline must not + // move — a turn granted the whole budget afresh would overrun it. + clock.advance(100_000); + setDeadline(600, { now: clock.now, startedAt: 0 }); + + expect(remainingMs()).toBe((600 - WALL_RESERVE_SEC) * 1000 - 100_000); + }); + + test("the deadline is reached once the budget less the reserve is spent", () => { + const clock = fakeClock(); + setDeadline(600, { now: clock.now, startedAt: 0 }); + + clock.advance((600 - WALL_RESERVE_SEC) * 1000 - 1); + expect(deadlineReached()).toBe(false); + + clock.advance(1); + expect(deadlineReached()).toBe(true); + }); + + test("a budget no larger than the reserve is spent before it starts", () => { + const clock = fakeClock(); + setDeadline(WALL_RESERVE_SEC, { now: clock.now, startedAt: 0 }); + + expect(remainingMs()).toBe(0); + expect(deadlineReached()).toBe(true); + }); + + // Every reader has to answer "keep going" for a session that never opted in, + // or turning the feature off would end turns rather than leave them alone. + test("an unbudgeted session is never out of time", () => { + expect(deadlineReached()).toBe(false); + expect(remainingMs()).toBeUndefined(); + }); + + test("now() reads the injected clock, and clearDeadline puts the real one back", () => { + const clock = fakeClock(1_000); + setDeadline(600, { now: clock.now, startedAt: 0 }); + expect(now()).toBe(1_000); + + clearDeadline(); + // The real clock, not the fake one frozen at 1,000. + expect(now()).toBeGreaterThan(1_600_000_000_000); + }); +}); + +describe("clamping a tool timeout to the budget", () => { + test("an unbudgeted session gets the timeout it asked for", () => { + expect(clampToBudget(300)).toBe(300); + }); + + test("a timeout that fits is passed through", () => { + const clock = fakeClock(); + setDeadline(600, { now: clock.now, startedAt: 0 }); + + expect(clampToBudget(30)).toBe(30); + }); + + test("a timeout that outlives the budget is cut to what is left", () => { + const clock = fakeClock(); + setDeadline(160, { now: clock.now, startedAt: 0 }); + + // 160s less the reserve leaves 100s, so a default 300s command gets 100. + expect(clampToBudget(300)).toBe(100); + }); + + // Zero or a negative number is not a shorter timeout, it is a command killed + // before it starts — reported as a failure that says nothing about the clock. + test("the clamp never returns less than a second", () => { + const clock = fakeClock(); + setDeadline(600, { now: clock.now, startedAt: 0 }); + clock.advance((600 - WALL_RESERVE_SEC) * 1000); + + expect(clampToBudget(300)).toBe(1); + }); +}); diff --git a/packages/tests/runtime/iterationBudget.test.ts b/packages/tests/runtime/iterationBudget.test.ts index a7f1f53..c908319 100644 --- a/packages/tests/runtime/iterationBudget.test.ts +++ b/packages/tests/runtime/iterationBudget.test.ts @@ -164,14 +164,17 @@ describe("running out of budget", () => { expect(budgetNotices(messages)[0]!.content).toContain("Only 5 more steps"); }); - test("a budget too small to warn in still runs and still ends", async () => { - // The threshold is five from the end, so a budget of two never reaches it. - // The turn must still exhaust cleanly rather than warn about a negative - // number of remaining steps. + test("a budget below the warning distance is warned about immediately", async () => { + // The notice used to be an equality against the iteration count, which + // silently never fired when the whole ceiling was below the five steps of + // warning — the turns with least room to spare were the ones told nothing. + // It is a threshold now, so a budget of two is announced at the first step. process.env.WOOPCODE_MAX_ITERATIONS = "2"; const { messages } = await runKeepingMessages(); - expect(budgetNotices(messages)).toHaveLength(0); + + expect(budgetNotices(messages)).toHaveLength(1); + expect(budgetNotices(messages)[0]!.content).toContain("Only 2 more steps"); }); }); diff --git a/packages/tests/runtime/wallBudget.test.ts b/packages/tests/runtime/wallBudget.test.ts new file mode 100644 index 0000000..b729916 --- /dev/null +++ b/packages/tests/runtime/wallBudget.test.ts @@ -0,0 +1,288 @@ +import { describe, test, expect, afterEach } from "bun:test"; +import { + BudgetExhaustedError, + IterationBudgetExhaustedError, + WallBudgetExhaustedError, + agentLoop, + stepsRemaining, +} from "../../../runtime/loop"; +import { + WALL_RESERVE_SEC, + clearDeadline, + remainingMs, + setDeadline, +} from "../../../runtime/deadline"; +import { TurnState } from "../../../runtime/turnState"; +import { EXIT_BUDGET_EXHAUSTED } from "../../../commands/agent"; +import type { ProviderClient, StreamEvent } from "../../../config/types"; +import { createRuntimeTest } from "../shared/testHelpers"; + +const ORIGINAL_WALL = process.env.WOOPCODE_MAX_WALL_SEC; +const ORIGINAL_ITERATIONS = process.env.WOOPCODE_MAX_ITERATIONS; + +afterEach(() => { + if (ORIGINAL_WALL === undefined) delete process.env.WOOPCODE_MAX_WALL_SEC; + else process.env.WOOPCODE_MAX_WALL_SEC = ORIGINAL_WALL; + + if (ORIGINAL_ITERATIONS === undefined) + delete process.env.WOOPCODE_MAX_ITERATIONS; + else process.env.WOOPCODE_MAX_ITERATIONS = ORIGINAL_ITERATIONS; + + // Module state outlives a file. The loop clears the deadline on every exit, + // but a test that armed one without running a turn has to take it back + // itself — and the clock with it. + clearDeadline(); +}); + +/** + * A provider that never volunteers to stop. + * + * Every response is cut off after the model has spoken, which the loop salvages + * and continues from — so the turn runs forever until a budget ends it, without + * calling a tool. + * + * No tool, deliberately. `iterationBudget.test.ts` mocks the tool module for + * the whole run, so a file that reaches the registry to keep a turn going is a + * file whose result depends on which other file ran first. + */ +function neverFinishingProvider(onIteration?: () => void): ProviderClient { + return { + async *stream(): AsyncGenerator { + onIteration?.(); + yield { type: "text", content: "still working" }; + // Retryable, so the loop keeps what arrived and asks again rather than + // ending the turn on the failure. + throw new Error("socket hang up"); + }, + } as ProviderClient; +} + +/** Runs a turn to whatever ends it, keeping the error and the transcript. */ +async function runToEnd(provider: ProviderClient) { + const { callbacks, messages } = createRuntimeTest(); + let reported: Error | undefined; + callbacks.onError = (error: Error) => { + reported = error; + }; + + let thrown: Error | undefined; + try { + await agentLoop(provider, messages, "", callbacks); + } catch (error) { + thrown = error instanceof Error ? error : new Error(String(error)); + } + return { thrown, reported, messages }; +} + +/** The wind-down nudge pushed into the conversation. */ +const windDownNotices = (messages: Array<{ role: string; content?: string }>) => + messages.filter( + (message) => + message.role === "user" && + (message.content ?? "").includes("before this turn is stopped"), + ); + +describe("the wall-clock budget", () => { + test("a turn with no wall budget is unaffected", async () => { + delete process.env.WOOPCODE_MAX_WALL_SEC; + process.env.WOOPCODE_MAX_ITERATIONS = "3"; + + const { thrown } = await runToEnd(neverFinishingProvider()); + + // The iteration ceiling is still what ends it, and the message still names + // the knob that bound. + expect(thrown).toBeInstanceOf(IterationBudgetExhaustedError); + expect(thrown?.message).toContain("WOOPCODE_MAX_ITERATIONS"); + }); + + // A real budget on the real clock: the reserve is subtracted from it, so + // anything at or below the reserve is already spent when the turn starts. + test("a budget already spent stops the turn before it asks the provider", async () => { + process.env.WOOPCODE_MAX_WALL_SEC = String(WALL_RESERVE_SEC); + process.env.WOOPCODE_MAX_ITERATIONS = "40"; + + let requests = 0; + const { thrown, reported } = await runToEnd( + neverFinishingProvider(() => { + requests += 1; + }), + ); + + expect(thrown).toBeInstanceOf(WallBudgetExhaustedError); + // Reported through onError and then rethrown, the same path the iteration + // ceiling takes. + expect(reported).toBe(thrown!); + expect(requests).toBe(0); + }); + + test("the error names its own knob, not the iteration one", async () => { + process.env.WOOPCODE_MAX_WALL_SEC = "45"; + + const { thrown } = await runToEnd(neverFinishingProvider()); + + expect(thrown?.message).toContain("WOOPCODE_MAX_WALL_SEC"); + expect(thrown?.message).not.toContain("WOOPCODE_MAX_ITERATIONS"); + expect(thrown?.message).toContain("(45s"); + }); + + test("a malformed budget is ignored rather than read as a deadline", async () => { + process.env.WOOPCODE_MAX_WALL_SEC = "soon"; + process.env.WOOPCODE_MAX_ITERATIONS = "2"; + + const { thrown } = await runToEnd(neverFinishingProvider()); + + // Falling back to "no wall budget" and not to "out of time": a typo in a + // job config must not end every turn at its first step. + expect(thrown).toBeInstanceOf(IterationBudgetExhaustedError); + }); + + test("the deadline is never consulted through a checkpoint", async () => { + process.env.WOOPCODE_MAX_WALL_SEC = String(WALL_RESERVE_SEC); + process.env.WOOPCODE_MAX_ITERATIONS = "40"; + + const { callbacks, messages } = createRuntimeTest(); + callbacks.onError = () => {}; + let asked = 0; + callbacks.onBudgetExhausted = async () => { + asked += 1; + return "continue"; + }; + + let thrown: unknown; + try { + await agentLoop(neverFinishingProvider(), messages, "", callbacks); + } catch (error) { + thrown = error; + } + + // A handler exists, and the wall deadline still ends the turn without + // putting a question in front of a clock that keeps running. + expect(asked).toBe(0); + expect(thrown).toBeInstanceOf(WallBudgetExhaustedError); + }); + + test("the deadline is disarmed when the turn ends", async () => { + process.env.WOOPCODE_MAX_WALL_SEC = String(WALL_RESERVE_SEC); + + await runToEnd(neverFinishingProvider()); + + // Left armed, a spent deadline follows the session into the next turn: the + // wind-down would fire at its first step, and a clamped tool timeout would + // be one second. The loop clears it in the `finally` every exit runs + // through, so it is gone whichever way the turn ended — this one threw. + expect(remainingMs()).toBeUndefined(); + + // And a later turn that sets no budget of its own is unbudgeted again. + delete process.env.WOOPCODE_MAX_WALL_SEC; + process.env.WOOPCODE_MAX_ITERATIONS = "2"; + + const { thrown } = await runToEnd(neverFinishingProvider()); + expect(thrown).toBeInstanceOf(IterationBudgetExhaustedError); + expect(remainingMs()).toBeUndefined(); + }); +}); + +/** + * Both budgets end a turn the same way, and the exit code says so. + * + * 2 means "worked, did not finish, judge the result rather than treat this as a + * crash", which is what a deadline produces just as much as a ceiling. A + * distinct code would be booked as an exception by a harness not yet updated to + * know it, dropping exactly those trials from the mean. + */ +describe("the exit-code contract", () => { + test("both budgets answer to the type the exit code is read from", () => { + expect(new WallBudgetExhaustedError(600)).toBeInstanceOf( + BudgetExhaustedError, + ); + expect(new IterationBudgetExhaustedError(40)).toBeInstanceOf( + BudgetExhaustedError, + ); + expect(EXIT_BUDGET_EXHAUSTED).toBe(2); + }); + + test("an ordinary failure is not mistaken for a spent budget", () => { + expect(new Error("provider refused the request")).not.toBeInstanceOf( + BudgetExhaustedError, + ); + }); +}); + +/** + * The wind-down converts time into steps. + * + * Tested here rather than through a turn because the alternative is a real + * clock and a real budget: driving the conversion end to end would mean a test + * that waits minutes, and one that waits is one that fails on a loaded runner. + */ +describe("steps remaining", () => { + /** A turn `iterations` steps in, each having taken `stepMs`. */ + function turnAt(iterations: number, stepMs: number, budgetSeconds: number) { + let at = 0; + setDeadline(budgetSeconds, { now: () => at, startedAt: 0 }); + const state = new TurnState(); + at = iterations * stepMs; + state.iterations = iterations; + return state; + } + + test("the iteration ceiling answers while no time has been spent", () => { + setDeadline(3_600, { now: () => 0, startedAt: 0 }); + const state = new TurnState(); + + // Nothing has completed, so there is no rate to convert the clock with. + expect(stepsRemaining(state, 40)).toBe(40); + }); + + test("an unbudgeted turn is counted in iterations alone", () => { + const state = new TurnState(); + state.iterations = 35; + + expect(stepsRemaining(state, 40)).toBe(5); + }); + + test("the closer of the two budgets is what is reported", () => { + // 660s less the 60s reserve is 600s of usable budget. Ten steps at 20s + // each leaves 400s, which is twenty more steps — while the ceiling of 12 + // leaves only two. + const state = turnAt(10, 20_000, 660); + expect(stepsRemaining(state, 12)).toBe(2); + + // Same turn, a ceiling far away: now the clock is the binding one. + expect(stepsRemaining(state, 1_000)).toBe(20); + }); + + test("a slower turn has fewer steps left in the same time", () => { + // Twice the wall per step over the same elapsed time: 400s left at 40s a + // step is ten, where 20s a step was twenty. + const state = turnAt(5, 40_000, 660); + expect(stepsRemaining(state, 1_000)).toBe(10); + }); + + test("time already overspent reads as no steps left", () => { + const state = turnAt(10, 70_000, 660); + + // The loop throws before it gets here; the arithmetic must still not report + // room that does not exist. + expect(stepsRemaining(state, 1_000)).toBeLessThanOrEqual(0); + }); +}); + +describe("the wind-down warning", () => { + test("it is sent once for a turn, however many steps it takes", async () => { + process.env.WOOPCODE_MAX_ITERATIONS = "8"; + delete process.env.WOOPCODE_MAX_WALL_SEC; + + const { messages } = await runToEnd(neverFinishingProvider()); + + // A flag, not an equality on the step count: it fires at the first step + // inside the warning distance and stays quiet for the rest of them. + expect(windDownNotices(messages)).toHaveLength(1); + expect(windDownNotices(messages)[0]!.content).toContain("Only 5 more steps"); + }); + + // The reset that fires it again for a turn the user extended is covered in + // packages/tests/runtime/iterationBudget.test.ts, where the checkpoint that + // raises the ceiling is exercised — a salvaged response continues before + // reaching it, so this file's provider never gets there. +}); diff --git a/runtime/deadline.ts b/runtime/deadline.ts new file mode 100644 index 0000000..eede692 --- /dev/null +++ b/runtime/deadline.ts @@ -0,0 +1,138 @@ +/** + * The wall clock the current turn is running against. + * + * The loop has only ever measured its budget in iterations, and the harness + * that runs it enforces time — so a benchmark trial was killed by its own + * 200th iteration at 406s of an 1800s budget, mid-work, while Harbor's own + * timeout never fired. `docs/adr/0001-wall-clock-budget-for-the-agent-loop.md` + * has the per-task measurements. + * + * Module state rather than a parameter on `Tool.execute`, for the reason + * `runtime/sandbox/registry.ts` argues at length for the same shape: three + * tools will need it, and threading it through would change the `Tool` + * interface in `config/types.ts` and every tool's signature. + * + * `clampToBudget` has no caller yet. Until `run_terminal`, `run_tests` and + * `repl` clamp against it the deadline is advisory — the loop checks the clock + * between iterations, and a command started just inside the budget still runs + * to its own 300s default. That wiring is deliberately a separate change. + * + * Unbudgeted until a turn says otherwise, so nothing changes for a session that + * never sets `WOOPCODE_MAX_WALL_SEC`: every reader below answers "keep going" + * and every timeout is passed through untouched. + */ + +/** + * Wall seconds held back from the budget the operator gave. + * + * The harness kills the process at the number in the task's `task.toml`, so the + * loop has to stop before it — this covers the iteration in flight when the + * deadline is noticed, the final assistant message, and the session write that + * follows. A run that is hard-killed instead loses all three. + * + * It does **not** cover the ~90s provider spike CLAUDE.md records under + * benchmarking. Reserving for that would spend a sixth of `overfull-hbox`'s + * entire 750s budget on a case that fires rarely; one request that slow will + * overrun this reserve and the harness will kill the process, which is the + * behaviour that exists today for every run. + */ +export const WALL_RESERVE_SEC = 60; + +/** + * When this process began, captured at module load. + * + * The harness starts its clock at exec, so the loop counts from there too. + * Counting from the turn instead would let a second interactive turn restart a + * budget the harness is still spending down. + */ +const PROCESS_STARTED_AT = Date.now(); + +/** + * The clock every reader here uses, and the seam that keeps `Date` alone. + * + * A test that stubbed the global would be stubbing it for the entire run — + * `mock.module` cannot be undone, and this suite has been burned by that once + * already. Injecting the clock is what makes elapsed time testable without it. + */ +let clock: () => number = Date.now; + +/** Absolute time the turn must have stopped by, or unset when unbudgeted. */ +let deadlineAt: number | undefined; + +/** + * Arms the deadline for a budget expressed in whole wall seconds. + * + * The reserve is subtracted **here**, not by the caller: `agent.py` forwards + * Harbor's `timeout_sec` verbatim so a published number traces back to + * `task.toml` with no arithmetic in between, which leaves the safety margin as + * one constant in one repository. + * + * `now` and `startedAt` are for tests. Passing `now` installs the clock and + * leaves it installed, so a later call from the loop — which passes neither — + * keeps measuring against the same fake rather than silently reverting to the + * real one mid-turn. + */ +export function setDeadline( + budgetSeconds: number, + options: { now?: () => number; startedAt?: number } = {}, +): void { + if (options.now) clock = options.now; + + const from = options.startedAt ?? PROCESS_STARTED_AT; + // Floored at zero: a budget smaller than the reserve is already spent, which + // is the honest answer, rather than a deadline placed before the process ran. + const usable = Math.max(budgetSeconds - WALL_RESERVE_SEC, 0); + deadlineAt = from + usable * 1000; +} + +/** The current time on whichever clock is installed. */ +export function now(): number { + return clock(); +} + +/** Milliseconds left before the turn must stop, or undefined when unbudgeted. */ +export function remainingMs(): number | undefined { + return deadlineAt === undefined ? undefined : deadlineAt - clock(); +} + +/** + * Is the turn out of time? + * + * False when unbudgeted, so a session that never opted in reads "keep going" + * from every call site rather than ending on a deadline nobody set. + */ +export function deadlineReached(): boolean { + const left = remainingMs(); + return left !== undefined && left <= 0; +} + +/** + * The timeout a command may actually have, given what is left of the budget. + * + * Without this the deadline is advisory: the loop checks the clock between + * iterations, but a command started just inside the budget runs to its own + * timeout regardless — `run_terminal` defaults to 300s, which is 40% of + * `overfull-hbox`'s entire budget. + * + * Never less than one second. Zero or a negative timeout does not shorten a + * command, it kills it before it starts and reports a failure that says nothing + * about the clock having run out. + */ +export function clampToBudget(seconds: number): number { + const left = remainingMs(); + if (left === undefined) return seconds; + + return Math.max(1, Math.min(seconds, Math.floor(left / 1000))); +} + +/** + * Back to an unbudgeted session on the real clock. + * + * Restores the clock as well as clearing the deadline: this is module state, so + * a test that installed a fake and cleared only the deadline would leave every + * later test in the run measuring against a clock that never moves. + */ +export function clearDeadline(): void { + deadlineAt = undefined; + clock = Date.now; +} diff --git a/runtime/loop.ts b/runtime/loop.ts index 0409508..e09017d 100644 --- a/runtime/loop.ts +++ b/runtime/loop.ts @@ -4,6 +4,13 @@ import { takePendingImages } from "../tools/readImage"; import { blockedInPlanMode, planModeRefusal, planModeTools } from "./planMode"; import { isRetryableError } from "./retry"; import { compactToolHistory, toolHistoryBudget } from "./compaction"; +import { + WALL_RESERVE_SEC, + clearDeadline, + deadlineReached, + remainingMs, + setDeadline, +} from "./deadline"; import { TurnState, normalizeToolKey } from "./turnState"; import { recentMessages } from "../config/config"; import { SYSTEM_PROMPT } from "../config/systemPrompt"; @@ -175,7 +182,14 @@ const MAX_TURNS = 6; */ const SAME_TOOL_THRESHOLD = 2; -/** Iterations left when the model is told the budget is running out. */ +/** + * Steps left when the model is told the budget is running out. + * + * Steps rather than seconds, because the same constant has to serve both + * budgets and a duration is the wrong shape across this task set: 120s is 16% + * of `overfull-hbox`'s budget and 1% of `build-pov-ray`'s. Time is converted + * into steps instead, at the rate this turn has actually been running at. + */ const REMAINING_ITERATIONS_WARNING = 5; /** @@ -186,14 +200,19 @@ const REMAINING_ITERATIONS_WARNING = 5; const MAX_VERIFICATION_REMINDERS = 1; /** - * Raised when the loop runs out of iterations. + * Raised when the loop runs out of budget, of either kind. * * Distinct from a generic failure because it is not one: the agent ran, it * simply did not finish inside its budget. Callers that report an exit status * use this to separate "produced an incomplete result" from "something broke", - * which matters to any harness that treats the two differently. + * which matters to any harness that treats the two differently — and both + * budgets produce the same situation, so both answer to this one type. The + * subclasses exist so the message can name the knob that actually bound. */ -export class IterationBudgetExhaustedError extends Error { +export class BudgetExhaustedError extends Error {} + +/** Raised when the loop runs out of iterations. */ +export class IterationBudgetExhaustedError extends BudgetExhaustedError { constructor(limit: number) { super( `Agent exceeded the maximum number of iterations (${limit}).\n\n` + @@ -206,6 +225,27 @@ export class IterationBudgetExhaustedError extends Error { } } +/** + * Raised when the loop runs out of wall-clock time. + * + * Its own message rather than the iteration one, which tells the caller to + * raise `WOOPCODE_MAX_ITERATIONS` — advice that would send whoever reads it to + * the knob that did not bind, and the loop would stop at the same second again. + */ +export class WallBudgetExhaustedError extends BudgetExhaustedError { + constructor(limitSeconds: number) { + super( + `Agent ran out of wall-clock time (${limitSeconds}s, less a ${WALL_RESERVE_SEC}s reserve ` + + `for finishing up).\n\n` + + `This usually means:\n` + + ` • The task needs more time than the harness allows for it\n` + + ` • Work is partially done - judge what is on disk rather than treating this as a crash\n` + + ` • More time is needed - raise WOOPCODE_MAX_WALL_SEC`, + ); + this.name = "WallBudgetExhaustedError"; + } +} + /** * Resolves the loop budget, allowing `WOOPCODE_MAX_ITERATIONS` to set it. * @@ -228,6 +268,56 @@ function maxIterations(env: Record = process.env): n return parsed; } +/** + * Resolves the wall-clock budget from `WOOPCODE_MAX_WALL_SEC`, in seconds. + * + * Null when unset, and that is the ordinary case: an interactive session has a + * person deciding when a turn has gone on too long, and giving it a clock it + * never asked for would end turns that were going fine. Only a harness that + * enforces one of its own sets this, and it passes its whole budget — the + * reserve is subtracted in `setDeadline`. + * + * Mirrors `maxIterations` down to the warn-and-fall-back, so a typo in a job + * config is visible on stderr rather than being read as "no budget". + */ +function maxWallSeconds( + env: Record = process.env, +): number | null { + const raw = env.WOOPCODE_MAX_WALL_SEC?.trim(); + if (!raw) return null; + + const parsed = Number.parseInt(raw, 10); + if (!Number.isFinite(parsed) || parsed < 1) { + process.stderr.write( + `⚠️ ignoring WOOPCODE_MAX_WALL_SEC=${raw} (expected a positive integer)\n`, + ); + return null; + } + return parsed; +} + +/** + * Steps this turn has left, from whichever of its two budgets is closer. + * + * The wall budget is converted into steps at the rate the turn has been running + * at, so one warning and one flag serve both. Before the first iteration + * completes there is no rate to convert with, and the iteration count stands + * alone — which is the right answer anyway, since no time has been spent. + * + * Exported for its own test: the arithmetic is what decides when the model is + * told to wrap up, and driving it through a whole turn to observe it would take + * a real clock and a real budget. + */ +export function stepsRemaining(state: TurnState, budget: number): number { + const byIterations = budget - state.iterations; + + const mean = state.meanStepMs(); + const left = remainingMs(); + if (mean === undefined || left === undefined) return byIterations; + + return Math.min(byIterations, Math.floor(left / mean)); +} + /** Per-turn switches that are not part of the conversation. */ export interface AgentLoopOptions { /** @@ -632,12 +722,28 @@ export async function agentLoop( // Read once per turn so a mid-turn environment change cannot make two // iterations of the same turn assemble to different rules. const historyBudget = toolHistoryBudget(); + // The second budget, and the one an automated harness actually enforces. Read + // once per turn for the same reason, and armed before the first request so + // the clock covers the whole turn rather than starting after it. + const wallBudget = maxWallSeconds(); + if (wallBudget !== null) setDeadline(wallBudget); const state = new TurnState(); try { while (state.iterations < budget) { - state.iterations++; + // Checked before the iteration rather than after, so the turn stops with + // its reserve intact instead of starting a step it cannot finish. Inside + // the `try`, so it takes the same onError-then-rethrow path the iteration + // ceiling takes. + // + // `onBudgetExhausted` is deliberately not consulted. The ceiling can + // afford to ask because iterations do not tick while a human thinks; a + // clock does, and the only path with a handler is the interactive one, + // which does not set this budget in the first place. + if (wallBudget !== null && deadlineReached()) { + throw new WallBudgetExhaustedError(wallBudget); + } // Said to the model and to nobody else. A benchmark trial that exhausted // its 200 iterations was still writing at its 198th tool call, because @@ -648,18 +754,28 @@ export async function agentLoop( // ended the turn as a failure and a warning was the only notice they got. // Now the ceiling asks them directly, so a row saying the turn is nearly // over is a worse version of a question they are about to be asked. - if (state.iterations === budget - REMAINING_ITERATIONS_WARNING) { - const remaining = budget - state.iterations; + // + // A flag rather than an equality on the iteration count: two budgets can + // each come into view, and the equality it replaces silently never fired + // when the ceiling was below the warning distance. + const stepsLeft = stepsRemaining(state, budget); + if (!state.windDownWarned && stepsLeft <= REMAINING_ITERATIONS_WARNING) { + state.windDownWarned = true; messages.push({ role: "user", + // Floored at one: the count can round down to zero or below when the + // clock is what is binding, and "0 more steps" reads as a turn that + // is already over to a model that is about to get another one. content: - `Only ${remaining} more steps are available before this turn is stopped. ` + + `Only ${Math.max(stepsLeft, 1)} more steps are available before this turn is stopped. ` + `Finish what you have started rather than beginning anything new, ` + `make sure the work is in a usable state, and report what is done and ` + `what is not.`, }); } + state.iterations++; + // Measured from the same array that is sent, so the segment sizes and // the provider's token count describe one and the same request. // Compaction is opt-in; see runtime/compaction.ts for the benchmark that @@ -790,6 +906,10 @@ export async function agentLoop( } budget += BUDGET_STEP; + // A turn the user chose to extend has a new end, and deserves the same + // warning as it comes into view. Without this reset the second stretch + // would run to its ceiling silently. + state.windDownWarned = false; } } @@ -816,6 +936,17 @@ export async function agentLoop( // and session exit are what end those. closeReplSessions(); + // The deadline is module state, so a turn that ended has to disarm it or + // the next one reads a clock that stopped counting: its wind-down would + // fire at the first step, and once tool timeouts clamp against this, every + // command would be cut to a second. + // + // Unconditional, including for a turn that armed nothing — "the loop leaves + // no deadline behind" is the invariant worth having, and it also restores + // the real clock, so a test that injected one does not leak it into the + // rest of the run. + clearDeadline(); + // An image read on the last call before a cancellation is never attached, // because the path that attaches them returns before reaching it. Dropping // it here is what stops it arriving in the next turn, where it would be diff --git a/runtime/turnState.ts b/runtime/turnState.ts index fc05e63..b661f17 100644 --- a/runtime/turnState.ts +++ b/runtime/turnState.ts @@ -9,12 +9,32 @@ */ import { classifyInvocation, toolEffect } from "./toolEffects"; +import { now } from "./deadline"; import type { TurnSummary } from "../config/types"; export class TurnState { /** Provider responses so far. One iteration may carry several tool calls, or none. */ iterations = 0; + /** + * When the turn began, read from the deadline's clock rather than `Date`. + * + * The same clock the wall budget is measured on, so a test that injects one + * moves both — an elapsed time taken from `Date.now()` while the deadline ran + * on a fake would report a rate for a turn that never happened. + */ + readonly startedAt = now(); + + /** + * Whether the model has been told this turn is winding down. + * + * A flag rather than the equality test it replaces (`iterations === budget - + * REMAINING_ITERATIONS_WARNING`), because two budgets can each come into view + * and an equality on one of them silently never fired when the ceiling was + * below the warning distance. + */ + windDownWarned = false; + /** * Tools actually run. * @@ -108,6 +128,24 @@ export class TurnState { } } + /** + * Wall milliseconds one step of this turn costs, measured on this turn. + * + * Elapsed over iterations, deliberately **not** an average of the provider's + * `durationMs`: a step is the request plus every tool it went on to run, and + * the two differ by about half — `make-mips-interpreter` measured 1.77s of + * provider time against 2.03s of wall per iteration. + * + * Undefined before an iteration has completed, and while no time has passed, + * because neither can be divided into a rate. Callers read that as "no + * estimate yet" and fall back to the iteration count. + */ + meanStepMs(): number | undefined { + const elapsed = now() - this.startedAt; + if (this.iterations === 0 || elapsed <= 0) return undefined; + return elapsed / this.iterations; + } + /** * Did this turn change files and then run nothing to check them? *