diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 7603ad0b7..170817cd6 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -151,3 +151,11 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b **Rider — a shared code is client-visible, so "same code" is a contract decision and not a test detail.** Where a route answers `{ error: code }` with no message, the operator cannot tell the two defects apart: one reading `issuer_metadata_incomplete` cannot distinguish a document that names no issuer from one that names no endpoint, and the code is all the response carries. Splitting the vocabulary is a separate, client-visible change; making that choice deliberately — or surfacing the message — is the honest form, and the worst outcome is neither: a test that cannot tell the two apart beside a response that cannot either. *(Earned: 2026-09-29, #2007/TASK-172 — the RFC 8414 §3.3 issuer check versus the endpoint guard in `backend/services/hostedMcpIntakeService.ts`. Found by the gate, not the author: deleting `if (!body.authorization_endpoint || !body.token_endpoint)` left the neighbourhood green, and one line adding an `issuer` to that arm's fixture restored the witness. **The first draft of this rule then drew the wrong conclusion from its own incident** — that a shared code obliges every arm asserting it to assert the message too — and the docs gate refused it with the counter-example that the measured table above now carries: at the repaired head the code assertion alone discriminates. Both halves are the rule, and the second is a caution about this file: a checklist line that over-prescribes the *assertion shape* sends every future reader to add a second assertion instead of asking which guard answers their input.)* + +## The merge queue + +43. **A queued PR refuses every push, and that refusal is what keeps the merged head equal to the queued head.** Once a pull request is in the merge queue, GitHub rejects any push to its branch — `GH006: Protected branch update failed for refs/heads/ … branches that are queued for merging cannot be updated. To modify this branch, dequeue the associated pull request.` Read it as a mechanism rather than an obstacle: the queue merges the head it saw, so a delta arriving after queue entry cannot silently ride a stale clearance into main, and rule 32's tree comparison is then guaranteed rather than lucky. It is not a clearance in itself, though — that the *queued* head is the *gated* head is still the press's job to check, which is rule 44's question. Land a late delta as a **follow-up PR** based on the merged main, showing per-file fidelity is empty (`git rebase --onto origin/main `, then `git patch-id --stable` per file); the costlier alternative is to dequeue, apply, re-gate and re-queue, which spends every stamp bound to the head. The follow-up is **ungated** until it is gated at its own head: cite the pre-rebase commit as provenance, by **full** sha, since a short sha will not fetch and a provenance citation that gets read as a clearance is the defect rule 44 exists for. *(Earned: TASK-180's #1984 — ux-lead asked for the `%` fold at 04:48:45Z and the PR was queued at 04:50:20Z, 95 seconds later, so the fold could not land on that head at all and became #1988: one catalog line, a rebase and two gates. The queue had already merged the gated head faithfully — squash `f381b5aa` against `7f171637`, the three TASK-180 files byte-identical — so the pressed artifact was the reviewed artifact, which is the outcome the refusal buys. Before this entry existed the operator-facing half was documented nowhere else, and the count is **per string, not per union**: `merge queue` appeared in exactly one tracked file, `.github/workflows/tests.yml:13`, whose surrounding lines are about the `merge_group` trigger rather than the push refusal, while `GH006` and `queued for merging` appeared in **no** tracked file at all — `git grep -l -F` for the three strings over the whole tree on main returns 0, 0 and 1. Read those as the state of main *before* this rule landed, because the rule quotes all three: at this head it is the second file for every one of them, and a grep for `GH006` alone returning nothing is that pre-landing state and not a rule that has gone stale.)* + +## What a clearance can bind + +44. **A clearance has to exist as a PASS recorded against the head being pressed, and prose is not a record.** The question is existence, not identity: not whether the gated content is what merged (that is rule 32), but whether *any* clearance was ever taken at the head the press will read. Use GitHub's own record rather than anyone's prose — every review in `GET /pulls//reviews` carries `commit_id`, the head it was submitted against, so selecting reviews whose `commit_id` is the pressed head finds the reviews submitted against it: candidates, not clearances. **Existence is necessary and not sufficient, because a review at the head can be a refusal.** `state` carries no verdict here: measured across #1988, #1989 and #2000, every review on all three is `COMMENTED` — **18 of 18 when this was written, and that number only grows as seats post more, so re-run the count rather than quoting it** — not one `APPROVED` or `CHANGES_REQUESTED` across any of them, so the verdict is the body's first line and nothing else. The check is therefore **per required gate: a review whose `commit_id` is the pressed head and whose first line names that same head and declares that gate's PASS.** The head name is not decoration. `commit_id` records the head **at submission**, so a gate taken at X and posted after a push to Y lands at Y and reads fresh at a head nobody read — and a check that stops at `commit_id` will accept it. A first line naming an *older* head is a rule 32 carry question, not an existence pass: the two heads may share a tree, in which case the artifact really was gated (32's test) and no existence check can know it; or they may **differ**, in which case the clearance names content nobody gated. 44 answers whether a record claims *this* head; 32 answers whether an older claim still carries to it. Two counterexamples, one per half. #2000 @ `d1d56e4c` is what a bare existence test clears: its only review is a `DOCS-GATE: CHANGES` at that head. #1981 is what a `commit_id`-only test clears: pressed at `3d6e1763`, where its code review carries that `commit_id` and reads `DELTA RE-GATE: PASS @ a4566e29`. A head whose reviews are all refusals, or all corrections — "Correction to my gate above" declares no verdict — is ungated, which is the honest answer rather than a pass by default. `submitted_at` orders reviews but does not bind them to a head, and it cannot be used to select one either: every seat here posts under one GitHub identity, so `.[-1]` returns whichever seat reviewed last. Two checks that look right and are not. `git merge-base --is-ancestor ` passes a clearance a later head move has already spent — `b8048c72` is an ancestor of `177ba436` — which is 32's class of defect, not this one. And a sha missing from your clone says nothing about the remote: a commit can be there with no ref pointing at it, so the remote has it, no ordinary fetch brings it in, and `refs/pull//head` keeps a squashed-away head fetchable — an absence read in one clone is as clone-local as a presence read. `commit_id` cannot tell a carry from a re-derivation, so the body still has to be read for that; it answers only whether a review was submitted against that head at all. Its own entry rather than a rider on 32, and the argument is 41's test applied to 32: every guard 32 sends you to check passes here — nothing had merged, no head had moved, the required set was green — and the defect survives all of them, because 32 presupposes a clearance and this asks whether one exists. *(Earned: TASK-186's #1988. The row recorded "CODE PASS @ `58298156`" from 06:54Z on 2026-09-28 and carried it for **29.4h**; the board title carried "… (UX PASS @ `1a811d84` + CODE PASS)" for **8.7h** from 03:34Z the next morning. No clearance was ever taken at that commit: it is a **child** of `7f171637` — a sibling of `1a811d84`, not an ancestor of it, and `GET /pulls/1988/reviews` held four reviews at that point — every one of them `commit_id=1a811d84` — of which only the first existed before 12:17Z on 2026-09-29. The commit itself was confirmable — `gh api commits/` resolved it and a full-sha fetch retrieved it — but no advertised ref pointed at it — `git ls-remote` listed 2,579 of them when this was measured and not one was this sha, a number that only grows, so re-run it rather than quoting it — and `git fetch origin ` fails, so no ordinary fetch brought it into any other clone. So the commit could be confirmed and the gate could not, because there was no gate; a rule written about unresolvable heads would have mis-stated this incident. The title's *shape* did as much damage as its content — binding the sha to the UX pass and leaving the code pass bare is what let a later reader collapse it into "CODE PASS @ `1a811d84`", a head that was real and resolvable and so looked verifiable. And the `commit_id` half has its own incident: #1981 was pressed at `3d6e1763`, and the code review bearing that `commit_id` reads `DELTA RE-GATE: PASS @ a4566e29` — the stack had been re-authored from `a4566e29` to `3d6e1763` between the gate and its posting, and the two commits share tree `0e3b2d01cf`, so the artifact was gated and the record could not say so. Same PR, and the hazard's harsher half: the code review submitted at 21:26:47Z carries `commit_id=245dbaf4` while its first line names `5e0e7193`, and those trees **differ** — three files, three lines, the "pod, not room" string among them — so it attached a clearance to content nobody had gated; superseded seven minutes later and never pressed, which is why the incident above is the one that reached a press, and why a check that stops at `commit_id` cannot be trusted at all.)*