Skip to content

ci(l1,l2): stop the merge queue satisfying the required integration checks with a skipped job #7213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ilitteri wants to merge 5 commits into
base: main
Choose a base branch
from
Open

ci(l1,l2): stop the merge queue satisfying the required integration checks with a skipped job #7213

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bdab8f1
Make the merge queue enforce the required integration checks instead …
ilitteri Aug 24, 2026
f21035d
Fail the integration gate when change detection itself does not concl…
ilitteri Aug 24, 2026
95a1f6f
Read the queued pull request's own gate verdict instead of matching s…
ilitteri Aug 31, 2026
35172eb
Run the integration gate in the merge queue whatever change detection…
ilitteri Aug 31, 2026
ae2f3dd
Skip the gate's checkout outside the merge queue.
ilitteri Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 105 additions & 0 deletions .github/scripts/check-queued-pr-checks.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
#!/usr/bin/env bash
#
# Assert that every check run matching one of the given name prefixes concluded
# successfully on the head commit of each pull request in the current merge group.
#
# Usage: check-queued-pr-checks.sh "Hive - " "Assertoor - " ...
#
# Why this exists: the expensive suites (hive, assertoor) are deliberately not
# re-run inside the merge queue, so the required gate job has nothing of its own
# to inspect there. Skipping the gate instead is not a safe substitute — GitHub
# counts a skipped check run as satisfying a required status check, so the queue
# would merge a pull request whose suites were red or still running. Reading the
# pull request's own results here keeps the queue cheap while making the
# requirement real, and because it runs at merge time it also catches a result
# that turned red (or was re-triggered) after the pull request was queued.
set -euo pipefail

