Skip to content

gate: accept MEMBER/selfReview/admin PASS when GitHub stamps CONTRIBUTOR #936

Closed

Description

@hsliuustc0106

Problem

A tip-named Verifier PASS can be discarded when GitHub reports author_association: CONTRIBUTOR even for a repository admin (seen on #925). Listing the login in selfReview (#926) does not clear that stamp. Org repos do not report OWNER; MEMBER is the org bar.

Wanted

enqueue-guard / merge-gate / review-gate shared decide should accept a tip PASS when any of:

  • association is OWNER, MEMBER, or COLLABORATOR
  • login is in selfReview
  • collaborator permission API reports admin for that login

Still refuse a bare CONTRIBUTOR. Admin must not skip the reviewers list or the own-author rule. Permission lookup only for Tip+Verdict conclusions by a login in reviewers that association/selfReview do not already qualify; fail closed on 403/404.

Tracking

Implemented on draft #935 (tip 861b845). Out of scope: #933.

Activity

  1. Birfy commented on Oct 24, 2026

    @Birfy
    Collaborator

    Triage: the need is real, but one of the three qualifications must not ship. Line numbers on main 7be4e9b7.

    The need, confirmed

    author_association is GitHub's stamp about the commenter's relationship to the repository, and it says CONTRIBUTOR for a login that plainly holds admin on this repository (seen on #925: the tip PASS was discarded, and selfReview did not clear it). Accepting OWNER/MEMBER/COLLABORATOR is right, MEMBER is the org bar, and a permission-API check for a repository admin is the only way to recover the truth GitHub did not put in the stamp. Keep that.

    What must not ship: selfReview qualifying other people's pull requests

    selfReview is documented in .github/agent/review-policy.json as the own-author exception — "A pull request's own author does not count unless the author is listed in selfReview". #928/#931 shipped it as exactly one narrow carve-out for that case (review-gate.mjs:166-168 on 7be4e9b7: selfOnly = selfReview.has(login(r)) && login(r) === authorLogin), with a pinned negative case. Making the same list qualify any pull request keeps the name but changes the meaning: it becomes a second reviewers list, keyed on a list the policy file defines as something else, and it makes the #931 carve-out untestable (the negative case disappears).

    Note it is also not needed for this issue: hsliuustc0106 is in reviewers, and the admin-permission path is what recovers his PASS on #925 — not selfReview.

    Requirements

    1. Association qualifies on OWNER/MEMBER/COLLABORATOR (review-gate.mjs:36).
    2. The own-author selfReview carve-out stays exactly as merged (:166-168), negative case included.
    3. A repository admin qualifies a CONTRIBUTOR, asked once per such conclusion, only for a login already in reviewers, only for a Tip+Verdict report, fail closed on 403/404/HTML — and never called when the tier needs no PASS.
    4. Nothing else moves: the Written-by check, the own-author rule (:183), the reviewers list (:184).

    Acceptance

    1. Replay of Fix mock admin @all and in-place compaction in smoke (#917) #925: a tip PASS by a login in reviewers whose association is CONTRIBUTOR and whose permission is admin → counts: true.
    2. The fix(gate): count a listed self-reviewer's PASS on its own pull request #931 negative case stays green unchanged: a selfReview login, CONTRIBUTOR, not an admin, judging someone else's pull request → counts: false, same reason text as today. The test is not rewritten to expect the new behaviour.
    3. An admin who is the pull request's author and is not in selfReview → own PASS still false.
    4. An admin who is not in reviewers → still false (the platform-admin case must not open the reviewers list).
    5. permission: admin and permissions.admin: true both count; 403, 404, a non-JSON body and a write/triage/maintain permission do not.
    6. adminsFor is not called when the result is exempt (T0/T1), nor for a report that names no tip or carries no verdict.
    7. The three scripts share decide: the same behaviour is asserted in review-gate.test.mjs, merge-gate.test.mjs and enqueue-guard.test.mjs.

    #935 currently implements requirement 2 the other way round (it deletes the own-PR restriction at review-gate.mjs:166-168 and rewrites the #931 test to expect true). It must drop that branch before it is judged; I noted the exact change on the pull request.

    Written-by: Triage

  2. added
    agent-prPull request opened by an agent
    area:automationScheduled tasks and automation
    enhancementNew feature or request
    on Oct 24, 2026
  3. added 2 commits that reference this issue on Oct 24, 2026
    031d1e8
    f8d7ddc
  4. added a commit that references this issue on Oct 24, 2026
    381b94c
  5. added a commit that references this issue on Oct 24, 2026
    2288844
  6. removed
    agent-prPull request opened by an agent
    on Oct 24, 2026
  7. added a commit that references this issue on Oct 24, 2026
    c812e5d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:automationScheduled tasks and automationenhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions