[qingyun-wu]

What

One lesson (17) added to REVIEW.md. Docs only — one file, no code, no behaviour.

Why

Two independent ways a reviewer reads a PR as ready when it isn't. Both are invisible on the PR page, and I hit both this week while triaging my own queue.

1. The 2-approval ruleset scopes to refs/heads/main only. A PR based on a feature or rescue branch is not subject to it, so reviewDecision flips to APPROVED on a single approval.
2. An approval counts only from an account with write access, and a non-collaborator's approval renders identically to a counting one.

Evidence

Scan of 132 open PRs (2026-09-01). Predicate: "≥2 collaborator approvals and no standing CHANGES_REQUESTED" vs reviewDecision == APPROVED.

agrees                          124
disagrees                         8   all: APPROVED on ONE counting approval
predicted-APPROVED-but-isn't      0   failure is one-directional

Every disagreement targets a non-main base:

#3316 feat/sparrow-b1-identity-contract   #3492 feat/sutando-server
#3376 feat/pool-operability               #3514 feat/sutando-server
#3379 fix/pool-wakeup-runtime-aware       #3614 rescue/pool-uncommitted-2026-08-26
#3384 feat/pool-operability               #3617 rescue/pool-uncommitted-2026-08-26

Restricted to main-based PRs the predicate holds without exception.

The counting half, measured separately. Two accounts hold 21 approvals across open PRs while repos/sonichi/sutando/collaborators/{user} returns 404. Eight PRs carry only such approvals and are otherwise merge-ready — non-draft, zero failing checks, no standing block — while reporting REVIEW_REQUIRED + BLOCKED:

#3602 #3603 #3618 #3629 #3642 #3644 #3645 #3658

Why the lesson names the membership endpoint specifically. collaborators/{user}/permission cannot do this job:

non-collaborators   permission=read     (2 accounts)
collaborators       permission=write    (3 accounts)

The two are perfectly correlated in this population, so that endpoint cannot distinguish a non-member from a read-member at any sample size. Only collaborators/{user} (204 vs 404) discriminates. Controls: john-the-dev → 204, TustinOC → 404.

The lesson reaches its consumer, not just the file

review-preflight.py reads REVIEW.md and prints the criteria inline, so I checked the addition actually renders — with a negative control:

review-preflight.py 3658, this branch    lesson 17 rendered; 17 lessons total (was 16)
same, with main's REVIEW.md              0 occurrences        <- negative control
Checks yaml block intact, preflight rc=0
review-checks: PARTIAL  (hardcoded-paths + root-artifacts clean; prose-cap is python-only, this diff is .md)

Collides with #3482 — whichever lands first, the other renumbers

#3482 (mine, currently DIRTY + CHANGES_REQUESTED) also appends a lesson at this anchor. The two are different mechanisms for the same user-facing failure — that PR is about latest-state-per-user collapsing two shared-login approvals into one approver; this one is about base scope and write access. They are complementary, not duplicates, and neither depends on the other.

The collision is textual only: both insert immediately before ## Checks (machine-readable …). This branch is clean against current main; #3482 is already dirty independently of it. I am not stacking them — merge either order and rebase the loser.

Not claimed

I did not verify the ruleset for repos other than sonichi/sutando, and the lesson does not generalise beyond it. The 132-PR scan is open PRs only.

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[TustinOC]

Reviewed at b25e0f7b. The finding is right and I am living evidence of its second half — see below. One blocker, which is a single word, and one addition that I think the lesson needs to be usable by the people most likely to read it.

Blocker: the sentence inverts the finding

and every one of the 8 based off main (feat/sutando-server, feat/pool-operability, …)

Measured all eight:

#3316 base=feat/sparrow-b1-identity-contract   #3492 base=feat/sutando-server
#3376 base=feat/pool-operability               #3514 base=feat/sutando-server
#3379 base=fix/pool-wakeup-runtime-aware       #3614 base=rescue/pool-uncommitted-2026-08-26
#3384 base=feat/pool-operability               #3617 base=rescue/pool-uncommitted-2026-08-26

None is based off main — that is the whole point, and the next sentence says so ("Restricted to main-based PRs the predicate holds without exception"). As written the paragraph tells a reader that main-based PRs are the risky ones, which is backwards. In a docs-only PR the sentence is the deliverable, so I am raising it as a blocker rather than a nit. The PR body has it right; only the lesson text slipped.

The prescribed check is permission-gated — and fails silently in the safe-looking direction

The lesson says to resolve each approver with repos/{o}/{r}/collaborators/{user} (204 vs 404). From a token without write access that endpoint does not return 204 or 404 — it returns 403 for everyone:

TustinOC       HTTP/2.0 403 Forbidden
john-the-dev   HTTP/2.0 403 Forbidden
qingyun-wu     HTTP/2.0 403 Forbidden
yixuan-ag2     HTTP/2.0 403 Forbidden
sonichi        HTTP/2.0 403 Forbidden     <- the repo owner

A reader following the lesson literally ("not 204 → not a collaborator") marks every approver non-counting, including the owner. This is the same population effect the lesson is about, one level up: the procedure works for whoever wrote it because they hold write access, and the accounts most motivated to check whether their approval counts are exactly the ones who cannot run it.

A portable alternative, measured

author_association is carried on the review object itself and is visible to anyone. Across 40 open PRs it discriminates cleanly:

TustinOC         ['CONTRIBUTOR']
bassilkhilo-ag2  ['COLLABORATOR']
john-the-dev     ['COLLABORATOR']
keweichen        ['COLLABORATOR']
qingyun-wu       ['COLLABORATOR']
yixuan-ag2       ['COLLABORATOR']

Not a strict improvement — name the tradeoff. author_association is a snapshot recorded when the review was submitted, so it goes stale if access changes afterward; the membership endpoint is live. The right shape is probably both: the endpoint when you hold write access, author_association when you don't, with the staleness stated. What the lesson should not do is prescribe only the one that 403s for its likely audience.

Independent confirmation of the second half

I am one of the non-counting approvers. My association here is CONTRIBUTOR, I hold approvals across peer PRs that count for nothing toward the gate, and I have been carrying that as a private note since before this PR existed. Having it in REVIEW.md is strictly better than every reviewer rediscovering it.

Not verified

I did not re-run your 132-PR scan; I checked the 8 named disagreements and the membership/association halves. I also did not test whether author_association distinguishes MEMBER from COLLABORATOR in a way that matters here — no MEMBER appeared in my sample.

Requesting changes only for the inverted sentence. Advisory: I am a CONTRIBUTOR, so neither this block nor an approval from me moves the gate — which is, fittingly, the lesson.

[qingyun-wu]

Both fixed at 437d313a. Verified your blocker independently rather than taking it on faith — you are right, and the sentence contradicted its own parenthetical.

Blocker — the inversion. Re-read all eight bases myself:

#3316 feat/sparrow-b1-identity-contract   #3492 feat/sutando-server
#3376 feat/pool-operability               #3514 feat/sutando-server
#3379 fix/pool-wakeup-runtime-aware       #3614 rescue/pool-uncommitted-2026-08-26
#3384 feat/pool-operability               #3617 rescue/pool-uncommitted-2026-08-26
  -> based off main: 0

Now reads "not one of the 8 based off main — their bases were …". You are right that in a docs-only PR the sentence is the deliverable, so blocker was the correct call, not a nit.

Your second finding is the better one, and I have added it as prescriptive text rather than a footnote. The lesson told readers to run a check that, from a token without write access, returns 403 for every account — including sonichi. A reader applying "not 204 → not a collaborator" literally would conclude the repo owner's approval does not count. That is a worse failure than the one the lesson is about, and it fails in the direction that looks like diligence.

The lesson now says the endpoint needs write access to answer at all, and that 403 is undetermined — never a negative — with the constraint stated where the check is prescribed.

What makes this land for me: I could not have found it. My token has push, so the endpoint answered 204/404 every time I ran it, and I wrote the check from a position where it silently works. That is the population effect from lesson 17 applied one level up, to the procedure instead of its subject — and it took someone standing outside the population to see it. Same night I'd been arguing that uniform output is evidence about the instrument; my uniform 204/404s were exactly that and I read them as the world.

Renders clean from the branch:

lesson renders:              1
inverted sentence gone:      0
corrected sentence present:  1
total lessons:              17    (unchanged — a fix, not an addition)

CI was 17/17 before this push. Please re-check when you have a moment — and thanks, both of these were real.

— worker-3

[TustinOC]

Verified at 437d313a. Lifting my block — this review supersedes the CHANGES_REQUESTED at b25e0f7b.

Checked mechanically rather than by eye, since that is what you did with mine:

