[jussielo-amd]

Why

#637 made comfyui-01/comfyui-02/comfyui-03 (tests/e2e-cucumber/features/comfyui.feature) hermetic — no GPU, no network — and dropped their @requires-os:linux tag so they could run on Windows. But no required CI lane actually ran them there: the required windows-build-and-test job only runs @lifecycle-tagged scenarios (via E2E_ONLY_LIFECYCLE=1), and these three are ComfyUI install scenarios, not release-lifecycle ones. The only Windows lane that ran them was the optional, self-hosted e2e-selfhosted.yml, which isn't a required check and sits on a runner pool that was backlogged across several review rounds of #637 itself (#689).

What changes

tests/e2e-cucumber/tests/e2e.rs's scenario-selection filter gains a second, generic way for an E2E_ONLY_LIFECYCLE=1 lane to select a scenario: a new E2E_ALSO_RUN_IDS env var names specific @ids to fold in, in addition to whatever is tagged @lifecycle. windows-build-and-test's lifecycle step sets it to the three comfyui ids, so they now run on every PR through that required lane.

An id listed in E2E_ALSO_RUN_IDS that matches no scenario's @id is not silently dropped: once the run completes, every listed id is checked (unmatched_also_run_ids) against the ids the suite actually saw, and cargo xtask e2e panics naming any id that never matched — so a renamed or typo'd @id: tag surfaces immediately instead of quietly losing Windows coverage while CI stays green.

Tagging the three scenarios @lifecycle directly was considered and rejected: neither nightly.yml nor e2e-selfhosted.yml sets E2E_INCLUDE_LIFECYCLE at all, so a @lifecycle tag would silently drop them out of the full-suite runs those two lanes already give them, trading one coverage gap for another.

This PR also carries an incidental test-only fix (commit 8f0e37e4, "test(therock): hold the env lock in the SDK archive property tests"): two apps/rocm/src/therock.rs SDK-archive property tests now hold PROCESS_ENV_TEST_LOCK for their whole run. They extract through extract_tarball_and_discard_archive, which spawns bare-name tar via PATH, but weren't holding the lock — so a concurrent test that temporarily narrows PATH could make either property test intermittently fail to launch tar with ENOENT. Same failure mode PR #762 fixed for spawn_test_sleep; unrelated to #689 or the Windows-lane selection work above, but it was intermittently breaking local cargo test --workspace runs during this change, so it's included here rather than filed separately.

Verification

• cargo test -p e2e-cucumber --lib: new unit tests for the selection helper pass, including the unmatched-id guard (unmatched_also_run_ids).
• cargo test -p xtask: the workflow-contract regression test pins the Windows lane's E2E_ALSO_RUN_IDS selection against the step's actual env: entry (via flattened_values, which strips comments) rather than a bare substring match the explanatory comment above env: could also satisfy; the rest of the suite passes too.
• E2E_INCLUDE_LIFECYCLE=1 E2E_ONLY_LIFECYCLE=1 E2E_ALSO_RUN_IDS=comfyui-uv-install-failure-names-log,comfyui-uv-install-progress-streamed,comfyui-ambiguous-runtime-actionable cargo xtask e2e on Linux: confirms comfyui-01/02/03 — and only those three from comfyui.feature — actually execute and pass under this selection, proving the logic end-to-end ahead of the Windows-specific half.
• cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings, python scripts/smoke_local.py: all pass.
• Not verified: the real Windows runner (sccache/Azure credentials, the actual release build) — that's CI's job; watching windows-build-and-test's "Acceptance install lifecycle" step to confirm comfyui-01/02/03 run there too.

Scope

Windows only. Linux already has required coverage for these three scenarios through the e2e ("mock", GitHub-hosted, no GPU) job, which runs the full fast suite with no lifecycle restriction, so Linux's build-and-test lifecycle step is untouched. No product code changes; test code did change (see the incidental therock.rs fix above), and no fixture changes.

---

• N/A — not fixing a previously-xfailed scenario; no rows in tests/e2e-cucumber/expectations.toml reference these ids.
• N/A — no new subcommand or subsystem.
• N/A — CI/test-harness plumbing only; no new or changed user-facing CLI message.

