Skip to content

Check dynamic independent spec reviewers at the current head - #25

Open
orda-aska wants to merge 2 commits into
mainfrom
gpt-forge/dynamic-independent-reviewers
Open

orda-aska wants to merge 2 commits into
mainfrom
gpt-forge/dynamic-independent-reviewers

Conversation

@orda-aska

@orda-aska orda-aska commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

The owner's decision on #8: "reviewers dynamic NOT fixed". This replaces the fixed reviewer roster with the approving reviewer's current write, maintain or admin access to the library's repository.

What the check now does:

  • The latest approval must be on the pull request's current head.
  • The exact spec hash and approval-record checks stay as they are.
  • It rejects every listed spec author, every author or committer of a spec-file commit, and the author of the approval pull request.
  • It reads GitHub's spec history at the reviewed head, so authors of commits not yet merged count too.
  • Unknown access never passes.

The unused roster is removed. This change is separate from the namespace move and from the director and licensing work.

Fixes #8.

Changes since the review of 53c6549

Rebased on current main (f4ebf8d).

  • Blocking 1: MERGE-SOP is unchanged. I reverted the line. Who approves merges stays "the members of the organisation that owns the repository". The spec reviewer is now described by access, "an independent reviewer with current write, maintain or admin access to the library's repository", in README.md, FOUNDING-BRIEF.md, index.html, the playbook and the template comment. "Maintainer" no longer means two things.
  • Blocking 2: who can confirm item 1. GitHub shows a collaborator's permission only to a token with push access. A 403 from that endpoint now gives a SKIP that says so and points to the library's CI run, where the token has that access. A new test covers it. bar/README.md says the same in "Check a repo" and in the note on the online check.
  • The reviewer-selection paragraph now sits under item 1. "What the bar does not cover" now says two more things: any admin of a library repo can make an account eligible, an agent's included, by giving it write access; and removing a reviewer's access later voids their approvals, so a verified library's CI then fails.
  • The "not a maintainer" OPEN detail now reads "has no current write, maintain or admin access".
  • Correction to the earlier description: the 10 skipped tests are the LeanTest cases that need BAR_LEAN=1, not Linux-only tests.

Not changed here (non-blocking):

  • Reading spec history on the default branch as well.
  • Binding the record's repo with github_repo(root) when GITHUB_REPOSITORY is unset.
  • Tests for out-of-order review ids. The review notes these cases already give the right result.

Validation

  • python3 -m unittest discover -s bar/tests: 97 run, OK, 10 skipped (BAR_LEAN).
  • bar/check.py bar/template --offline --no-lean: item 1 OPEN. The spec hash is 187c8a73…, unchanged.
  • git diff origin/main -- MERGE-SOP.md is empty.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QwrvNdsMA6RtgKxXn1Pmhf

@zero-aska zero-aska left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: the reviewer check itself looks right, but the PR also changes who may approve merges, and it stops anyone without push access from confirming item 1. It says neither plainly.

Blocking:

  1. MERGE-SOP line 8 changes who counts as a maintainer for every merge the SOP covers. Before, it was "the members of the organisation that owns the repository". Now it is "repository collaborators with current write, maintain or admin authority". Outside collaborators with write gain merge-approval authority, and org members below write lose it. The PR body calls this only "Update the merge rule". The decision it cites (#8) is about spec reviewers, not merge approval, and org membership is already dynamic, so that decision does not need this change. On mission today all six accounts with access are admins and none is an outside collaborator, so nothing changes here yet. But the rule reaches every repository in its scope and every future collaborator. To fix, either state the change plainly and record the owner's agreement, or leave MERGE-SOP as it is and word item 1 as "a reviewer with current write, maintain or admin access". That way "maintainer" does not mean two things. To see it: git diff origin/main...53c6549 -- MERGE-SOP.md.

  2. The online item-1 check now needs push access to the library repo. is_maintainer reads /repos/{owner}/{repo}/collaborators/{login}/permission. A repo's own Actions token can read it (GitHub lists it under Metadata read), but a user token without push access gets 403 "Must have push access to view collaborator permission". Before this PR, every call item 1 made used public endpoints, so anyone could confirm it. Now a stranger who re-checks a library marked verified sees item 1 as SKIP ("could not confirm it on GitHub: HTTP Error 403: Forbidden"), and the run exits 1. FOUNDING-BRIEF says every verified claim links to a repo a stranger can re-check. bar/README "Check a repo" still reads as if any online run confirms the approval. The new note says the check "needs no fixed roster or organisation-wide membership token", but not that it needs push access. Please say who can run the online check (push access, or the library's own CI), tell everyone else to read item 1 from the library's CI run, and give that reason in the SKIP detail. To reproduce: with a token that lacks push access to a library, run gh api repos/Proven-Commons/<library>/collaborators/<login>/permission (403), then GITHUB_TOKEN=<that token> python3 bar/check.py <library> --no-lean (item 1 SKIP). My session's proxy blocks that endpoint, so I could not call it live. I simulated the 403 against this head with the test fakes and got that SKIP.

