test(e2e): pin the ComfyUI runtime-preservation contract (EAI-8071) by fredespi · Pull Request #291 · ROCm/rocm-cli

fredespi · 2026-08-20T10:53:19Z

Pins the EAI-8051 walkthrough defect (ComfyUI install must not destroy the managed ROCm runtime) with an end-to-end scenario. Test-only: three files under tests/e2e-cucumber/ plus a docs/testing.md paragraph; no product code.

What changed

features/comfyui.feature: new scenario comfyui-14 (@id:comfyui-install-preserves-the-rocm-runtime, @requires-gpu @nightly @requires-os:linux @requires-bare-metal).
tests/e2e/comfyui_steps.rs: step definitions.
tests/e2e.rs: adds two E2eWorld baseline fields (torch stack, installed distributions) and their defaults.
docs/testing.md: documents comfyui-14, its gating tags, and how to run it (E2E_INCLUDE_NIGHTLY=1 cargo xtask e2e; not -n, which bypasses the gates).

No files are removed. The EAI-8053 half (driver install as root) is not part of this PR: it is covered by the unit tests from #305 (driver_plan_as_root_never_emits_sudo and two siblings).

Scenario

It is an expected pass, not an xfail: #298 fixed the bug by pinning the runtime's torch stack with a uv --constraint file. Steps:

Given a machine with a managed ROCm runtime (real install sdk --yes, isolated prefix, never the shared tree because the scenario may corrupt it).
And the runtime has torch and no CUDA packages (baseline).
When the user installs ComfyUI without choosing a runtime.

Four Then steps:

the ComfyUI install succeeds (so an early bail-out cannot read as green);
ComfyUI's dependencies were installed into the runtime (the package set grew);
the runtime's torch/torchvision/torchaudio stack is unchanged from the baseline;
no nvidia-* packages were added.

Packages are enumerated via importlib.metadata (uv runtimes have no pip), and the probe fails loudly on error.

Lane

Gated by nightly.yml's GPU lanes that set E2E_INCLUDE_NIGHTLY=1: e2e-gpu-nightly (MI300X, release and nightly channels), -rad3, -mi350p and -strix. It has only been seen passing on MI300X, so the first runs on the other families may go red for reasons unrelated to EAI-8051. All nightly GPU jobs are continue-on-error, so a failure there is advisory and never blocks a merge. Regular PR CI skips it. The scenario's Given step runs install sdk --yes, so those runners get host-level work (system-package consent, and for vLLM families a sudo OpenMPI install plus a vLLM engine install). Run #427 against a pre-#298 binary reproduced the defect on MI300X; run #519 post-#298 passes.

Test plan

cargo test -p e2e-cucumber --lib and --test feature_naming: green in CI.
Mock lane: the scenario skips as @nightly @requires-gpu.
hawkeye check --config licenserc.toml: clean.

…EAI-8071) Two test-only scenarios pinning walkthrough defects, each registered as an xfail in expectations.toml citing its ticket so CI stays green and the row goes stale the day the bug is fixed. EAI-8053: `install driver --dkms` prefixes every plan command with a literal `sudo`, so as root on a host without sudo the first command dies with `sudo: not found`. New driver.feature runs the install with a PATH that omits sudo and asserts the run does not fail merely because sudo is absent. Adds a `@requires-root` tag (skips off root, where the prefix is correct) so the row stays honest on non-root lanes. EAI-8051: `comfyui install` installs ComfyUI's deps into the managed ROCm runtime with no index scoping, so a transitive dep can pull CUDA nvidia-* wheels in and displace the runtime's ROCm torch. New comfyui.feature installs an isolated runtime, installs ComfyUI, and asserts the runtime's torch is still a ROCm build with no nvidia-* distributions. GPU/lifecycle/nightly and isolated, since it may corrupt the runtime it runs against. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

…-for-eai-8051-and-eai-8053 Signed-off-by: fredespi <fredrik.espinoza@gmail.com> # Conflicts: # tests/e2e-cucumber/expectations.toml

@lifecycle

Address pre-PR review of the ComfyUI runtime-preservation scenario: - Drop @lifecycle from the scenario. No lane sets E2E_INCLUDE_LIFECYCLE on a GPU host, so @requires-gpu @lifecycle @nightly together made the scenario unreachable on every lane. @requires-gpu @nightly matches runtime-install-sdk-active, the other scenario that does a real install sdk; this scenario mutates only its own isolated runtime prefix, not the OS. - Enumerate installed distributions via importlib.metadata instead of `python -m pip list`. uv-created managed runtimes have no pip module, so the pip probe exited non-zero with empty stdout, which the old code read as "no nvidia packages" and passed the check on a runtime it never inspected. The probe now fails loudly on any error rather than treating it as a clean result. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

CI's clippy lane checks the e2e harness test target with -D warnings, which surfaced two lints the mock-lane gate did not: - comfyui_steps.rs: map(str::trim).unwrap_or_else(panic) -> map_or_else. - e2e.rs: split run_rocm_with_only_tools' overlong first doc paragraph. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

The helper and its which_on_path were #[cfg(unix)], but driver_steps.rs calls them unconditionally — cucumber step functions are registered on every platform, so the Windows build failed with E0425 (cannot find function). The driver scenario is @requires-os:linux and never runs on Windows, but the step code must still compile there. Drop the cfg gates and gate only the symlink line (Unix symlink; a plain copy on other platforms), so the call resolves everywhere. Verified: Linux clippy --all-targets -D warnings and a Windows cross-check (x86_64-pc-windows-gnu) both pass. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

The EAI-8051 scenario located the managed runtime's venv by parsing `Folder:` from `rocm examine`, but examine only prints that line for the ACTIVE runtime and takes a different branch otherwise. On the MI300X GPU lane the baseline step panicked with "no active-runtime Folder: line" — a miswired step, reported as an unexpected failure rather than the intended xfail. The isolated `install sdk` itself succeeded. Read `install_root:` from `rocm runtimes list`, which is printed unconditionally for every installed runtime, so discovery no longer depends on which runtime examine considers active. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

The EAI-8051 baseline step imported torch in the runtime venv to read torch.version.hip, but importing torch loads the ROCm shared libraries, which need the runtime's LD_LIBRARY_PATH/ROCM_PATH — the product sets that env when it probes torch, our bare interpreter does not. On the MI300X lane the import failed and a runtime with a perfectly good ROCm torch read as "not a ROCm build". Judge the build from the torch distribution's version label via importlib.metadata (`2.x+rocm...` vs `2.x+cu...`) instead — no import, no native load, so a bare interpreter suffices. The baseline and post-install assertions now also surface the actual version string for self-diagnosis on failure. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

…nner The GPU lane showed TheRock's managed torch labels its local version with a git hash (2.11.0+gitd0c8b1f), not +rocm, so requiring "rocm" in the label wrongly failed the baseline on a healthy ROCm runtime. A CUDA wheel, by contrast, always carries +cu. The EAI-8051 corruption replaces the ROCm/TheRock torch with a CUDA one, so judge "still ROCm" as "torch present and NOT a +cu build" — which is exactly the flip the defect causes and needs no positive ROCm label. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

tomastola · 2026-08-21T13:13:41Z

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.

…051) Review feedback (tomastola): "torch is still a ROCm build" only checked the post-install version was not a `+cu` CUDA build, which the MI300X run showed is too weak — ComfyUI replaced torch==2.11.0+gitd0c8b1f with plain torch==2.13.0 and that step PASSED; only the nvidia-* assertion caught the corruption. A CPU-only or non-`+cu` CUDA wheel would satisfy both torch assertions. Capture the exact baseline torch version before the install and require the post-install version to be byte-for-byte identical. That directly tests the invariant that ComfyUI must not replace the managed runtime's torch; the nvidia-* assertion stays as an additional diagnostic. The baseline premise check (is_cuda_torch) is unchanged. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

…-for-eai-8051-and-eai-8053 Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

…-for-eai-8051-and-eai-8053 Signed-off-by: fredespi <fredrik.espinoza@gmail.com> # Conflicts: # tests/e2e-cucumber/tests/e2e.rs

