[jussielo-amd]

Summary

The runtime-02 scenario (@id:runtime-path-not-nested) could only fail. Its step looked for a Folder: line in rocm examine output, but examine states the active managed runtime's folder as active_runtime_root: and never prints Folder: (that label is used by the chat assistant's answers and the services text). The expected-fail row for the scenario, added for the recursive-nesting bug that was fixed in #203 (tracked as EAI-7384), kept hiding that: the lane counted the failure as expected, so nobody saw that the step was not testing nesting at all.

This change makes the step read the field examine really prints, and removes the now-stale expected-fail row in the same change.

What changed

The judgement moves into the e2e library as check_runtime_root_not_nested (new tests/e2e-cucumber/src/runtime_root.rs), so unit tests can run it; the step in runtime_steps.rs now only calls it and panics with the reason plus the examine output. It reads exactly the active_runtime_root field (a sibling such as active_runtime_pip_cache_dir or a stray Folder: line is never read), counts the runtimes/wheel pair of path segments segment by segment in either separator form, accepts at most one, and fails loudly when the field is missing or empty. Comparing segments rather than the substring means a folder that merely contains old-runtimes/wheel-cache is not miscounted.

The runtime-path-not-nested row is removed from expectations.toml. The harness treats a passing expected-fail scenario as a stale entry and fails the lane, so the corrected step and the row removal have to land together.

Why removing the row is safe, and what that rests on

I did not run the scenario locally: it is @requires-gpu and the machine I used reports no GPU, so it would be skipped. The evidence is CI on this head commit (6cb1c85, tested merged into 176323d). In the first attempt of the E2E self-hosted run, runtime-02 passed (Then the managed runtime folder path is not recursively nested) on the MI300X lane and on both Strix Halo Ubuntu lanes (release and nightly). On main at the base commit and on each of the three completed main push runs after it (all on 2026-11-04), the same scenario failed on both Strix Halo Ubuntu lanes, which is the step that could only fail; this change turns that failure into a pass. The Windows lane also passed, and the examine output captured on the W7900 and MI300X lanes of an earlier main push run printed one runtimes/wheel in active_runtime_root (<data dir>/runtimes/wheel/<runtime key>, in the Linux and the Windows separator form respectively), which is what the new unit tests are built around. The W7900 lane was cancelled in the first attempt, so there is no completed W7900 run on this head; the rerun was queued when this was written. The WSL2 lane skips the scenario because its capability probe reports no GPU. If a lane did print a nested root, the scenario would now fail red where before it would have been an expected failure, which is the signal the scenario exists to give.

The Strix Halo Ubuntu and MI300X jobs of that attempt are red for other scenarios. bench-06, chat-06, chat-07, serve-01, serve-02, serve-07 and serve-40 fail identically on the Strix Halo Ubuntu lanes of main at the base commit, and none of them reads the runtime folder; this change touches only the e2e library and one step. The MI300X failures are a subset of those same scenarios, but its main run at the base commit was cancelled, so there is no like-for-like comparison for that lane. automations-11 additionally failed on the Strix Halo Ubuntu release lane of that attempt; it did not fail on the main runs I checked and it exercises the background helper rather than the runtime folder, so I treat it as unrelated until the rerun shows whether it recurs.

Tests

Nine unit tests in runtime_root.rs: a flat Linux root and a flat Windows root (drive colon intact) pass and are returned; a root with runtimes/wheel twice fails in the Linux, the backslash and the mixed-separator forms and names the folder and the count; look-alike folders are not counted; a missing field and an empty value fail loudly; only the exact field is read. I checked they protect the step by breaking it two ways: making the nesting check unable to fail turned the three nested-root tests red, and reverting the field name to Folder turned seven of the nine red; both were restored. I also ran the mutation suggested in review, changing the count > 1 bound to count > 2: the three nested-root tests (backslash, mixed-separator and the Linux form naming the count) went red and the other six stayed green; it was restored afterwards. cargo test -p e2e-cucumber --lib, cargo clippy -p e2e-cucumber --all-targets -- -D warnings and cargo fmt --check pass, and the pre-push hooks passed.

No Gherkin scenario added: this is test-harness code, and the existing runtime-02 is the scenario that this change makes able to pass.

Limit of the guard: runtime-02 inspects only the runtime that is active when it runs, so it catches a nested root in that runtime but would not reproduce nesting created by re-provisioning; it is a narrower guard than the original bug.

Overlap with other open PRs

#517 and #518 change this scenario's @requires-gpu tag to @requires-real-gpu, reshape expectation.rs, and edit an unrelated step near the top of runtime_steps.rs. This change does not touch the feature file or expectation.rs and does not depend on either PR; it should rebase over them without conflict. #820 rewrites runtime-09 in the same runtime_steps.rs, but this change only edits assert_runtime_path_not_nested.

Not run

The scenario itself, locally (no GPU on this machine). On CI, W7900 has no completed run on this head yet.

[Copilot]

🟢 Approval recommended