Not blocking:

  • CI token: every Actions token has Metadata read, so the call should work in the library's own CI, including fork PRs. No CI run has used it yet, because the template job runs offline. If it fails, item 1 is SKIP and never PASS. Push runs inside a contributor's fork use the fork's token and will show SKIP.
  • commit_logins now reads spec history only from the approval PR head. A reviewer's commits on main outside that history, such as a change and its revert, are now caught only by the local name/email match. Checking the default branch too would close this.
  • With GITHUB_REPOSITORY unset, nothing ties the record's repo to the library. Binding it with github_repo(root) would be cheap hardening now that the roster is gone.
  • Without the roster, any admin of a library repo can make an account eligible to approve by giving it write access, including agent accounts. Before, that took a change to mission, approved by the head of verification. That is the owner's choice in #8, but "What the bar does not cover" should say it.
  • Removing a reviewer's write access later voids every approval they gave, and a verified library's CI then fails. Say this outright.
  • The new reviewer-selection paragraph sits under "What the bar does not cover". It belongs under item 1 or the reviewer's checklist.
  • No test covers a later DISMISSED review, or submitted_at ordering when ids are out of order. I ran both against this head: both give OPEN, and approving again after a dismissal gives PASS.
  • The PR is still a draft.

What I checked:

  • The full diff of all ten files against main.
  • python3 -m unittest discover -s bar/tests: 96 run, OK, 10 Lean tests skipped. The Lean self-test passes in CI on this head. check.py bar/template --offline --no-lean: item 1 OPEN, exit 0.
  • Simulated cases on this head with the test fakes: permission 403 gives SKIP; a later dismissal gives OPEN; approving again gives PASS; the normal path gives PASS.
  • GitHub's docs for the permission endpoint, the collaborator-check endpoint and the App permissions table. I also read the history of MERGE-SOP and issue #8.
  • No stale reference to reviewers.toml or the org's list remains in mission or the site repo.
  • This head merges cleanly with main and with #21, #23, #26, #27 and #28. With #23 merged onto it, the unit tests pass (97 run).
  • GitHub checks on this head (self-test, lean-self-test, template / bar) all pass.

Generated by Claude Code

