feat(ci): branch-protection-sync follows .gitmodules — push + weekly triggers (BLOCKED on #2519) - #2528
Conversation
…triggers
Coverage is derived from .gitmodules, so it has to follow .gitmodules. Until
now the workflow was workflow_dispatch-only and had run three times ever, last
on 2026-06-10 with failed=0. Every submodule added or re-pointed since fell out
of coverage silently: 15 consumed branches ended up with no protection at all,
not one of them because the workflow got anything wrong. A manual re-run fixes
today without fixing the class.
push on .gitmodules the primary vector — a submodule added or re-pointed is
exactly the event that drops it from coverage, so the
check runs on the commit that causes it
schedule, weekly backstop for drift that never touches .gitmodules: a
branch renamed or deleted on a fork, protection removed
by hand, a fork created outside a superproject commit.
Weekly rather than daily because push already covers the
frequent vector, and a daily audit nobody reads is noise
The non-obvious part is the env block. `inputs.*` is empty on any trigger other
than workflow_dispatch, and the script tests `[ "$DRY_RUN" = "true" ]` — so
adding a schedule/push trigger naively would evaluate DRY_RUN="" as "not a dry
run" and issue live PUTs across every derived fork on the first tick, with
ONLY_UNPROTECTED="" simultaneously disabling the gap-only filter. That is a
fleet-wide write nobody asked for, and on this fleet it would lower
required_approving_review_count from 1 to 0 on 41 repos.
Both flags are therefore pinned to "true" for non-dispatch events via
`github.event_name != 'workflow_dispatch' && 'true' || inputs.<x>`, which passes
the input through unchanged on dispatch and forces audit-only + gaps-only
otherwise. Automated runs report; writing stays a deliberate human act.
Nothing else about the workflow changes: same derivation, same policy, same
token scope, same preserve-existing-checks path.
Blocked on #2519, which adds the five missing .gitmodules branch fields. Without
it, four of those forks resolve to the "main" default and target a branch that
does not exist on them. Blast radius and the arming instructions are in the PR
body.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Two clarifications on the arming question, since "merging arms a cron" deserves precision: Nothing is armed right now. GitHub only honours The Which suggests a small refinement to the recommended sequence in the description: merging #2519 first, then this PR means the CI on this PR: 19 pass, 0 fail. The |
1 participant
Blocked on #2519
#2519 (open,
fix/defects-surfaced-by-2511) adds the five missingbranchfields to.gitmodules:Without it, the workflow's
[ -z "$branch" ] && branch="main"default sends four of those five at amainbranch that does not exist on them (verified per repo:obico-server→release,moonraker-obico→master,OctoPrint-Obico→master,fluidd→develop; onlyOrcaSlicerreally ismain). I reproduced the workflow's derive step against both versions of.gitmodules; the diff is exactly those four:Everything below is computed against the post-#2519 state, since that is the state this workflow would operate in.
What changed in the workflow
Two triggers, and one safety fix that the triggers make mandatory.
pushon.gitmodulesis the primary vector — adding or re-pointing a submodule is precisely the event that drops it from coverage, so the check now runs on the commit that causes it. The weeklyscheduleis the backstop for drift that never touches.gitmodules: a branch renamed or deleted on a fork, protection removed by hand, a fork created outside a superproject commit. Weekly rather than daily becausepushalready covers the frequent vector, and a daily audit nobody reads is noise rather than signal.The safety fix, which is the part worth reviewing
inputs.*is empty on every trigger exceptworkflow_dispatch, and the script tests:So adding
schedule:/push:naively would evaluateDRY_RUN=""as not a dry run and issue live PUTs across all 63 derived forks on the first tick — withONLY_UNPROTECTED=""simultaneously disabling the gap-only filter, so it would rewrite already-protected branches too. On this fleet that single unattended run would lowerrequired_approving_review_countfrom 1 to 0 on 41 repos.Both flags are now pinned for non-dispatch events:
Dispatch behaviour is unchanged (the input passes through, including
dry_run: false). Automated runs can only audit, and only look at gaps.Nothing else changes: same derivation, same policy body, same token scope, same preserve-existing-checks path.
Scope: 65 entries → 63 (repo, branch) pairs
The derive step keys on the URL basename, not the section name, then
sort -u. Reproduced verbatim:This answers the double-registration question: no, they are not protected twice.
PMOVES-Archon(registered as bothPMOVES-Archonandpmoves/integrations/archon) andPMOVES-pinokio(registered as bothPMOVES-pinokioandpbnj) each produce an identicalrepo<TAB>branchline, whichsort -ucollapses. Both pairs track the same branch, so there is no conflict to resolve. It is harmless today and would only matter if the two registrations ever disagreed on branch — at which point the derive step would emit two lines and the second PUT would silently win.Blast radius of a live run
Beneficiaries — 13 pairs gain protection where there is none
These are the intended effect. All currently return
Branch not protected.PMOVES-ClawZPMOVES.AI-Edition-HardenedPMOVES-Danger-infraPMOVES.AI-Edition-HardenedPMOVES-E2B-Danger-RoomPMOVES.AI-Edition-HardenedPMOVES-fluidddevelopPMOVES-headscalePMOVES.AI-Edition-HardenedPMOVES-hermes-agentPMOVES.AI-Edition-HardenedPMOVES-MAI-UIPMOVES.AI-Edition-HardenedPMOVES-moonraker-obicomasterPMOVES-obico-serverreleasePMOVES-OctoPrint-ObicomasterPMOVES-ollamaPMOVES.AI-Edition-HardenedPMOVES-OrcaSlicermainpmoves-pipecatPMOVES.AI-Edition-HardenedPMOVES-ClawZ(which has no classic protection but is covered by a~ALLruleset, so #2522 did not count it as ungated). The missing 3 are the no-branch cases below.Each would receive: PR required (
required_approving_review_count: 0, so automation can still self-merge), no force pushes, no deletions, conversation resolution required,enforce_admins: false,required_status_checks: null.Would CHANGE existing protection — 41 pairs, one field
This is the question that matters. With
only_unprotected: false, the run rewrites every derived branch from the policy body, and the policy is not a superset of what is there now.Exactly one field regresses, on 41 of the 47 currently-protected pairs:
required_approving_review_countPMOVES-a0-pluginsPMOVES.AI-Edition-HardenedPMOVES-A2UIPMOVES.AI-Edition-HardenedPMOVES-Agent-ZeroPMOVES.AI-Edition-HardenedPMOVES-AgentGymPMOVES.AI-Edition-HardenedPmoves-AgentGym-RLPMOVES.AI-Edition-HardenedPMOVES-ArchonPMOVES.AI-Edition-HardenedPMOVES-autoresearchPMOVES.AI-Edition-HardenedPMOVES-BoTZPMOVES.AI-Edition-HardenedPMOVES-BotZ-gatewayPMOVES.AI-Edition-HardenedPmoves-cipherPMOVES.AI-Edition-Hardenedpmoves-cipher-mcpPMOVES.AI-Edition-HardenedPMOVES-CreatorPMOVES.AI-Edition-HardenedPMOVES-Deep-SerchPMOVES.AI-Edition-HardenedPMOVES-DoXPMOVES.AI-Edition-HardenedPMOVES-E2B-Danger-Room-DesktopPMOVES.AI-Edition-Hardenedpmoves-e2b-mcp-serverPMOVES.AI-Edition-HardenedPMOVES-E2b-SpellsPMOVES.AI-Edition-HardenedPmoves-Health-wgerPMOVES.AI-Edition-HardenedPMOVES-HiRAGPMOVES.AI-Edition-HardenedPmoves-hyperdimensionsPMOVES.AI-Edition-HardenedPMOVES-JellyfinPMOVES.AI-Edition-HardenedPmoves-Jellyfin-AI-Media-StackPMOVES.AI-Edition-HardenedPMOVES-llama-throughput-labPMOVES.AI-Edition-HardenedPMOVES-n8nPMOVES.AI-Edition-HardenedPMOVES-neo4jPMOVES.AI-Edition-HardenedPMOVES-Open-NotebookPMOVES.AI-Edition-HardenedPMOVES-OpenRoomPMOVES.AI-Edition-HardenedPMOVES-pinokioPMOVES.AI-Edition-HardenedPMOVES-Pinokio-Ultimate-TTS-StudioPMOVES.AI-Edition-HardenedPmoves-pretextPMOVES.AI-Edition-HardenedPMOVES-Remote-ViewPMOVES.AI-Edition-HardenedPMOVES-space-agentPMOVES.AI-Edition-HardenedPMOVES-supabasePMOVES.AI-Edition-HardenedPMOVES-surfPMOVES.AI-Edition-HardenedPMOVES-TailscalePMOVES.AI-Edition-HardenedPMOVES-tensorzeroPMOVES.AI-Edition-HardenedPMOVES-ToKenism-MultiPMOVES.AI-Edition-HardenedPMOVES-transcribe-and-fetchPMOVES.AI-Edition-HardenedPMOVES-Ultimate-TTS-StudioPMOVES.AI-Edition-HardenedPMOVES-WealthPMOVES.AI-Edition-HardenedPMOVES.YTPMOVES.AI-Edition-Hardened0, andPMOVES-crushhas protection with no PR-review block at all (it would gain one, at0, plus conversation resolution).Every other field is safe. Checked across all 47 protected pairs:
enforce_adminsfalseon all 47 — the policy valuerequire_code_owner_reviewsfalse/unset on all 47restrictions(push allowlist)restrictions: nullwipes nothingrequired_linear_historyfalseon all 47required_signaturesallow_force_pushes/allow_deletionsfalseon all 47 — the policy is a no-oprequired_conversation_resolutiontrueon 46,falseon 1 → the policy raises itOn the #1767 preserve-existing-checks path
You asked me to confirm it still holds for every repo in scope. The precise answer is that it is not exercised by any repo in scope:
Zero of the 47 protected pairs has any required status check on its consumed branch. The forks that do have required checks —
PMOVES-hermes-agent(9) andPMOVES-pinokio(CodeRabbit) — carry them onmain, which is not the branch the monorepo consumes. This is the same wrong-branch pattern #2522 documented, showing up in the classic layer.So the #1767 regression cannot repeat on this scope, but only because there is nothing to strip. The preserve path is untested here rather than proven; I am not claiming it works, I am reporting that it has no work to do. The first fork to gain a required check on its hardened branch will be the real test.
3 pairs will 404 — branch absent from the remote
pmoves-hirag-mcpPMOVES.AI-Edition-HardenedmainPMOVES-jcodemunch-mcpPMOVES.AI-Edition-HardenedmainPM-Spark-video-search-and-summarizationPMOVES.AI-Edition-HardenedmainNot fixed by #2519, and not fixable here — you cannot protect a branch that does not exist. The workflow already handles this gracefully:
404is caught per fork, tallied intoFAILED, and annotatedtracked branch '<b>' missing on <repo> (fix .gitmodules). It does not fail the run.That is the right behaviour for a fleet sweep, but it means these three will show up as
failed=3on every audit until.gitmodulesis corrected — an amber light that never goes green. Worth fixing in the same pass as #2519 rather than living with it.Recommended arming sequence
~ repo@branch: UNPROTECTED -> WOULD applylines are the gap list).dry_run: falseonly_unprotected: true← this is what keeps the 41-repo downgrade off the table; it makes the run additive-only.gitmodules, then re-run.only_unprotected: falseis the mode to avoid unless the intent really is to reset all 63 to the policy baseline. It is the only path that produces the downgrade table above.One thing this PR deliberately does not do
The scheduled audit reports into the run log and exits 0 even when it finds unprotected branches. It surfaces drift only to someone who opens the run. Making the dry-run exit non-zero when gaps exist would turn it into a real signal — but that changes what the workflow does, not just when it runs, so it is out of scope here and left as an operator call.
Method
Read-only against GitHub; the only write is the workflow file on this branch. Per (repo, branch) pair:
GET repos/{slug}/branches/{branch}for existence andGET repos/{slug}/branches/{branch}/protectionfor the full current body, 63 pairs, paced. The derive step was reproduced verbatim from the workflow's own shell against both the current and post-#2519.gitmodules, read fromorigin/mainandorigin/fix/defects-surfaced-by-2511rather than a working tree. The workflow was not run.🤖 Generated with Claude Code