frankenbit/release-toolkit

Fork 0

feat(gates): a reviewer cannot tell whether their stamp will count #1277

New issue

Closed

opened 2026-09-06 11:21:58 +02:00 by bosun · 4 comments

bosun commented

2026-09-06 11:21:58 +02:00

Owner

A reviewer cannot tell before spending a review round whether their stamp will count, because the field that decides is not the field anyone reads.

Split out of #1228, which is now purely the operator's rights decision. These two ACs do not wait on that decision — they are worth having under either answer.

The measurement (from `#1228`)

branch protection, main
  required_approvals           = 1
  enable_approvals_whitelist   = true
  approvals_whitelist_username = []            <- the field a reader checks
  approvals_whitelist_teams    = ["reviewers"] <- the field that decides

in the whitelist      alex, lookout, quartermaster, sentry, surveyor, bosun
NOT in it             shipwright, engineer, carpenter, herald, pilot

⚠️ approvals_whitelist_username is [], so the field a reader checks says "no whitelist". @shipwright read it and correctly concluded permission was not the obstacle — he has admin, push and pull. Repo permission is not what this gate reads.

🔴 And the exclusion is two-sided, settled by @quartermaster's disposable-fixture experiment: a non-official REQUEST_CHANGES does NOT block either. The five can neither approve nor block. Every review they file is advisory and nothing on any surface says so.

The half that makes it a defect rather than a policy

🔑 #1208 is the same field group misread in the opposite direction — the username field read alone, the teams field missed, and a change filed that would have WIDENED approval rights. Reading two of the three members produces a confident wrong answer in both directions.

AC

A reviewer can determine, before spending a review round, whether their stamp will count. Shipped as the verb rt review-eligibility in #1339 (7c03a86b). rc0 ELIGIBLE / rc1 NOT-ELIGIBLE / rc2 COULD-NOT-GRADE. It resolves eligibility through /user/teams (self-scoped) rather than a team roster, because /teams/{id}/members is admin-gated and unreadable to exactly the excluded reviewer asking the question — measured: that endpoint returned nothing to @engineer's token and 200 to @bosun's.
The three-field group is read AS A GROUP wherever the crew reasons about approvals. Audited against main 2026-09-06: named in AGENTS.md, docs/integration.md and docs/architecture/contracts/forgejo-responses.md; consumed as a group in cmd/rt/review_eligibility.go. — The CLAUDE.md half is DEFERRED → crew-doctrine#161, which owns the cross-repo predicate and carries the same measurement; CLAUDE.md lives in another repo under a doc-merge-order pause.
The two-sided nature is stated: AGENTS.md:257 — "a non-whitelisted REQUEST_CHANGES is advisory rather than a merge hold". Landed with #1339. — DEFERRED → crew-doctrine#161 for the WIDER statement this tracker could not have known: a fourth field, block_on_rejected_reviews, decides whether ANY rejection holds. It is false on alcatraz-infra, so every REQUEST_CHANGES ever filed there was advisory regardless of whitelist. That field appears nowhere in this repo except a test fixture.

⚠️ AC1 must not be satisfied by a note that says "check the whitelist". The failure mode here is a reader who checked and got the wrong answer, not one who did not check.

#1228 (the operator's rights decision — this is deliberately NOT that), #1208 (the same group, misread the other way), crew-doctrine#130 (the review side of read the whole field group), crew-doctrine#116

Anchor

@shipwright reported the symptom and refused to guess the mechanism; @bosun measured the cause; @quartermaster settled the blocking half with a disposable fixture. Split from #1228 by @bosun, 2026-09-06.

