The review worker

What drains the queue. Implemented in src/pr_review_agent/worker.py, run and kept alive by the daemon.

The poll loop stops at enqueue. This is the other half: it claims a trigger through the budget governor, resolves the pull request, puts it on disk, hands it to a review engine, records what it cost, and closes the row.

It does not decide what a comment says. Rendering and posting belong to the publisher, which the worker calls twice: once for the 👀 the moment the lease is confirmed, and once with the recorded run.

It does spend. The engine it runs is the configured claude CLI adapter, so a claim here is real money. Everything between the claim and the subprocess is what bounds that: the governor's windows and ladder at admission, the size gate and path exclusions at checkout, the pre-flight estimate immediately before the engine, and worker.count over the whole lot. FakeEngine is a test double and is never wired into a running daemon.

🔁 One run

claim(admit=governor.admit)        the lease and the reservation, one commit
  → publisher.acknowledge(...)     the 👀, before anything slow is attempted
  → GET /pulls/{n}                 head_sha for a mention, and the size counts
  → facts.is_open?                 merged or closed: abandon before the checkout
  → runs.history(repo, pr)         what the last round found (a describe: none)
  → workspace.checkout(...)        the tree, the merge-base diff, exclusions
  → governor.preflight(...)        the last free refusal -- releases its own hold
  → publisher.notify(...)          on a refusal only: says so on the pull request
  → engine.review(request)         the only agent-specific step, and the spend
  → numbering.assign(...)          stable numbers, before anything is stored
  → runs.record(...)               the findings, so a failed publish is retryable
  → governor.settle(claim, usage)  release whatever was not spent
  → governor.headroom(now)         what the review left, logged as event 6
  → publisher.publish(run)         one new comment per review, nothing edited
  → complete / release / abandon   close the row
  → queue.fold(claim, ...)         close the triggers this review answered

A row whose kind is publish skips all of it: the worker looks the named run up, posts it, and closes the item. That path reaches no engine and writes no ledger row — see PUBLISHER.md.

A failed post is handed back unattempted, so it does not spend the max_attempts bound that measures allowance drained. It is counted against publish.max_publish_attempts instead: at that many failures the run is stamped publish_failed_at, the item is closed rather than handed back, and an ERROR names the run an operator has to look at. Without that bound a post GitHub will never accept was repeated on every claim forever.

The fold

A review that recorded findings closes every other trigger for that pull request that was already waiting when it started, named no other commit and asked for the same thing — review or describe: three mentions on one pull request asked one question, and the agent posts one comment. Only a run that recorded something folds anything — a run that recorded nothing has no answer to give them. The rule, and why a trigger enqueued during the review is left alone, is in QUEUE.md.

The history read and the numbering

Two steps were added so a re-review reads as a continuation rather than a fresh opinion. Neither reaches an engine and neither spends anything.

runs.history is one query on a table this worker already writes. It answers what the last completed round found — which becomes ReviewRequest.prior, stripped of bodies — and the largest finding number this pull request has ever issued.

numbering.assign runs after the engine and before runs.record, and the order is the point. The high-water mark is read back out of the stored findings, so a run recorded without numbers would let a retired number come back on something else in a later round. Numbering at render time would be too late. See PUBLISHER.md for what the numbers mean to a reader.

The closed pull request is refused before the checkout

facts.is_open is read the moment GET /pulls/{n} returns and before workspace.checkout, and a pull request that is merged or closed ends the row there: settle at zero with stop_reason = closed, abandon, return.

The gap it closes is a timing one. A trigger is enqueued while the pull request is open; it waits its turn behind other work, or behind the pacer; and by the time a worker claims it the pull request has been merged. Nothing between the enqueue and the claim asks GitHub anything, so without this check the agent clones the tree, runs the engine and posts a review on a pull request nobody will read — at full price. The state is already in the response the worker fetches for head_sha, so reading it costs no extra request.

It settles at zero, alongside a pre-flight refusal and EngineUnavailable: no engine ran, which is provable rather than assumed. It is a distinct stop reason rather than refused because an operator running GROUP BY stop_reason is asking two different questions — refused is a limit that may want raising, closed is a queue draining more slowly than the repository moves. And it abandons rather than releases, for the same reason the pre-flight refusal does: the answer is deterministic, and another attempt would reserve allowance only to read the same state back.

