docs(review-checklist): rules 27-32 — evidence instruments, rendered strings, and merge verification

docs/development/review-checklist.md

@@ -87,3 +87,19 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b
 ## Defaults derived for a consumer
 
 26. **A default you derive for a consumer must be one that consumer *accepts* — so run the derived value through the consumer's own guard, not through a shape assertion. And the set that *consumes* a thing is not the set that can *enforce* it.** The two capability sets get conflated the moment a default is chosen from one of them: "reads `mcp[]`" is not "can confine a process", and an adapter can sit in the first while being unable to honour the second. The wrong pick then fails in one of two directions, and the quieter failure is not the safer one. A declaration that **engages nothing** is silent — the record looks configured, the seat runs unconfined (rule 7's shape, arriving at a security posture). A declaration the consumer **must refuse** is fatal and equally silent from the writer's side: the daemon writes the record, the spawn throws, and the stored row looks correct the whole time, so every derived seat is unspawnable while the board says it is installed. Assert acceptance by importing the consumer's guard and running the derived value through it — *the spec this adapter receives is one this adapter accepts* — because `expect(derived).not.toHaveProperty('sandbox')` passes on exactly the state that breaks the seat. Mutation is the proof and it is cheap here: revert the predicate and exactly the paired tests should go red. Rider, since a default is a declaration made on someone else's behalf: when the consumer cannot honour it, the choice is a posture decision rather than a default — state the residual and raise it, do not let a default settle it. *(Earned: #1754/TASK-052, 2026-09-18 — four instances of one family in one PR, all found by running the consumer rather than inspecting the serializer. The predicate had been `ADAPTERS_WITH_DEFAULT_MCP` — the set that consumes `mcp[]` — where the question was which adapters can enforce a sandbox. Claude's two consumers read one declaration independently and disagreed at runtime; codex's `got unset` throw would have made every derived codex seat unspawnable; pi's `assertNoSandboxDeclared` made every derived **pi** seat unspawnable, and two fixtures were pinning that broken shape green; and the doc comment this PR wrote beside the constant asserted the opposite mechanism (it is in that PR's diff, not on `main`) — "a pi seat gets the block and is NOT confined by it" — when pi in fact fails closed on it. The fix that closed the class was the set change plus a fixture that runs the derived value through pi's own guard; reverting it reds exactly those two tests.)*
