Skip to content

review-box.md

Latest commit

 

History

History
996 lines (826 loc) · 51.9 KB

review-box.md

File metadata and controls

996 lines (826 loc) · 51.9 KB
Raw

The review runner

A machine that runs reviews unattended: the supervisor (review-supervisor.py) and the cron jobs around it, reviewing every labelled PR around the clock and publishing the results. How the supervisor works is in supervisor-design.md; this is how to set a box up and keep it running.

The reference deployment is a dedicated box: 32 cores, 30G RAM, a few hundred GB free, Ubuntu under WSL2. Cores and RAM matter because a run starts up to eight passes per model at once, several of which build and fly SITL.

Layout

Run data and scripts live under ~/review, which is $REVIEW_ROOT:

path what
~/review/bin/ the scripts from runner/bin/ in this repo
~/review/etc/ local.conf, routing.json, the crontab
~/review/data/ the store and all scratch. $REVIEW_DATA
~/review/data/runs/ one directory per run, its attempts (worktree, evidence, result) inside
~/review/data/results/, outbox/, receipts/, membership/ accepted reviews, owed deliveries, delivered ones, page rows
~/review/data/references/ the reference clones passes check out from, one per repository
~/review/data/locks the one lock file every run, pass and drain shares. Never replace it
~/review/repositories/ base clones of every reviewed repo, refreshed nightly. $REVIEW_REPOS
~/review/cache/ caches passes share: virtualenvs, uv, pip, npm
~/review/work/ the directory a run starts in, so no base checkout is its working directory; a run refuses to start without it
~/review/logs/ run logs, 30-day retention. $REVIEW_LOGS

$REVIEW_AUTH is ~/review.auth/ - one directory per account and a symlink per role, never in git. It is a sibling of ~/review/ rather than a directory inside it, because the reviewing agent is started with --add-dir $REVIEW_ROOT. A box set up before that changed moves across with runner/bin/migrate-auth-root.sh (--dry-run first), which renames the directory and adds the new deny rule without removing existing protections. Pause runs (pause-runs.sh) across the migration and deployment. The migrator has its own $HOME/review.auth default and does not source the deployed environment; for a custom location pass --auth-root /absolute/path explicitly. It validates every account before moving, refuses links whose meaning would change, and resumes unfinished settings updates if the directory has already moved. A completed migration can be run again safely. Inspect a refusal before releasing the lock.

Historical Codex quota deltas recorded under the old home paths stay blank after the move. Run logs are preserved: a vanished home does not prove ownership of another directory's usage history.

review-env.sh points TMPDIR into $REVIEW_DATA/tmp. On a box where /tmp is a tmpfs, a bare mktemp -d there fills RAM and takes the machine down with it — this has happened, so the redirection is not optional.

Install

git clone https://github.com/ArduPilot/APReview.git ~/APReview
mkdir -p ~/review/etc

# bin is a symlink into the checkout, not a copy: `git pull` is then the whole
# deploy step, and the scripts find repos.json beside themselves. A copied bin
# has no checkout to resolve back to, and clone-repos.sh will say so and stop.
ln -s ~/APReview/runner/bin ~/review/bin

cp ~/APReview/runner/etc/crontab.reviewprs ~/review/etc/
cp ~/APReview/runner/etc/local.conf.example ~/review/etc/local.conf   # then edit
echo '{"schema": 1, "repositories": [], "labels": [], "modes": ["all"]}' \
    > ~/review/etc/routing.json          # everything to the supervisor

~/review/bin/clone-ardupilot.sh      # base clone with submodules
~/review/bin/clone-repos.sh          # everything in repos.json, submodules included
~/review/bin/base-build.sh           # proves the toolchain, warms ccache

loginctl enable-linger "$USER"       # passes run as user units; see below

Then seed a reference clone under ~/review/data/references/ for every repository and every ArduPilot-owned submodule, as described in Supervisor operations: a run refuses to start while one is missing. Set REVIEW_GITHUB_WRITES=1 and GH_TOKEN in local.conf once you want it to post. For the accounts, see Accounts and copy runner/etc/policy.json.example to ~/review.auth/policy.json.

/reviewprs, the single-session slash command, is not used by the runner. Link commands/reviewprs.md into ~/.claude/commands/ only to review by hand.

Before enabling cron, merge these settings into settings.json in every Claude account directory a role can select (including ~/.claude if using the default fallback). Preserve existing settings and deny entries:

{
  "permissions": {
    "defaultMode": "auto",
    "deny": [
      "Bash(git push)",
      "Bash(git push:*)",
      "Read(~/review.auth/**)"
    ]
  }
}