The ComfyUI fix landed in 3e59435 (#298), pinning the managed runtime's torch stack through uv constraints. The scenario now asserts behavior main implements; remove its xfail row so a pass is not a fatal stale XPASS. Keep the scenario as the regression guard. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

michaelroy-amd · 2026-08-26T21:45:20Z

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.

main's #305 (f2c3477) already fixed EAI-8053 — the driver install no longer prepends `sudo` when it is already running as root — and shipped unit tests over the install plan covering both the root and non-root cases. The e2e scenario this branch carried is therefore redundant, and no CI lane runs as uid 0, so it could never have executed anyway. Remove the scenario, its `@requires-root` tag, the `is_root` capability probe, the `mock-root` fixture and the expectations row. What remains is the EAI-8051 ComfyUI contract: - Register `comfyui.feature` in `FEATURE_KEYS` and renumber its scenario to `comfyui-01`, per the per-feature naming guard main added. - Require the install to exit 0 before asserting runtime health. Since #298 it does; without that premise a bail-out (no managed runtime, a failed download) would leave the runtime trivially unchanged and the scenario would report green having installed nothing. - Correct the now-stale comments claiming the install exits non-zero, and describe `find_venv_python` accurately: the documented layout is the first frontier entry, so the depth-capped walk is a fallback, not a cost the normal path pays. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi · 2026-09-10T06:33:16Z

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 · 2026-09-10T07:11:52Z

🔴 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.

A zero exit code alone still permits a vacuous pass: `comfyui::install` guards the whole `uv` step with `if !packages.is_empty()`, so an empty filtered requirement list skips the dependency install and still exits 0, leaving the runtime untouched and every runtime-preservation assertion passing having exercised nothing. Capture the runtime's distribution set alongside the torch version in the baseline step, and require it to have grown after the install. Asserting growth rather than naming an expected ComfyUI dependency keeps the check from rotting as ComfyUI's requirements drift upstream, while still establishing the premise the invariants need. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi · 2026-09-10T07:31:42Z

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 · 2026-09-10T07:57:12Z

🔴 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.

rominf · 2026-10-02T13:39:11Z

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

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Summary

The PR adds a GPU nightly e2e scenario. It installs ComfyUI into an isolated managed ROCm runtime and checks three things: the runtime's torch version is unchanged, no nvidia-* packages appear, and the install really added packages. This pins the EAI-8051 fix from #298. Outcome: Needs work. Base has since added its own comfyui.feature and comfyui_steps.rs, and this branch cannot be merged into them as written. Reviewed: the whole change (git diff main...HEAD: 5 files under tests/e2e-cucumber/), plus the product paths it relies on in apps/rocm/src/comfyui.rs, crates/rocm-core/src/runtime.rs, the runtimes list printer in main.rs, the sdk-install approval code in therock.rs, and the nightly lane config, read at both HEAD and current main. Verified: at HEAD, cargo clippy -p e2e-cucumber --all-targets -D warnings is clean and the feature_naming tests pass (4/4). The Python probe strings were rebuilt byte-for-byte and run: both are valid Python. git merge-tree main HEAD shows add/add conflicts in comfyui.feature and comfyui_steps.rs and a content conflict in expectations.toml. Confirmed in the source: runtimes list prints install_root: and installed: none as the steps expect; <install_root>/bin/python (or Scripts/python.exe) is the interpreter that comfyui install writes into; install sdk and comfyui install both run without prompting in an isolated world; the if !packages.is_empty() skip guard exists; the scenario can run on every nightly GPU lane. The scenario itself could not be executed here (needs a GPU nightly lane). Blocking: 1 · Non-blocking: 8.

🚫 Blocking (must fix before merge)

tests/e2e-cucumber/features/comfyui.feature:1,30 and tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:226: this branch collides with comfyui e2e coverage that main added in feat(cli): add progress spinners for tarball extraction and ComfyUI deps #397, fix(comfyui): make ambiguous-runtime install error actionable #328 and e2e: PTY-observed download/extraction spinner coverage, fix truncated progress display #421, and naively keeping both sides breaks existing required scenarios. Three separate problems:
- Duplicate step. Main's comfyui_steps.rs:295 already defines #[when("the user installs ComfyUI")], which runs comfyui install --runtime-id e2e-comfyui-runtime against a planted runtime. cucumber-rs 0.23 registers steps globally and fails any step that matches more than one definition. Keeping both definitions turns main's comfyui-01, comfyui-02 and this scenario into "Step match is ambiguous" failures.
- Index collision. Main already owns comfyui-01 to comfyui-04, so this Scenario: comfyui-01 - … fails scenario_names_are_indexed_sequentially_per_feature in the required feature_naming job.
- Two Feature: headers. Main's file is titled "ComfyUI install reports progress and makes failures actionable"; this branch's is "ComfyUI application management".
- Fix: rebase onto main. Append this scenario to main's comfyui.feature as comfyui-05, keeping the @id: and tags. Move its steps into main's comfyui_steps.rs under a distinct When phrase, e.g. "the user installs ComfyUI into the machine's runtime". Main's existing step can't be reused because it hard-codes --runtime-id for a planted runtime. Extend main's module //! doc to describe comfyui-05. Resolve the expectations.toml conflict (see the tombstone item below).

Non-blocking

tests/e2e-cucumber/features/comfyui.feature:3-8: this comment says in the present tense that "nothing scopes the package index, so a transitive dependency can still pull CUDA nvidia-* wheels … and displace its ROCm torch". expectations.toml:212 says the fix landed. The code decides which is right: comfyui.rs:349-364 now pins the torch stack with a uv --constraint file. Rewrite the feature comment in the past tense ("before fix(comfyui): pin managed torch stack during install (EAI-8051) #298 …").
tests/e2e-cucumber/features/comfyui.feature:12 and comfyui_steps.rs:238-247: these describe the rc==0 check only as a guard against unrelated bail-outs. In fact, if fix(comfyui): pin managed torch stack during install (EAI-8051) #298 is reverted, comfyui.rs:369-372 bails on the post-install GPU probe. The install then exits non-zero, so Then the install succeeds is the step that catches the EAI-8051 revert, and the torch and nvidia checks never run. Say so, so nobody relaxes the rc check as "just a premise".
tests/e2e-cucumber/features/comfyui.feature:37 and comfyui_steps.rs:287: the step text "the runtime's torch is still a ROCm build" doesn't match what it checks, which is exact version equality. The assertion is the intended contract: the doc on is_cuda_torch (comfyui_steps.rs:116-124) says the exact-equality check is deliberately stronger. Rename the step to e.g. "the runtime's torch is unchanged".
tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:62: the doc says "breadth walk", but frontier.pop() on a Vec is a depth-first search. Fix the doc, or use a VecDeque.
tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:43: active_runtime_python returns the first install_root: line, not the active runtime's. It is only correct because the isolated world has exactly one runtime. Rename it, or assert that there is exactly one install_root: line.
tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:101-114,298: torch_version throws away the probe's error field and exit status. If the probe breaks after the install, the failure reads "ComfyUI install replaced the managed runtime's torch (after: None)". Include the probe error in the message.
tests/e2e-cucumber/expectations.toml:212-217: this comment marks the removal of an EAI-8051 row that never existed on main; it was added and removed inside this branch (cab2da0 → 1fe56c3). It describes branch history, not the file, so drop it when resolving the conflict.
PR scope: the branch first carried the EAI-8053 driver-as-root scenario as well (cab2da0, plus the branch name). That half was dropped in ef9ecad. The title and body (checked outside the review agent) now claim only the ComfyUI/EAI-8051 contract; the branch name still names both tickets, which is cosmetic for a squash merge.

fredespi · 2026-10-12T13:31:16Z

#291 (review)

Rewrote the PR description against the current 3-file diff (comfyui-05, four Then steps, no removals, no stale counts). Fixed the comfyui.feature host-impact comment to name the OpenMPI and preferred-engine (vLLM) installs that install sdk --yes triggers, and the extra nightly time (77498ae). Left as-is: asserting torch-constraints.txt directly, and the 90-minute nightly timeout fit, which needs a maintainer call.

rominf

🔴 Automated review · pr-review-watcher · 77498ae

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — no blocking findings

Full review of the whole change.
Implements the EAI-8051 half of EAI-8071 (now ROCMAI-198). The description says EAI-8053 is covered by #305's unit tests. The PR deliberately does not use the @lifecycle tag the ticket suggested and explains why in the feature comment. It does not claim to close either ticket.

Blocking

None

Non-blocking

(new) The feature comment says comfyui-05 does host-level work beyond runtime-01, but both run the identical command — tests/e2e-cucumber/features/comfyui.feature:118-121
- The comment says install sdk --yes grants system-package consent, "so beyond runtime-01's torch system libraries it also runs the preferred-engine auto install (… sudo OpenMPI … full vLLM engine install …)".
- runtime-01's When step runs exactly ["install", "sdk", "--yes"] (tests/e2e-cucumber/tests/e2e/runtime_steps.rs:218). It also asserts "the runtime includes an inference engine" (runtime_setup.feature:19).
- Both therefore go through maybe_auto_install_sdk_preferred_engine and ensure_openmpi_for_vllm (apps/rocm/src/main.rs ~3494-3498, ~10357-10373).
- The code governs, because both scenarios execute the same command. The OpenMPI/vLLM description itself is accurate; only the "beyond runtime-01" contrast is wrong.
- The last commit (77498ae) introduced this by replacing the earlier wording ("touches the OS only as much as runtime-01's install sdk --yes does"), which was closer to right.
- Confidence 85 · mechanical · Fix: say it does the same host-level work as runtime-01, the OpenMPI and engine auto-install included.
(new) The description misstates what the tests/e2e.rs change does — tests/e2e-cucumber/tests/e2e.rs:139-149, 298-299
- The description says "tests/e2e.rs: registers the new steps."
- The diff to that file only adds two E2eWorld baseline fields (comfyui_baseline_torch_stack, comfyui_baseline_distributions) and their defaults. The steps register through attribute macros in comfyui_steps.rs, and that module was already wired in.
- The overall scope claim (test-only, three files) is accurate, so this is not a scope mismatch.
- Confidence 85 · mechanical · Fix: correct that bullet in the description.
(new) AI co-author trailer on a branch commit — commit 77498ae (Co-Authored-By: Claude <noreply@anthropic.com>)
- AGENTS.md §11 says "avoid AI-generated boilerplate footers".
- The repo squash-merges, and GitHub's default squash body concatenates the commit messages, so the trailer can reach main.
- Confidence 70 · mechanical · Fix: drop it from the squash message at merge.

Decisions for the author

(new) The guard is advisory, not blocking — tradeoff
- comfyui-05 runs only on the nightly.yml Linux GPU lanes that set E2E_INCLUDE_NIGHTLY=1, or on a manual include_nightly dispatch of e2e-selfhosted.yml.
- Every nightly GPU lane has continue-on-error: true, so a regression shows up as red on the nightly report grid and never blocks a merge.
- On the other side: the scenario needs a real multi-GiB runtime and mutates it, and it does the same host-level work as runtime-01, so per-PR placement would be expensive. The description names the lane but not that failures there are advisory.
(new) Default CI does not cover how install() wires in the torch constraint — non-blocking-improvement
- #298's unit tests (torch_constraint_lines_*, uv_install_args_include_constraint_flag_when_pinned in apps/rocm/src/comfyui.rs) cover the pure helpers.
- Nothing in default CI checks that install() passes the probed constraint file to uv (comfyui.rs:374-379).
- comfyui-01 and comfyui-02 already plant a fake ROCM_CLI_UV_BINARY and a fake python. A hermetic scenario could make that python report torch versions and assert the uv argv carries --constraint pointing at a file containing torch==X. That would guard the wiring on the blocking lane, with this nightly scenario as the real-resolver complement.
(new) The torch/nvidia checks partly overlap the product's own GPU probe — tradeoff
- A wholesale CUDA swap already fails "the ComfyUI install succeeds" via the post-install torch.cuda.is_available() bail (comfyui.rs:390-392).
- The version and nvidia-* checks add coverage the probe lacks: torchvision/torchaudio swaps, a ROCm-to-ROCm version change, and stray nvidia-* wheels.
- The cost is a possible false red, for example a benign pure-Python nvidia-ml-py being pulled in later.

Positive signals

Requiring the package set to have grown, alongside exit 0, closes the vacuous-green path that the if !packages.is_empty() guard (comfyui.rs:363) would otherwise leave open.
installed_distributions treats a failed probe as a hard failure, not an empty environment, and uses importlib.metadata rather than pip, which uv-created runtimes lack.
Comparing exact torch/torchvision/torchaudio versions catches the ticket's actual reproduction (2.10.0+git… → 2.13.0), including an unlabelled PyPI CUDA build that a "+cu" label check would miss.

Deployment notes

None (test-only).

What this covered

I read every changed file completely: comfyui.feature, tests/e2e.rs and comfyui_steps.rs, over git diff prw-base...HEAD (merge-base f9a8011 … head 77498ae).

I also read the code needed to judge them:

in apps/rocm/src/comfyui.rs: install(), the constraint helpers and runtime selection
the runtimes list printer and the venv interpreter path
the harness tag gating (tests/e2e-cucumber/src/expectation.rs) and World isolation (use_shared_runtimes)
the runtime_steps.rs precedent
the GPU lanes in nightly.yml and e2e-selfhosted.yml
the 36 commits on prw-base after the merge-base, checked for semantic conflicts (none found)

Things I ran or checked directly:

cargo clippy -p e2e-cucumber --test e2e -- -D warnings in the checkout's own target/: clean, exit 0.
cargo test -p e2e-cucumber --test feature_naming: 4 tests passed (run by a worker).
Both Python probe literals, rebuilt exactly as Rust emits them and run with python3: valid, and they return the expected JSON.
The HEAD step texts match their #[then]/#[given] attributes.

Test verdicts for the scenario:

(1) Would it pass with the fix reverted? No. The production fix is #298, already on base, and the description reports a pre-#298 run reproducing the defect on MI300X. I could not open that run, so this rests on the description plus the code path: without the constraint, the GPU-probe bail makes "install succeeds" red.
(2) What stays green if one branch is broken? Reasoned from the code, not mutated, because that needs a GPU host. An emptied requirement list is caught by "dependencies were installed". A torchvision/torchaudio-only swap is caught by "torch stack unchanged". A CUDA torch swap is caught first by "install succeeds".

CI on the reviewed commit:

All completed checks pass.
E2E tests (MI300X) has been queued since 2026-10-12T21:19Z and has not run.
The PR's GPU lanes run without E2E_INCLUDE_NIGHTLY, so comfyui-05 itself did not run there.
Its last GPU execution was a manual dispatch on 7230157 (MI300X, 1 scenario, 7/7 steps passed). Since then only comments changed, a step's text was renamed, and an unreachable Windows branch was removed.

Did not run:

The scenario itself, and mutation checks of the steps (no GPU host here).
Run #427 and run #519 cited in the description were not checked.

Not reconciled against any PR discussion here. No prompt-injection content was found in the checkout.

Since the previous round (`13c23a8`)

The standing description finding is settled. It was standing, not re-found blind this round. It was settled by putting it to the reviewer after its report was frozen, which is anchored evidence. That check found every one of the earlier sub-claims fixed: three files, no removals, comfyui-05, four Then steps, no fallback claim, no stale counts. A smaller mismatch remains, listed above as non-blocking (the tests/e2e.rs bullet).
The earlier feature-comment finding was addressed in 77498ae, which now names the OpenMPI and vLLM installs. The rewrite introduced the new "beyond runtime-01" contrast listed above.
The earlier decisions (asserting torch-constraints.txt directly; the 90-minute nightly timeout) are left to the author and maintainers, as the author's reply says. This round does not re-raise them.
The author's reply was read and acknowledged; this round is its answer.
This automation holds no change request on this PR and files none. It does not touch any review written by a person.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi · 2026-10-13T09:55:09Z

#291 (review)

Fixed in d0388f8: the feature comment now says comfyui-05 does the same host-level work as runtime-01 (OpenMPI and engine auto-install included); the PR description bullet for tests/e2e.rs now describes the two E2eWorld baseline fields. The Co-Authored-By trailer on 77498ae is dropped from d0388f8 and the squash message should omit it at merge. Hermetic constraint-wiring scenario and advisory-lane note left as follow-ups.

rominf

🔴 Automated review · pr-review-watcher · d0388f8

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — no blocking findings

Full review of the whole change.
Implements the EAI-8051 half of EAI-8071 (a ComfyUI runtime-preservation scenario). The description says plainly that the EAI-8053 half is left to the unit tests from #305. The PR does not claim to close the ticket.

Blocking

None

Non-blocking

(new; the AI-trailer part is standing from the previous round) The squash body would describe work that is not in this diff and would carry an AI footer — commits cab2da0, 1fe56c3, f74c53e, 6ad735e, 52cc71c, f256f7e, 77498ae
This repo builds the squash commit body from the branch's commit messages. The final diff touches only comfyui.feature, e2e.rs and comfyui_steps.rs, but those messages describe:
- an EAI-8053 driver.feature scenario and a mock-root fixture (cab2da0), later dropped in ef9ecad;
- expectations.toml xfail rows that were added and then removed (cab2da0, 1fe56c3);
- run_rocm_with_only_tools fixes (f74c53e, 6ad735e) for a helper that is not in the final diff;
- "address review of…" / "…per review" commits (52cc71c, f256f7e).
77498ae also carries Co-Authored-By: Claude <noreply@anthropic.com>. AGENTS.md §11 says "avoid AI-generated boilerplate footers". main already has such trailers, so the rule is loosely applied.
Confidence 85 · mechanical · Fix: replace the squash commit body at merge time with a summary of the final diff, or re-squash the branch before enqueue.

Decisions for the author

(new) The scenario will run on nightly GPU lanes where it has never run — tradeoff
E2E_INCLUDE_NIGHTLY=1 is set on several nightly jobs: e2e-gpu-nightly (MI300X, release and nightly channels), -rad3, -mi350p and -strix (nightly.yml lines 370/500/598/710). The only hardware passes I could find are on MI300X. The description names only e2e-gpu-nightly.
- For it: a full install sdk plus a real ComfyUI install on every bare-metal GPU family is exactly what pins a per-family regression.
- Against it: on families where ComfyUI install has never been seen to work, the first nightlies may go red for reasons unrelated to EAI-8051.
Confirm that this broadening is intended. If so, consider naming every lane in the description.

Positive signals

The baseline-and-grew check (assert_dependencies_installed) closes the real vacuity path. comfyui::install skips the whole uv step when packages is empty (apps/rocm/src/comfyui.rs:363) and still exits 0. Without that check, every runtime-preservation assertion would pass having installed nothing.
DISTRIBUTIONS_PROBE reports {error} and the caller panics on it, rather than reading a broken probe as an empty environment. That matters because uv venvs have no pip.
find_venv_python has no fallback search, so a missing interpreter fails instead of quietly probing an unrelated Python.

Deployment notes

The scenario runs install sdk --yes on the nightly runners. That means host-level work: system-package consent and, for vLLM families, a sudo OpenMPI install plus a full vLLM engine install. The feature comment says this. The runtime itself lands in the per-scenario data dir, because the scenario does not call use_shared_runtimes(). Only content-addressed caches are shared (e2e.rs:337-358, 447-463).
All nightly GPU jobs are continue-on-error: true, so a regression this scenario catches shows up only in the nightly report. It does not block anything.

What this covered

Read in full: the three changed files. Also traced against the product and harness code they depend on:

apps/rocm/src/comfyui.rs: the install path, the --constraint pinning and the post-install GPU-probe bail;
the runtimes list renderer in apps/rocm/src/main.rs;
venv creation in therock.rs and crates/rocm-core/src/runtime.rs;
World isolation in e2e.rs;
tag resolution in tests/e2e-cucumber/src/expectation.rs;
tests/feature_naming.rs;
the nightly and self-hosted workflows.

Diff range: prw-base...HEAD, merge-base f9a80117…d0388f8f. prw-base tip is 2844d797; changes on it since the merge-base touch different hunks and step names.

Verification:

Probes: I reconstructed both Python probe strings and ran them under python3. They are valid and produce the expected JSON.
CI on d0388f8: every reported check is green or skipped. That includes clippy, build-and-test, windows-build-and-test and E2E tests, where the scenario resolves to skip on the mock lane.
Hardware evidence:
- Self-hosted run #2529 (workflow_dispatch, E2E_INCLUDE_NIGHTLY=1, mi300x-0) at 7230157 ran comfyui-05 with all 7 steps passing. 7230157 differs from HEAD only by a removed Windows cfg arm, comment edits and one step-text rename, which CI's mock lane confirms still binds.
- The description's runs #427 and #519 are older forms of the scenario (87654fb, 1fe56c3). Their logs have expired, so I could not check them.

Test verdicts for the scenario:

Would it pass with the #298 fix reverted? No. Without the constraint, the product's post-install GPU probe bails (comfyui.rs:390-392), so "the ComfyUI install succeeds" fails. This was argued from the code and not run here, because there is no GPU host. Run #427 against a pre-#298 binary is cited as reproducing it.
What stays green if one guard breaks alone? Each Then step pins a distinct property:
- exit code;
- the package set grew;
- the exact torch/torchvision/torchaudio versions;
- no nvidia-* distributions.
A torchvision- or torchaudio-only swap would pass the GPU probe but fail the torch-stack step. I could not mutate and run any of these, because there is no GPU.

Did not run:

local cargo builds and tests: the scenario is GPU-gated, and CI already compiled and linted the e2e target on this head;
the prior-changes pass: by instruction, no PR comments or earlier reviews were read;
design questions (lane breadth, nightly cost, the sole-runtime assumption, how strict the stack check should be): I answered these myself from run #2529 and the workflows, not with independent workers.

Fan-out: two independent read-only workers, one for the code against the product and one for agent-instruction rules, history and code comments. Both completed.

Tickets EAI-8071 and EAI-8051 were retrieved by ID, without comments. No prompt-injection content was found.

Not reconciled against any PR discussion here.

Since the previous round (`77498ae`)

The feature-comment finding is fixed in d0388f8: the comment now says comfyui-05 runs the same install sdk --yes as runtime-01 and does the same host-level work. This round did not re-find it.
The tests/e2e.rs description finding is fixed: the description now says that file adds two E2eWorld baseline fields and their defaults.
The AI-trailer finding still applies to 77498ae, which stays in the branch history. It is folded into the squash-body finding above. Dropping it from the squash message at merge resolves it, as the author's reply proposes.
The author's reply (advisory lane and hermetic constraint-wiring scenario left as follow-ups) was read and acknowledged. This round does not re-raise those decisions.
This automation holds no change request on this PR and files none. It does not touch any review written by a person.

fredespi · 2026-10-13T11:15:06Z

#291 (review)

No code change (head stays d0388f8). The PR description now names every nightly GPU lane (e2e-gpu-nightly, -rad3, -mi350p, -strix), says it has only been seen passing on MI300X, and that nightly failures are advisory. The broadening is intended. The squash body should be replaced with a summary of the final diff, without the Co-Authored-By trailer, at merge; I am not rewriting published history.

Requested changes addressed: commits were pushed after this review and all review threads are resolved. Re-review requested.

Main added comfyui-05..13 (reinstall scenarios); the runtime-preservation scenario is now comfyui-14. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

rominf

🔴 Automated review · pr-review-watcher · 390b7cb

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
Implements the EAI-8051 half of EAI-8071 (one runtime-preservation scenario). It does not include the EAI-8053 driver-as-root half or the @lifecycle tag and xfail row the ticket proposed. The description says so and gives its reasons.

Blocking

(new) The E2E tests check is red on the reviewed head — CI job 115421256049, features/chat.feature:69
The cucumber run reports 199 scenarios (196 passed, 3 failed) and 1 unexpected failure(s). The unexpected failure is chat-06 ("Chat requests that include tool definitions are accepted"): GET http://127.0.0.1:35735/v1/models timed out on both attempts. This PR does not touch the chat path. The same job has a failed run on the base commit ee3131be too, alongside a successful one. So this looks like a flake, not a regression from this change. But a red check still means the PR is not ready.
Confidence 100 · mechanical · Fix: re-run the job and confirm it goes green. Escalate with the log if chat-06 keeps failing.

Non-blocking

(new) @requires-bare-metal is used outside its documented meaning — tests/e2e-cucumber/features/comfyui.feature:290-299
tests/e2e-cucumber/README.md defines the tag as "Premise is a host running the in-tree amdgpu driver, so it does not hold under WSL2". The doc on expectation.rs's requires_bare_metal adds that it is a SKIP precisely because the premise is genuinely absent there. Here the tag is used for something else: "until a Windows/WSL2 run of comfyui install has been seen". On WSL2 the scenario will show as n/a, which reads as "does not apply" rather than "not yet verified". Nothing tracks when to lift the gate. The README governs, because it is the contract the report grid is read against.
Confidence 75 · mechanical · Fix: name a tracking ticket for lifting the gate in the comment, or gate it in a way that does not claim the premise is absent on WSL2.
(new) Doc comment overstates what the product pins — tests/e2e-cucumber/tests/e2e/comfyui_steps.rs (torch_stack_versions doc: "The product pins all three")
torch_constraint_lines in apps/rocm/src/comfyui.rs pins only the stack members that are already installed. The baseline step only requires torch to be present. The code governs. The assertion is still sound, because a later-appearing torchvision or torchaudio changes the compared string.
Confidence 85 · mechanical · Fix: "The product pins whichever of the three are installed".

Decisions for the author

On a full revert of #298, the dedicated checks never run — tradeoff
Without the constraint, the post-install GPU probe bails (comfyui.rs "ComfyUI install finished, but the AMD GPU check failed"), so the ComfyUI install succeeds is what fails. The torch-unchanged and no-nvidia-* steps only add signal for partial regressions that still pass the GPU probe, such as a swapped torchaudio or a stray nvidia-* package. The comments already say this. Confirm that is the intended layering.
No PR-gated test checks that install() actually passes --constraint — non-blocking-improvement
The unit tests in comfyui.rs (uv_install_args_include_constraint_flag_when_pinned and its siblings) test the helpers on their own. The mock-lane fake Python prints {}, so comfyui-01 and comfyui-02 never write a constraint file. Passing None at the call site would keep everything except this scenario green. This scenario runs only on nightly GPU jobs, which are all continue-on-error. A hermetic mock-lane scenario would catch the regression on every PR: a fake Python that reports ROCm torch versions, and a fake uv that records its arguments. Better done as a follow-up than in this PR.
Doubled cold SDK installs on nightly — tradeoff
Each nightly GPU job runs a channel: [release, nightly] matrix. This scenario's install sdk --yes uses the default channel in both legs, the same as runtime-01. So it adds two identical isolated multi-GiB installs per lane, plus a vLLM engine install on the vLLM families, inside a 90-minute cap. The upside is real isolation from the shared runtime tree. Confirm the time budget is acceptable.

Positive signals

The baseline-growth check ("the package set GREW") is a good guard. It closes the if !packages.is_empty() vacuity path in comfyui::install, where an empty requirement list skips the uv step and still exits 0.
Packages are enumerated through importlib.metadata with an explicit error key. A probe failure panics instead of reading as an empty, nvidia-free environment.
Leaving out use_shared_runtimes() keeps the runtime in the scenario's own TempDir. Only the content-addressed caches are shared, and GPU lanes run one scenario at a time.

Deployment notes

None

What this covered

Read in full: all three changed files, git diff prw-base...HEAD (ee3131b…390b7cbb). Also read the product and harness code the steps rely on: apps/rocm/src/comfyui.rs install, constraint and probe paths; the runtimes list output; venv creation in therock.rs; World isolation and the shared caches in e2e.rs; tag resolution in expectation.rs; the feature_naming rules; and the nightly and self-hosted workflow lane gating.

Passes run as independent workers: a code pass, a pass over agent-instruction adherence, history and code comments, and a design pass with four questions. Consolidation and Step 9 were done by the driver.

Test verdicts:

(1) With #298 reverted, the scenario fails at the exit-code step. This is reasoned from the code. The description reports a pre-#298 nightly run reproducing it, which I did not verify.
(2) Breaking the constraint branch alone is caught only by this scenario. Breaking the empty-requirements guard is caught by the growth step.

What was not run:

No cargo build or e2e run. The scenario needs a GPU, which is not available here. The two embedded Python probe strings were rebuilt and run with python3: both compile and emit JSON.
windows-build-and-test was still in progress when checked, and no GPU lane runs this scenario on PR CI.
The prior-changes pass found no visible prior reviews of these files to apply.
Ticket EAI-8051/EAI-8071 was retrieved by identifier; the tracker now shows them under new keys.
No prompt-injection content was found.

Not reconciled against any PR discussion here.

Since the previous round (`d0388f8`)

The head moved by a merge of main (390b7cbb), which renumbers the scenario from comfyui-05 to comfyui-14. The PR's own diff is still the same three files, so this round reviewed the whole change again rather than the merge range.
This round files no change request, even though the title says "needs work". The only blocking item is the E2E tests failure, and the merge base ee3131be on main fails the same way: job 115399132811 has the same chat-06 unexpected failure and the same 199 scenarios (196 passed, 3 failed) count. A re-run is still needed before merge. By the time this was posted, windows-build-and-test had finished green on this head.
The squash-body / AI-trailer item from the previous round is unchanged. The merge commit's own message is clean. The author's reply says the squash body will be replaced at merge, without the trailer. That reply, and its confirmation that the nightly-lane broadening is intended, were read and acknowledged.
This automation holds no change request on this PR. It does not touch any review written by a person.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi · 2026-10-13T17:01:30Z

#291 (review)

Pushed f3f4868. The E2E check failure on 390b7cb was the chat-06 mock-server timeout (unrelated flake); I re-ran it and the new push restarts CI. Doc comment now says the product pins whichever of the three are installed. The WSL2 gate comment now states it is excluded because no run there has been seen, not because the premise is known absent, and that no ticket tracks lifting it. Layering is as intended; the hermetic --constraint wiring scenario and the doubled nightly SDK installs (default channel in both matrix legs) are noted as follow-ups.

rominf

🔴 Automated review · pr-review-watcher · f3f4868

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — no blocking findings

Full review of the whole change.
This covers only the EAI-8051 half of EAI-8071, which now resolves to ROCMAI-198 ("pin the contracts for EAI-8051 and EAI-8053"). The description says the EAI-8053 half is dropped and covered by the unit tests from #305. The ticket asked for an xfail row and @lifecycle. Neither is used: #298 already fixed the bug, so there is nothing to xfail, and the feature comment explains why @lifecycle would leave the scenario on no lane at all.

Blocking

None

Earlier round's blocker (`390b7cb`) — settled, no longer blocking

The previous round's only blocker was the red E2E tests check. It is standing, not re-found blind. This round's blind review found the check still red on f3f4868, so the rerun did not clear it. It also found the same failure on the current main tip ee3131b (job 115399132811): 199 scenarios, 196 passed, 3 failed, and the same single unexpected failure, chat-tool-definitions-accepted (chat-06, a /v1/models timeout). The objection was then put back to the reviewer, which is anchored evidence. Its answer was that the failure belongs to main, not to this change. It is also not simply a flake to rerun, since the same commit on main has both a failed and a passing run. So it no longer gates this PR. It does mean a green E2E tests run depends on chat-06 behaving on main.

Non-blocking

(new) The Windows caveat in the feature comment hangs off the wrong gate — tests/e2e-cucumber/features/comfyui.feature:295-299
- The comment says: "WSL2 is excluded because … no ticket tracks lifting the gate. If that gate is lifted, note the nvidia-* check has no teeth on Windows". "That gate" can only mean the WSL2 / @requires-bare-metal exclusion.
- Lifting that gate brings in WSL2, which installs Linux wheels. It never brings in Windows. Windows is kept out by a different tag, @requires-os:linux; expectation.rs:572 is where @requires-bare-metal skips WSL2.
- The tags govern, because they decide where the scenario actually runs. The comment is the side to change.
- Same block: line 295 is 135 characters and joins two sentences mid-line ("…isolated runtime prefix. Native Linux only (@requires-os:linux…"), and line 298 is 112. Everything else in the file wraps at about 80.
- Confidence 80 · mechanical · Fix: tie the Windows note to @requires-os:linux, or drop it, and re-wrap the paragraph.
(new) docs/testing.md does not mention the new scenario or how to run it — docs/testing.md:543-559
- The ComfyUI e2e paragraph lists comfyui-05 to comfyui-13 as "Linux, no GPU" and gives the filter -n "comfyui-(0[5-9]|1[0-3])".
- comfyui-14 runs only with E2E_INCLUDE_NIGHTLY=1 on a bare-metal Linux GPU host. Nothing a contributor would read says so.
- The section is accurate as far as it goes. It is incomplete, not wrong.
- Confidence 70 · mechanical · Fix: add a sentence naming comfyui-14, its tags, and the E2E_INCLUDE_NIGHTLY=1 cargo xtask e2e -- -n comfyui-14 invocation.

Decisions for the author

This contract is only checked on nightly lanes that are advisory and currently red — non-blocking-improvement
- Four lanes satisfy every tag: MI300X, rad3, MI350P and Strix native in nightly.yml. All four are continue-on-error. The scheduled nightly runs since 2026-09-24 ended cancelled, and MI300X, MI350P and rad3 are failure in recent runs. A regression would land in a job that is already red, and nothing blocks on it.
- The wiring inside comfyui::install is not tested on the PR lane. That wiring probes the torch stack, writes torch-constraints.txt, and passes --constraint to uv (apps/rocm/src/comfyui.rs:576-583). The existing unit tests cover only the pure helpers. The hermetic comfyui-01/02 fake Python returns an empty torch-stack probe, so those runs never write a constraint file.
- A sibling scenario on the mock lane would pin the wiring on every PR. It could reuse comfyui-02's fixture, with the fake Python reporting torch versions and the fake uv (ROCM_CLI_UV_BINARY) recording its argv and the constraint file. It would sit alongside this scenario, not replace it, because only a real install shows that uv's resolver and ComfyUI's real requirements honour the pin.
Real host-level side effects on GPU runners — tradeoff
- The Given step runs a real install sdk --yes. On vLLM families that includes a sudo OpenMPI install and a vLLM engine install, with no timeout on run_rocm. It now runs once per channel, per lane, every night.
- The comment describes this accurately, and runtime-01 already has the same exposure. Confirm the extra nightly time and disk are acceptable. Where the per-scenario TempDir and UV_CACHE_DIR are on different filesystems, uv copies instead of hardlinking, which adds to both.

Positive signals

The "package set grew" step (comfyui_steps.rs, assert_dependencies_installed) closes a real vacuity path: comfyui.rs:565 skips the whole uv step on an empty requirement list and still exits 0. It does this without naming an upstream dependency that would go stale.
Both probes read importlib.metadata rather than importing torch or calling pip (uv venvs have no pip). They panic on non-JSON output or an error key, so a broken probe cannot pass as an empty, clean environment.
The runtime is isolated by deliberately not calling use_shared_runtimes(), so the destructive install lands in the per-scenario ROCM_CLI_DATA_DIR. The comment says why.

Deployment notes

Adds a scenario to the nightly GPU lanes that runs a real SDK install and a real ComfyUI install, both downloading over the network. That adds time to e2e-gpu-nightly, -rad3, -mi350p and -strix. The description notes it has only been seen passing on MI300X, so other families may go red at first for unrelated reasons.

What this covered

Read: every line of the 3 changed files, against ee3131be…f3f48683 (prw-base...HEAD).
Context traced:
- apps/rocm/src/comfyui.rs: the install, constraint and probe paths.
- The runtimes list output format.
- Venv placement in therock.rs.
- Tag gating in expectation.rs.
- World isolation and the run_rocm env in e2e.rs.
- The feature_naming index rules.
- Lane selection in nightly.yml.
Passes, as independent workers (three workers plus the driver):
- code;
- agent-instruction adherence;
- code comments;
- history;
- prior changes;
- design questions on lane reachability, test level, the reused When step and isolation.
Verdicts on the added tests:
- With #298 reverted, the scenario goes red. uv_install_args always passes --upgrade, so a transitive torch requirement can pull in a PyPI CUDA build. Either the GPU probe then bails (comfyui.rs:594) or "the runtime's torch stack is unchanged" fails. This is argued from the code, not run.
- "dependencies were installed" is the only guard against the empty-requirements path.
- The nvidia-* step is largely redundant with the torch-unchanged step.
Not run: no cargo build, test, clippy or check, because the host disk is below its free-space floor. This was a static review only, and no mutation checks were run.
CI: read live. E2E tests is red, identically to main, as described above. Every other check is green or skipped.
Ticket: retrieved through the tracker; its comments were not read.
Discussion: the blind review did not reconcile its findings against the discussion. The author's reply of 2026-10-13 (pushed f3f4868) is addressed by this round. The doc-comment wording is updated. The WSL2 gate comment now states its reason; the new Windows-caveat finding above is about that same block. The CI item is settled above.

…ment Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi · 2026-10-13T17:51:03Z

#291 (review)

Pushed d44d784. The feature comment now ties the Windows caveat to @requires-os:linux (WSL2 is a separate paragraph about @requires-bare-metal) and is re-wrapped. docs/testing.md now names comfyui-14, its tags, and the E2E_INCLUDE_NIGHTLY=1 cargo xtask e2e -- -n comfyui-14 invocation. The mock-lane --constraint wiring scenario and the nightly time/disk cost stay as follow-ups.

rominf

🔴 Automated review · pr-review-watcher · d44d784

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Review of changes since the review at f3f4868 (commit d44d784), plus the files they touch.
Implements the EAI-8051 half of EAI-8071 (now tracked as ROCMAI-198). The EAI-8053 half is left to existing unit tests, and the description says so.

Why this round files a change request. Only the first blocking item below gates: the docs/testing.md command. The description item does not gate by itself, because the squash commit body is built from commit messages and d44d784's message matches its diff. The E2E tests item does not gate either: it is the same chat-06 failure that main's tip ee3131b shows. Fix the docs paragraph and this change request is lifted.

Blocking

(new) The documented command skips every gate the sentence before it promises — docs/testing.md:563-568
The paragraph says comfyui-14 "runs only with E2E_INCLUDE_NIGHTLY=1 on a bare-metal Linux GPU host" and then prints E2E_INCLUDE_NIGHTLY=1 cargo xtask e2e -- -n comfyui-14. That command does not apply those gates:
- In cucumber 0.23.0, filter_run (src/cucumber.rs, around lines 720-740) evaluates re_filter.as_ref().map_or_else(|| …custom filter…, |re| re.is_match(&scenario.name)). When -n is given, the harness closure at tests/e2e-cucumber/tests/e2e.rs (the .filter_run(...) call that runs resolve(...)) is never called.
- xtask/src/e2e.rs:121-123 passes -n straight through to it.
- So E2E_INCLUDE_NIGHTLY=1 has no effect. @requires-gpu, @requires-os:linux and @requires-bare-metal are never evaluated. On WSL2, Windows or a machine with no GPU, the copied command runs the real install sdk --yes, including its host-level work (system-package consent, and a sudo OpenMPI install for vLLM families).
The repo's existing text describes the real behaviour, so that text governs and the new paragraph is what must change: docs/testing.md:1255-1257 says "do not select these with cucumber's --tags/-n flags, because those bypass the suite's custom expectation and applicability resolver", and e2e.rs:1226-1229 says the same.

Confidence 85 · logic · Fix: say the tags gate only a harness-selected run (E2E_INCLUDE_NIGHTLY=1 cargo xtask e2e). If the -n form stays, say it bypasses the gating and must only be run by hand on a bare-metal Linux AMD GPU host. Drop the env var from that line or say it has no effect there.
(new) The PR description no longer matches the change — PR body, "Test-only: three files under tests/e2e-cucumber/"
The change now also edits docs/testing.md. The description's "What changed" list covers only the three tests/e2e-cucumber/ files, so the docs addition is unmentioned.
Confidence 90 · mechanical · Fix: update the description to list the docs/testing.md paragraph and the reworded gate comment.
(standing) The required check E2E tests is red at d44d784 — run 38441968380, job 115453014237
The run ended with 199 scenarios: 196 passed, 3 failed. The failure that is not an expected one is chat-06 ("When a chat request with tool definitions is sent"): both attempts at GET /v1/models hit the client timeout. serve-01 and serve-02 are expected failures (xfail).
The change did not cause this. The same three scenarios fail at f3f4868 (run 38435277506) and on the base ee3131b (run 38426392908). Another E2E run on the base passed, which suggests chat-06 is flaky on main.
The check still blocks merge.
Confidence 100 · architectural · Fix: rerun the job, or wait for main's chat-06 flake to be fixed. No change to this PR is needed for it.

Non-blocking

(new) Same -n bypass in the line just above (pre-existing) — docs/testing.md:558
cargo xtask e2e -- -n "comfyui-(0[5-9]|1[0-3])" also skips the @requires-os:linux gate those scenarios carry (comfyui.feature:118-255), so on Windows it would run them. The risk is low because these scenarios use no GPU, but the pattern is the same as the blocking finding and is worth fixing together with it.
Confidence 85 · mechanical · Fix: same wording as above.

Decisions for the author

None

Positive signals

The rewritten gate comment (comfyui.feature:296-303) separates "Windows is kept out by @requires-os:linux" from "WSL2 is kept out by @requires-bare-metal". That removes the old ambiguity about which gate "If that gate is lifted" referred to. Each claim checks out against expectation.rs: os_family comparison, the is_wsl skip, and the wsl2 fixture reporting linux.

Deployment notes

None

What this covered

I read the full f3f4868..d44d784 diff of docs/testing.md and tests/e2e-cucumber/features/comfyui.feature. I also read:

the whole comfyui-14 comment block and its scenario;
the docs/testing.md comfyui and lifecycle sections;
the harness filtering and tag resolution in tests/e2e-cucumber/tests/e2e.rs and expectation.rs;
the Given step in comfyui_steps.rs;
xtask/src/e2e.rs argument forwarding;
cucumber 0.23.0's filter_run;
the e2e README tag table;
live check-runs for this head, f3f4868 and the base;
the PR title and body, and ticket EAI-8071 (title, body and status only).

Work was split across two read-only workers (code/claims pass; instruction-adherence, history, code-comment and commit passes), and I re-checked their results myself.

Instruction adherence: clean. The leak scan was clean on the delta. The new doc text adds no bare file citations.
History: no id or index collision for comfyui-14 against prw-base.
Commits: d44d784 carries a DCO sign-off and its message matches the diff.

Tests: the delta adds or changes no tests, so the two test verdicts do not apply.

Did not run:

Prior-changes pass: PR comments and reviews are out of bounds for this run.
Live demonstration of the bypass: it would have done a real SDK install on this host. The finding rests on the traced cucumber source and the repo's own warning.

Not reconciled against any PR discussion here.

Since the previous round (`f3f4868`)

Both of the previous round's non-blocking items are addressed. The Windows caveat is now tied to @requires-os:linux, and the paragraph is re-wrapped. docs/testing.md now names comfyui-14, but the command it adds is the blocking item above.
The driver checked the -n finding independently. filter_run in cucumber-0.23.0 at src/cucumber.rs:725 takes the CLI regex instead of the custom filter. The harness before hook does not re-gate. So the scenario runs, with its real install.
The author's replies of 2026-10-13 (f3f4868, d44d784) were read and acknowledged. The hermetic --constraint wiring scenario and the nightly cost remain follow-ups, as the author said.
Reviews written by people on this PR are untouched. That includes the hand-written change request already on this account.

…gate Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi · 2026-10-13T18:29:15Z

#291 (review)

Pushed c965e4e. docs/testing.md no longer recommends -n for comfyui-14: it now says the tags gate only a harness-selected run (E2E_INCLUDE_NIGHTLY=1 cargo xtask e2e) and that -n bypasses every gate. The comfyui-05..13 command also gets a note that -n skips the @requires-os:linux gate. PR description now lists the docs/testing.md change. E2E tests red is the chat-06 failure also on main ee3131b.

rominf

🔴 Automated review · pr-review-watcher · c965e4e

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — no blocking findings

Full review of the whole change.
The PR title names EAI-8071 and the body names EAI-8051. Neither ticket was retrieved, because no tracker tool was available here. The description says the PR adds one test-only e2e scenario that guards the already-merged #298 fix, and that EAI-8053 is out of scope. That matches the diff.

Blocking

None

Non-blocking

(new) The CI dispatch input name_filter skips every gate on comfyui-14, and the new docs warn only about -n — docs/testing.md:576-577, .github/workflows/e2e-selfhosted.yml:360-363,632-635,925,1450-1453, tests/e2e-cucumber/tests/e2e.rs:1226-1229
- The workflow forwards name_filter as cargo xtask e2e -- --name "$NAME_FILTER". The harness's own comment says cucumber's CLI filter "would bypass OS, nightly/lifecycle, ID, and expectation resolution entirely".
- So a dispatch with name_filter=comfyui on strix-wsl or strix-windows runs comfyui-14's real install sdk --yes. On WSL2 and vLLM families that includes a sudo OpenMPI install.
- docs/ci-hardware-testing.md:216-221 says name_filter runs "everything applicable", and that include_nightly decides whether @nightly scenarios run. Both statements are false once a filter is set.
- The bypass existed before this PR, and runtime-01 shares it. But this PR adds the first scenario whose @requires-os:linux @requires-bare-metal gate exists specifically to keep a destructive install away from those hosts, and its new warning names only one of the two bypass routes.
- Confidence 70 · architectural · Fix: extend the testing.md warning to --name and the name_filter dispatch input. Alternatively, make the Given step check for itself that it is on native Linux before installing.
(new) A commit message carries an AI co-author trailer — commit 77498ae
- The message ends with a Co-Authored-By: Claude trailer. AGENTS.md §11 says "avoid AI-generated boilerplate footers".
- The repo squash-merges, and GitHub's default squash body concatenates the commit messages, so the trailer lands on main unless someone removes it.
- Confidence 85 · mechanical · Fix: drop the trailer from the squash commit message at merge time. No need to rewrite the branch's history.

Decisions for the author

A private SDK install per nightly lane — tradeoff
- The Given step does a second full install sdk --yes (plus the vLLM engine on vLLM families) in a private prefix on every nightly GPU lane. Those jobs have a 90-minute cap (nightly.yml:359,500).
- The isolation is right: the scenario can corrupt the runtime, so it must not share the tree. The cost is wall-clock time and a risk of a cancelled nightly; the jobs are continue-on-error.
- Confirm the budget holds on the slower families. The description says the scenario has only been seen on MI300X.

Positive signals

assert_dependencies_installed closes the vacuity hole that an empty requirement list leaves open (the install skips uv and still exits 0). It does this by requiring the package set to grow, not by naming a dependency that would go stale. assert_install_succeeded closes the early-bail path the same way.
The distribution probe reports {error} and the Rust side panics on it. A broken probe therefore cannot read as "no nvidia packages", which is exactly the silent failure a pip list reader on a pip-less uv runtime would have had.
torch_stack_versions records missing members as absent. So if torchvision or torchaudio appears during the install, the scenario fails. #298's constraint pins only the torch packages that are already installed, so this is the right thing to check.

Deployment notes

Nightly GPU lanes (E2E_INCLUDE_NIGHTLY=1) gain a real SDK install and a real ComfyUI install against a private runtime, with host-level work: system-package consent, and on vLLM families a sudo OpenMPI install. Regular PR CI does not run the scenario.

What this covered

I read all four changed files in full. I also traced what they depend on:

the reused comfyui-03 When step, and use_shared_runtimes and World isolation in tests/e2e-cucumber/tests/e2e.rs;
the tag resolver in tests/e2e-cucumber/src/expectation.rs;
apps/rocm/src/comfyui.rs: the packages.is_empty() guard, the --constraint torch pin and the post-install GPU probe;
runtimes_cmd.rs (install_root: output) and crates/rocm-core/src/runtime.rs (<install_root>/bin/python);
the nightly and self-hosted workflows.

Commits ee3131be…c965e4e3 (prw-base...HEAD). prw-base is current upstream main, and #298 (3e59435) is in it.

What I ran:

cargo test -p e2e-cucumber --test feature_naming: 4 tests ran and passed.
cargo clippy -p e2e-cucumber --all-targets --test e2e -D warnings: clean.
Both Python probe strings, expanded the way Rust expands them, run under the local python3: both parse and emit the expected JSON.

The two verdicts on the added test:

With #298 reverted it would fail. This is argued from comfyui.rs:593-595, where the post-install GPU probe bails and the install exits non-zero, failing "the ComfyUI install succeeds". The torch-unchanged and nvidia steps back that up. I did not run it: it needs a GPU host and a real install. The description cites run #427 reproducing the defect pre-#298.
Each of the four Then steps guards a distinct branch. Bail-out goes to the exit code, an empty requirement list to growth, a torch-stack swap to the version comparison, CUDA wheels to the nvidia check. No single mutation leaves all of them green.

The last MI300X workflow_dispatch run on this branch, at 7230157, succeeded. The scenario's step logic is unchanged since then apart from deleting a Windows interpreter arm and renaming one step's text. I did not open that run's log to confirm the scenario actually executed in it.

Did not run:

The scenario itself: it needs a GPU, a real SDK install and a real ComfyUI install, which are out of bounds on this host. It also never runs in PR CI, being @nightly.
The EAI-8071 and EAI-8051 tickets: no tracker tool was available.

The design-review step was folded into the three workers' cross-file questions rather than run as separate workers. Fan-out ran as three independent workers: one on the code group, one on agent instructions and code comments, one on history and prior changes.

Not reconciled against the PR's discussion.

Since the previous round (`d44d784`)

The docs/testing.md -n blocker is resolved, so this automation's change request is lifted. The blind review above did not re-find it. It was standing, and was settled by putting it to the reviewer after its report was frozen (anchored evidence), then checked by the driver against the d44d784..c965e4e diff. The -n comfyui-14 command is gone, the only command shown is E2E_INCLUDE_NIGHTLY=1 cargo xtask e2e, and the text now warns that -n bypasses every gate. The name_filter route above is the one part of that concern still open, and it is non-blocking.
The previous non-blocking item is addressed. The comfyui-05..13 command now notes that -n skips the @requires-os:linux gate.
The description item is resolved. The PR body now lists the docs/testing.md change. Commit c965e4e's message matches its diff and carries a DCO sign-off.
The E2E tests item is resolved. All reported checks pass at c965e4e. "Sphinx docs build (-W)" and "Skill checks (skillscope)" did not report.
The author's reply of 2026-10-13 was read and acknowledged.
Reviews written by people on this PR are untouched. That includes the hand-written change request already on this account and siloteemu's change request.

Resolved at c965e4e: docs/testing.md no longer shows a -n command for comfyui-14 and warns that -n bypasses every gate; see review 5508660439.

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi · 2026-10-13T19:31:50Z

#291 (review)

Pushed 2218e5b: the docs/testing.md warning now covers -n/--name and the self-hosted dispatch name_filter input, both of which bypass comfyui-14's gates. The Co-Authored-By trailer on 77498ae should be dropped from the squash message at merge; I am not rewriting history. Nightly time budget on slower families is unverified (only seen on MI300X); left as a maintainer call.

rominf

🔴 Automated review · pr-review-watcher · 2218e5b

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Round history: the previous round (c965e4e) raised two non-blocking items. The docs/testing.md warning now covers -n/--name and the name_filter dispatch input (2218e5b); this round read that file in full and raised nothing on it. The Co-Authored-By: Claude trailer is still on 77498ae; the author has said it will be dropped from the squash message at merge. The failing "E2E tests" check (chat-06, serve-01, serve-02) fails identically on main's tip ee3131be, so it does not bear on this change.

Review — no blocking findings

Full review of the whole change.
Implements the EAI-8051 half of EAI-8071 (one GPU-only nightly e2e scenario, comfyui-14), not the EAI-8053 half. The description says so and points to #305's unit tests for that half. The scenario differs from the ticket's suggested shape in two ways, and the description and the feature comment justify both. First, it is an expected pass rather than an xfail, because #298 is already on the base. Second, it is tagged @nightly rather than @lifecycle, because no GPU lane sets E2E_INCLUDE_LIFECYCLE. The title cites EAI-8071 but does not claim to close it.

Blocking

None

Non-blocking

None

Decisions for the author

This guard only runs on advisory lanes (tradeoff)
- Only the nightly lanes that set E2E_INCLUDE_NIGHTLY reach comfyui-14. Those are MI300X, R9700, MI350P and Strix Halo Linux in .github/workflows/nightly.yml.
- All four lanes are continue-on-error: true. Recent nightly runs are already red or cancelled for unrelated reasons.
- So a regression of #298 would surface as one more red row on a lane that is already red and advisory.
- The other side: the scenario needs a real multi-GiB SDK install that mutates the runtime, and that cannot go on a blocking per-PR lane.
- The description states this openly. Confirm that an advisory-only guard is the level of protection you intend.
A cheap per-PR check of the constraint wiring is still missing (non-blocking-improvement)
- The existing unit tests in apps/rocm/src/comfyui.rs cover the pure helpers only. For example, torch_constraint_lines_pin_present_stack_in_stable_order and uv_install_args_include_constraint_flag_when_pinned.
- Nothing checks that install() actually passes --constraint <file> to uv. The mock fixture's fake python prints {}, so the constraint list is always empty on the mock lane.
- A mock-lane scenario could close this gap and run on every PR. It would use a fake python that reports a ROCm torch and a recording uv shim via ROCM_CLI_UV_BINARY. That check would be complementary: it is not a substitute for comfyui-14, which is the only thing that exercises real uv resolution.
The nvidia-* match is a bare prefix (tradeoff)
- nvidia_only (tests/e2e-cucumber/tests/e2e/comfyui_steps.rs) treats any nvidia-* distribution as a CUDA leak. That includes pure-Python ones such as nvidia-ml-py.
- The broad match catches every CUDA family without upkeep. The cost is that a future upstream ComfyUI dependency on a harmless nvidia-* package would turn the scenario red (or fail the Given step's premise) on a healthy runtime.
- No current dependency was confirmed to pull one in.

Positive signals

assert_dependencies_installed requires the runtime's package set to have grown. That closes the if !packages.is_empty() vacuity path in comfyui::install, where an install that does nothing still exits 0 and every preservation check would pass. It checks for growth rather than naming a dependency, so it won't rot as upstream requirements drift.
DISTRIBUTIONS_PROBE reports {error} instead of returning an empty list, and the Rust side panics on that. A broken probe therefore cannot read as "no nvidia packages".
The step deliberately skips use_shared_runtimes(). The runtime lands under the per-scenario TempDir, which teardown removes.

Deployment notes

On nightly vLLM-family hosts (e.g. MI300X), the Given step runs a real install sdk --yes. That includes system-package consent, a sudo OpenMPI install when it is missing, and a vLLM engine install, all of which add to the nightly lane's time budget.
Neither install sdk nor comfyui install has a per-step timeout. This matches the existing runtime-01 pattern, so a hung resolve runs until the job cap.

What this covered

What was read: every changed file, completely (docs/testing.md, tests/e2e-cucumber/features/comfyui.feature, tests/e2e-cucumber/tests/e2e.rs, tests/e2e-cucumber/tests/e2e/comfyui_steps.rs). Each code-comment and doc claim was checked against the product code:
- apps/rocm/src/comfyui.rs: the empty-list guard, the --constraint pin, the GPU-probe bail-out and the early bail-outs.
- apps/rocm/src/runtimes_cmd.rs: the install_root: line.
- The runtime python path, isolate_env, the tag gating in tests/e2e-cucumber/src/expectation.rs, -n bypassing filter_run, and name_filter forwarding --name in .github/workflows/e2e-selfhosted.yml.
- The base is ee3131be…, the head 2218e5b8….
Checks run:
- cargo test -p e2e-cucumber --test feature_naming passed 4/4, including the sequential-index and unique-index checks.
- cargo clippy -p e2e-cucumber --test e2e -- -D warnings exited 0.
- Both Python probes, rebuilt from the Rust literals, produced valid JSON on Python 3.13.
CI: every check passed except "E2E tests". That failure is not attributable to this change: the base tip's own run (job 115399132811) fails the same three scenarios (chat-06, serve-01, serve-02) with the same "199 scenarios (196 passed, 3 failed)". No self-hosted GPU lane ran for this head.
Verdicts for the new test steps:
- "the ComfyUI install succeeds": with #298 reverted, uv resolves a CUDA torch, the post-install GPU probe bails, and the step goes red. That was traced through comfyui::install, not run.
- "torch stack unchanged" and "no nvidia-* added": these never run on a revert, because the step before them fails. On their own they catch any torch-stack swap and any nvidia-* addition.
- "dependencies were installed": stays green on a revert, by design. It guards only against an install that did nothing.
- What any single-branch break would leave green: packages outside the three pinned torch packages and the nvidia-* names being upgraded by --upgrade. That is outside the scenario's stated contract.
Not run, and why:
- comfyui-14 itself was not executed: it needs a bare-metal Linux AMD GPU host with the nightly opt-in. So no finding here rests on a run of the scenario.
- Prior-changes pass: limited to commit history and conventions, because reading PR discussion is out of bounds.
Discussion: this report was not reconciled against the PR's comments or earlier reviews.

tomastola

Re-reviewed at 2218e5b8. The torch-version-equality check I asked for is in and resolves my earlier thread. The exit-0 premise closes the vacuous-pass paths. CI is green on this head. No blocking findings.

Non-blocking suggestions:

The -n/name_filter warning in docs/testing.md omits that --tags also bypasses the applicability gates, and that these filters skip the @nightly opt-in too.
Consider a --reinstall case, since a reinstall re-runs the constraint step.

nowycondro

🔴 Automated review · pr-review-watcher · 2218e5b

Review — no blocking findings

Read the full diff against main (merge-base ee3131be): the new comfyui-14 Gherkin scenario and its step definitions (comfyui_steps.rs), the two new E2eWorld baseline fields in tests/e2e.rs, and the docs/testing.md paragraph documenting how to run it. This is a test-only addition (no product code touched), and I traced its logic against the actual CLI behavior it exercises (apps/rocm/src/comfyui.rs, runtimes_cmd.rs, main.rs, the applicability-resolver tests in expectation.rs) rather than taking the extensive doc comments at face value. Everything I checked held up: the packages.is_empty() short-circuit the comments warn about is real, the <install_root>/bin/python venv path convention matches how the rest of the suite locates it, single_ready_runtime_key really does auto-select the sole ready runtime, the --requires-bare-metal/--requires-os:linux/--nightly tag combination is exercised by existing resolver unit tests, and the claim that -n/--name bypasses the custom applicability filter is confirmed by the harness's own main() comment. The description accurately scopes the change to these four files.

Blocking

None

Non-blocking

None

Decisions for the author

None — the design choices called out in the scenario's own comments (comparing the whole torch/torchvision/torchaudio triplet rather than a single package, asserting the package set grew rather than naming a specific dependency, probing via importlib.metadata instead of pip list) are all sound tradeoffs already justified in-line, and I did not find a better alternative worth raising.

Positive signals

The scenario is deliberately defended against the two vacuity traps that would make a destructive GPU test pass "green" without exercising anything: it asserts the install exits 0 and that the runtime's package set actually grew, so an early bail-out or an empty filtered dependency list can't read as success.
nvidia_distributions/installed_distributions fail loudly (panic) rather than silently treating a broken probe (e.g. no pip in a uv-managed venv) as "zero packages, check passes" — a real, non-obvious failure mode for this kind of assertion.
The feature file and step module both carry detailed rationale comments (why @lifecycle isn't used, why WSL2 is excluded, why the runtime must stay unshared/isolated) that will save the next person real investigation time.

Deployment notes

All CI checks on this PR are green, including the gated self-hosted/nightly jobs and the Read the Docs build. The new scenario only runs under E2E_INCLUDE_NIGHTLY=1 on a bare-metal Linux AMD GPU host, so it did not execute in this PR's own CI run (per the PR description, that lane picks it up going forward); nothing here is merge-blocking as a result since it's purely additive test coverage with no product-code or default-lane impact.

What this covered

Read in full: docs/testing.md's added paragraph, tests/e2e-cucumber/features/comfyui.feature's new scenario and its comment block, tests/e2e-cucumber/tests/e2e.rs's two new E2eWorld fields, and all ~260 new lines of tests/e2e-cucumber/tests/e2e/comfyui_steps.rs, diffed between merge-base ee3131be347c107fe3ae1e22bb8e877b3d328208 and head 2218e5b855001ff0a71abd5abc92e47bf3d0e48b. Cross-checked several factual claims made in the new code's comments against the actual product source (apps/rocm/src/comfyui.rs, runtimes_cmd.rs, main.rs) and the applicability-resolver's own test suite (expectation.rs), plus the CI workflow files that gate this scenario.

Did not run: no Rust toolchain or network access in this sandbox, so I did not run cargo test, cargo clippy, or the e2e suite itself (including this nightly-gated scenario, which also requires real GPU hardware I don't have here) — correctness was established by static tracing against the current source rather than execution. I did not reconcile this against the extensive existing human review discussion on this PR (28 prior review entries, ending in an approval); this is an independent blind pass and did not duplicate or refute any of that discussion.

fredespi added 3 commits August 20, 2026 12:16

Merge remote-tracking branch 'origin/main' into e2e-pin-the-contracts…

03dea77

…-for-eai-8051-and-eai-8053 Signed-off-by: fredespi <fredrik.espinoza@gmail.com> # Conflicts: # tests/e2e-cucumber/expectations.toml

fredespi requested a review from a team as a code owner August 20, 2026 10:53

fredespi requested a review from r0x0r August 20, 2026 10:53

fredespi added 5 commits August 20, 2026 13:36

tomastola reviewed Aug 21, 2026

View reviewed changes

Comment thread tests/e2e-cucumber/tests/e2e/comfyui_steps.rs Outdated

fredespi mentioned this pull request Aug 22, 2026

fix(comfyui): pin managed torch stack during install (EAI-8051) #298

Merged

2 of 3 tasks

volen-silo mentioned this pull request Aug 24, 2026

fix(install): stop requiring sudo when the driver install is already root #305

Merged

1 task done

fredespi added 3 commits August 24, 2026 19:05

Merge remote-tracking branch 'origin/main' into e2e-pin-the-contracts…

b769da9

…-for-eai-8051-and-eai-8053 Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

Merge remote-tracking branch 'origin/main' into e2e-pin-the-contracts…

459d87f

…-for-eai-8051-and-eai-8053 Signed-off-by: fredespi <fredrik.espinoza@gmail.com> # Conflicts: # tests/e2e-cucumber/tests/e2e.rs

rominf requested changes Sep 2, 2026

View reviewed changes

Comment thread tests/e2e-cucumber/features/driver.feature Outdated

Comment thread tests/e2e-cucumber/tests/e2e/driver_steps.rs Outdated

Comment thread tests/e2e-cucumber/tests/e2e/comfyui_steps.rs Outdated

Comment thread tests/e2e-cucumber/src/capability.rs Outdated

siloteemu previously requested changes Sep 9, 2026

View reviewed changes

fredespi changed the title ~~test(e2e): pin the driver-as-root and comfyui-runtime contracts (EAI-8071)~~ test(e2e): pin the ComfyUI runtime-preservation contract (EAI-8071) Sep 10, 2026

rominf reviewed Oct 13, 2026

View reviewed changes

test(e2e): say comfyui-05 does the same host-level work as runtime-01

d0388f8

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi requested a review from rominf October 13, 2026 09:55

rominf reviewed Oct 13, 2026

View reviewed changes

fredespi requested a review from a team October 13, 2026 16:01

Merge origin/main into e2e-pin-the-contracts-for-eai-8051-and-eai-8053

390b7cb

Main added comfyui-05..13 (reinstall scenarios); the runtime-preservation scenario is now comfyui-14. Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

rominf reviewed Oct 13, 2026

View reviewed changes

test(e2e): correct the torch-pin doc comment and the WSL2 gate rationale

f3f4868

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi requested a review from rominf October 13, 2026 17:01

rominf reviewed Oct 13, 2026

View reviewed changes

docs(e2e): document comfyui-14 and untangle its Windows/WSL2 gate com…

d44d784

…ment Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi requested a review from rominf October 13, 2026 17:51

rominf previously requested changes Oct 13, 2026

View reviewed changes

docs(e2e): stop recommending -n for comfyui-14; note -n skips the OS …

c965e4e

…gate Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi requested a review from rominf October 13, 2026 18:29

rominf reviewed Oct 13, 2026

View reviewed changes

docs(e2e): warn that name_filter bypasses comfyui-14's gates too

2218e5b

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

fredespi requested a review from rominf October 13, 2026 19:31

rominf reviewed Oct 13, 2026

View reviewed changes

tomastola approved these changes Oct 15, 2026

View reviewed changes

nowycondro reviewed Oct 27, 2026

View reviewed changes

Conversation

fredespi commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Scenario

Lane

Test plan

Uh oh!

Uh oh!

tomastola commented Aug 21, 2026

Uh oh!

michaelroy-amd commented Aug 26, 2026

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

fredespi commented Sep 10, 2026

Uh oh!

siloteemu commented Sep 10, 2026

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

fredespi commented Sep 10, 2026

Uh oh!

siloteemu commented Sep 10, 2026

Summary

🚫 Blocking (must fix before merge)

Verification of the new step

Declines — all four reasonable

Non-blocking

Uh oh!

rominf commented Oct 2, 2026

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

fredespi commented Oct 12, 2026

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Review — no blocking findings

Blocking

Non-blocking

Decisions for the author

Positive signals

Deployment notes

What this covered

Since the previous round (13c23a8)

Uh oh!

fredespi commented Oct 13, 2026

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Review — no blocking findings

Blocking

Non-blocking

Decisions for the author

Positive signals

Deployment notes

What this covered

Since the previous round (77498ae)

Uh oh!

fredespi commented Oct 13, 2026

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Review — needs work

Blocking

Non-blocking

fredespi commented Aug 20, 2026 •

edited

Loading

Since the previous round (`13c23a8`)

Since the previous round (`77498ae`)

Since the previous round (`d0388f8`)

Earlier round's blocker (`390b7cb`) — settled, no longer blocking

Since the previous round (`f3f4868`)

Since the previous round (`d44d784`)