review-rigor: R1d — shared-state and sibling-fix interactions within one change

docs/core/review-rigor.md

@@ -133,6 +133,7 @@ The diff is never the whole system. Most expensive misses live in the *relations
 - **R1b — Trace from the entrypoint, not the hunk — and from EVERY entrypoint for shared code.** For any new route/feature, start at `main.go` / the router / the composition root and confirm the path is reachable end to end. "The in-diff logic is correct" is not "the feature is reachable" — the wiring that connects the hunk to the entrypoint lives outside the hunk, and that's exactly where it gets dropped. A **shared library / exported symbol has more than one entrypoint**: `grep` the whole repo for **every** caller (across other services/packages, not just the file you're reading) and trace each — the credential/session/gate helper you changed is very likely invoked by an in-process function in *another* service, not only by this service's HTTP routes. Reviewing one service's mounted routes while a sibling service calls the same exported function on a different path is the canonical miss.
 - **R1b-sec — A security/credential/authorization gate must fire on EVERY entry variant.** When a change adds, splits, moves, or renames an auth gate (account-status, lockout, method-toggle, permission check, session control), enumerate **all** authentication entry points for that credential — the HTTP handler(s) **and** every exported in-process `Authorize*`/verify function other services call — and confirm the gate fires on each. A gate split (e.g. moving deactivation out of a lockout check into a new helper) silently drops the gate on any caller that still calls only the old function. Detection cue: two functions authenticating the same credential (an HTTP handler and an exported in-process variant) with different gate sequences → factor one shared gate helper both call, or flag the drift as a blocker.
 - **R1c — Test-wiring must match entrypoint-wiring.** When a test injects a dependency through a mock or helper — a denylist, a fake sender, a hand-built client/RP, an in-test registration — confirm the **production entrypoint constructs the same dependency**. A divergence yields a green suite over a broken production path: the tests mask the integration gap. Detection cue: a test helper named `mock*With*` / `*WithX` supplying wiring the entrypoint lacks.
