From ecf6a77a04b6aae96c2ae817362f68424555db52 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Sun, 30 Aug 2026 08:51:13 +0530 Subject: [PATCH] feat(runtime): stop a turn on the clock as well as the step count The loop has only ever measured its budget in iterations while the harness that runs it enforces a wall clock, so `make-mips-interpreter` was killed by its own 200th iteration at 406s of an 1800s budget, mid-work, with `exception_info: null` proving Harbor's timeout never fired. Across the five-task config every trial spent 2-23% of the time it was given. `WOOPCODE_MAX_WALL_SEC` gives the loop a second budget and it stops on whichever binds first. The operator passes the whole budget; `setDeadline` subtracts a 60s reserve so the last step, the final answer and the session write still land. The deadline lives in module state for the reason `runtime/sandbox/registry.ts` gives for the same shape. `onBudgetExhausted` is deliberately not consulted for the deadline: the step ceiling can afford to ask because iterations do not tick while a human thinks, and a clock does. `WallBudgetExhaustedError` is a sibling of `IterationBudgetExhaustedError` under a shared `BudgetExhaustedError`, so both exit 2 - one code, because a distinct one would be booked as an exception by any harness not yet updated to know it. The wind-down now converts remaining time into steps at the turn's own measured rate, so one message and one flag serve both budgets. That flag replaces an equality test that silently never fired when the ceiling was below five, which is a behaviour change: a two-step budget is now announced at its first step. Prompt assembly is untouched - `bun run replay:baseline` is byte-identical before and after, across all ten fixtures and the corpus totals. Reasoning and the per-task measurements: docs/adr/0001-wall-clock-budget-for-the-agent-loop.md. Deferred by design: clamping tool timeouts to the remaining budget (#61) and wiring the budget through `agent.py`, `job.yaml` and CLAUDE.md (#62). Until #61 lands the deadline is advisory for a command already running. Closes #60 --- commands/agent.tsx | 13 +- config/types.ts | 5 + ...01-wall-clock-budget-for-the-agent-loop.md | 108 +++++++ docs/reference/configuration.md | 1 + packages/tests/runtime/deadline.test.ts | 122 ++++++++ .../tests/runtime/iterationBudget.test.ts | 13 +- packages/tests/runtime/wallBudget.test.ts | 288 ++++++++++++++++++ runtime/deadline.ts | 138 +++++++++ runtime/loop.ts | 147 ++++++++- runtime/turnState.ts | 38 +++ 10 files changed, 855 insertions(+), 18 deletions(-) create mode 100644 docs/adr/0001-wall-clock-budget-for-the-agent-loop.md create mode 100644 packages/tests/runtime/deadline.test.ts create mode 100644 packages/tests/runtime/wallBudget.test.ts create mode 100644 runtime/deadline.ts 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? *