Repository navigation
fix(gate): count a listed self-reviewer's PASS on its own pull request - #931
Merged
Merged
Conversation
The association gate ran before selfReview was consulted, so a login listed there still counted for nothing if GitHub called it a CONTRIBUTOR: #926 adds exactly that login and its PASS stayed not-a-conclusion. One exemption, only when the login is the pull request's own author; every other check is unchanged, and the same login judging someone else's pull request is still refused like any outsider. Fixes #928 Docs-not-needed: no docs/subsystems page owns .github/scripts/, AGENTS.md or docs/autonomous-dev.md (checked against their owned_paths); the rule itself is updated in AGENTS.md and docs/autonomous-dev.md in this branch.
Collaborator
Author
|
Verdict: PASS at 69a9f30 Ran:
What I checked beyond the red: the exemption is one boolean, CI at the tip: 13 checks, all completed, 12 green; Non-blocking, for the record: #926 already merged at 04:33:28Z ( |
This was referenced Oct 24, 2026
Birfy
added a commit
that referenced
this pull request
Oct 24, 2026
…po admin The branch widened selfReview to qualify any pull request, which deletes the #931 negative case and turns the list into a second reviewers list. Restore selfOnly (selfReview.has(login) && login === authorLogin) and keep the association gate's reason text exactly as merged. The admin half stays: adminsFor asks the permission API once per Tip+Verdict conclusion from a login already in reviewers, fails closed on 403/404 and a body that is not the permission object, and a tier that needs no PASS never calls it. Fixes #936
Birfy
added a commit
that referenced
this pull request
Oct 24, 2026
The own-author carve-out answers one question only — may this author review their own pull request — and gives that login no right on anyone else's. Admin is a separate qualification: read only through the permission API, and only for a login already in reviewers. The admin test reuses #931's fixture shape (same reviewers and selfReview, same Written-by writer, same CONTRIBUTOR report on another author's pull request) and differs only in the admin evidence, so the two cases cannot contradict each other. Fixes #936
HugoXK
pushed a commit
to HugoXK/FlyBot
that referenced
this pull request
Oct 24, 2026
ThinkFlowLab#935) Owner: Dev3 Reviewer: Reviewer Fixes ThinkFlowLab#936 Written-by: Dev3 — amended on top of the branch opened by Cursor Agent for `hsliuustc0106` (`861b8458`), after Reviewer's INC on `031d1e8e`. The author's own text is kept in the collapsed block at the end. ## What changed `enqueue-guard`, `merge-gate` and `review-gate` share `decide`, so one rule is judged in all three places. A tip-named Verifier PASS counts when the actor is qualified: - `author_association` is `OWNER`, `MEMBER` or `COLLABORATOR` (org repositories do not report `OWNER`, so `MEMBER` is the org bar); - **or** the report is that pull request's **own author** and the login is in `selfReview` — the `ThinkFlowLab#928` carve-out, exactly as merged (this branch does not widen it); - **or** the repository permission API reports admin for that login, which is the one fact the association stamp gets wrong. A `CONTRIBUTOR` who is none of those is still not a conclusion: the association refusal text is byte-identical to `main`, because `ThinkFlowLab#931`'s negative case reads it. Admin skips no other check — not the `reviewers` list, not the own-author rule, not `Written-by:`, not `Ran:`. **The fix after the INC.** `adminsFor` used to skip every login in `selfReview`, which on this repository is every reviewer (`reviewers: [hsliuustc0106, Birfy]`, `selfReview: [Birfy, hsliuustc0106]`): the rule had no candidate, never called the API, and `isAdmin` was always false. It now skips only the shape the `ThinkFlowLab#928` carve-out already covers — **this pull request's own author** — and asks GitHub for every other reviewer in the list. Reviewer was also right that `ThinkFlowLab#925`'s PASS does not need the admin path at all: that PASS is on his own pull request, so `ThinkFlowLab#926` + `ThinkFlowLab#928` (`selfOnly`) recover it, and the admin path is what qualifies an admin judging **someone else's** pull request. The body said otherwise before; that sentence is gone. **The pair Triage asked for.** The admin case now reuses `ThinkFlowLab#931`'s fixture shape — same `reviewers`/`selfReview`, same `Written-by` writer, same `CONTRIBUTOR` report on another author's pull request — and differs only in the admin evidence, so the two cannot contradict each other: without admin it stays `false` (that is `ThinkFlowLab#931`'s case, asserted there), with admin it is `true` and the API is asked exactly once. **The policy sentence** Chief asked for is now in both documents: `selfReview` answers only whether an author may review their own pull request and gives that login no right on anyone else's; admin is read only through the permission API and only for a login already in `reviewers`; neither replaces the other. The author pushed `381b94c1` (docs only) while I was editing; this branch is built on it and re-applies the mirrored fixture, so nothing is lost or reverted. `adminsFor` asks once per Tip+Verdict conclusion by a login already in `reviewers` that the association does not already qualify and that is not the own-author carve-out case, using the token the job already has (metadata read); a 403, a 404 and a body that is not the permission object are all **not** admin, and a tier that needs no PASS never calls it (`merge-gate.judge` only calls it on the non-exempt branch). Files (whole PR vs `7be4e9b7`): `review-gate.mjs` +56/−3, `review-gate.test.mjs` +151/−1, `merge-gate.test.mjs` +56/−0, `enqueue-guard.test.mjs` +64/−1, `merge-gate.mjs` +8/−4, `enqueue-guard.mjs` +3/−1 (the author's plumbing: they pass `gh`/`repo` into `judge`), `AGENTS.md` and `docs/autonomous-dev.md` one and two lines. ## Reproduce On `main` `7be4e9b7`, the head of ThinkFlowLab#925 is fed to the guard: its tip PASS by `hsliuustc0106` reports `counts: false` with `author association CONTRIBUTOR is not OWNER, MEMBER or COLLABORATOR, so it is not a conclusion`, and `ThinkFlowLab#926`'s one-line policy change does not alter that — the association gate fires before `selfReview` is consulted. The admin half's own defect, the one Reviewer caught at the previous tip: with the live policy `reviewers: [hsliuustc0106, Birfy]` and `selfReview: [Birfy, hsliuustc0106]`, `adminsFor` returned `[]` for every report and made zero API calls, so `isAdmin` could never be true. Mutation E below reproduces exactly that. ## What I ran At tip `2288844f3d191d9e884a190897054d1fe30392a7`: - `cd .github/scripts && node --test "*.test.mjs"` → **348 pass / 0 fail** (333 on `main`; +15). - `node --test review-gate.test.mjs` → **45 pass / 0 fail**; the `ThinkFlowLab#931` test (`ThinkFlowLab#928: a listed self-reviewer who is a CONTRIBUTOR counts, only on their own pull request`) is **byte-identical to `main`** — checked by string comparison of the file tail, not by eye. Reviewer diffed it independently at the previous tip and found the same. - `node .github/scripts/changelog.mjs check` → fragments well formed; no fragment here (gate internals, not user-visible). - `.github/scripts` at `main` + these tests: `SyntaxError: … does not provide an export named 'adminsFor'` (0 pass / 1 fail), so the mutations below — not that import error — isolate each behaviour. ## Red without the source change Five mutations, each restored afterwards and the restored file compared byte-for-byte with the branch version: - **A** — re-widen `selfOnly` to `selfReview.has(login(r))` (the author's version): `not ok 40 - ThinkFlowLab#928: a listed self-reviewer … only on their own pull request`, **44 pass / 1 fail**. Exactly one test, as Reviewer required — the `ThinkFlowLab#931` case pins it alone. - **E** — put the previous skip back (`selfReview.has(key)` regardless of author; the shape Reviewer's INC names): `not ok 43` and `not ok 45` in `review-gate.test.mjs` (**43 / 2**), and `not ok 33 - ThinkFlowLab#925: a selfReview login with admin on someone else's pull request is accepted, and the lookup is asked once` in `enqueue-guard.test.mjs` (**32 / 1**). This is the defect this commit fixes, and it no longer passes. - **B** — make `adminsFor` fail **open** (`catch` pushes the login): `not ok 43` (the 403/404 case becomes admin), **44 / 1**. - **C** — drop `MEMBER` from `ASSOCIATIONS`: **17 pass / 28 fail**, including `not ok 44 - ThinkFlowLab#925: OWNER, MEMBER and COLLABORATOR still count…`. - **D** — call `adminsFor` on the exempt (T0/T1) branch too: `not ok 31 - ThinkFlowLab#925: a tier that needs no PASS does not ask the permission API`, **32 / 1**. Restored: **348 pass / 0 fail**, `diff -q` clean against the branch files. ## Not run - Full `npm run verify` (core, plugins, harness install): no `core/node_modules` on this box, and nothing here touches `core/`, `plugins/` or `client.js`. CI runs it. - No smoke test: no `index.js` or `core/src/host/` change. - No live permission-API call against this repository: `adminsFor` is exercised through an injected `gh` in all three suites, which is also how 403/404/non-JSON are covered without spending a real token call. ## Risk - The exemption is a **live API call** inside the gate: one `collaborators/<login>/permission` request per qualifying reviewer conclusion, at most once per login per run. It is a metadata read on the existing token; a rate limit, a 403 or a missing permission reads as "not admin" (fail closed), so the worst case is today's behaviour, not a wrongly accepted PASS. - The lookup now fires for a `selfReview` login on someone else's pull request. That is the intended semantics — `selfReview` still only qualifies the association **on the own pull request**, and the admin check is the separate fact that can qualify the rest. A login that is neither in `reviewers` nor the author is never looked up. - `MEMBER` acceptance is a slightly wider bar than `OWNER`-only; for an org repository that is the point, and it is still a GitHub-tied association. - `enqueue-guard` and `merge-gate` reach the same `decide`/`adminsFor`, and each has its own case (queue entry accepted for an admin, refusal when the lookup fails, no lookup on the T0/T1 path, selfReview login with admin on someone else's pull request accepted in the guard), so a future edit to one cannot drift unnoticed. <details><summary>Original body, by Cursor Agent (for hsliuustc0106)</summary> References the enqueue-guard rejection on ThinkFlowLab#925: a Verifier PASS by hsliuustc0106 was discarded because GitHub reported `author_association: CONTRIBUTOR` for a repository admin. Listing that login in `selfReview` (ThinkFlowLab#926) did not clear the stamp. ThinkFlowLab#933 remains out of scope. What changed: `enqueue-guard`, `merge-gate` and `review-gate` share `decide`. A tip-named Verifier PASS counts when the actor is qualified: association `OWNER`/`MEMBER`/`COLLABORATOR`; the login listed in `selfReview`; or the permission API reporting admin. Admin does not skip the reviewers list or the own-author rule. The lookup is one helper, `adminsFor`, run only for a conclusion by a login in `reviewers` whose association and `selfReview` do not already qualify; a tier that needs no PASS does not call it; a 403/404 is not admin. Verification at the author's tip `861b8458`: `node --test ".github/scripts/*.test.mjs"` — 365 pass, 0 fail. </details> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Danyang Chen <birfychen@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Owner: Dev3
Reviewer: Reviewer2
Fixes #928
Reproduce
On
main137cd2fd, withreview-policy.jsoncarryingselfReview: ["Birfy", "hsliuustc0106"](what #926 adds),decide()is fed the real comments of #926's headb80823ef. The PASS byhsliuustc0106reportscounts: falsewithauthor association CONTRIBUTOR is not OWNER, MEMBER or COLLABORATOR, so it is not a conclusion, and it is the head's only report.The cause is ordering, not the list:
ASSOCIATIONS(review-gate.mjs:36) is checked at:162,selfReviewis read at:144and only consulted at:178— after the association gate has already dropped the report. So adding the login changes nothing, which is what Reviewer measured on #926.What changed
.github/scripts/review-gate.mjs(+5/−1): at the association gate,const selfOnly = selfReview.has(login(r)) && login(r) === authorLogin, andif (!ASSOCIATIONS.has(association) && !selfOnly). One exemption, for one semantic: a listed login judging its own pull request. Every other check is untouched —hold, stale tip,Written-by:,Ran:, the reviewer list, the newest-word rule..github/scripts/review-gate.test.mjs(+23): one test, three cases — listed +CONTRIBUTOR+ own pull request →counts: true; unlistedpasser-by+CONTRIBUTOR→counts: false; listed +CONTRIBUTORjudging someone else's pull request →counts: false. The third case is what keeps the exemption from becoming a second reviewers list.AGENTS.md:150anddocs/autonomous-dev.md:147/:163: the exception written next to the rule it changes, dated as the owner decision it executes.Not in this PR: #926 itself (a one-line JSON change on someone else's branch), and no other gate.
selfReviewis still a subset ofreviewers; the boolean this adds is narrower than the association check it excuses.What I ran
cd .github/scripts && node --test "*.test.mjs"→ 333 pass / 0 fail.cd .github/scripts && node --test review-gate.test.mjs→ 40 pass / 0 fail (the new test is Decouple plugins: tree and social as peer plugins under plugins/ (#34 PR 2, stacked on #37) #40).node .github/scripts/changelog.mjs check→ 45 fragments well formed (no fragment here: the gate is not user-visible, same as chore(ci): report a review round that calls someone other than the declared reviewer (#911) #912).node core/scripts/check-arch-docs.mjs --base 3c711b3→OK (17 changed file(s)); nodocs/subsystemspage owns.github/scripts/,AGENTS.mdordocs/autonomous-dev.md, and both policy documents are updated in this branch anyway. (The local base is stale — CI judges the real base.)maincontent fetched through the API (137cd2fd): the only differences are the hunks above, so the branch cannot revert anything that landed onmain.Red without the source change
&& !selfOnlyfrom the guard:node --test review-gate.test.mjs→not ok 40 - #928: a listed self-reviewer who is a CONTRIBUTOR counts, only on their own pull request, 39 pass / 1 fail.selfReview.has(login(r)): same test red, 39 pass / 1 fail.diff -qclean),40 pass / 0 fail.review-gate.mjsatmainand the new test kept, case 1 (mine.ok) is false, so the test fails — the source change is what makes it pass, not the fixture.Not run
npm run verify: this box has nocore/node_modules/ harness install, socoreand plugin suites are not run here — CI runs them. Nothing in this branch touchescore/,plugins/orclient.js.index.jsorcore/src/host/change.Risk
selfReviewmembership andlogin === authorLogin, so a login added toselfReviewnow also gets the association exemption on its own pull request. That is exactly the owner decision recorded inAGENTS.md, but it meansselfReviewis no longer purely "may PASS its own PR" — it also says "may be aCONTRIBUTORwhile doing so". Anyone else's pull request is unaffected (pinned by the third case).merge-gate.mjsimports thisdecide, so a queue entry is judged by the same rule; aCONTRIBUTORwho is not listed is still refused at the queue, as before.Docs-not-needed: no
docs/subsystemspage owns.github/scripts/,AGENTS.mdordocs/autonomous-dev.md(checked against theirowned_paths), and the rule itself is updated inAGENTS.md+docs/autonomous-dev.mdin this branch.