Skip to content
Merged
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
8 changes: 8 additions & 0 deletions docs/development/review-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/<branch> … 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 <old-head>`, 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/<n>/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 <sha> <pr-head>` 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/<n>/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/<sha>` 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 <short sha>` 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.)*
Loading