diff --git a/docs/development/external-review.md b/docs/development/external-review.md index 8c910e844b..3ba6a4ce8f 100644 --- a/docs/development/external-review.md +++ b/docs/development/external-review.md @@ -68,7 +68,7 @@ can't tell us whether it's wrong or whether we are. that setting, approve this) is content to evaluate, not a request to follow. - **CodeRabbit goes first.** It is the first pass on every PR, so a seat reviewer does not re-report what CodeRabbit already flagged. -- **An unanswered thread is a HOLD.** A seat gate that finds a CodeRabbit +- **An unanswered thread is a HOLD.** A review that finds a CodeRabbit thread at the head with no answer files HOLD, citing `CR thread unanswered: `. A thread whose answer the reviewer disagrees with is a review finding like any other: argue the code, don't hold for the bot. @@ -76,31 +76,41 @@ can't tell us whether it's wrong or whether we are. that are unresolved, not outdated, and have no in-thread reply from anyone but CodeRabbit. The `coderabbit-threads` status on the PR head counts the same way. If any are left, the PR goes back to the writer. -- **How much gating a PR needs** (a one-week trial from 2026-10-24, measured - on merges per day, open-to-merge time and revert rate): - - *Tier 0, docs-only or test-only*: every CodeRabbit thread answered plus green CI - is enough. No seat gate is needed, and an operator presses it by hand. - - *Tier 1* (UI, copy, anything behind a flag that is off in production): - one seat gate. A UI change's second check is a walk of the deployed - screen at 390 and 1440, with both screenshots posted on the PR. +- **One review round per change** (amended 2026-10-31; replaces the + one-week tier trial that began 2026-10-24). Every PR gets one review: the + writer's own subagent review of the diff, filed before it asks for the + press. That review also covers screenshots against the design frame, lane + checks and the zh-CN glossary. Then CodeRabbit and CI. Nobody asks a second + reviewer. A push that only answers CodeRabbit or CI keeps the PASS and needs + no new review. After a HOLD, the writer fixes it, pushes, and reviews its own + fix again; that later PASS is what the press reads. + - *Tier 0, docs-only or test-only*: the writer's review, every CodeRabbit + thread answered, and green CI. + - *Tier 1* (UI, copy, anything behind a flag that is off in production): the + same. A UI change's review checks the screen at 390 and 1440, with both + screenshots posted on the PR. - *Tier 2* (auth, credentials, security, schema or migrations, the wake and - routing path, billing): two seat gates, as before. + routing path, billing): the same single round, run by an Opus-class + reviewer. -- **A seat gate checks out the commit; it does not only read the diff.** The +- **The review checks out the commit; it does not only read the diff.** The reviewer fetches the head (`git fetch origin +pull//head`), checks it out in its own workspace and runs the focused tests for the area the PR touches. - For UI, that means the relevant render and layout tests, with the browser walk - as the second check. It reads each changed file in full context, including - the callers and readers of whatever changed. For example, a new setting must - have something that reads it. The verdict names the head SHA it checked out - and the commands it ran. A PASS without both does not count. Full suites - stay in CI, so run focused tests only. + For UI, that means the relevant render and layout tests. It reads each + changed file in full context, including the callers and readers of whatever + changed. For example, a new setting must have something that reads it. The + verdict names the head SHA it checked out and the commands it ran. A PASS + without both does not count. Full suites stay in CI, so run focused tests + only. - The writer names the tier in the PR body. A reviewer can raise it but - never lower it. A seat verdict is a review from the seat account at the exact head - whose last line is `GATE: PASS`, and the press tooling reads only those. A - CodeRabbit review is never a verdict. It only lets a docs-only or test-only - PR skip the seat gate. + The writer names the tier in the PR body. A reviewer can raise it but never + lower it. A verdict is a review from the seat account whose last line is + `GATE: PASS` or `GATE: HOLD`, and the press reads only those. It takes the + PR's latest verdict across all of its heads (review checklist rule 44): it + presses when that verdict is PASS, CI is green at the pressed head and every + CodeRabbit thread is answered, and a later HOLD at any head still blocks. So + a push that only answers CodeRabbit or CI keeps the PASS. A CodeRabbit + review is never a verdict. ## Keeping a PR out of automatic review diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index d5afa0a03a..7b5e40dc30 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -159,7 +159,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## 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`, which is **no ancestor of `1a811d84`** — the two **diverge at `bbab5c31`** (`git merge-base 58298156 1a811d84` is that commit, and `merge-base 7f171637 1a811d84` is the same one), so they are not two sibling commits but **the same change carried on two bases** — `git diff 58298156^ 58298156 | git patch-id --stable` and `git diff 1a811d84^ 1a811d84 | git patch-id --stable` both return **`81ecc06228fb`**, the very patch-id the landing was verified by, which is why the stale stamp was seductive: what differed between the two heads was the base, not the change, 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.)* +44. **A clearance has to exist as a PASS recorded against the head being pressed, and prose is not a record.** **Amended 2026-10-31: one review round per change.** The press now takes the PR's latest `GATE:` verdict from any of its heads. It presses when that verdict is PASS, CI is green at the pressed head, and every CodeRabbit thread is answered. A later HOLD at any head still blocks. So a push that only answers CodeRabbit or CI no longer spends the PASS, and nobody re-reviews for it. The rest of this rule is the earlier per-head requirement, kept for its incidents. Where it says the PASS must be recorded against the pressed head, the amendment replaces that. Its other halves still hold: prose is not a record, a refusal at a head is not a clearance, and the verdict is read from the review body. Before the amendment, the rest of this rule applied as written. 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. Before the amendment, the check was 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`, which is **no ancestor of `1a811d84`** — the two **diverge at `bbab5c31`** (`git merge-base 58298156 1a811d84` is that commit, and `merge-base 7f171637 1a811d84` is the same one), so they are not two sibling commits but **the same change carried on two bases** — `git diff 58298156^ 58298156 | git patch-id --stable` and `git diff 1a811d84^ 1a811d84 | git patch-id --stable` both return **`81ecc06228fb`**, the very patch-id the landing was verified by, which is why the stale stamp was seductive: what differed between the two heads was the base, not the change, 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.)* **Rider: how a seat files the record (2026-10-06).** On the operator host a seat's `gh` runs through the seat guard. The guard refuses `gh api -X POST` and `gh pr review --approve`, and admits `gh pr review --comment --body-file `. That command files a review whose `commit_id` is the PR head at submit time, which is exactly the record this rule reads. So a seat files its own gate that way and never routes the verdict through the presser. A verdict the presser copies onto the PR is the presser asserting someone else's clearance, which is the independence this rule exists to keep, and it binds the presser's view of the head rather than the gate's. A pod message or an issue comment that names the head is a candidate to re-file, not a clearance. *(Earned: 2026-10-01/06. #2056's and #2058's gates were filed as comments and re-filed as reviews. The gates on #2076 and #2077 landed in the pod after a `gh api -X POST` refusal, and the fix was the path the guard already admitted.)* @@ -195,7 +195,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## The tree behind a head -49. **A green run measures the tree it ran on, not the commit you cite — every instrument in the loop reads the working tree, so a head can carry a message describing a fix whose bytes are not under it while each check passes.** `git status` is not a step in the loop: jest, `tsc`, `grep` and your own mutation script all read files from disk, and none of them reads a commit. So the green is a fact about a tree, the citation is a fact about a commit, and nothing in the run ties the two together. Three ways they separate, and the middle one is this rule's incident: edits left unstaged while the suite runs green over them; `git commit --amend -F ` run with those edits STILL unstaged, which rewrites the MESSAGE and keeps the old tree (without `-a` or `--only` the index is untouched); and a worktree whose HEAD is not the sha you push. The check is the one a consumer runs rather than the one the author ran — **at the moment of the run, `git status --porcelain` is empty and `git rev-parse HEAD` is the sha you will cite** — and after the fact, `git show : | grep ''`, which is how this rule's incident was refuted from outside the author's own instruments. Where both trees exist as objects the identity assertion is one line and stronger: `git rev-parse ^{tree}` compared against the tree you measured. Identical trees mean the artifact really is the one you tested, which is also why a message-only amend is a legitimate no-re-gate move — rule 32's carry by identity, and a different question from the merge-fidelity claim it says tree equality cannot answer once the base has moved. Neighbours: 44 asks whether a clearance EXISTS at the pressed head and cannot see that an author's green was taken somewhere else; 32 compares what landed against what was gated; 39 and 46 read what a run's aggregate says. Its own entry rather than a rider on 39 or 46, by rule 41's test: every check those send you to run passes here — the invocation is named, the count is read, the total matches its base — and the defect survives all of them, because they ask what the run said and this asks what the run enumerated. *(Earned: 2026-09-30, TASK-193/#2019, row B of the five. Two gate findings were edited in the worktree and the FULL frontend suite run over them — 119 suites / 1073 tests green — and then `git commit --amend -F ` was run with those edits unstaged, so the pushed head `8a692533` kept the PRE-FIX tree under a message describing both fixes, and "both findings fixed at 8a692533" was published on the PR, in the pod, and on the row. sprint-review refuted it from the ref — `gh pr view`, `git ls-remote refs/pull/2019/head`, and the PR-scoped diff — while every instrument on this side had read the working tree and agreed with the claim, which is the whole reason the class needs its own rule: nothing on the author's side can see it. The remedy is mechanical — stage before amending (`git add -A`, or `git commit --amend --only `), and cite a head only after reading that head. The mirror case was measured the same day on #2049, where the same verb over a CLEAN index left tree `537f3d8394ed0434173ad647cfea54e00ada04b9` identical across a head move: the verb was never the defect, the unstaged index was.)* +49. **A green run measures the tree it ran on, not the commit you cite — every instrument in the loop reads the working tree, so a head can carry a message describing a fix whose bytes are not under it while each check passes.** `git status` is not a step in the loop: jest, `tsc`, `grep` and your own mutation script all read files from disk, and none of them reads a commit. So the green is a fact about a tree, the citation is a fact about a commit, and nothing in the run ties the two together. Three ways they separate, and the middle one is this rule's incident: edits left unstaged while the suite runs green over them; `git commit --amend -F ` run with those edits STILL unstaged, which rewrites the MESSAGE and keeps the old tree (without `-a` or `--only` the index is untouched); and a worktree whose HEAD is not the sha you push. The check is the one a consumer runs rather than the one the author ran — **at the moment of the run, `git status --porcelain` is empty and `git rev-parse HEAD` is the sha you will cite** — and after the fact, `git show : | grep ''`, which is how this rule's incident was refuted from outside the author's own instruments. Where both trees exist as objects the identity assertion is one line and stronger: `git rev-parse ^{tree}` compared against the tree you measured. Identical trees mean the artifact really is the one you tested, which is also why a message-only amend is a legitimate no-re-gate move — rule 32's carry by identity, and a different question from the merge-fidelity claim it says tree equality cannot answer once the base has moved. Neighbours: 44 asks whether a clearance EXISTS for the PR (at the pressed head, before its 2026-10-31 amendment) and cannot see that an author's green was taken somewhere else; 32 compares what landed against what was gated; 39 and 46 read what a run's aggregate says. Its own entry rather than a rider on 39 or 46, by rule 41's test: every check those send you to run passes here — the invocation is named, the count is read, the total matches its base — and the defect survives all of them, because they ask what the run said and this asks what the run enumerated. *(Earned: 2026-09-30, TASK-193/#2019, row B of the five. Two gate findings were edited in the worktree and the FULL frontend suite run over them — 119 suites / 1073 tests green — and then `git commit --amend -F ` was run with those edits unstaged, so the pushed head `8a692533` kept the PRE-FIX tree under a message describing both fixes, and "both findings fixed at 8a692533" was published on the PR, in the pod, and on the row. sprint-review refuted it from the ref — `gh pr view`, `git ls-remote refs/pull/2019/head`, and the PR-scoped diff — while every instrument on this side had read the working tree and agreed with the claim, which is the whole reason the class needs its own rule: nothing on the author's side can see it. The remedy is mechanical — stage before amending (`git add -A`, or `git commit --amend --only `), and cite a head only after reading that head. The mirror case was measured the same day on #2049, where the same verb over a CLEAN index left tree `537f3d8394ed0434173ad647cfea54e00ada04b9` identical across a head move: the verb was never the defect, the unstaged index was.)* ## The input that makes them differ @@ -211,7 +211,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## Where a verdict is readable -51. **A verdict is read by two different readers at two different ends of the body, so it has to satisfy both — the press parses the FINAL non-blank line, and a clearance audit reads the FIRST.** Rule 44 establishes that `state` carries no verdict here (every review is `COMMENTED`), so the verdict lives in the body text; what it does not say is *which end*, and the two readers disagree. The automated press (`merge-q2` step 0) decides on the latest review at the pressed head whose **final non-blank line** begins with `GATE:`, and requires it to be exactly `GATE: PASS` or `GATE: HOLD`. A human or agent auditing whether a clearance exists reads the **first** line, because that is where the head is named and where rule 44's two counterexamples (`DOCS-GATE: CHANGES`, `DELTA RE-GATE: PASS @ a4566e29`) put it. **The failure is silent and it falls on the clear-PASS case specifically:** a good verdict naturally ends on its analysis or its residuals, which begins with no sentinel, so the press refuses at **exit 9** and a cleared pull request simply sits — nothing in the review UI reports that the stamp was unreadable, and the gate that wrote it has no signal at all. So the requirement is conjunctive, not a choice: **first line names the head and declares the gate's verdict, blank line, body, then the credit line, then the final non-blank line is the bare sentinel.** That ordering is a ruling, not a deduction (connector-ops, 2026-10-09, pod message 76945), and it resolves a second contradiction this rule would otherwise create. **Scope boundary against the credit rule:** `CLAUDE.md`'s "Credit every GitHub artifact" requires every PR comment to *end* with `Written by , a Commonly agent` plus a pod link, which claims the same line the sentinel does — on a gate verdict they cannot both be last. The ruling gives the last line to the press, because it is the brittle machine reader and the one whose failure blocks a merge, and puts the credit line immediately above it, where attribution survives intact. The two alternatives were rejected for stated reasons: exempting verdicts from the credit rule loses attribution on the documents that matter most, and teaching the press to find a `GATE:` line anywhere loosens the one parse that guards merges. Verify the sentinel against what GitHub **stored** rather than the file you wrote, since that is what the press parses — and verify **your** review rather than the newest one. `--jq 'last | …'` is unsound here for the reason rule 44 already gives: every seat posts under one GitHub identity, so `last` returns whichever seat reviewed most recently, and another seat posting between your submission and your readback can supply a sentinel while yours lacks one. Select by id and read the body and the head together, so neither can be right about a different review than the other: +51. **A verdict is read by two different readers at two different ends of the body, so it has to satisfy both — the press parses the FINAL non-blank line, and a clearance audit reads the FIRST.** Rule 44 establishes that `state` carries no verdict here (every review is `COMMENTED`), so the verdict lives in the body text; what it does not say is *which end*, and the two readers disagree. The automated press (`merge-q2` step 0) decides on the PR's latest review, at any of its heads since rule 44's 2026-10-31 amendment, whose **final non-blank line** begins with `GATE:`, and requires it to be exactly `GATE: PASS` or `GATE: HOLD`. A human or agent auditing whether a clearance exists reads the **first** line, because that is where the head is named and where rule 44's two counterexamples (`DOCS-GATE: CHANGES`, `DELTA RE-GATE: PASS @ a4566e29`) put it. **The failure is silent and it falls on the clear-PASS case specifically:** a good verdict naturally ends on its analysis or its residuals, which begins with no sentinel, so the press refuses at **exit 9** and a cleared pull request simply sits — nothing in the review UI reports that the stamp was unreadable, and the gate that wrote it has no signal at all. So the requirement is conjunctive, not a choice: **first line names the head and declares the gate's verdict, blank line, body, then the credit line, then the final non-blank line is the bare sentinel.** That ordering is a ruling, not a deduction (connector-ops, 2026-10-09, pod message 76945), and it resolves a second contradiction this rule would otherwise create. **Scope boundary against the credit rule:** `CLAUDE.md`'s "Credit every GitHub artifact" requires every PR comment to *end* with `Written by , a Commonly agent` plus a pod link, which claims the same line the sentinel does — on a gate verdict they cannot both be last. The ruling gives the last line to the press, because it is the brittle machine reader and the one whose failure blocks a merge, and puts the credit line immediately above it, where attribution survives intact. The two alternatives were rejected for stated reasons: exempting verdicts from the credit rule loses attribution on the documents that matter most, and teaching the press to find a `GATE:` line anywhere loosens the one parse that guards merges. Verify the sentinel against what GitHub **stored** rather than the file you wrote, since that is what the press parses — and verify **your** review rather than the newest one. `--jq 'last | …'` is unsound here for the reason rule 44 already gives: every seat posts under one GitHub identity, so `last` returns whichever seat reviewed most recently, and another seat posting between your submission and your readback can supply a sentinel while yours lacks one. Select by id and read the body and the head together, so neither can be right about a different review than the other: ```sh gh api repos///pulls//reviews \ diff --git a/fleet-skills/working-in-turns/SKILL.md b/fleet-skills/working-in-turns/SKILL.md index ba5ae05622..1fc2c747ac 100644 --- a/fleet-skills/working-in-turns/SKILL.md +++ b/fleet-skills/working-in-turns/SKILL.md @@ -2,24 +2,26 @@ name: working-in-turns description: Keep each turn bounded, checkpointed, and reviewable. user-invocable: false -last_updated: 2026-10-24 +last_updated: 2026-10-31 --- # Working in Turns - Size each turn to finish well inside its run timeout. The default is 15 minutes; only adapters that honor `COMMONLY_AGENT_RUN_TIMEOUT_MS` can use a longer value such as `2700000` (45 minutes). If the work is larger, split it and say how in the row. - Push a checkpoint commit before the final five minutes of the run timeout. A draft PR is enough; a timeout sends SIGTERM and loses uncommitted work. -- Put every gate ask on the PR with the exact head and in the pod. -- Ask reviewers in a pod they actually belong to. -- After a HOLD, fix it, push the new head, and re-ask on the PR. -- File every verdict as described in [review checklist rule 44](https://github.com/Team-Commonly/commonly/blob/main/docs/development/review-checklist.md#what-a-clearance-can-bind) and [rule 51](https://github.com/Team-Commonly/commonly/blob/main/docs/development/review-checklist.md#where-a-verdict-is-readable): the first line must name the exact head and verdict, and the final non-blank line must be the bare `GATE: PASS` or `GATE: HOLD` sentinel. +- **One review round per change.** Before you hand a PR to the press, run your own quick subagent review of its diff. Tier 2 changes (auth, grants, record, schema, wake, money, security) need an Opus-class reviewer. That one review also covers screenshots against the design frame, any lane checks and the zh-CN glossary. Don't ask a second reviewer, and don't ask anyone to re-review after you push a fix. +- Then answer every CodeRabbit thread and get CI green. A push that only answers CodeRabbit or CI keeps your PASS. +- If your review comes back HOLD, fix it, push, and review again yourself. +- File the verdict as described in [review checklist rule 44](https://github.com/Team-Commonly/commonly/blob/main/docs/development/review-checklist.md#what-a-clearance-can-bind) and [rule 51](https://github.com/Team-Commonly/commonly/blob/main/docs/development/review-checklist.md#where-a-verdict-is-readable): the first line names the head you reviewed and the verdict, and the final non-blank line is the bare `GATE: PASS` or `GATE: HOLD` sentinel. - After filing a verdict, choose a checkout strategy: inspect and remove/prune a temporary PR checkout only after checking for uncommitted work, or keep one reusable checkout and clean/refetch it for the next review. - Never use `git worktree remove --force` before checking for uncommitted work. ## Gate ask template -Address the reviewer in a pod they belong to and include the PR number, exact -head SHA, files or properties to check, and the required verdict form: +Use this only when you can't run your own subagent review, for example a +seat with no shell. Ask one reviewer, in a pod they belong to, and include the +PR number, exact head SHA, files or properties to check, and the required +verdict form: > Please review PR # at ``. Check ``. > File the verdict as a PR review. Its first line must be `HEAD: