Thermonuclear Review
A review harness for velo changes that assumes the diff is guilty until proven
innocent. It is not a style pass — it exists to catch the bug classes that
have actually shipped breakage in this repo (dual-crate semver skew, drain
races, FFI lifetime bugs) plus the ones RDMA work newly introduces.
Run it on: the current branch's diff against origin/main (default), or an
explicit PR number / commit range given in the arguments.
Resolve the target once, before Stage 0, and pass it to every agent. Set
REVIEW_RANGE from the invocation arguments — a commit range verbatim
(443d965..HEAD), a PR number via gh pr view <n> --json baseRefName,headRefName
(then <base>...<head>), or origin/main...HEAD when no argument was given. A
working-tree review is the one case with no range: commit first (see Ground
rules), then use the resulting range. Every finder prompt must carry the
resolved range; a finder left to assume origin/main...HEAD reviews the wrong
diff and reports a false pass.
Ground rules (violations invalidate the review)
- Commit the code under review before starting. A review of an
uncommitted working tree has no pinned baseline: an agent edit silently
corrupts every later agent's reading, and there is no diff to restore from.
- Finders and verifiers are strictly read-only on the repo. They report;
they never Edit/Write source, not even "temporarily". State this in every
agent prompt.
- Mutation testing is allowed only in an isolated copy (spawn with
isolation: "worktree", or copy the files to scratch and build there).
A mutation applied to the live tree — even if reverted — taints every
verdict produced in the window, and a missed revert plants a real bug.
(This rule exists because it happened: a reviewer's unreverted ||→&&
mutation was then "found" by another reviewer as a bug in the diff.)
- After the review completes,
git status must be clean relative to the
pinned commit. If it isn't, find out which agent edited what before
trusting anything.
Stage 0 — Mechanical gates (run first, fail fast)
Run these directly (not via agents). Any failure is a finding of severity
blocker and the review continues so the report is complete:
cargo fmt --check
cargo clippy --all-features --no-deps --all-targets -- -D warnings
cargo machete
scripts/check-semver.sh # if the diff touches lib/velo-ext or lib/velo
cargo test --all-features --all-targets # note: nats/etcd tests need local servers; skip-and-report if absent
Also verify the boundary invariant when lib/velo-ext is touched:
cargo tree -p velo-ext | grep -c prometheus must print 0.
Stage 1 — Fan-out (Workflow tool, sonnet finders)
Launch a Workflow. One finder agent per dimension, each given the resolved
REVIEW_RANGE from above (have each agent run git diff $REVIEW_RANGE itself
plus read full files it flags — never a hardcoded range). Dimensions — drop
any that cannot apply to the diff, add ad-hoc ones the diff suggests:
- correctness — logic errors, off-by-one, error paths, races between
check and use, lost wakeups, TOCTOU on DashMap entries.
- concurrency & atomics — Ordering choices (this repo documents SeqCst
litmus reasoning in the shutdown module — hold new code to that bar),
lock-across-await, progress-thread ownership violations (any
ucp_* worker
call off the progress thread is a finding), Doorbell arm/disarm races.
- unsafe & FFI — every
unsafe block gets a SAFETY comment audit: is the
stated invariant actually upheld at every call site? Completion-owned Arc
discipline (exactly one reclaim per posted op, all three *_nbx exits),
callback unwind guards, pointer lifetimes into UCX (who holds the buffer
until completion? who frees requests?), use-after-unmap/dereg windows.
- wire & protocol compatibility — rendezvous protocol structs, transport
address blobs (BLOB_VERSION bumps), AM id space, serde format changes
(serde_json vs rmp), old-peer/new-peer matrix: can a new node talk to an
old node in BOTH directions for every touched message?
- semver & workspace boundary — anything new in velo-ext? Are new trait
methods default-implemented? Does the
= pin still track? Publishable
crate set unchanged? Feature-gating: does the crate build with
--no-default-features, with each new feature alone, and with
--all-features? (Actually run the builds.)
- shutdown & drain contract — new inbound paths route through
TransportAdapter::admit_message; no bare is_draining() gates; new
resources (registrations, EPs, leases) have a teardown order and a bounded
drain; nothing can wedge wait_for_drain.
- resource lifecycle & leaks — every register has a deregister on every
path (incl. error paths and panics), RAII guards can't double-free or leak
under racing drop/shutdown, caches have bounded growth and their evictions
can't free memory still referenced by in-flight ops.
- test adequacy — for each behavior the diff claims, is there a test that
would fail if the behavior regressed? Deliberately break something small in
your head and check a test catches it. Flag untested error paths.
Each finder returns findings as {file, line, claim, why-it-breaks, severity}.
Stage 2 — Adversarial verification (opus)
Every finding goes to a verifier agent prompted to refute it: read the
real code (not the finder's summary), construct the concrete failure scenario
or prove it impossible. Findings that survive get CONFIRMED; refuted ones
are dropped with the refutation recorded. Verify perspective-diverse for
severity ≥ major: one correctness lens, one "does it actually reproduce"
lens. A finding needs a concrete input/state sequence, not vibes.
Stage 3 — Loop until dry
Feed confirmed findings back: one more finder round (fresh agents, told what
was already found) across the dimensions that produced hits. Stop when a
round produces zero new confirmed findings, or after 3 rounds.
Stage 4 — Report
Report with ReportFindings if the harness asks for typed findings; otherwise
as markdown: confirmed findings ranked blocker → major → minor, each with
file:line, the failure scenario, and a suggested fix direction. Then the
refuted-findings appendix (one line each). State explicitly which Stage-0
gates ran, which were skipped and why. Do not auto-apply fixes — the caller
decides what gets fixed, then re-runs the affected dimensions.
Calibration
- Finders: sonnet. Verifiers and any judgment call: opus. (User preference:
Fable only for synthesis/judgment, not bulk work.)
- Silence is a valid outcome: "0 confirmed findings" with the refutation
appendix is a pass, not a failure to try hard enough.
- Never soften a blocker because the fix is annoying. Never report a style
nit as a finding — clippy owns style.
1---2name: thermonuclear-review3description: Maximum-intensity multi-agent review of a velo diff/branch before it becomes a PR. Fans out repo-specific finder dimensions (correctness, concurrency, FFI/unsafe, wire-compat, semver boundary, shutdown contract), adversarially verifies every finding, loops until dry, then reports confirmed findings ranked by severity. Use after each implementation phase, or when the user says "thermonuclear review".4---56# Thermonuclear Review78A review harness for velo changes that assumes the diff is guilty until proven9innocent. It is *not* a style pass — it exists to catch the bug classes that10have actually shipped breakage in this repo (dual-crate semver skew, drain11races, FFI lifetime bugs) plus the ones RDMA work newly introduces.1213Run it on: the current branch's diff against `origin/main` (default), or an14explicit PR number / commit range given in the arguments.1516**Resolve the target once, before Stage 0, and pass it to every agent.** Set17`REVIEW_RANGE` from the invocation arguments — a commit range verbatim18(`443d965..HEAD`), a PR number via `gh pr view <n> --json baseRefName,headRefName`19(then `<base>...<head>`), or `origin/main...HEAD` when no argument was given. A20working-tree review is the one case with no range: commit first (see Ground21rules), then use the resulting range. Every finder prompt must carry the22resolved range; a finder left to assume `origin/main...HEAD` reviews the wrong23diff and reports a false pass.2425## Ground rules (violations invalidate the review)2627- **Commit the code under review before starting.** A review of an28 uncommitted working tree has no pinned baseline: an agent edit silently29 corrupts every later agent's reading, and there is no diff to restore from.30- **Finders and verifiers are strictly read-only on the repo.** They report;31 they never Edit/Write source, not even "temporarily". State this in every32 agent prompt.33- **Mutation testing is allowed only in an isolated copy** (spawn with34 `isolation: "worktree"`, or copy the files to scratch and build there).35 A mutation applied to the live tree — even if reverted — taints every36 verdict produced in the window, and a missed revert plants a real bug.37 (This rule exists because it happened: a reviewer's unreverted `||`→`&&`38 mutation was then "found" by another reviewer as a bug in the diff.)39- After the review completes, `git status` must be clean relative to the40 pinned commit. If it isn't, find out which agent edited what before41 trusting anything.4243## Stage 0 — Mechanical gates (run first, fail fast)4445Run these directly (not via agents). Any failure is a finding of severity46`blocker` and the review continues so the report is complete:4748```shell49cargo fmt --check50cargo clippy --all-features --no-deps --all-targets -- -D warnings51cargo machete52scripts/check-semver.sh # if the diff touches lib/velo-ext or lib/velo53cargo test --all-features --all-targets # note: nats/etcd tests need local servers; skip-and-report if absent54```5556Also verify the boundary invariant when `lib/velo-ext` is touched:57`cargo tree -p velo-ext | grep -c prometheus` must print `0`.5859## Stage 1 — Fan-out (Workflow tool, sonnet finders)6061Launch a Workflow. One finder agent per dimension, each given the resolved62`REVIEW_RANGE` from above (have each agent run `git diff $REVIEW_RANGE` itself63plus read full files it flags — never a hardcoded range). Dimensions — drop64any that cannot apply to the diff, add ad-hoc ones the diff suggests:65661. **correctness** — logic errors, off-by-one, error paths, races between67 check and use, lost wakeups, TOCTOU on DashMap entries.682. **concurrency & atomics** — Ordering choices (this repo documents SeqCst69 litmus reasoning in the shutdown module — hold new code to that bar),70 lock-across-await, progress-thread ownership violations (any `ucp_*` worker71 call off the progress thread is a finding), Doorbell arm/disarm races.723. **unsafe & FFI** — every `unsafe` block gets a SAFETY comment audit: is the73 stated invariant actually upheld at every call site? Completion-owned Arc74 discipline (exactly one reclaim per posted op, all three `*_nbx` exits),75 callback unwind guards, pointer lifetimes into UCX (who holds the buffer76 until completion? who frees requests?), use-after-unmap/dereg windows.774. **wire & protocol compatibility** — rendezvous protocol structs, transport78 address blobs (BLOB_VERSION bumps), AM id space, serde format changes79 (serde_json vs rmp), old-peer/new-peer matrix: can a new node talk to an80 old node in BOTH directions for every touched message?815. **semver & workspace boundary** — anything new in velo-ext? Are new trait82 methods default-implemented? Does the `=` pin still track? Publishable83 crate set unchanged? Feature-gating: does the crate build with84 `--no-default-features`, with each new feature alone, and with85 `--all-features`? (Actually run the builds.)866. **shutdown & drain contract** — new inbound paths route through87 `TransportAdapter::admit_message`; no bare `is_draining()` gates; new88 resources (registrations, EPs, leases) have a teardown order and a bounded89 drain; nothing can wedge `wait_for_drain`.907. **resource lifecycle & leaks** — every register has a deregister on every91 path (incl. error paths and panics), RAII guards can't double-free or leak92 under racing drop/shutdown, caches have bounded growth and their evictions93 can't free memory still referenced by in-flight ops.948. **test adequacy** — for each behavior the diff claims, is there a test that95 would fail if the behavior regressed? Deliberately break something small in96 your head and check a test catches it. Flag untested error paths.9798Each finder returns findings as `{file, line, claim, why-it-breaks, severity}`.99100## Stage 2 — Adversarial verification (opus)101102Every finding goes to a verifier agent prompted to **refute** it: read the103real code (not the finder's summary), construct the concrete failure scenario104or prove it impossible. Findings that survive get `CONFIRMED`; refuted ones105are dropped with the refutation recorded. Verify perspective-diverse for106severity ≥ major: one correctness lens, one "does it actually reproduce"107lens. A finding needs a concrete input/state sequence, not vibes.108109## Stage 3 — Loop until dry110111Feed confirmed findings back: one more finder round (fresh agents, told what112was already found) across the dimensions that produced hits. Stop when a113round produces zero new confirmed findings, or after 3 rounds.114115## Stage 4 — Report116117Report with ReportFindings if the harness asks for typed findings; otherwise118as markdown: confirmed findings ranked blocker → major → minor, each with119`file:line`, the failure scenario, and a suggested fix direction. Then the120refuted-findings appendix (one line each). State explicitly which Stage-0121gates ran, which were skipped and why. Do not auto-apply fixes — the caller122decides what gets fixed, then re-runs the affected dimensions.123124## Calibration125126- Finders: sonnet. Verifiers and any judgment call: opus. (User preference:127 Fable only for synthesis/judgment, not bulk work.)128- Silence is a valid outcome: "0 confirmed findings" with the refutation129 appendix is a pass, not a failure to try hard enough.130- Never soften a blocker because the fix is annoying. Never report a style131 nit as a finding — clippy owns style.
ai-dynamo