feat(review): add runtime-axis, table-sweep and isolation witness forms

packages/cli/src/commands/review/lib/agent-briefs.ts

@@ -647,6 +647,8 @@ For each finding you were given:
 
 **When the defect is mechanically enumerable, sweep the real population — the count is the verdict.** For a claim about a pattern, a predicate, or a parser ("this misclassifies X", "this mishandles shape Y"), do not stop at the one reported instance: run the check over every real instance this repo holds (every workflow step body, every call site, every input the code will actually see) and report the count. "195 of 434 real \`run:\` bodies reach this path" confirms the finding, sizes its severity, and hands the author a number they can re-run rather than argue with — and a count of **zero** is the quoted contradiction that rejects it. Two rules keep a sweep evidence rather than theatre: its oracle must be an **external authority** — the real parser, the real tool, \`bash -n\` — never your own reimplementation of the logic under test, because a mirror shares the blind spots of what it mirrors and mirrored sweeps have manufactured false findings out of their own bugs; and spot-check one hit by reading it before you quote a nonzero count.
 
+**The commonest sweepable thing in a diff is a hardcoded table of another system's namespace** — heap-space names, error codes, MIME types, status codes, locales, a runtime's own enums. It reads as data rather than logic, so it invites being checked by eye against a list you retype; a retyped list is a mirror of the thing under test, which the oracle rule above already rejects, and it is the mirror you are most likely to type correctly and therefore believe. **Parse the literal out of the source** and take the set difference against the authority at runtime — the real enum, the real registry, the real API call. Both directions are findings and they are not the same finding: a name the table has and the authority does not is dead weight; a name the **authority** has and the table does not is an under-count, which is the direction that ships and the one no test written against the table can see, because the table is what those tests enumerate.
+
 **A suggested fix you did not run is a hypothesis; say which one you are giving.** When a finding's fix is cheap to apply, patch it in, re-run the same probe/harness to show it works, then revert — and state that every other number in your report comes from the unmodified PR (the contamination line is what lets a reader trust the rest). A fix too costly to verify is still worth proposing, labeled untested.
 
 **A probabilistic failure gets a RATE, not an anecdote.** For a timing/race claim, run N repetitions per arm and report the rates as the verdict; amplify with full CPU load to force the window open (a live case went from 4/11 idle to 5/5 loaded). And attribute honestly: a lower idle rate with no structural change is luck, not a fix. Fake-timer tests hardcode one ordering by construction — they cannot discriminate a race, so a green fake-timer suite is non-evidence here.
@@ -671,6 +673,8 @@ It builds the merge base in a sibling worktree and reports \`available\` and \`p
 
 A finding an A/B settled carries \`Source: [probe]\` like any other run-produced evidence, with both sides' output quoted. **Do not remove the base tree** — \`cleanup\` sweeps it at the end of the review, and a later finding may need it.
 
+**When the claim is about a version you are not running, fetch that version — the A/B's other axis is not always git.** "This handles the next major", "this survives the runtime's next release", "this parses the format's new field" cannot be settled on the one runtime, dependency or format version the harness happens to have, and a green CI does not settle it either: a matrix is evidence about the versions in the matrix. Install the version the claim names, run the **smallest discriminator** on both — often one expression, not a suite — and quote both outputs. Measured: a heap-space classifier written against the eleven space names Node 22 reports covers them with nothing left over, and silently drops the two more that Node 24 adds. This is cheap where \`base-tree\` is not — a download and one \`-e\`, no dependency install and no build — so keep it to the versions the claim itself names (a claim about a support range names its floor and its newest, which is two runs, not a matrix), and a version you cannot fetch is \`witness: not run — <why>\` like any other unreachable claim.
+
 **When the claim is about what a WORKFLOW does, run the step — do not read the YAML.** A finding against a CI workflow ("this step posts the wrong body", "the sanitizer is bypassed on this path", "this only changed a log line") is a claim about a shell script that happens to live inside YAML, and reading it in place is where workflow review goes wrong quietly: the \`run:\` body is indented inside a block scalar, the \`env:\` that decides its behaviour is spread over three levels (workflow, job, step — nearest wins, and two of them are nowhere near the step), and every \`\${{ … }}\` is a hole the reader fills in from imagination. Lift it out instead:
 
 \`\`\`bash
@@ -696,6 +700,8 @@ until [ -s <plan dir>/mock.json ]; do sleep 0.1; done  # its port is in that rep
 
 For anything that is not one of those two wires — the project's own HTTP service, an MCP server, an OAuth endpoint — stand it up yourself and let \`drive\` own the lifecycle.
 
+**When the observable is an AGGREGATE, change the population rather than instrumenting the reader.** A total, a maximum across children, a count over a fleet — a claim about how one of those combines cannot be settled from a single reading, and the obvious repair (add a per-component dump and read that) costs the verdict its standing, because the numbers then come out of a build you edited. Shrink the contributing population instead: read the aggregate with every contributor live, remove exactly one — kill the process, unregister the workspace, drop the feed — and read it again. Both numbers come from unmodified code, and what they mean depends on the combining rule you are testing for: doubled with the population is a sum, flat is not a sum, and reducing the population to a single contributor makes the reading that contributor's own value outright. Only for a sum is the **difference** a contributor's value; under a maximum, removing a non-holder moves nothing and removing the holder exposes the next-largest. Identify what you removed by something the product did not choose for you — a process's own working directory, its port, its registered id — because removing the one you assumed answers a different question than the one asked.
+
 **When the claim is about GITHUB's behaviour, neither tree can settle it — only GitHub can.** A claim like "this encoding renders identically and can never ping", "GitHub strips this tag", "this markdown shape closes the fold" is about the comment pipeline's parser, sanitizer allowlist and notification path, none of which exist in this environment — a local markdown library is a model of GitHub, and judging a sanitizer claim against a model of the authority is exactly the parser-divergence failure under review. Measured live: an \`@\` → \`&#64;\` defusal read as sound in every local trace, and GitHub's real renderer registered the mention and fired the notification. So:
 
 - **If the environment variable \`QWEN_REVIEW_SCRATCH_REPO\` is set** (an \`owner/repo\` the user designated for disposable test posts), you may adjudicate on the real renderer: post the payload as an issue comment there — \`gh api repos/$QWEN_REVIEW_SCRATCH_REPO/issues/<n>/comments -f body=@<file>\` against an issue you created there for this purpose — read it back with \`-H "Accept: application/vnd.github.html+json"\`, and rule on the returned HTML (and, for mention claims, the timeline events). The observation is the verdict; quote it. This is the ONLY write destination other than \`submit\`'s that any part of this review may touch, it is user-designated, and nothing about the PR under review, its code, or its authors may appear in what you post there — post the minimal payload shape, not the report.
@@ -707,7 +713,7 @@ Return, for each finding, one verdict:
 - **confirmed (low confidence)** — the mechanism is real but the trigger is uncertain (timing, environment, configuration). Say what would confirm it. Carry the severity.
 - **rejected** — the code does not do what the finding claims (**quote the contradicting code**), or it matches an Exclusion Criterion (one-line reason).
 
-**A confirmed Critical returns its witness.** Alongside the verdict, include a \`witness:\` line quoting the observed output that settled it — the probe's two sides, the A/B's \`BASE:\`/\`PR:\` pair, the extracted step's run, the sweep count — trimmed to the deciding lines. When every run-capability above is genuinely inapplicable and the confirmation rests on the trace alone, write the one line \`witness: not run — <why no run could settle this claim>\` instead; writing that line is also the moment you notice when the claim was runnable after all. This is mechanical downstream — enforced in code at the findings canonicalization, not merely by the orchestrator's read of its rules: a confirmed Critical returning neither the witness nor the reason line is filed at **low confidence** — terminal-only, never posted — whatever your prose argued, because the evidence a run produced is the one part of a Critical its author can act on without re-deriving the bug.
+**A confirmed Critical returns its witness.** Alongside the verdict, include a \`witness:\` line quoting the observed output that settled it — the probe's two sides, the A/B's \`BASE:\`/\`PR:\` pair, the extracted step's run, the sweep count, the two versions' outputs, the table's set difference, the two readings either side of a removal — trimmed to the deciding lines. When every run-capability above is genuinely inapplicable and the confirmation rests on the trace alone, write the one line \`witness: not run — <why no run could settle this claim>\` instead; writing that line is also the moment you notice when the claim was runnable after all. This is mechanical downstream — enforced in code at the findings canonicalization, not merely by the orchestrator's read of its rules: a confirmed Critical returning neither the witness nor the reason line is filed at **low confidence** — terminal-only, never posted — whatever your prose argued, because the evidence a run produced is the one part of a Critical its author can act on without re-deriving the bug.
 
 **Rejecting a Critical carries a higher bar than anything else, and it is one-way.** A rejected Critical is gone — no later stage revisits it, it vanishes from both the pull request and the terminal. To reject one you must **quote the specific code that contradicts the claim**. A passing test, a plausible-looking guard, or "I could not reproduce the reasoning" is not enough — when you cannot quote the contradiction, the floor is \`confirmed (low confidence)\`, never rejection. Downgrading is reversible; a human still sees a low-confidence finding under "Needs Human Review". Rejection is not.
 

packages/core/src/skills/bundled/review/DESIGN.md

@@ -459,6 +459,28 @@ Three details of that mechanical half are worth naming, because each was a way t
 - **A `shell:` declared as `bash` is not the runner's default `bash`.** The default is `bash -e {0}`; declaring `shell: bash` (at any level) makes it `bash --noprofile --norc -eo pipefail {0}`. A pipeline whose middle stage fails aborts under one and not the other, so the emitted header carries `set -eo pipefail` or `set -e` accordingly — a distinction that decides whether the extraction measures the same script the runner ran.
 - **The header must be inert, line by line.** A `env:` value can be a YAML block scalar; commenting only the entry's first line left its continuation lines in command position, and under the header's own `set -e` the step died in its preamble before its body ran. Four steps in this repo produced exactly that. The test oracle asserts the property directly — the file is the header plus the body verbatim, and every line before the body is a comment or a named directive — rather than filtering the output for lines that look executable, a filter that could not tell the header's `set -e` from one the body legitimately contains.
 
+## Why the version axis is not only git
+
+`base-tree` answers "and what did it do before", which is the git axis, and for two years that was the only axis this pipeline had. It leaves a whole class of claim unfalsifiable: the code is asserted to handle a version of something it does not ship — the runtime it will be executed by, a dependency's next major, a wire format that will gain a field. Nothing in the diff, the merge base, or the test suite can settle that, because every one of them runs on the version the harness already has. CI does not settle it either, and the way it fails to is the trap: a green matrix reads as "this works" when it only ever said "this works on the versions in the matrix".
+
+The measurement that made this concrete cost one download. Node 22 reports 11 heap spaces, eight of them old-generation, so an eight-name old-generation set classifies every space that runtime exposes with nothing left over — and would have been reported as complete on that evidence alone. Node 24 reports 13: the two extra names, `shared_trusted_space` and `shared_trusted_large_object_space`, are both old-generation, and are exactly the kind a set written against the runtime in hand does not contain. Whether a given set covers them is a one-line question — `v8.getHeapSpaceStatistics()` on both — and unanswerable by reading.
+
+So the capability is deliberately small. One other version, named by the claim rather than swept; the smallest discriminator rather than the suite; both outputs quoted. The cost asymmetry is the point — a runtime download and one `-e` against `base-tree`'s install-and-build — and it is also the reason to cap it: a capability this cheap invites a matrix, and a matrix spends the review's budget answering versions nobody asked about. An unfetchable version stays `witness: not run`, exactly like infrastructure the harness lacks, because a claim the harness cannot reach is a fact about the harness.
+
+## Why the table sweep is called out separately from the impact sweep
+
+The impact sweep's oracle rule — an external authority, never a reimplementation — was written for parsers and predicates, where the temptation to mirror is obvious enough that naming it is usually enough. The hardcoded enumeration is the same failure wearing a disguise: a table of another system's namespace (heap spaces, error codes, MIME types, status codes, locales) is small, legible, and looks like data rather than logic, so a verifier checks it by reading it against a list they type out — and a hand-typed list is a mirror, with the additional hazard that a table short enough to retype accurately is a table one feels confident about.
+
+Two properties make the check worth naming rather than leaving to the general rule. First, the literal is already in the diff, so the sweep's population comes free: parse the table out of the source, take the set difference against the authority at runtime, done. Second, the two directions are different findings with different severities, which a "does the table look right" reading collapses. A name the table has and the authority does not is dead weight. A name the **authority** has and the table does not is an under-count — and an under-count is invisible to every test written against the table, because the table is what the test enumerates. That asymmetry is the whole reason the check pays: the failure it catches is the one the diff's own tests cannot.
+
+## Why an aggregate is verified by removing contributors, not by instrumenting the reader
+
+A claim about an aggregate — this total is a maximum and not a sum; this field is per-child and not fleet-wide; a child that reports nothing is skipped rather than counted as zero — cannot be settled from the aggregate alone, and the obvious repair is to add a per-component dump. That repair costs the verdict its standing. The numbers then come out of a build the review edited, and the one thing a witness is for is being a run of the code under review rather than a run of the reviewer's version of it.
+
+Removing a contributor inverts the dependency. The reader is untouched, the population changes, and both readings come from unmodified code. The delta is the removed contributor's own value only where the rule is a sum; the general form is weaker and more useful — how the aggregate responds to a population it did not choose is the combining rule itself, and a population reduced to one contributor reads that contributor outright. It is also usually available — processes can be killed, workspaces unregistered, feeds dropped — and it produces the two numbers a sum-versus-maximum question needs without any per-component surface existing at all. A daemon holding two ACP children answered exactly that: summed RSS moved 193.6 → 377.5 MB across the two while the reported heap peak moved 103.5 → 103.7 MB. That pair excludes a sum — a sum would have moved with the RSS — and no more than that on its own; a fleet-wide maximum, a per-component reading following one child, and the parent's own heap all reproduce it. Which of those it is takes the second removal: take the population down to one contributor and the reading becomes that contributor's own value, or does not.
+
+The one discipline it needs is identifying what you removed by something the product did not choose for you. Two children of the same daemon are indistinguishable in `ps`; removing "the first one" and reporting the remainder as the second's value answers a question nobody asked. A working directory, a port, a registered id — anything the harness can read back and name — turns the removal into a measurement.
+
 ## Why the round-3 lenses are prose, not detectors
 
 Seven lenses joined the briefs from one verification round, and each is a judgment with a crisp trigger rather than a decidable predicate — which is what separates a brief lens from a subcommand here: