[jussielo-amd]

Why

The ComfyUI install-path e2e scenarios never ran on Windows — not because the CLI logic they test is Linux-specific (production already resolves .dll/.so names and python.exe/uv.exe paths per host via runtime_is_windows() and friends), but because their test fixtures hardcoded POSIX shell shims. That left only function-level unit tests covering apps/rocm/src/comfyui.rs on Windows, with nothing exercising a real rocm comfyui install/rocm comfyui status run there.

What changes

• comfyui-01, comfyui-02, and comfyui-03's fixtures in tests/e2e-cucumber/tests/e2e/comfyui_steps.rs now plant the uv/Python subprocesses they shim as real compiled executables (fake-uv, fake-python), declared as ordinary [[bin]] targets of this crate — the same convention remote_steps.rs's fake-tailscale stand-in already uses. Their behavior (exit code, stdout line) is configured through env vars rather than generated per scenario. fake-uv panics on an unparseable FAKE_UV_EXIT_CODE instead of silently defaulting; fake-python rejects an argv shape matching neither of the runtime Python's two invocation forms (probe_torch_stack_versions's -c, probe_comfyui's script form) instead of guessing — both failing loudly, the same rule fake-tailscale already follows.
• plant_ready_runtime — the shared Given every comfyui scenario (01 through 13) builds its runtime from — now plants fake-python as that runtime's default Python, instead of the old #!/bin/sh exit 0 stand-in that failed the post-install GPU check. This reaches comfyui-04..13 too, though none of their own behavior changes: used_comfyui_install (comfyui-05..13's shared Given) used to immediately overwrite that planted Python with its own GPU-check shell shim; that overwrite is now dropped as redundant, since fake-python already answers the same post-install probe unconditionally. comfyui-11's GPU-check transition used a dedicated shell-shim helper for both halves (failing, then passing again); the passing-again half now plants fake-python like every other scenario, so only the failing half still needs a real shell shim, and comfyui.feature's header comment for comfyui-05..13 is reworded to say so.
• Planting a fixture binary goes through rocm_core::fake_executable::write_fake_executable rather than std::fs::copy, so it does not reopen test: write fake executables without the ETXTBSY fork race #649's ETXTBSY race: a plain copy holds a write fd on the destination for the duration of the copy, which is exactly the window a concurrent scenario's spawn can inherit. That helper needs the rocm-core dev-dependency, which this crate's library target cannot reach, so install_fixture_binary() now lives in tests/e2e.rs (the test binary that can) rather than alongside the pure fixture_binary()/fixture_target_name() lookups in src/fixture_binary.rs.
• A .bat script was tried first and rejected: Rust's CVE-2024-24576 mitigation rejects any argument containing a newline once the target resolves to a batch file, and the pre-install torch-stack probe (apps/rocm/src/comfyui.rs) passes its probe script as exactly such an argument — a real compiled executable has no such restriction.
• The planted rocm_sdk library stubs use a small fixture-local rocm_library_filename() helper instead of importing rocm_core::runtime_rocm_library_filename(): readiness only checks path.is_file() on whatever the manifest lists (probe_has_resolved_library in apps/rocm/src/therock.rs), never the filename, so the import would buy nothing while breaking this suite's black-box-only rule (no imports from the product crates — see tests/e2e-cucumber/README.md's Design Principles).
• The [[bin]]-locating, suffix-naming, and plant logic is shared between this and remote_steps.rs's fake-tailscale stand-in, which previously duplicated the same lookup-and-copy body privately. tests/e2e-cucumber/README.md documents the [[bin]] fixture pattern for the next stand-in.
• @requires-os:linux is dropped from comfyui-01/comfyui-02/comfyui-03 in comfyui.feature, since their fixtures no longer need it.
• comfyui-04 keeps @requires-os:linux: it drives rocm under a real PTY, the same pre-existing limitation dash.feature's header comment already documents (portable-pty compiles on Windows via ConPTY, but that path isn't yet promoted to a blocking contract) — unrelated to the fixture changes here, now cross-referenced instead of left unexplained.

Verification

• cargo clippy -p e2e-cucumber --test e2e --bins -- -D warnings and cargo check -p e2e-cucumber --test e2e: clean.
• cargo xtask e2e -- -n comfyui: all 13 scenarios, 105/105 steps pass on Linux (comfyui-04..13 unaffected in behavior by the plant_ready_runtime/used_comfyui_install change above).
• cargo xtask e2e -- -n remote: the 8 non-Docker scenarios (which exercise the shared fixture-plant path via fake-tailscale) pass; the @requires-docker scenarios are unaffected by this change.
• E2E tests (Strix Halo, Windows): comfyui-01, comfyui-02, and comfyui-03 all pass there (141 scenarios, 138 passed + 3 expected xfails unrelated to this PR).

Scope

No production code changes — apps/rocm/src/comfyui.rs and crates/rocm-core already resolve platform-correct paths; only the test fixtures needed to catch up. The other e2e files with the same unix-only shim-writing pattern (dependency_guard_steps.rs, diagnose_steps.rs, engines_steps.rs) are untouched — their scenarios are @requires-os:linux for unrelated reasons (apt/dpkg, D-Bus Secret Service, a bash-only prompt marker), not this fixture limitation.

#629 names comfyui-01, comfyui-02, comfyui-ambiguous-runtime-actionable (comfyui-03) "and others in the file". This PR ungates those three. comfyui-05/-06/-07/-09/-10/-13 also lose their shell dependency as a side effect of the plant_ready_runtime change above, but ungating them needs their reinstall behavior actually run on Windows first, which this PR doesn't do — tracked separately in #697 to keep this change scoped to the fixture fix. comfyui-04, -08, -11, -12 keep @requires-os:linux for their own, unrelated reasons (a real PTY, or a remaining sh/shell-shim dependency), documented in comfyui.feature's header comments.

---

• 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: ComfyUI install/status shims (uv, Python, rocm_sdk) are Linux-only, so Windows has no e2e coverage of the actual CLI run #629)
• N/A — no new subcommand or subsystem added.
• N/A — no new or changed user-facing CLI message.

Part of #629 (remaining scope tracked in #697)

[Copilot]

🟡 Changes recommended

Two generated Windows shim programs contain invalid Rust format strings and fail during fixture setup.

1 open finding

• Escape empty JSON format braces in generated Windows shim · New

What changed in this PR

Makes ComfyUI install/status E2E fixtures portable to Windows.

Changes:

• Adds native Windows subprocess shims.
• Uses platform-specific ROCm library names.
• Enables three scenarios on Windows while retaining the PTY scenario’s Linux restriction.

File	Description
comfyui_steps.rs	Adds cross-platform executable and library fixtures.
comfyui.feature	Removes obsolete Linux-only tags and documents PTY limitations.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The portable fixture implementation is coherent and only a non-blocking documentation typo remains.

1 open finding

• Remove duplicated “Both” · New

1 resolved since last review

• Escape empty JSON format braces in generated Windows shim

🧠 Review effort: Balanced

[rominf]

🔴 Automated review · pr-review-watcher · 2ec2b2c

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 #629 for comfyui-01, comfyui-02 and comfyui-03, which lose @requires-os:linux. comfyui-04 stays Linux-only, and the feature file now gives the reason, which is the issue's own fallback for scenarios that stay gated.

First automated round on this PR: every finding below is new. This review requests changes for the three blocking items. None of the self-hosted lanes is a required check on main right now, so nothing would stop a merge before the Windows run happens. For the description finding, the gate rests on the commit message: rocm-cli builds the squash body from commit messages, so 4d3cd74's claims land on main. Correcting the PR description alone would not clear it.

Blocking

• No run has exercised the newly ungated scenarios on Windows at this head — tests/e2e-cucumber/features/comfyui.feature:14, :34, :63
  The only point of this change is that comfyui-01, -02 and -03 pass on Windows, and nothing has run them there yet:

  • At head 2ec2b2c4, the E2E self-hosted workflow run (created 2026-10-12T12:20:41Z) is still pending with no check-runs. That workflow holds the Strix Halo Windows lane.
  • The hosted E2E tests job runs on ubuntu-latest.
  • windows-build-and-test skips the e2e target, which is test = false in tests/e2e-cucumber/Cargo.toml.
  • The PR description says the same: "Not verified: an actual Windows run of these scenarios."
  • Fixes #629 will close the issue on merge without any proof the scenarios pass on Windows.

  Confidence 100 · mechanical · Fix: get the stalled self-hosted run going again (cancel it and re-run), and merge only once the Strix Windows lane reports comfyui-01, -02 and -03 passing.

• The PR description misstates what the fixtures do — the PR description's first "What changes" bullet, against tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:99-150

  • The description says the fixtures "plant a platform-appropriate stand-in … a POSIX shell script on unix, and on Windows, two real executables (fake-uv, fake-python)".
  • The code does something else. write_shim and write_gpu_probe_shim are deleted, and install_fake_uv and install_fake_python copy the fake-uv and fake-python [[bin]] stand-ins on every platform. No shell shim is left on Linux.
  • A reader trusting the description would assume Linux still runs the old shims, and that the new binaries are proven only by the unrun Windows lane.
  • The squash message will probably carry the same error. Commit 4d3cd74 describes compiling a stand-in with rustc at test time, and says the .so names were among "the only thing keeping these scenarios Linux-only". Neither is true of the final code (see the next finding).

  Confidence 85 · mechanical · Fix: narrow the description to the actual mechanism, Rust stand-ins on all platforms, and write the squash message from the final state. Keep Fixes #629, which currently appears only in 4d3cd74.

• New product-crate import breaks the e2e black-box rule, and the fixture never needed it — tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:60, :172-173, :252-253; tests/e2e-cucumber/Cargo.toml:75-77

  • tests/e2e-cucumber/README.md:244 says: "Black-box only. … No imports from the rocm-cli codebase."
  • The file's own module doc says the same at comfyui_steps.rs:49-50: "Black-box throughout … not typed imports from the product crates".
  • The import buys nothing. Readiness never looks at library filenames. probe_has_resolved_library (apps/rocm/src/therock.rs ~5259) only checks path.is_file() on whatever paths the planted manifest lists, and validate_wheel_runtime_manifest (apps/rocm/src/main.rs:9167-9181) only checks that python_executable exists. So the old libamdhip64.so literals already worked on Windows.
  • runtime_rocm_library_filename has no production caller. Its only other uses are in comfyui.rs's #[cfg(test)] module.
  • The two earlier exceptions each carry a comment justifying them: service_cleanup_steps.rs:312 (FileLock) and dash_steps.rs:1327 (AppPaths). This one has none.
  • The rocm-core dev-dependency comment in Cargo.toml is now further out of date.

  Confidence 85 · mechanical · Fix: plant literal library names and drop the use. If the import is kept on purpose, add a justification comment and update the Cargo.toml comment.

