ci(l1,l2): stop the merge queue satisfying the required integration checks with a skipped job - #7213
ci(l1,l2): stop the merge queue satisfying the required integration checks with a skipped job#7213ilitteri wants to merge 5 commits into
Conversation
…of skipping them.
`Integration Test` and `Integration Test L2` are required status checks on main,
but both gate jobs bailed out whenever a dependency was skipped:
if: ${{ ... && needs.run-hive.result != 'skipped' ... }}
Hive, assertoor and the L2 suites are all excluded from `merge_group` to keep the
queue cheap, so inside the queue that condition was always false and the gate job
was skipped. GitHub counts a skipped check run as satisfying a required status
check, so every merge group satisfied both requirements without running or even
consulting the suites they exist to enforce. A pull request queued while its hive
run was red, or while a re-run was still in flight, merged on that vacuous green.
Two of the three commits that landed on main on the day this was found had a red
`Hive - Devp2p tests` and a failed `Integration Test` on their own head, and main
went red on devp2p immediately afterwards.
The gate jobs now always run when the workflow is not skipped wholesale, and on
`merge_group` they read the queued pull request's own results for those suites
via `check-queued-pr-checks.sh`. That keeps the queue's cost profile unchanged
while making the requirement real, and because it runs at merge time it also
catches a suite that turned red or was re-triggered after the pull request was
added to the queue: a still-running suite blocks the group rather than passing.
The script resolves the queued pull requests from the merge group's commit
subjects, so a batched group is covered rather than only its last pull request,
and it keeps the most recently started check run per name so a superseded red run
cannot block a head that is now green. A skipped suite still counts as satisfied,
which is what an L2-only pull request looks like to the L1 workflow. Finding no
matching check run at all fails: a gate that cannot see what it is verifying must
not report success.
Not addressed here: `check-cargo-locks` is in the L1 gate's `needs` but its
result is still never inspected, so `Check Cargo.lock` remains unenforced by the
required check. That is a separate policy call.
…ude. Same bug class as the parent commit, one dependency further up. The gate keys off `needs.detect-changes.outputs.run_tests`, and a job that failed publishes no outputs, so `'' == 'true'` was false and the gate skipped — which GitHub counts as satisfying the required check. A broken change-detection step therefore turned both `Integration Test` and `Integration Test L2` green without anything having been evaluated. The gate now also runs when `detect-changes` did not succeed, and fails immediately in that case: with no `run_tests` answer there is no way to tell whether the suites were required, and an unanswerable gate must not report success.
|
🤖 Kimi Code ReviewThis PR correctly implements a fail-closed gate for GitHub merge queues to prevent merging PRs with failing/skipped expensive test suites (Hive, Assertoor, L2 integration tests). The approach of reading the original PR's check runs rather than relying on skipped jobs in the merge group is the correct solution to GitHub's behavior where skipped checks satisfy required status checks.
|
🤖 Codex Code ReviewFindings
This PR only touches CI/workflow code, so I did not find any Rust/EVM/consensus-path changes to review here. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewReview: PR 7213 — merge queue enforcement of required integration checksThis is a CI/workflow change rather than core protocol code, so the review focuses on the correctness of the gating logic and the new shell script rather than EVM/consensus concerns. Findings1.
The result: in the merge queue, 2. In 3. Reliance on squash-commit-subject convention for identifying queued PRs (low severity, flagged for awareness)
Positive notes
Automated review by Claude (Anthropic) · sonnet · custom prompt |
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
Reviewed the merge-queue gate change against live repo data. The problem it targets is real and I confirmed it end to end: PR #7209 merged on 2026-08-24 with Hive - Devp2p tests red, and its merge group reported Integration Test as skipped. Reading the queued pull request's own check runs is the right shape for a fix, and the bash is careful.
Two things stop it from actually closing the hole. Both are inline, both verified against real commits.
1. skipped is accepted, and that state is reachable with the suites genuinely not run. Head e104cdbc really exists: Build Docker = failure, Hive - ${{ matrix.name }} = skipped, Assertoor - ${{ matrix.name }} = skipped. Replaying this script's loop over that head prints All required suites passed.
The natural defense is that such a pull request cannot enter the queue because its own Integration Test is red. That does not hold: Integration Test is declared by three workflows (pr-main_l1.yaml, pr-main_l1_l2_dev.yaml, pr-main_levm.yaml), and on PR #7209's head the L1 one was failure while the L2-Dev one was success. It merged.
2. The gate still skips entirely in the queue for any non-Rust change. On merge_group both workflows set run_tests = code_changed, which matches only **/*.rs, **/*.toml, **/*.lock. PR #7193 (CI-only) merged with every job in its merge group skipped, Integration Test included, while its head carried seven real green Hive - * results the queue never read. This pull request is in that same class, so it would merge under the behavior it is fixing.
What it gets right. Dropping the needs.<job>.result != 'skipped' guards is a genuine fix on the pull_request side: run-hive and run-assertoor both need docker_build, so a broken image used to skip the gate into a green required check. I also checked and found no problems with injection (PR numbers are digits after grep -oE/tr, names are quoted, jq @tsv escapes), set -euo pipefail behavior (the empty-array guard catches the grep miss, [[ -z ... ]] && continue does not trip set -e, the here-string keeps matched/failed in scope), the --paginate --slurp shape, the (#N) title heuristic (the repo is squash-only with PR_TITLE), and the permissions: blocks, which are minimal and sufficient.
Remaining inline notes are smaller.
| if [[ "$status" != "completed" ]]; then | ||
| echo "PENDING ${name} (status=${status}) is still running, so it cannot be merged yet" | ||
| failed=1 | ||
| elif [[ "$conclusion" != "success" && "$conclusion" != "skipped" && "$conclusion" != "neutral" ]]; then |
There was a problem hiding this comment.
skipped is accepted unconditionally, and it is reachable with the suites genuinely not run.
Real head e104cdbccb2aad1c771ea4ab397fff8a10520f5b carries:
Build Docker failure
Hive - ${{ matrix.name }} skipped
Assertoor - ${{ matrix.name }} skipped
run-hive and run-assertoor both declare needs: [detect-changes, docker_build], so a broken image turns the whole suite into skips. Feeding that head's check runs through this loop prints All required suites passed on every queued pull request head.
Rejecting skipped outright is not the fix: an L1-only pull request legitimately shows all four L2 suites skipped (PR #7194 is exactly that), so it would become unmergeable. Telling the two apart needs a second signal, for example the head's own Integration Test / Integration Test L2 check run, or the pull_request run conclusion from /actions/runs?head_sha=....
There was a problem hiding this comment.
95a1f6f02
The prefix loop is gone — the gate now reads its own verdict on the queued head, so skipped is no longer a suite-level signal at all.
e104cdbc no longer passes. With the != 'skipped' guards dropped, that head's Check if any job failed step sees needs.run-assertoor.result = skipped and exits 1, so the gate itself is failure. skipped still passes, but now only means this gate was not required, which is the #7194 case you point at. A dependency failure cannot produce it.
| done | ||
| done <<<"$all_checks" | ||
|
|
||
| if [[ $matched -eq 0 ]]; then |
There was a problem hiding this comment.
matched is one counter across every prefix, so this net only fires when all prefixes come up empty. Rename or delete a single suite and it silently stops being enforced while the others keep the gate green.
Reproduced under bash with the L2 caller's four prefixes: rename state-diff-test's job, leave it red, and the script still prints All required suites passed and exits 0.
Worth noting that none of the six suite jobs carry the # "..." is a required check, don't change the name comment that all-tests has, and the prefix strings live in a third file with no link back to them. A per-prefix counter closes this.
There was a problem hiding this comment.
95a1f6f02
No prefix list remains. There is one gate name, and finding no job carrying it fails closed, so a rename cannot silently drop enforcement — and a suite added later is covered without touching the script.
| "repos/${GITHUB_REPOSITORY}/commits/${pr_head}/check-runs?per_page=100" | | ||
| jq -r '[.[].check_runs[]] | ||
| | group_by(.name) | ||
| | map(max_by(.started_at // "")) |
There was a problem hiding this comment.
This collapses same-named check runs from different workflows, keeping whichever started later.
daily_hive_report.yaml:24 declares name: Hive - ${{ matrix.test.name }} with a Rpc Compat tests matrix entry and continue-on-error: true, and it triggers on pull_request for .github/workflows/daily_hive_report.yaml and .github/scripts/publish_hive.sh. On PR #7170's head both workflows reported:
Hive - Rpc Compat tests success 2026-08-20T17:35:44Z (daily_hive_report)
Hive - Rpc Compat tests success 2026-08-20T17:53:25Z (pr-main_l1)
Which one survives is runner scheduling. Across ten recent daily runs its hive jobs concluded 104 success, 1 failure, 5 cancelled, so this can mask a red L1 hive, or fail the gate on a cancelled that blocks nothing.
Two smaller points on the same expression: the comment above attributes the dedupe to re-triggers, but commits/{sha}/check-runs already defaults to filter=latest, so that is not what it is doing. And started_at has second granularity, so ties resolve by API array order; .id descending would at least be deterministic. Matching on the workflow or .app.id alongside the name would be better than either.
There was a problem hiding this comment.
95a1f6f02
Check runs are no longer read. Jobs come from one workflow run resolved by .path (taken from GITHUB_WORKFLOW_REF), so daily_hive_report is out of scope by construction rather than by a tie-break on started_at. The inaccurate filter=latest comment went with it.
|
|
||
| failed=0 | ||
| for pr in "${pr_numbers[@]}"; do | ||
| pr_head=$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${pr}" --jq '.head.sha') |
There was a problem hiding this comment.
This resolves the pull request's head at gate time, not the SHA that was squashed into the merge group, and the merge_group payload carries no PR-head SHA to compare it against.
Every branch I could construct is fail-closed: a force-pushed head has no check runs, so matched == 0; a fresh push leaves them in_progress, so PENDING. What is lost is the property this file's header advertises, catching a result that turned red after queueing, during that window. Low priority, but a comment saying the live head is deliberate would help.
There was a problem hiding this comment.
95a1f6f02
Documented in the per-pull-request loop, including why every way the live head and the squashed commit can disagree is fail-closed.
| # status check, so a gate that skips is a gate that always passes: inside the | ||
| # merge queue, where assertoor and hive do not run, that let a pull request | ||
| # whose suites were red merge on a vacuous green. | ||
| if: ${{ always() && (needs.detect-changes.result != 'success' || needs.detect-changes.outputs.run_tests == 'true') }} |
There was a problem hiding this comment.
This still skips the gate whenever run_tests is false, and on merge_group both workflows set run_tests = code_changed, which matches only **/*.rs, **/*.toml and **/*.lock. Any pull request touching only workflows, scripts, fixtures or configs keeps the old behavior.
PR #7193 (CI-only) is the worked example: merge group run 32502079792 skipped every job including Integration Test, and it merged. Its head was not untested; it carried seven green Hive - * runs, because the pull_request filter is !crates/l2/** and .github/** passes it. The queue just never read them.
This pull request changes only .github/** .yaml and .sh, so it is in that class itself.
Separately, always() also runs the job when the run is cancelled. The old expression short-circuited on outputs.run_tests == 'true' being empty, so a concurrency-cancelled run skipped the gate; now it runs and the step below exits 1, stamping a red Integration Test on superseded commits. !cancelled() && (...) avoids that.
There was a problem hiding this comment.
35172eb57
The gate now runs on merge_group regardless of run_tests. A pull request that genuinely required nothing still passes, because the verdict it then reads on the head is skipped.
!cancelled() replaces always() for the reason you give: with the condition widened, a concurrency-cancelled run would otherwise run the gate and stamp a red required check on a superseded commit.
| exit 1 | ||
|
|
||
| - name: Checkout sources | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
Only the merge_group step uses the checked-out tree; the other two steps are inline shell over the needs context. if: ${{ github.event_name == 'merge_group' }} here would skip a full clone on every pull_request and push run of both gates.
There was a problem hiding this comment.
ae2f3dd9c
Gated on merge_group in both gates.
| if: ${{ github.event_name == 'merge_group' }} | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: ./.github/scripts/check-queued-pr-checks.sh "Hive - " "Assertoor - " |
There was a problem hiding this comment.
engine-ef-tests is in this job's needs, skips in the merge queue exactly like hive and assertoor, and is not a required context on its own, but it is missing from this prefix list. PR #7194's head carries a real Engine EF tests success run, so adding the prefix closes it in one line.
The Hive - prefix also over-matches: daily_hive_report.yaml emits check runs under that same prefix on any pull request touching its trigger paths. See the note on the jq dedupe in the script.
There was a problem hiding this comment.
95a1f6f02
Both halves are subsumed rather than patched. The gate reads its own verdict, which already covers everything in its needs, so Engine EF tests cannot be omitted from a list that no longer exists; and jobs are read from one workflow run, so daily_hive_report's Hive - cannot collide.
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| ./.github/scripts/check-queued-pr-checks.sh \ | ||
| "Integration Test - " \ |
There was a problem hiding this comment.
Integration Test - matches Integration Test - TDX (integration-test-tdx, line 509), but the Check if any job failed step below checks only integration-test, state-diff-test, uniswap-swap and integration-test-shared-bridge. TDX is in needs and has no continue-on-error.
So a red TDX leaves Integration Test L2 green on the pull request, the pull request is queueable, and the merge group then rejects it with nothing the author can turn green by re-running. Sampling twelve recent pr-main_l2 pull_request runs, TDX concluded 2 failure, 3 cancelled, 4 skipped, 1 success, so this is not a rare state.
Either add the tdx branch to the step below or narrow the prefix, but the two sides should agree on what is required.
There was a problem hiding this comment.
95a1f6f02
TDX stays not required, and the two sides now agree by construction: the merge group reads the pull_request gate's verdict, and that step is the only definition of what is required. Promoting a job that concluded failure or cancelled in 5 of your 12 sampled runs to a merge blocker felt like a policy change rather than a fix to this hole, so it is recorded under Not addressed here instead.
…uite check-run names. Matching check-run names on a commit was ambiguous in four ways, each of which let a real red result through: - A suite `skipped` because a dependency failed was indistinguishable from one skipped by design. Head e104cdb had `Build Docker` = failure, which turns every hive and assertoor job into a skip, and the old loop reported it green. - `matched` was one counter across all prefixes, so renaming or deleting a single suite silently stopped enforcing it while the others kept the gate green. - `group_by(.name) | max_by(.started_at)` collapsed same-named check runs from different workflows. `daily_hive_report.yaml` publishes `Hive - <name>` on any pull request touching its trigger paths, with `continue-on-error: true`, so which run survived was runner scheduling. - `Engine EF tests` was in the gate's `needs` and skips in the queue exactly like hive, but was missing from the prefix list, and `Integration Test - ` matched `Integration Test - TDX`, which the pull_request side deliberately does not require. The gate now resolves the latest `pull_request` run of its own workflow on each queued pull request's head, taking the workflow path from `GITHUB_WORKFLOW_REF` so it cannot drift, and reads the verdict of this same gate job inside that run. That leaves one definition of what is required — the `Check if any job failed` step — so the two sides cannot disagree about TDX or about any suite added later, and an unrelated workflow's identically named job is out of scope by construction. `skipped` still passes, but now means something checkable: the gate itself was not required, which is what an L1-only pull request looks like to the L2 workflow. A dependency failure no longer produces it, because dropping the `needs.<job>.result != 'skipped'` guards makes the gate run and fail in that case. A run that is not yet completed fails, which is the case this exists for: re-running a suite bumps the run attempt, so a re-run in flight blocks the merge group instead of being bypassed. Resolving the pull request's live head rather than the squashed commit is deliberate and now documented in the script: the merge_group payload carries no pull request head to compare against, reading the newest head is what catches a result that turned red after queueing, and every way the two can disagree is fail-closed.
… decided. `run_tests` on `merge_group` is `code_changed`, which matches only `**/*.rs`, `**/*.toml` and `**/*.lock`. Any pull request touching just workflows, scripts, fixtures or configs therefore still skipped the gate in the queue and turned both required checks green with nothing consulted — the exact hole this branch set out to close, and the class this branch itself belongs to. PR #7193 is the worked example: its merge group skipped every job, `Integration Test` included, and it merged, while its head carried seven green `Hive - *` results the queue never read. The gate now runs on `merge_group` regardless. A pull request whose changes genuinely required nothing still passes, because the verdict it reads on the head is then `skipped`. `!cancelled()` replaces `always()` at the same time. The old expression short-circuited on an empty `outputs.run_tests`, so a concurrency-cancelled run skipped the gate; with the condition widened it would instead run and fail, stamping a red required check on a commit that has already been superseded.
Only the `merge_group` step uses the checked-out tree; the pull_request and push paths evaluate the `needs` context in inline shell and need nothing from disk. Both gates cloned the repository on every run regardless.
2 participants
Motivation
A pull request whose
Hiverun was red, or whose re-run was still in flight, could mergethrough the merge queue without that result ever being consulted.
Integration TestandIntegration Test L2are required status checks onmain, and bothgate jobs bailed out whenever a dependency was skipped:
Hive, assertoor and the L2 suites are all excluded from
merge_groupto keep the queuecheap, so inside the queue that condition was always false and the gate job was skipped.
GitHub counts a skipped check run as satisfying a required status check, so every merge
group satisfied both requirements without running or even consulting the suites they exist to
enforce.
Merge when readycompounds it: GitHub decides queue eligibility when the pull request isenqueued and does not re-evaluate the head afterwards. A stale green
Integration Testisenough to get in, a later red result cannot evict it, and the merge group's own gate was
vacuous. So re-running a failed Hive job did not protect
main— the merge went ahead whilethe re-run was still going, and the re-run's failure landed after the merge.
Observed on three merges the same day:
Integration Teston its headfailureat 16:49:47failureat 17:03:50Both had a red
Hive - Devp2p tests, andmainwent red on devp2p immediately afterwardsuntil #7204 fixed the underlying discv5 bug.
Description
No suite is added to the merge queue. All fourteen
github.event_name != 'merge_group'exclusions are untouched, so hive, assertoor, reorg, engine-EF and the L2 suites still do not
run there, and the queue's cost profile is unchanged. What changes is what the gate does.
On the pull request side, three things stop the gate from passing vacuously:
always passes. This is also what makes a broken docker build — which turns every hive and
assertoor job into a skip — produce a red
Integration Testrather than no verdict at all.detect-changespublishes no outputs, so'' == 'true'skipped the gate andturned both required checks green with nothing evaluated. The gate now runs and fails.
!cancelled()replacesalways(), so a concurrency-cancelled run does not stamp a redrequired check on a commit that has already been superseded.
On
merge_groupthe gate runs.github/scripts/check-queued-pr-checks.sh, which assertsthat this same gate was green on each queued pull request's own head. Per queued pull
request:
compareAPI, notfrom the queue branch name, because a batched group's ref names only its last pull request.
pull_requestrun of this workflow on that pull request's head. Theworkflow path comes from
GITHUB_WORKFLOW_REFrather than an argument, so it cannot driftif the file is renamed.
completed. Re-running a suite bumps the run attempt, so a Hivere-run in flight now blocks the merge group instead of being bypassed — the case this pull
request is really about. Because this runs at merge time rather than at enqueue time, it
also catches a result that turned red after the pull request was queued.
successorskipped.Reading the gate's own verdict rather than matching suite check-run names is what keeps the
gate honest, and it is the part of this pull request worth reviewing closely:
Check if any job failedstep. A prefix list is a second definition that can disagree with it, which isexactly how
Integration Test - TDXended up matched by the merge-group side while thepull_request side does not require it, and how
Engine EF testsended up in the gate'sneedsbut in no prefix list. Neither is expressible now, and a suite added later iscovered without touching the script.
from one workflow run resolved by
.path, sodaily_hive_report.yaml— which publishesHive - <name>withcontinue-on-error: trueon any pull request touching its triggerpaths — is out of scope by construction rather than by a tie-break on
started_at.skippedremains a pass, but now means something checkable: this gate was not required,which is what an L1-only pull request looks like to the L2 workflow and a docs-only one to
both. A suite skipped for the wrong reason no longer produces it, because a failed
dependency makes the gate run and fail.
is verifying must not report success.
One more hole closed on the same dependency: on
merge_group,run_testsiscode_changed,which matches only
**/*.rs,**/*.tomland**/*.lock. Any pull request touching justworkflows, scripts, fixtures or configs therefore skipped the gate in the queue anyway — this
branch included. PR #7193 is the worked example: its merge group skipped every job,
Integration Testamong them, and it merged, while its head carried seven greenHive - *results the queue never read. The gate now runs on
merge_groupregardless ofrun_tests; apull request that genuinely required nothing still passes, because the verdict it then reads
on the head is
skipped.The gate's
checkoutis also limited to themerge_grouppath, the only one that reads thetree.
Verification
The gate's decision logic is not something CI can exercise, so it was driven directly against
live repository data, with only the merge group's
compareresponse stubbed so the case undertest could name any pull request. Fifteen cases, all as expected:
skippedIntegration Test= failure, redHive - Rpc Compat tests)in_progress)action_required, zero jobs)(#N)in any subjectmerge_groupeventGITHUB_WORKFLOW_REFunsetHead
e104cdbc— the docker-build failure that producedBuild Docker= failure with hiveand assertoor
skipped— is the case the old prefix loop reported green. It is not in thetable because its recorded gate verdict predates this branch: with the
!= 'skipped'guardsdropped, that head's
Check if any job failedstep seesneeds.run-assertoor.result=skippedand exits 1, so the gate isfailureand the merge group blocks.Known limitations
If the merge group's gate has already reported green and a Hive job is re-run after that, the
merge can still complete. Closing that would need the suites to run in the queue, which is the
trade this pull request deliberately does not make: it would add roughly twenty minutes per
queued pull request and let a hive flake evict pull requests from the queue.
Not addressed here
Integration Testis declared by three workflows:pr-main_l1.yaml,pr-main_l1_l2_dev.yaml(which also runs onmerge_group) andpr-main_levm.yaml. Arequired check name shared by several workflows is ambiguous to branch protection — on PR
fix(l1): spend blob cost out of the balance
eth_estimateGasrecaps against #7209's head the L1 one wasfailurewhile the L2-Dev one wassuccess, and it merged.This pull request makes
pr-main_l1.yaml's own verdict real in the queue, but resolving thecollision needs one of the jobs renamed and the required-check settings updated together,
which cannot be done from a pull request alone.
check-cargo-locksis in the L1 gate'sneedsbut its result is still never inspected, soCheck Cargo.lockremains unenforced by the required check. That is a separate policy call.Integration Test - TDXstays not required. It is in the L2 gate'sneedswith nocontinue-on-error, and concluded failure or cancelled in 5 of 12 sampledpull_requestruns; promoting it to a merge blocker is a policy change, not a fix to this hole. Both sides
now agree that it does not gate.
Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.