[jussielo-amd]

Summary

PR #501 added TuiSession::spawn_in, giving dash scenarios a controllable working directory for the real crossterm event loop. PR #638 landed dash-25, proving spawn_in threads a scenario-owned folder into onboarding's Adopt-existing-folder browser end to end — but dash-25's whitespace case lives in a child entry (adopt me) under that folder, not in the spawn_in cwd itself, so it never drives PR #500's quote_display_arg quoting fix through the real approval-preview render a user actually sees.

This adds dash-35 (next free index after the dash-26..dash-34 quit-confirm scenarios that landed in #595 while this branch was in progress): the scenario's own spawn_in cwd has a space in its own name, so onboarding's Adopt flow (which seeds its folder browser from std::env::current_dir()) resolves its start path to that whitespace-containing directory directly. Choosing [ use this folder ] stages an approval whose --root argument is that same path, and the new assertion checks the rendered preview shows it quoted.

PR #500 already has widget-level TestBackend snapshot coverage of the quoting in isolation (approval_snapshot_quotes_args_containing_spaces in crates/rocm-dash-tui/tests/wave0_job_bridge.rs); this closes the black-box coverage gap the issue asks for.

This is additive test coverage only — no production code changes.

CI lane: runs in the default E2E lane (@requires-os:linux, same tagging as every other scenario in dash.feature; no GPU or special lane needed).

How I proved the assertion is load-bearing

The fix (quote_display_arg in crates/rocm-dash-tui/src/ui/exec.rs) is already merged, so I verified the new scenario is a genuine regression test by temporarily hand-editing quote_display_arg to a no-op (returning the value unchanged), rebuilding rocm, and re-running the scenario:

• With the no-op: the scenario fails exactly at the new Then step, and the captured screen shows the unquoted, ambiguous preview (--root /tmp/.../a b --python ...).
• After reverting the edit (confirmed via a clean diff on that file) and rebuilding: the scenario passes again.

Test plan

• cargo xtask e2e -- -n "dash-35" — 13/13 steps pass
• cargo xtask e2e -- -n "dash-(23|25|2[6-9]|3[0-5])" — 12 scenarios / 147 steps pass (onboarding siblings dash-23/dash-25 plus the newly-landed dash-26..dash-34 quit-confirm scenarios, confirming no regression from the rebase/renumber)
• cargo test -p e2e-cucumber --test feature_naming — all 4 naming-invariant tests pass (sequential numbering, unique indexes)
• cargo clippy -p e2e-cucumber --all-targets -- -D warnings — clean
• cargo fmt --check -p e2e-cucumber — clean
• cargo test --workspace --all-targets — full suite green
• Leak scan against origin/main — clean
• No conflict markers after rebase

Fixes #693

[tomastola]

Requesting changes for one blocking issue: the new dash-35 scenario fails in CI (inline comment on approval_preview_quotes_whitespace_root).

Non-blocking, for you to confirm: accepting either " or ' quoting keeps the test from pinning today's quote character, but it also means a regression that switches quote styles would pass silently. The reasoning in the comment is sound and I would keep it.

Nice touches: the scenario uses the whitespace directory as the spawn_in cwd itself, which reaches the --root argument dash-25 never touches, and the wait for the approval title to clear between the two Esc presses avoids the coalescing race.

[tomastola]

Blocking: this step times out in CI because the matcher expects a layout the real popup doesn't produce.

E2E tests (Strix Halo, Ubuntu, nightly, current) fails on dash-35 here with timed out after 90s: https://github.com/ROCm/rocm-cli/actions/runs/38902403040/job/116977856347

The step requires --root "<root>" on a single terminal row. The runner's temp path (/mnt/scratch/actions-runner-01/_work/_temp/rocm-e2e-Qce2gv/a b) is about 70 characters, and the popup wraps at word boundaries. The captured screen shows rocm runtimes adopt --root on one row and the quoted path on the next. The product output is correct (the path is quoted on screen); only the matcher is wrong. This also contradicts the "margin to spare" claim in the ADOPT_WHITESPACE_ROOT doc comment, and it makes the result depend on the host's temp-path length. That is probably why WSL2 passed.

Two ways to fix it:

1. Match on the popup's own text: slice each screen row between the popup's border columns and join those slices, or match the quoted path "<root>" without the --root prefix.
2. Make the scenario root short and deterministic instead of relying on the ambient TMPDIR.

Either way, please correct the "margin to spare" comment.

[jussielo-amd]

Fixed — and thanks for the precise diagnosis, that's exactly what was happening.

Dropping the --root prefix (matching the quoted value alone) fixed the CI failure you hit, but re-checking it turned up that it only narrows the flake window rather than closing it: a slightly longer runner path, or one long enough to force a hard mid-word split (ratatui breaks an unbroken token like a bare path with no embedded whitespace, consuming no space at the break), would still time out the same way.

So I went with your first suggestion instead: the step now reconstructs the popup's own body text by finding its content columns from the title row's own border characters, slicing every body row to that span, and rejoining — telling a true word-boundary wrap (which leaves trailing blank padding behind it) apart from a hard mid-word split (which doesn't), so only the former gets a join space back. That holds regardless of host path length rather than depending on one; verified it against the original CI path length plus a much longer one that forces several hard splits across multiple rows.

Also corrected the ADOPT_WHITESPACE_ROOT doc comment's "margin to spare" claim, which stopped being true the moment the matcher depended on anything about path length at all.

[jussielo-amd]

On the quote-style question: still accepting either " or ' in the final version — the normalization only changes how matched text is assembled across wrapped rows, not what's being matched, so that reasoning carries over unchanged.

fixed

[tomastola]

Re-review of #707: changes since my 2026-10-16 review (head a6f55cc4)

Verdict: approve. The one blocking issue is fixed, and the fix is the more thorough option I suggested.

What changed

Since 5c0778e8 the branch has been rebased onto current main. Main had meanwhile added its own dash-35, the compact GPU VRAM scenario. This PR renumbers its own scenario to dash-36 and updates every reference to it in comments, so the IDs no longer clash. The GPU scenario and the model-serving comment changes come from main, not from this PR.

The PR's own changes are these:

1. The matcher reads the popup's own text now. approval_preview_quotes_whitespace_root no longer searches the whole screen for --root "<root>". It calls the new TuiSession::wait_for_popup_body_where, which rebuilds the popup's text using popup_body_text(ADOPT_APPROVAL_TITLE). It then matches only the quoted value ("<root>" or '<root>'), without the --root prefix.

2. popup_body_text works out the popup's columns from its title row:

   • Left edge: a fixed 3-column offset. I checked this against panel::stamp_title (lb = area.x + 2, which renders ╭─╮<title>╭), and it fails loudly if the notch isn't there.
   • Right edge: a walk through the border characters to the closing ╮.

   It then slices each row between the title and Approve ( to those columns and joins the pieces back into one string. A row with blank cells left at its end is treated as a word-boundary wrap and gets a space when joined; a full row is treated as a word split mid-way and gets none.

3. The ADOPT_WHITESPACE_ROOT doc comment no longer claims "margin to spare". It now says plainly that the line wraps on long temp paths and points to the rebuilt text instead.

Against what I raised before

• Blocking: dash-35 timed out in CI. Resolved. E2E tests (Strix Halo, Ubuntu, nightly, current) now passes, and on the same runner that failed before (/mnt/scratch/actions-runner-01/_work/_temp/rocm-e2e-…). The job log shows dash-36 ran and passed Then the approval preview quotes the whitespace-containing root path; it was not skipped. All other checks are green, including clippy, after the pedantic-lint commit.
• Correct the "margin to spare" comment. Done.
• Non-blocking: accepting either quote style. Settled. You're keeping it and explain why in a PR comment. I said last time I'd keep it too.
• Geometry checks I made:
  • draw_popup_frame uses panel::popup, which has no padding (Padding::ZERO, 1-cell inset). So the slice left+1..right covers exactly the popup's inner area, and no padding column can look like trailing blank space.
  • The button row sits directly below the body because Constraint::Length(2) puts the one-line Paragraph at its top. So stopping at Approve ( ends the body at the right place.
  • On the matcher's argument that "<root>" can't also match --python's quoted "<root>/bin/python": it holds.

Non-blocking notes

1. Non-blocking — one case can still fail, but it's avoidable. If a word wrap lands exactly on the last column, ratatui drops the space and leaves no blank cell, so the code reads it as a mid-word split. The docstring admits this case. Here it can only hit the one space inside a b. If it happens, the result is ab" and the step times out.
   • The temp-dir suffix is a fixed 6 characters, so each runner's path length is fixed. If a runner hits this, it will fail every time, not intermittently.
   • Simpler and fully wrap-proof: remove all whitespace from both the body and the expected string before comparing. Quoting is what this step checks, not the space itself. An unquoted render would still fail because the "/' would be missing. That would make the trailing-blank logic and its long explanation unnecessary.
2. Non-blocking — popup_body_text slices by character index, not by screen column. The fixed-offset check only covers the title row. A double-width character to the left of the popup on a body row would shift that row's slice by one. Nothing renders one there today, so this is low priority.
3. Non-blocking — the comments repeat review history. Phrases like "(PR #707 review, 2026-10-16, and a follow-up review of that fix on the same date)" don't belong in the code. The reasoning that follows them is enough without the dates.

None of these block the merge.