You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Restores the install-folder (--prefix) choice that PR #67 dropped when it retired the legacy tui.rs and rebuilt onboarding as a minimal wizard. The deferral was tracked rather than intended — see #75.
Onboarding Configure step (crates/rocm-dash-tui/src/ui/onboarding.rs) grows a Folder row. Tab opens the existing FolderBrowser (the same one the Adopt-existing-folder path uses) to pick an install location; confirming includes --prefix PATH on the staged rocm install sdk. Leaving it unset keeps the default managed folder, byte-identical to today.
docs/manual-testing.md — the onboarding bullets are rewritten to match what the wizard actually does.
Why this way: the CLI plumbing (rocm install sdk --prefix) and the UI pattern (FolderBrowser + a prefix field) already exist and are exercised by the Install overlay and the Adopt path, so this wires the established pattern into the Configure step rather than inventing a new one.
Risk: low. The unset path is byte-identical — --prefix is the only insertion and it is conditional.
Scope beyond the onboarding change — please read
Review rounds pulled in work outside the one-file scope #75 describes. Calling it out rather than leaving it to be discovered in the diff:
crates/rocm-dash-tui/src/ui/format.rs is new. Two copies of display_or_placeholder pre-existed, byte-for-byte, in install_manager.rs and serve_wizard.rs; a third was written by this PR's own first commit (for the new onboarding Folder row) and deduped into this shared module four commits later — so nothing pre-existing was triplicated, and this PR is what introduced the third copy in the first place. That is why install_manager.rs / serve_wizard.rs appear in the diff at all: they now call the shared helper instead of their own copy.
The dedupe changed behaviour, not just location. The shared helper's emptiness check moved from v.is_empty() to v.trim().is_empty(), which is a real rendering change at the five pre-existing call sites this PR does not otherwise touch: install_manager's Channel and Folder rows, and serve_wizard's Model, Host, and Port rows now render the placeholder for a whitespace-only value, where they previously rendered the literal whitespace. install_manager::tests::whitespace_only_channel_and_prefix_render_placeholders (new) pins this at a call site, not only in the helper's own unit test — it fails if the predicate reverts to v.is_empty(), and no other test would have caught that.
docs/manual-testing.md loses two bullets it previously had, describing a recommended-ROCm-folder line and a downloads stay inside: <ROCm folder>\pip-cache display. Neither string exists anywhere in the onboarding code — they documented a flow the wizard does not implement.
Trimming rule, after a round of getting this wrong in both directions: the crate's rule is byte-exact where the folder browser is the only writer, trimmed where a human types. Onboarding's cfg.prefix has no typing path, so it is passed through exactly — a directory whose name legally ends in whitespace is not silently redirected. install_manager's prefix and serve_wizard's model are both typed and browser-filled, so both stay trimmed as they always were. In serve_wizard that is load-bearing: the approval title, the job id and the duplicate-launch guard all derive from model.trim(), so an untrimmed argv would let two launches share one job id while running different commands.
Test plan
cargo test -p rocm-dash-tui — covers, with one named test per branch: Tab opens the browser from Configure; choosing a folder sets the prefix without staging an approval; confirming after a choice stages --prefix; cancelling leaves an already-chosen prefix intact; the onboarding prefix survives a trailing space unchanged; install_manager / serve_wizard trim theirs; Tab browses without touching the channel while ←/→ still toggle it; a whitespace-only channel/prefix render the placeholder at a call site.
cargo test -p e2e-cucumber --test e2e — see Gherkin coverage below for the two new scenarios.
Verification gap on the substantive commit (dbc4a8c), stated plainly: it was authored in an environment with no Rust toolchain and no network route to install one, so cargo test / clippy / build were not run against it at the time. The change was reviewed by reading — types, borrows, and each new test traced by hand against the production path. The latest commits on top were made in an environment where those checks were available and do close the gap: 2ecd47d7 is a mechanical cargo fmt --check-driven cleanup (two formatting-only sites, no logic); the commits closing this review round run and pass cargo test -p rocm-dash-tui, cargo clippy -p rocm-dash-tui -p rocm --all-targets, and cargo build -p rocm locally, in addition to CI.
Gherkin coverage
AGENTS.md §3 asks for a scenario in tests/e2e-cucumber/features/ for user-observable behaviour, or an explicit justification. Two scenarios were added to close this:
@id:dash-onboarding-configure-folder-browse (dash.feature) — opens the onboarding wizard from the Observe tab, advances to the SDK Configure step, presses Tab to open the install-folder browser, and confirms the browser's default "use this folder" entry. The final assertion is on the Configure step's rendered Folder row (it must show the chosen path, not the unset placeholder), not merely that the browser popup appeared — a browser-opened-only assertion would not cover the staged --prefix, which is the behaviour Restore install-folder (--prefix) choice in the dash onboarding wizard #75 actually restores.
@id:bootstrap-setup-no-tty-advertises-install-folder (bootstrap.feature, new feature file) — runs rocm bootstrap setup piped (no PTY, matching every CI/script invocation) and asserts the printed message mentions choosing an install folder.
Both run on the existing mock/no-GPU lane; neither needs a GPU or a real install.
The rebuilt onboarding wizard (PR ROCm#67) dropped the folder picker the
old tui.rs offered for `rocm install sdk --prefix`, always installing
to the default managed folder. Add a Folder row to the SDK Configure
step, reusing the same FolderBrowser and --prefix plumbing the Install
overlay and Adopt-existing-folder path already use.
FixesROCm#75
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Extract the message into a constant and add a unit test asserting it
still advertises the install-folder choice, addressing PR review
feedback that this wording change had no regression coverage.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Onboarding's Configure Tab reopened the folder browser at cwd instead
of the already-chosen --prefix, and it duplicated install_manager's
display() placeholder helper verbatim.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Avoid trimming valid trailing whitespace from install paths
crates/rocm-dash-tui/src/ui/onboarding.rs:205
The selected value is a filesystem path, so trimming it can silently redirect installs when a valid Linux directory name ends in whitespace (for example, choosing /opt/rocm-sdk stages /opt/rocm-sdk). The browser already supplies a non-empty path; preserve that exact value and only use is_empty() to decide whether to emit --prefix.
cfg.prefix only ever comes from the folder browser, never typed, so
.trim()-ing it before staging --prefix could silently redirect the
install if a real directory name ends in whitespace.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
install-manager's prefix is typed directly too, not only set by the
folder browser (unlike onboarding's, which really is browser-only).
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Tab rebuilt the reopen path from cfg.prefix.trim(), so a real folder
name ending in whitespace got silently redirected to a different,
trimmed path on re-open.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
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.
Summary
Adds an optional install-folder row (--prefix) to the dash-tui onboarding wizard's SDK Configure step via the shared FolderBrowser, restores the install-folder wording in the non-interactive rocm bootstrap setup message, and moves a placeholder helper into ui/format.rs — no blocking findings, with a handful of prose-vs-code cleanups worth doing. Verified: all six commits read individually; each of the four headline tests discriminates the branch it names (Tab-opens-browser, choose-sets-prefix-without-staging, confirm-stages---prefix, cancel-leaves-prefix-untouched all fail under a single-branch mutation, not just a wholesale revert), and the two trim-related tests fail if build_install_args/the Tab reopen path trims again; the adopt-vs-prefix disambiguation (install_config.is_some()) is sound because key dispatch intercepts every key while Configure holds focus, so activate_choice can never open the adopt browser with a stale config; the empty-prefix path is genuinely byte-identical (the --prefix pair is only pushed when non-empty, arg order otherwise unchanged); the PR's claims about reusing the existing browser, the unchanged default path, low additive risk, and the passing local checks all hold against the code at this head — a targeted run of the onboarding unit tests passed here, but the full suite was not run here. Checks at review time: 22 success, 2 failure, 2 pending, 1 skipped. Blocking: 0 · Non-blocking: 6.
🚫 Blocking (must fix before merge)
None.
Non-blocking
crates/rocm-dash-tui/src/ui/format.rs:21-29 — the doc comment a commit in this range exists specifically to correct is still inaccurate: it says the helper is shared "for their --prefix folder row display", but install_manager.rs:383 also uses it for the Channel row; widen the wording to "an optional field's value, or a placeholder when unset".
crates/rocm-dash-tui/src/ui/onboarding.rs:280 — the section comment still reads // 1) Adopt folder browser has focus., but that block now also handles the Configure prefix browser, and the entire adopt-vs-prefix disambiguation rests on install_config.is_some(); retitle it and state the invariant, or a later reader adding a third browser opener will break the branch silently.
crates/rocm-dash-tui/src/ui/onboarding.rs:608 — draw_configure's doc comment still lists only "channel toggle, pin-mode selector, and … the value field"; it now also draws the Folder row.
docs/manual-testing.md:71-73 — the two bullets immediately above the one this PR rewrites still claim the setup screen "shows a recommended ROCm folder" and shows downloads stay inside: <ROCm folder>\pip-cache; neither string exists anywhere in the onboarding code, so they describe a flow the wizard does not implement.
crates/rocm-dash-tui/src/ui/serve_wizard.rs:539 — a third byte-identical copy of the deduped helper (display_value) survives, so the dedupe is incomplete crate-wide; also worth a thought whether a UI placeholder helper belongs in a module whose header scopes it to "humanized number / unit formatters".
apps/rocm/src/bootstrap.rs test and crates/rocm-dash-tui/src/ui/onboarding.rs (cancelling_folder_browser_leaves_configure_untouched) — both are weak guards rather than behaviour tests: the first pins a const against itself and would still pass if the folder choice it advertises were later removed from the wizard, and the second's prefix.is_empty() assertion is true by construction today since only the Chosen arm ever writes the field. Separately, the PR text does not say why no tests/e2e-cucumber scenario accompanies a user-observable change, which the contributor guide asks for explicitly even when the answer is "a TUI wizard isn't reachable from that suite".
… row
Widen the display_or_placeholder doc comment to cover its Channel-row
use, retitle the folder-browser dispatch comment to state the
install_config disambiguation invariant, note the Folder row in
draw_configure's doc comment, dedupe the last byte-identical
display_value copy in serve_wizard, strengthen the cancel-leaves-
prefix-untouched test to actually assert non-clearing behavior, and
drop two manual-testing.md bullets describing a recommended-folder /
pip-cache display the wizard doesn't implement.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Addressed the non-blocking items from the latest automated review (3c05640) in ae97d7a:
format.rs:21-29 — widened the display_or_placeholder doc comment to cover its Channel-row use (not just --prefix), and noted it in the module header.
onboarding.rs:280 — retitled the folder-browser dispatch comment and stated the install_config.is_some() disambiguation invariant explicitly.
onboarding.rs:608 — draw_configure's doc comment now mentions the install-folder row.
serve_wizard.rs:539 — removed the last byte-identical display_value copy; it now calls the shared format::display_or_placeholder.
onboarding.rscancelling_folder_browser_leaves_configure_untouched — the test now pre-populates cfg.prefix before Tab+Esc and asserts it's unchanged, instead of trivially checking it's still empty.
docs/manual-testing.md:71-73 — removed the two stale bullets ("recommended ROCm folder" / pip-cache display) that don't correspond to anything the wizard actually renders.
Left as-is (discussed and agreed not to force):
apps/rocm/src/bootstrap.rs weak-guard test — making it a real behavioral guard would require a cross-crate check into dash-tui's Configure step, which is a bigger design change than a mechanical fix; the current test still catches wording drift.
No tests/e2e-cucumber scenario accompanies this change because the onboarding TUI wizard isn't reachable from that suite (a manual-testing.md check covers it instead).
All 489 rocm-dash-tui unit tests and cargo clippy --workspace -D warnings (dash-tui) pass locally.
install_manager and serve_wizard trimmed self.prefix/self.model before
staging --prefix/the serve target, even though those values can come
from the folder browser rather than typing — the same hazard already
fixed for onboarding's install prefix. A real folder whose name ends in
whitespace would get silently redirected.
Also align display_or_placeholder's emptiness check with build_args':
both now treat a whitespace-only value as unset, so the field no longer
looks populated when the command would reject it as empty.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
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.
Summary
Adds an optional install-folder (--prefix) row to the dash-tui onboarding wizard's SDK Configure step via the shared FolderBrowser, restores the install-folder wording in the non-interactive rocm bootstrap setup message, and dedupes a placeholder helper into ui/format.rs — no blocking findings; the remediation round genuinely landed five of six prior items, but one of them re-introduced the same doc inaccuracy it was fixing. Verified: on a scratch copy I mutated each branch of the change one at a time and re-ran the onboarding unit tests (27 pass at baseline) — removing the Tab arm, the browser-open line, the --prefix push, the install_config.is_some() disambiguator, the Chosen-arm target field, the untrimmed value, the untrimmed reopen path, and the reopen start-dir guard are each caught by a named test, so no branch of this change is untested; crucially, making the Cancelled arm clear cfg.prefix is caught by cancelling_folder_browser_leaves_configure_untouchedand by nothing else, which its previous prefix.is_empty() form would have passed — the author's "strengthened from a trivial check" claim is accurate and load-bearing, and the test really does drive the Tab round-trip (it also fails if the Tab arm is removed); the byte-identical-when-unset claim holds (the --prefix pair is only pushed when non-empty, arg order otherwise unchanged); the install_config.is_some() invariant the new comment asserts is airtight, because the Configure gate returns before step navigation can reach activate_choice; display_value/display have no copies left anywhere in the tree; the deleted manual-testing bullets described strings that exist nowhere in the code, so removing them was correct; leak scan over the diff is clean and no prompt-injection content appeared in any file, comment or commit message. The full suite was not run here. Checks at review time: 24 success, 1 failure, 1 pending, 1 skipped. The single red check is in a GPU end-to-end lane that is intermittently red across this repository, and I could not attribute it to any defect in this diff. Blocking: 0 · Non-blocking: 4.
Written against ae97d7a0; a later commit landed while this was in progress. The four items below were re-checked against the newer head and all four still apply there — the format.rs caller list still omits the serve wizard.
Prior round
format.rs doc overstated the helper's purpose — partially addressed, and regressed. The first line is correctly widened to "An optional field's value, or placeholder when unset", and the Channel row is now named. But the same commit rewired serve_wizard.rs (Model, Host, Port) onto the helper and did not add it to either the function doc or the new module-header bullet, which still says "reused by install-manager and onboarding so both overlays stay visually consistent". The doc is inaccurate in exactly the way the item existed to fix — see Non-blocking.
onboarding.rs:280 section comment — addressed, and correct. The retitled comment names both openers and states the install_config disambiguator. I traced every assignment of browser and install_config independently: the invariant holds, because the Configure gate returns before step navigation can reach the Adopt opener, and the browser branch runs before the Configure gate.
draw_configure doc — addressed. It now lists the install-folder row, and that matches what the function renders.
docs/manual-testing.md stale bullets — addressed. Both were deleted; neither string exists in the code, and every remaining bullet in that section still matches real behaviour.
Third display_value copy — addressed. No copy of the helper survives anywhere in the tree, and the module-header note answers the "does a UI helper belong in a numeric-formatter module" half of the item. display_or_placeholder is genuinely the only non-numeric function there, so that claim is accurate.
Two weak tests + missing scenario rationale — split outcome. The cancel test is now a real behavioural guard (mutation-confirmed above). The bootstrap.rs refusal is reasonable: the test is still tautological, but the author disclosed that accurately ("still catches wording drift"), and making it behavioural would mean asserting across a crate boundary into dash-tui — disproportionate for a printed string. The e2e refusal's stated ground is not accurate — see Non-blocking.
Nothing from the prior round was silently dropped; all six items were touched.
🚫 Blocking (must fix before merge)
None.
Non-blocking
crates/rocm-dash-tui/src/ui/format.rs:17-19,26-30 — the module bullet ("reused by install-manager and onboarding so both overlays") and the function doc's caller list both omit serve_wizard, which the very same commit added as a third caller; the parenthetical "either via the FolderBrowser, or, in install-manager's case, by typing directly" also misses the serve wizard's typed Host/Port fields. Rather than adding a third name that will rot again, delete the caller enumeration and keep only the behavioural contract and the "trimming is a caller-specific decision" note.
The stated reason for shipping no tests/e2e-cucumber scenario — "the onboarding TUI wizard isn't reachable from that suite" — is refuted by tests/e2e-cucumber/features/dash.feature, which drives the real TUI through a pseudo-terminal, sends keystrokes, and whose dash-01 already navigates to "Set up / Install ROCm". The honest framing is "reachable, but would need new step definitions"; dash-11's own comment is the repository's established precedent for pairing a characterization scenario with a named discriminating unit test. Worth correcting the rationale even if the scenario itself stays out of scope.
crates/rocm-dash-tui/src/ui/onboarding.rs:188-190 — build_install_args's doc still says a None pin "leaves the command at --channel <ch> --format wheel", but this PR added a --prefix <path> pair to that same function (and --approve-replacing-active-default was already unlisted). Extend the sentence, or drop the literal command shape and describe the two conditional pairs instead.
crates/rocm-dash-tui/src/ui/onboarding.rs:204 — the gate !cfg.prefix.trim().is_empty() drops a prefix that is entirely whitespace, so a real directory literally named " " would silently install into the default folder — the opposite of what the adjacent comment promises ("never trim the value itself … a real folder name ends in whitespace"). Vanishingly unlikely, but the comment currently over-promises; either note that an all-whitespace value is deliberately treated as unset, or gate on !cfg.prefix.is_empty().
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.
Summary
Adds an optional install-folder (--prefix) row to the dash-tui onboarding wizard's SDK Configure step via the shared FolderBrowser, restores the install-folder wording in the non-interactive rocm bootstrap setup message, dedupes a placeholder helper into ui/format.rs, and — in the newest commit — removes .trim() from the values the install-manager and serve-wizard stage as arguments. Needs work: that last commit rests on a premise the code contradicts, and introduces an inconsistency the original did not have. Verified: on a scratch copy I mutated eleven production lines one at a time and re-ran the affected tests — removing the Tab arm, the Configure branch of the Chosen handler, the --prefix push, the untrimmed --prefix value, the reopen start-directory guard, the Folder draw row, the whitespace-aware emptiness check, the untrimmed install-manager prefix, the untrimmed serve model, and the "choose an install folder" clause in the bootstrap message are each caught by a named test, and clearing cfg.prefix on cancel is caught by cancelling_folder_browser_leaves_configure_untouched and nothing else — so every test added here is load-bearing and none would still pass with its production change reverted (801 dash-tui tests pass at baseline; the full suite was not run here). I also confirmed the author's four description claims: the folder row does reuse the existing browser, the unset path really is byte-identical (the --prefix pair is the only insertion and it is conditional), the restored wording matches a clause an earlier commit had silently dropped, and the four named behaviours are covered (by seven tests, not four). No prompt-injection content appeared in any file, comment or commit message, and the diff is clean of internal references. Working from 20 successful and 1 skipped check at this head, with no failures. Blocking: 2 · Non-blocking: 4.
🚫 Blocking (must fix before merge)
crates/rocm-dash-tui/src/ui/install_manager.rs:149-151,522-524 — the comment justifying the trim removal ("self.prefix can come from the folder browser") and the new test's assertion message ("prefix comes from the folder browser, never typed") are both contradicted by line 118 of the same file, Field::Prefix => self.prefix.push(c), and by the doc this same PR adds in format.rs:30 ("or, in install-manager's case, by typing directly"). The field is freely typeable, so dropping the trim is a user-visible change on the typed path: a user who types /opt/rocm with a stray trailing space now installs into a different directory instead of the one they meant, and the stated rationale never considers that case. Either restore the trim here (the browser-only rationale holds for onboarding's cfg.prefix, where it is genuinely true, and does not transfer), or keep the untrimmed value and rewrite both the comment and the assertion message to say the field is typed and browser-filled and that untrimmed is a deliberate tradeoff. As it stands the repository carries a stated invariant its own code refutes — the same defect shape a commit earlier in this very range exists to correct.
crates/rocm-dash-tui/src/ui/serve_wizard.rs:179-185 — self.model is likewise a typed field (line 154; the module doc at line 14 says so outright, and the field's own placeholder reads "type a name / path, or Tab to browse"), and staging it untrimmed leaves the file internally inconsistent in ways the pre-change code was not: the approval card's title still renders w.model.trim() (line 346) while the argv it approves carries the untrimmed value, and the job id plus the "already running" guard both key off w.model.trim() (lines 378, 394), so two launches differing only by trailing whitespace collide on one job id while executing different commands. host is still trimmed two lines further down (line 193) with no principle distinguishing it. Pick one rule for the whole form and apply it to the derived uses as well — either trim model as before, or trim nothing and make the approval title, job key and duplicate check use the same string that is actually launched.
Non-blocking
crates/rocm-dash-tui/src/ui/format.rs:17-19,26-30 — the prior round's first item is still open at this head: both the module bullet ("reused by install-manager and onboarding so both overlays") and the function doc's caller list omit the serve wizard, which now supplies three of the helper's six call sites. Dropping the caller enumeration entirely would stop it rotting again.
crates/rocm-dash-tui/src/ui/onboarding.rs:189-191 — likewise still open: build_install_args's doc claims a None pin "leaves the command at --channel <ch> --format wheel", which this PR made untrue by adding a conditional --prefix pair to that same function (--approve-replacing-active-default was already unlisted).
crates/rocm-dash-tui/src/ui/onboarding.rs:421 — the new Tab arm sits outside configure_key's if let Some(cfg) = o.install_config.as_mut() guard, so it relies entirely on the single caller having checked install_config.is_some(). It is safe today, but if that ever changes the browser opens with no Configure behind it and the chosen path is staged as an adopt instead; moving the check inside, or noting the caller-enforced invariant, closes it cheaply.
docs/manual-testing.md:71 — the bullet says "the Install ROCm SDK step", while the on-screen menu entry is "Install ROCm SDK (pip)"; also, no Gherkin scenario accompanies this user-observable change and the contributor rules ask for one or an explicit justification — the existing setup scenario stops short of the Configure step, so if the justification lives only in the PR text it is not visible from the tree.
The reason will be displayed to describe this comment to others. Learn more.
There's already a review here from siloteemu flagging two real bugs — I re-checked both against the surrounding code and they hold up, so I won't repeat them: the untrimmed install_manager prefix contradicting its own "browser-only" doc comment/test message, and the same issue in serve_wizard's model field where the approval title, job-id derivation, and duplicate-launch guard still use the trimmed value while build_args() now pushes untrimmed. Its four non-blocking notes (format.rs caller list, the build_install_args doc comment, the ungated new Tab arm, and the manual-testing.md wording) also stand as written.
On top of that, one more thing worth fixing before merge, plus a couple of things I'd just flag for awareness:
The PR silently narrows what Tab does for existing users. Before this change, configure_key's catch-all arm matched KeyCode::Left | KeyCode::Right | KeyCode::Tab to toggle the channel. This PR pulls Tab out into its own earlier arm that opens the folder browser instead, leaving the toggle arm with just Left | Right (inline comment below). That's a real behavior change for anyone who was using Tab to cycle channel on the Configure step, and it isn't mentioned anywhere in the PR body or in issue #75 — it reads like an accidental side effect of claiming the Tab key for the new browser, not a deliberate decision.
Scope is bigger than the PR body says. The Summary claims this only touches onboarding.rs, bootstrap.rs, and "the one docs/manual-testing.md bullet," but three of the eight commits substantially rewrite install_manager.rs and serve_wizard.rs too, and a later commit drops two more manual-testing.md bullets (the recommended-ROCm-folder and pip-cache display lines) beyond the one called out — so it's three bullets changing, not one. Issue #75 scoped this work narrowly and explicitly left the channel-toggle behavior to a separate issue (#73), so it'd be good to either fold the install_manager/serve_wizard fixes into the PR description honestly or split them out.
Minor, not blocking: the same "don't trim a browser-picked path" fix had to land as three separate commits across onboarding.rs, install_manager.rs, and serve_wizard.rs, each with its own copy of the guard. Given the PR itself dedupes the display-formatting logic into one format::display_or_placeholder helper, doing the same for this guard would avoid needing a fourth near-identical patch the next time a browser-fed field grows the same bug.
Both blocking findings from @siloteemu's review, endorsed by @juhovainio, were checked against the code before acting. Both hold, and neither was a false positive.
Blocking 1 — install_manager.rs: the stated invariant was false
The comment and test message added in 39b8592 claimed the prefix "comes from the folder browser, never typed". install_manager.rs:118 is Field::Prefix => self.prefix.push(c) — the field is freely typeable, and this PR's own format.rs doc said so two files away. The browser-only rationale was borrowed from onboarding, where it is genuinely true, and does not transfer.
Took the first of the two options offered: restored the trim. Dropping it was a user-visible regression on the typed path — type /opt/rocm with a stray trailing space and you'd install somewhere else.
Blocking 2 — serve_wizard.rs: the untrimmed model split argv from everything derived from it
Same false premise (serve_wizard.rs:154 types into self.model, and the module doc at :14 says so), plus the internal inconsistency you identified: the approval title (:346), the job id (:377-383) and the already-running guard (:394) all key off model.trim() while build_args staged the untrimmed value. Two launches differing only by trailing whitespace would collide on one job id while running different commands. host and port were still trimmed with no principle separating them. Restored the trim here too.
The rule, stated once so it stops being re-litigated
Byte-exact where the folder browser is the only writer; trimmed where a human types. Onboarding's cfg.prefix has no typing path, so it stays exact. install_manager's prefix and serve_wizard's model are both, so both are trimmed. The comment in install_manager now names the cost out loud rather than implying there is none: a directory whose name legally ends in whitespace, picked through the browser, will be staged trimmed. That is accepted because the same whitespace is far more often a typo on a field a user types into.
This also makes @juhovainio's last point moot — with only onboarding keeping the untrimmed rule there is no guard to deduplicate across three files.
Non-blocking notes
Note
Done
format.rs caller list omits the serve wizard
Enumeration dropped entirely, per your suggestion, rather than extended a third time
Kept, documented in the PR body, pinned by a new test — replied in the thread, left open for your call
Scope is bigger than the PR body says (@juhovainio)
PR description rewritten with a "Scope beyond the onboarding change" section naming format.rs, the two call-site files, the three doc bullets, and the Tab reassignment
Two things I did not do, deliberately
1. No Gherkin scenario, and this remains a real gap. AGENTS.md §3 wants one for user-observable behaviour. The honest position, replacing the vaguer "a TUI wizard isn't reachable from that suite": it is reachable — dash.feature drives the real crossterm loop through a pseudo-terminal, so a scenario covering launcher → Install ROCm SDK → Tab → folder browser is feasible, and the bootstrap setup no-TTY message is plain piped output that is easier still. Both are written up as an unclosed gap in the PR body. The bootstrap.rs unit test is a weak guard and is acknowledged as one — it pins a constant against itself and would still pass if the folder choice it advertises were removed from the wizard.
2. I could not compile or test. This change was authored in a sandbox with no Rust toolchain and no network route to install one, so cargo test, clippy and build were not run against dbc4a8c. It was reviewed by reading instead — types, the disjoint-field borrow in the new Tab arm, and each new test traced by hand against the production path — and existing tests were checked for ones depending on the reverted behaviour. That is not the same as a green run. Please treat CI as the first real verification, not this comment. If something fails to compile, that is why, and it is on this change rather than on the branch it landed on.
No reviewer threads were resolved — per AGENTS.md §11 these aren't mine to close.
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.
Summary
Adds an optional install-folder (--prefix) row to the dash-tui onboarding wizard's SDK Configure step via the shared FolderBrowser, restores the install-folder wording in the non-interactive rocm bootstrap setup message, and dedupes a placeholder helper into ui/format.rs. No blocking findings — both objections we were holding are discharged by the code at this head, and the four non-blocking items from the last round are each closed. Verified, item by item: (1) the install-manager prefix is trimmed again (install_manager.rs:163), so build_args is byte-identical to the base version apart from its comment, and the comment and the new test's assertion message now both say the field is typed and browser-filled and state the tradeoff — the stated invariant no longer contradicts type_char at line 118; (2) the serve-wizard model is trimmed again (serve_wizard.rs:186), so the argv, the approval title, the job id and the duplicate-launch guard all derive from the same string, host/port follow the same rule, and that build_args is likewise byte-identical to base apart from its comment; (3) format.rs no longer enumerates its callers; (4) build_install_args's doc was rewritten and I confirmed the ordering and conditions it claims hold on every path through the function; (5) the Tab arm now sits inside its own install_config guard with the disambiguation invariant written down; (6) the manual-testing bullet now uses the exact on-screen label. On a scratch copy I mutated the four production lines this round's new or rewritten tests name — the install-manager trim, the serve-wizard trim, the Tab browser arm, and a Tab-also-toggles-channel regression — and each is caught by the named test and, for the channel case, by that test alone; cargo test -p rocm-dash-tui passes at baseline (833 tests across the crate's targets) and cargo fmt --check is clean. The full suite and the end-to-end suite were not run here. Checks at review time: 20 success, 1 skipped, no failures. Blocking: 0 · Non-blocking: 4.
🚫 Blocking (must fix before merge)
None.
Non-blocking
crates/rocm-dash-tui/src/ui/format.rs:31-32 — the replacement doc says whitespace-aware emptiness means "a row never looks populated while the command would reject the value as empty"; that holds for the two required fields (install-manager channel, serve-wizard model) but the other four callers omit an optional flag rather than rejecting, so "reject" overstates it — "ignore" or "treat as unset" would be exact.
apps/rocm/src/bootstrap.rs — the added test asserts a substring of the constant against the constant, so it would still pass if the folder choice it advertises were removed from the wizard; the PR text acknowledges this accurately, and it is still the only guard on that wording.
No scenario under tests/e2e-cucumber/features/ accompanies this user-observable change; the contributor rules ask for one or an explicit justification, and the PR text now gives one — but it also says the PTY harness could reach the Configure folder row, so the justification is "not written here", not "not reachable", and it lives only in the PR description rather than anywhere in the tree.
The PR description's verification-gap paragraph still names the previous commit as the latest one, so as written it no longer describes this head; the rustfmt commit on top of it also reports a cargo fmt --check result, which reads oddly against "no toolchain was available".
Both objections are discharged at 2ecd47d. The install-manager and serve-wizard values are trimmed again, so both argument builders are byte-identical to the base version apart from their comments, and the comments no longer state an invariant the code contradicts. Verified by mutating each production line the new tests name; each is caught by its named test. Withdrawing; the remaining points are non-blocking and are in the comment.
Addressed two of the four non-blocking notes from the latest automated review (2ecd47d):
format.rs:31-32 — reworded: unset input is now described as "rejected outright" only for the two required fields (install-manager channel, serve-wizard model), "silently omitted" for the rest, instead of implying every caller rejects it.
Verification-gap paragraph — now names the actual latest commit at review time, 2ecd47d7, and states plainly that its cargo fmt --check pass was a formatting-only cleanup that doesn't extend to the substantive dbc4a8c commit, which still hasn't had cargo test/clippy/build run against it.
Left as-is, per the PR text already covering them: the bootstrap.rs weak-guard test, and the missing e2e-cucumber scenario — both acknowledged gaps, not silently dropped.
cargo fmt --check -p rocm-dash-tui passes on the new commit.
The reason will be displayed to describe this comment to others. Learn more.
All three points from my earlier CHANGES_REQUESTED review are resolved:
The silent Tab-behavior narrowing is now deliberate and documented: the PR body calls it out explicitly, the code carries an inline comment explaining why Tab can't do both jobs, and tab_browses_instead_of_toggling_the_channel pins the split. The configure_key change also now gates the Tab-browse arm on install_config.is_some(), fixing a bug I hadn't flagged (opening the browser with install_config unset would have silently staged the pick as an adopt-path folder instead).
The PR body now has a full "Scope beyond the onboarding change" section covering format.rs, the manual-testing.md bullet removals, and the Tab reassignment.
My trim-dedupe suggestion turned out moot: the design correctly diverged instead. onboarding's prefix stays byte-exact (only the browser writes it), while install_manager's prefix and serve_wizard's model are now trimmed to match the job-id/approval-title/duplicate-guard derivation that already used .trim() — which also fixes the two real bugs siloteemu and I originally flagged (untrimmed value contradicting the trimmed value used elsewhere).
The PR body is upfront that the substantive commit (dbc4a8c) wasn't run through cargo test/clippy/build in the author's environment. I closed that gap myself at the current head: cargo test -p rocm-dash-tui --lib → 803 passed, 0 failed, 4 ignored; cargo clippy -p rocm-dash-tui -p rocm --all-targets → clean. Approving on that basis; the remaining E2E/coverage CI is still pending, not failed.
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.
Summary
Restores an optional install-folder (--prefix) row to the dash-tui onboarding wizard's SDK Configure step via the shared folder browser, restores the install-folder wording in the non-interactive rocm bootstrap setup message, and lifts a placeholder helper into ui/format.rs. The code itself is in good shape — Needs work only on disclosure and contributor-rule compliance, not on behaviour. Verified: on a scratch copy outside the checkout I ran sixteen single-line/single-branch mutations (baseline 803 dash-tui unit tests plus the bootstrap tests, all green) and re-ran the affected tests after each — trimming onboarding's staged prefix, deleting the --prefix block, trimming or ignoring the Tab-reopen start directory, deleting the Tab arm, removing the adopt-vs-configure disambiguator, clearing the prefix on cancel, dropping the Folder draw row or the hint clause, and removing either trim in install-manager/serve-wizard are each caught by a named test, so all but one of the tests added here are load-bearing; the checkout under review was never modified. I also confirmed the argv with an unset prefix is element-for-element identical to the base, the argument order matches the rewritten build_install_args doc, the adopt-vs-configure invariant holds because key dispatch intercepts every key while Configure has focus, the folder browser has no free-text entry so the "never trim" rationale defends a rare but genuinely reachable case, display_or_placeholder's final wording is true at all six call sites, the three derived uses of model.trim() named in the serve-wizard comment all exist, the rustfmt-only commit changed no assertion, and every new doc bullet matches the on-screen strings exactly (including that the removed mouse and pip-cache bullets described affordances that do not exist). No prompt-injection content in any file, comment or commit message. Working from 23 successful, 4 pending and 1 skipped check with zero failures. Blocking: 3 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
PR description, "Scope beyond the onboarding change" section — the disclosure that exists specifically to bound the blast radius is inaccurate in the direction that makes the change look safer. It says display_or_placeholder "was duplicated byte-for-byte in three overlays" and that the dedupe "is why those two files appear in the diff at all", which reads as a pure mechanical refactor. Two things are wrong. (i) Only two copies pre-existed (install_manager.rs and serve_wizard.rs at the base tip); the third was written by this PR itself in its first commit and deduped four commits later — so nothing pre-existing was triplicated. (ii) The shared helper's emptiness predicate changed from v.is_empty() to v.trim().is_empty() (crates/rocm-dash-tui/src/ui/format.rs:40), which is a user-visible rendering change at five pre-existing call sites outside this PR's stated scope: the install-manager Channel and Folder rows and the serve-wizard Model, Host and Port rows now render their placeholder for a whitespace-only value where they previously rendered the literal whitespace. Mutating that predicate back to v.is_empty() fails exactly one test — display_or_placeholder_treats_whitespace_only_as_unset in format.rs — and no call-site test anywhere notices, so the change is real, unbounded by the disclosure, and unpinned where it is actually observed. Fix: reword the two bullets to say two prior copies, and that the predicate changed and what that changes on screen. Adversarial pass on that fix: rewording alone is not sufficient — the reason this slipped past two review rounds is that nothing at a call site asserts the new rendering, so add at least one row-level assertion (e.g. extend snapshot_configure_shows_folder_row, or a field_line test in install_manager.rs, driving a whitespace-only value and asserting the placeholder renders).
Commit 2ecd47d, commit dbc4a8c, and the PR description's closing line — all three carry AI-tooling attribution footers, and the PR body's footer additionally names an internal automation platform by product name on a public surface. The base-tip contributor rules forbid both independently: §11 says to avoid AI-generated boilerplate footers, and §2 forbids internal or proprietary product names in PR bodies and commit messages. Nine of the eleven commits in the range have no such trailer, so this was introduced by the latest remediation round rather than being the repo's habit. Fix: edit the PR body to drop the footer (cheap, do this first — it is the one that is publicly visible), and drop the trailer from the two commits. Adversarial pass: the commit half is not a free fix — it needs a history rewrite and a force-push over commits that have already been reviewed, which §11 itself governs; say on the PR what changed and why, or agree with a maintainer to leave the trailers and fix only the body.
No end-to-end scenario for a user-observable change — the base-tip contributor rules §3 require behaviour a CLI user can observe to be covered by a scenario under tests/e2e-cucumber/features/, and offer exactly three escapes: name an existing scenario's identifier and say the change makes it pass; say the scenario can only run on a gated lane and name the lane; or say the change is purely internal. This PR adds a keyboard affordance and a rendered row to an interactive wizard and changes a printed CLI message, adds no scenario, and takes none of the three escapes — it offers a fourth, that the authoring environment had no toolchain, which §3 does not sanction. The rule's own "not feasible" mitigation is also unavailable here: the existing pseudo-terminal harness genuinely drives the dash TUI and can write arbitrary keystrokes and wait on arbitrary screen text, and an existing scenario already reaches the setup-actions screen one step short of Configure, so the scenario is reachable. The PR is admirably honest that this is an open gap — but honest disclosure is not one of the escapes. Fix: add a scenario that opens the launcher, enters the SDK step, presses the browse key and asserts the folder browser appears, plus one asserting the no-terminal message; if they cannot be run locally, say so in the PR text and let CI be the evidence, which §8 explicitly permits. Adversarial pass: the naive fix is insufficient if the scenario only asserts that the browser opened — that would not cover the staged --prefix, which is the actual behaviour restored; assert on the resulting command or the Folder row's rendered value too.
Non-blocking
crates/rocm-dash-tui/src/ui/onboarding.rs:209 — the whitespace-only guard is the one unpinned branch in the change: weakening !cfg.prefix.trim().is_empty() to !cfg.prefix.is_empty() fails no test, unlike the equivalent guards in install-manager and the display helper, which both have one.
crates/rocm-dash-tui/src/ui/onboarding.rs:456-459 — the comment says re-adding the browse key to the channel arm is pinned by tab_browses_instead_of_toggling_the_channel; it is not. Re-adding it there passes every test, because the explicit arm above shadows it — the test pins the split in the other direction only. Either say the arm above makes it unreachable, or drop the claim.
crates/rocm-dash-tui/src/ui/install_manager.rs:250 and crates/rocm-dash-tui/src/ui/serve_wizard.rs:318 — undocumented divergence between the three sibling forms the new comments compare themselves to: onboarding reopens the browser at the already-chosen prefix, while these two always restart at the process working directory and discard the prior pick. Pre-existing and untouched here, but the comments now assert a shared rule that does not extend to this path.
crates/rocm-dash-tui/src/ui/serve_wizard.rs:629 — the test name claims the argv matches the job id and approval title, but it only calls build_args; nothing drives a whitespace-padded model through the launch path, so the three derived uses the comment relies on are asserted only in a message string.
apps/rocm/src/bootstrap.rs:21,39 — mutation confirms the acknowledged weakness: removing the folder row from the wizard entirely leaves this test passing, so the doc's "must keep advertising … the onboarding Configure step" is a cross-module guarantee nothing enforces; the const is also used once, so the hoist buys only the tautology.
Requesting changes on three items. The underlying code is in good shape — every one of these is about disclosure or contributor-rule compliance rather than about the behaviour the change implements, and sixteen single-branch mutations run on a scratch copy confirmed that almost all of the tests added here are load-bearing. The remaining non-blocking notes are in the round comment posted alongside this.
1. A user-visible rendering change at five pre-existing call sites, outside the stated scope and unpinned where it is observed.
The shared placeholder helper's emptiness predicate changed from an emptiness check to a trimmed emptiness check. That is not a mechanical dedupe: five call sites that this change does not otherwise touch — two rows in the install manager and three in the serve wizard — now render the placeholder for a whitespace-only value where they previously rendered the literal whitespace. Mutating the predicate back fails exactly one test, the helper's own, and no call-site test anywhere notices.
The description's scope section is also inaccurate in the direction that makes the change look safer: it says the helper was duplicated byte-for-byte in three overlays, but only two copies pre-existed; the third was written by this pull request in its first commit and deduped four commits later, so nothing pre-existing was triplicated.
Fix: correct the two bullets to say two prior copies and to state that the predicate changed and what that changes on screen. Rewording alone is not sufficient — the reason this went unnoticed across two review rounds is that nothing at a call site asserts the new rendering. Add at least one row-level assertion driving a whitespace-only value and asserting the placeholder renders.
2. Attribution footers, one of which puts an internal product name on a permanently public surface.
Two commits in the range and the pull request description's closing line all carry generated-tooling attribution footers, and the description's footer additionally names an internal automation platform by product name. The contributor rules at the base tip forbid these independently: one section asks that generated boilerplate footers be left out, and another forbids internal or proprietary product names in pull request bodies and commit messages. Nine of the eleven commits in the range carry no such trailer, so this arrived with the most recent round rather than being the project's habit.
Deliberately not repeating the name here. Fix the description first — it is the publicly visible half, it is the one that carries the internal name, and it costs one edit. The commit half is not a free fix: it needs a history rewrite and a force-push over commits that have already been reviewed. Either do that and say on the pull request what changed and why, or agree with a maintainer to leave the trailers and correct the body alone.
3. A user-observable behaviour change with no end-to-end scenario, and none of the escapes the rule offers.
The contributor rules require behaviour a command-line user can observe to be covered by a scenario, and offer exactly three escapes: name an existing scenario and say the change makes it pass, say the scenario can only run on a gated lane and name that lane, or say the change is purely internal. This adds a keyboard affordance and a rendered row to an interactive wizard and changes a printed message. It adds no scenario and takes none of the three escapes — it offers a fourth, that the authoring environment lacked a toolchain, which the rule does not sanction.
The rule's own not-feasible mitigation is also unavailable: the existing pseudo-terminal harness genuinely drives this interface, can write arbitrary keystrokes and wait on arbitrary screen text, and an existing scenario already reaches the screen one step short of the one in question. The pull request is admirably honest that this is an open gap, but honest disclosure is not one of the escapes.
Fix: add a scenario that opens the launcher, enters the step, presses the browse key and asserts the browser appears, plus one asserting the no-terminal message. If they cannot be run where the change was written, say so in the pull request text and let the pipeline be the evidence — the rules explicitly permit that. The naive version of this fix is insufficient: a scenario that only asserts the browser opened would not cover the staged prefix, which is the behaviour actually being restored. Assert on the resulting command, or on the rendered row's value, as well.
jussielo-amd
added a commit
to jussielo-amd/rocm-cli
that referenced
this pull request
Sep 30, 2026
…essage
AGENTS.md sec3 asks for a Gherkin scenario for user-observable behaviour;
ROCm#75/ROCm#441 had none. Add two:
- dash-onboarding-configure-folder-browse: opens onboarding from the
Observe tab, advances to the SDK Configure step, presses Tab to open
the install-folder browser, confirms its default "use this folder"
entry, and asserts the Configure step's Folder row then shows the
chosen path rather than the unset placeholder -- a browser-opened-only
assertion would not cover the staged --prefix, which is the behaviour
actually restored.
- bootstrap-setup-no-tty-advertises-install-folder (new bootstrap.feature):
runs `rocm bootstrap setup` piped, matching every CI/script invocation,
and asserts the printed message mentions choosing an install folder.
Both run on the existing mock/no-GPU lane. Verified against the built
rocm binary, including that each fails under its own regression: the
folder-browser scenario fails if Tab is prevented from opening the
browser, mirroring the existing display_or_placeholder mutation check.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
The previous commit removed .trim() from install_manager's prefix and
serve_wizard's model on the grounds that both "come from the folder browser,
never typed". Both are in fact typed: install_manager.rs's type_char pushes
into self.prefix, and serve_wizard's module doc and placeholder advertise the
model field as free text. The rationale did not transfer from onboarding, and
in serve_wizard it also split the argv from the three values still derived
from model.trim() - the approval title, the job id, and the already-running
guard - so two launches differing only by trailing whitespace would share one
job id while running different commands.
Restore the trim in both, and state the rule the crate actually follows:
byte-exact where the folder browser is the only writer (onboarding's prefix),
trimmed where a human types. The accepted tradeoff is named in the comment
rather than left implicit.
Also addresses the non-blocking review notes: drop the caller enumeration from
display_or_placeholder's docs instead of letting it rot again, correct
build_install_args' doc comment to list --prefix and
--approve-replacing-active-default, gate onboarding's Tab arm on install_config
so it cannot open a browser whose choice would be staged as an adopt, name the
Configure step as it appears on screen in manual-testing.md, and pin the
Tab-browses / arrows-toggle-channel split with a regression test.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
cargo fmt --check flagged two assert_eq! call sites in the previous
commit: the expected-value argument in install_manager's trim test, and the
chained accessor in onboarding's Tab/channel test. Applied rustfmt's own
output; no behaviour change.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
The doc claimed unset input is always "rejected" by the downstream
command, but that's only true for the two required fields
(install-manager channel, serve-wizard model). The other callers just
omit an optional flag. Flagged by @siloteemu's review.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
display_or_placeholder's emptiness check moved from v.is_empty() to
v.trim().is_empty() when it was deduped into format.rs, which is a real
rendering change at five pre-existing call sites (install-manager's
Channel and Folder rows, serve-wizard's Model/Host/Port rows): a
whitespace-only value now renders the placeholder instead of the
literal whitespace. Nothing at a call site asserted that, so mutating
the predicate back only failed the helper's own unit test.
Add a snapshot test driving InstallManagerState with a whitespace-only
channel and prefix and asserting both rows render their placeholder.
Confirmed load-bearing: reverting the predicate to v.is_empty() fails
this test.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Addressed two of the three blocking items from the latest automated review (dae18cf4):
PR description accuracy — corrected the "Scope beyond the onboarding change" section: format.rs had two pre-existing copies (not three), and the dedupe's v.is_empty() → v.trim().is_empty() predicate change is now called out explicitly, including the five call sites it affects. Also dropped the closing footer (AI-tooling attribution + an internal automation-platform name — the latter should never have been on a public surface).
Missing regression test at a call site — install_manager::tests::whitespace_only_channel_and_prefix_render_placeholders (7d3f388) pins the predicate change where it's actually observed, not just in format.rs's own unit test. Confirmed load-bearing: reverting the predicate to v.is_empty() fails it.
Gherkin coverage — added two scenarios (a5b5984): @id:dash-onboarding-configure-folder-browse drives the real onboarding wizard through a PTY — opens Configure, presses Tab, confirms a folder, and asserts the Folder row shows the chosen path (not just that the browser opened, which wouldn't cover the staged --prefix); @id:bootstrap-setup-no-tty-advertises-install-folder (new bootstrap.feature) covers the piped non-interactive message. Both pass locally against the built binary; each was checked to fail under its own regression.
Still open, needs a call: commits 2ecd47d/dbc4a8c carry a Co-Authored-By: Claude Opus 5 (1M context) trailer the contributor rules ask not to have. Fixing it means rewriting and force-pushing over commits you've already reviewed. I've prepared the rewrite locally (trailer-only change, tree byte-identical, re-signed and re-signed-off) but have not pushed it — that needs your explicit go-ahead per this repo's force-push policy. Alternatively, per the review's own offered escape, we leave the trailers and this comment stands as the record of what changed and why.
jussielo-amd
added a commit
to jussielo-amd/rocm-cli
that referenced
this pull request
Sep 30, 2026
…essage
AGENTS.md sec3 asks for a Gherkin scenario for user-observable behaviour;
ROCm#75/ROCm#441 had none. Add two:
- dash-onboarding-configure-folder-browse: opens onboarding from the
Observe tab, advances to the SDK Configure step, presses Tab to open
the install-folder browser, confirms its default "use this folder"
entry, and asserts the Configure step's Folder row then shows the
chosen path rather than the unset placeholder -- a browser-opened-only
assertion would not cover the staged --prefix, which is the behaviour
actually restored.
- bootstrap-setup-no-tty-advertises-install-folder (new bootstrap.feature):
runs `rocm bootstrap setup` piped, matching every CI/script invocation,
and asserts the printed message mentions choosing an install folder.
Both run on the existing mock/no-GPU lane. Verified against the built
rocm binary, including that each fails under its own regression: the
folder-browser scenario fails if Tab is prevented from opening the
browser, mirroring the existing display_or_placeholder mutation check.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Rewrote 2ecd47d/dbc4a8c to drop the Co-Authored-By trailer (force-pushed, tree byte-identical — new hashes a536b07/5885332). All three blocking items from dae18cf4 are now addressed.
…essage
AGENTS.md sec3 asks for a Gherkin scenario for user-observable behaviour;
ROCm#75/ROCm#441 had none. Add two:
- dash-onboarding-configure-folder-browse: opens onboarding from the
Observe tab, advances to the SDK Configure step, presses Tab to open
the install-folder browser, confirms its default "use this folder"
entry, and asserts the Configure step's Folder row then shows the
chosen path rather than the unset placeholder -- a browser-opened-only
assertion would not cover the staged --prefix, which is the behaviour
actually restored.
- bootstrap-setup-no-tty-advertises-install-folder (new bootstrap.feature):
runs `rocm bootstrap setup` piped, matching every CI/script invocation,
and asserts the printed message mentions choosing an install folder.
Both run on the existing mock/no-GPU lane. Verified against the built
rocm binary, including that each fails under its own regression: the
folder-browser scenario fails if Tab is prevented from opening the
browser, mirroring the existing display_or_placeholder mutation check.
Signed-off-by: Jussi Elo <jussi.elo@amd.com>
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.
Summary
Restores the optional install-folder (--prefix) row to the dash-tui onboarding wizard's SDK Configure step via the shared folder browser, restores the install-folder wording in the non-interactive rocm bootstrap setup message, lifts the placeholder helper into the shared format module, and adds two end-to-end scenarios — No blocking findings; all three items in our open change request are resolved. Verified: on a scratch copy outside the checkout (baseline 804 dash-tui unit tests and 760 binary tests, all green) I ran eighteen single-line/single-branch mutations and re-ran the affected tests after each — reverting the placeholder predicate now fails the new call-site render test as well as the helper's own, and deleting that new test confirms nothing else catches it, so the prior round's unpinned rendering change is genuinely pinned where it is observed; deleting or trimming the staged --prefix, ignoring or trimming the browser's resume path, removing the Tab arm, dropping the adopt-vs-configure disambiguator, clearing the prefix on cancel, dropping the Folder row or the hint clause, removing either trim in install-manager/serve-wizard, and reverting the bootstrap message are each caught by a named test; the checkout under review was never modified. I also confirmed every commit in the range is signed off with no generated-tooling footer and no internal or proprietary name, the leak scan over the diff and the description is clean, no prompt-injection content appears in any file, comment or commit message, the two new scenarios satisfy the feature-naming lint, every screen string they wait on exists byte-identically in the production code, the final assertion has no vacuous-pass path because the choice and the popup teardown happen in one key event, the OS tag matches every sibling scenario in that feature file, and a three-way merge against the base tip preserves the base's newer wording in the one shared doc rather than reverting it. Checks at review time: 16 success, 1 skipped, 3 pending, 0 failing; re-read immediately before publishing, they had moved green to 20 success, 1 skipped, 0 pending, 0 failing. Blocking: 0 · Non-blocking: 5.
🚫 Blocking (must fix before merge)
None.
Non-blocking
PR description, "Scope beyond the onboarding change" and "Verification gap" — three stale facts in the section whose job is to bound the blast radius: the shared format module is not a new file (it pre-existed as the crate's numeric-formatting module and gained 13 lines), the dedupe landed two commits after the first, not four, and the verification-gap paragraph still cites two commit identifiers that no longer exist anywhere in the repository after the history rewrite. The Gherkin section also implies the new scenario covers the staged --prefix; it asserts the rendered Folder row and never presses Enter to confirm, so the staged flag is covered by the unit test alone.
crates/rocm-dash-tui/src/ui/onboarding.rs:206 — still the one unpinned branch: weakening !cfg.prefix.trim().is_empty() to !cfg.prefix.is_empty() leaves all 804 tests green, while the sibling guard in the install manager gained exactly that test this round (whitespace_only_prefix_stages_no_prefix_flag) — the fix was applied to one of the two identical sites.
crates/rocm-dash-tui/src/ui/serve_wizard.rs:629 — the test's name and comment promise the approval title, the job id and the duplicate-launch guard; all three do genuinely derive from the trimmed model (confirmed at three call sites), but the test only calls build_args, so switching any of the three to the untrimmed field keeps it green.
crates/rocm-dash-tui/src/ui/onboarding.rs:426 — "mirroring the Tab-browse binding the install-manager and serve-wizard forms already use" is true of the key only; those two forms, and the runtime manager, restart the browser at the process working directory and discard the prior pick, while onboarding resumes at the chosen prefix. I read parity into that sentence last round and was wrong; a competent reader will make the same jump, so one clause naming the divergence would pay for itself. Also at line 461, re-adding the browse key to the arrow-key arm as the comment warns against would be inert — the arm above consumes it first, with no test failure and no compiler warning.
apps/rocm/src/bootstrap.rs:9 — the constant's doc still asserts a cross-module guarantee nothing enforces, and the unit test asserts the string against itself; the new piped scenario does now prove the real binary prints it, so the only residual gap is the tie back to the wizard's folder row.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
siloteemu
left a comment
juhovainio
left a comment
Summary
Restores the install-folder (
--prefix) choice that PR #67 dropped when it retired the legacytui.rsand rebuilt onboarding as a minimal wizard. The deferral was tracked rather than intended — see #75.crates/rocm-dash-tui/src/ui/onboarding.rs) grows a Folder row.Tabopens the existingFolderBrowser(the same one the Adopt-existing-folder path uses) to pick an install location; confirming includes--prefix PATHon the stagedrocm install sdk. Leaving it unset keeps the default managed folder, byte-identical to today.rocm bootstrap setup's non-interactive fallback message (apps/rocm/src/bootstrap.rs) advertises the install-folder choice again, wording Retire legacy tui.rs: routerocm bootstrap setupto the dash onboarding, then delete the ~45k-line module #67 had dropped along with the picker.docs/manual-testing.md— the onboarding bullets are rewritten to match what the wizard actually does.Why this way: the CLI plumbing (
rocm install sdk --prefix) and the UI pattern (FolderBrowser+ a prefix field) already exist and are exercised by the Install overlay and the Adopt path, so this wires the established pattern into the Configure step rather than inventing a new one.Risk: low. The unset path is byte-identical —
--prefixis the only insertion and it is conditional.Scope beyond the onboarding change — please read
Review rounds pulled in work outside the one-file scope #75 describes. Calling it out rather than leaving it to be discovered in the diff:
crates/rocm-dash-tui/src/ui/format.rsis new. Two copies ofdisplay_or_placeholderpre-existed, byte-for-byte, ininstall_manager.rsandserve_wizard.rs; a third was written by this PR's own first commit (for the new onboarding Folder row) and deduped into this shared module four commits later — so nothing pre-existing was triplicated, and this PR is what introduced the third copy in the first place. That is whyinstall_manager.rs/serve_wizard.rsappear in the diff at all: they now call the shared helper instead of their own copy.v.is_empty()tov.trim().is_empty(), which is a real rendering change at the five pre-existing call sites this PR does not otherwise touch:install_manager's Channel and Folder rows, andserve_wizard's Model, Host, and Port rows now render the placeholder for a whitespace-only value, where they previously rendered the literal whitespace.install_manager::tests::whitespace_only_channel_and_prefix_render_placeholders(new) pins this at a call site, not only in the helper's own unit test — it fails if the predicate reverts tov.is_empty(), and no other test would have caught that.docs/manual-testing.mdloses two bullets it previously had, describing a recommended-ROCm-folder line and adownloads stay inside: <ROCm folder>\pip-cachedisplay. Neither string exists anywhere in the onboarding code — they documented a flow the wizard does not implement.Tabno longer cycles the Release/Nightly channel on the Configure step. feat(dash): restore Release/Nightly channel toggle + version pin in onboarding #73 boundTabthere alongside←/→; this PR reassigns it to the folder browser, because Tab-opens-a-path-browser is what the Install and Serve forms already do and one key cannot do both.←/→remain the channel keys and are what the on-screen hint has always advertised, so no documented affordance changes — but the feat(dash): restore Release/Nightly channel toggle + version pin in onboarding #73 binding does go away, deliberately, andtab_browses_instead_of_toggling_the_channelpins the split. Flagging it because Restore install-folder (--prefix) choice in the dash onboarding wizard #75 explicitly left channel behaviour to feat(dash): restore Release/Nightly channel toggle + version pin in onboarding #73.cfg.prefixhas no typing path, so it is passed through exactly — a directory whose name legally ends in whitespace is not silently redirected.install_manager's prefix andserve_wizard's model are both typed and browser-filled, so both stay trimmed as they always were. Inserve_wizardthat is load-bearing: the approval title, the job id and the duplicate-launch guard all derive frommodel.trim(), so an untrimmed argv would let two launches share one job id while running different commands.Test plan
cargo test -p rocm-dash-tui— covers, with one named test per branch:Tabopens the browser from Configure; choosing a folder sets the prefix without staging an approval; confirming after a choice stages--prefix; cancelling leaves an already-chosen prefix intact; the onboarding prefix survives a trailing space unchanged;install_manager/serve_wizardtrim theirs;Tabbrowses without touching the channel while←/→still toggle it; a whitespace-only channel/prefix render the placeholder at a call site.cargo clippy -p rocm-dash-tui -p rocm --all-targets,cargo build -p rocm.cargo test -p e2e-cucumber --test e2e— see Gherkin coverage below for the two new scenarios.Verification gap on the substantive commit (
dbc4a8c), stated plainly: it was authored in an environment with no Rust toolchain and no network route to install one, socargo test/clippy/buildwere not run against it at the time. The change was reviewed by reading — types, borrows, and each new test traced by hand against the production path. The latest commits on top were made in an environment where those checks were available and do close the gap:2ecd47d7is a mechanicalcargo fmt --check-driven cleanup (two formatting-only sites, no logic); the commits closing this review round run and passcargo test -p rocm-dash-tui,cargo clippy -p rocm-dash-tui -p rocm --all-targets, andcargo build -p rocmlocally, in addition to CI.Gherkin coverage
AGENTS.md §3 asks for a scenario in
tests/e2e-cucumber/features/for user-observable behaviour, or an explicit justification. Two scenarios were added to close this:@id:dash-onboarding-configure-folder-browse(dash.feature) — opens the onboarding wizard from the Observe tab, advances to the SDK Configure step, pressesTabto open the install-folder browser, and confirms the browser's default "use this folder" entry. The final assertion is on the Configure step's rendered Folder row (it must show the chosen path, not the unset placeholder), not merely that the browser popup appeared — a browser-opened-only assertion would not cover the staged--prefix, which is the behaviour Restore install-folder (--prefix) choice in the dash onboarding wizard #75 actually restores.@id:bootstrap-setup-no-tty-advertises-install-folder(bootstrap.feature, new feature file) — runsrocm bootstrap setuppiped (no PTY, matching every CI/script invocation) and asserts the printed message mentions choosing an install folder.Both run on the existing mock/no-GPU lane; neither needs a GPU or a real install.
Fixes #75