ci: install test deps from uv.lock so upstream releases can't break every PR (BE-3292)#569
ci: install test deps from uv.lock so upstream releases can't break every PR (BE-3292)#569mattmillerai wants to merge 2 commits into
Conversation
…very PR (BE-3292) The pytest and build-and-test workflows installed test dependencies unpinned (pip install pytest ... + pip install -e .), resolving transitive deps fresh on every run and ignoring the versions pinned in uv.lock. When an upstream package publishes a new release, CI broke on every open PR at once with failures unrelated to the PR's own diff (e.g. tomlkit 0.15.1 on 2026-07-17 broke 9 config_parser/node_init tests). Install from the lockfile via uv sync/uv run --frozen so CI matches local dev; --frozen fails loudly if uv.lock drifts from pyproject.toml rather than silently resolving something new. Upgrades become deliberate lockfile bumps in their own reviewable PRs.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ 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)
Pin astral-sh/setup-uv to the immutable commit d4b2f3b (v5.4.2, the current v5 tag) instead of the mutable v5 tag, so a retagged or compromised upstream release can't run arbitrary code in CI. Matches the SHA-pinning convention already used for codecov/codecov-action. Addresses CodeQL alerts 44/45 and adversarial cursor-review findings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ELI-5
Our CI didn't use the versions of test tools we'd locked in
uv.lock. Everytime it ran, it fetched the newest versions of everything from the internet.
So when some unrelated package shipped a new release, CI could suddenly go red
on every open PR at once — for reasons that had nothing to do with the PR.
This makes CI install exactly the locked versions instead, so a surprise
upstream release can't ambush everyone.
What & why
.github/workflows/pytest.yml(andbuild-and-test.yml) installed test depsunpinned:
pip install -e .re-resolves transitive deps fresh on every run, ignoring theversions pinned in
uv.lock. This isn't hypothetical — on 2026-07-17,tomlkit0.15.1 shipped and 9 tests began failing(
tests/comfy_cli/registry/test_config_parser.py×8 +test_node_init.py::test_node_init_strips_credentials) withValueError: Comment cannot contain line breaks, red-ing PRs whose diffstouched none of that. The tomlkit source fix is handled separately (#533 /
BE-3285); this PR closes the CI-reproducibility gap that let it happen.
Change
Both workflows now install from the lockfile via
uv:uv sync --extra dev --frozen— install the exact pinned versions.uv run --frozen --extra dev pytest ...— run against them.--frozenmakes CI fail loudly ifuv.lockdrifts frompyproject.toml, instead of silently resolving something new.--extra devsuppliespytest/pytest-cov/jsonschema(the repo'sexisting
optional-dependencies.devgroup), replacing the ad-hocpip install.--python 3.10forpytest.yml's min-version job;
--python ${{ matrix.python-version }}forbuild-and-test.yml's matrix), so
uvprovides the interpreter andactions/setup-pythonis no longer needed.Upgrades now become deliberate: bump
uv.lockin its own PR where a break isattributable and reviewable, instead of ambushing whoever opens the next PR.
Verification (run locally on this branch)
uv sync --extra dev --frozen --python 3.10succeeds — lockfile is in syncwith
pyproject.toml; installstomlkit==0.13.3.uv run --frozen --extra dev --python 3.10 pytest --cov=comfy_cli --cov-report=xml .→ 2573 passed, 37 skipped (coverage.xml written, sopytest-cov/jsonschemaare present under--extra dev).overridden —
uv run ... --with tomlkit==0.15.1 pytest <the 9 tests>→9 failed (exactly the ones from the incident); the default lockfile run
(0.13.3) passes them. Confirms CI now tracks
uv.lock.build-and-test.ymlCUDA step runs underuv runand skips gracefully with no GPU.
Judgment calls / scope
Scope = the two workflows the ticket named (
pytest.yml,build-and-test.yml), both of which run the pytest suites via the unpinnedpattern.
Deliberately left as follow-ups (noted, not changed here):
run-on-gpu.yml— same pattern, but runs on a self-hosted GPU runnerI can't verify locally; converting it unverified would be risky.
test-windows.yml/test-mac.yml— these are CLI integration smoketests (
comfy install+comfy launchin an explicit venv), notpytest-suite runs, so the tomlkit-class break doesn't hit them and the
conversion is structurally different. (
test-windowsis also a known,separate infra flake.)
ruff_check.ymlalready pins its tool (ruff==0.15.15);publish_package.ymlis release-time, not per-PR.
A separate PR could extend the lockfile approach to these if desired.