From 9703f1e11988169a5af54cd001d160ae07833cd6 Mon Sep 17 00:00:00 2001 From: Kyle Date: Wed, 26 Aug 2026 03:05:51 -0700 Subject: [PATCH 1/4] [ci]: do not abort the merge-gate trigger when cancelling old builds fails The Trigger Merge Gate workflow cancels stale Buildkite builds in step 2 and starts the merge-gate build in step 7. A step failure ends the job, so when the cancel step dies the gate build is never started at all. The check then goes red for a reason unrelated to the pull request, and nothing in the log distinguishes "the tests failed" from "the tests never ran". That is what happened on #1710: jq: error (at :1): Cannot index string with string "env" Process completed with exit code 5 `curl` is called without `--fail-with-body`, so an HTTP error is treated as success and its body is piped onward. Buildkite answers a rate limit or an unauthorized read with an object such as {"message": "Not Found"}, and `.[]` over an object yields its values, so `.env` then runs against a string and jq exits non-zero. Three changes, all to the same step: `continue-on-error: true`, because cancelling stale builds only saves agent time. Failing to cancel wastes an agent; failing to trigger means untested code, and step 7 keeps its hard failure. `--fail-with-body` on the lookup, with the response echoed as a warning, so a lookup that fails says why instead of feeding an error body to the parser. `if type == "array" then .[] else empty end` in the filter, so a response that is not a build list yields no matches rather than aborting. Verified against six response shapes: a matching build, a build for another PR, the error object from #1710, an empty array, a build with no env, and a bare string. The first returns the build number and the rest return nothing, where the error object previously exited 5. --- .github/workflows/ci-trigger-full-suite.yml | 29 +++++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci-trigger-full-suite.yml b/.github/workflows/ci-trigger-full-suite.yml index e96ad9c813..4ecfdcde14 100644 --- a/.github/workflows/ci-trigger-full-suite.yml +++ b/.github/workflows/ci-trigger-full-suite.yml @@ -38,19 +38,38 @@ jobs: if (!hasReady) core.info('No ready label — skipping merge-gate trigger.'); - name: Cancel previous Buildkite builds + # Cancelling stale builds only saves agent time. If it cannot run, the + # merge gate must still be triggered by the steps below, so a failure + # here is reported and stepped over rather than ending the job. + continue-on-error: true if: steps.check.outputs.has_ready == 'true' env: BUILDKITE_API_TOKEN: ${{ secrets.BUILDKITE_API_TOKEN }} PR_BRANCH: ${{ github.event.pull_request.head.ref }} PR_NUMBER: ${{ github.event.pull_request.number }} run: | - # Match both branch and PR number: forks can reuse the same branch name. - builds=$(curl -sS --get -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \ + # `curl` without --fail-with-body treats an HTTP error as success and + # pipes the error body onward. Buildkite answers a rate limit or an + # unauthorized read with an object such as {"message": "Not Found"}, + # and `.[]` over an object yields its values, so `.env` then runs + # against a string and jq exits non-zero. + if ! response=$(curl -sS --get --fail-with-body \ + -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \ --data-urlencode "branch=$PR_BRANCH" \ --data-urlencode "state=running,scheduled" \ - "https://api.buildkite.com/v2/organizations/${{ vars.BUILDKITE_ORG_SLUG }}/pipelines/${{ vars.BUILDKITE_PIPELINE_SLUG }}/builds" \ - | jq -r --arg pr_number "$PR_NUMBER" \ - '.[] | select((.env.TEST_SCOPE? == "merge") and (.env.PR_NUMBER? == $pr_number)) | .number') + "https://api.buildkite.com/v2/organizations/${{ vars.BUILDKITE_ORG_SLUG }}/pipelines/${{ vars.BUILDKITE_PIPELINE_SLUG }}/builds" 2>&1); then + echo "::warning::Could not list Buildkite builds, skipping cancellation: $response" + exit 0 + fi + + # Match both branch and PR number: forks can reuse the same branch name. + if ! builds=$(printf '%s' "$response" | jq -r --arg pr_number "$PR_NUMBER" \ + 'if type == "array" then .[] else empty end + | select((.env.TEST_SCOPE? == "merge") and (.env.PR_NUMBER? == $pr_number)) + | .number' 2>&1); then + echo "::warning::Could not parse the Buildkite build list, skipping cancellation: $builds" + exit 0 + fi for build_num in $builds; do echo "Cancelling Buildkite build #$build_num" curl -sS -X PUT -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \ From 2dc322b60b24507834b447019d71ff75a58adb10 Mon Sep 17 00:00:00 2001 From: William Lin <8941107+SolitaryThinker@users.noreply.github.com> Date: Wed, 26 Aug 2026 04:20:24 -0700 Subject: [PATCH 2/4] [ci]: harden best-effort Buildkite cancellation --- .github/workflows/ci-trigger-full-suite.yml | 86 ++++++++++++++----- .../tests/contract/test_ci_test_collection.py | 39 +++++++++ 2 files changed, 105 insertions(+), 20 deletions(-) diff --git a/.github/workflows/ci-trigger-full-suite.yml b/.github/workflows/ci-trigger-full-suite.yml index 4ecfdcde14..5494dd7c6c 100644 --- a/.github/workflows/ci-trigger-full-suite.yml +++ b/.github/workflows/ci-trigger-full-suite.yml @@ -44,37 +44,83 @@ jobs: continue-on-error: true if: steps.check.outputs.has_ready == 'true' env: + BK_ORG: ${{ vars.BUILDKITE_ORG_SLUG }} + BK_PIPELINE: ${{ vars.BUILDKITE_PIPELINE_SLUG }} BUILDKITE_API_TOKEN: ${{ secrets.BUILDKITE_API_TOKEN }} PR_BRANCH: ${{ github.event.pull_request.head.ref }} PR_NUMBER: ${{ github.event.pull_request.number }} run: | - # `curl` without --fail-with-body treats an HTTP error as success and - # pipes the error body onward. Buildkite answers a rate limit or an - # unauthorized read with an object such as {"message": "Not Found"}, - # and `.[]` over an object yields its values, so `.env` then runs - # against a string and jq exits non-zero. - if ! response=$(curl -sS --get --fail-with-body \ + set -euo pipefail + response_file=$(mktemp) + builds_file=$(mktemp) + trap 'rm -f "$response_file" "$builds_file"' EXIT + + if [[ ! "$PR_NUMBER" =~ ^[1-9][0-9]*$ ]]; then + echo "::warning::Invalid pull request number; stale Buildkite builds may continue." + exit 1 + fi + + if ! curl -sS --fail-with-body --get \ -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \ --data-urlencode "branch=$PR_BRANCH" \ - --data-urlencode "state=running,scheduled" \ - "https://api.buildkite.com/v2/organizations/${{ vars.BUILDKITE_ORG_SLUG }}/pipelines/${{ vars.BUILDKITE_PIPELINE_SLUG }}/builds" 2>&1); then - echo "::warning::Could not list Buildkite builds, skipping cancellation: $response" - exit 0 + --data-urlencode "state[]=running" \ + --data-urlencode "state[]=scheduled" \ + --data-urlencode "exclude_jobs=true" \ + --data-urlencode "exclude_pipeline=true" \ + --output "$response_file" \ + "https://api.buildkite.com/v2/organizations/${BK_ORG}/pipelines/${BK_PIPELINE}/builds"; then + echo "::warning::Could not list Buildkite builds; stale merge-gate builds may continue." + exit 1 + fi + + if ! jq -e ' + if type != "array" then false + else all(.[]; + if type != "object" then false + else + (.number | if type == "number" then . > 0 and floor == . else false end) + and ( + (.env? | if . == null then {} else . end) as $env + | if ($env | type) != "object" then false + else + ($env.TEST_SCOPE? | . == null or type == "string") + and ($env.PR_NUMBER? | . == null or type == "string") + end + ) + end + ) + end + ' "$response_file" >/dev/null 2>&1; then + # Do not print the response body: it is remote data and may contain + # multiline values that would be interpreted as workflow commands. + echo "::warning::Buildkite returned an invalid build list; stale merge-gate builds may continue." + exit 1 fi # Match both branch and PR number: forks can reuse the same branch name. - if ! builds=$(printf '%s' "$response" | jq -r --arg pr_number "$PR_NUMBER" \ - 'if type == "array" then .[] else empty end - | select((.env.TEST_SCOPE? == "merge") and (.env.PR_NUMBER? == $pr_number)) - | .number' 2>&1); then - echo "::warning::Could not parse the Buildkite build list, skipping cancellation: $builds" - exit 0 + if ! jq -r --arg pr_number "$PR_NUMBER" ' + .[] + | select((.env.TEST_SCOPE? == "merge") and (.env.PR_NUMBER? == $pr_number)) + | .number + ' "$response_file" > "$builds_file"; then + echo "::warning::Could not select stale Buildkite builds; stale merge-gate builds may continue." + exit 1 fi - for build_num in $builds; do + + cancellation_failed=0 + while IFS= read -r build_num; do echo "Cancelling Buildkite build #$build_num" - curl -sS -X PUT -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \ - "https://api.buildkite.com/v2/organizations/${{ vars.BUILDKITE_ORG_SLUG }}/pipelines/${{ vars.BUILDKITE_PIPELINE_SLUG }}/builds/${build_num}/cancel" - done + if ! curl -sS --fail-with-body -o /dev/null -X PUT \ + -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \ + "https://api.buildkite.com/v2/organizations/${BK_ORG}/pipelines/${BK_PIPELINE}/builds/${build_num}/cancel"; then + echo "::warning::Could not cancel Buildkite build #$build_num; trying remaining builds." + cancellation_failed=1 + fi + done < "$builds_file" + + if (( cancellation_failed != 0 )); then + exit 1 + fi # Check out the immutable BASE SHA: pull_request_target must never run a # planner or gate script from the untrusted PR head. diff --git a/fastvideo/tests/contract/test_ci_test_collection.py b/fastvideo/tests/contract/test_ci_test_collection.py index 4a0e0b1798..f8c065a495 100644 --- a/fastvideo/tests/contract/test_ci_test_collection.py +++ b/fastvideo/tests/contract/test_ci_test_collection.py @@ -164,6 +164,45 @@ def test_merge_comment_has_one_change_aware_trigger_path(): assert "__FASTVIDEO_CI_PLAN_ALL__" in ready_workflow +def test_merge_gate_buildkite_cancellation_is_best_effort_and_strict(): + workflow = yaml.safe_load((REPO_ROOT / ".github/workflows/ci-trigger-full-suite.yml").read_text()) + steps = workflow["jobs"]["trigger"]["steps"] + cancel_step = next(step for step in steps if step.get("name") == "Cancel previous Buildkite builds") + trigger_step = next(step for step in steps if step.get("name") == "Trigger Buildkite merge gate") + cancel_script = cancel_step["run"] + + # Stale-build cleanup is best effort; creating the replacement gate remains + # a hard failure so merge protection can never pass without a test build. + assert cancel_step["continue-on-error"] is True + assert trigger_step.get("continue-on-error") is not True + assert "curl -sS --fail-with-body -X POST" in trigger_step["run"] + + assert cancel_script.splitlines()[0] == "set -euo pipefail" + assert "curl -sS --fail-with-body --get" in cancel_script + assert cancel_script.count('--data-urlencode "state[]=running"') == 1 + assert cancel_script.count('--data-urlencode "state[]=scheduled"') == 1 + assert cancel_script.count('--data-urlencode "exclude_jobs=true"') == 1 + assert cancel_script.count('--data-urlencode "exclude_pipeline=true"') == 1 + assert 'state=running,scheduled' not in cancel_script + + # Only a validated array of build records can reach the URL construction. + assert 'if type != "array" then false' in cancel_script + assert 'if type != "object" then false' in cancel_script + assert 'if type == "number" then . > 0 and floor == .' in cancel_script + assert 'if ($env | type) != "object" then false' in cancel_script + assert '$env.TEST_SCOPE?' in cancel_script + assert '$env.PR_NUMBER?' in cancel_script + + # API bodies are kept out of annotations, every cancellation is checked, + # and one failure does not stop attempts for the remaining build numbers. + assert '--output "$response_file"' in cancel_script + assert 'cat "$response_file"' not in cancel_script + assert "if ! curl -sS --fail-with-body -o /dev/null -X PUT" in cancel_script + assert "cancellation_failed=1" in cancel_script + assert cancel_script.index("cancellation_failed=1") < cancel_script.index('done < "$builds_file"') + assert cancel_script.index('done < "$builds_file"') < cancel_script.index("if (( cancellation_failed != 0 ))") + + def test_full_ssim_has_a_weekly_slurm_schedule(): workflow = (REPO_ROOT / ".github/workflows/ci-scheduled-ssim.yml").read_text() ssim_step = next(step for step in _pipeline_steps() if step["key"] == "ssim") From 6a80eecc4867cca46b8f300dc7e0ab142fefc807 Mon Sep 17 00:00:00 2001 From: William Lin <8941107+SolitaryThinker@users.noreply.github.com> Date: Wed, 26 Aug 2026 04:33:22 -0700 Subject: [PATCH 3/4] [ci]: bound stale Buildkite cleanup --- .github/workflows/ci-trigger-full-suite.yml | 6 ++++-- fastvideo/tests/contract/test_ci_test_collection.py | 9 +++++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci-trigger-full-suite.yml b/.github/workflows/ci-trigger-full-suite.yml index 5494dd7c6c..b8d2213d4c 100644 --- a/.github/workflows/ci-trigger-full-suite.yml +++ b/.github/workflows/ci-trigger-full-suite.yml @@ -42,6 +42,7 @@ jobs: # merge gate must still be triggered by the steps below, so a failure # here is reported and stepped over rather than ending the job. continue-on-error: true + timeout-minutes: 3 if: steps.check.outputs.has_ready == 'true' env: BK_ORG: ${{ vars.BUILDKITE_ORG_SLUG }} @@ -60,11 +61,12 @@ jobs: exit 1 fi - if ! curl -sS --fail-with-body --get \ + if ! curl -sS --fail-with-body --connect-timeout 5 --max-time 20 --get \ -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \ --data-urlencode "branch=$PR_BRANCH" \ --data-urlencode "state[]=running" \ --data-urlencode "state[]=scheduled" \ + --data-urlencode "state[]=failing" \ --data-urlencode "exclude_jobs=true" \ --data-urlencode "exclude_pipeline=true" \ --output "$response_file" \ @@ -110,7 +112,7 @@ jobs: cancellation_failed=0 while IFS= read -r build_num; do echo "Cancelling Buildkite build #$build_num" - if ! curl -sS --fail-with-body -o /dev/null -X PUT \ + if ! curl -sS --fail-with-body --connect-timeout 5 --max-time 20 -o /dev/null -X PUT \ -H "Authorization: Bearer $BUILDKITE_API_TOKEN" \ "https://api.buildkite.com/v2/organizations/${BK_ORG}/pipelines/${BK_PIPELINE}/builds/${build_num}/cancel"; then echo "::warning::Could not cancel Buildkite build #$build_num; trying remaining builds." diff --git a/fastvideo/tests/contract/test_ci_test_collection.py b/fastvideo/tests/contract/test_ci_test_collection.py index f8c065a495..c7977f7da2 100644 --- a/fastvideo/tests/contract/test_ci_test_collection.py +++ b/fastvideo/tests/contract/test_ci_test_collection.py @@ -174,13 +174,17 @@ def test_merge_gate_buildkite_cancellation_is_best_effort_and_strict(): # Stale-build cleanup is best effort; creating the replacement gate remains # a hard failure so merge protection can never pass without a test build. assert cancel_step["continue-on-error"] is True + assert cancel_step["timeout-minutes"] == 3 assert trigger_step.get("continue-on-error") is not True assert "curl -sS --fail-with-body -X POST" in trigger_step["run"] assert cancel_script.splitlines()[0] == "set -euo pipefail" - assert "curl -sS --fail-with-body --get" in cancel_script + assert cancel_script.count("--connect-timeout 5") == 2 + assert cancel_script.count("--max-time 20") == 2 + assert "curl -sS --fail-with-body --connect-timeout 5 --max-time 20 --get" in cancel_script assert cancel_script.count('--data-urlencode "state[]=running"') == 1 assert cancel_script.count('--data-urlencode "state[]=scheduled"') == 1 + assert cancel_script.count('--data-urlencode "state[]=failing"') == 1 assert cancel_script.count('--data-urlencode "exclude_jobs=true"') == 1 assert cancel_script.count('--data-urlencode "exclude_pipeline=true"') == 1 assert 'state=running,scheduled' not in cancel_script @@ -197,7 +201,8 @@ def test_merge_gate_buildkite_cancellation_is_best_effort_and_strict(): # and one failure does not stop attempts for the remaining build numbers. assert '--output "$response_file"' in cancel_script assert 'cat "$response_file"' not in cancel_script - assert "if ! curl -sS --fail-with-body -o /dev/null -X PUT" in cancel_script + assert ("if ! curl -sS --fail-with-body --connect-timeout 5 --max-time 20 " + "-o /dev/null -X PUT") in cancel_script assert "cancellation_failed=1" in cancel_script assert cancel_script.index("cancellation_failed=1") < cancel_script.index('done < "$builds_file"') assert cancel_script.index('done < "$builds_file"') < cancel_script.index("if (( cancellation_failed != 0 ))") From 4c65c8e61df9efc33caebd5e69ddea9145b4263c Mon Sep 17 00:00:00 2001 From: William Lin <8941107+SolitaryThinker@users.noreply.github.com> Date: Wed, 26 Aug 2026 04:51:34 -0700 Subject: [PATCH 4/4] [ci]: invoke merge gate from slash command --- .github/workflows/ci-slash-commands.yml | 26 +++++++- .github/workflows/ci-trigger-full-suite.yml | 63 ++++++++++++++----- docs/contributing/ci_architecture.md | 3 +- docs/contributing/pull_requests.md | 3 +- .../tests/contract/test_ci_test_collection.py | 31 +++++++-- 5 files changed, 101 insertions(+), 25 deletions(-) diff --git a/.github/workflows/ci-slash-commands.yml b/.github/workflows/ci-slash-commands.yml index 2755a7bb6d..495bc7f709 100644 --- a/.github/workflows/ci-slash-commands.yml +++ b/.github/workflows/ci-slash-commands.yml @@ -32,7 +32,7 @@ jobs: } core.setOutput('has_write', String(hasWrite)); - - name: Add ready label and react + - name: Add ready label if: steps.perm.outputs.has_write == 'true' uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 with: @@ -40,14 +40,34 @@ jobs: const owner = context.repo.owner; const repo = context.repo.repo; const prNumber = context.payload.issue.number; - try { await github.rest.issues.removeLabel({ owner, repo, issue_number: prNumber, name: 'ready' }); } catch {} await github.rest.issues.addLabels({ owner, repo, issue_number: prNumber, labels: ['ready'] }); + + - name: React to comment + if: steps.perm.outputs.has_write == 'true' + continue-on-error: true + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | await github.rest.reactions.createForIssueComment({ - owner, repo, + owner: context.repo.owner, + repo: context.repo.repo, comment_id: context.payload.comment.id, content: 'rocket', }); + trigger-merge-gate: + needs: handle-merge + if: needs.handle-merge.result == 'success' + permissions: + actions: read + contents: read + pull-requests: read + uses: ./.github/workflows/ci-trigger-full-suite.yml + with: + pr_number: ${{ github.event.issue.number }} + secrets: + BUILDKITE_API_TOKEN: ${{ secrets.BUILDKITE_API_TOKEN }} + parse-command: if: >- github.event.issue.pull_request != null diff --git a/.github/workflows/ci-trigger-full-suite.yml b/.github/workflows/ci-trigger-full-suite.yml index b8d2213d4c..5a8573f016 100644 --- a/.github/workflows/ci-trigger-full-suite.yml +++ b/.github/workflows/ci-trigger-full-suite.yml @@ -3,6 +3,15 @@ name: Trigger Merge Gate on: pull_request_target: types: [labeled, synchronize] + workflow_call: + inputs: + pr_number: + description: Pull request number to enter into the merge gate + required: true + type: number + secrets: + BUILDKITE_API_TOKEN: + required: true permissions: contents: read @@ -10,13 +19,14 @@ permissions: actions: read concurrency: - group: merge-gate-${{ github.event.pull_request.number }} + group: merge-gate-${{ inputs.pr_number || github.event.pull_request.number }} cancel-in-progress: false jobs: trigger: if: >- - (github.event.action == 'labeled' && github.event.label.name == 'ready') + inputs.pr_number > 0 + || (github.event.action == 'labeled' && github.event.label.name == 'ready') || github.event.action == 'synchronize' runs-on: ubuntu-latest # Gate below may wait for cheap checks (up to MAX_WAIT_SECS = 25 min). @@ -25,16 +35,39 @@ jobs: - name: Check ready label id: check uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + env: + CALLED_PR_NUMBER: ${{ inputs.pr_number }} with: script: | + const eventPrNumber = context.payload.pull_request?.number; + const calledPrNumber = Number(process.env.CALLED_PR_NUMBER); + const prNumber = eventPrNumber ?? calledPrNumber; + if (!Number.isSafeInteger(prNumber) || prNumber <= 0) { + core.setFailed(`Invalid pull request number: ${process.env.CALLED_PR_NUMBER}`); + return; + } const { data: pr } = await github.rest.pulls.get({ owner: context.repo.owner, repo: context.repo.repo, - pull_number: context.payload.pull_request.number, + pull_number: prNumber, }); + if (pr.state !== 'open') { + core.setFailed(`PR #${prNumber} is not open.`); + return; + } + if (pr.base.repo.full_name !== context.payload.repository.full_name + || pr.base.ref !== context.payload.repository.default_branch) { + core.setFailed(`PR #${prNumber} does not target this repository's default branch.`); + return; + } const hasReady = pr.labels.some(l => l.name === 'ready'); core.setOutput('has_ready', String(hasReady)); core.setOutput('changed_files', String(pr.changed_files)); + core.setOutput('pr_number', String(pr.number)); + core.setOutput('head_sha', pr.head.sha); + core.setOutput('head_ref', pr.head.ref); + core.setOutput('base_sha', pr.base.sha); + core.setOutput('title', pr.title); if (!hasReady) core.info('No ready label — skipping merge-gate trigger.'); - name: Cancel previous Buildkite builds @@ -48,8 +81,8 @@ jobs: BK_ORG: ${{ vars.BUILDKITE_ORG_SLUG }} BK_PIPELINE: ${{ vars.BUILDKITE_PIPELINE_SLUG }} BUILDKITE_API_TOKEN: ${{ secrets.BUILDKITE_API_TOKEN }} - PR_BRANCH: ${{ github.event.pull_request.head.ref }} - PR_NUMBER: ${{ github.event.pull_request.number }} + PR_BRANCH: ${{ steps.check.outputs.head_ref }} + PR_NUMBER: ${{ steps.check.outputs.pr_number }} run: | set -euo pipefail response_file=$(mktemp) @@ -124,20 +157,20 @@ jobs: exit 1 fi - # Check out the immutable BASE SHA: pull_request_target must never run a - # planner or gate script from the untrusted PR head. + # Check out the immutable BASE SHA: neither pull_request_target nor the + # privileged slash-command call may run code from the untrusted PR head. - name: Checkout trusted merge planner if: steps.check.outputs.has_ready == 'true' uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: - ref: ${{ github.event.pull_request.base.sha }} + ref: ${{ steps.check.outputs.base_sha }} persist-credentials: false - name: Collect changed paths if: steps.check.outputs.has_ready == 'true' env: GH_TOKEN: ${{ github.token }} - PR_NUMBER: ${{ github.event.pull_request.number }} + PR_NUMBER: ${{ steps.check.outputs.pr_number }} EXPECTED_CHANGED_FILES: ${{ steps.check.outputs.changed_files }} run: | set -euo pipefail @@ -172,18 +205,18 @@ jobs: if: steps.check.outputs.has_ready == 'true' env: GH_TOKEN: ${{ github.token }} - PR_SHA: ${{ github.event.pull_request.head.sha }} - PR_NUMBER: ${{ github.event.pull_request.number }} + PR_SHA: ${{ steps.check.outputs.head_sha }} + PR_NUMBER: ${{ steps.check.outputs.pr_number }} run: bash .github/scripts/gate_full_suite.sh - name: Trigger Buildkite merge gate if: steps.check.outputs.has_ready == 'true' env: BUILDKITE_API_TOKEN: ${{ secrets.BUILDKITE_API_TOKEN }} - PR_SHA: ${{ github.event.pull_request.head.sha }} - PR_BRANCH: ${{ github.event.pull_request.head.ref }} - PR_NUMBER: ${{ github.event.pull_request.number }} - PR_TITLE: ${{ github.event.pull_request.title }} + PR_SHA: ${{ steps.check.outputs.head_sha }} + PR_BRANCH: ${{ steps.check.outputs.head_ref }} + PR_NUMBER: ${{ steps.check.outputs.pr_number }} + PR_TITLE: ${{ steps.check.outputs.title }} BK_ORG: ${{ vars.BUILDKITE_ORG_SLUG }} BK_PIPELINE: ${{ vars.BUILDKITE_PIPELINE_SLUG }} MERGE_TEST_PLAN: ${{ steps.plan.outputs.merge_test_plan }} diff --git a/docs/contributing/ci_architecture.md b/docs/contributing/ci_architecture.md index 1a630dd9fa..1202b5e60a 100644 --- a/docs/contributing/ci_architecture.md +++ b/docs/contributing/ci_architecture.md @@ -193,7 +193,8 @@ The Buildkite agent and Slurm have deliberately separate responsibilities: ```text /merge PR comment - -> GitHub verifies write permission and refreshes the `ready` label + -> GitHub verifies write permission, adds the `ready` label, and directly + calls the trusted base-branch merge-gate workflow -> base-branch `ci-trigger-full-suite` workflow fetches the PR file list, computes MERGE_TEST_PLAN plus focused golden/SSIM basenames, and gates on cheap checks diff --git a/docs/contributing/pull_requests.md b/docs/contributing/pull_requests.md index 02f682916d..b581b4edf9 100644 --- a/docs/contributing/pull_requests.md +++ b/docs/contributing/pull_requests.md @@ -67,7 +67,8 @@ filters, and workflow files. 3. Fix pre-commit failures locally with `pre-commit run --all-files`. 4. Wait for at least one approving review. 5. When the PR is approved and ready, comment `/merge`. -6. `/merge` adds `ready`, waits for cheap checks, and triggers the minimal +6. `/merge` adds `ready`, directly calls the trusted merge-gate workflow, + waits for cheap checks, and triggers the minimal path-relevant integration lanes for the PR branch. 7. If all required checks pass, Mergify squash-merges the PR to `main`. 8. If the merge gate fails, fix the regression, push again, and re-run diff --git a/fastvideo/tests/contract/test_ci_test_collection.py b/fastvideo/tests/contract/test_ci_test_collection.py index c7977f7da2..c71ce5ac71 100644 --- a/fastvideo/tests/contract/test_ci_test_collection.py +++ b/fastvideo/tests/contract/test_ci_test_collection.py @@ -145,23 +145,44 @@ def test_all_gpu_ci_routes_use_the_trusted_slurm_dispatcher(): def test_merge_comment_has_one_change_aware_trigger_path(): slash_commands = (REPO_ROOT / ".github/workflows/ci-slash-commands.yml").read_text() - merge_job = slash_commands.split(" parse-command:", maxsplit=1)[0] + merge_jobs = slash_commands.split(" parse-command:", maxsplit=1)[0] ready_workflow = (REPO_ROOT / ".github/workflows/ci-trigger-full-suite.yml").read_text() - assert "labels: ['ready']" in merge_job - assert "api.buildkite.com" not in merge_job - assert "BUILDKITE_API_TOKEN" not in merge_job + assert "labels: ['ready']" in merge_jobs + assert "removeLabel" not in merge_jobs + assert "continue-on-error: true" in merge_jobs + assert "needs: handle-merge" in merge_jobs + assert "if: needs.handle-merge.result == 'success'" in merge_jobs + assert "uses: ./.github/workflows/ci-trigger-full-suite.yml" in merge_jobs + assert "pr_number: ${{ github.event.issue.number }}" in merge_jobs + assert "BUILDKITE_API_TOKEN: ${{ secrets.BUILDKITE_API_TOKEN }}" in merge_jobs + assert "actions: write" not in merge_jobs + assert "api.buildkite.com" not in merge_jobs + assert "workflow_call:" in ready_workflow + assert "BUILDKITE_API_TOKEN:\n required: true" in ready_workflow + assert "CALLED_PR_NUMBER: ${{ inputs.pr_number }}" in ready_workflow + assert "Number.isSafeInteger(prNumber)" in ready_workflow + assert "pull_number: prNumber" in ready_workflow + assert "pr.state !== 'open'" in ready_workflow + assert "pr.base.repo.full_name !== context.payload.repository.full_name" in ready_workflow + assert "pr.base.ref !== context.payload.repository.default_branch" in ready_workflow + assert "core.setOutput('head_sha', pr.head.sha)" in ready_workflow + assert "core.setOutput('base_sha', pr.base.sha)" in ready_workflow assert "api.buildkite.com" in ready_workflow assert 'jq -r --arg pr_number "$PR_NUMBER"' in ready_workflow assert '.env.PR_NUMBER? == $pr_number' in ready_workflow assert 'TEST_SCOPE: "merge"' in ready_workflow assert 'FULL_SUITE: "true"' in ready_workflow assert "plan_merge_ci.py" in ready_workflow - assert "github.event.pull_request.base.sha" in ready_workflow + assert "ref: ${{ steps.check.outputs.base_sha }}" in ready_workflow + assert "PR_SHA: ${{ steps.check.outputs.head_sha }}" in ready_workflow + assert "PR_NUMBER: ${{ steps.check.outputs.pr_number }}" in ready_workflow assert "MERGE_TEST_PLAN" in ready_workflow assert "MERGE_GOLDEN_TESTS" in ready_workflow assert "MERGE_SSIM_TESTS" in ready_workflow assert "__FASTVIDEO_CI_PLAN_ALL__" in ready_workflow + downstream_steps = ready_workflow.split(" - name: Cancel previous Buildkite builds", maxsplit=1)[1] + assert "github.event.pull_request" not in downstream_steps def test_merge_gate_buildkite_cancellation_is_best_effort_and_strict():