+- **R1d — Shared-state and sibling-fix interactions within one change.** R1b enumerates callers of a shared symbol *across services*; this is its intra-change twin, and it is where a change of **several fixes to one subsystem** breaks itself. Two triggers: (1) a fix **repurposes or reinterprets a shared field / flag / cached value** that more than one function reads — enumerate **every** reader of that state and confirm each still holds under the new meaning, because a field can carry two roles (e.g. a cache that is *both* the value a getter publishes *and* the baseline a diff compares against) and a fix serving one role silently corrupts the other. (2) **multiple fixes land in the same change** and each changes state a sibling fix depends on — verify the **cross-product**, not each fix against its own purpose in isolation, because fix A's new guard and fix B's new skip can each be correct alone and combine into a stranded/dead state (a guard that rejects the very lane a sibling's "healthy, skip it" path just declined to rebuild). The self-review trap: checking each fix against the blocker it closes passes every fix while the **interaction** — the state one fix writes and another reads — goes unexamined; a discriminating test targets that seam directly. `pass` = every reader of the changed state, and every sibling fix touching it, is enumerated and holds · `blocker` = a second consumer or a sibling fix reads the state under an assumption the change just invalidated · `n/a` = the change is a single fix touching no multi-reader state. Detection cues: one field/flag read in more than one function where the diff changes what it means or when it is set/cleared; two or more behavioral fixes to the same module/subsystem in one change; a fix that flips a "skip healthy / already-done" predicate while another adds a guard on that same lane's I/O.
 - **R2 — Comment/doc/runbook co-change sweep.** A behavior / path / flag / name change must update every comment, docstring, `--help` string, README row, and runbook step that describes the old behavior — almost all of which live OUTSIDE the diff. Stale prose that contradicts the new code is a blocker, not cosmetic. **Fenced blocks are sweep subject, not opaque literals**: a prompt/command template embedded in a doc is the literal text an agent or shell receives — the most load-bearing prose in the file — so when a section, flag, or term it references changes, sweep the fence's contents like any prose. Detection cue: a diff that renames or reshapes a section/contract in a doc that also embeds a fenced template referring to it.
 - **R2b — Prose-claims-about-code ground-truth check (R2's reverse direction).** When the change's *prose* — a doc, a code comment, a runbook, the PR body — asserts how code behaves ("X is never deleted", "children share branch Y", "the driver retries Z"), READ that code and verify the claim against it; do not clear it by internal consistency alone. A docs diff can be fully self-consistent — every cross-reference resolves, every cited doc agrees — and still be wrong against the code it describes; consistency among documents is not evidence about behavior. The described code is in scope for the review even when it lives outside the diff, **including in a sibling repo** (doctrine describing another service/driver: open that repo's source). Each behavioral claim resolves like any R-check: `pass` (cite the function/lines that exhibit the claimed behavior) · `blocker` (the code contradicts the prose — the prose is the bug to flag) · `n/a` (the diff's prose makes no behavioral claims). A behavioral claim whose ground truth you could not read is `blocker`, not "probably accurate". **Ground truth is not limited to repo code:** a claim about an external mechanism — shell semantics, a CLI tool's flags or behavior, a platform/API guarantee ("flag X aborts the pipeline", "version Y lacks flag Z") — is verified by *executing* the mechanism or citing its authoritative spec, exactly as a code claim is verified by reading the code; familiarity with the tool is not evidence. **Enumerate the claim set before verifying — coverage is part of the verdict.** List every behavioral/guarantee claim the changed prose asserts FIRST, then resolve each one; verifying a sampled subset and approving is a degraded review. On an APPROVE, the posted "Verified" section must map to the **check classes that fired**, not only to how deeply a few were verified — deep verification of three checks is not coverage of the set, and the coverage ledger (→ *Review stamps*) is what makes that gap visible. Sampled coverage is why two rigorous passes over the identical diff can return opposite verdicts — each samples different claims, and the missed defect lives precisely in the claim neither the verdict nor its "verified:" list ever names (e.g. a pass that empirically verified three tool-behavior claims in a remedy list and approved, while a sibling pass trace-refuted the list's one load-bearing abort claim on the same head). Detection cues: a docs/comment diff naming concrete mechanisms (branch names, retry/teardown semantics, survival guarantees, ordering); a review note like "consistent with the docs it cites" standing in for verification; an approving review whose verified-claims list is shorter than the claims the diff's prose asserts.
 - **R3 — Claim-vs-diff reconciliation.** For "Closes #N", "no behavior change", or an enumerated list of deliverables: verify each claim is actually present in the diff, and that nothing beyond the declared scope changed. Sharper case: a claimed *guarantee* delivered by a **weaker mechanism** than stated, with no test exercising it, is a claim-vs-impl gap — e.g. the body claims "corrupt/truncated input is rejected" but the code only magic-byte-sniffs and then detect-and-corrects, never rejecting. Require the stronger mechanism, or drop the claim. **A claim the change authored is always in review scope, even when the code that would fulfill it is not** — the scope-discipline out-of-scope carve-out (see *Scope discipline*) governs the untouched *code*, never the *claim* this change wrote in its body / commit / branch. So when a claim overstates what the diff delivers and the code that would deliver the rest is pre-existing or out of scope to fix, the deterministic verdict is `blocker` with the **cheap in-scope remedy: narrow the claim to what the diff actually delivers** (a one-line body/commit edit the author is right here to make) — never an `approve` that lets the overbroad claim stand as a "non-blocker", and never a demand to fix the out-of-scope code. Two reviewers splitting here — one blocking the claim, one waving it through as out-of-scope — is the canonical verdict-variance this rule removes: the claim's fate is decided by R3 identically for both, not by whether the referenced code is in scope. (e.g. a body claiming "makes *every* X-gated dial resilient" while several pre-existing dials in untouched services still read the raw env → narrow to "the three shared gates + Y", or route them; a "Closes #N" claiming full resolution while half the acceptance lives in an untouched module → narrow to "Partially addresses #N", or deliver it.)