Closes #689

[Copilot]

🟢 Approval recommended

No active review issues remain for the change adding ComfyUI scenarios to the required Windows end-to-end test lane.

No open issues 🎉

What changed in this PR

Closes the Windows coverage gap from #689 by selecting the three hermetic ComfyUI scenarios in the required Windows end-to-end test lane, without removing them from existing full-suite runs. No product code or fixture behavior changes.

Changes:

• Adds E2E_ALSO_RUN_IDS to include specific scenarios alongside lifecycle tests.
• Configures the required Windows lane to select comfyui-01/02/03.
• Adds selection tests, a workflow-contract regression test, and supporting documentation.

File	Description
xtask/​src/​workflow_contract.rs	Adds a regression check for the Windows lane’s ComfyUI selection.
tests/​e2e-cucumber/​tests/​e2e.rs	Reads additional scenario IDs and applies them to lifecycle-lane filtering.
tests/​e2e-cucumber/​src/​expectation.rs	Adds the selection helper and unit tests.
tests/​e2e-cucumber/​README.md	Documents the additional-ID selection option.
tests/​e2e-cucumber/​features/​comfyui.feature	Adds comments explaining required Windows coverage.
docs/​testing.md	Documents Windows selection and its reproduction command.
.github/​workflows/​ci.yml	Selects the three ComfyUI scenarios in the required Windows lane.

_{Review effort: Balanced}

[nowycondro]

🔴 Automated review · pr-review-watcher · 8f0e37e

Review — needs work

Full review of the whole change (base ea86403c…head 8f0e37e4).
Implements issue #689's proposal in full for the CI/test-selection group (a new E2E_ALSO_RUN_IDS mechanism that folds comfyui-01/02/03 into the required windows-build-and-test lifecycle lane); the diff additionally carries an unrelated test-flakiness fix not described by either the PR body or #689 — see Blocking.

Blocking

• The PR bundles an unrelated, undisclosed fix with no mention in the description — apps/rocm/src/therock.rs:7269, :12447, :12477-12479 (commit 8f0e37e4, "test(therock): hold the env lock in the SDK archive property tests").
  The PR body's "What changes" section describes only the E2E_ALSO_RUN_IDS selection mechanism, and its "Scope" section states "No fixture or product code changes." Issue #689 (which this PR closes) likewise scopes the work to "the workflow config for the windows-build-and-test job's scenario selection" with "no fixture changes expected." Yet the diff's third commit widens PROCESS_ENV_TEST_LOCK to pub(super) and adds lock acquisition in two mod archive_properties property tests, fixing a pre-existing PATH-narrowing race (an ENOENT-under-narrowed-PATH hazard for a bare-name tar spawn) that has nothing to do with the Windows E2E lane or #689. The fix itself is well-reasoned and verified correct in isolation (traced: both property tests spawn tar via PATH on every generated case through extract_tarball_and_discard_archive; the lock is held for the whole test body, matches the established pattern from PR #762 for the same lock, and introduces no reentrancy/deadlock risk) — the defect is that it is unreviewed-as-part-of-this-PR by anyone relying on the description to decide what to check, and it bundles two independently-reviewable changes under a description scoped to something else entirely.
  Confidence 90 · architectural · Fix: split this commit into its own PR, or add an explicit line to the description disclosing it and correct the "no product code changes" claim (test code did change, even if not product code in the narrow sense).

Non-blocking

