docs(research): repo admin already suffices to require a check; only 5 of 30 checks are safe to require (#358) - #377
Conversation
…5 of 30 checks are safe to require (#358) Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
serina-mcfall
left a comment
There was a problem hiding this comment.
Independent review — no blockers, one non-blocking issue filed
Reviewed in a fresh context. I am an agent and do not approve — posting as a comment so @serina-mcfall can.
Claim (b) reproduced exactly. I re-derived the check sets independently:
- PR #308 (docs-only): 6 distinct successful checks.
- PR #216 (981-file sync): 30 distinct successful checks.
- Intersection:
adr-boundary,check,Dead Token Reference Guard,Detect Changed Paths,scripts— exactly the five the document names.
The denominator and all five names match. The predicate — "succeeded on both a docs-only PR and a 981-file sync PR", i.e. unaffected by ci.yml's path filter — is mechanical and falsifiable, and the document is honest that it is "a sound lower bound on 'always runs', not a proven invariant."
Claim (a) holds on the load-bearing part. Independently confirmed: exactly 4 repo admins (joshuavial, baradev, tucktuck101, jatin-puri-coder), and repos/launchpad-26/buzz/rulesets returns 200 with [] both with and without includes_parents. Both of those read fine without admin, which is the point — no org-level privilege is needed to reach the surfaces that require a check.
Could not check: the specific branch-protection values quoted (approvals: 1, enforce_admins: false, the 11-user restrictions.users list). branches/launchpad/protection and branches/main/protection both 404 for a maintain token, which is what I hold. The document discloses the same limitation, so I am reporting it as unverified rather than inferring past the 404.
Non-blocker, filed as #387: the select(.conclusion=="success") filter absorbs reruns. On PR #216 the check named check has two failure conclusions and a later success, so it appears in both sets — meaning "safe to require" as measured is eventually green, not always green. That distinction matters once a check actually gates a merge.
Worth flagging for a different PR, not this one: this document gets the admin count right at 4. ADR-0019 on #281 — still open — says "five repository admins" and builds an escalation on "five of the eleven". I've raised that on #281 directly.
No contradiction with ADR-0019. Both shapes #377 proposes (requiring the five always-running checks; a launchpad-ci-gate aggregator) are deterministic scripts, not model verdicts.
Correct placement, no private-tooling paths. Ready for approval.
benmitchell11
left a comment
There was a problem hiding this comment.
Reviewed for factual accuracy against the live repo (closes #358).
Independently reproduced with my own gh api calls against launchpad-26/buzz (a different token/user than the PR author, useful as a cross-check):
- Collaborator/admin list:
gh api "repos/launchpad-26/buzz/collaborators?per_page=100"reproduces the exact same 4 admins (joshuavial, baradev, tucktuck101, jatin-puri-coder) and the same maintain/write roles for everyone else, byte-for-byte. gh api repos/launchpad-26/buzz/rulesets -ireturns200 OKwith[]— confirmed.gh api orgs/launchpad-26 --jq '{login,plan:.plan.name}'returns"enterprise"— confirmed.- The check-run comparison is the load-bearing claim in this PR, so I re-derived it independently rather than trusting the pasted output: pulled
head.shafor PR #308 and PR #216 (matches the SHAs cited exactly), fetched each commit's successful check-runs, and intersected the two sets myself. Result: exactly the same 5 checks —adr-boundary,check,Dead Token Reference Guard,Detect Changed Paths,scripts— and the same counts (6 successful on #308, 30 on #216). This is the strongest possible confirmation since I computed it fresh rather than checking the pasted numbers.
Not independently re-verified: gh api repos/launchpad-26/buzz/branches/{launchpad,main}/protection — my own token returned 404 (my collaborator role is maintain, not admin, so I lack the permission the PR author had as an admin). This is an access limitation on my end, not a discrepancy with the PR — I have no way to confirm or dispute the pasted checks: null / enforce_admins: false output from here, and want to flag that explicitly rather than silently assume it's right.
Scope check: the doc explicitly labels the enforce_admins bypass claim as "belief, not fact" and declines to test it (correctly — the only test is pushing to main). It presents two possible shapes for a required-check workflow without picking one, deferring the decision to #154. No overreach found.
No factual errors found in what I could check; the highest-stakes claim (5-of-30 safe checks) was independently re-derived from scratch and matches exactly.
serina-mcfall
left a comment
There was a problem hiding this comment.
Approved. Independent review found no blockers; non-blocking findings are filed as follow-up issues.
Summary
Adds one research document overturning a premise carried by four open decisions: requiring a status check on
launchpaddoes not need an org admin. @tucktuck101 already holds repository admin, there are four repo admins, and both mechanisms are available — branch protection now, and repository rulesets (GET /rulesetsreturns 200, org is on an enterprise plan). Separately establishes the real obstacle nobody had named:ci.ymlis path-filtered, so only 5 of the 30 checks can be required today without deadlocking documentation PRs.Related issue
Closes #358
Issue type
Task
Agent provenance
Objective
Add
launchpad/Research/358-who-can-require-a-check.mdrecording who can require a status check onlaunchpad, whether rulesets are available, and which checks are safe to require.Impacted components
Approach and rejected alternatives
Answered the permission question by reading the collaborator list and the token's own
permissionsobject, rather than reasoning from the org structure — which is where the "needs an org admin" belief came from. Answered ruleset availability by calling the endpoint and reading the status code: a200with[]is the feature responding, and distinguishes "unused" from "unavailable", which the earlierlength == 0measurement could not.Then went one step past the question. Permissions turned out not to be the obstacle, so the document establishes what the actual obstacle is by comparing the checks that succeeded on a documentation-only PR against a 981-file sync — which is what identifies the five always-running checks.
Rejected: creating a test ruleset to prove a repo admin can. It would change repository configuration, which is outside a read-only investigation and is a decision in its own right. The document says plainly that "a repo admin can create one" rests on GitHub's permission model rather than an observed success.
Rejected: testing whether
enforce_admins: falselets admins push tomain. The only test is pushing tomain. Recorded as an explicit belief with its consequence for #298 named, not as a finding.Rejected: recommending which checks to require. That is #154's decision. The document lays out two shapes and their costs and stops.
Verification
Command run:
Raw output:
Not verified
One belief is explicitly untested: that
enforce_admins: falselets the four repo admins push tomaindespiterestrictions.usersnaming onlyjoshuavial. That is GitHub's documented behaviour for the setting and it would materially shrink #298, but the only test is pushing tomain, which I did not and will not do. I could not enumerate org-level rulesets or org branch-protection policy (admin:orgmissing; 404 pasted above), so an org policy could in principle constrain what a repo admin may configure — the repo-level200shows the feature is reachable, not that nothing overrides it. I created no ruleset and changed no protection, so "a repo admin can create one" rests on the permission model, not an observed success. I compared two pull requests, not all 216, so the five-check intersection is a sound lower bound on "always runs", not a proven invariant — a workflow-only or mobile-only PR could shrink it. I did not determine whyauditpassed on #308 but is absent from #216, nor whyaespaldiis missing fromlaunchpad's push-restriction list. I did not runjust ci; the diff is one markdown file.Security implications
The document changes no configuration, but it corrects a security-relevant misapprehension in the fork's own reasoning: four people can already tighten branch protection on this repository, and nobody has. That cuts both ways — the fork is one admin action away from a real merge gate, and it has been one admin action away from that for weeks while three sweeps recorded it as blocked on privilege. It also records, as belief, that
enforce_admins: falsemay meanmain's single-user push restriction is not the control it appears to be; if true, that is a weaker boundary than #298 assumes and should be weighed there.Escalations
#154, #298, #299 and #301 all rest on "needs an org admin", and that premise is false. I have not edited any of them — the correction changes what those decisions are about, and rewriting another agent's open decisions is not mine to do. Each needs a human pass.
The reading order advice on #273 should be reconsidered. An earlier sweep advised settling #298 and #299 first "because admin latency is the long pole". There is no admin latency. That advice was mine to give and it was wrong.
Requiring a genuinely useful check needs a new workflow, and that is a design decision I have not made. The path-filter conflict means requiring
Unit TestsorDesktop Coretoday would deadlock documentation PRs. The standard fix is an always-running aggregator job in alaunchpad-*.ymlworkflow that reports the CI outcome.launchpad/AGENTS.md§3 permits exactly that and it touches no upstream file, but whether to build it belongs to #154.