fix(janitor): round-trip restrictions/lock_branch/allow_fork_syncing in set-required-checks.sh - #65
Merged
asachs01 merged 1 commit intoAug 28, 2026
Conversation
…in set-required-checks.sh The branch-protection PUT payload hardcoded restrictions:null and omitted lock_branch/allow_fork_syncing, instead of reading and preserving current values the way required_pull_request_reviews already is. Found reviewing #55, which cites this script as already run live against 32 *-mcp repos. Verified live against all 32 target repos: none currently have any of the three configured, so no repo was actually clobbered -- but the script mutates fleet-wide branch protection and was going to run again, so this closes the gap rather than leaving it as a known footgun. Also extends the pre-write drift check to refuse if any of the three would change, matching the existing guard on review settings.
Member
Author
|
Reviewed (forge). Traced the jq logic directly, not just the diff text:
No bugs found. The verification methodology in the PR body (32/32 live repos checked, limitation honestly stated re: no pre-mutation snapshot to diff against) is the right level of rigor for a script that mutates fleet-wide branch protection. Confirmed #55 (the target branch) is still open/unmerged, so the stated merge-order note still holds. Native approve is blocked by shared GitHub identity — comment-only, merge routes through the usual Aaron override bundle. |
asachs01
added a commit
that referenced
this pull request
Aug 28, 2026
* feat(release): gate autonomous merges out of the auto-deploy path Adds a `gate` job to mcp-server-release.yml that holds the release when every commit since the last tag carries an Auto-Merged-By trailer, plus release-sweeper.yml/.sh to ship the held batch daily. dependabot-janitor.sh now writes that trailer on its squash merges. Why: a janitor dry run on 2026-08-17 would merge 118 PRs across 71 repos in one sweep. Every caller's release.yml fires on push:[main], so that is up to 71 semantic-release -> GHCR -> Azure deploys in a single unbatched wave, into containers holding live customer API credentials. The rule is narrow on purpose: only an all-autonomous range is held. A human merge in the range means the human owns the release and accumulated autonomous commits ship alongside. A gate with no opener is worse than no gate -- the fleet would silently stop releasing, the same silent-absence failure as the 27-day janitor outage. The sweeper opens it with one empty chore(release): commit per held repo. Chosen over `gh workflow run`, which would need workflow_dispatch on all 58 thin callers; this needs nothing from the caller. Gate logic verified against six cases including the sweeper's own commit correctly reopening the gate. Reaches the fleet by pin bump, no caller edits. Also adds scripts/set-required-checks.sh + derive-required-checks.sh and docs/required-checks.md, the tooling used to set required_status_checks on the 32 *-mcp repos with substantive CI (applied and verified 2026-08-17: 32/32 set, 0 lost code-owner review). * fix(janitor): round-trip restrictions/lock_branch/allow_fork_syncing in set-required-checks.sh (#65) The branch-protection PUT payload hardcoded restrictions:null and omitted lock_branch/allow_fork_syncing, instead of reading and preserving current values the way required_pull_request_reviews already is. Found reviewing #55, which cites this script as already run live against 32 *-mcp repos. Verified live against all 32 target repos: none currently have any of the three configured, so no repo was actually clobbered -- but the script mutates fleet-wide branch protection and was going to run again, so this closes the gap rather than leaving it as a known footgun. Also extends the pre-write drift check to refuse if any of the three would change, matching the existing guard on review settings. * fix(release-sweeper): route through gh pr merge, default DRY_RUN safe Review-driven (warden's Gate-3 pass on this PR), two blocking findings: 1. release-sweeper.sh pushed a raw commit straight to each held repo's default branch. Branch protection on the 32 target repos requires a review, and wyre-projects-bot has no bypass_pull_request_allowances entry, so the push almost certainly gets rejected everywhere it matters. Now opens a branch + PR and goes through the same approve-then-gh-pr-merge pattern dependabot-janitor.sh already uses -- succeeds where nothing blocks it, and reports a new 'blocked, needs a human merge' bucket (with the PR URL) everywhere protection actually requires a distinct reviewer, instead of silently failing or fighting the protection. 2. release-sweeper.yml's DRY_RUN expression (`inputs.dry_run && 'true' || 'false'`) treated the schedule trigger's missing input as falsy, so the only trigger that actually matters -- the cron -- defaulted to LIVE. Gated on github.event_name first: a missing/absent dry_run input can now only ever resolve to safe (true); only an explicit workflow_dispatch with the box unchecked goes live. Scoped to exactly these two blockers per warden's review and boss's ruling -- the two non-blocking items (GH App token scoped org-wide instead of to target repos; sweeper's discovery regex not independently verified against mcp-gateway exclusion) are left as PR comments for follow-up, not folded in here. * fix(release-sweeper): correct misleading self-approval comment Warden's re-review, cosmetic-only per their note: the comment implied the self-approval was situational (fails only when the same identity is on both sides). It is not situational here -- the App token mints both the PR and the approval attempt every single run, so it's a guaranteed rejection, not a maybe. Behavior unchanged; comment now says so.
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
1 participant
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.
Follow-up to a review finding on #55 (targets that PR's branch,
feat/release-decoupling, sincescripts/set-required-checks.shdoesn't exist onmainyet).Problem
set-required-checks.sh's branch-protection PUT payload hardcodedrestrictions: nulland omittedlock_branch/allow_fork_syncingentirely, instead of reading and preserving their current values the way it already does forrequired_pull_request_reviews. The PR body for #55 states this script has already been run live against the 32*-mcprepos listed indocs/required-checks-baseline.tsv, so any of the three fields that were set on those repos before 2026-08-17 would have been silently cleared.Verification — full 32-repo enumeration, not a sample
The original review that caught this spot-checked 3 of the 32 repos. Boss asked for the full set given how cheap it is (one GET per repo) — here it is, all 32:
Result: 32/32 currently show
restrictions=null,lock_branch=false,allow_fork_syncing=false— the safe/default state on every repo. No live damage found.Honest limitation, stated explicitly rather than overclaimed: this is current live state, not a diff against a captured pre-mutation snapshot. The script's own
BACKUP_DIRrollback JSON was written locally when it ran on 2026-08-17 and was never committed or uploaded as an artifact, so no pre-image survives to diff against directly. The measured result above is the strongest evidence available — a uniform, all-default result across all 32 repos, on settings that are rare enough in practice that a real pre-existing configuration accidentally landing on the exact default is unlikely — but it is inference from current state, not a direct before/after comparison. Flagging this so nobody reads "32/32 clean" as a stronger claim than it is.Fix
Round-trips
restrictions,lock_branch, andallow_fork_syncingthe same read-merge-write way the script already handlesrequired_pull_request_reviews:restrictions: converts GET's shape (objects withlogin/slug) to PUT's expected shape (arrays oflogin/slugstrings), or passes throughnullunchanged.lock_branch/allow_fork_syncing: unwrapped from GET's{enabled: bool}to the plain boolean PUT expects, matching the existing pattern already used forenforce_admins/allow_force_pushes/etc.required_pull_request_reviews) to also refuse the write if any of these three would change — same "refuse rather than silently clobber" discipline, now covering all read-merge-write fields, not just reviews.Verified the jq logic against synthetic test cases (both
restrictions: nulland a populatedrestrictionswith users/teams, since none of the 32 live repos exercise that path) — round-trips correctly in both directions, and the new drift check correctly reports no-op for both.Merge order
Should land after (or alongside, if easier to just fold in) #55, since it fixes a file #55 introduces. Fine to squash into #55 directly instead of merging separately if that's simpler on your end — opened as its own PR per boss's ask so the verification output has a clear place to live.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.