No active review issues are identified for this test-only change restoring managed-runtime nesting regression coverage.

No open issues 🎉

What changed in this PR

Repairs runtime-02 so it checks the managed runtime path reported by rocm examine, restoring regression coverage for the recursive-nesting bug fixed in #203. No product code changes.

Changes:

• Reads the exact active_runtime_root field instead of the absent Folder: label.
• Extracts validation into a library helper with nine unit tests covering path separators, nesting, look-alike segments, and missing or empty fields.
• Removes the stale expected-failure entry so nesting regressions fail the lane.

The GPU-gated scenario was not run; the PR reports unit-test and captured-output validation.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​runtime_steps.rs	Delegates path validation to the helper and includes examine output on failure.
tests/​e2e-cucumber/​src/​runtime_root.rs	Adds exact-field parsing, segment-based nesting detection, and unit tests.
tests/​e2e-cucumber/​src/​lib.rs	Exports the runtime-root validation module.
tests/​e2e-cucumber/​expectations.toml	Removes the scenario’s stale expected-failure entry.

_{Review effort: Balanced}

[juhovainio]

The change itself looks right to me: the step now reads the field examine actually prints, a missing or empty field fails loudly, and the new unit tests cover the path shapes well. I have two things I'd like sorted before merge.

1. Lane evidence for the removed expectation row. Dropping runtime-path-not-nested is only safe if every lane prints a flat active_runtime_root. The MI300X and W7900 E2E jobs were still pending when I looked, and the PR notes there is no log for the Strix Ubuntu lane. Please confirm those lanes pass runtime-02 with the row removed. Separately, the Strix Halo Ubuntu nightly and release E2E jobs are red (about 37 min, in the "Run E2E tests on Strix Halo" step); I couldn't get their logs, so please check whether that is the known timeout/infra problem or something this change touches, since that lane also runs runtime-02.

2. PR body vs. the tracker. The body says EAI-7384 is "now closed in the tracker", but that ticket (now ROCMAI-391) is still In Progress. Please correct the wording or close the ticket.

Minor, no action needed: the nesting check only counts runtimes/wheel occurrences, so a root outside that layout would pass. That is fine for this scenario's purpose.

[juhovainio]

Removing this row only holds if every lane prints a flat active_runtime_root. With it gone, a nested root on any lane turns runtime-02 red instead of being an expected failure. Please confirm the W7900, MI300X and Strix Ubuntu lanes pass with the row removed before merge.

[jussielo-amd]

The PR text records the lane evidence for this head: runtime-02 passed on the MI300X lane and on both Strix Halo Ubuntu lanes in the first attempt of the E2E run, and the Windows lane passed too. The W7900 lane has no completed run on this head yet (it was cancelled in the first attempt and the rerun is still queued), so that is the one gap. If any lane did print a nested root, runtime-02 now fails red there where before it would have been an expected failure, which is the signal the scenario exists to give.

[nowycondro]

🤖 Automated review (Claude Code) · 6cb1c85

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. No ticket named in the PR body as a "closes" target; the body references EAI-7384 (Jira ROCMAI-391) as background for why the removed expectations.toml row is stale, not as something this PR closes.

Blocking

• The PR's "safe to remove the xfail row" argument is not yet fully confirmed by CI on this head commit — tests/e2e-cucumber/expectations.toml (removed runtime-path-not-nested row), tests/e2e-cucumber/tests/e2e/runtime_steps.rs:1062-1072.
  Checked all 32 check-runs for SHA 6cb1c850a43f454160f453dab5384f891c3f3714 directly. Two of the five GPU E2E lanes this argument depends on (MI300X, W7900) are still queued and have not run. The other two Linux GPU lanes that did run — Strix Halo Ubuntu (release) and Strix Halo Ubuntu (nightly) — both report failure. Pulling the job logs for both shows the failures are two unrelated scenarios in a different feature area this diff doesn't touch (automations-helper-stops-when-no-watcher-is-enabled, serve-stop-leaves-no-background-helper), and that runtime-02 itself — the scenario this PR's fix backs — passed cleanly on both lanes (✔ Then the managed runtime folder path is not recursively nested), which is good independent confirmation the fix itself is correct where it has run. But the required checks are still red, and two of the lanes the PR's own description leans on for safety haven't executed at all yet.
  Confidence 90 · architectural · Fix: wait for the MI300X/W7900 lanes to complete and confirm runtime-02 passes there too; separately get the two Strix Halo Ubuntu failures explained or resolved (they read as pre-existing/unrelated, but are currently red on this commit).

• PR body misstates the referenced ticket's status — PR body, "Why removing the row is safe" section ("...fixed in #203 (EAI-7384, now closed in the tracker)...").
  Retrieved the ticket directly (EAI-7384 resolves to Jira ROCMAI-391, "Managed runtime path nests recursively on re-provisioning"): its live status is In Progress, not closed. The underlying technical claim (the nesting bug is fixed) is independently corroborated by the passing runtime-02 runs above, so this doesn't put the diff's correctness in doubt — but the description's evidentiary claim about the tracker is factually wrong as written.
  Confidence 100 · correctness · Fix: correct the wording (e.g. "fixed in #203, tracked by EAI-7384") or get the ticket's status updated to match.