For a custom REVIEW_AUTH, review-auth.sh login claude <account> prints the exact absolute rule. The pre-flight accepts Read(~/path/**) or Read(//absolute/path/**) covering the auth directory or an ancestor. Bare Read removes that tool but does not deny the path to other readers such as Grep. Dot and parent-directory segments are refused: resolving a filesystem path does not prove that the CLI's pattern matches it. A single leading slash is relative to the settings source, not the filesystem root. Other glob forms are refused if coverage cannot be established. These rules are guardrails; arbitrary shell readers can still reach credentials under the same uid.

On an existing box, update all Claude accounts before pulling this change: the bin symlink makes that pull the deploy, and missing rules abort every slot. Then enable cron:

crontab ~/review/etc/crontab.reviewprs

local.conf is where the site-specific settings go: the rsync destination, its credentials, the public URL reports appear at, the posting token and the alert address. It is never committed. Accounts are not in it - they are the directories and role links under ~/review.auth/, described below.

Scripts

Running reviews:

  • bin/run-reviewprs.sh <mode> — the cron entry point. followup | all | rsync | any label. Picks the accounts, runs the pre-flights, freezes the run's configuration (review-run-config.py) and starts the supervisor.
  • bin/review-now.sh <mode|PR> — the same, by hand; a PR asked for by name is reviewed even if it was reviewed recently. --abort <run> stops a run.
  • bin/review-supervisor.py — one run: discovery (review_discovery.py), claims, passes, acceptance and delivery.
  • bin/review-guardian.py — owns one pass: its unit, cgroup, timeout and terminal record. Started by the supervisor, never by hand.
  • bin/review-heavy.sh — what a pass wraps a build or SITL run in, for a build slot; --netns adds a private network namespace through netns-run.sh, so parallel SITL keeps the default TCP ports instead of needing --uds (which changes SITL UART timing and produces false findings).
  • bin/run-reviewprs.sh --resume <run>, bin/review-resume.py — continue an unfinished run from its own record.
  • bin/review-outbox.sh — drains owed deliveries every minute: pages, comments, board.
  • bin/post-comments.py — the comment rules delivery follows: post, edit, or deprecate and repost.
  • bin/review-retire.py — takes a PR off a page that can never be published.
  • bin/pause-runs.sh — stops new admissions; delivery carries on.
  • bin/review-route.py, bin/review-handoff.py — which path owns a target, and the move from the legacy runner.

Accounts and quota:

  • bin/review-auth.sh — sign accounts in, point roles at them, show what each resolves to.
  • bin/quota.py, bin/accounts.py — what each account has left, and which one a role would run on.
  • bin/check-auth.py — mails REVIEW_ALERT_MAIL when a role's account cannot run; see When an account stops working.
  • bin/claude-usage-probe.sh — samples the real usage meter, tagged by account.

Keeping the box healthy:

  • bin/review-env.sh — source this first. Sets $REVIEW_*, TMPDIR, CCACHE_DIR, the toolchain PATHs, GIT_CONFIG_GLOBAL, the publish variables, and reads local.conf.
  • bin/review-gc.py — removes build trees, evidence and old runs once nothing can need them.
  • bin/reap-orphans.sh — kills processes a run left behind, and reconciles supervisor attempts by their recorded identity.
  • bin/refresh-repos.sh — nightly refresh of the base clones, lock-guarded.
  • bin/clone-ardupilot.sh, bin/clone-repos.sh — the base clones, submodules included.
  • bin/base-build.sh — SITL + a ChibiOS board; proves the toolchain and warms ccache.
  • bin/make-runs-page.py, bin/publish-runs-page.sh — the runs dashboard. The publisher never fails its caller; what goes wrong (build, upload, lock busy, killed at the timeout) is in logs/runs-page.log.
  • bin/project-sync.py — the results board; see below.
  • bin/review-context.py, bin/review-baseline.py — what fills each pass's context, and per-run pass counts and minutes.
  • bin/review-pilot.py — freezes, runs and scores a paired quality pilot (quality-pilot.md).

Things the deployment gets wrong if you skip them

Never replace data/locks. Every lock is a kernel lock on a byte range of that one file: a PR, a page, a provider's session slots, an account, a build slot, the reference refresh, a run, the board, pause. They die with their holder and vanish on reboot, so nothing needs cleaning up after a crash, but a new file with the same name is a new set of locks, and two runs would each hold "the" lock on one PR. A stale header written inside it is not a stale lock.

Linger must be on. Each pass runs in its own systemd-run --user unit with a payload cgroup beneath it, outside the supervisor's cgroup, so a dying supervisor does not take running passes with it and an out-of-memory kill takes one pass, not the terminal it was started from. User units die with the user manager when the last login session ends unless loginctl enable-linger is set. Each unit is throttled (MemoryHigh) at 80% of RAM divided by the build slots and killed (MemoryMax) past 40% of RAM.

Every pass gets a two-level working directory. The checkout is runs/<run>/attempts/<attempt>/wt, a detached worktree of the reference clone, so autotest.py's ../buildlogs — and the autotest.lck in it — stays inside that attempt's own directory. Sibling checkouts sharing a parent serialise on one lock, however well their networks are isolated.

Never --uds. AF_UNIX changes the SITL anti-lag throttle from 1024 to 65536 bytes of outqueue, so tests behave differently from what the PR author sees. The Claude passes' prompts say so (the Codex passes run sandboxed with no network and fly no SITL); a pass wraps the test in review-heavy.sh --netns instead — and wraps the test, not itself: inside the namespace lo is the only interface, so a CLI started there cannot reach its own model API.

Reference clones are full clones. Discovery fetches each PR's head into the reference clone, and a pass gets a worktree of it, so nothing is downloaded twice. A partial clone (--filter) is refused when a pass is prepared.

Accounts

Each account is a directory under ~/review.auth, and a symlink per role says which account that role uses. Nothing here is in git — these hold credentials.

auth/claude-ardupilot/   a CLAUDE_CONFIG_DIR   auth/claude-default -> claude-ardupilot
auth/claude-personal/                          auth/claude-rsync   -> claude-personal
auth/codex-work/         a CODEX_HOME          auth/codex-default  -> codex-work
auth/codex-personal/                           auth/codex-rsync    -> codex-personal

default covers ArduPilot work; rsync covers the non-ArduPilot target, which is reviewed on a personal subscription so the project's quota is not spent on it.

review-auth.sh status                        # what every role resolves to
review-auth.sh list                          # the accounts available
review-auth.sh login claude personal         # how to sign one in
review-auth.sh use claude default personal   # move a role to another account

Switching account is repointing a symlink. When the ArduPilot subscription runs out of weekly quota, use claude default personal moves the whole workload across and the next run picks it up; use claude default ardupilot moves it back. Nothing is edited and no run is interrupted.

A directory may record the address it is meant to hold, in a file called ACCOUNT — an address or an id, nothing else, so a token put there by mistake is refused rather than echoed into a run log. A run refuses to start if:

  • the role has no link, and it is not default (a missing claude-rsync must not quietly become the project's subscription);
  • the link dangles, resolves outside auth/, or points somewhere other users can read;
  • the directory holds no credentials;
  • its ACCOUNT disagrees with what it is signed in as;
  • the directory's own record and the CLI disagree about the address — two local records agreeing proves nothing about which subscription pays;
  • the Codex directory authenticates with an API key. An account id left in auth.json by an earlier subscription login still matches ACCOUNT, but the key is what gets billed;
  • auth/ itself is writable by other users, or not yours — private account directories protect nothing if anyone can repoint the links that choose between them;
  • an ACCOUNT exists but cannot be read, or the signed-in address cannot be determined to check it against. A constraint must not disappear because reading it failed.

An inherited credential or provider selector decides the account whatever the role says, so before anything else the run clears every ANTHROPIC_* and OPENAI_* variable and every CLAUDE_*/CODEX_* one whose name contains TOKEN, KEY, AUTH, SECRET, CREDENTIAL, CONFIG_DIR, STORAGE, BASE_URL, HOME or USE_, anywhere in the name, and prints what it cleared - names only. Naming them one at a time missed CLAUDE_SECURESTORAGE_CONFIG_DIR, which points the CLI at another credential store while the selected directory still reports its own address; matching only the end of the name missed CLAUDE_CODE_OAUTH_TOKEN_FILE_DESCRIPTOR. It clears rather than refuses because a manual run from a terminal inside Claude Code carries CLAUDE_CODE_* variables that are not credentials.

A variable declared readonly survives unset, and bash reports that only on stderr - so the sweep checks each name is really gone afterwards and the run refuses if one is not, rather than printing it as cleared.

No sweep can be proved complete, so the run also asks the CLI what it actually did. claude auth status --json reports authMethod, apiProvider and configDirectory: the run refuses anything but a claude.ai login on the firstParty provider reading the directory the role selected. A token, a cloud provider or another config directory all show up there whatever the environment looked like. All three are required: a CLI too old to report them must be upgraded. Empty, null, sentinel or control-bearing fields are invalid, not missing metadata. The CLI must also exit successfully; a JSON body from a failed command is not proof of authentication. Paths are preserved without trimming whitespace.

Codex's auth.json must contain subscription tokens and a valid account id; an id left behind without tokens is not a login. Unsupported authentication modes are refused. The runner and status share the check of config.toml:

  • inference endpoints must use HTTPS on an OpenAI host, with no userinfo, nonstandard port, query or fragment;
  • provider credentials, auth commands, headers and query overrides are refused, including inline bearer tokens;
  • model providers must be openai, and the credential store must be file if explicitly selected. A keyring can authenticate somebody other than the account recorded in the checked auth.json;
  • unused custom provider definitions are allowed; selecting one in any config layer or profile is refused. Overrides of the openai provider are checked;
  • every table is walked, not only the ones that route inference: [otel] takes an exporter endpoint and its own authorization header. mcp_servers is the exception - an MCP server's endpoint and headers authenticate that server, and a third-party host there is the point of it;
  • an unreadable, malformed or dangling config is refused. An absent one is fine;
  • config.toml is one layer of several. /etc/codex/config.toml, managed_config.toml and requirements.toml, and the <name>.config.toml that --profile merges, are all checked, and the refusal names the layer.

The refusal names the setting, including in status, without printing its value. Reported identities are validated before they reach logs.

One layer is deliberately not checked here: Codex also merges project-local configuration from the tree it runs in, and a run works in checkouts of other people's pull requests. Nothing in this pre-flight can settle that - the checkout does not exist yet when it runs - so it needs its own answer in the part of the system that prepares those trees, not here.

Record the account id for Codex and the address for Claude — that is what each tool reports, and the runner compares like with like.

Both tools are checked, not just Claude: a missing auth.json used to surface only when the validation pool failed, well into a run.

The reviewing agent is started with --add-dir $REVIEW_ROOT and reads other people's pull requests. The accounts used to live inside that directory, which is why they no longer do: $REVIEW_AUTH is outside the granted review tree. Relative paths such as ../review.auth can still reach it. This reduces exposure without providing containment - the agent runs as the same user, so file modes stop nothing and a shell reader is not bound by a tool rule. The permission pre-flight therefore still requires a deny rule covering $REVIEW_AUTH, alongside the git push denials, and refuses to start without one; the Install block above gives the exact settings. Note that a rule written for the old layout - Read(~/review/**) - no longer covers the accounts, and is refused. The migrator preserves that rule and adds protection for the new root. A refusal is written to the run log and shows on the dashboard: these checks used to run before the log was opened, so under cron a refused run said nothing anywhere and the slot simply went quiet.

status clears the same variables before it asks, and then makes exactly the judgements the runner makes - including a variable it could not clear, a partial answer from the CLI, and a redirected Codex config - so a role that reads as healthy there is one a run will accept. That includes the fallback below: a default role with no link is reported as the tool's own account, with the same checks applied, rather than as "not set".

Two things worth knowing. An account directory keeps its address in <dir>/.claude.json, but only if it was created by claude auth login under CLAUDE_CONFIG_DIR; the tool's own ~/.claude keeps it elsewhere, so a run whose role resolves there leaves the variable unset rather than setting it to the same path, which would make claude auth status report no address at all. That applies only when the path is genuinely that directory: if ~/.claude is itself a symlink the variable is pinned to what it resolves to now, so repointing it mid-run cannot move the work to another account. A missing default link uses the tool's own directory; every other missing role is refused.

Switching takes a per-role lock. Two switches of the same role could otherwise interleave, and a switch that turned out to be invalid would revert over one that had already succeeded. If a revert cannot be made, use says so and names the account the role has been left on rather than reporting success.

Reading what an account has left

runner/bin/quota.py asks each account what quota it has, and records it. The selector below reads those records.

quota.py            a table of every account
quota.py --json     the same as records
quota.py --record   append them to $REVIEW_LOGS/quota.jsonl

The dashboard is published, so the identities in it are masked - an address keeps its domain and an account id its first eight characters, which is enough to tell the right account from the wrong one and not enough to reuse. The whole value is on the box, from review-auth.sh status or quota.py.

Cron records a reading for every account hourly, at :05, and the runs dashboard has a Quotas section showing the newest one per account: what is left, the windows behind that figure, when the soonest rolls over, and how old the reading is. The page reads the recorded file - it is rebuilt every ten minutes, and asking an account for its quota starts the CLI, which is how runs and probes come to contend for the OAuth refresh.

Both CLIs publish structured figures, which is worth knowing because the hourly probe still reads Claude's by regex over English prose:

tool where windows
claude claude -p /usage --output-format stream-json, usage_report.rate_limits session (5h) and weekly, plus per-model
codex codex app-server, account/rateLimits/read whatever the plan has - a Pro account reports weekly only

Two things the structured answers settle that the old sources could not. Codex's is a live query rather than the last figure a session rollout happened to record, and it carries ordinaryUsageAllowed and a credit balance - which is what decides whether running out means stopping or starting to cost money. Claude's arrives as a window list with reset times rather than a sentence.

free_pct is what is left of the worst window that gates ordinary work. Per-model windows are recorded but do not count towards it: a spent model is not an account that cannot start. The app-server shuts down when its stdin closes, so the query has to hold the pipe open - writing the request and closing it loses the reply.

When an account stops working

A run refuses to start on an account that is signed out, which is safe and silent: on 2026-10-09 ~/.claude lost its login at 18:05, the 18:13 all run logged status=wrong-claude-account, and nothing else said so. bin/check-auth.py runs from cron every three hours and mails REVIEW_ALERT_MAIL (set in etc/local.conf) when a role cannot run. It reads two things, and opens no session of its own - using an account can refresh its token, and two refreshes at once can sign it out:

  • review-auth.sh status, the sign-in checks a run makes before it starts: a row that is not signed in, or that runs will refuse, is a problem.
  • the newest hourly probe in quota.jsonl for each account a role uses. The probe really uses the account, so a login the server no longer accepts shows up there while it still looks signed in locally. A busy lease, or a reading older than three hours, is not counted.

The mail names each role, why, and the command that signs its directory in again. It is sent on every check that finds a problem, so it repeats until the account works. check-auth.py --dry-run prints it instead; the log is logs/auth-check.log.

It goes out through the local sendmail, from REVIEW_ALERT_FROM when that is set. Gmail refuses mail that passes neither SPF nor DKIM (550 5.7.26), which is all mail sent straight from a home address, so the box needs a relay that signs for the sender's domain. The reference box relays through the site's own mail server, which already trusts the LAN and DKIM-signs for its domain:

sudo postconf -e "relayhost = [<mail server>]"
sudo systemctl reload postfix

with REVIEW_ALERT_FROM an address in that domain. Without such a server, relay through smtp.gmail.com:587 with SASL and an app password instead.

Choosing an account

runner/bin/accounts.py decides which account a role runs on, from what the accounts have left.

accounts.py                what every role would select, and why
accounts.py --role rsync   one role
accounts.py --record       append the decisions to $REVIEW_LOGS/select.jsonl
accounts.py --live         ask the accounts rather than using a recent reading
accounts.py --select --role R   the answer a run acts on: "tool<TAB>name<TAB>dir"
                                per line, the walk on stderr, exit 3 for nothing
                                usable

The policy is $REVIEW_AUTH/policy.json, copied from runner/etc/policy.json.example - an ordered list of account names per tool and role. Order is priority, and the list is also the whole of what that role may reach. That second part is the one that matters: rsync reviews a project the ArduPilot subscription does not pay for, so its lists name only the accounts that may pay for it, and running out of its own cannot fall through to one that may not. A missing or invalid policy is refused rather than defaulted.

An account is used when more than min_free_pct is left. Unknown is not spare - a reading that failed, or one carrying no figure, is skipped rather than tried, because an unattended run cannot check the guess. Nor is credit: an account past its included allowance (ordinaryUsageAllowed false) is passed over however healthy its window looks, because these runs may stop but may not start billing. Figures come from the hourly recording while it is younger than fresh_minutes, and from the account itself otherwise: asking starts the CLI, and that is what makes two processes contend for the OAuth refresh below.

What a run does with it

run-reviewprs.sh selects once per run, and the run's configuration freezes the choice: a resumed run spends the account it started on, even if a role link or the quota has moved since.

Every check that follows - signed in, subscription login, first-party provider, the directory the CLI actually read, the ACCOUNT record, Codex's config.toml

  • runs against whatever was selected, so nothing is validated any less than when the role links decided alone. The runner also re-checks the chosen directory against the same containment and ownership rules a role link must satisfy (account_dir_ok), rather than taking accounts.py's word for the path.

With every listed account spent, the run still starts, but starts no inference: it logs quota: inference deferred; supervisor will drain deliveries, recovers unfinished work and delivers what is owed, and leaves the reviews to the next run. Reviewing without the Codex passes was the alternative, and a review is not worth having without them.

Three ways out of the selector, and they are different:

situation what happens
no policy.json the feature is not configured; the role links decide, as before
policy.json unreadable or invalid status=account-policy-error, the run stops
policy fine, every account spent inference deferred; delivery and recovery go ahead

The second is not the first. A policy that is meant to be in force and is not would leave a run choosing its own account, which is the one thing this must never do.

The OAuth refresh lock

claude takes a lock in its config directory while it refreshes the OAuth token, and leaves it behind if the refresh fails - reporting the failure as its output and exiting zero. Anything that starts next finds it and dies with "another Claude Code process is refreshing it".

A run clears a stale one before it reads the quotas, and again once its accounts are chosen and checked. The second clear is the one that matters: between them the run takes its own usage reading, and claude -p /usage is exactly the sort of short invocation that meets an expired token, fails to refresh, and leaves the lock a few seconds before the first pass needs it. A lock is only removed when no live claude is using that directory. That was the 2026-09-19 19:47 followup and three others in the five days before it - each one a whole slot lost, three hours apart.

The probe says so now when it comes back with nothing, which it did not: a non-zero exit was swallowed by || exit 0, and a reply carrying prose rather than a meter simply recorded nothing. Either way the run log showed no probe at all, so the only sign was a reading missing from the history.

Compiling what pymavlink generates

A review of a generator change can execute its output rather than read it. bin/install-gen-toolchains.sh installs the compilers; this section is the part that is not guessable - what each language needs beyond its own compiler.

pymavlink's own CI generates and never compiles, so an output that does not build is not something upstream would have caught.

The definitions are not in the pymavlink checkout (message_definitions is a submodule that is not fetched). They are in the mavlink clone:

D=$HOME/review/repositories/upstream-mavlink/message_definitions/v1.0
export PYTHONPATH=$HOME/review/repositories
cd $HOME/review/repositories/pymavlink
python3 -m pymavlink.tools.mavgen --lang=<L> --wire-protocol=2.0 -o <out> $D/<dialect>.xml
language compiler builds
C, C++11 gcc, g++ yes
Python, JavaScript interpreters yes
Lua, WLua luac5.4 -p yes
TypeScript tsc --noEmit no (see below)
Java javac v1 and v2
Ada gprbuild, then run obj/test v1 and v2, minimal and standard
CS dotnet build -f netstandard2.0 v1 and v2
ObjC gcc -x objective-c + GNUstep v1 140/144, v2 228/237 (see below)
Swift swiftc v1 only (see below)
Spin2 flexspin -2 v1 and v2

Java needs nothing special:

javac -nowarn -d build $(find com -name '*.java')

Ada builds the test project the generator writes, and the binary runs:

cd <out>/tests && gprbuild test.gpr && ./obj/test

CS is an SDK-style project with NuGet PackageReferences, so the build needs network the first time. net461 builds too, via the reference-assemblies package; -f netstandard2.0 alone is enough to compile-check the generated code.

ObjC needs three things the generated tree does not say:

  • the C output as well, on the include path: the ObjC is a wrapper over it and MVMessage.h imports mavlink.h
  • every dialect subdirectory as a -I, because MVMavlink.h imports each dialect's aggregate header by bare name
  • Foundation forced in, because no generated header imports it - the code uses NSObject, NSData and NSString and assumes a prefix header, the way Xcode supplies one
INC="-I."; for d in <out>/*/; do INC="$INC -I$d"; done
gcc -c -x objective-c $(gnustep-config --objc-flags) -include Foundation/Foundation.h \
    $INC -I<c-out> -I<c-out>/<dialect> <file>.m -o <file>.o

Swift has no build file; compile the sources as a module:

cd <out> && swiftc -emit-module -module-name MAVLink -o /tmp/m.swiftmodule $(find . -name '*.swift')

Spin2 emits one file, named after --output rather than placed in it, so -o <out> writes <out>.spin2. -2 selects the P2, and Lib has to be on the include path or nothing resolves:

flexspin -2 -L /opt/flexspin/Lib -o /tmp/out.binary <out>.spin2

TypeScript writes into <out>/enums and <out>/messages without creating them, so generation exits 1 with no message unless they exist already. Typecheck with its own tsconfig.json and no file arguments - naming files on the command line makes tsc stop at TS5112 before checking anything, which looks exactly like success:

mkdir -p <out>/enums <out>/messages
cd <out> && tsc --noEmit --skipLibCheck

Six defects this found

All six are in master, none is new, and none is reachable without a compiler.

  • ObjC, any field with a multi-line description. mavgen_objc.py:269 emits //! ${description} - a one-line comment for text that is not one line, so every line after the first lands in the header as code. MISSION_COUNT's opaque_id produces This field is used when... where a declaration belongs. Eight of common.xml's generated files will not compile: MISSION_COUNT, MISSION_ACK, HOME_POSITION, RADIO_RC_CHANNELS, STORAGE_INFORMATION, AUTOPILOT_VERSION, and the two aggregates that import them. The template line dates from the original generator in 2013; the descriptions grew multi-line later.
  • ObjC, a field called description. OPEN_DRONE_ID_SELF_ID has one, and the accessor collides with NSObject's own -description: redefinition of '-[MVMessageOpenDroneIdSelfId description]'. Nothing in the generator reserves the names the base class already uses.
  • Swift generates MAVLink 2 that cannot compile. mavgen_swift.py:148 writes public static let id = UInt8(${id}). Message IDs are 8-bit in v1 and 24-bit in v2, so every v2 message numbered above 255 fails with integer literal '12900' overflows when stored into 'UInt8' - nine of them in common.xml, all OpenDroneID. The file has no reference to wire_protocol_version at all: the generator is v1-only and says nothing when asked for v2. test_generate_all.sh runs --lang='Swift' --wire-protocol=2.0 and passes, because it only generates.
  • TypeScript has the same multi-line description leak as ObjC. The comment is // ${description} on one line, so everything after the first line of MAV_CMD_DO_FIGURE_EIGHT's description becomes TypeScript. 1761 errors across mav-cmd, mav-frame, mav-ftp-err, mav-protocol-capability, mav-standard-mode, autopilot-version and storage-information.
  • TypeScript does not create its own output directories. generate() opens <out>/enums/... without mkdir, so a first run into a fresh directory dies with FileNotFoundError that mavgen reports only as exit 1.
  • Ada, common.xml and ardupilotmega.xml. mavgen_ada.py:459 asserts types_size[f.enum] == f.type_length and raises Different size for one enum, so generation stops with a traceback before writing anything. minimal and standard generate, build and pass their generated test. Whether generator/Ada/v2/test.sh ever passed depends on the definitions it was run against - the assert is about the XML, not the generator - but against the current upstream definitions it stops on its second dialect.

The APReview Results board

github.com/orgs/ArduPilot/projects/33 — every open PR carrying a trigger label that this system has posted a verdict on, with a sortable Result column (ACCEPT, COMMENT, REQUEST CHANGES) and a PR Author column. Not Author: that name is reserved, and createProjectV2Field refuses it with "Name cannot have a reserved value".

bin/project-sync.py makes the board match reality. It takes no arguments and is told nothing by the run that calls it: it asks GitHub what is labelled, open and reviewed, and adds, relabels or removes rows to suit. The same command is therefore correct at the end of a review and from cron a quarter of an hour later.

project-sync.py                what it does on the two triggers below
project-sync.py --dry-run      say what would change, change nothing
project-sync.py --show         the verdict it reads for each PR, and from where
project-sync.py --prune-only   only remove what no longer belongs

Two triggers, both calling bin/project-sync.sh, which cannot fail its caller:

  • the EXIT trap of every run, beside the dashboard publish
  • */15 * * * *, which is what takes a merged or closed PR off — nothing tells us a PR closed, it simply stops coming back from the search

Where the verdict comes from

The posted comment, not the published report. Under the legacy runner each label's report page was overwritten by the next run of that label, so it only ever covered the most recent sweep: measured on 2026-09-26, the reports held a verdict for 35 of the 168 open labelled PRs, against 166 from the comments. Where both had one they disagreed ten times, the report being the stale one. Pages are now rendered from the store and keep every PR, but the comment is still what the PR's author and reviewers see, so it stays the source.

apreview_verdict.py reads it from line 2 of the comment, directly under the AI-generated marker line:

**Automated review note — AI-generated (Claude+Codex), validated against the live diff.** Please sanity-check before acting.
**Verdict: REQUEST CHANGES**

That line is the whole rule. It is there for the reader first - the conclusion before the reasoning, where some reports had made it hard to find - and the board simply reads the same line. post-comments.py refuses a body without it, unless the plan says the entry is a no-verdict note (a "your code moved" followup, which leaves the last stated verdict standing). The invisible <!-- apreview: verdict=... --> marker a few days of comments carried is still read as a fallback; nothing else is.

It was not always so. Until 2026-09-28 the verdict was recovered from prose, in seven phrasings surveyed across the open labelled PRs, with anchors, hedge words and CommonMark stripping to keep "if tests pass, the verdict moves to ACCEPT" from reading as ACCEPT. Two adversarial reviews each found a fresh way to read the wrong verdict out of it. verdict-backfill.py is where that parser retired to: it read every open labelled PR's newest live comment once, put the line on it (six needed a person to decide), and remains for any comment written the old way.

The verdict is the newest comment that states one, not the newest comment. A followup note says only that the author's code moved and carries no verdict; treating that as "no review" would drop a reviewed PR off the board. Superseded comments are skipped, so a stale verdict folded into a deprecation cannot outrank a live followup that has none.

A comment counts as ours only if it carries the AI-generated marker and was posted by one of REVIEW_COMMENT_ACCOUNTS. Author alone is not enough: commenting moved from a person's account to the bot, so the older reviews are authored by someone who also writes ordinary comments, and "I would request changes here" is not a verdict.

Making the column visible

A field exists whether or not a view shows it. Creating Result set a value on all 166 rows and the board displayed none of them, because a new field is not added to views that already exist - so the sync configures the views too. It adds what is missing to what a view already shows rather than replacing the set, so a column someone arranged by hand survives, and a view already showing all of ours is left untouched.

Column order cannot be chosen. visibleFieldIds is documented as ordered, but asking for Result, Title, Repository returns Title, Repository, Result: columns follow the order the fields were created in, so Result sits last.

Which account it runs as

Not the bot. review-env.sh exports GH_TOKEN so reviews are posted as AP-Review, and that token is deliberately narrow — public_repo and nothing else, which cannot even read a project. project-sync.sh unsets it and falls back to the box's own gh login: writing to a project needs the project scope, and granting that to the commenting token would widen what a compromised review run can reach, for the sake of a board whose items carry no author anyway.

The box login needs the scope once:

gh auth refresh -s project,read:project

Until then the sync exits 2 and says so; nothing else is affected.

A row stays while its PR is open

A trigger label is what brings a PR to the board: the label search is how new reviews are found. Losing the label does not take it off. DevCallEU and DevCallTopic come off after the call, the PR is still open and still reviewed, and followups still land on it, so it stays until it is merged or closed. The candidates are therefore the labelled PRs plus everything already on the board.

A row the label search did not return is a question, not an answer. Search is capped at 1000 results, its index lags, an org can answer empty, and a transient failure looks the same as "everything merged". So each such row is asked about directly, by the PR's node id - a repository renamed or transferred and its old name reused can put a different, closed PR at the same address. Open: the row stays, and its verdict is re-read from the PR's own comments, so a followup review reaches the board with or without the label. Closed or merged: the row goes. No answer: the row stays as it is.

That check is the protection. The count threshold behind it is an alarm for something systematically wrong: more than 25 removals, or more than a quarter of the board, or the last row off a board that had more than one, is refused with exit 3 and nothing changed. --force-prune overrides it.

Numbers are sent with -F, not -f. -f makes every value a String and Int! refuses a String, which the caller reads as "cannot tell" - safe, and useless: it made every check answer unknown, so nothing was ever removed.

Comments are read newest-first and paged backwards until a verdict is found, up to five pages, and not past a cursor that fails to advance. A PR busy enough to push its review out of the newest hundred would otherwise look unreviewed, and lose its row.

Nothing is written before the safety check - not the view, and not the two fields either. A refused sweep that had already created a column was printing "Nothing was changed" untruthfully.

One sync at a time: project-sync.sh holds a lock, because the cron sweep and a run's exit path can fire together and interleave - the first reads the board, the second adds a row, the first deletes it as "not in my search". Every gh call has a deadline, since the sync can run from a run's exit path.

Called from a legacy run's exit trap, the wrapper inherits fd 9 already open on the run lock. It closes that copy before opening its own lock, and if the sync lock cannot be opened it logs and skips: otherwise flock would be asked about the inherited run lock, say yes, and the sync would run with no lock at all. The lock descriptor is then handed to python, which does the work and so holds the lock for as long as it runs - a wrapper killed under it must not free the lock for the next sweep while the first is still writing. Search, board and comment paging each stop on a cursor that fails to advance.

Publishing

Reports go out with rsync, either to an rsync daemon (rsync://user@host plus a password file) or over ssh (host:path, no auth option). Set REVIEW_PUBLISH, RSYNC_AUTH and REVIEW_PUBLIC_URL in local.conf; comments link to the pages under REVIEW_PUBLIC_URL. With REVIEW_PUBLISH unset, a run keeps its report local and says so.

Pages are rendered from the store, not from a run, so a page always shows every PR that belongs on it, whichever run reviewed it last. Each is a delivery in the outbox: a failed upload retries hourly without end, and the comment that links to a page waits until the page is up. What goes where:

page what
DevCallReviews/<label>/devcall_pr_reviews.html each label's PRs; DevCallReviews/<YYYY_MM_DD>/<label>/ keeps a dated copy, for a dev-call label the date of its next call
DevCallReviews/followups/<stamp>/devcall_pr_reviews.html one followup run's re-reviews
<REVIEW_RETAINED_PREFIX>/<owner>/<repo>/<n>/index.html a PR reviewed by name (default prefix PRReviews)
UserReviews/<author>.html an author sweep
RsyncReviews/index.html the rsync target
DevCallReviews/runs.html the runs dashboard

REVIEW_PUBLIC_URL is always used as <url>/<Module>/..., so moving the whole tree is a change to that one variable and nothing in the code.

The reference deployment, and the move off fjall

Since 2026-09-26 the reports are served from firmware.ardupilot.org/Tools/APReview, not from uav.tridgell.net:

REVIEW_PUBLISH="rsync://reviews@autotest.ardupilot.org"
RSYNC_AUTH="--password-file=$HOME/review/etc/rsync.password.autotest"
REVIEW_PUBLIC_URL="https://firmware.ardupilot.org/Tools/APReview"

Three rsyncd modules on that host — DevCallReviews, RsyncReviews and UserReviews, the same names fjall used, so only the host changed. They write into /home/autotest/APM/buildlogs/binaries/Tools/APReview/<module>, as uid = autotest. That path is under the tree the site serves: /home/autotest/APM is a symlink to /storage/autotest/APM, which is what firmware.ardupilot.org publishes.

Each module carries hosts allow = fjall.tridgell.net, which is the review box's egress address — its own hosts allow rather than relying on the file's global one, so the review credential cannot be used from anywhere else.

Old links are not broken. /home/tridge/UAV-web/.htaccess on fjall carries:

RedirectMatch 301 ^/(DevCallReviews|RsyncReviews|UserReviews)(/.*)?$ \
    https://firmware.ardupilot.org/Tools/APReview/$1$2

Permanent, because those URLs are in thousands of review comments already posted on GitHub and cannot be rewritten. The path after the module name is carried over unchanged, so deep links and their #pr<number> anchors still land in the right place. The old files are deliberately left in place underneath; nothing serves them now, but they are the fallback if the new site has to be rebuilt.

Supervisor operations

$REVIEW_ROOT/etc/routing.json says which targets the supervisor owns; the rest go to the legacy prompt-driven runner. The reference box has been fully cut over since 2026-09-29: its routing names the mode all, which hands everything to the supervisor. A missing file means everything is legacy-owned, and an invalid one stops admission. The three arrays list supervisor-owned canonical repositories, labels and modes; repository ownership applies to manual URLs, bare and qualified PR references, followup, author and all-label discovery. Query it without starting a run:

. "$HOME/review/bin/review-env.sh"
python3 "$REVIEW_ROOT/bin/review-route.py" rsync
python3 "$REVIEW_ROOT/bin/review-route.py" 'RsyncProject/rsync#123'
"$REVIEW_ROOT/bin/run-reviewprs.sh" rsync --dry-run

On a box where both paths still own something, the prompt's review-route.py --filter old step is mandatory for every candidate source, including manifests and board rows, and new discovery applies the inverse boundary in code. Use @author for author mode. Do not transfer a mutable PR label as a substitute for the drained repository handoff.

The shared lock file is $REVIEW_DATA/locks. Never replace or unlink it: ownership is on its inode, with separate OFD byte regions for PRs, pages, providers, accounts, reference refresh, runs, board and pause. A stale advisory header is not a stale kernel lock. New runs do not take $REVIEW_ROOT/etc/reviewprs.lock; old runs still do. pause-runs.sh 60 takes both pause and the old lock (pause is immediate even while an old run finishes). pause-runs.sh status and pause-runs.sh resume inspect/signal the recorded process identity. Delivery continues while admission is paused.

Check linger and the user-systemd backend on a new box:

loginctl enable-linger "$USER"
loginctl show-user "$USER" -p Linger
systemctl --user list-units 'review-attempt-*'

Guardian units run independently of controllers and own the PR/provider/account descriptors and delegated payload cgroups. A controller exit does not kill another run or its surviving attempts. reap-orphans.sh still kills the old runner's unregistered leaks by path, since nothing registers them, but skips anything the supervisor owns (a review-attempt- cgroup or REVIEW_JOB_DIR in the environment) and reconciles those by recorded identity instead. The path scan retires with the old runner. Keep new full reference clones under REVIEW_NEW_REPOS (default $REVIEW_DATA/references) while old mutable clones still exist.

The wrapper freezes account homes (without secrets), model pins, effort, permissions, granted directories, endpoints, repositories, limits and admission deadline settings. Codex needs a model pin in the selected account's config.toml or REVIEW_CODEX_MODEL; its configured sandbox and effort are also frozen. Up to REVIEW_ACCOUNT_SLOTS (default 8) review sessions share one login, matching REVIEW_POOL_SIZE (default 8) passes per provider; concurrent sessions, including a simultaneous token refresh, were tested safe on 2026-09-30. Builds stay at REVIEW_HEAVY_SIZE (4). Each attempt's unit is throttled (MemoryHigh) at 80% of RAM divided by the build slots and killed (MemoryMax) past 40% of RAM; on blu6 that is about 6G and 12G. Sessions hold numbered slots, not the account key, so quota probes and admission are never blocked by a running review. Exhausted quota defers inference but still starts recovery and drains deliveries. GitHub writes in the new adapter need explicit REVIEW_GITHUB_WRITES=1 in local.conf; installation does not enable it.

"$REVIEW_ROOT/bin/run-reviewprs.sh" --resume "$REVIEW_DATA/runs/<run>"
"$REVIEW_ROOT/bin/review-now.sh" --abort '<run>'
"$REVIEW_ROOT/bin/review-outbox.sh"

A publish entry whose page can never exist again (a renamed prefix, a missing rsync module) exhausts its attempts and then blocks the comment behind it, because verification repairs the same intent. Take the PR off that page:

python3 "$REVIEW_ROOT/bin/review-retire.py" --data "$REVIEW_DATA" \
    --reason 'PRReviews module never existed' \
    'page:review/PRReviews/ardupilot/mavproxy/1764/index.html' 'pr:ardupilot/mavproxy#1764'

That marks the membership row removed and unreachable and writes the superseded receipts delivery would have written, so the next drain posts the comment and later generations do not inherit the page.

Resume uses the frozen run, even after role symlinks or models change, and refuses a target transferred back to old ownership. Abort durably records the request before signalling verified boot/PID/start-time identities. It never waits for the run lock; after a bounded grace it escalates those identities and starts bounded cleanup. Accepted results and debts survive. Repeat abort or resume after a cleanup timeout. The outbox drain is bounded and does no inference; each entry uses its own frozen delivery settings.

For the rsync canary, use the exact mirror, dry-run, handoff and rollback commands in the migration section. The handoff pauses both paths itself; do not first take a separate manual pause and then wait on your own pause. It waits for old jobs and new attempts/debts, imports previous sections, scrubs shared pages, publishes them under page locks and only then switches routing. Both directions are idempotent and have --dry-run. A pending publication journal must be completed before reversing. The full cutover is review-handoff.py new --full --pages <mirror>, with a --dry-run first; it needs a reference clone for every repository under $REVIEW_DATA/references (seed them from the old base clones with git clone --reference <old> --dissociate --recurse-submodules), and one under its basename for every ArduPilot-owned submodule of ardupilot, which discovery sweeps without a repos.json entry (seed each from the checkout inside the ardupilot reference clone: git clone --reference <its git dir> --dissociate <url> <basename>). A run refuses to start while one is missing. Afterwards the existing cron lines route to the supervisor by themselves. The new cron alternatives are commented out in runner/etc/crontab.reviewprs; replace the appropriate old line explicitly, never enable both.

When a run is stuck, start with runs/<run>/summary.json, then its attempts/<attempt>/status.json, job.json, payload.log, launch.json and the unit journal. The dashboard shows PR states, delivery debt, provider, heartbeat and session-attributed usage. A silent log or old heartbeat does not prove death: boot/PID/start time and cgroup state decide liveness. Check outbox/*.json for a dependency, sending/uncertain write or retry deadline; check receipts before retrying a post. Check account/quota and pool ownership before assuming a queued PR needs another run. Dashboard generation and rsync replacement share page:review/DevCallReviews/runs.html.