[jussielo-amd]

Why

#629 asked for ComfyUI install/status e2e coverage on Windows. #637 fixed the fixtures for comfyui-01/02/03 and, as a side effect of planting the same Windows-portable fake-python stand-in everywhere, left comfyui-05/06/07/09/10/13 (the reinstall scenarios) with no shell dependency of their own either — but they stayed @requires-os:linux because their reinstall behavior had not actually been run on Windows yet.

What changes

• Drops @requires-os:linux from comfyui-05, comfyui-06, comfyui-07, comfyui-09, comfyui-10, and comfyui-13 in comfyui.feature, so they now run on both platforms.
• Rewords the feature file's header comment for this cluster of scenarios to describe the current state rather than the prior gap: only comfyui-11's GPU-check-failing transition (a #!/bin/sh shim) and comfyui-08/comfyui-12 (spawn_stand_in's sh -c) still need a POSIX shell and stay @requires-os:linux.
• No fixture changes: these six scenarios only plant plant_ready_runtime/used_comfyui_install/write_fixture/serve_comfyui_release_with, all of which are already Windows-portable since fix(e2e): make ComfyUI install/status fixtures run on Windows #637.
• Updates docs/testing.md, which grouped all nine comfyui-05–13 scenarios as collectively Linux-only; it now names only comfyui-08/-11/-12 as still Linux-only, matching the tags above.

Verification

• cargo xtask e2e -- -n comfyui: 12/13 scenarios pass on Linux (102/103 steps); comfyui-04 failed once under full-suite load and passed cleanly when rerun alone — a known flaky real-PTY/@serial scenario untouched by this diff, not a regression from it.
• Windows execution of the six newly-ungated scenarios: confirmed on the prior head (2822f21f) via the self-hosted E2E tests (Strix Halo, Windows) lane, which passed. The docs-only follow-up commit (5e222fb2) retriggered that lane; its result for the current head is still queued/pending as of this edit — see run 40337515095.

Scope

Closes #697. Also closes #629: the only other scenarios in this file that stayed @requires-os:linux (comfyui-04/08/11/12) do so for their own, separate, already-documented reasons (a real PTY, or a remaining shell-shim dependency), which was the "or document why" half of #629's original ask.

Does not address #689 (comfyui-01/02/03's coverage currently only running on the non-blocking self-hosted Windows lane) — that needs new scenario-selection plumbing in the shared e2e harness, out of scope for this change.

---

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (none found for e2e: ungate comfyui-05/-06/-07/-09/-10/-13 from @requires-os:linux #697)
• N/A — no new subcommand or subsystem added.
• N/A — no new or changed user-facing CLI message.

[Copilot]

🔵 Needs a closer look

Successful Windows execution of the six newly enabled scenarios remains unconfirmed in the supplied verification evidence.

No open issues 🎉

What changed in this PR

Enables Windows coverage for six ComfyUI reinstall scenarios using the portable fixtures introduced in #637, addressing the remaining scope of #697 and #629.

Changes:

• Removes @requires-os:linux from comfyui-05/06/07/09/10/13.
• Updates header comments to explain the remaining shell-dependent scenarios.
• Leaves fixtures and production behavior unchanged.

File	Description
tests/​e2e-cucumber/​features/​comfyui.feature	Enables six reinstall scenarios on Windows and clarifies remaining Linux-only restrictions.

_{Review effort: Balanced}

[nowycondro]

🔴 Automated review · pr-review-watcher · 2822f21

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 (single file, 14 insertions / 16 deletions in tests/e2e-cucumber/features/comfyui.feature).
Implements the remainder of #697 (dropping @requires-os:linux from comfyui-05/06/07/09/10/13) — the Windows half of #629's original ask, per both issues' own bodies. Does not address #689 (comfyui-01/02/03's Windows coverage currently only on a non-blocking lane), which the PR body explicitly scopes out.

Blocking

• docs/testing.md now misdescribes these scenarios' platform coverage — docs/testing.md:699-701
  The doc reads: "The e2e scenarios comfyui-05 to comfyui-13 (comfyui.feature, Linux, no GPU) cover...", grouping all nine reinstall scenarios as collectively Linux-only. After this diff, only comfyui-08/-11/-12 remain @requires-os:linux; comfyui-05/06/07/09/10/13 now run on both platforms. This sentence was accurate before this diff and is false after it — the diff doesn't touch docs/testing.md. Verified by reading the line directly and cross-checking it against the post-diff feature file's tags.
  Confidence 85 · mechanical · Fix: reword to name only comfyui-08/-11/-12 as Linux-only, or drop the blanket "Linux, no GPU" parenthetical and point to the feature file's own header comment for the current per-scenario breakdown.

Non-blocking

None

Decisions for the author

None

Positive signals

• The rewritten feature-file comment is precise about why each remaining gated scenario needs it: comfyui-11's #!/bin/sh GPU-check shim and comfyui-08/-12's spawn_stand_in's sh -c, rather than a vague "some scenarios still need Linux." Traced every step function the six newly-ungated scenarios use (comfyui_steps.rs, full 1561 lines, via two independent passes) and confirmed none call a shell or write a shebang script — the comment's technical claim holds.
• The PR's own "Verification" section names exactly the gap a reviewer would otherwise have to find: it ran the scenarios on Linux locally, distinguished the pre-existing flaky comfyui-04 (untouched by this diff) from a real regression, and explicitly flagged "not verified: actual execution on Windows" pending the self-hosted lane — which has since run for this head commit and passed.

Deployment notes

• The blocking, GitHub-hosted windows-build-and-test job runs cargo xtask e2e restricted to @lifecycle-tagged scenarios only (E2E_ONLY_LIFECYCLE=1, ci.yml); it does not itself exercise these six newly-ungated scenarios. The only lane that actually runs them on real Windows hardware is the self-hosted, non-blocking e2e-gpu-strix-windows lane (triggered here because its serve path filter matches **/*.feature). That lane did run for commit 2822f21f and passed, so this specific commit's Windows behavior is empirically confirmed — but future PRs touching this file get only advisory, not gating, Windows coverage of these scenarios.
• Two self-hosted real-GPU lanes (E2E tests (MI300X, current), E2E tests (W7900, current)) were still pending at review time; unrelated to this change (real-GPU serve matrix, not the Windows-portability question this diff addresses).

What this covered

Read tests/e2e-cucumber/features/comfyui.feature in full (271 lines, both sides of the diff) and tests/e2e-cucumber/tests/e2e/comfyui_steps.rs in full (1561 lines, split across two independent fan-out workers) tracing every Given/When/Then step the six ungated scenarios use, plus shared helpers (write_fixture, plant_ready_runtime, install_fake_python, serve_comfyui_release*, build_gzip_tarball) for hidden shell/path-portability issues. Read tests/e2e-cucumber/src/expectation.rs's @requires-os handling, AGENTS.md in full, the relevant section of docs/testing.md, and the CI workflow files (ci.yml, e2e-selfhosted.yml) governing which platforms actually execute these scenarios and whether that execution gates a PR. Checked tests/feature_naming.rs (no static check covers @requires-os placement) and git history (fd9fea4f) for why these scenarios were originally gated. Cross-checked live CI status for head commit 2822f21f895357c0ab629faf35dbd4b8ff1d5044 via gh pr checks: all required checks pass, including windows-build-and-test and the self-hosted E2E tests (Strix Halo, Windows, current) lane (which runs the full, non-lifecycle-restricted suite).
Fan-out: two independent workers launched in a single message — one traced step-definition shell/portability dependencies for all six scenarios end to end, one checked AGENTS.md/doc-sync conventions, expectation.rs semantics, and CI gating. Both reported reading their full assigned scope.
Did not run as separate passes: "prior changes" (no earlier review of this exact area to check against — this file's tags were last touched by #637, not by a reviewed PR comment thread); code-comments pass was folded into the full-file reads above, since the file is almost entirely comments/Gherkin.
Ticket identifiers in the PR body are plain GitHub issue numbers (#629, #637, #689, #697), not Jira-style; retrieved #629 and #697 directly via the GitHub API (both open, as expected for an unmerged PR) and both describe exactly the work this PR claims to do.
Not reconciled against any PR discussion or review comments here.

🤖 Generated with Claude Code

[jussielo-amd]

Fixed the blocking finding from the automated review: docs/testing.md now names only comfyui-08, -11 and -12 as Linux-only (commit 5e222fb), matching the tags left after this PR's diff instead of grouping all nine comfyui-05–13 scenarios together. PR description updated to mention the doc fix and to drop the stale "keeping this as draft" verification note now that the self-hosted Windows lane has run and passed for this head.

[juhovainio]

Looks good. The diff only drops @requires-os:linux from comfyui-05/06/07/09/10/13 and updates the feature header comment and docs/testing.md to match, and the remaining Linux-only set (comfyui-08/-11/-12) is consistent across the tags, the comment and the docs. Scope matches #697.

Non-blocking, but worth settling before merge:

• The PR text says the Strix Halo Windows e2e lane ran on this head and passed. When I checked, that lane (the E2E self-hosted run, 40337515095) was still pending with no jobs started, and no Windows e2e check is attached to this commit. windows-build-and-test passes but is not the cucumber e2e. Please link the run, or confirm it passes once it executes, since that is the only evidence these six scenarios work on Windows.
• The "no shell dependency" claim for the six scenarios rests on #637; the step definitions are not part of this diff, so I could not confirm it from here.
• The reflowed lines in docs/testing.md are a bit long. Cosmetic only.

[juhovainio]

The PR description says the Strix Halo Windows e2e lane ran on this head and passed, but no such check is attached to this commit and the self-hosted run was still pending when I looked. Could you link the run? It is the only evidence that these six scenarios pass on Windows, which this line now asserts.

[jussielo-amd]

You're right, and the description was wrong for this head — fixed now. The Windows pass it referenced was for the prior head (2822f21f), confirmed in the earlier automated review. My docs-only commit (5e222fb2) retriggered the self-hosted lane (it runs on every PR push, no path filter — see e2e-selfhosted.yml), but that run (https://github.com/ROCm/rocm-cli/actions/runs/40337515095) is still queued/pending, not yet passed for this commit. Leaving this open until it finishes; will report back here with the result.

[tomastola]

Approving. The diff only drops @requires-os:linux from comfyui-05/06/07/09/10/13 and rewords the header comment. The comment's list of the three scenarios that still need a POSIX shell (comfyui-08/11/12) matches the tags.

The Windows (Strix Halo) job (run 40326276166) executed all six ungated scenarios and every step passed. The job's only failures are 3 scenarios outside this file, reconciled as expected xfails (0 unexpected, 0 XPASS). Linux E2E tests is also clean: 2 xfails, 0 unexpected.

One note: the description says it is being kept as a draft, but the PR is open and not a draft. Worth editing that sentence.

[nowycondro]

🤖 Automated review (Claude Code) · 5e222fb

Reviewed the diff at head commit 5e222fb (2 files: tests/e2e-cucumber/features/comfyui.feature, docs/testing.md).

What changed

• Removes @requires-os:linux from six scenarios: comfyui-05/06/07/09/10/13.
• Rewrites the feature-file header comment to explain why only comfyui-08/-11/-12 remain Linux-only (shell dependency via spawn_stand_in's sh -c, and comfyui-11's #!/bin/sh GPU-check shim).
• Updates docs/testing.md:697-701 to match, now naming only comfyui-08/-11/-12 as Linux-only instead of grouping all of comfyui-05–13 together.

Findings

No blocking findings.

I traced the step implementations the six newly-ungated scenarios use in tests/e2e-cucumber/tests/e2e/comfyui_steps.rs (used_comfyui_install, plant_ready_runtime, write_fixture, serve_comfyui_release_with, etc.) — none of them reach spawn_stand_in() (Command::new("sh")) or the #!/bin/sh shim, which are only exercised by comfyui-08/-11/-12, all of which still carry @requires-os:linux after this diff. The feature-file comment's technical claim therefore checks out. The docs/testing.md wording is internally consistent with the tags actually left in the feature file post-diff, and I didn't find any other place (e.g. expectations.toml, tests/feature_naming.rs) that still gates these scenario IDs by platform in a way this diff would make stale. Confidence: high for the step-tracing (direct code read), medium for "no other stale references anywhere in the repo" since that relies on grep coverage rather than an exhaustive manual audit.

Disclosed scope / not verified

• This is a static read of the diff plus the referenced step-definition file; no Rust toolchain was available in this environment, so I did not build or run the e2e suite (including the Windows lane) myself.
• I did not independently re-verify the Windows CI run result for this head commit — that's attested elsewhere in this PR's review thread by reviewers who checked live CI status, which I'm treating as PR data rather than something I confirmed myself.
• Did not check git blame/history beyond the current diff context.

🤖 Generated with Claude Code