Non-blocking

None

Decisions for the author

• runtime_root.rs's field-line parser duplicates a pattern that already exists elsewhere — non-blocking-improvement.
  active_runtime_root() (new, in the test crate) and chat_tool_value() (existing, private to apps/rocm/src/chat.rs) both parse name: value lines out of CLI text output, independently. Not a defect — chat_tool_value is private to a different, non-library crate and isn't reachable from tests/e2e-cucumber without exporting it — but worth a shared helper if a third copy of this pattern shows up.

Positive signals

• The 9 new unit tests are each pinned to a distinct branch/invariant rather than restating the happy path (separator form, exact-nesting-count, exact-field-match against a similarly-named sibling field, missing/empty field) — verified independently that mutating any one of those branches flips a specific test, not all of them, consistent with the author's own description of having broken the code two ways to confirm this.
• runtimes_wheel_count compares path segments, not substrings, specifically avoiding the false-positive a naive .contains("runtimes/wheel") would hit on a folder like old-runtimes/wheel-cache, and that exact case has a dedicated regression test.

Deployment notes

None — test-harness-only change, no user-facing behavior, no migration.

What this covered

Read in full against head 6cb1c850a43f454160f453dab5384f891c3f3714 (base main @ 176323d4): tests/e2e-cucumber/expectations.toml, tests/e2e-cucumber/src/lib.rs, tests/e2e-cucumber/src/runtime_root.rs (new), tests/e2e-cucumber/tests/e2e/runtime_steps.rs; plus surrounding context (apps/rocm/src/examine_report_text.rs, apps/rocm/src/chat.rs, apps/rocm/src/examine_cmd.rs, tests/e2e-cucumber/features/runtime_setup.feature, the E2eWorld struct). Resolved entirely via the GitHub API; the PR branch was never checked out.

Fan-out: three independent workers ran to completion — a code-correctness + impact-analysis pass over all four changed files (no blocking findings; verified segment-counting, field-parsing, type compatibility, TOML-removal safety, and the 9 unit tests' specificity), an agent-instruction-adherence pass against this repo's AGENTS.md (no blocking findings), and a combined version-history + prior-open-PR pass (identified PR #203/ROCMAI-391 as the code fix behind the removed row; found no other open PR touching the same lines). All three completed independently; one needed a second spawn attempt after hitting a shared background-agent concurrency limit, not a content issue.

CI was read directly, not assumed: all 32 check-runs and the one status context for this exact head SHA, plus the job logs for the two failing checks, as described above. MI300X and W7900 E2E tests were still queued at review time.

The referenced ticket (EAI-7384 / Jira ROCMAI-391) was retrieved directly rather than trusted from the PR body.

A human reviewer (juhovainio, MEMBER) independently posted a CHANGES_REQUESTED review on this exact commit raising the same two points as this review's Blocking findings above; this review reached them independently (CI and the ticket were checked directly before that review was read, as the final race-check step of this process), noted here only for the reader's context.

Not reconciled against any other discussion or comment thread.

🤖 Generated with Claude Code

Both points addressed in the PR description (no code change was needed). The body now cites runtime-02 passing on the MI300X and both Strix Halo Ubuntu lanes at this head, the same scenario failing on main at the base commit, and the other red scenarios failing identically on main; it states that W7900 has no completed run on this head yet. The ticket wording is corrected to "tracked as EAI-7384". Requesting a fresh review.

[juhovainio]

Looks good. The step now reads active_runtime_root from examine, the nesting check counts runtimes/wheel segments (at most one allowed), and the xfail row is dropped in the same change. Scope matches the ticket and the PR description matches the diff.

Two non-blocking notes, one inline below: the scenario only inspects the already-active runtime, so it is a weak guard against the re-provisioning case; and the PR text does not say the new unit tests were checked to go red when the guard is broken.

This is a diff-level read. I did not build or run the tests locally. The Strix Halo nightly/release failures look like runner/network issues (llama.cpp download failures, GitHub API 403) rather than this change, and MI300X/W7900 were still pending when I looked.

[juhovainio]

This scenario only inspects the runtime that is already active, so it will not reproduce nesting created by re-provisioning. It is a weak regression guard for the original bug; worth saying so in the PR text. Also worth recording a mutation check for the new unit tests (e.g. change the count > 1 bound to count > 2 and show them go red).

[jussielo-amd]

Agreed on both points, and both are in the PR text now. It says that runtime-02 inspects only the runtime that is active when it runs, so it is a narrower guard than the original bug and would not reproduce nesting created by re-provisioning. I also ran the mutation you suggested, changing the count > 1 bound to count > 2: the three nested-root unit tests go red and the other six stay green, and the file was restored afterwards.