test(e2e): pin the ComfyUI runtime-preservation contract (EAI-8071)

[fredespi]

Pins the EAI-8051 walkthrough defect with an end-to-end scenario, completing the pinning set alongside EAI-8024 (#241) and EAI-8050. Test-only — git diff origin/main touches five files, all under tests/e2e-cucumber/, and no product code.

What changed since the last review

The driver half is gone. Both reviews were right that @requires-root is satisfied by no lane, so driver-install-as-root-does-not-require-sudo had never run and could not. Both offered the same two ways out: add a root lane, or cover the uid-aware prefix with a unit test on build_driver_install_plan.

Since then #305 (f2c34779) landed on main and did exactly the latter — it fixed EAI-8053 and shipped three unit tests over the plan, parameterised across every planning distro:

• driver_plan_as_root_never_emits_sudo
• driver_plan_off_root_still_escalates_every_privileged_command
• driver_plan_as_root_drops_the_sudo_binary_precondition

The scenario is therefore redundant as well as unrunnable, so this PR removes it rather than duplicating coverage main already has: driver.feature, driver_steps.rs, the @requires-root tag, the is_root capability probe, the mock-root fixture and the expectations row all go.

EAI-8051 — ComfyUI must not destroy the ROCm runtime

comfyui.feature is the suite's first ComfyUI coverage. It installs a managed runtime, installs ComfyUI, and asserts the machine's ROCm runtime survives.

It is an expected pass, not an xfail: #298 (3e59435a) fixed the bug by pinning the runtime's torch stack with a uv --constraint file. The xfail row was deleted in 1fe56c3e and replaced by a comment recording why, so the next reader does not re-add it.

Three assertions, two of them added in response to review:

• The install exits 0. comfyui::install bails early on several paths (no managed runtime, a runtime that is not ready, a non-wheel format, a failed source download or uv acquisition). On any of those the runtime is trivially unchanged and the invariants below would report green having installed nothing.
• The runtime's torch version is byte-for-byte what it was before. A +cu label check alone is not sufficient: the measured GPU run replaced torch==2.11.0+gitd0c8b1f with a plain torch==2.13.0, which carries no +cu label at all.
• No nvidia-* distributions appear in the runtime.

Package enumeration goes through importlib.metadata rather than pip list, because uv-created runtimes have no pip module — python -m pip there exits non-zero with empty stdout, which a naive reader takes for "no packages installed". The probe fails loudly instead of letting a broken probe read as clean. Neither probe imports torch, which would require the runtime's own LD_LIBRARY_PATH/ROCM_PATH.

The scenario runs against its own isolated runtime prefix, never the shared tree, because it may corrupt the runtime it checks.

Lane

The scenario is @requires-gpu @nightly, so per AGENTS.md §3 the gating lane is nightly.yml's e2e-gpu-nightly job, which sets E2E_INCLUDE_NIGHTLY=1 on a GPU host. e2e-selfhosted.yml also runs it on manual dispatch when include_nightly is set. It first runs on the next nightly schedule after merge. Regular PR CI skips it, so this scenario does not gate merges.

It has already been exercised end to end on MI300X. Run #427, against a pre-#298 binary, reproduced the defect exactly: comfyui install replaced torch==2.11.0+gitd0c8b1f with stock CUDA torch==2.13.0 plus 15 nvidia-* wheels, after which the CLI reported Error: AMD GPU check failed. Run #519, post-#298, passes with 0 xfail and 0 XPASS.

Remaining review items

• expectations.toml — restored the blank line before the EAI-8031 block.
• find_venv_python — the comment was wrong, not the code, so only the comment changed. root is the initial frontier entry, and the product creates the venv at install_root (uv_venv_args(python, install_root), and venv_python_path is runtime_python_executable_in_env(install_root)), so <install_root>/bin/python matches on the first iteration with no directory traversal at all. The depth-capped walk is a fallback for a tree that does not match, not a cost the normal path pays.
• The @requires-root README tag-table entry, the missing @requires-bare-metal gate, and run_rocm_with_only_tools's bare-rocm PATH fallback all dissolved with the driver half.
• comfyui.feature is registered in FEATURE_KEYS and its scenario renumbered comfyui-01, per the per-feature naming guard main added while this branch was in review.
• The branch was CONFLICTING; it is now merged up to main.

Test plan

Verified in a Linux container (native-Linux remote, matching CI):

• cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings — clean.
• cargo clippy -p e2e-cucumber --test e2e -- -D warnings — clean.
• cargo test -p e2e-cucumber --lib — 113 passed.
• cargo test -p e2e-cucumber --test feature_naming — 4 passed.
• No-GPU mock lane: 78 scenarios, 75 passed, 2 xfail, 0 XPASS (0 flaky, 0 stale). comfyui-install-preserves-the-rocm-runtime skips there as @nightly @requires-gpu, as designed. The one unexpected failure is engine-shell-marks-the-prompt, a bash-prompt/PTY gap in the container environment that reproduces on unmodified main and is green in real CI.
• hawkeye check --config licenserc.toml (v7.0.0, the version CI pins) — 231 files, 0 changes.

The previously red Lint (PowerShell) and Test (affected crates) were both Failed to download action … after 3 attempts — action-tarball fetch failures unrelated to this branch. They need a re-run on the new head.

[tomastola]

Non-blocking verification note: the targeted MI300X dispatch reproduced EAI-8051, but the --name filter bypassed the custom expectation filter, so it was reported as an unexpected failure rather than an EAI-8051 xfail. Regular PR CI skips this @nightly scenario. It would be useful to capture one run that exercises both the failure and its xfail reconciliation.

[michaelroy-amd]

Current head 1fe56c3eb1fead0ebf44b1cb0d290f9a023b9d6a is mergeable, but two required gates are failing: Test (affected crates) and Lint (PowerShell). Please fix or demonstrate those failures are unrelated on this exact head, then rerun the required suite and re-request review. I did not approve while required checks are red.

[rominf]

I looked at this at 1fe56c3e. The ComfyUI half is real work — the #298 graduation is correctly timed, the --constraint fix did land in 3e59435a, and the nightly gating reasoning in the feature comment holds up (nightly.yml sets E2E_INCLUDE_NIGHTLY=1 on e2e-gpu-nightly, so that scenario genuinely runs). I also checked the things that looked risky and found them fine: process_is_root's Uid: effective-field parsing, run_rocm_with_only_tools not perturbing plan selection (build_driver_install_plan reads /etc/os-release directly, not via PATH), stderr propagation from sh -c through to the captured output, and both sh-dialect spellings of "not found" being covered.

The blocking problem is the driver half: @requires-root is satisfied by zero lanes, so that scenario has never run and can't. I pulled this PR's own report artifacts to confirm rather than reasoning about it. All five lanes report the same thing:

mock:                {"id":"driver-install-as-root-does-not-require-sudo","expected":"skip",
                      "reason":"requires the runner to be root; this process is not root"}
mi300x:              same          (capability.is_root = false)
strix-halo-windows:  same          (capability.is_root = false)
strix-halo-wsl:      same          (capability.is_root = false)
rad3 r9700:          same          (capability.is_root = false)

There is no container: key anywhere in .github/workflows/, and every runs-on is either a GitHub-hosted image or a self-hosted machine with a non-root service account. So there is no lane that could satisfy it. AGENTS.md §3 asks for the gated lane to be named in the PR text when a scenario can only run on one — here the honest answer is that none exists.

Both new scenarios also rest on pure negative assertions with no premise check, which means they can report green having exercised nothing. Details inline.

The PR description doesn't match the branch or the runs. Three things:

• The lane table says EAI-8053 … Lane: mock (root). The mock lane is not root — see the artifact above; it skips, it doesn't xfail.
• The Test plan says "51 scenarios, 3 xfail, 0 XPASS". The actual run at this head (32902998719, job "E2E tests") reports 72 scenarios (70 passed, 2 failed) / Reconciliation: 2 xfail … 0 XPASS. Those 51/3 numbers look like a local root-container run, which is exactly the environment CI doesn't provide — worth saying so explicitly if that's what happened.
• EAI-8051 is still listed under "Pinned as expected failures", but your own final commit 1fe56c3e deleted that row.

CI. Lint (PowerShell) and Test (affected crates) are both red, and both are pure infra — Failed to download action … after 3 attempts on the action tarball fetch, nothing to do with this branch. Worth a re-run so the PR isn't sitting red. E2E tests (Strix Halo, Ubuntu) was cancelled. Also note the branch is CONFLICTING against main now.

One thing I'd suggest regardless of how the root question resolves: @requires-root isn't in the tag table at tests/e2e-cucumber/README.md:111-121, which documents every other capability tag. That table is where the next author looks before writing a gated scenario, and an undocumented gate is how you get a second scenario that silently never runs.

[siloteemu]

🔴 Automated review · pr-review-watcher · 1fe56c3

Summary

Test-only PR adding two e2e cucumber scenarios (a ComfyUI-install runtime-preservation contract and a root-without-sudo driver-install contract), a new @requires-root tag with an is_root capability probe, and an expectations-matrix row/removal pair. Verdict: Needs work — the ComfyUI scenario is genuinely falsifiable and well built, but the driver scenario is pinned on a lane that does not exist and its single assertion can pass against a completely unfixed binary. Verified: comfyui-install-preserves-the-rocm-runtime would fail against a pre-fix binary (baseline premises are asserted, both probes panic rather than defaulting benign) but runs only on @nightly+@requires-gpu lanes, i.e. nightly.yml (schedule/dispatch only) — zero merge-gating coverage; driver-install-as-root-does-not-require-sudo resolves to Skip on the only blocking lane (ci.yml:678 e2e, ubuntu-latest, no container:, and its own Install native build deps step at ci.yml:713 uses sudo — direct in-repo evidence the lane is not uid 0), the Strix self-hosted lanes point HOME at a non-root path, and the remaining self-hosted lanes' uid is not determinable from this repo. A previously-reported "skips on every lane" finding still holds for every lane I could resolve and is unresolvable for MI300X/R9700/WSL; a previously-reported "bare negative with no premise check" finding still holds and I traced the exact path (apps/rocm/src/main.rs:2547 swallows a missing /etc/os-release via unwrap_or_default(), an unmatched distro yields supported: false, and main.rs:2550 returns Ok with exit 0 having executed nothing — all three negative substrings absent, scenario passes). On the 4 red checks I could not fetch check names (review-only, no GitHub contact) — cargo fmt --all --check is clean and I found no compile-level defect in the diff, so the most parsimonious reading is the self-hosted e2e lanes, which this workflow's own header (e2e-selfhosted.yml:11-15) says stay in main's required-check list and report red/missing when a runner is offline; the diff-caused alternative is a stale-XPASS on a root self-hosted lane via exactly the vacuous-pass path above, and those two cannot be told apart without the check names. Branch is 34 commits behind origin/main. Blocking: 2 · Non-blocking: 6.

🚫 Blocking (must fix before merge)

• tests/e2e-cucumber/tests/e2e/driver_steps.rs:37-53 — the assertion is a bare negative with no premise check, so it passes against an entirely unfixed binary. Traced: with an unsupported or undetectable distro, read_os_release() failure is discarded (apps/rocm/src/main.rs:2547), build_driver_install_plan falls through to supported: false, mutating: false, and install_driver returns at apps/rocm/src/main.rs:2550 with exit 0 having run no command. None of "sudo: not found", "sudo: command not found", "driver command failed: sudo " can appear, so the step passes — and because the row is when = {}, that vacuous pass registers as a deterministic XPASS and fails the lane with a "stale xfail, remove it" message about a bug that was never exercised. The Given at driver_steps.rs:18-23 is an empty no-op, so the premise it names ("a root machine with no sudo command available") is asserted nowhere. Fix: assert the premise before the negative — that the process is uid 0, that sudo is genuinely unreachable on the sanitised PATH, and that the run actually reached command execution (the rendered plan is supported/mutating, or the output contains the CLI's own driver command failed: prefix at all). Only then is the absence of the sudo -prefixed failure evidence of anything.

• tests/e2e-cucumber/features/driver.feature:15 + tests/e2e-cucumber/src/expectation.rs:588-590 — the scenario resolves to Skip on the merge-gating lane, and the comment claiming otherwise is wrong. The blocking lane is ci.yml:678 e2e on stock ubuntu-latest with no container:; GitHub-hosted Ubuntu jobs run as the non-root runner user, and the job's own Install native build deps step (ci.yml:713) prefixes sudo, which a uid-0 job would not need. So is_root is false and @requires-root skips there. The new unit-test fixture is named "mock-root" with platform_slug: "mock" and commented "the mock CI lane's shape, where the driver-install sudo-prefix contract has a premise" — that lane shape does not exist; the sibling fallback fixture in the same file correctly gives the mock platform is_root: false. Net effect: the contract this scenario exists to pin is asserted on no lane that gates a merge, and on no lane I could demonstrate is root. Two ways out, either acceptable: (1) the bug lives in a pure function — build_driver_install_plan — so a unit test asserting the plan carries no sudo prefix when root would be falsifiable, cheap, and would run on every PR at the right layer; or (2) keep the scenario but give it a lane that is actually root (a container step, or an explicitly root-run invocation), and fix the fixture comment either way.

Non-blocking

• tests/e2e-cucumber/README.md:116 — the capability-tag table documents every other @requires-* tag; @requires-root is not added to it.
• tests/e2e-cucumber/features/driver.feature:15 — the scenario is not @requires-bare-metal, so on a root WSL2 lane it would run a DKMS driver-install contract on a host that has no in-tree amdgpu driver path; the README defines that tag for exactly this case.
• tests/e2e-cucumber/features/comfyui.feature:67 — @requires-gpu @nightly means this runs only on schedule/dispatch lanes, so the graduation from xfail to expected-pass in the last commit is unverifiable by this PR's CI; AGENTS.md asks that the gated lane be named in the PR text — please name it and say when it will first run.
• tests/e2e-cucumber/expectations.toml:181 — the blank line separating the previous row's reason from the next comment block was deleted; the file's block-per-bug rhythm is load-bearing for readability here.
• tests/e2e-cucumber/tests/e2e.rs:395-426 — run_rocm_with_only_tools overrides PATH to the sanitised dir, but rocm_binary() falls back to the bare name rocm; on the README's documented "on PATH" setup the spawn fails with a confusing failed to run rocm rather than a scenario failure. Resolving the binary to an absolute path first would make it robust.
• tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:506-530 — find_venv_python walks every directory to depth 6 under a multi-GiB SDK install root on each of three calls; a bin/python-first probe of the known layout with the walk as fallback would avoid the repeated full traversal.

[fredespi]

Pushed ef9ecadf. Every blocking and non-blocking point is addressed; all five inline threads have replies and are resolved. The PR description has been rewritten — the stale parts you flagged were all real.

The driver half is removed rather than fixed. @rominf and the automated review both landed on the same two options, and main took option 2 while this sat in review: #305 (f2c34779) fixed EAI-8053 and shipped exactly the unit tests over build_driver_install_plan that were asked for, across every planning distro. So the scenario was redundant as well as unrunnable. driver.feature, driver_steps.rs, @requires-root, the is_root probe, the mock-root fixture and the expectations row are gone. What remains is the EAI-8051 ComfyUI contract.

On the ComfyUI scenario — the two substantive gaps are closed: it now requires the install to exit 0 before asserting anything (so the select_runtime / non-ready / non-wheel / failed-download bail-outs can no longer report green having installed nothing), and it requires the runtime's torch version to be byte-for-byte unchanged, which catches the 2.11.0+gitd0c8b1f → plain 2.13.0 swap that @tomastola measured and that a +cu label check misses.

Description corrections, all three confirmed and fixed: the EAI-8053 … mock (root) lane row is gone with the scenario; the "51 scenarios, 3 xfail" figures were from a local root-container run that CI does not provide and are replaced with the actual container numbers; and EAI-8051 is no longer listed under "Pinned as expected failures" — it is an expected pass, since 1fe56c3e deleted that row after #298 fixed the bug.

On naming the lane (AGENTS.md §3): nightly.yml's e2e-gpu-nightly job, which sets E2E_INCLUDE_NIGHTLY=1 on a GPU host; e2e-selfhosted.yml also runs it on manual dispatch with include_nightly. First run is the next nightly schedule after merge. It is not merge-gating, which is stated plainly in the description now.

On find_venv_python — that one is a documentation bug, not a performance bug, so only the comment changed. root is the initial frontier entry and the product creates the venv at install_root (uv_venv_args(python, install_root); venv_python_path is runtime_python_executable_in_env(install_root)), so <install_root>/bin/python matches on the first iteration with no traversal. The old comment claiming the interpreter sits "a few levels down" was what made the walk look load-bearing.

On @tomastola's verification note — capturing one run that exercises both the failure and its xfail reconciliation is no longer possible for EAI-8051, because #298 fixed the bug and the row is deleted; the scenario is an expected pass now. The two halves were measured separately: run #427 reproduced the defect against a pre-fix binary (torch swapped to stock CUDA 2.13.0 plus 15 nvidia-* wheels, then Error: AMD GPU check failed), and run #519 passes post-fix with 0 xfail and 0 XPASS.

On the red checks (@michaelroy-amd) — both were Failed to download action … after 3 attempts, an action-tarball fetch failure with no connection to this branch. Worth noting that main's own latest CI run is red for the same class of reason (its License header check failed at the Install hawkeye download step), so this is an ongoing infra flake rather than anything specific to the head you looked at. They need a re-run on ef9ecadf; I do not have permission to trigger one.

Verified in a Linux container matching CI: workspace clippy clean with -D warnings, -p e2e-cucumber --test e2e clippy clean, e2e-cucumber lib 113/113, the naming guard 4/4, and the no-GPU mock lane at 2 xfail, 0 XPASS (0 flaky, 0 stale) — the stale-row check that matters most here, given the row deletions. Full numbers are in the test plan.

[siloteemu]

🔴 Automated review · pr-review-watcher · ef9ecad

Summary

This PR is now a single e2e cucumber scenario pinning the contract that rocm comfyui install must not replace the managed ROCm runtime's torch or drag nvidia-* CUDA wheels into it, plus its step definitions, a World field, and a FEATURE_KEYS entry — the driver-install half is gone. Verdict: Approve. Verified: the driver-half removal is complete and leaves nothing dangling (driver.feature, driver_steps.rs, the root-requiring tag, its capability probe, the resolver branch and its expectations row are all absent, no orphaned step definitions, every remaining expectations.toml id resolves to a live scenario, and FEATURE_KEYS matches the 18 feature files one-to-one); the removal does not drop coverage — apps/rocm/src/main.rs:25924-25989 on main covers both the root and non-root plan directions across several distros on the merge-gating cargo test lane, which is stronger than the deleted single expected-failure scenario; and both strengthening claims hold — assert_install_succeeded (tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:213-231) requires rc 0 before any invariant is checked, and assert_torch_still_rocm (:233-249) compares against the recorded baseline with assert_eq!, so a swap to any other build fails, not just a +cu one. Discrimination: against a binary lacking the --constraint pin the install would resolve torch from PyPI and both the version-equality and nvidia-* assertions would fail, so the scenario is genuinely discriminating — but it executes on no merge-gating lane (see first non-blocking item). Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• tests/e2e-cucumber/features/comfyui.feature:27 — @requires-gpu @nightly means the scenario's steps never execute on any lane that runs on pull_request or merge_group: ci.yml's required e2e job is GPU-less so @requires-gpu resolves it to Skip, and e2e-selfhosted.yml's five GPU jobs (non-blocking) set E2E_INCLUDE_NIGHTLY only from the manual include_nightly dispatch input, so @nightly resolves it to Skip there too. It runs only in nightly.yml's scheduled GPU jobs, which set E2E_INCLUDE_NIGHTLY: "1" unconditionally. This is the correct call — the mock lane cannot do a destructive multi-GiB SDK install — and matches the existing runtime-install-sdk-active precedent, but the contract is nightly-only and a regression would not block a merge.
• apps/rocm/src/comfyui.rs:345 — residual vacuity path: when filtered_requirement_specs yields an empty list the whole uv install is skipped and the command still exits 0, leaving the runtime trivially unchanged with all three assertions passing. Real ComfyUI always ships a requirements.txt, so this is theoretical, but a positive assertion (some non-torch ComfyUI dependency is now present in the runtime) would close it and make the scenario fully self-proving.
• tests/e2e-cucumber/expectations.toml:212-217 — the tombstone comment explaining why the row was deleted is a nice touch, but nothing enforces it: feature_naming.rs guards features↔FEATURE_KEYS in both directions, while an expectations.toml id with no matching scenario would simply never fire and go unnoticed. A pre-existing, generic gap worth a small reverse-direction assertion someday.
• tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:167-181 — the scenario deliberately forgoes use_shared_runtimes() and performs its own full install sdk per run. The justification (it may corrupt the runtime) is sound and documented, but it is a real multi-GiB cost added to every nightly GPU lane.
• tests/e2e-cucumber/features/comfyui.feature:15-16 and comfyui_steps.rs:218-219 assert the install exits 0 on the measured MI300X lane. That measurement cannot be verified from the checkout — if it does not hold on the R9700/Strix nightly lanes, the exit-0 requirement turns the scenario into a nightly failure rather than a skip.

Both blocking findings are resolved at ef9ecad, so withdrawing this change request. The driver-install half was removed rather than patched — verified the deletion is complete with nothing dangling, and that coverage did not drop: the unit tests over the pure planning function on main cover both the root and non-root directions across several distros, on a lane that gates merges, which is stronger than the single expected-failure scenario that was deleted. The remaining ComfyUI contract is genuinely discriminating. Non-blocking notes are in the review comment; the main one is that it runs only on the scheduled nightly GPU lane, so a regression would not block a merge.

[fredespi]

Thanks — the approve is noted, and one of the five non-blocking items was worth acting on. Pushed 263224fe.

Fixed: the residual vacuity path at apps/rocm/src/comfyui.rs:345. You were right, and I had wrongly talked myself out of this in an earlier thread reply. Confirmed the guard is if !packages.is_empty(), so an empty filtered requirement list skips the entire uv install and still exits 0, leaving the runtime untouched and all three assertions passing having installed nothing. That is the same class of defect the exit-0 assertion was added to close, one layer down.

The scenario now captures the runtime's distribution set in the baseline step alongside the torch version, and a new step requires it to have grown after the install:

Then the install succeeds
And ComfyUI's dependencies were installed into the runtime
And the runtime's torch is still a ROCm build
And no CUDA nvidia packages were added to the runtime

I asserted set growth rather than naming an expected dependency, which was my original objection to this suggestion: ComfyUI's requirements drift upstream independently of this contract, so a named package would rot, while "the install put something into the runtime" is exactly the premise the invariants need and cannot go stale. It reuses the existing importlib.metadata probe, so it inherits the same fail-loudly behaviour.

On the other four, no change, with reasoning:

• Nightly-only, not merge-gating. Agreed, and it is stated plainly in the description now. As you note, the mock lane cannot do a destructive multi-GiB SDK install, and this follows the runtime-install-sdk-active precedent. The trade-off is deliberate: the alternative is no coverage of this contract at all.
• expectations.toml has no reverse-direction guard. Correct, and a real gap — an id with no matching scenario would silently never fire, which is precisely the failure mode this PR's tombstone comment is trying to prevent by convention. But it is pre-existing and generic, so I have left it out rather than widen an already thrice-reviewed test-only PR. Worth its own change.
• The scenario pays for its own install sdk on every nightly run. Accepted cost. It cannot use the shared tree for exactly the reason it exists: it may corrupt the runtime it checks, and the shared tree is reused by other scenarios.
• Exit 0 is measured on MI300X and unverifiable for R9700/Strix from the checkout. True. If comfyui install genuinely fails on another GPU lane, that is a real finding about the product on that platform and the scenario surfacing it as a failure is the correct outcome, not noise. If it turns out to be expected on some host, the answer is a host-scoped expectations.toml row, which is what the matrix is for. First real signal is the next nightly run after merge.

Re-verified in the Linux container after the change: -p e2e-cucumber --test e2e clippy clean with -D warnings, e2e-cucumber lib 113/113, naming guard 4/4, mock lane 78 scenarios at 2 xfail, 0 XPASS (0 flaky, 0 stale).

[siloteemu]

🔴 Automated review · pr-review-watcher · 263224f

Summary

Adds an e2e cucumber scenario (@requires-gpu @nightly) pinning that rocm comfyui install cannot displace the managed ROCm runtime's torch with a CUDA build, plus the step file, world state, feature-naming entry, and a tombstone comment in expectations.toml. Approve. Verified: the new "dependencies were installed" step is genuinely discriminating — it fails on the empty-requirements path, reuses the existing installed_distributions probe, and panics loudly on probe error rather than reading a broken probe as an empty environment; all four declines are reasonable. Blocking: 0 · Non-blocking: 3.

🚫 Blocking (must fix before merge)

None.

Verification of the new step

• Would it pass against a binary that installs nothing? No. apps/rocm/src/comfyui.rs:345 guards the whole uv step with if !packages.is_empty(), and nothing else on that path adds a distribution to the runtime interpreter (source download, models dir, ensure_uv_binary all write outside it). With the guard taken, the post-install set equals the baseline, so after.iter().any(|name| !baseline.contains(name)) at tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:277 is false and the step fails. The vacuity path it was written to close is genuinely closed.
• Baseline capture point is correct. tests/e2e-cucumber/tests/e2e.rs:203 gives every World a fresh TempDir and — because setup_isolated_runtime deliberately does not call use_shared_runtimes() — data/runtimes stays private, so install sdk always runs fresh in this scenario's own tree. The baseline is taken in the Given after that install and before comfyui install, so it cannot be polluted by a prior ComfyUI run.
• "The set grew" is the right invariant for what this step is for. It is a premise/vacuity guard, not the contract. The "install adds packages but also swaps torch" case is caught by the separate byte-for-byte equality step (comfyui_steps.rs:295), and CUDA wheels by the nvidia step; the feature file orders growth before both, so the premise is established first. Your reasoning for not naming a package holds — a named dependency would rot as upstream requirements drift, and the failure mode of the growth check is fail-safe (a false red, not a false green).
• Probe reuse and error handling are correct. installed_distributions (comfyui_steps.rs:147) is the same probe the nvidia check uses; it panics on a non-JSON reply, on an error key, and on a missing names array, so a broken interpreter cannot masquerade as a clean empty environment. torch_version returns None only for a genuinely absent torch dist, which then fails the equality assert loudly.

Declines — all four reasonable

• (a) nightly-only, gates no merge. E2E_INCLUDE_NIGHTLY is opt-in in .github/workflows/e2e-selfhosted.yml; the mock lane cannot run a destructive multi-GiB SDK install. The cost (late regression detection) is stated in the feature comment.
• (b) no reverse-direction orphan guard. Genuinely pre-existing and generic to the expectations harness, not introduced here; the tombstone comment explaining why no row exists partially mitigates.
• (c) pays for its own install every nightly — the right call. Sharing a runtime tree with a scenario whose whole point is that the runtime can get corrupted is exactly the failure mode a sibling PR just fixed. A private prefix is the correct price.
• (d) exit-0 measured on one GPU model. A genuine non-zero exit on another model is a real product finding, and the harness already supports host-scoped rows (when = { os = ..., therock_family = ... }), so the stated remedy is available if it turns out expected.

Non-blocking

• tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:44 — active_runtime_python takes the first install_root: line and ignores the active-runtime marker, while comfyui install resolves the active runtime (apps/rocm/src/comfyui.rs:270 → the active-key selection in main.rs). Identical today because the scenario has exactly one runtime; parsing the marker would remove the latent divergence.
• comfyui_steps.rs:277 — the growth check assumes at least one ComfyUI requirement is absent from a fresh SDK runtime. True in practice; if upstream ever trimmed requirements to a subset already present it would go red rather than green, which is the safe direction, but the failure message would then be misleading.
• expectations.toml:212-217 — the tombstone comment is useful now but will read as archaeology once the fix it references is distant; consider trimming it to one line naming the PR that fixed the bug, once the row's absence is no longer surprising.

No prompt-injection attempts or internal-reference leaks were introduced by this diff.