ci: give GitHub Actions bypass on the release-branch ruleset - #8379
ci: give GitHub Actions bypass on the release-branch ruleset#8379mengw15 wants to merge 6 commits into
Conversation
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8379 +/- ##
============================================
- Coverage 94.03% 94.02% -0.01%
+ Complexity 4821 4819 -2
============================================
Files 1204 1204
Lines 48991 48991
Branches 5956 5956
============================================
- Hits 46067 46065 -2
- Misses 1458 1459 +1
- Partials 1466 1467 +1
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The backport workflow prefers a PAT, so the Actions integration bypass will not unblock its pushes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Splits branch rulesets so GitHub Actions can bypass merge-queue requirements on release branches without bypassing main.
Changes:
- Restricts the existing ruleset to the default branch.
- Adds an equivalent release ruleset with an Actions integration bypass.
File summaries
| File | Description |
|---|---|
.asf.yaml |
Separates release protections and adds the bypass. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 389 | 0.237 | 24,179/34,225/34,225 us | 🔴 +15.9% / 🔴 +109.6% |
| 🟢 | bs=100 sw=10 sl=64 | 814 | 0.497 | 120,425/146,841/146,841 us | 🟢 -6.4% / 🔴 +33.1% |
| ⚪ | bs=1000 sw=10 sl=64 | 923 | 0.564 | 1,081,968/1,140,629/1,140,629 us | ⚪ within ±5% / 🔴 +8.3% |
Baseline details
Latest main 1cbe857 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 389 tuples/sec | 437 tuples/sec | 749.39 tuples/sec | -11.0% | -48.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.237 MB/s | 0.267 MB/s | 0.457 MB/s | -11.2% | -48.2% |
| bs=10 sw=10 sl=64 | p50 | 24,179 us | 24,013 us | 13,045 us | +0.7% | +85.4% |
| bs=10 sw=10 sl=64 | p95 | 34,225 us | 29,524 us | 16,328 us | +15.9% | +109.6% |
| bs=10 sw=10 sl=64 | p99 | 34,225 us | 29,524 us | 19,518 us | +15.9% | +75.3% |
| bs=100 sw=10 sl=64 | throughput | 814 tuples/sec | 827 tuples/sec | 968.07 tuples/sec | -1.6% | -15.9% |
| bs=100 sw=10 sl=64 | MB/s | 0.497 MB/s | 0.505 MB/s | 0.591 MB/s | -1.6% | -15.9% |
| bs=100 sw=10 sl=64 | p50 | 120,425 us | 118,917 us | 103,330 us | +1.3% | +16.5% |
| bs=100 sw=10 sl=64 | p95 | 146,841 us | 156,887 us | 110,305 us | -6.4% | +33.1% |
| bs=100 sw=10 sl=64 | p99 | 146,841 us | 156,887 us | 121,264 us | -6.4% | +21.1% |
| bs=1000 sw=10 sl=64 | throughput | 923 tuples/sec | 925 tuples/sec | 998 tuples/sec | -0.2% | -7.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.564 MB/s | 0.565 MB/s | 0.609 MB/s | -0.2% | -7.4% |
| bs=1000 sw=10 sl=64 | p50 | 1,081,968 us | 1,076,833 us | 1,009,354 us | +0.5% | +7.2% |
| bs=1000 sw=10 sl=64 | p95 | 1,140,629 us | 1,137,554 us | 1,052,748 us | +0.3% | +8.3% |
| bs=1000 sw=10 sl=64 | p99 | 1,140,629 us | 1,137,554 us | 1,081,982 us | +0.3% | +5.4% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,514.49,200,128000,389,0.237,24179.14,34224.69,34224.69
1,100,10,64,20,2457.30,2000,1280000,814,0.497,120424.75,146840.89,146840.89
2,1000,10,64,20,21658.29,20000,12800000,923,0.564,1081967.51,1140628.99,1140628.99
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 2 must-fix · 4 advisory · 0 polish: the design holds up, but a duplicate on: key stops Required Checks from starting.
Correctness (3)
.github/workflows/required-checks.yml:27: duplicateworkflow_dispatch:key, the workflow fails at startup (must-fix, see inline).github/workflows/direct-backport-push.yml:576: a dispatch failure reports a landed backport as failed (must-fix, see inline).github/workflows/direct-backport-push.yml:585: qualified--refwhere the API documents a bare branch name (advisory, see inline)
Design & architecture (1)
.asf.yaml:101: therules:block is duplicated with nothing keeping the two copies equal (advisory, see inline)
Simplifications (1)
.github/workflows/direct-backport-push.yml:26:actions: writeis workflow-wide where one step needs it (advisory, see inline)
Conventions (1)
.asf.yaml:72: no linked fork proof run for the workflow changes (advisory, see inline)
Verification trace
I checked the three load-bearing claims against the tree. After the split, Merge Queue covers only ~DEFAULT_BRANCH, so the bypass does not reach main. Default Branch Protection still carries restrict_deletion and restrict_force_push over release/* with only apache/root bypassing, so Actions can fast-forward a release branch but cannot delete or rewrite one. The two rules: blocks are deep-equal today, which makes the duplication a drift risk rather than a present bug.
On the startup failure: yaml.safe_load parses required-checks.yml clean, because PyYAML accepts a redefined key and keeps the last. That is why a local parse check could not catch this while GitHub's loader rejects it.
|
|
||
| rulesets: | ||
| - name: Merge Queue | ||
| # Rule-for-rule identical to "Merge Queue" below; split out so the bypass |
There was a problem hiding this comment.
Advisory:
The PR-level gates ask a CI or workflow change to link a proof run, usually on a fork. The description explains why the ruleset half cannot be proven pre-merge, and that reasoning holds. The workflow half is fork-provable, and a fork run of Required Checks is exactly what would have surfaced the startup failure above.
There was a problem hiding this comment.
The dispatch mechanism is the documented GITHUB_TOKEN exception ("workflow_dispatch and repository_dispatch events always create workflow runs"), and after 1864bda a surprise there degrades to a warning rather than a failed job. The .asf.yaml half only takes effect where Infra applies it — the body's post-merge verification covers that. Happy to stage a fork demo if you'd still like one.
Splits the Merge Queue ruleset in two, rules identical, so the release half can carry a bypass for the Actions app without extending it to main. Workflows (GITHUB_TOKEN) can then push release/* again, which revives direct-backport-push.yml's fast path, rejected by the ruleset since 2026-07-24 (apache#8377). People and PATs still face every rule.
asfyaml applies rulesets in file order. With the new ruleset listed first, a rejected creation aborts the apply while today's protections still cover the release branches; the old order would shrink "Merge Queue" first and leave them uncovered if the creation then failed.
The fast path has pushed with AUTO_MERGE_TOKEN since apache#4676, so GitHub evaluates that push as the PAT's owner, whom the Actions-app bypass does not cover. Pushing with the default GITHUB_TOKEN puts it under the bypass; the release-branch CI the PAT existed to retrigger is dispatched explicitly instead, which workflow_dispatch permits a GITHUB_TOKEN to do.
required-checks.yml already carried workflow_dispatch at the bottom of its trigger block, so the added one was a duplicate key -- PyYAML keeps the last silently, GitHub's loader rejects the file. The explanatory comment moves to the existing key instead. The dispatch after a fast-path push is now best-effort with a warning: the backport has landed by then, and a failed dispatch would otherwise trip the failure reporter into claiming the backport was lost. The ref uses the documented bare branch form, and actions: write shrinks from the workflow to the one job whose step dispatches.
The two Merge Queue rulesets are one policy split so the release half can hold an Actions bypass that must not reach main; nothing else kept the copies equal. The same script parses .asf.yaml and every workflow with a duplicate-key-strict loader, the class of error a plain parse passes and GitHub's loader rejects.
582d9f6 to
711265b
Compare
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 4 resolved · 2 open · 3 new (3 new = 3 newly introduced · 0 late catches)
All four findings from the last round are fixed, verified in the tree. The committed check that answers the fifth cannot run in CI.
Correctness (1)
.github/scripts/test_asf_rulesets.sh:40—import yamlwith no PyYAML in the infra job; both legs die andRequired Checksgoes red (must-fix, see inline)
Design & architecture (1)
.github/scripts/test_asf_rulesets.sh:85— the check guardsrulesequality but not the bypass placement the split exists for (advisory, replied on the.asf.yaml:101thread)
Polish: 1 quick touch-up (see inline comments).
Still open from last round: the .asf.yaml rules: duplication has no working guard until the above is fixed, and no fork proof run is linked.
Verification trace
I checked each prior finding against the tree rather than the replies. grep -n workflow_dispatch .github/workflows/required-checks.yml returns one hit, line 35, so the duplicate on: key is gone. The dispatch at direct-backport-push.yml:595-598 is now if ! gh workflow run … ; then echo "::warning::…"; fi, so a failure no longer skips the success annotation at line 600, and --ref passes the bare branch name. The workflow-level map is back to actions: read and the new job-level block is complete: enumerating every API call inside push-backports gives repos.createCommitStatus, repos.createCommitComment, issues.createComment and actions.listJobsForWorkflowRun, with no pulls.* or checks.* call, so dropping pull-requests: write and checks: read for that job denies it nothing.
On the new script: it passes locally, where PyYAML happens to be installed, printing OK: 24 files duplicate-key clean; Merge Queue rules identical. In CI it never reaches that line.
The infra job's only Python install is amber/dev-requirements.txt, and setup-python ships no PyYAML, so the new invariant check died on import in both legs -- the constraint release_branches.py already records. The check now also asserts what the split exists for: no bypass_actors on "Merge Queue", exactly the Actions app on "Merge Queue (release)".
github-actions
Bot
added
dependencies
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟡 3 resolved · 1 open · 2 new (2 new = 2 newly introduced · 0 late catches)
The PyYAML must-fix is closed and the assertion block reaches CI for the first time on this head. Two small things left.
Design & architecture (1)
.asf.yaml:78: ruleset order is declared load-bearing, and nothing asserts it (advisory, see inline)
Polish: 1 quick touch-up (see inline comments).
Still open from last round: no fork proof run is linked for the dispatch step (the .asf.yaml:72 thread).
Verification trace
I checked the fix in the tree rather than in the reply. amber/dev-requirements.txt:43 pins pyyaml==6.0.3, build.yml:1289-1290 installs that file in the same job whose find at build.yml:1303 discovers the script, and build / infra is green on both ubuntu-latest and macos-latest at this head, so the assertion block executed for the first time.
A green assertion proves nothing until it can go red, so I replayed the block at test_asf_rulesets.sh:82-103 against mutated copies of the live .asf.yaml. Changing one merge_queue parameter on "Merge Queue" gives "rules differ", adding bypass_actors to "Merge Queue" gives "main has bypass", and both appending a second actor and deleting the release bypass give "release bypass wrong". All four failure paths the new assertions claim are real.
The dispatch runs the ref's own copy of the workflow, so I fetched required-checks.yml from apache/texera at release/v1.1, release/v1.2 and release/v1.3. All three already declare workflow_dispatch:, so the fast path works on the existing release lines without waiting for this PR to be backported.
| # direct-backport-push.yml's fast path pushes with (#8377). It cannot be | ||
| # scoped to a single workflow. People and PATs still face every rule. | ||
| # | ||
| # Listed BEFORE "Merge Queue" deliberately: asfyaml applies rulesets in |
There was a problem hiding this comment.
Advisory:
This comment makes file order load-bearing and names the failure mode. The one check that would catch a reorder cannot see order: test_asf_rulesets.sh:77 keys the rulesets into a dict. Both the rules-equality and the new bypass-placement assertions then run on an order-free view, so an edit that alphabetises the rulesets stays green while removing the fail-safe. Asserting that "Merge Queue (release)" precedes "Merge Queue" is a two-line addition beside test_asf_rulesets.sh:93.
|
|
||
| # Required by `bin/local-dev.sh -i` (the interactive Textual TUI). | ||
| textual==8.2.8 | ||
| # .github/scripts/test_asf_rulesets.sh (infra job shell tests) |
There was a problem hiding this comment.
Polish:
Every other comment and pin group in this file is blank-line separated (:22, :27, :31, :35, :39). This entry reads as part of the textual group.
| # .github/scripts/test_asf_rulesets.sh (infra job shell tests) | |
| # .github/scripts/test_asf_rulesets.sh (infra job shell tests) |
What changes were proposed in this PR?
The Merge Queue ruleset requires every change into
release/*to arrive as a PR with one approving review, green required checks, and a pass through the merge queue. Right for people — but it also blocksdirect-backport-push.yml, whose fast path pushes clean cherry-picks; every such push has been rejected since 2026-07-24, and five backports were silently lost (#8377).This splits the ruleset in two, rule-for-rule identical:
Merge Queuekeeps~DEFAULT_BRANCH, and a newMerge Queue (release)carries the three release branches plus abypass_actorsentry for the GitHub Actions app (actor_id: 15368). The split exists because a bypass is ruleset-wide — kept in one ruleset, it would let workflows pushmaintoo.Scope, stated precisely: the bypass exempts actions performed as the Actions app — any workflow's
GITHUB_TOKEN, not just the backport workflow, since rulesets cannot scope a bypass to one workflow. People and PATs still face every rule on every branch;maingets no bypass; force pushes and branch deletion stay blocked for everyone, Actions included, byDefault Branch Protection.Ordering inside the file is load-bearing: asfyaml applies rulesets in file order, so
Merge Queue (release)is created beforeMerge Queuestops covering the release branches. If GitHub rejects the new ruleset, the apply aborts with today's protections fully intact — no failure path leaves the release branches uncovered.The bypass alone would not revive the fast path: since #4676 the push job checked out with
AUTO_MERGE_TOKEN, so GitHub evaluated its pushes as that PAT's owner — every pre-ruleset direct push shows a person as the pusher — and an Actions-app bypass would not cover them. The push job now uses the defaultGITHUB_TOKEN, which the bypass does cover, and dispatchesRequired Checkson the pushed branch explicitly, since aGITHUB_TOKENpush starts no push-triggered runs whileworkflow_dispatchis the documented exception that always creates one. The conflict path keeps the PAT: it pushes unprotectedbackport/*branches, where the opened PR's CI must still trigger.Any related issues, documentation, discussions?
Closes #8377. #8378 took the PR-plus-auto-merge route to the same problem and is closed in favor of trying the bypass first. What lands on a release branch through this path is still only a cherry-pick of a commit that passed main's full CI and, once #8096 lands, its release manager's approving review.
How was this PR tested?
.asf.yamland the workflows parse, and the structural check is now committed instead of run once:.github/scripts/test_asf_rulesets.sh(picked up by build.yml's glob-discovered infra tests) asserts the two rulesets'rulesblocks stay deep-equal and that.asf.yamland every workflow parse under a duplicate-key-strict loader, with PyYAML pinned inamber/dev-requirements.txt— the file the infra job installs; every failure path (duplicate key, rules drift, bypass on main, bypass tampered, missing PyYAML) was verified red before trusting the green. asfyaml treats a ruleset carryingtarget/rules/bypass_actorsas a raw payload and forwards it verbatim (_RAW_RULESET_KEYSinfeature/github/rulesets.py; its upstream tests assert the POST payload carriesbypass_actors).What cannot be proven before merge is GitHub accepting the Actions app as a bypass actor on this org: the same payload on a personal repository is rejected with "Actor GitHub Actions integration must be part of the ruleset source or owner organization", and no ASF repository uses an Integration bypass actor yet — hence the fail-safe ordering above. After Infra applies the merged file,
GET /repos/apache/texera/rulesetsshould listMerge Queue (release); if it does not, the apply failed closed and nothing changed. The next clean backport is the end-to-end test.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-fable-5)