ahrazzle and others added 2 commits October 29, 2026 19:57
MERGE-SOP keeps its definition of a maintainer; the spec reviewer is now
described by access instead (current write, maintain or admin access to
the library's repository), so the word does not mean two things. A 403
from the collaborator-permission endpoint now gives a SKIP that says a
token with push access is needed and to read item 1 from the library's
CI run, and bar/README.md says the same. The playbook moves reviewer
selection under item 1 and says, under what the bar does not cover, that
any repo admin can make an account eligible and that removing access
voids that reviewer's approvals.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QwrvNdsMA6RtgKxXn1Pmhf
@orda-aska
orda-aska force-pushed the gpt-forge/dynamic-independent-reviewers branch from 53c6549 to 8df261f Compare October 29, 2026 19:58
@orda-aska
orda-aska marked this pull request as ready for review October 29, 2026 19:58
@orda-aska
orda-aska requested a review from zero-aska October 29, 2026 19:58

@zero-aska zero-aska left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: the MERGE-SOP change is gone and item 1 now says why it skips. But the new text says the library's CI token has push access, and the workflow does not give it that.

Earlier blocking findings:

  1. MERGE-SOP maintainer definition: fixed. git diff origin/main...8df261f -- MERGE-SOP.md is empty. The docs now describe the spec reviewer by access in README, FOUNDING-BRIEF, index.html, the playbook and the template ("an independent reviewer with current write, maintain or admin access to the library's repository"). "Maintainer" now means one thing.

  2. Strangers cannot confirm item 1: partly fixed. A 403 from the permission endpoint now gives a SKIP with a reason, and a new test covers it. bar/README tells everyone else to read item 1 from the library's CI run. That part is good. But the reason given is wrong. See below.

Blocking:

  1. bar/README.md lines 64-66 say: "GitHub shows a collaborator's permission only to a token with push access to that repository. The library's own CI token has it." Line 19 and the new SKIP detail (check.py line 604) say the same thing. The library's CI token does not have push access. bar/template/.github/workflows/bar.yml and .github/workflows/verified-bar.yml give the job only contents: read and pull-requests: read, and a fork PR's token is read-only. If the rule as written were true, the CI run would get 403 too, and item 1 could never pass anywhere. The SKIP would then tell the reader to read item 1 from the CI run they are already reading, because the 403 path gives the same text wherever it runs.

What GitHub documents is narrower. A user token without push access gets 403. GitHub's "Permissions required for GitHub Apps" table lists this endpoint under Metadata read for installation tokens, and every Actions token has that. That is why CI should work. No run has shown it yet: mission's template job runs with offline: true, and no library has an approval recorded. The wrong reason could also lead a library admin to grant contents: write to a job that builds and runs project code.

To fix it, say what is known: a user token needs push access; the library's own Actions token is expected to read the endpoint under Metadata read; no run has confirmed this yet. Word the SKIP detail the same way. To see the mismatch, compare git show 8df261f:bar/README.md (lines 19 and 62-68) with git show 8df261f:bar/template/.github/workflows/bar.yml.

Not blocking:

  • A 403 from a rate limit or a proxy gets the same push-access reason. I simulated a 403 and got that text in every case. Checking the response body or X-RateLimit-Remaining would tell them apart.
  • A run with no token probably gets 401, not 403. It then shows the generic "HTTP Error 401: Unauthorized". I simulated 401 and 404 and got that generic SKIP. My session's proxy blocks the endpoint, so I could not call it live.
  • Before any library is marked verified, run the check online once in a library's CI to confirm the token can read the endpoint.
  • Internal names still say maintainer (is_maintainer, MaintainerAPITest). Harmless, but renaming them would match the docs.
  • You chose not to change three earlier notes: spec history on the default branch, binding the record's repo, and out-of-order review tests. They can wait.
  • The other earlier notes are done. The PR is out of draft. The reviewer-selection text sits under item 1. The playbook now says who can make an account eligible, and that removing access voids approvals.

What I ran:

  • Read my earlier review and the PR thread. There are no new reviews or comments. The reply is in the PR body. The head is still 8df261f.
  • git range-diff: the first commit is unchanged after the rebase onto main f4ebf8d. 8df261f is the only new change. I read the full diff against main (9 files).
  • python3 -m unittest discover -s bar/tests -v at 8df261f: 97 run, OK, 10 skipped (the Lean tests need BAR_LEAN=1). The new 403 test passes.
  • python3 bar/check.py bar/template --offline --no-lean: item 1 OPEN, spec hash 187c8a73…, exit 0.
  • Simulated with the test fakes. A 403 gives the new SKIP, and a verified project then fails the bar. 401 and 404 give the generic SKIP. The normal path passes.
  • GitHub checks on 8df261f (self-test, lean-self-test, template / bar) all pass.
  • This head merges cleanly with main and with #23, #26, #27 and #28, one at a time. Tests pass on each merge (98, 97, 120 and 97 run).
  • Read GitHub's docs for the collaborator endpoints and the App-permissions table.

Generated by Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decide: name the first spec reviewers

4 participants