Design rationale¶
Why this exists, the four constraints that ruled out every off-the-shelf option, and what was considered and turned down. The mechanics live elsewhere: ARCHITECTURE.md for the components, TRIGGERS.md and POLLER.md for how work arrives, BUDGET.md for the spending rails.
๐ฏ The problem¶
Pull request review is a serial bottleneck on maintainer availability. First-pass feedback on mechanical issues โ missing test coverage, error handling gaps, security-relevant changes, violations of project conventions โ is often delayed by days, which slows contributors and pushes review effort to the end of a release cycle.
The goal is routine review feedback within a minute of a pull request being opened or a maintainer asking for it, without any pull request leaving our infrastructure and without ever exhausting the shared Claude usage allowance.
๐ง The four constraints¶
These are what make the problem interesting; each one removes a category of solution.
| # | Constraint | What it rules out |
|---|---|---|
| 1 | No inbound network access. The host cannot receive GitHub webhooks. | Every webhook-driven integration. |
| 2 | Restricted eligibility. Only an explicitly configured set of contributors may be auto-reviewed; others are reviewed only when an allowlisted maintainer asks. | Anything whose trigger surface is "all pull requests". |
| 3 | Hard usage ceiling with no overspend. The backend is a Claude Max 5x subscription with a weekly threshold, and all Claude surfaces share one usage pool. | Anything unbounded. A runaway reviewer does not merely overspend โ it locks maintainers out of their own interactive Claude Code sessions until the window resets. |
| 4 | Opaque limits. Subscription plans expose no programmatic quota API. claude -p --output-format json reports what a run cost, but neither it nor any other agent CLI reports remaining allowance or a window reset time. |
Querying the budget. Per-run token counts are reported; the plan's remaining budget is not, so enforcement must be self-maintained. |
Constraint 1 is why the design polls rather than listens. Constraint 2 is why the allowlist is gated on the commenter for mentions, not the author. Constraints 3 and 4 together are why there is a budget governor at all, and why it keeps its own ledger.
๐ The one rule¶
Nothing may call a review engine outside the budget governor.
Two classes of mistake in this system are not recoverable by a later fix: the
agent spends a shared, metered allowance, and it posts under a real GitHub
account. A change that widens what triggers a review, or that removes a cap,
is therefore held to a higher bar than the rest of the code โ it must say so
explicitly and add a test pinning the new bound. See CLAUDE.md ยง5.
The structural consequence is the phase ordering: the budget governor lands before the review worker, so the spending rails exist before anything can spend.
๐งญ Alternatives considered¶
| Option | Why not |
|---|---|
claude-code-action on GitHub-hosted runners |
Least work by a wide margin, but checkout and execution happen on GitHub's infrastructure, and trigger eligibility and spend controls are limited to what the action exposes. Fails the private-execution requirement. |
| Self-hosted GitHub Actions runner | Genuinely viable under the no-inbound constraint โ the runner long-polls outbound โ and keeps GitHub's event filtering and audit trail for free. Rejected as the primary design because the budget governor, ledger and per-PR leasing need a persistent stateful service regardless. Retained as the documented fallback past roughly fifteen repositories, where polling economics stop favouring a daemon. Must never be attached to a public repository: fork pull requests can execute arbitrary workflow code on the host. |
| Public relay VM forwarding webhooks to an outbound AMQP consumer | Lowest latency, and the private host still needs no open port. Rejected for now: an internet-facing component and a second piece of infrastructure, for a latency gain of a few seconds that the ๐ acknowledgement already makes imperceptible. |
| PostgreSQL instead of SQLite | The topology is one writer on one host, so Postgres's multi-client concurrency is capability paid for and never used โ while SQLite's write serialisation actively simplifies the hardest invariant in the system (reserve-then-settle). SQLite needs no daemon, no port and no DBA on a locked-down host, and sqlite3 is in the standard library. Revisit only if the agent becomes multi-host. |
| RabbitMQ as the queue | Reasonable if already operational, but the budget reservation must be atomic with the dequeue โ trivial in one database, awkward across a broker plus a separate store. |
| Celery | The same rejection as RabbitMQ, plus a framework on top: Celery dequeues in a broker while the ledger lives in SQLite, so no transaction spans both. See Why not Celery. |
Engine code inside the reviewed repository at review-agents/ |
The reviewer's CI would run on every change to that repository unless paths were filtered, its Python dependency tree would join that repository's supply chain, and reuse for a second repository would need vendoring or a monorepo-subdirectory dependency. |
| API-key billing instead of the subscription | Not rejected โ see Billing mode below. A live option that requires no redesign. |
| Human review only (status quo) | No new infrastructure or cost, but leaves the bottleneck unaddressed. |
๐ฟ Why not Celery¶
Celery has genuinely similar semantics to what QUEUE.md describes
โ a durable task queue, bounded retries, a visibility timeout that behaves
like a lease, and beat for periodic work. It is the obvious thing to reach
for, so the reasons it does not fit are worth writing down.
The blocking one: the reservation cannot be atomic with the dequeue. The governor reserves inside the same transaction as the claim because three workers can each check the remaining allowance, each correctly conclude there is budget, and collectively breach the cap. Under Celery the dequeue happens in the broker and the ledger lives in SQLite, so there is no transaction spanning both: what is left is a distributed transaction, or a window between dequeue and reserve, which is the breach. Kombu does ship a SQLAlchemy transport that could point at SQLite, but it has long been flagged experimental rather than a supported production path โ and it would not help anyway, because the dequeue still happens inside kombu's own session rather than one the governor can join.
The rest is mismatch rather than impossibility:
| Need | Celery | Cost |
|---|---|---|
| Adaptive 10 s โ 600 s poll interval, snapping to the floor on activity | beat schedules are static; it needs a custom scheduler or self-rescheduling with countdown= |
fights the framework for what is fifteen lines in interval.py |
| Enqueue at most once per dedupe key | not native โ delivery is at-least-once | implemented in the database regardless |
| One worker per pull request | not native | a broker-side lock, or celery-singleton |
Owner-guarded complete/release, bounded attempts, an abandoned status |
partly approximated by acks_late and the visibility timeout |
the inspectable version is hand-rolled anyway |
| No inbound port, no extra daemon on a locked-down host | needs a broker process | a Redis or RabbitMQ service, a port and an ops surface |
An asyncio-native client and poller |
Celery 5 has no first-class async task execution | the poller runs outside Celery regardless |
The scale argument also runs backwards. Celery exists to fan work across many workers; this topology is deliberately one writer on one host, and the governor actively wants that serialisation. Horizontal scale here is a hazard rather than an unused feature.
Two things it would genuinely buy: Flower gives operational visibility a
while loop does not, and task priorities would be a clean way to let an
explicit @claude overtake an auto-review, which today is ORDER BY
enqueued_at. Neither is worth a broker while the cheap local equivalents โ
structured logging, and an ordering tweak โ remain available.
Revisit under the condition STORAGE.md already names: if the agent ever becomes multi-host. The move then is not Celery-on-SQLite but PostgreSQL plus a broker, with the reservation becoming an explicit row lock.
๐ Repository placement¶
The engine lives in its own repository, along a split that already exists in the problem: the engine is generic; the standards are per-repository.
- The engine has no knowledge of the reviewed repository, has its own release cadence and dependency tree, and holds the GitHub App private key and the agent credential. That is a real security boundary, not tidiness. Adding a second repository to its scope later is a config change rather than a fork.
- The standards stay in the reviewed repository, versioned alongside the
code they describe and changeable by a normal pull request:
AGENTS.mdfor conventions,.github/REVIEW.mdfor review guidance,review-standards/*.mdfor per-dimension checklists.
The GitHub App should be registered under the organisation, even though the engine source lives in a personal repository, so the reviewer's identity and installation stay independent of where the code is hosted. An App is not transferred cheaply; a repository is.
๐ Generalisation to other agents¶
The design generalises behind a single seam. Polling, allowlisting, dedupe,
leasing, staleness detection, publishing and the window arithmetic are all
agent-agnostic; only the "run a review" step is Claude-specific. A
ReviewEngine protocol with a declared Capabilities record
(structured_output, usage_reporting, read_only_sandbox, subagents,
prompt_caching) admits adapters for opencode, GitHub Copilot CLI, aider,
Codex CLI and Gemini CLI. The protocol, the record and the first adapter โ
claude โ are implemented; see ENGINE.md.
Every adapter is a command-line tool, invoked as a subprocess. No vendor SDK is linked. That is a decision, not an accident of what shipped first:
- A CLI is the interface every one of these agents actually offers. Codex CLI, opencode and Gemini CLI have no Python SDK worth targeting, so an SDK-shaped seam would be a seam only Claude could fit through โ the opposite of the point.
- A subprocess is a containment boundary a library call is not. The review step runs over an untrusted tree; a separate process with its own working directory, its own environment and a kill-on-timeout is a control the agent holds, whereas an in-process agent loop shares this process's memory, credentials and file handles.
- Dependency surface. An SDK pins a vendor's transitive tree into a daemon
whose other dependencies are
httpx,PyYAMLand the standard library. A CLI is a version string and anargv.
The cost is real and worth naming: a subprocess boundary means parsing whatever the CLI prints, so an output-format change breaks an adapter in a way a typed SDK response would not. Each adapter therefore pins the CLI version it was written against and fails loudly on an unparseable run rather than treating it as an empty review.
Two limitations are worth stating honestly:
- Agents without schema-constrained output need a fenced-JSON prompt contract with a budgeted validation retry.
- Agents that report no token usage force the governor onto proxy controls
only โ run-count, wall-clock and turn caps. That is a materially weaker
guarantee, which is why every ledger row records a
usage_confidenceofexact/estimated/unavailable.
Review standards should therefore be authored in the cross-agent AGENTS.md
plus review-standards/*.md, with the Claude adapter generating CLAUDE.md
from them. Recommended sequencing: define the interface early, ship the Claude
CLI adapter only, and add one second engine plus a shared conformance suite in
the final phase. The seam pays for itself regardless โ it is what makes the
review step testable without spending tokens.
๐ณ Billing mode¶
One clause narrowed this question considerably. The Agent SDK overview states that "unless previously approved, Anthropic does not allow third party developers to offer claude.ai login or rate limits for their products, including agents built on the Claude Agent SDK", and points such products at API keys.
That clause does not bear on this agent, because the agent links no SDK. Every engine adapter is a command-line tool run as a subprocess โ see Generalisation โ so the agent is a user of Claude Code, not a product built on the Agent SDK, and it offers nobody else a login or a rate limit. The credential never leaves the host and no third party authenticates through it.
What remains is the narrower and older question, which the SDK note does not
answer: whether a persistent daemon running claude -p over other
contributors' pull requests counts as ordinary individual use of a personal
Max subscription. Anthropic's guidance is explicit that running the CLI from
cron or CI for one's own work is fine, and equally explicit that 24/7 bots and
business use belong on API keys. This sits between the two.
The design therefore keeps the escape hatch open as a planned configuration
key, billing.mode: subscription | api_key, which does not exist yet. api_key mode uses the identical
governor with USD-denominated windows, removes the ambiguity entirely, gives
authoritative per-run cost, and eliminates the plan-lockout failure mode, at a
usage-based cost that is modest for this volume. Subscription mode is retained
for now, with the circuit breaker built as BUDGET.md specifies.
Still to confirm before deploying in subscription mode: the Commercial Terms and the consumer usage policy, read against unattended Claude Code use specifically. The citations and the conclusion belong in this section when that is done.
๐ก Prompt injection is in scope¶
Pull request diffs and comment bodies are untrusted input, and a pull request containing text instructing the reviewer to approve itself is a realistic vector. Three mitigations, none of which relies on the model behaving:
- The review prompt frames diffs and comments explicitly as data.
- The worker runs with a read-only tool set.
- The publisher takes no approval or merge action regardless of what a review
concludes, so a machine's judgement can neither block nor authorise a
merge. As built this is stronger than "always event
COMMENT": the publisher never calls the reviews endpoint at all, and the only writes it knows how to make are a reaction and an ordinary issue comment. The capability is absent rather than guarded โ see PUBLISHER.md.
Under a CLI adapter the first two stop being prompt wording and become argv
the tool itself enforces โ --tools naming a read-only set, --restricted
removing the command-running tools and ignoring the tree's own settings files,
--permission-prompts none so nothing can escalate by asking. They are pinned
by tests that assert the argv element by element; see ENGINE.md.
The same rule governs the code: untrusted input may never widen what the agent is allowed to do. Allowlisting in particular is on the numeric user id and never the login, because a login can be renamed and the freed name registered by a stranger.
Where review standards may come from¶
The engine is generic and the standards are per-repository, so the standards are read out of the repository under review โ at the merge base, never at the pull request head. A pull request that can rewrite the reviewer's instructions has talked its way past every other control here, and the head is writable by anyone who can open one.
What that trusts, stated plainly rather than implied: anyone who can merge to the base branch can change what the reviewer is told to do. That is a much smaller set than "anyone who can open a pull request", and it is not an empty one. It is the same set that already controls the repository's CI, so it is a trust boundary the project has accepted elsewhere.
The agent's own environment is kept out of reach by the same logic. The child process gets an allowlisted environment rather than the daemon's own: a process reading an attacker's tree and feeding a public comment is the wrong place for the agent's GitHub credential.
๐ Retention¶
Review content is retained until the pull request is merged, then purged:
findings, rationales and code excerpts are deleted and run.content_purged_at
is stamped. Ledger rows are never deleted, because the rolling budget
windows are computed from historical usage. The split is therefore purge
content, retain metrics. A max_age_days backstop covers pull requests that
stay open indefinitely.
Comments already posted on GitHub live in the pull request and are outside this policy. The operator runbook must say so, so it is not misread as a deletion guarantee.
โ Prerequisites still to confirm¶
- Outbound HTTPS from the deployment host to
api.github.com,github.com,codeload.github.comandapi.anthropic.com. Runpr-review-agent host checkon the host (DEVELOPER.md); it now probes the git fetch route as well as the API, becausegithub.comandapi.github.comare different hosts and, on an allowlist-based firewall, different rules. This should be confirmed before implementation continues; it is the most common cause of schedule slip in this kind of deployment. The poller alone needs onlyapi.github.com. - The
gitbinary, version 2.32 or later, on the host'sPATH. The checkout shells out to it, and 2.32 is whereGIT_CONFIG_GLOBALarrived โ below that it is ignored without an error, so the control that neutralises the host's own gitconfig would be absent while appearing to be in force. Bootstrap checks the version before it checks the route, for that reason. - The reviewer account the agent posts as. Nothing needs to be
configured for it. It used to:
github.agent_user_idnamed it so the classifier could skip its own comments, and that key is gone โ the loop is closed in the publisher instead, which neutralises the handle in every body it posts. See TRIGGERS.md. If that account is also a contributor, list it like any other. - The remaining allowlist members.
- A GitHub token. Read-only access to the three endpoints is enough for
polling, but the publisher exists now, so write scope is required:
Pull requests read/write, Issues read/write, Metadata read
(TOKENS.md).
pr-review-agent host checkreports on it but cannot settle it -- the repository response exposes onlypermissions.push, which is contents: write, a permission the list above deliberately omits. The checkout deliberately does not use the token: the fetch is anonymous, so no credential can reach the git command line or.git/config.