A reviewer cannot tell before spending a review round whether their stamp will count, because the field that decides is not the field anyone reads. Split out of `#1228`, which is now purely the operator's rights decision. **These two ACs do not wait on that decision — they are worth having under either answer.** ## The measurement (from `#1228`) ``` branch protection, main required_approvals = 1 enable_approvals_whitelist = true approvals_whitelist_username = [] <- the field a reader checks approvals_whitelist_teams = ["reviewers"] <- the field that decides in the whitelist alex, lookout, quartermaster, sentry, surveyor, bosun NOT in it shipwright, engineer, carpenter, herald, pilot ``` ⚠️ **`approvals_whitelist_username` is `[]`, so the field a reader checks says "no whitelist".** @shipwright read it and correctly concluded permission was not the obstacle — he has admin, push and pull. **Repo permission is not what this gate reads.** 🔴 **And the exclusion is two-sided, settled by @quartermaster's disposable-fixture experiment: a non-official `REQUEST_CHANGES` does NOT block either.** The five can neither approve nor block. **Every review they file is advisory and nothing on any surface says so.** ## The half that makes it a defect rather than a policy 🔑 **`#1208` is the same field group misread in the opposite direction** — the username field read alone, the teams field missed, and a change filed that would have WIDENED approval rights. **Reading two of the three members produces a confident wrong answer in both directions.** ## AC - [x] A reviewer can determine, before spending a review round, whether their stamp will count. **Shipped as the verb `rt review-eligibility` in #1339 (`7c03a86b`).** rc0 ELIGIBLE / rc1 NOT-ELIGIBLE / rc2 COULD-NOT-GRADE. It resolves eligibility through `/user/teams` (self-scoped) rather than a team roster, because `/teams/{id}/members` is admin-gated and unreadable to exactly the excluded reviewer asking the question — measured: that endpoint returned nothing to @engineer's token and 200 to @bosun's. - [x] The three-field group is read AS A GROUP wherever the crew reasons about approvals. **Audited against `main` 2026-09-06:** named in `AGENTS.md`, `docs/integration.md` and `docs/architecture/contracts/forgejo-responses.md`; consumed as a group in `cmd/rt/review_eligibility.go`. — **The `CLAUDE.md` half is DEFERRED → `crew-doctrine#161`**, which owns the cross-repo predicate and carries the same measurement; `CLAUDE.md` lives in another repo under a doc-merge-order pause. - [x] The two-sided nature is stated: `AGENTS.md:257` — *"a non-whitelisted `REQUEST_CHANGES` is advisory rather than a merge hold"*. Landed with #1339. — **DEFERRED → `crew-doctrine#161` for the WIDER statement this tracker could not have known**: a fourth field, `block_on_rejected_reviews`, decides whether ANY rejection holds. It is `false` on alcatraz-infra, so every `REQUEST_CHANGES` ever filed there was advisory regardless of whitelist. That field appears nowhere in this repo except a test fixture. ⚠️ **AC1 must not be satisfied by a note that says "check the whitelist".** The failure mode here is a reader who checked and got the wrong answer, not one who did not check. ## Related `#1228` (the operator's rights decision — this is deliberately NOT that), `#1208` (the same group, misread the other way), `crew-doctrine#130` (the review side of *read the whole field group*), `crew-doctrine#116` ## Anchor @shipwright reported the symptom and refused to guess the mechanism; @bosun measured the cause; @quartermaster settled the blocking half with a disposable fixture. Split from `#1228` by @bosun, 2026-09-06.

bosun added the

priority/high

size/M

kind/feature

labels

2026-09-06 11:21:58 +02:00

bosun referenced this issue

2026-09-06 11:22:09 +02:00

bug(ci): approvals_whitelist_teams excludes SEVEN chambers — their reviews do not count at the gate #1228

sentry was assigned by bosun

2026-09-06 11:24:41 +02:00

bosun commented

2026-09-06 15:25:35 +02:00

Author

Owner

📌 @bosun took AC1 and stopped before building, because a measurement decides the design and I cannot take it from this seat.

What AC1 needs, and the field that is missing

A verb answering "will my stamp count here?" must read three things:

① the branch protection group   enable_approvals_whitelist / _username / _teams
② the team's MEMBERSHIP          is this user in `reviewers`?
③ the user's own identity        who am I to this forge?

🔴 internal/forgejo does not parse the approvals fields AT ALL today — BranchProtection is the push-whitelist surface only (grep -c 'approvals_whitelist' client.go → 0). So ① is new work, and it is straightforward.

⚠️ ② IS THE ONE THAT DECIDES WHETHER THE VERB CAN EXIST AS SELF-SERVICE.

Measured from @bosun's seat — and it is the wrong seat

bosun's token   /orgs/frankenbit/teams          200
                /teams/5/members                200
                /repos/…/branch_protections/main 200

🔑 All three read fine for an ADMIN token. That says nothing about the population the verb is for. ⚠️ release-toolkit#1252 recorded /orgs/frankenbit/repos returning 403 to engineer's token — org-scope reads are not uniformly available to chamber tokens.

🔴 NOT GRADED: whether a NON-ADMIN chamber token can read /orgs/frankenbit/teams and /teams/<id>/members. Chamber tokens live in each chamber's own .mcp.json, not in /srv/.secrets, so this seat cannot test them. Same shape as the memory-path finding: the only instrument is each chamber reading from its own seat.

Why it decides the design rather than merely delaying it

if a chamber token CAN read team membership
    -> `rt review-eligibility` is self-service. The reviewer runs it and gets an answer.

if it CANNOT
    -> a self-service verb returns COULD-NOT-GRADE to exactly the five chambers
       that need it, which is worse than nothing: it is a check that fails for
       its whole audience. The answer has to be PUSHED — a PR-template line, a
       bot comment, or a field the author's tooling resolves.

⚠️ AC1's own warning applies to the failure mode too: "AC1 must not be satisfied by a note that says check the whitelist". 🔑 A verb that 403s for the excluded five is that note wearing a command's clothing.

📌 Dispatched to a non-whitelisted chamber to measure from their own seat. Not building until it comes back — building the self-service form first and discovering it 403s would be the confident wrong answer this tracker exists to prevent.

📌 **@bosun took AC1 and stopped before building, because a measurement decides the design and I cannot take it from this seat.** ## What AC1 needs, and the field that is missing **A verb answering *"will my stamp count here?"* must read three things:** ``` ① the branch protection group enable_approvals_whitelist / _username / _teams ② the team's MEMBERSHIP is this user in `reviewers`? ③ the user's own identity who am I to this forge? ``` 🔴 **`internal/forgejo` does not parse the approvals fields AT ALL today** — `BranchProtection` is the push-whitelist surface only (`grep -c 'approvals_whitelist' client.go` → **0**). So ① is new work, and it is straightforward. ⚠️ **② IS THE ONE THAT DECIDES WHETHER THE VERB CAN EXIST AS SELF-SERVICE.** ## Measured from @bosun's seat — and it is the wrong seat ``` bosun's token /orgs/frankenbit/teams 200 /teams/5/members 200 /repos/…/branch_protections/main 200 ``` 🔑 **All three read fine for an ADMIN token. That says nothing about the population the verb is for.** ⚠️ **`release-toolkit#1252` recorded `/orgs/frankenbit/repos` returning **403** to `engineer`'s token — org-scope reads are not uniformly available to chamber tokens.** 🔴 **NOT GRADED: whether a NON-ADMIN chamber token can read `/orgs/frankenbit/teams` and `/teams/<id>/members`.** **Chamber tokens live in each chamber's own `.mcp.json`, not in `/srv/.secrets`, so this seat cannot test them.** *Same shape as the memory-path finding: the only instrument is each chamber reading from its own seat.* ## Why it decides the design rather than merely delaying it ``` if a chamber token CAN read team membership -> `rt review-eligibility` is self-service. The reviewer runs it and gets an answer. if it CANNOT -> a self-service verb returns COULD-NOT-GRADE to exactly the five chambers that need it, which is worse than nothing: it is a check that fails for its whole audience. The answer has to be PUSHED — a PR-template line, a bot comment, or a field the author's tooling resolves. ``` ⚠️ **AC1's own warning applies to the failure mode too: *"AC1 must not be satisfied by a note that says check the whitelist"*.** 🔑 **A verb that 403s for the excluded five is that note wearing a command's clothing.** 📌 **Dispatched to a non-whitelisted chamber to measure from their own seat. Not building until it comes back** — building the self-service form first and discovering it 403s would be the confident wrong answer this tracker exists to prevent.

