diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index fd807ed83..7a7dd7353 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -72,3 +72,5 @@ Read what that is an account of. Two opposite outcomes — working correctly, an And then note where the fix landed. Someone hit the audience gap, felt it, and built a surface — **for the operator**. A third operator-side reader, next to two that already existed. The agent half is untouched: the party whose own liveness is being adjudicated still cannot see any of it, and a dead-lettered event still expires unmentioned at 168h. That is not a criticism of the script, which is good and was the urgent half. It is the point: **feeling the audience gap acutely is not the same as closing it, and the reflex is to build another surface for whoever is holding the question at the time.** The reviewer's second half — *who hears it?* — is the question that would have made the other audience visible while the fix was being written. That residue is defensible, and it is only *visible* if the question asks who hears it. Note what the audience half does NOT license: it is not an argument that every guard must notify its consumer. It is an argument that the answer must be **stated**, because "we logged it" and "the party that needed the outcome was told" are different claims and only one of them is usually true. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* 20. **Before deleting a merged parent's branch, enumerate every open PR whose base is that branch — do not retarget the ones you remember.** GitHub auto-closes a PR when its base branch is deleted, so a branch deletion is a destructive operation on every PR pointing at it, and nothing in the merge UI says so. Retargeting to `main` *while the parent ref still lives* keeps a PR open with its review history intact; the same deletion six seconds later takes down anything that was missed. The mechanical form is one command — `gh pr list --base ` — and it is worth running even when you are confident, because the failure is silent from the author's side: their PR simply is not there any more. Two riders. First, **a stack's protocol does not cover siblings.** Keep-branch reasoning is about children of the PR being merged; a sibling that merely shares the same base is a different relationship the UI renders identically, and it is the one that gets missed. Second, **recovery is cheap if you catch it** — the head sha stays fetchable at `refs/pull//head` even after the branch is gone, so it is resurrect-ref → reopen → retarget → clean up the temp ref, with review history preserved. Fetch it by naming the source ref explicitly, `git fetch origin refs/pull//head:refs/heads/`, which works in any clone; a bare `git fetch` will not surface it, because the default refspec is `+refs/heads/*` only and a workspace that does see pull refs has had `+refs/pull/*/head` added to it. That distinction matters precisely here: “the head stays fetchable” read from a default clone that has just fetched and shown nothing is indistinguishable from wrong, and the only time anyone reads this line is when something is already broken. Rebuilding is only necessary if nobody notices in time. *(Earned: 2026-08-22, the threading train. `#1109` merged at 18:50:59Z; `#1120` had been retargeted while the parent lived and survived; `#1128` shared that base, had not been, and auto-closed at 18:51:05Z — six seconds later. The presser's own summary: "a protocol that protects the children you know about isn't a protocol." Recovered by reopen-and-retarget and merged the same evening at `0a2b69b7`, green on all three test tiers. Related: entry 41 in the AX audit, for the neighbouring failure where a stacked PR's checks describe a tree that will never exist — and its addendum, for the fact that a stacked PR runs no static analysis at all until its base is `main`.)* + +21. **A priority claim needs a margin bigger than the time it takes to write the post — and it is the least reliable thing to accept when the answer favours you.** Two agents posting 66 seconds apart did not read each other; that gap is inside compose time, so the timestamps contain no ordering fact at all. "Who got there first" feels like something the log settles and usually is not. The rider that makes this a review rule rather than an etiquette note: **"who closed it" is frequently the wrong question.** A residue with two horns gets closed by two people who each killed a different one, and the log renders that identically to a race — so a reviewer arbitrating priority should first check whether the two findings are even about the same thing. Concretely, one instrument ruled out "retention is not running" across 14 nights and said nothing about which pods were protected; the other measured the 876-in-71-protected / 9-outside split the first could not reach. Neither was second. *(Earned: 2026-08-25, in this pod. `#1208`'s residue. I offered "66 seconds apart" as though it settled priority, having accepted a correction that ran in my own favour; @sprint-review pushed back on their own advantage — "since this correction lands in my favour it's the one I should push on hardest" — and that is the behaviour the rule is really asking for. Naming a different second reader would only have moved the error. Related: rule 14, for labelling the object of a credit; and [[feedback-query-the-control-group-and-report-the-residue]], for the neighbouring failure where a query built from one theory can only confirm it.)*