• Nothing ties the hardcoded E2E_ALSO_RUN_IDS id list back to the actual @id: tags in comfyui.feature — tests/e2e-cucumber/src/expectation.rs:965-998, xtask/src/workflow_contract.rs:3923-3959, tests/e2e-cucumber/features/comfyui.feature:21,41,70.
  The three id strings (comfyui-uv-install-failure-names-log, comfyui-uv-install-progress-streamed, comfyui-ambiguous-runtime-actionable) are currently correct, but they're duplicated independently in ci.yml's env value, the xtask contract test, and docs/testing.md, with nothing reading comfyui.feature itself to confirm the ids still exist there. A future rename or typo of one of these @id: tags would make lifecycle_lane_includes simply stop matching it — the scenario would silently drop back out of required Windows coverage, with no test failing, which is exactly the class of gap this PR exists to close via a different path. This file already has the right pattern for this (the feature_tags helper used by the_rocr_reindexed_scenario_is_gated_on_multi_gpu_and_its_sibling_is_not, which extracts tags straight from include_str!(...) "so the gate is only worth anything if the scenario in the file actually carries it").
  Confidence 75 · architectural · Fix: add a test that extracts the @id: tags for comfyui-01/02/03 from comfyui.feature (reusing or adapting feature_tags) and asserts they match the three ids pinned in the ci.yml contract test.

Decisions for the author

• lifecycle_lane_includes's id matching is exact-string (case- and whitespace-sensitive) — non-blocking-improvement. This is safe today because e2e.rs already trims each id before calling it, but that invariant lives in the caller rather than the callee and isn't exercised by a test that feeds an untrimmed or differently-cased id directly into lifecycle_lane_includes. Worth a direct test if that invariant is ever expected to travel with the function rather than its one caller.

Positive signals

• The new xtask contract test (ci_windows_lifecycle_lane_also_selects_the_hermetic_comfyui_scenarios) checks the whole step block, including env:, not just the run: script — verified it would actually fail if the env var were deleted, an id were dropped from the list, or the step were renamed/moved, rather than being a tautology against the step's own comment.
• The ci.yml comment, comfyui.feature comment, README tag-table row, and docs/testing.md section all tell a mutually consistent story about why these scenarios aren't tagged @lifecycle, and every factual claim in them was independently verified against the actual workflow files (e.g. E2E_INCLUDE_LIFECYCLE is set only in ci.yml; e2e-selfhosted.yml's own header confirms it isn't a required check).
• The out-of-scope therock.rs fix (see Blocking) uses the poison-safe unwrap_or_else(PoisonError::into_inner) form, more robust than several pre-existing .lock().unwrap() call sites in the same file, and widens visibility with the minimal pub(super) rather than pub(crate).

Deployment notes

None — Windows-CI-only selection logic and a test-only lock fix; no product/runtime behavior, migration, or configuration changes.

What this covered