A reopened pull request does not get its trigger back. The row is abandoned and its dedupe key blocks a second enqueue; every trigger in this design is gated on a timestamp that has already passed — pr_opened on created_at, a mention on the comment's updated_at — so reopening produces nothing new to classify. A fresh @claude comment after the reopen does work, and it is the only thing that does, which is why the INFO line for a closed (as opposed to merged) pull request says so rather than leaving an operator to find out. A merged pull request gets no such advice: GitHub will not reopen it.

The check is deliberately not repeated in the publisher. By then the tokens are spent, and a check that runs after the money has gone protects nothing that matters.

The pre-flight estimate

governor.preflight(claim, checkout.reviewed.lines, now) is the last point at which a run can be refused for free: the tree is on disk but no engine has started, so nothing has been spent. It refuses a pull request predicted to cost more than max_run_tokens, and one with nothing left to review once budget.excluded_paths has been applied.

It releases the reservation inside that call, so the worker must not settle afterwards — a second settle on a settled row is the one mistake this arrangement is designed to make impossible. The row is then abandoned: both refusals are deterministic for this head, and another attempt would reserve allowance only to reach the same answer.

And then it says so on the pull request. It returns the sentence to post rather than a bool, which the worker hands to Publisher.notify. Until 1.4.0 a refusal was silent: the 👀 went on minutes earlier, the row was abandoned, and the reason existed only in the operator's journal — so a maintainer who typed the handle on a 6 000-line pull request had no way to learn that raising a cap is the fix (issue #78). The same is done for the size gate, which refuses earlier still, through PullRequestTooLarge.notice.

No other deterministic ending is announced. A transient failure is retried and the review it eventually produces speaks for itself — except an engine failure on the last attempt max_attempts allows, which posts the failure notice; a closed pull request is told nothing, because nobody is reading it; and a PayloadError is a bug in the agent rather than something a contributor can act on.

checkout.reviewed rather than facts is what it is asked about. Those are different numbers on purpose: the API's counts cover every changed path, while reviewed is what survived the exclusions, which is both what the gate measures and what the engine is shown.

The same number reaches settle as reviewed_lines, which is what the rate is fitted against — so the threshold the next run is measured by comes from what the runs before it actually cost. Only a completed review reports it — a description never does, and an incremental round is set aside by reviewed_since; see BUDGET.md for why the others say nothing.

settle runs before the queue verb, and both are guarded on the owner. A worker whose lease lapsed mid-run gets False from settle and stops there — which is how it learns to discard a result it is no longer entitled to act on.

The two loops, on one timeline

The interleaving is the part that is hard to see from the code, because await is what makes it happen and nothing names it. Both loops run in one process on one event loop.

Time	Poll loop	Worker loop
t+0 s	GET /pulls → 200, PR #42 is new. The classifier accepts it, enqueue inserts a pending row, the watermark advances.	idle
t+2 s	sleeping	claim() opens one transaction: governor.admit measures the windows, writes a reservation for max_run_tokens, and the lease is taken in the same commit.
t+3 s	polls again, enqueues an @claude on PR #7	GET /pulls/42 → PullRequestFacts
t+5 s	waiting	fetch, merge-base, diff, worktree add
t+5 s … t+4 min	keeps polling	await engine.review(...) — minutes, for a real adapter
t+4 min		teardown, settle at what the engine reported, complete
t+4 min		claims PR #7's mention

The mention on #7 waited four minutes. That is what a concurrency of one means, and it is what worker.count exists to change.

💸 What a failed run settles at

settle runs on every path, including a failure. A reservation that is never settled stays charged at its full ceiling until it ages out of its rolling window — the deliberate behaviour for a lost worker, whose process died. A caught exception is not a lost worker.

But "settle at what was actually spent" is unanswerable for a crashed engine: a CLI adapter killed on a timeout has spent real tokens and reports none. So the rule splits on the one thing that is knowable — whether the engine had started.

Where it failed	Settles at	Why
before engine.review	0, unavailable	Nothing reached an engine. Provable, not assumed.
merged or closed, before the checkout	0, exact	Refused before anything was cloned, so the zero is measured rather than inferred.
EngineUnavailable	0, unavailable	The subprocess never started, so no process existed to spend. Provable, like the row above it.
in or after engine.review	the full reservation	Anything may have been spent, and the governor cannot find out.
in engine.review, output still readable	the envelope's figure, exact	The tool failed but still printed what it measured, so the spend is known; the reservation would overstate it.
UsageLimited	0 or the envelope's figure, exact	The one failure where the spend is knowable.
nowhere — it succeeded	result.usage	What the engine reported.