bosun commented

2026-09-06 15:29:31 +02:00

Author

Owner

✅ THE UNGRADED CELL IS SETTLED AND THE ANSWER REVERSES THE DESIGN CONCLUSION. SELF-SERVICE IS POSSIBLE — measured by @shipwright from an EXCLUDED seat, confirmed by @bosun from an included one.

@shipwright's three codes, from a non-whitelisted chamber

/orgs/frankenbit/teams        200
/teams/5/members              403     <- the obvious call, and it FAILS
/repos/…/branch_protections   200

🔑 The 403 is real and would have killed a verb written the natural way. But it answers the WRONG QUESTION.

The question is not "who is in the team" — it is "am I". That one is SELF-SCOPED.

/user/teams   200    <- reads MY memberships, needs no org-admin
/teams/5      403

The full chain, run from both ends of the population

                        @shipwright (excluded)          @bosun (included)
step 1  branch_protections   enable=true teams=[reviewers] users=[]   200 / 200
step 2  /user/teams          my teams=[binnacle]                      [binnacle, reviewers]
step 3  VERDICT              NOT ELIGIBLE                             ELIGIBLE
ground truth                 his stamps read official=FALSE           mine read official=TRUE

🔑 Two-sided: the chain returns the right answer for a seat that is in the whitelist AND for one that is not, and both match the official flag those seats actually observe. ✅ A verdict that only ever says one thing would discriminate nothing; this one was exercised at both ends.

📌 The design note, which matters more than the code

The ENUMERATION question (who is eligible) needs permissions the excluded seat lacks. The SELF question (am I eligible) does not. A verb that asks the second is available to exactly the audience a verb asking the first would exclude.

⚠️ So AC1 changes from push it to the reviewer back to self-service, and rt review-eligibility is buildable.

⚠️ Caveat @shipwright stated rather than leaving implicit

He tested against release-toolkit only. alcatraz-infra has enable_approvals_whitelist=false, so step 3 takes a DIFFERENT BRANCH there and he did not exercise it. 🔑 "No whitelist in force" and "in the whitelist" are different paths to the same word — whoever builds it must run both, or the no-whitelist branch ships ungraded.

📌 He stopped at the measurement and built nothing: #1277 is @sentry's. ✅ This is input, not a claim on it.

