[Amperstrand]

What

Adapts the patterns that make shared lab infrastructure survivable — labgrid's acquire/use/release model, per-run ephemeral compose stacks, preemption-safe CI runners — to our single-docker-host cloud-lab, and writes down the twelve lessons that motivated each rule.

tests/cloud-lab/lab.sh — the run-scoped runner

One lab = one run = one lease. A single up creates, and nothing else shares: a compose project cl-<run-id> (stable per checkout: user + branch slug + worktree-path hash), container names, an allocated 172.31.64–250/24 subnet (inspected against live networks), its own images, and zero host port bindings. Plus:

• up --wait --wait-timeout 300 — kills the boot race
• tap <port> <service>:<port> — managed TCP taps (pidfile + log in run state; killed by recorded pid, never by pattern)
• down — taps, containers, network, volumes, state, all of it
• reap / reap --all — bounded, label-based (cl.lab.run=<id>) reclaim after any death, including SIGKILL; --all is a janitor with REAP_MINUTES

tests/cloud-lab/RUNBOOK.md — the why

The pattern/source table (labgrid, ephemeral-CI stacks, preemption-safe runners) and the lessons catalog — each one a real incident on this host: unstable run ids minting unreachable runs, stale shared :latest images probed as new code, global container_name collisions, the one-lab-per-host pinned subnet, pattern-matched pkill reaping a neighbor's taps, relative-$0 breakage after self-locating cd, nohup batteries dying with their spawning shell, vendored trees breaking gofmt, wrong-worktree command execution, shell-eaten backticks in gh --body, and unbounded teardown wedging callers.