In the code this is one variable taking three values, and the assignment that raises it to the full reservation sits on the line before the engine call. "Did the engine start" is expressed by control flow rather than by a flag that could disagree with reality.

EngineUnavailable is on the wrong side of that line, and is put back. It is raised where the engine call starts — create_subprocess_exec refusing a missing binary or an absent cwd — so control flow places it after the ceiling assignment while the fact it reports is the same one the rows above it report: nothing ran. It settles at zero and reads as engine_unavailable, apart from engine_error because the operator action differs. It is still handed back with release and still counts its attempt: a usage limit clears when a window rolls, but a missing binary clears only when somebody fixes the host, and an uncounted retry would hold every trigger in the queue forever.

Pessimism is the safe direction for a spending control. It is the same argument BUDGET.md already makes for refusing a crashed worker an amnesty.

UsageLimited is the exception that proves the rule. The ceiling is charged because a killed run's spend is unknowable; a usage limit is one of the few failures where it is not. Refused up front, the CLI did no work and the spend is zero; hit mid-run, the CLI still printed an envelope and that envelope measured it. So the exception carries the figure when it has one and the worker settles at it.

Charging the reservation instead would be worse than merely pessimistic. It would write tokens that were never spent into all three rolling windows, and because the windows are rolling and ledger rows are never deleted, those phantom tokens would keep refusing real runs for up to a week after the account had recovered — the ledger quietly taking over the breaker's job, with a far longer time constant and nothing in the logs to say so.

🎫 What happens to the row

A failed run has two possible fates and they are not interchangeable.

Failure	Fate	Why
Outcome.TRUNCATED	release	The run was cut off with work outstanding; another attempt may finish it
Outcome.FAILED	abandon	Anything else that went wrong inside a run that still answered
pre-flight refusal	abandon	Deterministic for this head, and already settled at zero
merged or closed pull request	abandon	Settled at zero before the checkout; a reopen re-triggers nothing, a fresh @claude does
GitHubClientError	release	5xx, rate limit, network — transient by construction
WorkspaceError, GitCommandError	release	A failed fetch is a failed network call
the engine raises anything	release	May succeed next time; bounded by max_attempts
UsageLimited	release, and the breaker trips	The wall is the account's; the row waits behind the breaker rather than being retried into it
PullRequestTooLarge	abandon	Deterministic on this head
PayloadError	abandon	Deterministic

release returns the row to pending with its attempt already counted, so three failures reach abandoned. abandon gives up at once.

The first two rows are not exceptions: a run can end badly and still return a ReviewResult, because it spent tokens and the ledger has to hear about that. Outcome decides what becomes of the row; it never decides whether the run settles. Only COMPLETED counts as progress for the supervisor's backoff.

Why not complete for a permanent failure? Because done means reviewed. A pull request refused for its size was never reviewed, and an operator reading the table should not have to guess which kind of done they are looking at. That is the whole reason ReviewQueue.abandon exists as a verb rather than being reachable only by exhausting max_attempts.

Why not retry a permanent failure anyway? Two more attempts reach the same refusal, and each one reserves allowance to get there.

PullRequestTooLarge is caught first

It subclasses WorkspaceError, so the order of the except clauses is what makes it abandon rather than retry. A test pins that.

Any engine failure is an engine failure

Every adapter is a foreign command-line tool. The worker has no catalogue of what one can raise and no way to tell a timeout from a parse error from a bug inside it — so the engine call is wrapped, and anything it raises becomes EngineError and is retried.

UsageLimited is the single exception, and it is re-raised rather than wrapped. It has to be: retrying it is the specific failure the circuit breaker exists to prevent, because the wall belongs to the account and a second attempt reaches it again having spent to get there.

Narrowing that boundary to the one call is the point: a bug in the worker is not caught by it, and propagates to the supervisor instead of being quietly retried three times.

🧹 What a run leaves behind

The tree under review is untrusted input, so "one pull request cannot affect the next" is a security property, not tidiness.

State	Lifetime	Reaches the next run?
worktree at runs/<uuid>	removed in finally	No — and sweep() clears the whole directory at startup
run ref refs/run/<uuid>	deleted in finally	No
the bare mirror	persistent, shared	Yes — the one shared artefact
the ReviewWorker instance	process lifetime	No field holds run data
the engine	a subprocess per run: own cwd, scrubbed environment, kill-on-timeout	No