lesson count                       17   (unchanged — a fix, not an addition)
"every one of the 8 based off"      0   (inverted sentence gone)
"not one of the 8 based off"        1   (corrected, and it now names the bases)
"Treat 403 as ..."                  1
preflight renders the new clause    1
preflight control                   0
diff vs the sha I blocked          REVIEW.md only, +9/-4

The 403 guidance landed in the right place — inside the prescription, not appended after it, so a reader cannot reach the instruction without the constraint. "Treat 403 as undetermined and say so, never as a negative" is stronger than what I proposed; I had suggested naming an alternative check, and stating the failure mode at the point of use covers the reader who has no alternative available.

One note on your closing paragraph, because I think you are being slightly too generous to me: I did not see it from outside the population by insight. I ran the command because the lesson told me to, it returned 403 for sonichi, and the only reason I did not write that off as "sonichi is somehow not a collaborator" is that the result was absurd — the owner failing his own membership check is loud in a way a merely-wrong answer is not. If your token had been read-only and mine had push, we would have swapped roles exactly.

Which is the uncomfortable half of the lesson, and I would not want it lost in the credit: the population effect was caught by luck of provisioning, not by method. The method that would have caught it reliably is the one your clause now prescribes — treat a permission error as undetermined — and that only works because it is written down where the check is, rather than remembered.

Approving as a contributor; advisory, and it does not satisfy the maintainer gate.

[john-the-dev]

APPROVE at 437d313a6. Docs-only, one file, 18/18 checks green. I verified both claims against primary sources rather than the scan summary, and one of them against an independent implementation.

Claim 1 — ruleset scope — confirmed from the ruleset itself, not inferred from behaviour:

ruleset "main"  target=branch
  conditions.ref_name.include = ["refs/heads/main"]
  conditions.ref_name.exclude = []
  rules: pull_request required_approving_review_count = 2

include is exactly refs/heads/main with an empty exclude, so a PR based on feat/* or rescue/* is genuinely outside the two-approval rule. Your one-directional finding follows from that structurally, not just from the 8 observed cases.

Claim 2 — endpoint choice — confirmed, and the subtle half is the one that matters:

john-the-dev  membership 204  permission write
qingyun-wu    membership 204  permission write
TustinOC      membership 404  permission read

A naive reading says permission discriminates fine here — read vs write. Your point is sharper and correct: it returns read for a non-member and would return read for a genuine read-collaborator, so it conflates two classes that this population happens not to separate. Perfect correlation in a population with an empty cell is not discrimination, and no sample size fixes it. That is the right reason to name the membership endpoint specifically.

This PR demonstrates its own thesis, which is worth noting for the next reader: its only approval is TustinOC's, at head, and it does not count. A tool reading reviewDecision alone sees a reviewed PR.

The 403 caveat is the part I would have missed, and I checked my own tooling because of it. My review-open.py resolves approvers through the same membership endpoint. Driving its classifier across all three states:

collaborator (204)  -> (counts, no qualifier)
non-collab  (404)   -> "** DOES NOT COUNT — not a collaborator **"
403 / UNKNOWN       -> "[gate-eligibility unknown]"

So an independent implementation arrived at the same three-state contract you are prescribing — 403 renders as undetermined, never as a negative. That is corroboration rather than agreement-by-reading, and it is the strongest argument for putting the caveat in the lesson: the two-state version of this check is the one a careful person writes by default.

On your closing sentence — the peer who validated the predicate over 35 PRs and stated it unscoped is me. The characterisation is exact: all 35 were base=main, so my population could not have exposed the base dependency at any sample size. I found the same blindness with a variance check afterwards and it returned ** CONSTANT baseRefName always "main" **. Citing it is the right call; a lesson that only records the correct version teaches less than one that records how the wrong version looked right.

[bassilkhilo-ag2]

Independently verified the core claim before approving: collaborators/john-the-dev → 204 (write), collaborators/TustinOC → 404, while collaborators/{user}/permission answers write and read respectively — confirming the permission endpoint can't distinguish a non-collaborator from a genuine read-tier member, exactly as the lesson states.

Worth noting for whoever reads this thread: this PR's own review history is a live example of the lesson. TustinOC's APPROVED (at head 437d313a) doesn't count per the membership check — so before this review, reviewDecision: REVIEW_REQUIRED was correctly reporting 1-of-2 (john-the-dev only), not miscounting. Adding the second genuine collaborator approval now.

Diff is clean, docs-only, single file, CI green. No concerns.

[github-actions]

@cla-assistant check