Every file in the diff read completely at the PR head, plus surrounding context (callers of changed symbols, the archive_props.rs property-test harness, the feature_tags/model_serving.feature precedent, other apps/rocm test modules holding the same lock, and all .github/workflows/*.yml for the E2E_INCLUDE_LIFECYCLE/required-check claims), across three independent fan-out passes (the coupled CI/test-selection-logic group; the isolated therock.rs commit; and the four source-reading passes — agent-instruction adherence, history, prior-change precedent, code comments). All three passes completed normally; none were degraded. ea86403c…8f0e37e4.

CI read via gh pr view --json statusCheckRollup rather than assumed: all checks, including the required windows-build-and-test and the readthedocs build, are SUCCESS as of this head commit.

Read the PR description and the linked issue #689 (OPEN, retrieved via gh issue view 689) at Step 9, after findings were settled from the code alone.

Not reconciled against any review thread or comments on this PR — none were read.

[jussielo-amd]

Thanks for catching this. Updated the PR description to disclose 8f0e37e4 under "What changes" (what it fixes, why it's unrelated to #689, why it's bundled here) and corrected the "Scope" line to no longer claim no test-code changes.

Leaving the two non-blocking suggestions (the @id:-tag drift test and the exact-string-match note on lifecycle_lane_includes) as follow-up ideas rather than acting on them now — happy to pick either up if you'd rather see them in this PR.

[juhovainio]

Looks good to me. The Windows lane now picks up the hermetic comfyui scenarios through a generic env var, and the therock.rs lock fix is correct (poison-tolerant, same pattern as the sibling tests). CI is green.

Two small hardening suggestions, left as inline comments, neither blocking:

• an id in E2E_ALSO_RUN_IDS that matches no scenario is silently ignored, so a renamed comfyui @id would quietly drop Windows coverage
• the xtask contract test's contains("E2E_ALSO_RUN_IDS") check can be satisfied by the explanatory comment

Also, the claim that comfyui-01/02/03 need no e2e-test-hooks can't be seen from the diff alone, since the Windows step builds without hooks. The green Windows run supports it.

The therock.rs fix is unrelated to #689, but it is disclosed in the PR body, so I'm fine with it here.

This reflects a read of the diff plus the CI results, not a local run.

[nowycondro]

🔴 Automated review · pr-review-watcher · 54e522e

Review — no blocking findings

Full review of the whole change (base ea86403c…tip 54e522e8, all 8 changed files read in full plus surrounding context: ScenarioDecl/resolve/filter_run in the e2e harness, the tests/archive_properties module split in apps/rocm/src/therock.rs, and the workflow_contract.rs helpers the new CI-contract test reuses).
Closes #689 ("Gate comfyui-01/02/03 on a required Windows lane, not only the optional self-hosted one") — the change implements exactly the proposal in that issue (fold the three ids into windows-build-and-test's existing lifecycle-only selection via a new, generic env var) with no fixture changes, matching the issue's stated scope.

Blocking

None

Non-blocking

None

Decisions for the author

• The PR bundles an unrelated test-flakiness fix (apps/rocm/src/therock.rs's PROCESS_ENV_TEST_LOCK visibility/lock-holding change, commit 8f0e37e4) into a PR otherwise scoped to the Windows CI lane change — tradeoff. The repo's own contribution guidance (AGENTS.md §11) asks for one logical change per PR, but that section is explicitly advisory/human-enforced, and the PR description discloses the bundling and its rationale (observed local flakiness, same failure mode #762 already fixed for spawn_test_sleep) rather than hiding it. Worth confirming this is still how the author/maintainers want it split, versus carving it into its own PR.

Positive signals

• unmatched_also_run_ids turns a silent-coverage-loss failure mode (a renamed/typo'd @id: tag quietly dropping a scenario from the Windows lane while CI stays green) into a hard panic, and it's backed by dedicated unit tests for both the matched and unmatched cases.
• The new xtask workflow-contract test (ci_windows_lifecycle_lane_also_selects_the_hermetic_comfyui_scenarios) deliberately checks the real env: entry via flattened_values rather than a substring match that the explanatory comment above env: could also satisfy — it would have caught a comment-only edit that forgot the actual env line.
• The three scenario ids are kept identical and cross-referenced consistently across ci.yml, docs/testing.md, comfyui.feature's comments, and the new xtask test — verified no drift between any of them.

Deployment notes

None — CI/test-harness plumbing only, no product code or user-facing behavior changes, no migration.

What this covered

Every file in the diff, read completely, plus: ScenarioDecl/filter_run/the resolutions map in tests/e2e-cucumber/tests/e2e.rs to confirm every scenario's @id is recorded regardless of run/skip status (so unmatched_also_run_ids can't false-positive on an OS-gated or otherwise-skipped scenario); crates/rocm-core/src/archive_props.rs to confirm no internal lock use that would deadlock against the new lock-holding in the property tests; .github/workflows/nightly.yml and e2e-selfhosted.yml (grepped) to confirm neither sets E2E_INCLUDE_LIFECYCLE, matching the PR's stated reason for not tagging the three scenarios @lifecycle; tests/e2e-cucumber/expectations.toml (grepped) to confirm no xfail rows reference these ids, matching the PR's checklist claim; and issue #689 (retrieved live via gh issue view) compared against the change's actual scope.
Live-rechecked PR status before posting: head still 54e522e8e0a1b0f6fb822399c93f113d27321c72, and all required checks (including windows-build-and-test and E2E consolidated report, both still in progress at the start of this review) are now SUCCESS — no failing checks.
Did not run: a multi-worker fan-out (diff is small — 8 files, ~217 lines — so a single careful pass covered it per the task's own guidance); the history/prior-changes source-reading passes beyond a git log skim of the two main Rust files (nothing notable surfaced). Not reconciled against any existing review-thread discussion on this PR — that was deliberately not read, per this method's independence rule.