Non-blocking

• The new stand-ins don't follow fake-tailscale's "fail loudly" rule — tests/e2e-cucumber/src/bin/fake-python.rs:24-32, tests/e2e-cucumber/src/bin/fake-uv.rs:17-25

  • fake-tailscale.rs exits 2 on any command a scenario didn't set up. Its comment says failing loudly "beats a silent empty answer".
  • fake-python treats every call that isn't -c as the GPU probe. rocm comfyui start runs python main.py --listen … (apps/rocm/src/comfyui.rs:558). If a future scenario reuses plant_ready_runtime for start, fake-python would write the probe JSON to a file named --listen and exit 0.
  • fake-uv ignores its arguments, and silently turns an unparseable FAKE_UV_EXIT_CODE into exit 0.
  • The module doc says comfyui "shells out to the runtime's Python twice". That holds only for install.
  • No current scenario reaches any of this.

  Confidence 85 · logic · Fix: accept only -c <script>, or exactly two arguments where the first ends in .py. Otherwise print argv to stderr and exit non-zero. Make fake-uv reject an unparseable exit code.

• The suffix and copy logic now exists three times — tests/e2e-cucumber/src/fixture_binary.rs:24-28, tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:102-108 (fixture_target_name), tests/e2e-cucumber/tests/e2e/remote_steps.rs:62-66

  • All three repeat the "Windows adds .exe" logic.
  • install_fake_uv, install_fake_python and plant_tailscale each repeat the same create-parent-directory-then-copy body.
  • A single install_fixture_binary(name, dest) next to fixture_binary, using std::env::consts::EXE_SUFFIX, would replace all of them. That fits the deduplication this change already started.

  Confidence 70 · mechanical · Fix: as above.

• Commit history carries a superseded approach — 4d3cd74, 16f2e6c

  • 4d3cd74 compiled a shim with rustc at test time, 16f2e6c fixed brace escaping in it, and 12c9e89 replaced the whole thing.
  • rocm-cli squash-merges, so this only matters for what the squash message says. See the description finding above.

  Confidence 70 · mechanical · Fix: write the squash message from the final state.

• The README doesn't mention the new fixture pattern — tests/e2e-cucumber/README.md (~235, "Adding a new test")

  • Nothing tells a contributor that a fake subprocess should be a [[bin]] under src/bin/, found with fixture_binary.
  • The shell-shim pattern it replaces is still in three other step files (dependency_guard_steps.rs, engines_steps.rs, diagnose_steps.rs). Their scenarios are Linux-only for product reasons, so leaving them unmigrated is fine.

  Confidence 70 · mechanical · Fix: add one step to the README.

Decisions for the author

• On Windows the planted Python is bin\python3.exe, not the real Scripts\python.exe layout — tradeoff
  • Install uses the manifest's python_executable exactly as recorded. infer_python_env_root (comfyui.rs:1328-1335) accepts both bin and Scripts, ignoring case, so the scenarios pass either way.
  • The cost is that a Windows-only bug in layout inference would go unnoticed.
  • Planting the real layout would close that gap. It needs either a product import (runtime_python_executable_in_env) or a hand-copied Scripts\python.exe literal. The literal keeps the black-box rule.

Positive signals

• Moving the [[bin]] lookup from remote_steps.rs into the shared src/fixture_binary.rs removes duplicate code and gives later stand-ins one place to start.
• fake-python.rs records why a .bat stand-in can't work: Rust's CVE-2024-24576 mitigation rejects the multi-line -c argument. That spares the next person from repeating the attempt.
• comfyui-04's remaining Linux gate now cites the PTY limitation that dash.feature documents, so it no longer reads as unexplained.

Deployment notes

None

What this covered

• What was read: every changed file, completely: tests/e2e-cucumber/{Cargo.toml, features/comfyui.feature, src/bin/fake-python.rs, src/bin/fake-uv.rs, src/fixture_binary.rs, src/lib.rs, tests/e2e/comfyui_steps.rs, tests/e2e/remote_steps.rs}. Against prw-base...2ec2b2c4bcc66372a5197a503b12370493810f65.
• Also read:
  • the production code the stand-ins replace: the Python/uv invocations in apps/rocm/src/comfyui.rs, crates/rocm-core/src/uv.rs, and the runtime readiness code in main.rs, therock.rs and runtime.rs
  • the e2e harness's env handling
  • tests/e2e-cucumber/README.md, docs/testing.md, AGENTS.md
  • the history of the modified regions
  • the PR's CI check rollup
• Test verdicts, reached by reading:
  • Each ungated scenario fails if the production behaviour it covers is reverted: the log-path wording (01), the streamed uv output and folder: line (02), the ambiguous-runtime refusal (03).
  • Each stand-in branch is exercised: fake-uv exit code (01) and stdout line (02); fake-python -c (01/02) and result-path (02/04).
• Did not run:
  • Nothing was compiled or executed: no cargo build/test/check/clippy, because the host disk is below 40 GB. Every claim about behaviour on Windows comes from reading the code, not running it.
  • The prior-changes pass covered commit history only; earlier reviews and comments on this PR were deliberately not read.
• Fan-out: three independent read-only workers (code/contract pass, source-reading passes, design questions). None failed.
• Prompt injection: none found in the checkout.
• Reconciliation: this report has not been checked against the PR's discussion.

[jussielo-amd]

Reply to the automated review at 2026-10-12T17:29:14Z (head 2ec2b2c4). All three blocking items are addressed in 88fc9df4 (squashed, then rebased onto current main), which also folds in the non-blocking items.

Blocking

