From 72f67d98774eb7dcdf9639811fd99bf62bc82551 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 3 Sep 2026 01:22:42 -0700 Subject: [PATCH 1/3] ci(gpu): gate the PR leg of the GPU workflow behind a maintainer-applied gpu-ci label The self-hosted `gpu-runners` job checks out and runs PR code (pip install -e . plus TEST_E2E=true pytest tests/e2e). Same-repo PRs get no workflow-approval gate -- GitHub's approval setting covers forks only -- so any same-repo PR touching comfy_cli/** executed its own code on GPU hardware before review. Gate the job on a maintainer-applied `gpu-ci` label, and add `labeled` to the pull_request trigger types so applying the label starts a run immediately instead of waiting for the next push. Push-to-main runs are unchanged. --- .github/workflows/run-on-gpu.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/run-on-gpu.yml b/.github/workflows/run-on-gpu.yml index ceb741ec8..646c3e2f1 100644 --- a/.github/workflows/run-on-gpu.yml +++ b/.github/workflows/run-on-gpu.yml @@ -12,6 +12,9 @@ on: - "!.coveragerc" - "!.gitignore" pull_request: + # `labeled` is required on top of the default trigger set: without it a + # freshly applied `gpu-ci` label would do nothing until the next push. + types: [opened, synchronize, reopened, labeled] branches: - main paths: @@ -27,6 +30,12 @@ permissions: jobs: test-cli-gpu: + # PR runs are opt-in via the maintainer-applied `gpu-ci` label, because this + # job checks out and executes PR code on self-hosted GPU runners. Push-to-main + # runs stay automatic. The label persists across later pushes to the same PR, + # so apply it only after reading the diff and remove it to re-gate the PR. + # Prerequisite for the review-gated auto-builder in #786. + if: github.event_name == 'push' || contains(github.event.pull_request.labels.*.name, 'gpu-ci') name: "Run Tests on GPU Runners" runs-on: group: gpu-runners From 20d9f12a3abc0fa55e1c1b200882d8ccf19fe44c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 3 Sep 2026 03:22:28 -0700 Subject: [PATCH 2/3] ci(gpu): bind the gpu-ci gate to the labeling event, not the label set Addresses the cursor-review panel on #843. The gate as written authorized the pull request rather than the reviewed commit: `synchronize` stayed a trigger and the label persisted, so once a maintainer labeled a benign diff the author could push arbitrary code that ran immediately on the self-hosted GPU runners (`pip install -e .` executes the PR's build backend, `pytest` loads its conftest). Raised by 6 of 6 reviewers. - Trigger on `types: [labeled, unlabeled]` only. A PR run is now one-to-one with a maintainer applying `gpu-ci`; a later push cannot inherit that approval because `synchronize` no longer triggers this workflow. Re-label to re-run. - Gate on the labeling event (`github.event.action == 'labeled' && github.event.label.name == 'gpu-ci'`) rather than membership in the label set. `labeled` fires for every label, so the membership test re-queued a full GPU run whenever any unrelated label was added to an already-labeled PR. - Pin checkout to `github.event.pull_request.head.sha`. The `gpu-runners` pool is small, so a labeled run can sit queued long enough for the author to push before the merge ref is resolved; this runs exactly the commit that was labeled. - Add a `concurrency` group keyed on PR + label name. Removing `gpu-ci` now cancels an in-flight run (the reviewers' "removing the label cannot stop an already-queued run"), while an unrelated label cannot cancel one. - Drop the `paths` filter from the PR leg. With runs opt-in, filtering only produced the surprise of a labeled PR that never runs. The push-to-main leg keeps its filter. Mirrors the pattern already used by ci-cursor-review.yml in this repo. Co-Authored-By: Claude Opus 5 --- .github/workflows/run-on-gpu.yml | 49 +++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/.github/workflows/run-on-gpu.yml b/.github/workflows/run-on-gpu.yml index 646c3e2f1..e75ae5642 100644 --- a/.github/workflows/run-on-gpu.yml +++ b/.github/workflows/run-on-gpu.yml @@ -12,30 +12,44 @@ on: - "!.coveragerc" - "!.gitignore" pull_request: - # `labeled` is required on top of the default trigger set: without it a - # freshly applied `gpu-ci` label would do nothing until the next push. - types: [opened, synchronize, reopened, labeled] + # Label events *only*, so a PR run exists one-to-one with a maintainer + # applying `gpu-ci`: a later push cannot inherit that approval, because + # `synchronize` no longer triggers this workflow at all. Re-label (remove, + # re-add) to re-run against new commits. `unlabeled` never runs the job -- + # it is here so removing the label lands in the same concurrency group and + # cancels an in-flight run. Same shape as ci-cursor-review.yml. + types: [labeled, unlabeled] branches: - main - paths: - - "comfy_cli/**" - - "!comfy_cli/test_**" - - "!.github/**" - - "!tests/**" - - "!.coveragerc" - - "!.gitignore" + # Deliberately no `paths` filter on this leg. Applying the label *is* the + # intent signal, so filtering here would only produce the surprise of a + # labeled PR that never runs -- e.g. one touching just tests/ or .github/. + # The push-to-main leg above keeps its filter, since those runs are + # automatic and do need narrowing. permissions: contents: read +concurrency: + # Keyed on the label name as well as the PR, so an unrelated label applied to + # an already-labeled PR cannot cancel an in-flight GPU run; removing `gpu-ci` + # shares the group and *does* cancel it, which is how a maintainer stops a run + # they no longer want. Push-to-main runs key on the SHA, so they queue. + group: run-on-gpu-${{ github.event.pull_request.number || github.sha }}-${{ github.event.label.name }} + cancel-in-progress: true + jobs: test-cli-gpu: # PR runs are opt-in via the maintainer-applied `gpu-ci` label, because this # job checks out and executes PR code on self-hosted GPU runners. Push-to-main - # runs stay automatic. The label persists across later pushes to the same PR, - # so apply it only after reading the diff and remove it to re-gate the PR. + # runs stay automatic. Gate on the labeling *event* rather than on the label + # being present in the set: `labeled` fires for every label, so a membership + # test would re-queue a full GPU run whenever any unrelated label (`bug`, an + # automation-applied area label) was added to an already-labeled PR. # Prerequisite for the review-gated auto-builder in #786. - if: github.event_name == 'push' || contains(github.event.pull_request.labels.*.name, 'gpu-ci') + if: >- + github.event_name == 'push' + || (github.event.action == 'labeled' && github.event.label.name == 'gpu-ci') name: "Run Tests on GPU Runners" runs-on: group: gpu-runners @@ -48,6 +62,15 @@ jobs: steps: - name: Check out code uses: actions/checkout@v7 + with: + # Pin to the commit that carried the label, not the merge ref resolved + # whenever a runner frees up. The `gpu-runners` pool is small, so a + # labeled run can sit queued for minutes -- long enough for the author + # to push after a maintainer read the diff. This makes the code that + # executes on the self-hosted runner exactly the code that was + # approved. It tests the PR head rather than the merged result; the + # push-to-main leg covers the merged state. + ref: ${{ github.event.pull_request.head.sha || github.sha }} - name: Check Nvidia run: | From 53854f3ab87a6fb607300b667b25b9a72cbad33b Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 3 Sep 2026 03:50:47 -0700 Subject: [PATCH 3/3] ci(gpu): bound the GPU job, drop persisted creds, correct the gate's comments Second review round on #843. Three substantive changes plus comment corrections where the previous round's comments overclaimed. - `timeout-minutes: 30` on `test-cli-gpu`. The job runs PR-supplied code on the shared self-hosted pool under the 6-hour default ceiling, and with the new concurrency group a stuck run also blocks that PR's next one. The last 20 successful runs all finished within ~11.5 min, so 30 is ~2.5x headroom. - `persist-credentials: false` on checkout. The default leaves the job's GITHUB_TOKEN in .git/config as an http.extraheader before untrusted PR code runs, readable for the job's lifetime on a persistent runner. Nothing here pushes and the build backend is plain setuptools with a static version, so it needs no git metadata. - Document the conflicted-PR trap: GitHub creates no `pull_request` runs while a PR has merge conflicts, so labeling a conflicted PR is silently lost, and resolving it emits only `synchronize`, which is no longer a trigger. Recovery is remove/re-add, removal first. Comment corrections, no behaviour change: - "Push-to-main runs key on the SHA, so they queue" was backwards. Each push has a distinct SHA and sits alone in its group, so those runs are unaffected -- neither cancelled nor serialized. - The checkout pin closes the queue window but is not proof the maintainer read that commit; labels are PR-level, so a push between reading the diff and clicking the label is still captured. Softened, with a note to check the head SHA when labeling. - "The push-to-main leg covers the merged state" does not hold for the PRs the `paths` removal newly admits, which that leg still filters out. Recorded the gap, along with the stale-branch case where the merge ref's YAML references a file the pinned head tree lacks. - The unlabel-cancels-in-flight behaviour depends on the merge ref's copy of this file, so it is a convenience rather than a stop button. Co-Authored-By: Claude Opus 5 --- .github/workflows/run-on-gpu.yml | 55 ++++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 10 deletions(-) diff --git a/.github/workflows/run-on-gpu.yml b/.github/workflows/run-on-gpu.yml index e75ae5642..cde63b547 100644 --- a/.github/workflows/run-on-gpu.yml +++ b/.github/workflows/run-on-gpu.yml @@ -17,7 +17,14 @@ on: # `synchronize` no longer triggers this workflow at all. Re-label (remove, # re-add) to re-run against new commits. `unlabeled` never runs the job -- # it is here so removing the label lands in the same concurrency group and - # cancels an in-flight run. Same shape as ci-cursor-review.yml. + # usually cancels an in-flight run (see the caveat on `concurrency` below). + # Same shape as ci-cursor-review.yml. + # + # Trap worth knowing: GitHub creates no `pull_request` runs at all while a + # PR has merge conflicts, so applying the label to a conflicted PR is + # silently lost -- and resolving the conflict emits only `synchronize`, + # which is not a trigger here. Recovery is the same remove/re-add, and it + # does need the removal first, since the label is already applied. types: [labeled, unlabeled] branches: - main @@ -33,8 +40,15 @@ permissions: concurrency: # Keyed on the label name as well as the PR, so an unrelated label applied to # an already-labeled PR cannot cancel an in-flight GPU run; removing `gpu-ci` - # shares the group and *does* cancel it, which is how a maintainer stops a run - # they no longer want. Push-to-main runs key on the SHA, so they queue. + # shares the group and so usually cancels it, which is how a maintainer stops + # a run they no longer want. Treat that as a convenience rather than a stop + # button: the cancelling run is created from the *merge ref's* copy of this + # file, so a push landed after labeling can edit this key or drop the + # `unlabeled` trigger and the removal then cancels nothing. + # + # Push-to-main runs fall back to `github.sha`, which is distinct per push, so + # each sits alone in its own group: they are simply unaffected here, neither + # cancelled nor serialized. group: run-on-gpu-${{ github.event.pull_request.number || github.sha }}-${{ github.event.label.name }} cancel-in-progress: true @@ -51,6 +65,13 @@ jobs: github.event_name == 'push' || (github.event.action == 'labeled' && github.event.label.name == 'gpu-ci') name: "Run Tests on GPU Runners" + # Bound a hung or hostile run on the shared self-hosted pool, which the + # 6-hour default does not: this job executes PR-supplied code (`pip install + # -e .` runs the PR's build backend, pytest loads its conftest), and with + # the concurrency group above a stuck run also blocks that PR's next one. + # The last 20 successful runs all finished within ~11.5 min end to end, so + # 30 leaves roughly 2.5x headroom. + timeout-minutes: 30 runs-on: group: gpu-runners labels: ${{ matrix.os }}-x64-gpu # @@ -63,14 +84,28 @@ jobs: - name: Check out code uses: actions/checkout@v7 with: - # Pin to the commit that carried the label, not the merge ref resolved - # whenever a runner frees up. The `gpu-runners` pool is small, so a - # labeled run can sit queued for minutes -- long enough for the author - # to push after a maintainer read the diff. This makes the code that - # executes on the self-hosted runner exactly the code that was - # approved. It tests the PR head rather than the merged result; the - # push-to-main leg covers the merged state. + # Pin to the head SHA carried by the `labeled` webhook rather than the + # merge ref, which would otherwise be resolved whenever a runner frees + # up. The `gpu-runners` pool is small, so a labeled run can sit queued + # for minutes -- long enough for the author to push first. This closes + # that queue window, but it is not proof the maintainer read this + # commit: labels are PR-level, so a push between reading the diff and + # clicking the label is still what gets captured. Check the head SHA + # when you label. + # + # Two consequences of testing the head rather than the merge result. + # A branch that predates a file main added can fail on a step the + # merged YAML still references (the workflow definition comes from the + # merge ref even though the tree no longer does) -- rebase to clear + # it. And for a PR outside the push leg's `paths` filter (tests/, + # .github/, pyproject.toml) the merged tree is never GPU-tested at + # all, since that leg will not run for it post-merge either. ref: ${{ github.event.pull_request.head.sha || github.sha }} + # Keep the job's GITHUB_TOKEN out of .git/config, where it would sit + # as an http.extraheader that the PR code running next could read and + # reuse for the job's lifetime on a persistent self-hosted runner. + # Nothing here pushes, and the build backend needs no git metadata. + persist-credentials: false - name: Check Nvidia run: |