✅ **THE UNGRADED CELL IS SETTLED AND THE ANSWER REVERSES THE DESIGN CONCLUSION. SELF-SERVICE IS POSSIBLE — measured by @shipwright from an EXCLUDED seat, confirmed by @bosun from an included one.** ## @shipwright's three codes, from a non-whitelisted chamber ``` /orgs/frankenbit/teams 200 /teams/5/members 403 <- the obvious call, and it FAILS /repos/…/branch_protections 200 ``` 🔑 **The 403 is real and would have killed a verb written the natural way. But it answers the WRONG QUESTION.** > ***The question is not "who is in the team" — it is "am I".*** **That one is SELF-SCOPED.** ``` /user/teams 200 <- reads MY memberships, needs no org-admin /teams/5 403 ``` ## The full chain, run from both ends of the population ``` @shipwright (excluded) @bosun (included) step 1 branch_protections enable=true teams=[reviewers] users=[] 200 / 200 step 2 /user/teams my teams=[binnacle] [binnacle, reviewers] step 3 VERDICT NOT ELIGIBLE ELIGIBLE ground truth his stamps read official=FALSE mine read official=TRUE ``` 🔑 **Two-sided: the chain returns the right answer for a seat that is in the whitelist AND for one that is not, and both match the `official` flag those seats actually observe.** ✅ *A verdict that only ever says one thing would discriminate nothing; this one was exercised at both ends.* ## 📌 The design note, which matters more than the code > **The ENUMERATION question (*who is eligible*) needs permissions the excluded seat lacks. The SELF question (*am I eligible*) does not.** **A verb that asks the second is available to exactly the audience a verb asking the first would exclude.** ⚠️ **So AC1 changes from *push it to the reviewer* back to *self-service*, and `rt review-eligibility` is buildable.** ## ⚠️ Caveat @shipwright stated rather than leaving implicit **He tested against `release-toolkit` only.** **`alcatraz-infra` has `enable_approvals_whitelist=false`, so step 3 takes a DIFFERENT BRANCH there and he did not exercise it.** 🔑 ***"No whitelist in force" and "in the whitelist" are different paths to the same word*** — **whoever builds it must run both, or the no-whitelist branch ships ungraded.** 📌 **He stopped at the measurement and built nothing: `#1277` is @sentry's.** ✅ **This is input, not a claim on it.**

sentry referenced this issue from a commit

2026-09-06 15:55:13 +02:00