So the entire carry-over surface is the mirror, and what a hostile pull request can put there is already bounded by WORKSPACE.md: objects and one namespaced ref, fetched over an https-only whitelist, never executed, with the diff computed in the bare repository so in-tree .gitattributes cannot hide the change. A large pull request can grow the disk. It cannot reach the next review's tree.

The worker holding no state is the last link in that chain, which is why it is a rule rather than an implementation detail. The one field that does persist, completed, counts finished runs and carries nothing from them.

🩺 The supervisor

daemon.supervise awaits worker.run_forever and, on an unexpected exception, logs the traceback and re-enters the loop. Because a worker holds no state between runs, re-entering the loop is a fresh worker.

The delay doubles from RESPAWN_BACKOFF (5 s) to RESPAWN_BACKOFF_MAX (300 s), returning to the floor whenever the worker finished a review in between: progress means the fault was not persistent, so the accumulated delay is not earned. Both are module constants, not configuration — a backoff describes a failure mode rather than an operator's preference.

Why not let the process die, as the poll loop does for an unexpected error? Because that takes the poller down for a fault confined to the worker, and a daemon that polls but never drains is the state this whole change exists to end.

A poison pull request cannot drive the respawn loop. An uncaught crash never releases the row, so it stays claimed under a live 30-minute lease: the restarted worker takes other work, and when the lease lapses the row is retried with its attempt counted, reaching abandoned after three. The backoff is for the crash that is not about any row — a full disk, a bug in the claim path — where the worker dies holding nothing.

⏱ Waiting

When claim() returns nothing, the worker waits WORKER_IDLE (30 s) on the same stop event the poll loop uses, so a SIGTERM is never held for the remainder of an idle wait.

Thirty seconds rather than five is about noise, not load — a claim is a local SQLite read. When the governor is refusing (the 85 % rung, or a contributor over their share) the claim returns None and the row stays pending, indistinguishable here from an empty queue, and the governor logs a warning every time. It will keep refusing until the window rolls, which can be hours; at five seconds that is some 720 identical lines an hour in the operator's journal.

🔢 worker.count

worker:
  count: 1

A spending control, not a throughput knob. Every concurrent run reserves budget.max_run_tokens up front, so count multiplies the floor below which the governor refuses everything. It is capped at 4, and both the default and the cap are pinned by tests — CLAUDE.md §5.

One pull request is never reviewed by two workers whatever this is: the per-pull-request lease holds that. Raising it parallelises across pull requests only.

Each worker gets a distinct owner id, and that is load-bearing rather than cosmetic: complete, release, abandon and settle are all guarded on the owner, so two workers sharing one could finish each other's rows.

🚫 Why not a process per review

The idea is reasonable — process spawn is negligible against a multi-minute review — and it was rejected for three reasons that are not about overhead.

1. It does not remove the loop. Something must still notice a claimable row, and an enqueue event is not enough: a released row and a budget-refused row produce no enqueue. A supervisor would poll the queue on an interval, so the pacing question survives unchanged.
2. It breaks the workspace's stated invariant. One asyncio.Lock serialises every write to the mirror's ref namespace, and that is sufficient because the daemon is a single process. Two processes fetching into one mirror contend on packed-refs.lock, and sweep()'s reasoning that at startup "no git of ours is running" stops being true. The SQLite side would be fine — WAL, busy_timeout, BEGIN IMMEDIATE — but the git side is a redesign.
3. It makes settle-on-failure worse. A dead child leaves an exit code and no usage, so the row stays claimed with an unsettled full reservation until the lease expires: exactly the lost-worker path a caught failure exists to avoid.

Containment does not pay for it either — the engine adapter is already a subprocess with its own working directory, a scrubbed environment and a kill-on-timeout. A Python process around it duplicates a boundary that exists.

🚧 What lands next

The publisher landed, and with it the head_sha re-check: a review of a commit that has since been superseded is discarded there rather than posted. The circuit breaker landed too.

What still needs a running engine is layer 3's per-run token ceiling and the ladder's 60 % rung — both want an engine that reports tokens as it spends them, not only at the end. After that, the retention sweep: runs.record is what accumulates review bodies, and nothing purges them yet.