• No Windows run had exercised the ungated scenarios at that head. True when the review was written — the E2E self-hosted run for 2ec2b2c4 was still queued. It finished afterward: E2E tests (Strix Halo, Windows) completed at 19:26:08Z with comfyui-01/-02/-03 all passing (the run's 3 xfails are unrelated: artifact-prefetch-01, serve-22, a local-server scenario). E2E self-hosted is queued again for the current head (88fc9df4) and will re-confirm it there; the fixture mechanism these scenarios exercise is unchanged by the fixes below (renamed call sites, same behavior).
• PR description misstated the fixture mechanism. Fixed — rewrote the "What changes" section to describe what the code actually does: install_fake_uv/install_fake_python copy the compiled [[bin]] stand-in unconditionally on every platform; no POSIX shell shim remains anywhere. Since this repo's squash-merge setting is COMMIT_MESSAGES, I also squashed the four commits into one with a message written from the final diff, so the inaccurate description in 4d3cd74 (and the brace-escaping fix it needed, since superseded) won't land on main. (The branch also needed a rebase onto current main — unrelated to this finding, done to fix a Test (affected crates) CI failure caused by the branch being 11 commits behind.)
• rocm_core::runtime_rocm_library_filename import broke the black-box rule. Fixed — dropped the import and added a small fixture-local rocm_library_filename() helper in comfyui_steps.rs instead, confirmed against probe_has_resolved_library (apps/rocm/src/therock.rs) that readiness never reads the filename, only path.is_file(). Cargo.toml's rocm-core dev-dependency comment already only claimed FileLock as the reason, so it's accurate again now that this was the only other use.

Non-blocking

• Fail-loud. fake-python now accepts only -c <script> or <probe.py> <result.json> (two args, first ending .py) and exits non-zero on anything else. fake-uv now panics on an unparseable FAKE_UV_EXIT_CODE instead of silently defaulting to 0.
• Triplicated suffix/copy logic. Deduped into fixture_binary::fixture_target_name()/install_fixture_binary(), used by comfyui_steps.rs and remote_steps.rs (fake-tailscale) alike.
• Commit history carried a superseded approach. Addressed by the squash above.
• README didn't mention the [[bin]] fixture pattern. Added a step under "Writing new tests".

Decision for the author

• Planted Windows python is bin\python3.exe, not Scripts\python.exe. Leaving as-is per the review's own framing — infer_python_env_root already accepts both layouts, and closing the gap would need either a product import (defeating the black-box fix above) or a hand-copied literal layout for marginal benefit.

Verified locally (before and after the rebase): cargo check/clippy -p e2e-cucumber --test e2e --bins -- -D warnings clean, cargo xtask e2e -- -n comfyui (4/4 scenarios, 23/23 steps) and cargo xtask e2e -- -n remote (8/8 non-Docker scenarios, confirming fake-tailscale still resolves through the deduped helper) both pass. All 25 hosted CI checks are green on 88fc9df4, including Test (affected crates).

fixed

[rominf]

🔴 Automated review · pr-review-watcher · 88fc9df

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.
Closes #629 by porting comfyui-01, -02 and -03 to compiled stand-ins. comfyui-04 stays Linux-only and now says why, which is the issue's "document it explicitly" option.

Round 2. The commit reviewed last round (2ec2b2c) is no longer on the branch, so this was a full review. It found no new blocking findings, and both non-blocking findings below are new. This is still a change request because one standing objection from the earlier change request still holds (see Blocking). It was not re-found in the blind review. It was settled by putting it, with the author's reply, to the reviewer after its report was done, so that verdict is anchored evidence, not a blind re-find. Of the other two earlier objections, one is resolved and one is downgraded; details at the end.

Blocking

• Standing, not yet verified: no Windows run of comfyui-01, -02 or -03 at this head — tests/e2e-cucumber/features/comfyui.feature, tests/e2e-cucumber/src/bin/fake-python.rs

  • The passing Windows run cited in the reply (run 38248443250, job "E2E tests (Strix Halo, Windows)") was at 2ec2b2c. Its log does show comfyui-01, -02 and -03 passing.
  • Six of the nine changed files differ between that commit and 88fc9df.
  • That includes fake-python.rs. The tested version wrote to argv[2] for any call without -c. The current one exits 1 unless it gets exactly -c <script> or <file>.py <result>. So "same behavior" is not literally true.
  • Reading the Windows call paths suggests the risk is low. The torch-stack probe is -c <script>, and probe_comfyui's script argument still ends in .py after runtime_path_for_windows_child. Reading cannot show that the .exe stand-ins are built and copied correctly under the Windows lane.
  • At 88fc9df, "E2E self-hosted" (run 38362830209) is still pending with no jobs. The run for the old head, 38248443250, is also still queued as a whole, even though its Windows job finished. It may be holding the workflow's concurrency group, so cancelling it may unblock the new run.
  • None of the self-hosted lanes is a required check, so nothing else stops Fixes #629 closing the issue before Windows has run this code.

  Confidence 100 · mechanical · Fix: get the self-hosted run going at this head, or at any commit with an identical tree. The block lifts once the Strix Windows lane reports comfyui-01, -02 and -03 passing. No code change is needed.

Non-blocking

• New: the description's Windows evidence is from a different commit than the one under review — PR description, Verification section

  • The cited run 38248443250 has head_sha 2ec2b2c…, not 88fc9df. The branch has been rebased since.
  • Six of the nine changed files have different blob SHAs at the two commits: fake-python.rs, fake-uv.rs, fixture_binary.rs, comfyui_steps.rs, remote_steps.rs and README.md.
  • The fake-python.rs difference matters most: it changes which calls the stand-in accepts (see Blocking above).

  Confidence 100 · mechanical · Fix: re-cite the Windows lane once it finishes at this head, or say that the cited run predates the rebase.

• New: a comment copied from runtime_rocm_library_filename describes checks this function does not have — tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:122-124

  • The comment says "so the .dll/.so suffix checks are intentionally case-sensitive".
  • That sentence comes verbatim from crates/rocm-core/src/runtime.rs:140-142. There it justifies #[allow(clippy::case_sensitive_file_extension_comparisons)] on the ends_with(".dll")/ends_with(".so") arms.
  • The fixture copy has no suffix checks and no allow; it only calls format!. The code is the reference here, because the comment describes behaviour the function body does not have.

  Confidence 85 · mechanical · Fix: drop the first sentence, and change "Duplicated from" to "A simplified stand-in for".

Decisions for the author

• Finding the stand-ins: walk up from current_exe(), or use cargo's CARGO_BIN_EXE_<name> — non-blocking-improvement
  • Today fixture_binary lives in the lib crate and walks up from current_exe(), skipping deps/. That keeps it shareable and matches the old fake_tailscale_binary.
  • Cargo sets CARGO_BIN_EXE_fake-uv (and the others) at compile time for integration tests, and the step modules under tests/e2e/ belong to the integration test. Using it would remove the directory heuristic and the runtime existence assert.
  • The cost: the lib cannot see those variables, so the test crate would have to pass the path in.

Positive signals

• fake-python rejects any call shape other than the two used by probe_comfyui and the torch-stack probe, and exits 1 instead of guessing. If the CLI ever adds a flag to either call, the scenarios fail loudly instead of passing silently on a stale stand-in.
• The current_exe() locator and the copy-with-panic now exist once, in fixture_binary.rs. remote_steps.rs's private copy was folded in instead of being left as a twin.

Deployment notes

None

What this covered

• What was read: all nine changed files, completely, against 9a8c5a0b…88fc9df4 (prw-base...HEAD).
• Product paths traced:
  • select_runtime, install, probe_comfyui and torch_stack_constraints in apps/rocm/src/comfyui.rs
  • readiness and probe_has_resolved_library in therock.rs
  • ensure_uv_binary and run_uv_logged_command in crates/rocm-core/src/uv.rs
  • runtime_path_for_windows_child
• Runs on Linux, with a target dir local to the checkout: cargo xtask e2e -- -n comfyui ran 4 scenarios.
  • comfyui-01, -02 and -03 passed.
  • comfyui-04 failed, and fails the same way with tests/e2e-cucumber reverted to the base. So it is a local-host issue that predates this PR: under a TTY, the startup runtime-update check spends its 30-second window downloading uv and Python. It is not counted against this PR.
• CI at this head: every completed check is green, including clippy, windows-build-and-test and E2E tests. "E2E self-hosted" is still pending.
• Did not run:
  • no local Windows build or run
  • the prior-changes pass did not query review threads on earlier PRs that touched these files
  • the design review was run by the reviewing context itself, not by independent workers, because the change is small
  • the description was read before the findings were settled; the code and source-reading workers never saw it
• Reconciliation with the PR discussion: the blind report was not reconciled against it. The earlier objections were settled afterwards, as below.

Earlier objections (at 2ec2b2c), settled after the review:

• Windows run: still holds. It is the Blocking item above.
• Commit message: partly holds, now below the bar for a change request.
  • Squashing removed the superseded rustc-shim description.
  • The new message's opening still says the scenarios were Linux-only partly because of "hardcoded … .so library filenames". Readiness only checks is_file() on the paths the manifest lists, and the same message says so four paragraphs later.
  • That leaves one self-contradicting clause in history. Deleting "and .so library filenames" from the opening sentence would fix it. Non-blocking.
• Product-crate import: resolved. comfyui_steps.rs no longer imports rocm_core; a comment mentions it only as provenance.

[rominf]

🔴 Automated review · pr-review-watcher · df9f514

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 #629 (open), taking its first suggested remedy: compiled stand-ins plus platform-correct library names, then dropping @requires-os:linux from comfyui-01/02/03. If the Windows pass holds, the PR covers what the issue asks for.

Round 3. The commit reviewed last round (88fc9df) is no longer on the branch (rebased onto main), so this was a full, blind review. The first blocking finding is new: it comes from the rebase onto #649. The second blocking finding is the standing Windows objection from the last change request, and this review found it again on its own. The standing commit-message item is listed at the end under Non-blocking. Last round's stale-comment finding is fixed in df9f514.

Blocking

• New: the fakes are planted with an in-process write, which brings back the ETXTBSY race that #649 fixed in these same scenarios — tests/e2e-cucumber/src/fixture_binary.rs:48-58 (install_fixture_binary → std::fs::copy), reached from comfyui_steps.rs install_fake_uv / install_fake_python and remote_steps.rs:60; tests/e2e-cucumber/README.md:241-246.

  • What the rule says. The base commit is #649 (2314b2d, "write fake executables without the ETXTBSY fork race"). It moved this file's write_shim onto write_fake_executable, and its doc comment warned that "a write fd another scenario's spawn inherits makes the CLI's first run of the shim fail with Text file busy". docs/testing.md ("Tests that run a fake executable") still states the rule this diff drops: "The same applies to E2E steps: scenarios run concurrently in one process, and the rocm child that runs the shim can be the one refused."
  • Why fs::copy breaks it. fs::copy holds a write fd on the destination inside the test process. That is exactly the window the helper exists to remove; a compiled binary does not avoid it, because the race is about the write fd, not the file type.
  • What I ran. I added a probe next to #649's regression test in crates/rocm-core/src/fake_executable.rs, same harness (16 threads × 40 runs), but planting with std::fs::copy("/bin/true", path). Result: 100 of 640 first runs failed with "Text file busy (os error 26)". #649's own test, using the helper, passed in the same run.
  • Caveat. In the e2e flow the exec happens inside the rocm child milliseconds later, so the window there is narrower than in the probe. I did not observe the race in an e2e run.
  • Two documents now disagree. docs/testing.md governs: its rule is backed by a measured regression test, and the measurement above shows fs::copy falls inside it. The new README step tells the next author to copy binaries into place, so the pattern will spread.
  • Confidence 85 · architectural · Fix: in install_fixture_binary, plant with rocm_core::fake_executable::write_fake_executable(dest, std::fs::read(fixture_binary(name))?). That keeps the portable [[bin]] approach and #649's guarantee, and the helper already sets 0755 on unix. Then make the README step point at it.

• Standing, found again: the description's verification claims a Windows pass on "this PR's head", but the only self-hosted run for df9f5148 has produced no result — PR description, "Verification".

  • What the description says. "The self-hosted E2E self-hosted / E2E tests (Strix Halo, Windows) run against this PR's head confirms comfyui-01, comfyui-02, and comfyui-03 all pass on a real Windows host".
  • What the runs show. actions/runs?head_sha=df9f5148… lists exactly one E2E self-hosted run (38393650291), and it is pending. Its lanes are absent from statusCheckRollup. The cited run is therefore for an earlier head.
  • No other check covers Windows. The hosted windows-build-and-test job ran only 13 scenarios, all lifecycle-10..22. So no check on this commit exercises comfyui-01..03 on Windows, which is the behaviour the PR exists to add. The fixture code changed since that run, because the branch now sits on #649.
  • Confidence 100 · logic · Fix: get the Windows lane to complete on the current head (if it is stalled in the concurrency group, cancel the superseded run), then update the link. Or narrow the claim to the commit it was actually observed on.
  • Round note: the Windows lane did pass comfyui-01, -02 and -03 at 88fc9df (run 38362830209, job "E2E tests (Strix Halo, Windows)", 10:11–10:25 UTC). This commit has a different tree: it is rebased onto a main that changed comfyui.rs, therock.rs and runtime.rs. So that pass does not settle the objection. The run for df9f514 (38393650291) has had no jobs since 11:56 UTC. The likely cause is that the 88fc9df run is still queued: its "E2E tests (MI300X)" job never started, and the run may be holding the workflow's concurrency group. Cancelling 38362830209 should let the new run start. The objection lifts once the Windows lane passes comfyui-01, -02 and -03 at this head, or at any commit with the same tree. The ETXTBSY fix above changes the tree, so in practice that means the head that carries that fix.

Non-blocking

• New: comments credit probe_comfyui with both Python invocation forms; it has one — tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:115-117; PR description ("neither of probe_comfyui's two forms").

  • The -c form is probe_torch_stack_versions (apps/rocm/src/comfyui.rs:1616-1621). probe_comfyui (:1800) only uses <probe.py> <result>. fake-python.rs:21 names them correctly.
  • The same sentence in the description also says both stand-ins reject unrecognised argv. fake-uv ignores argv entirely and only rejects a bad FAKE_UV_EXIT_CODE.
  • Confidence 85 · mechanical · Fix: "both of the runtime Python's invocation forms (probe_torch_stack_versions's -c, probe_comfyui's script form)".

• New: the README step names repo files in bare backticks — tests/e2e-cucumber/README.md:241-246.

  • src/bin/, fixture_binary, fake-uv, fake-python and fake-tailscale are not links.
  • AGENTS.md §5 asks for markdown citations of repo files to be relative links, e.g. [fixture_binary.rs](src/fixture_binary.rs), so lychee catches a move.
  • This README is not one of the exempt files (the root README.md and CONTRIBUTING.md are; tests/e2e-cucumber/README.md is not).
  • Confidence 70 · mechanical · Fix: link src/fixture_binary.rs and the three src/bin/*.rs files.

• Standing: the commit message's opening sentence still contradicts itself — commit 40bd832, first paragraph.

  • It still says the scenarios were Linux-only partly because the fixtures "hardcoded POSIX shell shims and .so library filenames". Later in the same message it says readiness only checks is_file() on the paths the manifest lists.
  • rocm-cli squash-merges, so this message becomes the commit on main.
  • This item was not re-found in the blind review. It was checked against the current commit message after the review. Non-blocking.
  • Fix: delete "and .so library filenames" from the opening sentence.

Decisions for the author

• Compiled [[bin]] stand-ins versus extending the shim helper — tradeoff
  • Compiled bins run unchanged on Windows and fail loudly on unexpected argv.
  • The cost is that their behaviour is configured through process env (FAKE_UV_*), which leaks into the rocm process under test. Nothing else reads those vars today.
  • Shell/batch shims are cheaper per scenario but cannot take the multi-line -c argument on Windows.
  • The current choice is reasonable; just confirm the env channel is intended as the pattern for future stand-ins.

Positive signals

• Moving fake-tailscale's private lookup-and-copy into the shared fixture_binary removes a duplicate rather than adding a third.
• Mutation-checked: making fake-python's -c arm exit 1 fails comfyui-01 and comfyui-02. Making its probe-script arm write nothing fails comfyui-02. Neither arm is decorative.
• fake-python rejects unrecognised argv instead of defaulting, so a future change to the probe's call shape surfaces as a fixture failure, not a false pass.

Deployment notes

None

What this covered

• What was read. Every file in the change, completely (prw-base…df9f5148, merge-base 2314b2d). Also the product paths the fixtures stand in for: the Python/uv invocations and runtime readiness in apps/rocm/src/comfyui.rs, probe_has_resolved_library in therock.rs, and rocm_core::fake_executable. The fan-out was two read-only workers: a code pass and the source-reading passes.
• What I ran.
  • cargo xtask e2e -- -n comfyui on Linux: comfyui-01/02/03 passed.
  • comfyui-04 failed locally, waiting for the progress frame. The screen showed the CLI rejecting this host's PATH python3 (3.13; cp312 required) and downloading a managed Python. That path is unchanged by this PR, and CI's Linux E2E passed comfyui-04.
  • The two fake-python mutations above, and the fs::copy race probe.
  • CI: all reported checks green; E2E self-hosted pending for this head.
• Not run, and why.
  • Prior-changes pass skipped: it would have needed other PRs' review discussion.
  • No Windows build or run was possible locally (no mingw toolchain).
  • Design review (Step 7) was done by the driver, not as independent workers, because the change is small.
• Not reconciled. This report was not reconciled against the PR's comment or review discussion.

[jussielo-amd]

Reply to the automated review at 2026-10-13T16:07:28Z (head df9f5148). All items are addressed in 3e43eea6.

Blocking

• ETXTBSY regression via fs::copy. Fixed — install_fixture_binary no longer calls std::fs::copy. It now reads the compiled [[bin]]'s bytes and plants them through rocm_core::fake_executable::write_fake_executable, the same helper write_shim already uses elsewhere in this file, closing the exact write-fd window the probe measured. That helper needs the rocm-core dev-dependency, which this crate's library target (src/fixture_binary.rs) can't reach, so install_fixture_binary moved to tests/e2e.rs (the test binary that can); fixture_binary.rs keeps only the pure lookup helpers. Both call sites (comfyui_steps.rs's install_fake_uv/install_fake_python, remote_steps.rs's fake-tailscale install) now go through crate::install_fixture_binary.
• No Windows run had exercised the ungated scenarios at df9f5148. The self-hosted Windows run for that head did subsequently complete after the review was posted (comfyui-01/-02/-03 all passing), but that tree is superseded now: fixing the ETXTBSY item above, plus a rebase onto origin/main (below), changed it further. A fresh Windows run against the current head (3e43eea6) is queued on the shared self-hosted runner pool — it's backed up well beyond this PR right now (several dozen runs ahead across the repo), so I don't have a confirmed pass on this exact head yet. Will follow up here with the run link once it lands; not blocking on it to reply to the rest.

Non-blocking

• probe_comfyui doesn't have two forms; the runtime Python does. Fixed — reworded both the install_fake_python doc comment and the PR description to "both of the runtime Python's invocation forms (probe_torch_stack_versions's -c, probe_comfyui's script form)", and corrected the description's claim about which stand-in rejects what (fake-uv panics on a bad FAKE_UV_EXIT_CODE; fake-python rejects an unrecognized argv shape — not both doing both).
• README cited files in bare backticks. Fixed — src/bin/, fixture_binary.rs, fake-uv.rs, fake-python.rs, fake-tailscale.rs, and tests/e2e.rs are now relative markdown links per AGENTS.md §5.
• Commit message's opening sentence self-contradicted on .so filenames. Fixed by squashing the branch into one commit with a corrected message (the "Why" no longer claims the fixtures hardcoded .so library filenames — only the shell shims).

Decision for the author

Confirmed: compiled [[bin]] stand-ins configured through process env is the intended pattern going forward, not a one-off for this PR.

Also found during this round

Rebasing onto origin/main (needed — this branch was 8 commits behind, and main had merged #546's "reinstall" scenarios in the meantime) merged cleanly with zero textual conflicts, but silently dropped write_shim/write_gpu_probe_shim: our diff replaced the region of comfyui_steps.rs those functions lived in (to introduce the compiled-fixture approach for comfyui-01/02/03), while #546 added new call sites for them elsewhere in the same file for the unrelated --reinstall scenarios. Restored both functions unchanged from main — this PR's scope is still only comfyui-01/02/03.

Separately, CI's hosted E2E tests job failed once on this head on an unrelated scenario (engine-shell-marks-the-prompt, in engine_shell.feature — nothing this PR touches). The same job independently failed on a different scenario on main's own CI run for the #546 merge commit, so this looks like existing flakiness rather than a regression; reran the job to confirm.

[rominf]

🔴 Automated review · pr-review-watcher · 3e43eea

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.
Claims Fixes #629. It makes comfyui-01/02/03 portable. The issue's "and others in the file" (comfyui-05..13) stay Linux-only, and the reason the file documents for that is now partly untrue (see Blocking 2).

Round 4. The commit reviewed last round (df9f514) is no longer on the branch, so this was a full, blind review. This round files no change request and lifts the two standing ones. Why:

• Standing ETXTBSY objection: settled, lifted. This round's blind review did not re-find it. It was then put to the reviewer together with the author's reply, and the reviewer judged it no longer holds: install_fixture_binary (tests/e2e.rs) now plants through write_fake_executable, and no std::fs::copy call remains. That verdict is anchored, because the reviewer was shown the objection. The driver also checked the code directly.
• Blocking 1 (Windows evidence): standing, re-found blind, but it does not gate on its own. Nothing has yet shown comfyui-01/02/03 passing on Windows. That is because E2E self-hosted run 38519200539 for this head has been pending since 05:54Z behind a stalled self-hosted queue. A lane that cannot run is missing evidence, not a defect in the change. A human should still see that lane pass before merging.
• Blocking 2 (stale header comment): new, but below the gate. It is a comment that no longer matches the code. The comfyui-05..13 scenarios themselves stay correctly gated. It should be fixed, but on its own it does not justify blocking the merge.
• Last round's non-blocking items: the probe_comfyui wording, the bare README paths and the .so clause in the commit message are all fixed at this head.

Blocking

• New: nothing has yet run the three newly ungated scenarios on Windows, and the only lane that would is not a required check — tests/e2e-cucumber/features/comfyui.feature:14,34,63
  • The PR exists so that comfyui-01/02/03 run on Windows.
  • At 3e43eea, the hosted windows-build-and-test job only runs cargo clippy -p e2e-cucumber --test e2e and unit tests. Its log (run 38519200508, job 115714817051) contains no cucumber scenario at all.
  • The run that would exercise them, E2E self-hosted (Strix Halo Windows), is run 38519200539. It has been pending since 05:54Z.
  • The header of .github/workflows/e2e-selfhosted.yml says these lanes are not in branch protection's required checks. So the PR can merge with the Windows result still missing, or red.
  • The description agrees ("Pending: a fresh self-hosted … Windows run").
  • Confidence 85 · logic · Fix: before merge, get a Strix Windows report for this head showing comfyui-01/02/03 passed (not skipped). A workflow_dispatch with platform=strix-windows and a name_filter would do.
  • Round note: this is the standing Windows objection from earlier rounds; this round's review found it again without being told about it.
• New: the documented reason comfyui-05..13 are Linux-only is now partly false, and the reinstall steps still override the fixture this change made portable — tests/e2e-cucumber/features/comfyui.feature:115-116, tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:748-757, comfyui_steps.rs:1538-1545
  • The header still says "Linux-only because the planted runtime uses .so names and a POSIX-shell Python stand-in". After this change, plant_ready_runtime plants rocm_library_filename(..) names and the compiled fake-python. Neither stated reason is true of the planted runtime any more.
  • The real remaining reasons are in the reinstall steps:
    • write_gpu_probe_shim / write_shim at :750, :1520 and :1535
    • Command::new("sh") at :1179
    • the hard-coded "python3" in runtime_python() and at :756, where plant_ready_runtime now uses fixture_target_name("python3")
  • used_comfyui_install also still overwrites the freshly planted fake-python with the shell shim. Its comment ("Answers the post-install GPU check…") describes something the planted runtime now already does. That contradicts install_fake_python's "one install serves every scenario".
  • #629 offers "document explicitly … why … Linux-only by design" as an acceptable resolution. So this header is the documentation that Fixes #629 relies on for the remaining scenarios.
  • Which side governs: the code. The planted runtime is what actually runs.
  • Confidence 85 · mechanical · Fix: rewrite the header to name the real reasons listed above. Either drop the redundant overwrite at :750, or keep it and say why.

Non-blocking

• New: README step 6 swaps the two helpers' roles — tests/e2e-cucumber/README.md:244-245
  • It says "fixture_binary's fixture_target_name locates and names it". In the code, fixture_binary() locates the built binary (src/fixture_binary.rs:24-37). fixture_target_name only appends .exe on Windows.
  • Confidence 85 · mechanical · Fix: "fixture_binary() locates it and fixture_target_name() names it".
• New: the "black-box" reason given for not importing runtime_rocm_library_filename doesn't hold for this test binary — tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:182-187
  • The comment says importing it "would cost the black-box guarantee". Yet the same test binary already imports rocm_core: this PR's own install_fixture_binary in tests/e2e.rs uses rocm_core::fake_executable, and dash_steps.rs uses AppPaths.
  • The local helper is fine. The stated reason is the inaccurate part: the README "No imports from the rocm-cli codebase" rule is already relaxed for rocm_core.
  • Confidence 70 · mechanical · Fix: justify the helper on its own terms ("only the filename's look matters; readiness checks is_file()"), or drop the black-box clause.
• New: the commit carries an AI co-author trailer — commit 3e43eea message (Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>)
  • AGENTS.md §11: "avoid AI-generated boilerplate footers".
  • Confidence 70 · mechanical · Fix: drop the trailer when squashing.

Decisions for the author

• Use cargo's compile-time path for the stand-ins instead of walking up from current_exe() — non-blocking-improvement
  • tests/e2e.rs is an integration test of the package that declares fake-uv, fake-python and fake-tailscale. Cargo therefore sets CARGO_BIN_EXE_<name> for it.
  • env!("CARGO_BIN_EXE_fake-uv") would replace the deps/ heuristic and the runtime exists() assert in src/fixture_binary.rs.
  • The cost is that the lookup moves into the test binary, next to install_fixture_binary, rather than the library target. The current approach is inherited from fake-tailscale and works.

Positive signals

• fake-python rejects any argv that isn't one of the two forms comfyui.rs actually uses. The old shell shim exited 0 for anything, so the fixture can now catch a change in how the CLI invokes the runtime Python, where before it would have hidden one.
• fake-tailscale now installs through write_fake_executable instead of std::fs::copy, which closes the ETXTBSY window for the remote scenarios as well.

Deployment notes

None

What this covered

• What was read. All 10 changed files, completely, against prw-base 9f2b3f3…3e43eea6. Also the product paths they stand in for:
  • the probe_torch_stack_versions / probe_comfyui argv
  • run_uv_logged_command's environment handling
  • ROCM_CLI_UV_BINARY resolution
  • probe_has_resolved_library
  • write_fake_executable on both OSes
  • every caller of plant_ready_runtime
• Linux runs (cargo xtask e2e, own target/, report.json mtime checked):
  • comfyui-01/02/03 passed (4 scenarios: 3 passed, 1 failed).
  • The comfyui-04 failure is a host artifact, not this change: the same failure reproduces with tests/e2e-cucumber reverted to prw-base. On this host the CLI goes off to fetch uv and Python 3.12 from the network.
• Test verdicts for comfyui-01/02/03:
  • With the fixture change reverted, they still pass on Linux. The behaviour this change adds (running on Windows) is exercised by nothing that has run yet; that is Blocking 1.
  • Mutating fake-uv to drop its FAKE_UV_STDOUT line turned comfyui-02 red. That proves the env vars reach uv through rocm and that this branch is tested.
  • comfyui-01 requires the non-zero exit path. This is reasoned from its assertion, not separately mutated.
  • fake-python's unexpected-argv branch is a fail-loud guard that no scenario reaches.
• Did not run or check:
  • The scenarios on Windows (not possible on this host).
  • A Windows-target cargo check. The hosted Windows clippy of --test e2e passed in CI.
  • Step 7 design questions were answered by the review driver from the code, not by independent workers, because the change is small.
• Passes that did run: prior-changes and history (earlier PRs #328, #333, #397, #421, #546 and #600; no recurring concern applies) and agent-instruction adherence.
• Prompt injection: none found in the checkout.
• Discussion: this report was not reconciled against the PR's discussion, apart from the standing-objection settlement described in the round note.

Both objections re-checked at 3e43eea. ETXTBSY: install_fixture_binary now plants through write_fake_executable, and no std::fs::copy remains. Windows evidence: still missing, but only because the self-hosted lane run for this head is stalled in the queue, which is not a defect in the change; a human should confirm that lane before merge. See review 5514907861.

[jussielo-amd]

Reply to the automated review at 2026-10-14T09:07:09Z (head 3e43eea6). The three fixable items are addressed in 32c07d43 (the branch was rebased onto main since — ca9313e3 — but that rebase changed nothing in this PR's own files; 32c07d43 is ca9313e3 with these fixes amended in).

Blocking

• Stale header comment for comfyui-05..13. Fixed — rewrote comfyui.feature's Linux-only comment to name the real remaining reasons: write_gpu_probe_shim/write_shim's #!/bin/sh scripts, spawn_stand_in's Command::new("sh"), and the literal "python3" path in runtime_python()/used_comfyui_install() (never the Windows-portable fixture_target_name() stand-in). The .so/POSIX-shim claim this replaced is no longer true of comfyui-01/02/03, which is exactly what made it stale.
• No Windows run has exercised comfyui-01/02/03 at this head. Still true, and still not code-fixable from here. E2E self-hosted for 32c07d43 is queued behind the same shared-runner backlog the last few rounds hit (the previous head's run is mid-flight on E2E tests (MI300X) right now). Not blocking a merge per e2e-selfhosted.yml's own header, but I won't claim a Windows pass until that lane reports one at this exact head.

Non-blocking

• README step 6 swapped fixture_binary()/fixture_target_name()'s roles. Fixed — now reads "fixture_binary() locates it and fixture_target_name() names it".
• Black-box justification for the local rocm_library_filename() helper was inaccurate. Fixed — dropped the "would cost the black-box guarantee" claim (this test binary already imports rocm_core elsewhere, e.g. install_fixture_binary in tests/e2e.rs) and reworded to the real justification: only the filename's look matters, since readiness only checks path.is_file().
• AI co-author trailer in the commit message. Fixed — dropped when amending; 32c07d43's message carries no Co-Authored-By line.

Decision for the author

Confirmed again, nothing new to add: compiled [[bin]] stand-ins configured through process env remain the intended pattern.

Verified locally at 32c07d43: cargo check/clippy -p e2e-cucumber --test e2e --bins -- -D warnings and cargo fmt --check -p e2e-cucumber clean, cargo xtask e2e -- -n comfyui (13/13 scenarios, 105/105 steps, including the reinstall scenarios whose header comment changed). All hosted CI checks are green on 32c07d43; E2E self-hosted is queued, will follow up with the Windows run link once it clears the backlog.

[rominf]

🔴 Automated review · pr-review-watcher · 32c07d4

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.
The PR closes #629 ("Fixes #629"). Of the issue's two remedies, it takes the first for comfyui-01/02/03 (portable stand-ins, tag dropped). For comfyui-04..13 it takes the second: the feature file now says why each stays Linux-only.

Round 5. The commit reviewed last round (3e43eea) is no longer on the branch, so this was a full, blind review. This round files no change request; none is standing on this PR. Why:

• Windows evidence (Blocking 1): standing, re-found blind, does not gate on its own. E2E self-hosted run 38559321320 for this head is still pending with zero jobs since 11:49Z, behind the stalled self-hosted queue. A lane that cannot run is missing evidence, not a defect in the change. A human should still see comfyui-01/02/03 pass on Strix Windows before merging.
• Description scope/verification (Blocking 2): new, below the gate. The mismatch is mainly in the PR description, and rocm-cli builds the squash commit from commit messages. The commit message also describes the change as fixture work for comfyui-01/02/03 only, but the reviewer ran all 13 comfyui scenarios and they pass (comfyui-04's failure reproduces on prw-base). So the inaccuracy has no consequence beyond the text. It should still be corrected.
• Last round's stale header comment: header half fixed, overwrite half standing (below the gate). This round's blind review did not re-find it. It was then put to the reviewer with the author's reply. The reviewer judged that the rewritten comfyui.feature header is now accurate, but the redundant overwrite in used_comfyui_install still holds; see the standing item under Blocking. That verdict is anchored, because the reviewer was shown the objection. The driver confirmed the overwrite in the code. It is a redundant test-fixture write with a misleading comment, and the scenarios pass, so it does not gate, the same as last round.
• Last round's non-blocking items: the README helper roles and the AI co-author trailer are fixed. The rocm_library_filename code comment is fixed, but the commit message still gives the old reason ("breaking this suite's black-box-only rule (no imports from the product crates)"), and that message is what lands on main.

Blocking

• New: nothing has run comfyui-01/02/03 on Windows, though that is the whole point of the PR — tests/e2e-cucumber/features/comfyui.feature:14,34,63
  • The hosted windows-build-and-test job ran E2E_INCLUDE_LIFECYCLE=1 E2E_ONLY_LIFECYCLE=1. Its log shows 13 scenarios (13 passed), and all 13 are lifecycle-10..22. No comfyui scenario ran.
  • The only lane that would run them is the self-hosted E2E self-hosted workflow (run 38559321320). For this head it has been pending with zero jobs since 11:49Z.
  • E2E tests (Strix Halo, Windows) is not one of the required checks on main (queried live). Nothing stops this merging, and closing #629, without a single Windows run.
  • I could not run Windows here. From reading the code, the CLI path looks platform-agnostic: validate_wheel_runtime_manifest only checks is_file(), probe_has_resolved_library only checks is_file(), and the -c and <probe.py> <result> forms match what fake-python accepts. But that is a prediction, not a run.
  • The PR body itself lists this as "Pending".
  • Confidence 100 (lane state and job logs read directly) · architectural · Fix: get a Strix Windows run for this head and confirm comfyui-01/02/03 appear as executed, not skipped, in its report. Better still, see the hosted-lane suggestion under Decisions.
  • Round note: this is the standing Windows objection from earlier rounds; this round's review found it again without being told about it.
• New: the PR description misstates its own scope and verification — PR body, What changes and Verification
  • It says only "comfyui-01, comfyui-02, and comfyui-03's fixtures" changed, and that cargo xtask e2e -- -n comfyui ran "all 4 scenarios, 23/23 steps".
  • At this head, comfyui.feature has 13 scenarios. plant_ready_runtime (comfyui_steps.rs:208) is shared with comfyui-04..13. Its default runtime Python changed from #!/bin/sh exit 0 (which fails the GPU check) to fake-python (which passes it).
  • paced_comfyui_source_archive_fixture (comfyui-04) was also edited, to drop its write_gpu_probe_shim call.
  • So ten more scenarios had their fixture behaviour changed without being mentioned, and the stated verification does not cover them.
  • I ran them myself. On HEAD: comfyui-01..13 gave 12 passed. comfyui-04 failed, but the identical failure (a network download of a managed Python 3.12 timing out) reproduces on prw-base, so it comes from this environment, not the PR. On HEAD merged with prw-base: comfyui-01/02/03/05/11 all passed. The risk is settled; the description is not.
  • Confidence 85 · mechanical · Fix: narrow nothing. Update the description to say plant_ready_runtime now gives every comfyui scenario a GPU-check-passing Python by default, and correct the verification line to match this head.
• Standing (from round 4, settled by putting it to the reviewer): used_comfyui_install still overwrites the planted fake-python with the shell shim — tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:745-756
  • used_comfyui_install calls plant_ready_runtime, which now plants fake-python. It then immediately overwrites that file with write_gpu_probe_shim at the literal bin/python3 path.
  • Its comment ("Answers the post-install GPU check install() runs after the swap.") describes what the planted runtime already does. Last round's fix offered two options: drop the overwrite, or keep it and say why. Neither has been done.
  • Because of the overwrite, the header's first reason for comfyui-05..13 being Linux-only (the #!/bin/sh GPU-check stand-in) is self-inflicted for this shared Given, not an independent limitation.
  • comfyui-11 (gpu_check_fails / gpu_check_passes_again) genuinely needs a shell shim, because fake-python has no failing-GPU mode. So the helpers stay; only this one call is in question. Whether dropping it keeps comfyui-05..13 green has not been run.
  • Fix: drop the overwrite in used_comfyui_install, or keep it with a comment that says why.
  • Round note: the header half of last round's objection is fixed, and this half was not re-found blind. The reviewer's verdict on it is anchored evidence, and the driver confirmed the code. It does not gate (see the round note).

Non-blocking

• New: commit message has a garbled sentence — commit 32c07d4
  • "...rather than std::fs::copy, so it does not reopen / the duration of the copy..." is missing a line. The PR body has the intact text: "#649's ETXTBSY race: a plain copy holds a write fd on the destination for".
  • Confidence 85 · mechanical · Fix: reword before merge, or make sure the squash message is taken from the PR body.

Decisions for the author

• Gate these scenarios on the required hosted Windows lane, not only on the optional Strix one — non-blocking-improvement
  • comfyui-01/02/03 are hermetic: no GPU, no network. They could run in the required windows-build-and-test job, which today runs only lifecycle scenarios.
  • As things stand, Windows coverage depends on a non-required self-hosted lane that only runs when changes.serve is true and is currently stalled. Moving them would make #629's fix enforced rather than incidental.
• Use env!("CARGO_BIN_EXE_<name>") instead of walking up from current_exe() — non-blocking-improvement
  • Cargo exposes the exact path of each [[bin]] to integration tests at compile time. install_fixture_binary already lives in tests/e2e.rs, so it could use that.
  • A rename would then fail to compile, instead of panicking at runtime with "was not built at…", and the src/fixture_binary.rs locator in the library target would no longer be needed.
  • The current approach keeps the lookup in the lib and matches the pre-existing fake-tailscale code, which is also a valid choice.
• Two copies of the runtime Python path — tradeoff
  • runtime_python() (comfyui_steps.rs:1537) still hard-codes bin/python3. plant_ready_runtime now uses fixture_target_name("python3").
  • On Linux they agree. Ungating any reinstall scenario on Windows later would write the shim to a different file than the one the manifest names.
  • The feature-file comment acknowledges this. Confirm it is deliberate rather than a pending unification.

Positive signals

• Each fixture branch is load-bearing. I broke them one at a time on the merged tree:
  • fake-uv ignoring FAKE_UV_EXIT_CODE → comfyui-01 fails.
  • fake-uv ignoring FAKE_UV_STDOUT → comfyui-02 fails.
  • fake-python failing the -c probe → comfyui-01 and -02 fail.
  • fake-python writing an empty GPU result → comfyui-02 fails.
  • comfyui-03 is unaffected by all of these, as expected: it tests runtime selection only.
• fake-python rejects any argv it doesn't recognise rather than guessing, so a future third Python invocation in comfyui.rs fails loudly instead of passing silently.
• fake-tailscale is now planted through write_fake_executable instead of std::fs::copy, which closes the ETXTBSY window on that path too.

Deployment notes

None

What this covered

• Read: every file in prw-base...HEAD (merge-base b8c4b3e…32c07d43), completely. Also the CLI code paths the fixtures stand in for: comfyui.rs (install, probe_torch_stack_versions, run_uv_logged_command, probe_comfyui, apply_runtime_environment), runtimes_cmd.rs (validate_wheel_runtime_manifest), therock.rs (probe_has_resolved_library, the Python resolver), rocm-core fake_executable, the self-hosted workflow gating, and the required-check list.
• Ran:
  • cargo xtask e2e for comfyui-01..03 (passed) and for all 13 comfyui scenarios on HEAD (12 passed; comfyui-04 failed locally, and also fails on prw-base).
  • comfyui-01/02/03/05/11 on HEAD merged with prw-base: no conflicts, all passed.
  • Four fixture mutations, as listed under Positive signals.
• Test verdicts:
  • comfyui-01/02/03 with the change reverted: they would still pass on Linux. Their new value is Windows execution, which no check has exercised (first blocking finding).
  • Breaking one branch at a time: every fake-uv/fake-python branch turns a scenario red, except the deliberate reject-unknown-argv branch.
• CI: all hosted checks green for 32c07d4. No self-hosted lane (Strix Windows, Strix Ubuntu, WSL2, GPU) has reported; the E2E self-hosted run is pending with zero jobs.
• Did not run:
  • Windows execution: there is no Windows host here, and the mingw cross-check was not needed because the hosted Windows clippy for --test e2e is green.
  • The prior-changes pass: earlier PRs' review comments were not consulted.
  • Fan-out: single-context review. The change is small enough for one pass, and the design questions were also investigated in this context rather than by independent workers.
• Not reconciled: against any discussion on the PR; that was not done here.

[jussielo-amd]

Reply to the automated review at 2026-10-14T18:21:40Z (head 32c07d43). All items are addressed in f1ac912d (amended in place over 32c07d43; same single commit, corrected message and content — no rebase needed, the branch is unmoved otherwise).

Blocking

• Windows evidence (standing). Confirmed since the review was posted: E2E tests (Strix Halo, Windows) for this exact head (32c07d43) completed at 00:29:56Z with comfyui-01/-02/-03 all passing — I pulled the job log directly and confirmed each scenario's steps show no failures near them (141 scenarios, 138 passed, 3 expected xfails unrelated to this PR: artifact-prefetch-01, serve-22, runtime-lifecycle-15). The lane was simply still working through the shared-runner backlog when the review ran.
• PR description misstated its own scope and verification. Fixed — rewrote the "What changes" section to say plant_ready_runtime (the shared Given every comfyui scenario builds its runtime from) now plants fake-python as the default for all 13 comfyui scenarios, not just 01-03, and corrected the verification line to "13 scenarios, 105/105 steps".
• used_comfyui_install overwrites the planted fake-python with a shell shim. Fixed by dropping the overwrite — I read fake-python.rs: it already answers the post-install GPU check unconditionally with the same result write_gpu_probe_shim produced, so the overwrite was genuinely redundant. Verified by actually running with it removed: cargo xtask e2e -- -n comfyui still gives 13/13 scenarios, 105/105 steps, and clippy/fmt stay clean (write_gpu_probe_shim is still used by gpu_check_fails/gpu_check_passes_again for comfyui-11's transition, so nothing goes unused). Also reworded comfyui.feature's header comment for comfyui-05..13, since the removed overwrite was the thing that made its "GPU-check stand-in they plant is a #!/bin/sh script" claim read as true for all of them — it's now scoped to comfyui-11's transition, which is the only place that's still accurate.

Non-blocking

• Commit message's garbled sentence. Fixed — the amended message now reads "...so it does not reopen test: write fake executables without the ETXTBSY fork race #649's ETXTBSY race: a plain copy holds a write fd on the destination for the duration of the copy..." matching the PR body's intact wording.

Decisions for the author

Not acted on this round — leaving open rather than guessing:

• Gate comfyui-01/02/03 on the required hosted windows-build-and-test lane instead of only the optional self-hosted Strix one. Agreed this would make Windows coverage enforced rather than incidental; holding off on moving them in this PR to keep the change scoped to the fixture fix e2e: ComfyUI install/status shims (uv, Python, rocm_sdk) are Linux-only, so Windows has no e2e coverage of the actual CLI run #629 asked for. Open to doing it as a fast follow if preferred.
• env!("CARGO_BIN_EXE_<name>") instead of walking up from current_exe(). Agreed this is the more robust approach long-term; install_fixture_binary's current form matches the pre-existing fake-tailscale code, so leaving as-is for consistency unless a repo-wide pass is wanted.
• Two copies of the runtime Python path (runtime_python() vs fixture_target_name()). Confirmed deliberate, as comfyui.feature's own comment already states — they agree on Linux, and any Windows reinstall-scenario ungating later would need to reconcile them then.

Verified locally at f1ac912d: cargo check/clippy -p e2e-cucumber --test e2e --bins -- -D warnings and cargo fmt --check -p e2e-cucumber clean, cargo xtask e2e -- -n comfyui (13/13 scenarios, 105/105 steps) and the Windows run linked above both pass. Leak scan against the merge-base (b8c4b3ef) is clean.

[jussielo-amd]

Filed #689 to track moving comfyui-01/02/03 onto a required Windows lane (the first decision item from the round-5 review reply above).

[rominf]

🔴 Automated review · pr-review-watcher · f1ac912

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 #629 (open) for comfyui-01, -02 and -03: they lose @requires-os:linux. comfyui-04 to -13 stay Linux-only and the feature file explains why, which is the issue's second accepted option. So "Fixes #629" is a fair claim.

Round 6. The commit reviewed last round (32c07d4) is no longer on the branch, so this was a full, blind review. No change request is filed or standing on this PR. What happened to last round's items (checked by the driver against this head):

• Windows evidence: resolved for the code, not for this exact head. E2E tests (Strix Halo, Windows) ran at 32c07d4 (job 116129072114, success). Its log shows comfyui-01, -02 and -03 executed with every step passing. At f1ac912 the E2E self-hosted run is still pending, so the Windows lane has not run at the live head. The reviewer compared the two heads: the PR's own patch differs only in the comfyui.feature comment at :115-120, but the base is newer.
• plant_ready_runtime scope: fixed. The commit message now says the change affects every comfyui scenario (01 through 13).
• used_comfyui_install overwrite: fixed. The overwrite is gone (comfyui_steps.rs:745-759).
• Garbled sentence in the commit message: fixed.
• "Black-box" reason in the commit message: still there. The README does state that rule, so the sentence is not false. It is now listed under Decisions below, along with the fact that this change itself adds a rocm_core import.
• All findings below are new this round.

Blocking

None

Non-blocking

• Comments added or left by this change misstate which ComfyUI scenarios still need the POSIX shell — tests/e2e-cucumber/features/comfyui.feature:90-94, tests/e2e-cucumber/features/comfyui.feature:115-120, tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:127-130, tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:280-281

  • comfyui.feature:115-120 says the whole comfyui-05..13 block is Linux-only partly because "the planted Python path is the literal bin/python3 rather than the Windows-portable stand-in". That is wrong.
    • Every one of those scenarios plants its runtime through plant_ready_runtime. That function now installs fake-python at bin/fixture_target_name("python3") (comfyui_steps.rs:212-219), the same stand-in comfyui-03 uses.
    • Only three scenarios actually depend on the shell:
      • comfyui-11 plants a #!/bin/sh shim through gpu_check_fails / runtime_python (comfyui_steps.rs:1508-1535).
      • comfyui-08 and comfyui-12 call spawn_stand_in, which runs sh -c (:1168).
    • comfyui-05, -06, -07, -09, -10 and -13 use none of these.
  • comfyui.feature:90-94 says the file's other scenarios have fixtures that "are now portable". That contradicts lines 115-120 of the same file.
  • The write_shim doc comment (comfyui_steps.rs:127-130) says the helper is "used by the --reinstall scenarios". Its only remaining caller is comfyui-11.
  • comfyui_steps.rs:280-281 says the Python stand-in answers only the empty-JSON probe. It now answers both probes.
  • The code is what runs, so the code governs.
  • Confidence 85 · mechanical · Fix: state the real per-scenario reasons. Either say the shell reasons apply only to -08, -11 and -12 and give whatever else keeps the rest Linux-only, or remove the tag from the scenarios that no longer need it.

• runtime_python() still hard-codes python3, while plant_ready_runtime now uses fixture_target_name("python3") — tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:1528-1535, :212-214

  • Both functions build the path to the same planted interpreter. They agree on Linux but name different files on Windows (python3 vs python3.exe).
  • If comfyui-11 is ever made portable, its shim would not land on the interpreter the CLI actually runs, and nothing would report it.
  • Confidence 70 · mechanical · Fix: use fixture_target_name("python3") in runtime_python too.

Decisions for the author

• rocm_library_filename duplicates rocm_core::runtime_rocm_library_filename — answered-by-intent
  • The commit message and description say: "the import would buy nothing while breaking this suite's black-box-only rule (no imports from the product crates)". The README does state that rule (tests/e2e-cucumber/README.md:253). Without that reason, this would be a non-blocking duplication finding: the two functions give identical output for amdhip64 and hipblas today.
  • Two things to confirm:
    • The in-code comment at comfyui_steps.rs:182-186 gives only "would buy nothing" and leaves out the black-box rule. The commit message's reason should also be in the code.
    • This same change adds a rocm_core::fake_executable import through install_fixture_binary (tests/e2e.rs:770-782), and moves fake-tailscale onto it. So the suite already crosses the rule, while its README states it without exception.
• FAKE_UV_* are left in command_env, which the first rocm call consumes — tradeoff
  • run_rocm_with_scenario_env takes these values with std::mem::take (tests/e2e.rs:910), so they apply only to the next CLI run. The old shell uv encoded its outcome in the file, so it held for the whole scenario.
  • This matches how ROCM_CLI_UV_BINARY already behaves, and no current scenario runs install twice. A future multi-install scenario would have to set them again.

Positive signals

• Both fixture branches are genuinely exercised. With fake-uv mutated to ignore FAKE_UV_EXIT_CODE and fake-python writing {} as the GPU result, the comfyui feature dropped from 13/13 to 5/13 passing (comfyui-01, -02, -04, -05, -08, -09, -10 and -12 failed). None of those scenarios was a false green.
• fake-python rejects any argv shape it does not expect, and fake-uv panics on a bad exit code. A wrong fixture setup fails the scenario instead of passing it.
• fake-tailscale is now planted through write_fake_executable instead of std::fs::copy, which closes the ETXTBSY window the copy left open in remote_steps.rs.

Deployment notes

None

What this covered

• Read: every changed file, completely, against prw-base 5e45915…f1ac912d. Also the product code the fixtures stand in for:
  • in apps/rocm/src/comfyui.rs, every place that starts the runtime Python or uv, and how environment variables reach them;
  • the readiness checks in apps/rocm/src/therock.rs;
  • rocm_core::fake_executable and runtime_rocm_library_filename.
• Fan-out: two read-only workers. One took the code and product-trace pass; the other took AGENTS.md adherence, history, code comments, docs and the CI lane config.
• Ran:
  • cargo xtask e2e -- -i features/comfyui.feature on Linux, in the checkout's own target/: 13 scenarios and 105 steps passed, exit 0.
  • The mutation run above.
• CI at f1ac912: all hosted checks are green, including windows-build-and-test, E2E tests and clippy. The E2E self-hosted workflow (GPU lanes, including Strix Windows) is still pending.
• Did not run:
  • the Windows e2e lane at this head, locally or in CI;
  • a Windows-target cargo check, which was left to windows-build-and-test;
  • the prior-changes pass.
• Not reconciled: the review itself was not checked against the PR discussion. The round note above is the driver's reconciliation.

[jussielo-amd]

Reply to the automated review at 2026-10-15T09:04:00Z (head f1ac912d). Both non-blocking items are addressed in 269d26f4 (amended in place over f1ac912d; same single commit, no rebase needed).

Non-blocking

• Comments misstated which comfyui-05..13 scenarios still need the POSIX shell. Fixed — I read every reinstall scenario's steps to check which call sites actually reach write_shim/write_gpu_probe_shim (comfyui-11's GPU-check transition) or spawn_stand_in's sh -c (comfyui-08/comfyui-12, both of which start a running ComfyUI stand-in). The other six (comfyui-05/-06/-07/-09/-10/-13) use only plant_ready_runtime, which already plants the portable fake-python. Rewrote comfyui.feature's header comment to name the real three-scenario shell dependency and say honestly why the other six stay @requires-os:linux anyway: out of this fix's scope (e2e: ComfyUI install/status shims (uv, Python, rocm_sdk) are Linux-only, so Windows has no e2e coverage of the actual CLI run #629 only asks about comfyui-01/02/03), and their reinstall behavior hasn't been run on Windows — not a false shell dependency. Also fixed the now-inconsistent claim in the earlier comfyui-04 header comment, write_shim's doc comment (its only remaining caller is comfyui-11, not "the --reinstall scenarios" plural), and the comfyui-02 fixture comment that undersold install_fake_python as single-purpose.
• runtime_python() hardcoded python3 while plant_ready_runtime uses the portable fixture_target_name("python3"). Fixed — runtime_python() now uses fixture_target_name("python3") too, so the two agree on Windows as well as Linux. Verified: cargo xtask e2e -- -i features/comfyui.feature still gives 13/13 scenarios, 105/105 steps (this path is exercised by comfyui-11, the only scenario that overwrites the file runtime_python() names).

Decisions for the author

• rocm_library_filename black-box reasoning. Added to the in-code comment, not just the commit message: it now states the black-box rule explicitly, and acknowledges that install_fixture_binary's own rocm_core::fake_executable import (for the ETXTBSY fix) already crosses that rule for a load-bearing reason this helper doesn't have.
• FAKE_UV_* env var scope/lifetime. No change — agreed this matches ROCM_CLI_UV_BINARY's existing behavior and no current scenario needs it to outlive one CLI run.

Verified at 269d26f4: cargo check/clippy -p e2e-cucumber --test e2e --bins -- -D warnings and cargo fmt --check -p e2e-cucumber clean, cargo xtask e2e -- -i features/comfyui.feature (13/13 scenarios, 105/105 steps). A fresh independent review pass (different model from the one that made these edits) found these changes correct and every edited comment now accurate, with no blocking or non-blocking findings of its own.

[rominf]

🔴 Automated review · pr-review-watcher · 269d26f

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.
Fixes #629 (title, description and commit trailer). The change does the first half of the issue: comfyui-01/02/03 are no longer gated to Linux. It does not cover the issue's "and others in the file", so the Fixes claim goes further than the change does (see Blocking).

Round 7. The commit reviewed last round (f1ac912) is no longer on the branch, so this was a full, blind review. This round files no change request, and none is standing on this PR. The reviewer marked two findings blocking; neither is used as a merge gate this round. Why, as checked by the driver against this head:

• Windows evidence (Blocking 1): standing since earlier rounds, re-found blind; does not gate on its own. E2E self-hosted run 38721961366 for this head is pending with 0 jobs. The self-hosted queue is stalled repo-wide: several other PRs' runs are queued behind it. A lane that cannot run is missing evidence, not a defect in the change. Since the Strix Windows pass at 32c07d4, the PR's own changes are comments plus runtime_python(). runtime_python() is used only by comfyui-11, which is still Linux-only. A human should still see comfyui-01/02/03 pass on Strix Windows at this head before merging.
• Fixes #629 scope (Blocking 2): new wording, below the gate. Last round judged "Fixes #629" fair, because the issue's second option ("document … why … Linux-only") was taken. What is new is the header comment at comfyui.feature:121-124. It now says "#629 only asks about comfyui-01/02/03", which the issue text contradicts. It also frames the remaining six scenarios' gate as deferral rather than design. That makes the comment wrong and the closing claim weaker. Merging would close #629 with those six scenarios gated and nothing tracking them. Either fix in the finding resolves this cheaply: Part of #629 plus a follow-up issue, or ungating the six. It is not used as a merge gate on its own.
• Last round's non-blocking items:
  • runtime_python() now uses fixture_target_name("python3"), so that item is fixed.
  • The shell-dependency comments now name the real three scenarios (-08, -11, -12). One leftover inconsistency remains in the comfyui-04 comment; see Non-blocking 2.
• The author's reply of 10:41Z is answered by this round.

Blocking

• The change's main claim, that comfyui-01/02/03 now pass on Windows, has no CI run at this head — tests/e2e-cucumber/features/comfyui.feature:14,34,63
  • At 269d26f, the E2E self-hosted workflow run 38721961366 is pending and has 0 jobs, so it has not run here. That workflow holds the Strix Halo Windows lane, the only Windows lane that runs the full suite.
  • The hosted windows-build-and-test job does pass. But its e2e step sets E2E_INCLUDE_LIFECYCLE=1 and E2E_ONLY_LIFECYCLE=1 (.github/workflows/ci.yml, step "Acceptance install lifecycle"), so it runs only @lifecycle scenarios and none of the ungated ComfyUI ones.
  • The Windows pass the description cites (job 116129072114) ran on 32c07d43. GitHub's compare shows that commit has diverged from this head (ahead 1, behind 1), so it is a different commit.
  • Confidence 85 · architectural · Fix: get the self-hosted run for this head to actually schedule. A run stuck at 0 jobs after a push usually means a superseded run is holding the concurrency group. Then confirm comfyui-01/02/03 ran and passed on the Strix Windows lane.
• Fixes #629 closes an issue the change only partly does, and the in-tree comment misstates what the issue asks — tests/e2e-cucumber/features/comfyui.feature:121-124, PR description / commit trailer Fixes #629
  • The comment says ungating comfyui-05/-06/-07/-09/-10/-13 "is out of this fix's scope (#629 only asks about comfyui-01/02/03)".
  • #629's text names "comfyui-01, comfyui-02, comfyui-ambiguous-runtime-actionable, and others in the file". It asks either to ungate them, or to document why ComfyUI e2e coverage is Linux-only "by design".
  • For those six scenarios the comment itself concedes there is no shell dependency left. It keeps the gate only as deferral ("has not been run on Windows"), which is neither of the two outcomes #629 asks for.
  • The issue text governs here: the comment paraphrases it, and the paraphrase is wrong.
  • Confidence 85 · mechanical · Fix: one of two.
    • Ungate the six shell-free scenarios and confirm them on the Windows lane.
    • Or change the trailer to Part of #629, open a follow-up for those six, and reword the comment so it no longer says what #629 asks.

Non-blocking

• The comment justifying the local rocm_library_filename twin rests on a false premise — tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:182-193
  • It presents install_fixture_binary as the black-box rule's "load-bearing exception elsewhere in this change".
  • The suite imported rocm_core well before this PR:
    • at prw-base, tests/e2e.rs:323 uses rocm_core::AppPaths;
    • automations_steps.rs:20 imports rocm_core::{AutomationProposalRecord, …};
    • write_shim in this same file already called rocm_core::fake_executable.
  • "In this change" will also read wrongly once merged.
  • The choice itself still holds, because readiness only checks is_file() (therock.rs::probe_has_resolved_library). Only the reasoning is wrong.
  • Confidence 85 · mechanical · Fix: keep only the "readiness only checks is_file()" justification and drop the exception narrative.
• The comfyui-04 comment calls the other scenarios' gate "a separate, narrower limitation" — tests/e2e-cucumber/features/comfyui.feature:90-95
  • The header 20 lines below says six of those scenarios are gated for scope reasons, not because of a technical limitation.
  • The two comments should agree. The lower header describes the gate the scenarios actually carry, so it governs.
  • Confidence 70 · mechanical · Fix: reword to "whose Linux gate is explained below".
• The GPU-probe answer now exists twice — tests/e2e-cucumber/tests/e2e/comfyui_steps.rs:143 (write_gpu_probe_shim), tests/e2e-cucumber/src/bin/fake-python.rs:25 (GPU_PROBE_JSON)
  • The shell shim's only remaining caller is gpu_check_passes_again (comfyui_steps.rs:1533).
  • That step could plant install_fake_python(&runtime_python(world)) instead. Doing so deletes the duplicated JSON and one of comfyui-11's two remaining shell dependencies.
  • Separately, the shim's doc comment still attributes the -c form to probe_comfyui; -c actually belongs to the torch-stack probe. This predates the PR, but the PR touches the function next to it.
  • Confidence 70 · mechanical · Fix: as described.

Decisions for the author

• Permissive stand-ins vs. contract checks — tradeoff
  • fake-python accepts any -c script and any *.py path, and fake-uv ignores its arguments. Neither checks what the CLI actually passed.
  • This matches the shell shims they replace, so nothing got weaker, and it keeps the fixtures simple.
  • The cost is that a wrong probe script or wrong uv flags would still go green.

Positive signals

• Planting fake-tailscale with write_fake_executable instead of std::fs::copy closes a copy site that #649's ETXTBSY fix missed. remote_steps.rs was not in #649's file list.
• The new stand-ins match the CLI's real contract:
  • Their argv shapes match what the CLI passes: 3 entries for probe_torch_stack_versions (comfyui.rs:1971) and 3 for probe_comfyui (comfyui.rs:2181).
  • GPU_PROBE_JSON matches the ComfyUiProbe struct field for field.
  • Both stand-ins fail loudly on any invocation they don't expect.

Deployment notes

None

What this covered

• What was read: all 10 changed files, read completely, at b8c4b3ef (merge-base with prw-base) … 269d26f4.
  • Product code on the paths the fixtures stand in for: apps/rocm/src/comfyui.rs, crates/rocm-core/src/uv.rs, runtimes_cmd.rs::validate_wheel_runtime_manifest, therock.rs::probe_has_resolved_library, crates/rocm-core/src/fake_executable.rs.
  • CI workflow gating for Windows e2e.
  • Issue #629's title, body and state.
• Passes: fan-out ran with two independent workers, one on code/contracts and one on AGENTS.md adherence, history and code comments. Consolidation, design review and verification were done in the driving context.
• Checks run:
  • cargo clippy -p e2e-cucumber --all-targets -- -D warnings on Linux, in the checkout's own target dir: clean.
  • The new pure-std files type-check for x86_64-pc-windows-gnu with rustc --emit=metadata.
  • A full Windows cargo check of the crate could not run, because ring needs x86_64-w64-mingw32-gcc, which is not installed here.
  • The e2e suite was not run locally.
• Test verdicts: this PR changes no production code, so the "passes with the production change reverted" question does not apply.
  • Breaking fake-python's .py arm would turn comfyui-02 and comfyui-05..13 red, because their post-install GPU check reads the result file.
  • Breaking its -c arm would fail comfyui-01/02 at the torch-stack probe.
  • A bad FAKE_UV_EXIT_CODE would flip comfyui-01 or comfyui-02.
  • These were reasoned from the code paths above, not run as mutations.
• Did not run: the prior-changes pass, because earlier review comments are outside this watcher's remit.
• Discussion: this report was not reconciled against any PR discussion.

[jussielo-amd]

Reply to the automated review at 2026-10-15T12:20:50Z (head 269d26f4). All blocking items are addressed, plus the non-blocking items, in 6490e6e3 (amended in place over 269d26f4; same single commit, no rebase needed).

Blocking

• No CI run at this head exercised comfyui-01/02/03 on Windows. Confirmed the read: the self-hosted E2E self-hosted lane is genuinely backed up repo-wide right now, not specific to this PR — at the time of the review, several dozen other PRs' runs across the repo were sitting queued/pending on the same workflow, and there's an open PR (ci/selfhosted-supersede-pr-runs) specifically fixing that backlog. Not something this PR can fix from here. A fresh run (38856168479) is queued for the new head 6490e6e3; will follow up with the Windows job link once the lane clears, same as the last several rounds.
• Fixes #629 closed an issue the change only partly does, and the in-tree comment misstated e2e: ComfyUI install/status shims (uv, Python, rocm_sdk) are Linux-only, so Windows has no e2e coverage of the actual CLI run #629's ask. Agreed on both counts — the comment's framing was wrong, and I'd rather fix the claim than ungate six scenarios whose Windows behavior nobody has actually run yet. Filed e2e: ungate comfyui-05/-06/-07/-09/-10/-13 from @requires-os:linux #697 to track ungating comfyui-05/-06/-07/-09/-10/-13, changed the trailer to Part of #629 (remaining scope tracked in #697), and reworded comfyui.feature's header comment to say the real reason they stay gated (unverified reinstall behavior on Windows, tracked separately) instead of misquoting e2e: ComfyUI install/status shims (uv, Python, rocm_sdk) are Linux-only, so Windows has no e2e coverage of the actual CLI run #629's scope.

Non-blocking

• rocm_library_filename comment's "load-bearing exception" claim was false — confirmed: rocm_core was already imported elsewhere in the suite before this PR (tests/e2e.rs, automations_steps.rs, and this same file's write_shim). Dropped the exception narrative; the comment now states only the real justification (readiness only checks is_file()).
• comfyui-04's "separate, narrower limitation" wording disagreed with the header below it. Reworded to "whose own Linux gate is explained below", deferring to the lower header as the one that actually describes it.
• GPU-probe answer existed twice (write_gpu_probe_shim's shell-script JSON and fake-python.rs's GPU_PROBE_JSON). Fixed as suggested: gpu_check_passes_again now plants fake-python via install_fake_python instead of the shell shim, so write_gpu_probe_shim has no remaining caller and is deleted. write_shim's doc comment is updated to say comfyui-11's GPU-check failing transition is its only remaining caller (the passing-again half no longer needs a shell shim at all).

Decision for the author

• Permissive stand-ins vs. contract checks. Agreed this is a real tradeoff and not a defect — matches what the shell shims they replace already did. No change.

Verified at 6490e6e3: cargo check/clippy -p e2e-cucumber --test e2e --bins -- -D warnings and cargo fmt --check -p e2e-cucumber clean, cargo xtask e2e -- -n comfyui (13/13 scenarios, 105/105 steps, including comfyui-11's GPU-check failing→passing-again transition through the new install_fake_python path). Leak scan against the merge-base (b8c4b3ef) is clean.

[r0x0r]

Overview

Ungates comfyui-01/-02/-03 from @requires-os:linux by replacing POSIX-shell shim fixtures (uv, the runtime's Python) with real compiled [[bin]] stand-ins (fake-uv, fake-python) that build on every platform, following the existing fake-tailscale convention. No production code changes.

Verification performed

• cargo check -p e2e-cucumber --test e2e: clean.
• cargo clippy -p e2e-cucumber --test e2e --bins -- -D warnings: clean.
• Traced fake-python's two accepted invocation forms (-c <script>, <script.py> <result-path>) against apps/rocm/src/comfyui.rs's actual probe call sites — both match.
• Confirmed write_shim (the real shell-shim writer) now has exactly one remaining call site (gpu_check_fails), matching the PR description's claim — the only scenario that needs a failing GPU check, which the unconditionally-success fake-python can't represent.
• Confirmed rocm-core is a [dev-dependencies] entry in tests/e2e-cucumber/Cargo.toml, which is why write_fake_executable plumbing had to move to tests/e2e.rs (reachable) rather than stay in the library's src/fixture_binary.rs (not reachable) — the stated rationale holds up.
• Could not run the actual cargo xtask e2e -- -n comfyui scenarios locally: building rocm/rocmd fails on this Darwin box in engines/lemonade/src/runtime_dir.rs (a u16/u32 st_mode mismatch), which is a pre-existing issue from an unrelated prior commit, not touched by this diff. The PR's linked Windows CI run (comfyui-01/02/03 passing, 141 scenarios total) and the author's reported Linux 105/105 are the available evidence there.

Code quality

• Fixture binaries fail loudly on unexpected input (unparseable FAKE_UV_EXIT_CODE, an argv shape matching neither probe form) rather than silently defaulting — consistent with fake-tailscale's existing rule.
• Shared lookup/plant logic (fixture_binary, fixture_target_name, install_fixture_binary) is factored out of remote_steps.rs's previously-private duplicate, and README.md's Design Principles section documents the pattern for the next stand-in.
• rocm_library_filename() staying a local test-only helper instead of importing the product crate's equivalent is justified (readiness only checks is_file(), never the name) and keeps the suite's black-box-only convention intact.
• Comments are precise about why, not just what (e.g., the .bat/CVE-2024-24576 rejection, the ETXTBSY race std::fs::copy would reopen).

Risk

Low — test-only fixture refactor, no production code paths changed. comfyui-04/-08/-11/-12 correctly keep their Linux-only gates with the reasons now documented inline, and the scope is explicitly limited to #629's three named scenarios (further ungating tracked in #697).

No blocking issues found. Approving.