ci: install test dependencies from uv.lock instead of resolving fresh#537
ci: install test dependencies from uv.lock instead of resolving fresh#537mattmillerai wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBoth GitHub Actions test workflows now use ChangesCI uv migration
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 2 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v8 |
There was a problem hiding this comment.
🟡 Medium — astral-sh/setup-uv@v8 is pinned to a mutable major-version tag rather than a full commit SHA. This job exposes secrets.CODECOV_TOKEN, so a retargeted or compromised upstream tag could run attacker-controlled code and exfiltrate that token; note codecov/codecov-action is already SHA-pinned in this same file, making this an inconsistent supply-chain gap. Pin the action to a full commit SHA. Raised by 2 of 8 reviewers (gpt-5.3-codex-xhigh adversarial, claude-opus-4-8-thinking-xhigh adversarial).
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v8 |
There was a problem hiding this comment.
🟡 Medium — astral-sh/setup-uv@v8 is referenced by a mutable tag instead of a pinned commit SHA, so a retagged or compromised upstream release would execute arbitrary code in CI with the job token and repository access. Pin the action to a full commit SHA (as codecov/codecov-action already is in pytest.yml). Raised by 2 of 8 reviewers (gpt-5.3-codex-xhigh adversarial, claude-opus-4-8-thinking-xhigh adversarial).
pytest.yml and build-and-test.yml installed test dependencies unpinned (`pip install pytest ...` + `pip install -e .`), so CI re-resolved transitive dependencies on every run and ignored the versions pinned in uv.lock. An upstream release could therefore break every open PR at once, with failures unrelated to the PR's own diff -- as tomlkit 0.15.1 did. Both workflows now install via `uv sync --extra dev --locked` and run tests through `uv run --locked`, so CI tracks the lockfile and matches local dev. Dependency upgrades become deliberate lockfile bumps, where a break is attributable and reviewable. Use `--locked` rather than `--frozen`: `--frozen` uses the lockfile as-is but exits 0 when it has drifted from pyproject.toml, so it would not fail loudly on drift. `--locked` errors instead. Also relock uv.lock, which had already drifted -- the `bench` extra (anthropic>=0.40) was added to pyproject.toml without a lock update, and `--locked` fails on that drift. The relock is purely additive: it adds anthropic and its dependencies and changes no existing pinned version.
c2141ed to
2522ad2
Compare
`astral-sh/setup-uv@v8` does not resolve -- setup-uv publishes floating major tags only through v7, so the job failed at "Prepare all required actions" with "unable to find version `v8`". Pin the action by commit SHA with the version in a trailing comment, matching how this repo already pins its other third-party action (codecov-action). This also suits a workflow whose purpose is reproducible CI: a mutable tag is the same class of problem as an unpinned dependency.
ELI-5
Our test CI used to download the newest version of every library each time it ran, ignoring the exact versions written down in
uv.lockthat we use on our own machines. So the day a library published a new release, CI broke on every open pull request at once — even PRs that had nothing to do with it. This makes CI read the list of exact versions instead, so it breaks only when we choose to upgrade.What happened
On 2026-07-17,
tomlkit0.15.1 shipped and 9 tests started failing withValueError: Comment cannot contain line breakson PRs that touched none of the relevant code.uv.lockpinstomlkit==0.13.3, butpip install pytest ...+pip install -e .re-resolves transitive dependencies from scratch, so CI never saw that pin.What changed
pytest.ymlandbuild-and-test.ymlnow install withuv sync --extra dev --lockedand run tests viauv run --locked --extra dev. Everything else — the tests run, coverage flags, Codecov upload, the 3-OS matrix,PYTHONPATH, env gates — is unchanged.The repo already had the inputs:
uv.lockandoptional-dependencies.dev.ruff_check.ymlalready pinsruff==0.15.15, so this brings the test workflows in line with the instinct the lint workflow already had.Verification
On real CI, this PR's
buildjob is green — and the log shows it doing the thing the ticket asked for:All checks green:
build,ruff_check,codecov/project(so the coverage upload still works), CodeQL, Socket Security.Locally, that CI now genuinely tracks the lockfile (the acceptance criterion) — against a
uv sync --lockedvenv on Python 3.10.20:uv lock --upgrade-package tomlkit→ 0.15.1 → exactly the 9 failures named in the report (8 intest_config_parser.py, plustest_node_init_strips_credentials).Judgment calls
1.
--locked, not--frozen. The plan specified--frozento "fail loudly ifuv.lockdrifts frompyproject.toml." It does not —--frozenuses the lockfile as-is but exits 0 on drift. Verified against the real drift below:--frozen--lockedBoth stop the fresh re-resolution that caused the outage, but only
--lockeddelivers the second stated goal. Used--lockedto match the plan's intent over its literal text.2.
uv.lockis relocked here (285 additive lines).--lockedimmediately failed onmain: thebenchextra (anthropic>=0.40) was added topyproject.tomlin #490 without a lock update. Without fixing that first,--lockedwould fail on day one. This is itself evidence for the ticket — the drift went unnoticed because nothing checked. The relock is purely additive: it addsanthropic+ 6 transitive deps and changes no existing pinned version (verified by diffing everyname/versionpair;tomlkitstays 0.13.3).3.
setup-uvpinned by commit SHA. The plan said@v5; that resolves but is well behind (current is v8.3.2).@v8does not resolve — setup-uv publishes floating major tags only throughv7, which failed CI outright. Rather than fall back to a mutable tag, it's pinned to the v8.3.2 SHA with the version in a trailing comment — matching how this repo already pins its other third-party action (codecov-action), and fitting for a workflow whose whole purpose is reproducibility: a mutable tag is the same class of problem as an unpinned dependency.4.
--extra devrepeated onuv run.uv runre-syncs the environment, and today's uv leaves the dev extra in place (verified) — but that's a default, not a guarantee. Naming the extra on both commands states the requirement explicitly, so a future uv default can't silently strippytest/pytest-cov/jsonschemaout from under the test step.5.
actions/setup-pythondropped in favor of setup-uv'spython-version. Keeps one tool owning the interpreter while preserving the pinned 3.10 (and the matrix input inbuild-and-test.yml), along with the "min version in pyproject.toml" comment.Known verification gap — worth a reviewer's eye
build-and-test.ymldoes not run on this PR. Itspaths:filter matches onlycomfy_cli/**andtests/e2e/**and explicitly excludes.github/**— so a PR that only touches workflows anduv.lockcan't exercise it. My changes to it are therefore verified locally but not on real CI, and it will first run for real on the next PR that touchescomfy_cli/**(across Linux/Windows/macOS).What I could check locally (macOS):
uv sync --extra dev --lockedsucceeds; thecomfyentrypoint resolves underuv run(the e2e tests shell out to it); the CUDA-detect step skips cleanly with no GPU (5 skipped); the e2e (19) and torch-backend (5) suites collect. Their full runs need CI's network/GPU. If you'd rather not take the first-run risk, the alternative is to drop thebuild-and-test.ymlhunk and do it in a PR that also touchescomfy_cli/**— happy to split it.Scope / follow-ups (not done here)
test-mac.yml,test-windows.yml,run-on-gpu.yml.run-on-gpu.ymlruns on self-hosted GPU runners, where introducing uv deserves its own verified change rather than a blind sweep.uv.lockpinsruff==0.12.7whileruff_check.ymlpinsruff==0.15.15— a real divergence (the older ruff flags 14UP038errors the pinned one doesn't). Neither workflow here runs ruff, so this PR leaves it alone rather than bundling a lint-behavior change.Trade-off worth knowing
--lockedmeans apyproject.tomlchange without a matchinguv locknow fails CI. That's the intended trade: the failure is loud, attributable to the PR that caused it, and fixed by runninguv lock. The alternative — a stale lock silently missing a newly added dependency — surfaces as a confusingImportErrorinstead.