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 e96ad9c813..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,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 }} 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 4a0e0b1798..c71ce5ac71 100644 --- a/fastvideo/tests/contract/test_ci_test_collection.py +++ b/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():