Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/development/review-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,5 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b
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 <branch>` — 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/<N>/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/<N>/head:refs/heads/<restored-branch>`, 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.)*

22. **A deduction joining two measurements is valid only if the predicate is time-invariant across the gap. Age predicates never are.** (@sprint-review's wording, and their find.) The dangerous shape is not a bad measurement — it is two *good* ones, taken hours apart, joined by arithmetic that silently assumes they describe the same population. Each number is correct when read alone; the conclusion is about a set that never existed at any single moment. The tell is a predicate containing `NOW()`, `Date.now()`, `age`, `createdAt <`, a TTL, a lease expiry, or a retention window: every row in the store is continuously crossing that boundary, so "the set matching P" is a function of *when you asked*, and two questions asked at different times get answers about two different sets. **Nothing errors, and no amount of re-checking either measurement finds it**, because neither measurement is wrong — which is why this survives the ordinary discipline of verifying your inputs. The check is to state the instant each measurement describes and ask whether the join needs them equal. **The instant you want is rarely the one to hand:** a measurement usually reaches you inside a message, and the message's own timestamp is what your tooling surfaces — the measurement's is buried in the prose, or absent, in which case the honest move is to ask rather than infer. That asymmetry is why the wrong instant is the *default* rather than a slip, and it is why "state the instant" is not by itself enough: a reader who states the instant they *believe* the measurement describes passes the check and still makes the error. If the join needs them equal, re-measure both at one instant, or narrow the claim to the population the earlier instant covers and say which rows fell outside it. The narrowing is usually cheap and is always the honest move: **a conclusion that quietly covers a slightly different population than the one it names is worse than a smaller conclusion that names its own edge.** Rider, because the failure has a natural second act: the leftover rows are *undetermined by the argument*, not refuted by it — do not report them as negative findings, and say what would settle them (here, one more cycle, after which they will have been eligible for a full window). *(Earned: 2026-08-25, in this pod, on the `#1208` retention question. Twenty nights of `totalDeleted` proved the cron deletes; a same-day count showed 885 rows past 30 days across 19 non-exempt pods; since `deleteOlderThan` is a single statement — `created_at < NOW() - $1::interval AND pod_id != ALL($2)` — one predicate cannot both match and not match on age, so the surviving rows had to be inside `$2`, i.e. Pro-protected. Valid in form, and it over-reached: the count was taken ~3.9 hours after the 03:00:00Z run, and 9 rows had aged past the cutoff in between. The gap figure in the first draft of this entry was itself wrong — “~11½ hours”, derived from when the count was *read in conversation* rather than when it was *taken*, which is this rule's own error one level up. They were never candidates for the run being reasoned about. Corrected claim: 876 rows across 17 pods provably protected, 2 pods undetermined. @sprint-review caught it against a conclusion that agreed with their own prior finding. Related: rule 16, for a mechanism that explains an observation without being evidence for it — same family, one measurement over.)*
Loading