Verified live (this branch, this host, beside another session's lab)

step	result
up	healthy, own subnet 172.31.64.0/24
smoke via run	test_smoke_payment.py 6 passed
tap 18885 mint:8085	200 through the tap
down	zero containers/networks/volumes/taps/state
reap on 5 crashed runs	all fully reclaimed
reap --all (fresh run, default age)	run untouched
reap --all (REAP_MINUTES=0)	reaped
run-id stability across invocations	✓
make go-battery	16 modules green

Notes

• The existing per-checkout COMPOSE_PROJECT_NAME/.env guidance stays valid for simple cases; lab.sh is the no-footgun path for anything concurrent.
• Every rule in RUNBOOK exists because its violation cost real debugging time on this repository — it is written to be handed to the next session, human or agent.

[Amperstrand]

Technical review (evidence pass)

Reviewed at 415986a (single commit, MERGEABLE, 0 commits behind main, +348/−0 across 4 files). This is exactly the infrastructure this shared host needs, and the core isolation claims are real — I reproduced them live beside a third session's concurrently running lab. Two substantive gaps and one must-fix typo below; none of them undercut the design.

What I actually ran (all on this host, today)

Another session's lab.sh run (cl-ubuntu--a08e8a-*, subnet 172.31.64.0/24) was already live when I started, which made this a genuine three-tenant test rather than a staged one:

step	result
docker compose config (default profile, merged override)	container names cl-<run>-<svc>, zero published ports, subnet fully replaced (base 172.28.0.0/16 gone, not appended), static IPs remapped
lab.sh up × 2 (two worktrees, same SHA)	run A cl-ubuntu--755b31 @ 172.31.65.0/24, run B cl-ubuntu--1f4120 @ 172.31.66.0/24 — allocator inspected live networks and skipped the tenant's .64; all 12 lab containers across 3 runs healthy simultaneously
host ports (ss -ltn)	0 listeners on 8085/8086/8087/2121/21214 with three labs up
cross-run reachability	A's upstream → A's mint 200; A → B's mint unreachable (timeout) — real network isolation, not just naming
lab.sh run --rm client -q test_smoke_payment.py (run A, two other labs live)	6 passed in 6.90s
crash sim on B (docker rm -f containers + network) → lab.sh reap	0 containers, 0 volumes, state dir removed — full reclaim
lab.sh down (run A)	containers, network, volumes, taps, state all gone; tenant run untouched throughout
shellcheck tests/cloud-lab/lab.sh	clean

Against the incident classes this PR targets: (a) container_name collisions — solved via the $PROJECT-<svc> rename; (b) subnet overlap — solved via live-inspected 172.31.64–250/24 allocation, proven with three concurrent /24s; (c) host ports — solved via ports: !reset []; (d) runbook — mostly, see the override.yml note below. Backwards compatibility is a clean pass: the diff touches no existing file's behavior (docker-compose.yml unchanged; README additions only), so the README quick start, all four lane scripts (run-keyset-rotation.sh, run-external-mints.sh, run-rejection-safety.sh, renewal_e2e.sh), and the COMPOSE_PROJECT_NAME/.env per-checkout path all work unchanged. No GitHub workflow invokes cloud-lab, so no CI lane invocations exist to break. I did not re-run make go-battery — the diff touches no Go module surface; shellcheck is the relevant battery here and it's clean.

Must-fix before merge

CHANGELOG links the wrong PR. The entry at CHANGELOG.md:184-191 cites (#551); this PR is #557. One-word fix; per the changelog rules the link is load-bearing at release-notes time.

Substantive gaps (follow-ups, ideally in this PR)

1. upstream-ext is missing from the override loop — the external-mints lane cannot run under lab.sh. The loop at lab.sh:78 covers mint mint-fees mint-rotate upstream reseller client but not upstream-ext (docker-compose.yml:161-187). Verified with docker compose config --profile external-mints against a generated override: the merged config keeps container_name: tg-upstream-ext, published: "21214", and ipv4_address: 172.28.0.12 — the last one is outside the run's allocated /24, so lab.sh up --profile external-mints fails at container creation, and the surviving host port is exactly the collision class this PR exists to kill. Not a regression (run-external-mints.sh stays on plain compose and works), but the README/RUNBOOK sell lab.sh as the shared-host path while one lane can't use it. Fix: add upstream-ext to the loop plus a svc_ip case (e.g. .12). Longer-term, the hardcoded list will need extending with every new lane service (#522, #535 both add ones) — deriving it from docker compose config --services would end the whack-a-mole.

2. tap reports success when its port is already taken — and your probe then silently hits the neighbor's service. This one bit me during verification, so here is the live transcript: with the tenant's tap already listening on 18885, lab.sh tap 18885 mint:8085 printed tap: localhost:18885 -> 172.31.65.2:8085 (pid 1344714, ...) — but socat had died instantly (tap-18885.log: E bind(...): Address already in use), the pidfile pointed at a dead pid, and curl localhost:18885/v1/keys returned 200 from the other session's mint through their tap. My first "tap works" data point was in fact served by the neighbor; a positive control on a free port (18886) bound, forwarded, logged traffic, and was reaped-by-pid at down as designed. On a shared host this is precisely the wrong-target confusion the RUNBOOK is about. Fix at lab.sh:183-186: after spawning, confirm the listener actually exists (poll ss -ltn / kill -0 for ~1s) and die loudly otherwise. While there, consider binding loopback-only (TCP-LISTEN:18885,bind=127.0.0.1): ss shows the current tap on *:18885, i.e. every tap is exposed on all host interfaces, and these are unauthenticated mints/portals.

3. reap --all defaults to reap when a timestamp fails to parse. The age computation at lab.sh:196-202 uses date -d "$created" +%s 2>/dev/null || echo 0; ts=0 always compares older-than-cutoff, so any run whose CreatedAt ever fails to parse gets force-removed regardless of REAP_MINUTES — the destructive default on a janitor path. For the record, the happy path is correct today: docker ps --format '{{.CreatedAt}}' yields 2026-09-24 08:30:17 +0200 CEST, which GNU date rejects verbatim but the script's cut -d' ' -f1-2 rescue works — I verified the exact pipeline read-only against the live tenant run and it would be spared at default age. Still, invert the fallback (continue on parse failure) so the janitor fails safe.

Smaller notes

• The RUNBOOK doesn't mention docker-compose.override.yml, which is a real mechanism in this tree: the lane scripts explicitly re-append it when present (run-keyset-rotation.sh:30-37, same in renewal_e2e.sh — the "docs(cloud-lab): document multi-branch lab traps (stale images, host ports) #436 trap"), and lab.sh's own comment references this host's hand-rolled 172.31.99.0/24 remap. lab.sh deliberately bypasses any dropped-in override (its explicit -f list), which is right, but a RUNBOOK line saying so would prevent mixed-usage surprise (lab.sh run vs ./run-keyset-rotation.sh diverging for override users). A prerequisites line (docker compose v2.24+ for !reset, socat, GNU date) would also help — socat is a new host-side dependency the docs don't name.
• usage() at lab.sh:53 prints lines 2–40, but the header ends at line 33 — the help text currently spills set -euo pipefail and the $0-absolutizing lines into its output. Trim the range to 2,33.
• The run-id slug isn't lowercased (git branch --show-current keeps case). Compose normalizes project names to lowercase, so an uppercase branch or username would make _reap_one's ${PROJECT}_ volume-prefix grep diverge from the real volume names on such checkouts. One tr 'A-Z' 'a-z' in the derivation closes it.

PR hygiene is clean: single commit with an accurate subject/body, no attribution footers, purely additive diff with no drive-by changes, and the PR body's claims each check out against the diff — including the verified-live table, which I independently reproduced (up/smoke/tap/down/reap) on this host. Overlap with open lane PRs (#522, #535, #425) is complementary, with the service-list coupling noted above. No security concerns beyond the tap exposure noted in finding 2.

Disposition: land-with-followups — fix the CHANGELOG PR number before merge (must); upstream-ext in the override loop and the tap bind-verification are small enough to land in this PR and I'd ask for them given the PR's whole point is shared-host safety; the rest are optional polish or RUNBOOK additions.

[Amperstrand]

First real mission found a bug: taps lacked fork, so socat served exactly one connection and exited — the second probe through a tap got connection-refused. Fixed on-branch (57864b5b), caught while running the #497 mint-replay experiment through the runner (which otherwise worked exactly as designed: run-scoped lab beside another session's, managed tap, clean down).

[felixfelix-bot]

Review — PR #557 at a15be70f — CHANGES REQUESTED

Reviewed at head a15be70ff3b414b0139d12f884e9c3c06ad494db (verified live via gh pr view 557 --json headRefOid), diffed against main (2796d96c): 23 files, +955/−2 — all of it src/tollwallet, src/cli, src/merchant wallet-recover code plus CHANGELOG/docs. Not one byte of cloud-lab test infrastructure.

The branch is not what this PR says it is.

1. [BLOCK] The advertised work is gone from the branch. This PR opened 2026-09-23 22:13Z as the run-scoped lab runner at 415986ae (+348/−0 across 4 files: tests/cloud-lab/lab.sh 223 lines, tests/cloud-lab/RUNBOOK.md 106 lines), then gained 57864b5b (the tap fork fix) as its tip. At 2026-09-24 20:40Z feat/cloud-lab-runner was force-pushed and neither commit is reachable from the branch anymore — neither file exists in the tree at head (git ls-tree -r a15be70f -- tests/cloud-lab/lab.sh → empty; the PR's file list has no cloud-lab entries at all). The title, the body, and the "Verified live" table all describe code that is not in this diff.

2. [BLOCK] The new head is a duplicate of open PR feat(cli): wallet recover — NUT-07 liveness check for journaled drain tokens #549. a15be70f is the head of feat/wallet-recover — PR feat(cli): wallet recover — NUT-07 liveness check for journaled drain tokens #549 (feat(cli): wallet recover — NUT-07 liveness check for journaled drain tokens), still open at the identical SHA, with the identical 9 commits. The CHANGELOG hunk in this very diff links the entry to feat(cli): wallet recover — NUT-07 liveness check for journaled drain tokens #549 (CHANGELOG.md:27, :35). Two open PRs at the same head is a merge hazard: whichever merges first lands these bytes and leaves the other carrying a stale title and body. The wallet-recover work has its home and its discussion trail on feat(cli): wallet recover — NUT-07 liveness check for journaled drain tokens #549 — including your self-review passes that fixed PENDING-as-spent and bounded checkstate against wedged mints (that head has since moved to a15be70f, so feat(cli): wallet recover — NUT-07 liveness check for journaled drain tokens #549 will need a re-review pass at the new tip regardless).

3. [INFO] No CI has run on this head — or anywhere in this repo since 2026-08-27. gh pr checks 557 → "no checks reported on the 'feat/cloud-lab-runner' branch"; the head commit has 0 check-runs, and the workflow history shows this is repo-wide, not specific to this PR (last test.yml run 2026-08-27; since 2026-09-01 only queued Dependency Graph runs). No CI ever ran on the original lab-runner commits either — your live evidence pass on test(cloud-lab): run-scoped lab runner + shared-host runbook #557 remains the only validation they got.

How to fix (pick one):

• Restore the branch — the lab-runner work is not lost, both commits are still in GitHub's object store. The pre-force-push tip was 57864b5b (includes the tap fork fix):

  git fetch origin 57864b5b5aa8de397c027e6b48a8de3b537ee9f3
  git push origin +57864b5b:feat/cloud-lab-runner
  

  This PR then becomes what its title says again, and I'll review the runner properly. For what it's worth, the timing — feat/wallet-recover, pr/wallet-recover, and feat/cloud-lab-runner all moving to a15be70f within ~2 minutes — reads like a mis-targeted push of the wallet branch onto the cloud-lab branch name, not a deliberate retarget.
• Or close this PR and let the wallet-recover bytes live only in feat(cli): wallet recover — NUT-07 liveness check for journaled drain tokens #549.

To be clear about the code that is here: the wallet-recover series itself looks sound — PENDING is treated as unknown, not spent (src/tollwallet/checkstate.go:78), checkstate is bounded against wedged mints (checkstate.go:27), and partial mint answers are rejected (checkstate.go:72). It will get its full review on #549. Carrying the same head on two PRs adds nothing but confusion.

[felixfelix-bot]

Review — PR #557 at a15be70f — CHANGES REQUESTED

Reviewed at head a15be70ff3b414b0139d12f884e9c3c06ad494db (verified live via gh pr view 557 --json headRefOid), diffed against main (2796d96c): 23 files, +955/−2 — all of it src/tollwallet, src/cli, src/merchant wallet-recover code plus CHANGELOG/docs. Not one byte of cloud-lab test infrastructure.

The branch is not what this PR says it is.

1. [BLOCK] The advertised work is gone from the branch. This PR opened 2026-09-23 22:13Z as the run-scoped lab runner at 415986ae (+348/−0 across 4 files: tests/cloud-lab/lab.sh 223 lines, tests/cloud-lab/RUNBOOK.md 106 lines), then gained 57864b5b (the tap fork fix) as its tip. At 2026-09-24 20:40Z feat/cloud-lab-runner was force-pushed and neither commit is reachable from the branch anymore — neither file exists in the tree at head (git ls-tree -r a15be70f -- tests/cloud-lab/lab.sh → empty; the PR's file list has no cloud-lab entries at all). The title, the body, and the "Verified live" table all describe code that is not in this diff.

2. [BLOCK] The new head is a duplicate of open PR #549. a15be70f is the head of feat/wallet-recover — PR #549 (feat(cli): wallet recover — NUT-07 liveness check for journaled drain tokens), still open at the identical SHA, with the identical 9 commits. The CHANGELOG hunk in this very diff links the entry to #549 (CHANGELOG.md:27, :35). Two open PRs at the same head is a merge hazard: whichever merges first lands these bytes and leaves the other carrying a stale title and body. The wallet-recover work has its home and its discussion trail on #549 — including your self-review passes that fixed PENDING-as-spent and bounded checkstate against wedged mints (that head has since moved to a15be70f, so #549 will need a re-review pass at the new tip regardless).

3. [INFO] No CI has run on this head — or anywhere in this repo since 2026-08-27. gh pr checks 557 → "no checks reported on the 'feat/cloud-lab-runner' branch"; the head commit has 0 check-runs, and the workflow history shows this is repo-wide, not specific to this PR (last test.yml run 2026-08-27; since 2026-09-01 only queued Dependency Graph runs). No CI ever ran on the original lab-runner commits either — your live evidence pass on #557 remains the only validation they got.

How to fix (pick one):

• Restore the branch — the lab-runner work is not lost, both commits are still in GitHub's object store. The pre-force-push tip was 57864b5b (includes the tap fork fix):

  git fetch origin 57864b5b5aa8de397c027e6b48a8de3b537ee9f3
  git push origin +57864b5b:feat/cloud-lab-runner
  

  This PR then becomes what its title says again, and I'll review the runner properly. For what it's worth, the timing — feat/wallet-recover, pr/wallet-recover, and feat/cloud-lab-runner all moving to a15be70f within ~2 minutes — reads like a mis-targeted push of the wallet branch onto the cloud-lab branch name, not a deliberate retarget.
• Or close this PR and let the wallet-recover bytes live only in #549.

To be clear about the code that is here: the wallet-recover series itself looks sound — PENDING is treated as unknown, not spent (src/tollwallet/checkstate.go:78), checkstate is bounded against wedged mints (checkstate.go:27), and partial mint answers are rejected (checkstate.go:72). It will get its full review on #549. Carrying the same head on two PRs adds nothing but confusion.

[Amperstrand]

Technical review (evidence pass) — PR head a15be70ff3b414b0139d12f884e9c3c06ad494db (base 21 commits behind main). This confirms, with independent evidence, what the 2026-09-25 review already requested changes for: the branch no longer contains the PR's subject matter. The head has not moved since 2026-09-24 20:38, so there is no delta to review — this is the same state, re-verified.

What the head actually is. feat/cloud-lab-runner and feat/wallet-recover (open PR #549) point at the same commit — a15be70f, identical 9-commit list on both PRs. Diffed against main, this PR is 23 files, +955/−2: all of it the wallet recover / NUT-07 CheckTokenSpendable stack (src/tollwallet/checkstate.go, src/cli/server.go, src/merchant/merchant.go, docs). git ls-tree -r at the head shows no tests/cloud-lab/lab.sh and no tests/cloud-lab/RUNBOOK.md — not one byte of cloud-lab infrastructure. The changelog entry the diff adds even links the content to #549 itself (CHANGELOG.md, lines 27–35 at head).

What happened, and the fix. The comment history pins it down: on 09-24 the branch was at 415986ae (the runner + runbook, +348 across lab.sh, RUNBOOK.md, tests/cloud-lab/README.md, CHANGELOG.md), then 57864b5b (the tap fork fix) — and between 08:47 and 20:38 that day it was force-pushed to the wallet-recover head. Both original commits are still on the remote (fetchable by SHA — I fetched them). To restore:

git push --force origin 57864b5b5aa8de397c027e6b48a8de3b537ee9f3:feat/cloud-lab-runner

That resurrects the real tip (lab.sh at 415986ae, RUNBOOK.md at 415986ae) — though note it forks from 13f9fcd4 and will need a rebase over the 21+ commits main has taken since. If the runner work is abandoned instead, close this PR and let #549 stand alone. Either way, do not fix this by pushing more wallet-recover commits here — that just deepens the duplication.

Consequences worth naming. Every claim in the PR body fails verification against this head — no up --wait, tap, reap, no runbook lessons; the "verified live" table cannot describe this content. As long as both PRs share a head, merging either makes the other unmergeable, and both are currently CONFLICTING with main (content conflict in CHANGELOG.md; the wallet-recover branch needs its own rebase on #549's side). Meanwhile dependent work is blocked: #569's crash-injection lane explicitly follows "the same isolation conventions as lab.sh" and cites RUNBOOK lesson #6 — it needs the real runner to land. Two smaller notes for when branches are sorted: the restored runner branch's changelog entry will hit the same CHANGELOG.md conflict and should be re-based consciously; and the golang.org/x/time v0.6.0 // indirect line in this diff's src/cli/go.mod is byte-identical to open #606, so #549's rebase should take that into account.

One gentle observation: the runbook's own lesson list names "wrong-worktree command execution" as a real incident on this host. The clobbering of this branch is that failure mode wearing a different hat — which is as good an argument as any for the runbook itself.

Disposition: request-changes. Blocking: restore feat/cloud-lab-runner to 57864b5b5aa8de397c027e6b48a8de3b537ee9f3 and force-push (or close this PR if the runner is abandoned). The wallet-recover content is #549's to carry. Happy to re-review the real runner content once the branch is back — the original 415986ae review (from this same thread) already flagged two substantive gaps and a must-fix there, so folding those in before re-requesting review will save a round trip.

[Amperstrand]

Branch restored and rebased (post-review maintenance). This branch had been force-pushed over with #549's wallet-recover series; the runner survived only as dangling commits. Restored from 57864b5b (the run-scoped runner + the lab.sh fork fix), rebased onto current main (afc86b3f) — both commits applied cleanly, bash -n tests/cloud-lab/lab.sh clean. Head is now 2f309274.

Still outstanding from the original 415986ae review round (unchanged by the restore): the two substantive gaps and the must-fix flagged there, plus this branch carries no CHANGELOG entry for the runner — add one under [Unreleased]/Changed / Internal with the #557 link before re-requesting review.

[felixfelix-bot]

Review — PR #557 at 2f309274bff0 — CHANGES REQUESTED

Reviewed at head 2f309274bff0f5c09dc0e88f5c15186d9b508cfa (verified live via gh pr view 557 --json headRefOid), diffed against main. The branch is its real content again — the wallet-recover clobber is resolved, the runner restored and rebased onto afc86b3f. Delta: 4 files, +348/−0 — tests/cloud-lab/lab.sh (223), RUNBOOK.md (106), README.md (+10), CHANGELOG.md (+9). Purely additive, test-only; no production code touched. shellcheck (0.10.0) and bash -n are clean.

The design is right, and the isolation core holds: one run = one compose project + container names + allocated /24, zero host ports, label-based reaping, taps as pidfile-managed resources. The findings below are the gap between "works on the happy path" and "safe on a shared host" — which is this PR's own stated bar.

[BLOCK] CHANGELOG.md:1058 — the entry links PR #551, not #557. Load-bearing at release-notes time; raised in the original round and still open. (Also a stray double blank line at :1059-1060.)

[RISK] lab.sh:61-66 — the run's "lease record" is never written; the state file holds only the subnet. init_state calls generate_override first (:62); its echo "subnet=…" >> "$META" at :96 creates $META. So by :64 the file exists, the [ -f "$META" ] || guard is skipped, and the lease block (run_id/project/cwd/started/user) is never written. Verified end-to-end (CLOUD_LAB_RUN_ID=verify557 … bash lab.sh ps): /tmp/…/meta contains exactly subnet=172.31.64.0/24, grep -c '^run_id=' → 0. The comment at :63 ("Lease record for humans: who, where, when — written once per run") and RUNBOOK.md:37 ("the run state (override, lease record, tap pidfiles)") both describe a record that does not exist. Fix: write the lease block before generate_override, or guard it on run_id rather than on file existence.

[RISK] lab.sh:78, :128 — lab.sh cannot select a compose profile, so the external-mints lane can't run through it and one host port survives. Two defects compound:

• upstream-ext (docker-compose.yml:161-187, profiles: ["external-mints"]) is missing from the override loop (mint mint-fees mint-rotate upstream reseller client), so it keeps ipv4_address: 172.28.0.12 — outside the run's 172.31.x.0/24 — and published: "21214", the one host-port binding this PR exists to strip. Confirmed by merging lab.sh's own override (COMPOSE_PROFILES=external-mints docker compose -f docker-compose.yml -f <lab.sh-override> config → 172.28.0.12, published: "21214"; the container then fails to come up because its IP isn't on the network). run-external-mints.sh still works on plain compose, so it's not a regression — but lab.sh's hardcoded -f list drops a --profile argument (compose only accepts --profile before the subcommand; via lab.sh up --profile … you get unknown flag: --profile). So the obvious fix — add upstream-ext to the loop — is not sufficient: profiles are reachable only through COMPOSE_PROFILES today. Derive the service list profile-aware (docker compose -f docker-compose.yml config --services returns just 4; --profile '*' returns all 7) and honour COMPOSE_PROFILES, and both problems close.

[RISK] lab.sh:183-186 — tap reports success when the port is taken, and binds on every host interface. fork (the tip commit) fixed the one-connection bug — good catch. But a socat that dies instantly (E bind(…): Address already in use) still leaves a pidfile at a dead pid while the command prints tap: … (no post-spawn liveness check), and the next curl on that port reaches the neighbouring session's service through its tap — exactly the wrong-target confusion this runbook is about. Separately, TCP-LISTEN:$local_port (no bind=) exposes every tap on 0.0.0.0; these are unauthenticated mints and portals. Fix: poll ss -ltn/kill -0 for ~1s after spawning and die loudly if the listener never came up; add bind=127.0.0.1.

[RISK] lab.sh:117-126 — the subnet allocator decides at /24 granularity from a 3-octet grep. grep -oE '172\.31\.[0-9]+' reduces every live network to its third octet, and the loop treats an octet as used if any 172.31.N appeared. Correct only if every live 172.31.x network is exactly a /24: a live 172.31.64.0/23 reports only 172.31.64, so the allocator hands out 172.31.65.0/24 — overlapping the network it exists to avoid (printf '172.31.64.0/23' | grep -oE … → 172.31.64). Parse the full subnet/len and compare overlaps.

[NIT] lab.sh:53 — usage() prints 2,40p; the header ends at :33. The help spills script body into its output (observed: bash lab.sh ends with set -euo pipefail and the SELF=/SCRIPT_DIR=/cd lines). Trim to 2,33.

[NIT] lab.sh:30 — the header's default run-id doc contradicts the implementation. It advertises <user>-<branch-slug>-<timestamp> "(stable per checkout+day is fine)", but :47 has no timestamp and appends an md5sum of the toplevel path — stable per checkout indefinitely. The doc undercuts the very stability the PR is selling.

[NIT] lab.sh:47 / :215 — the run-id slug isn't lowercased. git branch --show-current keeps case while compose lowercases project names, so on an uppercase branch or user the ${PROJECT}_ volume-prefix reap at :215 diverges from the real (lowercase) volume names. One tr 'A-Z' 'a-z'.

[NIT] lab.sh:199 — reap --all fails toward destruction on an unparseable timestamp. date -d "$created" … || echo 0 makes ts=0, always older-than-cutoff, so any run whose CreatedAt ever fails the cut -d' ' -f1-2 rescue is force-removed regardless of REAP_MINUTES. It's latent on this host (date -d accepts the truncated form), but this host's date is uutils coreutils, not GNU, and the truncation drops the timezone offset — a janitor path should fail safe (continue on parse failure).

[NIT] RUNBOOK.md — no prerequisites line (compose v2.24+ for !reset, host socat, and a date that parses the truncated CreatedAt are all hard dependencies now), and no note that lab.sh deliberately ignores a dropped-in docker-compose.override.yml (its explicit -f list), so lab.sh run and ./run-keyset-rotation.sh can diverge for override users.

[INFO] Branch state. Head is 9 commits behind main (rebased onto afc86b3f; origin/main is now 74cd6f99), and no CI runs on the branch (gh pr checks → "no checks reported on the 'feat/cloud-lab-runner' branch"). Rebase before merge — CHANGELOG.md is the conflict. The original round's live evidence (up / smoke / tap / down / reap, three concurrent labs) remains the only validation this runner has had; I did not re-run it here — starting a live lab on this shared host isn't mine to do mid-review. Everything above is from read-only checks: shellcheck, bash -n, docker compose config merges, the real lab.sh state-bootstrap (with a scratch CLOUD_LAB_STATE), and the usage() output.

Disposition: CHANGES REQUESTED — the wrong-PR CHANGELOG link and the profile/upstream-ext gap block the lane the PR advertises; the tap false-success/exposure, the lost lease record, and the allocator overlap are should-fix against this PR's own shared-host bar; the rest is polish.

[Amperstrand]

Review-round fixes applied (the 415986a round's must-fix + two substantive gaps + its explicit one-liners), rebased onto current main (74cd6f99). New head: 4ef118c4 (series: aa89579e runner → 851267a9 tap fork → 4ef118c4 fixes).

Must-fix — CHANGELOG linked the wrong PR. The runner entry now links #557 (was #551) and sits under [Unreleased]/Changed / Internal next to the entries it will release with (it had been appended at the tail of the section, with a stray double blank). The full CHANGELOG diff vs main is now exactly that one +9-line entry; the pre-existing #551 entry at :816 (admin-board credential work, a different PR) is untouched.

Gap 1 — upstream-ext joined the override loop (lab.sh): rename/ports/labels/IP remap now covers it, with its own static octet (.12, mirroring the base file's 172.28.0.12). Verified by generating the override through lab.sh's real bootstrap (scratch CLOUD_LAB_STATE, read-only docker network inspect) and merging: COMPOSE_PROFILES=external-mints docker compose -f docker-compose.yml -f <override> config → container_name: cl-<run>-upstream-ext, ipv4_address: 172.31.<run-N>.12 (inside the run's allocated /24), zero published ports anywhere in the merged config; without the env var the service stays inactive and the merge is clean, so the default topology is unchanged.

Gap 2 — tap no longer reports success when its port is taken, and no longer binds on every interface. socat now binds 127.0.0.1 only (taps front unauthenticated mints/portals), and after spawning the script polls (~1s bound) for a listener owned by its own pid — a bare port grep would be satisfied by the neighbour whose tap caused the bind failure — and dies loudly, killing the socat and removing the pidfile otherwise. Verified by driving the shipped tap block both ways: free port → exit 0, listener on 127.0.0.1:<port> owned by the pidfile pid; port pre-taken by a neighbour socat → E bind(...): Address already in use in the log, loud die, exit 1, pidfile gone, neighbour untouched.

The round's one-liners: usage() trimmed to 2,33 (output verified to end at the Exit-codes line, no script-body spill); the run id is lowercased to match compose's project-name normalization (CLOUD_LAB_RUN_ID=Fix557-UPPER verified to produce project cl-fix557-upper in the override, so _reap_one's ${PROJECT}_ volume grep can't diverge); reap --all now continues on an unparseable CreatedAt instead of computing ts=0 and force-removing.

Verification: bash -n tests/cloud-lab/lab.sh clean; shellcheck tests/cloud-lab/lab.sh clean; docker compose config merges above; tap positive/negative as above. No Go surface touched, so no Go battery run.

Not addressed (out of this round's scope, for the next pass): the 2026-10-01 review's lease-record bootstrap issue, profile-aware service derivation beyond COMPOSE_PROFILES (e.g. a --profile passthrough), the /23 allocator overlap, the run-id header doc's <timestamp> wording, and the RUNBOOK prerequisites/override-note additions the original round filed as optional polish.