feat(runtime): make the wall budget bind, in the tools and through Harbor - #66
Merged
Conversation
The deadline was advisory. The loop checks the clock between iterations, but a command started just inside the budget ran to its own timeout regardless — `run_terminal` defaults to 300s, which is 40% of `overfull-hbox`'s entire 750s budget, so one straddling call meant the harness hard-killed the process and the wind-down never happened. `run_terminal`, `run_tests` and `repl` now read `clampToBudget` through `tools/timeoutBudget.ts`. `process_start` is excluded: a background process does not hold the loop, so it cannot overshoot. `repl` resolves its own default first, because a timeout it never named could not be clamped — it passed `undefined` and let `replSession` grant the full 120s. Two things the helper exists for. The clamp's floor of one second must not *raise* a timeout the caller deliberately made shorter, so the request stays the ceiling and "clamped" only ever means lowered. And a call the budget cut short must not get the standing advice to retry with a larger timeout: the number was never the constraint, and the retry spends the last of the turn reaching the same end. Unbudgeted sessions are byte-identical to before, asserted on the whole string.
The loop gained a wall-clock budget in the previous commits, but nothing supplied it: `make-mips-interpreter` was still stopped by its own 200th iteration at 406s of the 1800s Harbor allows, mid-work, with `exception_info: null` proving Harbor's own timeout never fired. `agent.py` takes an `agent_timeout_sec` kwarg -- the name Harbor's Cline agent already uses -- and forwards it verbatim as WOOPCODE_MAX_WALL_SEC. Verbatim because the reserve is subtracted in `runtime/deadline.ts`, which keeps the safety margin one constant in one repository and lets a published number trace back to the task's `task.toml`. Harbor does not hand the agent its timeout: `AgentContext` has no such field and `Trial` holds `timeout_sec`, so an operator has to supply it. `--ak` runs its value through `json.loads`, so an int, a float and a str all reach the constructor. All three are coerced to whole seconds, and anything unusable raises at construction rather than reaching the container -- the CLI ignores a budget it cannot parse and runs unbudgeted, warning onto a stderr stream buried in the trial log, which would leave an operator reading an unbudgeted trial as evidence about their budget. `job.yaml` raises `max_iterations` 200 -> 1000, so the ceiling reverts to guarding a pathological loop, and sets `agent_timeout_sec: 750`. One kwarg covers every task in a job and these five span 750s to 12000s; the minimum is the only value under which no task can be hard-killed mid-work. CLAUDE.md's benchmarking section claimed the opposite rule -- "wall clock is the binding budget, not iterations" -- read off `overfull-hbox`, which has the shortest timeout in the set by 2.4x. Replaced with the per-task table from docs/adr/0001-wall-clock-budget-for-the-agent-loop.md and the corrected rule: both budgets bind, and at `max_iterations: 200` iterations bound first on every task measured.
`stepsRemaining` converts the wall budget into steps at the rate the turn has been running at, and `meanStepMs` after one iteration *is* that iteration. CLAUDE.md records provider latency between 1,742ms and 90,002ms inside a single probe, so a 115s opening request against job.yaml's 690s of usable wall reads as five steps left -- `floor(575000 / 115000)` -- with roughly 280 affordable. The model was then told to wrap up, and because the flag latched it was never told otherwise. That is the failure this budget exists to prevent, reached from the other side. The rate is now ignored until three steps have gone into it, by which point the mean has recovered (the same case reads 18 at step four), and the flag re-arms if the estimate comes back above twice the warning threshold. Both transitions move onto `TurnState` beside the flag they own, which is also what makes them testable without a real clock and a real budget. Also documents why `state.iterations++` sits below the warning rather than above it: the clock's term counts the step about to start, so the iteration term has to as well. Against the equality it replaced the notice lands one step later, and "5 more steps" now includes the step about to run. Replay over the ten fixtures is byte-identical before and after, recorded in the ADR.
Every tool this file drives goes through `requestCommandApproval`, which reads the configured mode with `getApprovalMode()`. Stubbing `setPendingCommand` answers the prompt but does not stop the mode being read from disk, so the file was reading the developer's real ~/.config/woopcode -- the bug approval.integration.test.ts documents at length, which passes every sequential sweep and only fails when something else touches the tree at the same time. Redirects XDG_CONFIG_HOME to a temp directory before the imports that read it, and restores it in afterAll rather than afterEach, for the reason its sibling gives: on a machine that does not set the variable, an afterEach delete drops the redirect after the first test. Proven by watching the real providers.json mtime across a run of this file alone. The two elapsed-time bounds now sleep 30s rather than 5s so the clamped and unclamped outcomes are 1s and 30s apart. A 1s-against-5s window is inside the noise a loaded runner produces, which is what broke the cancellation test on macOS.
`json.loads` reads `1e400` and `Infinity` as `inf`, and `int(inf)` raises OverflowError -- which `_whole_seconds` did not catch, so those two were the one unusable input that escaped as a traceback rather than the ValueError every other bad value gets. A command line without `-c job.yaml` reads none of it, so both budgets fall back to agent.py's defaults: max_iterations 200 and no wall budget. The single-task command documented in CLAUDE.md and the adapter's README could not exercise either, and would have been capped at 200 steps -- the exact ceiling this work raised. Both now pass the two kwargs. job.yaml's comment justified max_iterations 1000 by make-mips-interpreter's full 1800s while the same file caps every task at 750s; it now says which number applies where, and says outright that a run under this config is not comparable to jobs/tb2-post-1.1, which had no wall budget at all. The ADR was written as a site page -- frontmatter, summary, `related` pointing at Configuration -- but was in no nav section, so it rendered nowhere while two files sent readers to it.
…left Two wrong numbers in a string the model acts on. `clampToBudget` floors at one second, so a command starting on an already-spent clock is granted 1s -- and the notice reported that as "only 1s of budget were left when it started", which was never true. It now reports what the budget *allowed*, which is true by construction. And `remainingMs() ?? 0` turned a cleared deadline into "about 0s remain now"; the clause is omitted instead, because saying nothing is quiet where a zero is confidently wrong. The three tools that take a timeout held identical copies of the clamp branch, and nothing would have stopped a fourth being written without it -- the failure TOOL_EFFECTS avoids by making a missing entry mean `unclassified`. `formatTimeoutError` picks between the wall notice and each tool's own standing advice, so a new caller gets the branch by using the helper. `BudgetedTimeout` carries `requested` alongside `seconds`, since every site needed both and two loose numbers of the same type is how they get passed the wrong way round. Adds the two cases #61 asked for that only run_terminal had: a timeout already inside the budget is left alone, for run_tests and for repl.
It read `state.iterations`, `state.meanStepMs()` and the deadline, and nothing of the loop's own but the ceiling passed to it. Its partner `shouldWarnWindDown` moved onto TurnState with the re-arm fix and this half stayed behind, which left the two-budget arithmetic split across two files with MIN_RATE_SAMPLES on the far side from the mean it guards. No behaviour change: same arithmetic, same guard, same callers.
The contract with harbor_woopcode/agent.py is that a spent budget exits 2 -- "worked, did not finish, judge the result" -- and the ADR says changing either side means changing both. The only coverage was `expect(EXIT_BUDGET_EXHAUSTED) .toBe(2)`, a constant compared with itself: nothing connected a WallBudgetExhaustedError to the code it produces, so the mapping could have been broken without a test noticing. Inline in a `process.exit` beside a live provider and a real session, it could not be reached. `headlessExitCode` is that expression, named and exported, and the tests walk it the way runHeadless does -- classify the error by `instanceof BudgetExhaustedError`, then map. Both budgets give 2, an ordinary failure gives 1, a clean turn gives 0. Still no subprocess: this proves the mapping and the classification, not that the process really leaves with that status.
Replaces headlessExitCode's two same-typed booleans with HeadlessOutcome, whose record() is the classification runHeadless actually runs -- the test asserted against its own copy of that line before, and the meaningless 'exhausted but not failed' state is now unconstructible rather than pinned by assertion. Centralises the stringly-typed timeout detection that had reached three tools as isTimeoutError, corrects turnState.ts's header where it still claimed the file decides nothing, unexports a constant with no importer, lifts the duplicated budgetWith fixture into packages/tests/shared, and reasons job.yaml's max_iterations from the 750s it actually sets.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The loop has only ever measured its budget in iterations, and Harbor enforces a
wall clock. Running the checked-in five-task config with
WOOPCODE_MAX_ITERATIONS=200, every trial finished having spent between 2% and23% of the time it was given, and
make-mips-interpreterwas killed by its own200th iteration at 406s of 1800 — mid-work, with
exception_info: nullprovingHarbor's own timeout never fired.
#64 landed the mechanism —
WOOPCODE_MAX_WALL_SEC, the deadline, theBudgetExhaustedErrorhierarchy, the 60s reserve. It did not make it bind.clampToBudgethad no caller, so the deadline was advisory: the loop checkedthe clock between iterations, but a command starting just inside the budget ran
to its own 300s default regardless — 40% of
overfull-hbox's entire budget.And nothing set the variable, so on a benchmark run the budget was never armed
at all. This is the other half.
CLAUDE.md's benchmarking section records the opposite rule — "Wall clock is the
binding budget, not iterations" — which was measured on
overfull-hbox, theshortest timeout in the set by 2.4×. It does not generalise to the other four,
and correcting it is part of this change.
What
Tool timeouts clamp against the remaining budget.
tools/timeoutBudget.tsis the half of the deadline the tools see:
budgetedTimeoutreturns the numberto hand the executor alongside what was asked for, and whether the budget rather
than the caller chose it.
run_terminal,run_testsandrepluse it.process_startdeliberately does not — a background process does not hold theloop, so it cannot overshoot.
The clamp is read after approval, not before. The clock runs while a human
decides, and a number taken at the top of
executewould grant a command timethat was spent waiting to be allowed to run.
A clamped kill gets its own message. The standing advice for a timeout is to
retry with a larger one, which is exactly wrong here — the number was never the
constraint, and a retry spends what remains reaching the same end. The notice
reports what the command was granted rather than what was "left when it
started"; those come apart, because
clampToBudgetfloors at one second, so acommand starting on an overspent clock is granted 1s when nothing was left.
Harbor forwards its own timeout.
agent.pytakesagent_timeout_secandsets
WOOPCODE_MAX_WALL_SECfrom it, rejecting a value it cannot parse ratherthan running unbudgeted — an infinite timeout is refused for the same reason.
The reserve is subtracted inside
runtime/deadline.ts, not by the caller, soagent.pyforwards Harbor's number verbatim and a published result traces backto
task.tomlwith no arithmetic in between.job.yaml'smax_iterationsgoes 200 → 1000 so it stops binding first. Bothbudgets stand; neither replaces the other. With the wall bounding spend, the
iteration ceiling reverts to the role
loop.tsalready claims for it — a guardagainst a pathological loop with nobody watching.
Rejected: raising
max_iterationsalone. Zero code and immediatelytestable, but the loop still cannot see a clock, so on a 750s budget a slow run
is hard-killed mid-work instead of winding down. Also rejected: per-task
iteration budgets derived from each timeout ÷ measured rate — honest to the
data, but hand-tuning benchmark config per task overfits.
Wind-down is per-turn state.
stepsRemainingmoved off the module and ontoTurnState, which also owns the warned-once flag; one slow step could previouslywind a turn down early.
HeadlessOutcomeincommands/agent.tsxreplaces twobooleans whose fourth combination — exhausted but not failed — had no meaning,
and makes the exit-code contract reachable from a test. That contract has a
second party:
agent.pymaps exit 2 to success.Closes #61. Closes #62.
Verified
bun run verify --allon the merged tree, afterbun install:mainwas merged into this branch before that run. The merge was clean, whichis not by itself evidence — #65 changed plan mode's second gate while this
branch changed the timeout path through
tools/terminal.ts, and git found notextual conflict between them. Both sides were confirmed present afterwards by
content: the three tools import from
./timeoutBudget,job.yamlreadsmax_iterations: 1000,agent.pysetsWOOPCODE_MAX_WALL_SEC, and #65'sinline-script matching is still in
runtime/toolEffects.ts.harbor_woopcode/test_agent.py— 44 passed. Run by hand: neitherverify.tsnor
.github/workflows/ci.ymlmentions pytest, so these tests are ungated andcan rot silently. Worth wiring up separately.
Not verified: no live Harbor run. Nothing here has been exercised against a
real task — no
make-mips-interpreter, nooverfull-hbox. The measurementsabove are from
jobs/tb2-post-1.1, which predates this change and is whatmotivated it. The claim this makes is that the budget is now armed and binds;
whether it improves a score is unmeasured.