Run smoke matrix on push, full matrix on demand#14432
Conversation
📝 WalkthroughWalkthroughThe test-ci workflow is refactored to support operator-selectable test scope via a new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-ci.yml:
- Around line 31-40: The workflow currently relies on default GITHUB_TOKEN
permissions; add an explicit least-privilege permissions block to harden the
workflow and the affected jobs (notably the prepare job that exposes outputs
stable_python and run_nightly and the other test jobs referenced). For example,
add a top-level permissions section (or per-job permissions for prepare and the
test jobs) granting only the minimal scopes needed (e.g., contents: read,
actions: read, checks: write if tests report statuses) and remove/override
default broad permissions; ensure any job that needs extra rights explicitly
requests them rather than inheriting defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddae71c1-8441-4019-861d-b0397df623f5
📒 Files selected for processing (1)
.github/workflows/test-ci.yml
| jobs: | ||
| # Resolve the test scope from the trigger: | ||
| # push -> smoke (cheap per-commit signal on master) | ||
| # workflow_dispatch -> the scope chosen by the operator (defaults to full) | ||
| # Expanding coverage later (new Python versions, etc.) is a one-line edit to the JSON below. | ||
| prepare: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| stable_python: ${{ steps.scope.outputs.stable_python }} | ||
| run_nightly: ${{ steps.scope.outputs.run_nightly }} |
There was a problem hiding this comment.
Add explicit least-privilege permissions for this workflow/jobs.
The new prepare + refactored test jobs run with default token permissions because no permissions block is declared. This weakens CI hardening and is flagged in the changed regions. Please set explicit minimal permissions (workflow-level and/or per-job).
Suggested hardening patch
name: Full Comfy CI Workflow Runs
+permissions:
+ contents: read
+
on:
push:
branches:
- master
- release/**Also applies to: 56-57, 76-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test-ci.yml around lines 31 - 40, The workflow currently
relies on default GITHUB_TOKEN permissions; add an explicit least-privilege
permissions block to harden the workflow and the affected jobs (notably the
prepare job that exposes outputs stable_python and run_nightly and the other
test jobs referenced). For example, add a top-level permissions section (or
per-job permissions for prepare and the test jobs) granting only the minimal
scopes needed (e.g., contents: read, actions: read, checks: write if tests
report statuses) and remove/override default broad permissions; ensure any job
that needs extra rights explicitly requests them rather than inheriting
defaults.
Source: Linters/SAST tools
1 participant
Why
Right now every push to
masterruns the full GPU matrix (3× Python stable + nightly). As we expand coverage (more Python versions, more OSs), running everything on every commit gets too heavy. This separates a cheap per-commit signal from the on-demand full sweep.What changes
A
preparejob resolves the scope from the trigger and the rest of the matrix is driven from its output:master/release/**scope: smokeThe
fullscope is exactly today's matrix — nothing is lost, it just moves behind a manual trigger so it can be run on demand (Actions → Run workflow → scope). This is the "trigger runs manually for a complete scope" mechanism we wanted.Deliberately out of scope
This PR only changes triggering, not the version matrix. Updating Python/CUDA versions (drop 3.9/3.10, add 3.13, bump CUDA) depends on the self-hosted runners having the matching conda envs provisioned, and will be a separate PR that just edits the version arrays in the
preparejob. Keeping the two concerns apart so this one is safe to land independently.Notes / open question
prepareruns onubuntu-latest; the test jobs still target the existing[self-hosted, Linux]runners and the existing comfy-action (CUDA stays the action default, 12.1).