[ci]: make merge-gate startup reliable

.github/workflows/ci-slash-commands.yml

@@ -32,22 +32,42 @@ 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:
           script: |
             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

.github/workflows/ci-trigger-full-suite.yml

@@ -3,20 +3,30 @@ 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
   pull-requests: read
   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,52 +35,142 @@ 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
+        # 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
+        timeout-minutes: 3
         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 }}
+          PR_BRANCH: ${{ steps.check.outputs.head_ref }}
+          PR_NUMBER: ${{ steps.check.outputs.pr_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" \
+          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 --connect-timeout 5 --max-time 20 --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" \
-            | jq -r --arg pr_number "$PR_NUMBER" \
-              '.[] | select((.env.TEST_SCOPE? == "merge") and (.env.PR_NUMBER? == $pr_number)) | .number')
-          for build_num in $builds; do
+            --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" \
+            "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 ! 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
+
+          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 --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."
+              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.
+      # 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
@@ -105,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 }}

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

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

fastvideo/tests/contract/test_ci_test_collection.py

@@ -145,23 +145,88 @@ 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():
+    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 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 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
+
+    # 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 --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 ))")
 
 
 def test_full_ssim_has_a_weekly_slurm_schedule():