if [[ $# -eq 0 ]]; then
echo "usage: $0 <check-name-prefix> [<check-name-prefix> ...]" >&2
exit 2
fi

: "${GITHUB_REPOSITORY:?}"
: "${GITHUB_EVENT_PATH:?}"

base_sha=$(jq -r '.merge_group.base_sha' "$GITHUB_EVENT_PATH")
head_sha=$(jq -r '.merge_group.head_sha' "$GITHUB_EVENT_PATH")

if [[ -z "$base_sha" || "$base_sha" == "null" || -z "$head_sha" || "$head_sha" == "null" ]]; then
echo "No merge_group payload found; this script only runs on merge_group events." >&2
exit 2
fi

# One squashed commit per queued pull request, each titled "... (#1234)". Read
# them over the API rather than from a checkout so the job needs no clone.
mapfile -t pr_numbers < <(
gh api "repos/${GITHUB_REPOSITORY}/compare/${base_sha}...${head_sha}" \
--jq '.commits[].commit.message | split("\n")[0]' |
grep -oE '\(#[0-9]+\)$' |
tr -d '(#)' |
sort -u
)

if [[ ${#pr_numbers[@]} -eq 0 ]]; then
echo "Could not identify any pull request in merge group ${base_sha}..${head_sha}." >&2
echo "Refusing to pass: a gate that cannot find what to verify must not report success." >&2
exit 1
fi

echo "Merge group covers pull request(s): ${pr_numbers[*]}"

failed=0
for pr in "${pr_numbers[@]}"; do
pr_head=$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${pr}" --jq '.head.sha')

@MegaRedHand MegaRedHand Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilitteri ilitteri Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

95a1f6f02

Documented in the per-pull-request loop, including why every way the live head and the squashed commit can disagree is fail-closed.

echo "::group::PR #${pr} (head ${pr_head})"

# One row per check name, tab separated, keeping only the most recently started
# run of that name. A single commit can carry several check suites (a re-trigger
# creates a new suite rather than updating the old one), and without this a
# superseded red run would keep blocking a head that is now green.
all_checks=$(
gh api --paginate --slurp \
"repos/${GITHUB_REPOSITORY}/commits/${pr_head}/check-runs?per_page=100" |
jq -r '[.[].check_runs[]]
| group_by(.name)
| map(max_by(.started_at // ""))

@MegaRedHand MegaRedHand Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilitteri ilitteri Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| .[]
| [.name, .status, (.conclusion // "")]
| @tsv'
)

matched=0
while IFS=$'\t' read -r name status conclusion; do
[[ -z "$name" ]] && continue
for prefix in "$@"; do
if [[ "$name" == "$prefix"* ]]; then
matched=$((matched + 1))
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

@MegaRedHand MegaRedHand Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=....

@ilitteri ilitteri Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

echo "FAILED ${name} (conclusion=${conclusion})"
failed=1
else
echo "ok ${name} (${conclusion})"
fi
break
fi
done
done <<<"$all_checks"

if [[ $matched -eq 0 ]]; then

@MegaRedHand MegaRedHand Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilitteri ilitteri Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

echo "No check runs matching [$*] on PR #${pr}."
echo "Refusing to pass: the suites this gate exists to enforce never reported."
failed=1
fi
echo "::endgroup::"
done

if [[ $failed -ne 0 ]]; then
echo "Required suites did not pass on the queued pull request head(s)." >&2
exit 1
fi

echo "All required suites passed on every queued pull request head."
38 changes: 35 additions & 3 deletions .github/workflows/pr-main_l1.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,10 +468,40 @@ jobs:
name: Integration Test
runs-on: ubuntu-latest
needs: [detect-changes, run-assertoor, run-hive, check-cargo-locks, engine-ef-tests]
# Make sure this job runs even if the previous jobs failed or were skipped
if: ${{ needs.detect-changes.outputs.run_tests == 'true' && always() && needs.run-assertoor.result != 'skipped' && needs.run-hive.result != 'skipped' }}
# Runs even when a dependency failed, and deliberately does not bail out on a
# skipped one. GitHub counts a skipped check run as satisfying a required
# 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') }}

@MegaRedHand MegaRedHand Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilitteri ilitteri Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

permissions:
contents: read
checks: read
pull-requests: read
steps:
- name: Fail if change detection did not conclude
if: ${{ needs.detect-changes.result != 'success' }}
run: |
# Without a run_tests answer there is no way to tell whether the suites
# below were required, and an unanswerable gate must not report success.
echo "detect-changes concluded '${{ needs.detect-changes.result }}'"
exit 1

- name: Checkout sources
uses: actions/checkout@v6

@MegaRedHand MegaRedHand Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilitteri ilitteri Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ae2f3dd9c

Gated on merge_group in both gates.


# Assertoor and hive are skipped in the merge queue to keep it cheap, so
# there is no local result to inspect. Read the queued pull request's own
# results instead, which also catches a suite that turned red or was
# re-triggered after the pull request was added to the queue.
- name: Check the queued pull request's suites
if: ${{ github.event_name == 'merge_group' }}
env:
GH_TOKEN: ${{ github.token }}
run: ./.github/scripts/check-queued-pr-checks.sh "Hive - " "Assertoor - "

@MegaRedHand MegaRedHand Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilitteri ilitteri Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


- name: Check if any job failed
if: ${{ github.event_name != 'merge_group' }}
run: |
if [ "${{ needs.run-assertoor.result }}" != "success" ]; then
echo "Job Assertoor Tx Check failed"
Expand All @@ -483,7 +513,9 @@ jobs:
exit 1
fi

# engine-ef-tests is skipped in the merge queue (merge_group), which is OK.
# Tolerating a skipped engine-ef-tests is defensive rather than load
# bearing: outside the merge queue it only skips when run_tests is
# false, and then this job does not run either.
if [ "${{ needs.engine-ef-tests.result }}" != "success" ] && [ "${{ needs.engine-ef-tests.result }}" != "skipped" ]; then
echo "Job Engine EF tests failed"
exit 1
Expand Down
39 changes: 37 additions & 2 deletions .github/workflows/pr-main_l2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,10 +972,45 @@ jobs:
uniswap-swap,
integration-test-shared-bridge,
]
# Make sure this job runs even if the previous jobs failed or were skipped
if: ${{ needs.detect-changes.outputs.run_tests == 'true' && always() && needs.integration-test.result != 'skipped' && needs.state-diff-test.result != 'skipped' && needs.integration-test-tdx.result != 'skipped' && needs.uniswap-swap.result != 'skipped' && needs.integration-test-shared-bridge.result != 'skipped' }}
# Runs even when a dependency failed, and deliberately does not bail out on a
# skipped one. GitHub counts a skipped check run as satisfying a required
# status check, so a gate that skips is a gate that always passes: inside the
# merge queue, where none of these suites 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') }}
permissions:
contents: read
checks: read
pull-requests: read
steps:
- name: Fail if change detection did not conclude
if: ${{ needs.detect-changes.result != 'success' }}
run: |
# Without a run_tests answer there is no way to tell whether the suites
# below were required, and an unanswerable gate must not report success.
echo "detect-changes concluded '${{ needs.detect-changes.result }}'"
exit 1

- name: Checkout sources
uses: actions/checkout@v6

# These suites are skipped in the merge queue to keep it cheap, so there is
# no local result to inspect. Read the queued pull request's own results
# instead, which also catches a suite that turned red or was re-triggered
# after the pull request was added to the queue.
- name: Check the queued pull request's suites
if: ${{ github.event_name == 'merge_group' }}
env:
GH_TOKEN: ${{ github.token }}
run: |
./.github/scripts/check-queued-pr-checks.sh \
"Integration Test - " \

@MegaRedHand MegaRedHand Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ilitteri ilitteri Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"State Reconstruction Tests" \
"Uniswap Swap Token Flow" \
"Integration Test Shared Bridge - "

- name: Check if any job failed
if: ${{ github.event_name != 'merge_group' }}
run: |
if [ "${{ needs.integration-test.result }}" != "success" ]; then
echo "Job Integration Tests failed"
Expand Down
Loading