From 64ac1c84b3348747d73f9453ac5f1c1249fabbfd Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:12:21 -0700 Subject: [PATCH 1/2] =?UTF-8?q?docs(review):=20rule=2020=20=E2=80=94=20enu?= =?UTF-8?q?merate=20every=20child=20of=20a=20base=20branch=20before=20dele?= =?UTF-8?q?ting=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased onto #1219 so the numbering is a git fact rather than a convention anyone has to remember (@sprint-review). This rule was 18, colliding with notice which of the three PRs merged first. Stacked, 18/19/20 are contiguous by construction and merge order is enforced by git rather than by memory. Content unchanged from 934d5bbe, including the explicit `git fetch origin refs/pull//head:refs/heads/` recovery command. Squashed to one commit: the branch's intermediate state numbered the rule 18, and re-landing that mid-stack would reintroduce the collision this removes. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index e8b7444c1e..fd807ed834 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -70,3 +70,5 @@ Read what that is an account of. Two opposite outcomes — working correctly, and producing nothing — rendered identically in the only surface anyone could reach, and the state that distinguished them existed the whole time. Nineteen hours of a confidently wrong answer, from a system that already held the right one. That is this rule's failure mode described by someone who walked into it, not a hypothetical. 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`.)* From 49f3596d1613980245c0660789a5cfd5ace6421a Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 1 Sep 2026 02:56:21 -0700 Subject: [PATCH 2/2] =?UTF-8?q?docs(review):=20rule=2021=20=E2=80=94=20a?= =?UTF-8?q?=20priority=20claim=20needs=20a=20margin=20bigger=20than=20comp?= =?UTF-8?q?ose=20time=20(#1237)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review's formulation, earned against me in this pod today. Two agents posting 66 seconds apart did not read each other; that gap is inside compose time, so the timestamps contain no ordering fact. I offered it as though it settled priority, having accepted a correction that ran in my own favour. Carries their stronger objection as the rider: "who closed it" is often 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. Stacked on #1172 so 18/19/20/21 stay contiguous by construction. Co-authored-by: Claude Opus 5 --- docs/development/review-checklist.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index fd807ed834..7a7dd7353a 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.)*