+
+## Evidence and instruments
+
+27. **An evidence tool that accepts a parameter it does not implement makes its own evidence lie — and it lies in the filename.** When a tool's *output* is itself the evidence for a decision (a screenshot pair, a measurement, a diff), an ignored input is worse than a missing one: the artifact is still produced, still looks deliberate, and is still named as though the parameter applied. The reviewer reading the name cannot see the hole and the author has no reason to look. Three mechanical obligations: **refuse an unknown flag rather than defaulting** (a typo'd flag must not become a silent "off"); **echo the parameters that determine the output on the same line as the output** (viewport, auth mode, interaction, content hash), so the artifact travels with its provenance; and **never accept a parameter you do not consume**. Two riders. First, when one silent parameter turns up, every artifact that tool version produced is unlabeled the same way — re-derive them, do not spot-check. Second, a correct parameter set is still not a closed instrument: an input the tool cannot see can move the render as much as one it can, so the run line must also report the *failure signs* — here, every non-2xx asset fetch, because a web-font 403 silently swaps the typeface and the pair then differs for a reason that is neither revision. Delivery convention worth keeping: when the pair is handed to a human, give them **one** artifact to read (a before/after contact sheet at native pixels) and point at the committed originals as the record — an N-image set is N interruptions, and a resized sheet is not evidence of layout. *(Earned: 2026-09-19, #1757/#1758 — `scripts/ui-evidence-shot.mjs` accepted `--width` and never resized it, so a capture written as `--out ...-390.png` was a 1440×900 desktop render under a phone's name; the only tell was the file's own width, which nobody reads until they already suspect it, and a pair was posted on that naming. The fix is that PR's, not `main`'s — #1770 implements the three obligations, while `main` still hardcodes 1440×900 (`scripts/ui-evidence-shot.mjs:87`) and drops `--width` without a warning — and with it the re-captured pair is 2400 px at 1200 and 780 px at 390. Same arc, second instance: a symlinked `frontend/node_modules` made vite 403 the `@fontsource` woff2s, so the before render used fallback fonts and the pair differed by typeface. Both holes were invisible in the artifact and visible only in the run line, which now prints viewport, click selector and `innerText` hash per capture.)*
+
+28. **A before/after pair that is byte-identical where you expected a change is evidence about the SURFACE, not a null result — and identical pixels are not a stable property of two sessions.** Three distinct states render identically in a screenshot: the change did nothing, the element never mounted, and the element is in the DOM but not painted (`display:none`, or hidden by a media query at that width). The third reads as the first, which is why the discriminator is an element-level read and not a pixel compare — `document.body.innerText` is rendering-aware and drops hidden text while `textContent` keeps it, so taking one selector's own text separates *hidden* from *absent* from *unchanged*. Corollaries: an identity claim across two capture sessions is not supportable at all (relative timestamps and rebuild noise move between them, so "byte-identical to what I read earlier" is a claim no instrument can make — say what was compared, in which session); and when the change lands on a surface a stylesheet hides at the width you are shooting, name the surface that carries it — the aside that paints where the row does not — instead of shipping a file whose name asserts a difference its pixels cannot show. *(Earned: 2026-09-19, TASK-050/#1757 — the 390 row pair renders the same state before and after, and that is correct: `v2.css` hides the row's audience line under 760 px (#1712), so the fix shows at 1200 on the row and at 390 in the Manage aside, the one surface a phone actually paints. The pair was byte-identical in the round it was first observed (both files 117,949 bytes) and the re-capture of the same two revisions produced different bytes for that same state — one rule, both clauses, measured; both rounds were shot with #1770's width-capable harness, which is not on `main`. The identical pair had been discounted as showing nothing until the hide rule was named; the fix then moved to the aside, and the reviewer cleared it there.)*
+
+29. **One renderer, two meanings: a fix aimed at a rendered string must classify every call site by the DATA it receives, not by the text it emits.** When two surfaces share a label helper, the same string on both can be a defect on one and the fallback firing correctly on the other — and the string cannot discriminate them. Only whether the identifier *resolves* in the viewer's context can. So the review move is mechanical: grep the helper's call sites; for each, name the provenance of the argument and which branch it takes. A fix that upgrades members reaches the site whose argument *is* a member, and never the site whose argument is a seat with no installation and no membership — which is exactly why "the same mislabel, one surface down" is a hypothesis to test rather than a finding to file. **The ledger settles it, not the render**: query the rows behind the label, and if those rows are fixtures say so. *(Earned: 2026-09-19, TASK-050/#1757 — `seatLabel` (`V2ConnectorTools.tsx:271–274` on main) renders both the aside's `agents allowed` line (:727) and each trail row (:786). The aside's `an agent` for a resolvable member was the defect; the trail's `an agent · github.merge_pull_request` was the fallback firing correctly, because the caller — `agent_user_id` in PG `tool_calls` — is a seat with no installation row and no pod membership, so the helper has nothing better to offer. The room's first read called the second string the same mislabel; the ledger reversed it and the peer who asserted it retracted. A claim repeated by two seats is still one claim.)*
+
+30. **A question about what a rendered value MEANS is answered from the store, not from the renderer.** A renderer's fallback chain tells you which inputs *could* have produced a string; it cannot tell you which one did, and reading the chain to reason about the cause settles nothing. Enumerate the chain from source — each `||` is a hypothesis with a different mechanism — then measure the inputs in the store. Prefer a decisive experiment to more reading where one exists: a single write plus a single re-capture falsified a membership hypothesis in about a minute (and the probe was reverted, so the committed evidence still matches its seed). When the answer is "the artifact is a harness artifact", say so **with the artifact**: a screenshot cannot distinguish a fixture row from live data, so any evidence built from seeded records must name its seed and the fixture-shaped fields in the same breath as the image, or a reader will read the fixture as usage. Rider: a hand-seeded row is not equivalent to a posted one — the real write path also creates the identity row and the membership, so a hand-inserted record reproduces neither. *(Earned: 2026-09-19, the #1758 evidence — the seeded card rendered as `Unknown User` and the file-worthy hypothesis was pod membership; the measurement refuted it (added the member, the page read `2 members`, the label unchanged). The cause was the PG mirror: `backend/models/pg/Message.ts:214,261` LEFT JOINs `users` for `u.username`, and the hand-inserted card's author has no row there because it never went through `agentIdentityService.ts:618` (`INSERT INTO users`) inside `getOrCreateAgentUser`, which `agentMessageService.ts:1245–1249` calls together with `ensureAgentInPod` before posting. Nothing filed — and four seeded `tool_calls` rows read as live usage by a human until their provenance was stated.)*
+
+31. **A flake fix is proven by making the defect reproducible, not by rerunning the flake — and an assertion that counts a process-global resource must watch the subject's own identity.** "It passed ten times" is not evidence about a flaky assertion: the flake's rate bounds its cost, never whether it is fixed. The reproducible form is to mutate the guarded behaviour into existence and watch the same assertion go red, which also proves the assertion measures the thing it names. The follow-up question is what the assertion counts: a count over `os.tmpdir()` contents, a socket table or a lock directory is process- or machine-global state a sibling test or another actor can perturb, so the assertion must watch the *subject's own* identity — this run's path, this seat's row — not the pool it lives in. One negative result worth carrying, because it looks like an escape hatch and is not: setting `process.env.TMPDIR` does **not** redirect `os.tmpdir()` inside jest, though a bare `node` v20 does follow it — mechanism unexplained, do not build isolation on it. *(Earned: TASK-073/#1769 — the codex adapter creates its temp dir at `codex.js:442` and removes it in a `finally` at `:506`; the test now watches the directory that call created, so dropping that `rm` reddens exactly that test, with an in-seam `existsSync` control so it cannot pass against an adapter that never creates the directory. Control for the flake itself: branch 3/15, main 3/15, alone 0/10 — the rate was pre-existing; the instrument was the fix.)*
+
+## Merge mechanics
+
+32. **Verify a merge at the consumer: for a squash merge, the merge commit's tree must equal the carried head's tree.** A merge SHA and a green tick prove the workflow ran; they do not prove that what landed is what was reviewed — carries, rebases and stale-base reverts move code between review and merge, and the merge commit's message will not say so. Under this repo's squash convention the proof is one command per side: `git rev-parse <merge>^{tree}` against `git rev-parse <carried-head>^{tree}`, plus the parent and the author/trailer check. Fetch the head yourself (`git fetch origin refs/pull/<N>/head`) — a reviewer's claim about remote state is as checkable as a code claim — and diff from the natural merge base, because `git diff main...head` against a stale local `main` is a fiction (a checkout whose local `main` trails `origin/main` by double digits is the ordinary state here, and every PR is cut from the fetched `origin/main`). Riders: a **carry** is a *per-file* claim (`git patch-id --stable`, per file), not a whole-PR one, and it is needed only where non-doc code moved between bases — a docs-only addition on top of a cleared head is a re-stamp, so state old→new head and let the reviewer stamp what actually lands; and know the required-check set before calling anything green or blocked, because every check being green is not the same as the required check passing, and a required check that has not yet reported renders as `BLOCKED` — the ordinary in-flight state, not a verdict. *(Earned: every closure since — #1755 verified as `e938afc8`, tree `b86b05e4` equal to the carried head `ad3aa8d7`'s, parent `2d0ee979`, no foreign trailer; and the precedent that motivates the rule rather than any one incident: #568's fix regressed twice through a stale-base revert, which is exactly the class the tree comparison closes — the merge looked clean, the tick was green, and the fix was gone.)*