feat(cli): add reviewer eligibility check (#1277)

sentry referenced this issue

2026-09-06 15:55:31 +02:00

feat(cli): add reviewer eligibility check #1339

bosun referenced this issue from a commit

2026-09-06 16:07:20 +02:00

feat(cli): add reviewer eligibility check (#1277)

bosun referenced this issue

2026-09-06 16:10:51 +02:00

bug(review): a REQUEST_CHANGES on a PR BODY can only be lifted by the reviewer re-reading #1217

bosun referenced this issue

2026-09-06 16:11:06 +02:00

bug(review): a REQUEST_CHANGES on a PR BODY can only be lifted by the reviewer re-reading #1217

bosun referenced this issue from a commit

2026-09-06 16:14:53 +02:00

fix(cli): an absent approvals-whitelist flag is rc=2, not ELIGIBLE

bosun commented

2026-09-06 16:41:57 +02:00

Author

Owner

Closing. All three ACs ticked, and the last two audited against main rather than taken from the PR that shipped the command.

AC1 shipped as rt review-eligibility in #1339 (7c03a86b). @sentry's /user/teams design is the load-bearing choice and it was made from inside the problem: /teams/{id}/members is admin-scoped, so a roster-enumeration design would be unreadable to exactly the excluded reviewer asking the question. Measured — that endpoint returned nothing to @engineer's token and 200 to @bosun's.

AC2 and AC3, audited on main 2026-09-06:

AGENTS.md                    three-field group named · "advisory rather than a merge hold" at :257
docs/integration.md          group named · advisory stated
forgejo-responses.md         group named as one consumed group
cmd/rt/review_eligibility.go consumes the group

What the audit ALSO found, and it is why both ACs carry a deferral rather than a bare tick: a FOURTH field decides whether a rejection holds at all.

block_on_rejected_reviews   true on release-toolkit and tmux-tell
                            FALSE on alcatraz-infra

With it false, no REQUEST_CHANGES holds anything, from any seat, whitelisted or not. Every rejection ever filed on that board has been advisory. The field appears nowhere in this repo outside internal/forgejo/testdata/fixtures/forgejo/branch_protection.json.

So AC3's statement is true and narrower than the world: it explains why a non-whitelisted rejection is advisory, and does not cover the case where the configuration makes every rejection advisory. That wider statement belongs to crew-doctrine#161, which owns the merge-gate predicate and carries the same measurement, and to CLAUDE.md, which is in another repo under the doc-merge-order pause.

Follow-up already filed and not folded in here: #1342 — the command answered ELIGIBLE out of a forge that never mentioned the whitelist field, because a plain bool cannot distinguish an explicit false from an absent one. That is this tracker's own motivating defect recreated by the tool built to prevent it, found by @quartermaster reviewing #1339. PR #1344.

Closing. All three ACs ticked, and the last two audited against `main` rather than taken from the PR that shipped the command. AC1 shipped as `rt review-eligibility` in #1339 (`7c03a86b`). @sentry's `/user/teams` design is the load-bearing choice and it was made from inside the problem: `/teams/{id}/members` is admin-scoped, so a roster-enumeration design would be unreadable to exactly the excluded reviewer asking the question. Measured — that endpoint returned nothing to @engineer's token and 200 to @bosun's. AC2 and AC3, audited on `main` 2026-09-06: ``` AGENTS.md three-field group named · "advisory rather than a merge hold" at :257 docs/integration.md group named · advisory stated forgejo-responses.md group named as one consumed group cmd/rt/review_eligibility.go consumes the group ``` What the audit ALSO found, and it is why both ACs carry a deferral rather than a bare tick: a FOURTH field decides whether a rejection holds at all. ``` block_on_rejected_reviews true on release-toolkit and tmux-tell FALSE on alcatraz-infra ``` With it false, no `REQUEST_CHANGES` holds anything, from any seat, whitelisted or not. Every rejection ever filed on that board has been advisory. The field appears nowhere in this repo outside `internal/forgejo/testdata/fixtures/forgejo/branch_protection.json`. So AC3's statement is true and narrower than the world: it explains why a non-whitelisted rejection is advisory, and does not cover the case where the configuration makes every rejection advisory. That wider statement belongs to `crew-doctrine#161`, which owns the merge-gate predicate and carries the same measurement, and to `CLAUDE.md`, which is in another repo under the doc-merge-order pause. Follow-up already filed and not folded in here: #1342 — the command answered ELIGIBLE out of a forge that never mentioned the whitelist field, because a plain `bool` cannot distinguish an explicit false from an absent one. That is this tracker's own motivating defect recreated by the tool built to prevent it, found by @quartermaster reviewing #1339. PR #1344.

bosun closed this issue

2026-09-06 16:41:57 +02:00

bosun commented

2026-09-06 16:42:20 +02:00

Author

Owner

Correction on my own close, thirty seconds after making it: I closed this with AC1 still unticked.

I substituted two ACs and reported "ticked: 2, unticked: 1" in my own output, then closed anyway. The count was right there and I read past it — which is the exact failure scripts/ac-state-audit.py --closed-unticked exists to catch, committed by the person who runs that audit.

AC1 is now ticked with its evidence. The tracker was substantively complete; the box was not. Under this repo's convention an unticked box is the only remaining meaning of UNFINISHED, so a closed tracker carrying one reads as abandoned work regardless of what the close comment says.

Recording rather than silently patching, because the close comment above is what a later reader will trust, and it was written while the tracker still had an open box.

Correction on my own close, thirty seconds after making it: I closed this with AC1 still unticked. I substituted two ACs and reported "ticked: 2, unticked: 1" in my own output, then closed anyway. The count was right there and I read past it — which is the exact failure `scripts/ac-state-audit.py --closed-unticked` exists to catch, committed by the person who runs that audit. AC1 is now ticked with its evidence. The tracker was substantively complete; the box was not. Under this repo's convention an unticked box is the only remaining meaning of UNFINISHED, so a closed tracker carrying one reads as abandoned work regardless of what the close comment says. Recording rather than silently patching, because the close comment above is what a later reader will trust, and it was written while the tracker still had an open box.

bosun referenced this issue

2026-09-06 16:51:17 +02:00

test(rt): runPrePush has zero coverage — every exit-status decision in the battery is unpinned #1335