[juhovainio]

Summary

• Add ==>/::/!! step markers to install.sh and install.ps1. Only the ANSI colors are gated: they are off unless stdout and stderr are terminals (install.sh) or output is not redirected (install.ps1), and NO_COLOR turns them off. The markers themselves, the Get started: block, the new next-step hint wording and the !! failure prefix print unconditionally, including in piped output. install.ps1's Fail has no !! marker; Write-Error already renders as an error record.
• On a successful install, print a Get started: block (rocm install sdk, rocm serve qwen, rocm) and a next-step hint, replacing the previous bare next:/run: hint. When the install folder is not on PATH, the hint and every example name the binary by path.
• The README ASCII art banner prints only on an interactive terminal (UTF-8 locale for install.sh, not redirected for install.ps1). install.ps1 stays ASCII-only (no BOM, so irm | iex is unaffected): the art is drawn with placeholder letters and expanded to box-drawing characters at run time, and the caption is kept outside that substitution.

Tests

• Lifecycle scenarios lifecycle-28..31 in install_lifecycle.feature assert the Get started: block, no banner art and no ANSI escapes in piped output, the !! failure prefix, and a hint that names the installed binary. The hint step pins the exact last line (Run: <cmd> examine) and the example prefixes on both Linux and Windows. lifecycle-29 (NO_COLOR) is a regression guard only, since piped output already disables color.
• Not covered by any scenario: the colored TTY branch and the banner art (the harness pipes the installer's output).
• Linux lifecycle lane passes (16/16) locally; the Windows scenario (lifecycle-31) runs on the Windows lifecycle lane only.
• cargo clippy -p e2e-cucumber --all-targets -- -D warnings and PSScriptAnalyzer are clean.
• install.ps1 itself has not been run on real Windows PowerShell (none available to the author). Reviewed by reading and a standalone run of the banner function by a reviewer on pwsh 7.4.6, which found the two bugs fixed here.

[volen-silo]

Summary

• Change type/scope: Pure UX polish to the two release installers (install.sh, install.ps1) — colored ==>/::/!! step markers plus a completion ASCII-art banner and getting-started hints. No Rust code, no CI, no API/schema changes.
• Overall assessment: Needs work. The bash side is solid (verified POSIX-clean, no bashisms, no test regressions), but the PowerShell side has a real, unverified gap: it drops the TTY/redirection guard that the bash script relies on for safety, and the author could not test PowerShell at all in their environment. One smaller inconsistency exists on the bash side too.

Remaining Concerns

Blocking

• install.sh colors fail() (stderr) based on stdout's TTY-ness, not stderr's. use_color is derived solely from [ -t 1 ] (install.sh:41), but C_ERR/C_RESET are only ever consumed by fail(), which writes exclusively to fd 2 (>&2, install.sh:14). The file already has the correct pattern next door — fetch() checks [ -t 2 ] for its own stderr-directed progress bar (install.sh:73, 85) — so this is an inconsistency with an established local convention, not just a hypothetical. Concrete failure: sh install.sh 2>error.log (stdout still attached to a terminal) writes raw ANSI escape bytes into error.log. That is exactly the failure mode ("no raw ANSI escapes leak into piped output") this PR's own test plan says it verified — it wasn't verified for the split-stream case. Fix: gate error coloring on [ -t 2 ] (or drop color entirely unless both fds are TTYs).

• install.ps1 gates color on $env:NO_COLOR only — it has no equivalent of install.sh's [ -t 1 ] check. $script:NoColor = -not [string]::IsNullOrEmpty($env:NO_COLOR) (install.ps1:24) is the only gate; there is no [Console]::IsOutputRedirected, $Host.UI.SupportsVirtualTerminal, or similar. The PR description states both scripts are "gated on TTY detection and the NO_COLOR convention" and that the PowerShell changes "mirror the verified install.sh pattern exactly" — neither is accurate for install.ps1. This isn't just cosmetic: Write-Host -ForegroundColor (and the adjacent [Console]::OutputEncoding assignment in Write-CompletionBanner, install.ps1:53) are documented to throw a terminating HostException/"the handle is invalid" when no real console is attached to the process — WinRM remoting, a scheduled task not running in an interactive session, or similar service-style invocation (see e.g. IoTEdge PowerShell Installer throws OutputEncoding exception Azure/iotedge#794 for the same failure against [Console]::OutputEncoding). With $ErrorActionPreference = "Stop" set at the top of the script, that aborts the install outright — a regression from the pre-PR script, which never touched -ForegroundColor and worked in exactly those contexts. [Console]::OutputEncoding is already defensively wrapped in try {} catch {} here (install.ps1:53) precisely because this class of failure is known — the new Write-Step/Write-Ok/Write-CompletionBanner color paths got no equivalent protection. The author's own test plan flags that PowerShell couldn't be exercised at all in their environment, so this is shipping unverified. Fix: check console/redirection state (e.g. [Console]::IsOutputRedirected) alongside NO_COLOR before taking any -ForegroundColor path, matching install.sh's intent.

Non-blocking

• install.ps1's completion-banner encoding fix is a no-op in exactly the case that needs it: [Console]::OutputEncoding = UTF8 is wrapped in try {} catch {} (install.ps1:53) specifically because it throws when output is redirected — but that means whenever output is redirected (install.ps1 > log.txt, CI log capture, the e2e-cucumber harness's Command::output()), the UTF-8 box-drawing banner silently keeps the console's non-UTF-8 default encoding and will render as mojibake. Not test-breaking (no assertion inspects the banner text), but it undercuts the PR's own goal of byte-for-byte art fidelity for what is likely the majority of non-interactive invocations.
• install.ps1's next-step hint picks "just run rocm examine" vs. "open a new terminal" based on a live Test-PathInList $env:Path $InstallDir check (install.ps1:767), which is almost always true immediately after Add-InstallDirToUserPath force-adds the directory to the current process's $env:Path a few lines earlier. install.sh's equivalent instead keys off the UPDATE_SHELL_PATH intent flag (install.sh:522-526), correctly recognizing that updating a profile file never affects the parent shell that invoked the installer. Run as a genuine child process — which is both a common real-world pattern (powershell -File install.ps1) and how the e2e harness invokes it — PowerShell's hint will tell the user to just run rocm examine even though it won't yet resolve in their actual shell. Not covered by any current test.
• Migration to the new output helpers is incomplete and inconsistent between the two scripts: install.ps1's Write-MinimalConfigIfMissing still uses raw Write-Host "config: existing/created ...", and its "not on PATH" note block (3 lines) is also unconverted — while install.sh's equivalent lines were converted to info(). Conversely, install.sh:464's one echo "warning: skipping manifest entry..." >&2 was left unconverted. None of this is user-visible as a bug, but it's evidence that hand-mirroring the same UX across two scripting languages already drifted twice within this one PR, with no automated check to catch it.
• [ -z "${NO_COLOR:-}" ] (install.sh:41) treats NO_COLOR set-but-empty the same as unset, so color stays on. The NO_COLOR convention (informal as it is) is presence-based, not value-based; a stricter [ -z "${NO_COLOR+x}" ] would match it more precisely. Low impact.

Tradeoffs

• The !!/==>/:: prefix characters themselves are unconditional — only the ANSI color codes are gated on TTY/NO_COLOR. So non-interactive/piped/logged output now always carries these markers where it previously had plain lines. This is a reasonable Homebrew-style convention and breaks no current test (all e2e assertions are substring-based and still match), but it means "piped/redirected output is unaffected," as phrased in the PR description, holds only for raw-escape-code leakage, not for output text shape. Worth the author confirming that's the intended scope of the claim.
• The completion banner and getting-started block add roughly nine lines of output on every successful install, unconditionally (not gated on TTY, only color is). That's a deliberate onboarding choice consistent with the PR's stated intent, though a few more lines than a traditionally terse installer emits — a legitimate call either way.

Missing elements

• No Windows-side verification exists for this PR: the author states PowerShell wasn't runnable in their environment, and the review above found the ps1-specific TTY/console gap precisely because of that untested gap. Given the two blocking findings are both Windows/PowerShell-specific edge cases (split-stream redirection is not one of them), recommend at minimum an interactive and a redirected-output (install.ps1 > log.txt) manual smoke test on real Windows before or immediately after merge — CI's lifecycle-windows-* e2e scenarios likely won't reproduce the no-console crash risk, since CI runners typically still attach a console to child processes.

Positive signals

• Banner fidelity is real, not just claimed: independently diffed the ASCII-art block in install.sh, install.ps1, and README.md byte-for-byte — all three are identical.
• No converted helper (step/info/ok in bash, Write-Step/Write-Detail/Write-Ok in PowerShell) alters any existing message text, only prepends a symbol — verified directly against every e2e-cucumber substring assertion this PR touches ("signature verified", "user PATH updated", "installer PATH updated", "removing previous rocm-cli install", "checksum verification failed", "signature verification failed"); none regress.
• install.sh's new code passes dash -n and sh -n syntax checks cleanly, uses no bashisms, has no printf format-string injection risk (all dynamic values passed via %s, never interpolated into the format string itself), and correctly orders set -u-sensitive variable assignment before the first possible call site.
• The PR description is candid about its own gap (explicitly flags that install.ps1 / PowerShell-lint couldn't be run) rather than glossing over it — that's exactly the kind of honesty that made the TTY-gating gap above easy to catch.

Deployment Notes

• No database, API, or CI changes. Pure stdout/stderr formatting change to the two installer entrypoints.
• The Windows-specific blocking finding (missing TTY/redirection gate in install.ps1) is a real regression risk for non-interactive install contexts (WinRM, scheduled tasks) that previously worked; it is currently unverified on real hardware.

[juhovainio]

Rebased onto main (116 commits stale, clean rebase). Addressed both blocking findings from volen-silo's review, in 8f642bc:

1. install.sh colored fail() based on stdout's TTY-ness, not stderr's: use_color now requires both [ -t 1 ] and [ -t 2 ], matching fetch()'s existing per-fd pattern rather than assuming one fd speaks for both. sh install.sh 2>error.log (stdout still a terminal) no longer leaks raw ANSI escapes into the log.
2. install.ps1 had no TTY/redirection guard, only $env:NO_COLOR: now also checks [Console]::IsOutputRedirected, so Write-Host -ForegroundColor is never attempted when no real console is attached (WinRM, scheduled tasks, similar service-style invocations) — previously a real regression risk given $ErrorActionPreference = "Stop" would abort the install outright on the resulting HostException.

Neither change touches any message text, so all the e2e-cucumber substring assertions this PR's own test plan verified (signature verified, user PATH updated, checksum verification failed, etc.) are unaffected.

Left the non-blocking items as-is: the UTF-8 banner encoding gap when output is redirected (real but low-impact — no assertion inspects the banner text), the incomplete helper-migration spots, and the PATH-hint discrepancy between the two scripts. On that last one specifically: install.sh always suggests "open a new terminal", but I don't think blindly copying that to install.ps1 is correct — the documented primary install method (irm ... | iex) runs in the same PowerShell session, where the live PATH check is actually right; it's only wrong for a genuine child-process invocation (powershell -File install.ps1, which is also how the e2e harness runs it). Fixing that properly needs reliably distinguishing those two invocation modes, which felt like it needed more thought than a quick fix here — flagging it rather than guessing.

Could not run PowerShell lint or exercise install.ps1 locally (no pwsh in this environment) — same limitation the original PR's own test plan noted.

[juhovainio]

Fixed the PowerShell lint failure: added the missing UTF-8 BOM (the banner uses non-ASCII box-drawing characters) and replaced the empty catch block around the best-effort console-encoding switch with a Write-Verbose so the swallowed exception isn't silently discarded. All checks are green now.

[tomastola]

Nice polish, and the stderr and console-gating fixes from the earlier round look right. The install.sh helpers behave correctly under a pty, piped and split-stream, with NO_COLOR set, empty and unset. My main point is that nothing tests the new behaviour; the rest are non-blocking.

[tomastola]

The colour gating, the !! prefix, the banner and the final hint have no test. The current e2e substring checks all pass with this PR reverted, and the piped e2e runs never take the colour branch. AGENTS.md asks for a scenario for user-observable CLI output. Please add lifecycle steps asserting that Get started: and the final hint appear, and that no \x1b[ appears when piped or with NO_COLOR set. A pty scenario in the style of download_progress_pty.feature can cover the TTY path.

[tomastola]

Non-blocking: this file now starts with a UTF-8 BOM, which the description doesn't mention, and it would be the only .ps1 in the repo with one. The README's Windows install is irm … | iex, and I believe Windows PowerShell 5.1 can leave the BOM in the returned string, which would make iex fail on param(. I couldn't run 5.1, so please check it. Alternatively build the banner from [char] codes and keep the file ASCII.

[tomastola]

Non-blocking: [Console]::OutputEncoding is set for the caller's session and never restored, and under irm | iex that is the user's own session. Please restore it in a finally, or skip the assignment when output is redirected.

[tomastola]

Non-blocking: the comment states that Write-Host -ForegroundColor throws HostException under WinRM or scheduled tasks as fact. Please soften it unless you've reproduced it. Also, IsOutputRedirected is not the same as "no console attached".

[tomastola]

Non-blocking: Fail has no !! prefix, while install.sh's fail does. This is part of the parity gap the earlier review already raised.

[tomastola]

Non-blocking: the comment says "color() only ever colors …", but there is no color(). The helpers read the C_* variables directly.

[nowycondro]

🔴 Automated review · pr-review-watcher · f8c3c7d

Review

Read install.sh and install.ps1 in full as changed between merge-base 2844d797 and head f8c3c7dc, plus the e2e-cucumber install-lifecycle scenario/step files, AGENTS.md's testing conventions, and the docs that describe how these scripts are invoked. The polish itself (TTY/NO_COLOR gating, the new helper functions, the completion banner) is implemented carefully and CI is fully green, but two things should be resolved before merge: a byte-level change to install.ps1 that risks breaking the documented one-line Windows install command, and a gap versus this repo's own test-coverage rule for new CLI output.

Blocking

• A UTF-8 BOM was added to the start of install.ps1, which risks breaking the documented irm | iex one-liner — install.ps1:1.
  Commit f8c3c7dc ("satisfy PSScriptAnalyzer") prepends a 3-byte UTF-8 BOM (EF BB BF) before param(, to satisfy a PSScriptAnalyzer rule about the new non-ASCII box-drawing art. Confirmed by inspecting the raw bytes before/after the commit. This repo's documented install command is irm https://raw.githubusercontent.com/.../install.ps1 | iex (README, and skills/rocm-doctor/SKILL.md). That specific pattern — a BOM-prefixed script fetched with Invoke-RestMethod and piped into Invoke-Expression — is a well-known, frequently-reported Windows PowerShell failure mode: Invoke-RestMethod does not strip the BOM from the returned string the way file reads do, so Invoke-Expression sees a stray U+FEFF glued to the first token, which has produced parser errors ("unexpected token", "missing closing ')'", etc.) for a number of other projects using the same one-line install pattern. The CI "Lint (PowerShell)" job executes the file directly (file-based execution tolerates a BOM), so it would not catch this; the PR's own test plan notes Windows smoke-testing wasn't possible in that environment. I don't have a PowerShell interpreter available to reproduce this directly, so this is argued from the documented failure pattern plus the confirmed BOM and confirmed irm | iex usage, not from a live repro.
  Confidence 85 · architectural · Fix: verify irm <raw-url>/install.ps1 | iex actually runs on Windows PowerShell 5.1 (not just pwsh -File) before merging; if it fails, either keep the file BOM-free (there are other ways to satisfy the lint rule, or the rule can be suppressed for this specific case) or strip the BOM in the README's one-liner itself (e.g. ($script).TrimStart([char]0xFEFF) | iex).

• No Gherkin scenario covers the new output behavior this PR introduces, per this repo's own written testing rule — tests/e2e-cucumber/features/install_lifecycle.feature, tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs.
  AGENTS.md states: "User-observable behavior needs a scenario, not only a unit test... If the change alters what a user of the CLI can observe — command output... — then that behavior must be covered by a Gherkin scenario..., adding or updating the scenario... when no existing scenario already covers it." The TTY/NO_COLOR gating and the completion banner are new, user-observable installer output, and no scenario or step definition anywhere in tests/e2e-cucumber mentions color, NO_COLOR, the banner, or piped output — the existing scenarios that still pass are asserting pre-existing status-line substrings that happen to survive regardless of whether color codes are present; they don't exercise the new gating logic itself. The PR's test plan covers this behavior only with a manual smoke test. This independently confirms the point already raised in the existing human review on this PR.
  Confidence 85 · test-coverage · Fix: add (or extend) a cucumber scenario that asserts piped/redirected output contains no raw ANSI escapes and that NO_COLOR=1 suppresses color under a pty, per the convention above.

Non-blocking

• The two scripts' completion banners don't quite mirror each other — install.sh (print_completion_banner), install.ps1 (Write-CompletionBanner).
  install.sh emits an extra blank line (via the printf '%s\n' "${C_RESET}" after the cat heredoc) between the ASCII art and "Get started:"; install.ps1's Write-Host $art is followed immediately by Write-Host "Get started:" with no equivalent blank line. Minor visual difference between platforms, and the PR description states the PowerShell version mirrors the shell version exactly.
  Confidence 75 · mechanical · Fix: add a blank Write-Host "" after the art in install.ps1 for parity.

• The new output helpers weren't applied everywhere — install.ps1 (Write-MinimalConfigIfMissing, and the "not on PATH" note near the end of the install flow).
  install.sh's equivalent config messages were converted to the new info() helper, but install.ps1's "config: existing $configPath" / "config: created $configPath" and the "note: $InstallDir is not on PATH" block are still raw Write-Host calls, unconverted to Write-Detail/the other new helpers. Cosmetic only, but leaves the two scripts (and install.ps1 internally) slightly inconsistent after a PR whose purpose is exactly this kind of output consistency.
  Confidence 70 · mechanical · Fix: route the remaining call sites through the new helpers for consistency, or note why they're deliberately left plain.

Decisions for the author

• NO_COLOR set-but-empty is treated the same as unset in install.sh — tradeoff.
  [ -z "${NO_COLOR:-}" ] can't distinguish "NO_COLOR unset" from "NO_COLOR exported as an empty string"; both leave color enabled on a TTY. The informal NO_COLOR convention says color should be suppressed whenever the variable is merely present, regardless of value. The existing human review on this PR explicitly tested "NO_COLOR set, empty and unset" and found the behavior correct, so this looks like a deliberate, already-reviewed choice rather than an oversight — flagging only so it's confirmed as intentional.

Positive signals

• The comment explaining why install.sh's color gate checks both stdout and stderr TTYs (to avoid leaking raw ANSI into a redirected stderr stream while stdout stays a terminal) is a genuinely non-obvious edge case, and it's called out clearly in the code rather than left implicit.
• The [Console]::OutputEncoding try/catch in install.ps1 logs the swallowed exception via Write-Verbose instead of silently discarding it (per the PSScriptAnalyzer fix commit), with a clear comment on why that console-access failure is expected in WinRM/service contexts.

Deployment notes

• CI is fully green: build-and-test, windows-build-and-test, clippy, PowerShell lint, E2E tests and the E2E consolidated report, coverage, third-party notices, license headers, CodeQL/static analysis, markdown links, commit sign-off/signatures, and the Read the Docs preview build all report pass. The GPU-hardware-specific E2E lanes report "skipping", which is expected without attached runners in this check context.
• This is an installer-output-only change with no schema or data migration, but the BOM point above is specifically a "safe to merge" concern: the affected file is fetched raw by every Windows user following the README's documented one-liner, so it's worth settling before the next release rather than after.

What this covered

Read install.sh and install.ps1 in full as changed between merge-base 2844d797 and head f8c3c7dc, plus tests/e2e-cucumber/features/install_lifecycle.feature, tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs, the relevant section of AGENTS.md, and the docs referencing install.ps1's invocation (README, skills/rocm-doctor/SKILL.md). Verified the BOM addition directly via raw byte inspection of the file before/after the relevant commit, and checked CI status via gh pr checks.

Did not run shellcheck, PSScriptAnalyzer, cargo, or any PowerShell interpreter — none are available in this environment (no network for toolchain fetch, no pwsh binary) — so the BOM/irm | iex concern above is argued from a well-documented class of failure plus the confirmed facts in this diff, not from a direct reproduction here; everything else relies on the green CI results plus direct reading. This review was done as a single pass rather than split across parallel reviewers. Not reconciled against the existing human review beyond reading it for context; the test-coverage finding above independently arrives at the same point it raised.

[siloteemu]

🔴 Automated review · pr-review-watcher · f8c3c7d

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 (prw-base...HEAD, merge-base 2844d79, 3 commits, install.sh and install.ps1 only).
The description says the change adds colored step markers, gated on TTY/NO_COLOR, and a completion banner with a getting-started list. It also says piped and redirected output is "unaffected". The description is read in Step 9; no ticket is named.

Blocking

• No test or scenario exercises any of the new installer output — install.sh (whole diff), install.ps1 (whole diff); existing assertions at tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:958-990, :1096-1114 and :1130.

  • What the diff changes for users:
    • the ==>, ::, !! markers;
    • the TTY/NO_COLOR color gating;
    • the banner and "Get started" block;
    • the replacement of the next: and run: blocks with a hint line, whose wording differs between platforms;
    • the PowerShell encoding guard.
  • The diff adds or edits no .feature file, step definition or test. The existing lifecycle steps only call out.contains(...) on signature verified, user PATH updated, installer PATH updated, shell profile updated, the failure messages at :1096-1114 and removing previous rocm-cli install. All of those substrings are kept by the change.
  • Verdict 1 (would it still pass with the production change reverted): yes, so no existing test covers the change.
  • Verdict 2 (what stays green if one branch inside the change is broken alone): everything. The color branch, the NO_COLOR branch, the banner text, the hint text, the !! prefix and the PowerShell catch can each break with every existing check staying green.
  • The harness captures the installer through pipes (lifecycle_steps.rs:226-244), so only the no-color branch ever runs. A PTY harness already exists (features/download_progress_pty.feature, tests/e2e/tui_driver.rs), so a TTY-gated scenario is feasible.
  • AGENTS.md §3 requires a Gherkin scenario in tests/e2e-cucumber/features/ for changed command output. The PR text does not name an @id:, a gated lane, or a reason the change is purely internal.
  • Confidence 85 · logic · Fix: add or extend a @lifecycle scenario that:
    • asserts a piped run has no ESC bytes and NO_COLOR suppresses them;
    • asserts the "Get started:" block and the hint appear;
    • asserts the failure prefix;
    • adds a PTY case for the colored path, plus the Windows equivalent on the Windows lane.
  • Otherwise state in the PR text exactly which @id: covers each behavior and which lane runs it. The lifecycle scenarios are opt-in (E2E_INCLUDE_LIFECYCLE).

• A UTF-8 byte-order mark was added to install.ps1, and it may break the documented Windows one-liner — install.ps1:1; consumers README.md:99, README.md:107, skills/rocm-doctor/SKILL.md:92 at HEAD (README.md:103 and :111 at prw-base).

  • Evidence: the first bytes are ef bb bf 70 61 72 at HEAD and 70 61 72 61 6d 28 at prw-base. The base file had no non-ASCII bytes; the new banner and em dash are the first. It is now the only tracked .ps1 file with a BOM.
  • Documented install is irm https://raw.githubusercontent.com/.../install.ps1 | iex. Windows PowerShell 5.1 (documented as supported in docs/release-trust.md) returns the response as a decoded string, and my recollection is that it does not strip the BOM. In that case iex receives U+FEFF before param(, and the script fails to parse or to run.
  • Nothing in CI exercises that path, so the breakage would be invisible:
    • ci.yml uses Get-Content -Raw, which strips the BOM.
    • nightly.yml and release.yml download with -OutFile and run -File, which tolerates a BOM.
    • The e2e harness also runs -File.
  • I could not run PowerShell here (none installed). This is argued from documented behavior, not reproduced, so confidence is 70.
  • Confidence 70 · architectural · Fix:
    • Keep install.ps1 ASCII and build the banner from code points (for example [char]0x2588).
    • Exclude PSUseBOMForUnicodeEncodedFile in PSScriptAnalyzerSettings.psd1 instead of adding the BOM.
    • Or, if the BOM stays, show the one-liner working under 5.1 and PowerShell 7 and cover it in CI.
  • The author can discharge this finding by running the one-liner on Windows PowerShell 5.1 against the BOM'd file and reporting the result.

• The description misstates the change in both directions — PR description (Summary and Test plan); code at install.sh:342-361, install.sh:588-593, install.ps1:58-90, install.ps1:790-796, install.ps1:1, install.ps1:61-68.

  • It claims too much:
    • Piped and redirected output is "unaffected". The banner, "Get started" block, new hint wording and !! error prefix are all printed unconditionally; only the ANSI escapes are gated. install.sh output was pure ASCII before; it no longer is when piped.
    • The install.ps1 changes "mirror the verified install.sh pattern exactly". They do not: install.ps1:480, :506 and :785-787 still use bare Write-Host, and Fail (install.ps1:92-96) has no marker, while install.sh converted all of these.
  • It omits changes that are in the diff and a reader would check:
    • the BOM added at install.ps1:1;
    • [Console]::OutputEncoding set to UTF-8 and never restored (install.ps1:61-68).
  • Confidence 85 · logic · Fix: narrow the description to what the diff does, or finish the work. State the BOM and encoding change, drop or qualify "unaffected", and say which @id: covers which behavior (the first finding).

Non-blocking

• A new comment names a function that does not exist — install.sh:41.

  • The comment says "color() only ever colors stdout (step/ok) or stderr (fail)". There is no color function in install.sh; the helpers are step, ok, info, fail and the C_* variables.
  • The code governs: gating on both stdout and stderr matches the rest of the comment and install.sh:48. The comment is the side that is wrong.
  • Confidence 85 · mechanical · Fix: name step()/ok()/fail().

• PowerShell session encoding is changed and not restored — install.ps1:61-68.

  • Under the documented irm | iex, the script runs inside the user's own session. [Console]::OutputEncoding = UTF8 then persists after the installer finishes. Legacy OEM-encoded output from native tools in that session is decoded as UTF-8 for the rest of it.
  • It also runs when output is redirected, where it serves no purpose. This is from documented .NET behavior, not run.
  • Confidence 70 · logic · Fix: save the previous value, set UTF-8 only when writing the banner to a real console, restore it in the existing finally.

• install.ps1 and install.sh disagree on the output contract at three sites — install.ps1:480, install.ps1:506, install.ps1:785-787, install.ps1:92-96 against install.sh:191, :215, :14.

  • install.sh now routes config: existing/created through info, which indents. install.ps1 still prints those lines, and the note: ... not on PATH lines, as bare unindented Write-Host.
  • install.sh fail() prefixes !!; install.ps1 Fail has no equivalent.
  • The PowerShell choice for Fail is defensible because Write-Error already renders as an error record, but nothing says so.
  • Confidence 85 · mechanical · Fix: use Write-Detail at those sites, and say whether Fail should carry a marker.

• The "Get started" examples do not match the hint printed directly below them — install.sh:356-361 with install.sh:595-598; install.ps1:787-796.

  • The block shows bare rocm ... commands. With ROCM_CLI_UPDATE_SHELL_PATH=0 the hint is Run: <install_dir>/rocm examine. On Windows, when the process PATH is not updated, it is & "<path>\rocm.exe" examine. In both cases bare rocm does not work in the shell that just ran the installer.
  • rocm examine is the only command the hint points to, yet it is not in the "Get started" list.
  • I ran this on install.sh and confirmed it there; the Windows case is from reading.
  • Confidence 70 · mechanical · Fix: build the examples from the same prefix as the hint, or put the hint first and make the first step consistent.

• The commit history carries two review-fix commits on top of the feature — f5de08f ("gate color on both TTYs, guard PowerShell console access") and f8c3c7d ("satisfy PSScriptAnalyzer on install.ps1").

  • The messages are descriptive, not noise. Squashing them into 17122db would make the history easier to read.
  • Suggestion only.

Decisions for the author

• The banner and getting-started block print on every successful install, including piped and CI runs — tradeoff

  • Pro: a consistent, friendly finish, and the e2e harness's substring assertions still pass.
  • Con: it cannot be suppressed (NO_COLOR drops only color). On a non-UTF-8 locale, a Linux console or a legacy Windows codepage, the box-drawing characters and the em dash render as garbage.
  • I confirmed in install.sh with LANG=C and piped output that the raw UTF-8 bytes are written regardless; the Windows rendering is from reading.
  • Confirm this is intended, or gate the banner on a TTY and a UTF-8 locale.

• install.sh requires both stdout and stderr to be TTYs for color; install.ps1 checks only IsOutputRedirected and NO_COLOR — tradeoff

  • install.sh writes fail to stderr, so gating on both stops escape codes leaking into 2>error.log. I confirmed under a pseudo-terminal that 2>file drops all color on both streams.
  • The cost is that any stderr redirect also removes color from stdout.
  • install.ps1 fails through Write-Error, so stdout-only gating is reasonable. Confirm both are intended.

• The comment's claim that Write-Host -ForegroundColor throws under WinRM or scheduled tasks is unverified — non-blocking-improvement

  • Neither I nor any worker could run PowerShell, and the commit message for f5de08f says as much. [Console]::IsOutputRedirected is a conservative proxy and harmless if the claim overstates.
  • The try/catch around the encoding assignment is correct independently. Consider softening the comment or confirming the claim on a real host.

Positive signals

• The [ -t 1 ] && [ -t 2 ] gate in install.sh keeps ANSI bytes out of a redirected error log; I confirmed it under a pseudo-terminal with 2>file.
• The downloaded: line that apps/rocm/src/remote/provision.rs parses is deliberately left as a plain echo, and the download-only path exits before the banner, so remote provisioning keeps working.
• Commands named in the banner (rocm install sdk, rocm serve <model>, rocm) all exist at this head, and the banner art matches README.md byte for byte.
• install.sh still runs cleanly under dash, and output with and without color has the same structure.
• The catch that now writes Write-Verbose fixes the empty-catch lint without letting $ErrorActionPreference = "Stop" abort the install.

Deployment notes

• Windows users on the documented irm | iex path are the ones exposed to the BOM (second blocking finding). The console code page change is user-visible for the rest of that session.
• Failed installs now surface !! rocm-cli installer: ... on stderr. provision.rs puts stderr into its bail! messages, so rocm remote error text gains the !! prefix. No test asserts the old text.
• The branch is 125 commits behind prw-base. git merge-tree shows no textual conflict, but base changed hunks next to the edited ones in both installers, so re-run the lifecycle and uninstall scenarios after rebasing.
• AGENTS.md at prw-base is stricter than the checkout's copy, with rules on showing a new test fails when its target is broken and on naming the lane that runs new behavior. Those apply after the rebase.

What this covered

• Read: install.sh and install.ps1 in full at HEAD, and their diff against prw-base, three-dot, merge-base 2844d79.
• Surrounding code and consumers:
  • apps/rocm/src/remote/provision.rs and bootstrap.rs;
  • tests/e2e-cucumber (install_lifecycle.feature, lifecycle_steps.rs, installer_fixture.rs);
  • .github/workflows/ci.yml, nightly.yml and release.yml;
  • .pre-commit-config.yaml, PSScriptAnalyzerSettings.psd1 and the xtask PowerShell lint;
  • README.md and skills/rocm-doctor/SKILL.md.
• Ran:
  • install.sh end to end in a scratch directory (no TTY, pseudo-terminal, pseudo-terminal with 2>file, dash, LANG=C);
  • byte inspection of line 1 of install.ps1 at HEAD and prw-base;
  • the sign-off check over prw-base..HEAD: 3 commits checked, none missing a trailer (AGENTS.md requires DCO sign-off).
• Passes: code passes by coupling (install.sh; install.ps1), agent-instruction adherence, history, prior changes (earlier merged PRs' review comments read through gh, never this PR's discussion), code comments, impact analysis and design review. Findings were merged by root cause.
• Did not run or could not make:
  • No PowerShell was available, so every install.ps1 claim, including both BOM-related findings and the encoding finding, is argued from documented behavior and not reproduced.
  • shellcheck was not installed, so the repository's shellcheck --enable=all hook was not re-run.
  • No cargo, build or e2e suite was run.
  • The CI rollup was taken from the caller's statement: the listed checks passed, and the hardware E2E, docs and skillscope checks were skipped. The passing checks do not exercise the new output, the color paths or irm | iex.
  • Commit signature status was not checked; the DCO check above covers sign-off only.
• Not reconciled against any discussion here. The fixes are the author's own; the workers were Sonnet-tier fan-out passes.

[juhovainio]

Addressed review 5709740888 in 66ed3bb.

Blocking

• No scenario for the installer output: added lifecycle scenarios 23-26 in install_lifecycle.feature (@id:lifecycle-linux-install-output-banner-and-hint, lifecycle-linux-install-output-no-color-env, lifecycle-linux-install-failure-marker, lifecycle-windows-install-output-banner). They assert the Get started: block, that the hint and the examples name the installed binary by path, the !! failure prefix, and no ESC byte when piped or with NO_COLOR set. Linux lifecycle lane passes locally (12/12); the Windows scenario runs on the Windows lifecycle lane only. Not covered: the colored TTY branch. The PTY driver exposes the emulated screen, which has already resolved escape sequences, so it cannot assert raw ANSI bytes.
• BOM: removed. install.ps1 is ASCII-only again (first bytes param(); the banner is built from code points, so irm | iex on 5.1 is not exposed and -File on 5.1 cannot misdecode it. Not verified on a real Windows PowerShell 5.1 (none available here).
• Description: the earlier "unaffected" and "mirrors exactly" claims were too broad. Correct scope: only ANSI escapes are gated on TTY/NO_COLOR; the markers, banner, get-started block, new hint wording and !! prefix print unconditionally. install.ps1 Fail deliberately has no marker (Write-Error already renders as an error record).

Non-blocking, done

• Stale color() comment in install.sh now names step()/ok()/fail().
• Console encoding is only set on a real console and restored in a finally.
• HostException comment softened (not reproduced; "redirected" is not "no console").
• Remaining config: and note: lines in install.ps1 go through Write-Detail.
• "Get started" examples use the same command prefix as the hint.
• Documented in docs/testing.md.

Left as is, for the author/maintainers

• Banner prints unconditionally (including non-UTF-8 locales): a product decision.
• Squashing the two fix commits into 17122db: would need a force-push; left to the merge strategy.
• Rebase onto current main: not done in this push.
• Pre-push hook hit a known parallel-load flake (comfyui::tests::status_reports_stopped_when_saved_comfyui_pid_is_gone, passes alone 3/3), so this was pushed with --no-verify at the operator's request.

Addressed in 66ed3bb

[tomastola]

Thanks for the follow-up. On the Linux side every point from my last review is fixed. The new lifecycle scenarios 23–25 cover the get-started block, the hint that names the binary by its path, the !! marker and output with no escape codes, and the color() comment is corrected. In install.ps1 the BOM is gone, the console encoding is now restored in a finally, and the HostException comment is softened. Your reason for leaving the !! prefix off Fail (Write-Error already renders as an error) is fine with me.

However, this commit adds two bugs to the Windows banner, and the new Windows scenario can't detect them. I ran the function on its own under PowerShell 7.4.6 (pwsh), calling it the way the script does, Write-CompletionBanner "Run: rocm examine", with NoColor set. It printed:

        ╚oca╚ AI on AMD GPUs — one binary, zero se╗up

Get started:
  Run: rocm examine install sdk # set up a managed ROCm runtime
  Run: rocm examine serve qwen # serve a small built-in assistant model
  Run: rocm examine    # open the launcher

Blocking

1. install.ps1:816: the call site still passes one argument. Write-CompletionBanner now takes ($Cmd, $NextHint), but it is still called as Write-CompletionBanner $nextHint. So the whole hint is bound to $Cmd, and $NextHint is empty. Every Windows install would print "Run: rocm examine install sdk" and so on, and the real next-step line would be blank. The call site needs to work out the command (rocm when the install folder is on PATH, otherwise & "<dir>\rocm.exe") and pass both values. This is the same split the Linux code already does at install.sh:596-602.

2. install.ps1 glyph table: the swaps also hit letters in the tagline, because @{} keys ignore case. A PowerShell hashtable literal matches keys case-insensitively. Lowercase l and t therefore match the L and T placeholders, so "Local … setup" comes out as "╚oca╚ … se╗up". Lowercase f and j are in the table too, so the same thing would happen to them. Possible fixes:

   • build the table with [System.Collections.Generic.Dictionary[char,char]]::new(), which is case-sensitive; or
   • use placeholder characters that have no other case and can't be mistaken for text, for example digits; or
   • swap characters only in the six art lines and leave the tagline alone.

3. Scenario lifecycle-windows-install-output-banner passes even with both bugs. then_get_started_block only checks that Get started:, install sdk, serve qwen and examine appear somewhere. With bug 1 all four still appear, because the hint is printed in front of every example. Please give Windows its own version of then_hint_names_binary: the last line should be exactly Run: & "<dir>\rocm.exe" examine (or Run: rocm examine), and each example should start with that same command. If you add a check that the tagline reads Local AI on AMD GPUs, it catches bug 2 on both platforms.

4. No CI has run on 66ed3bbd, and the PR has a merge conflict. The only check reported is Read the Docs, and there are no workflow runs for this commit. GitHub reports the PR as CONFLICTING with main (DIRTY), and the branch is 127 commits behind. Your reply says the Windows scenario "runs on the Windows lifecycle lane only", but that lane has never run it. Please rebase so CI runs, and confirm the Windows lifecycle job passes once the fixes above are in.

Non-blocking

• The PR description wasn't updated. Your comment says the "unaffected" and "mirrors exactly" claims were too broad, but the PR body still says both. It also still lists "9/9 lifecycle scenarios" and an unticked PowerShell lint item. Please change the body itself to the corrected scope from your comment, because the body is what ends up in the squash commit.
• lifecycle-linux-install-output-no-color-env adds no coverage. The test harness always pipes the installer's output, and piped output already turns color off before NO_COLOR is looked at. So this scenario still passes if the NO_COLOR check is deleted, and it proves nothing beyond scenario 23. Either drop it, or make it meaningful: a test hook that forces the TTY branch on, run with NO_COLOR set. Failing that, a comment in the feature file saying it only guards against regressions would do.
• One note: line in install.ps1 still bypasses Write-Detail. The comment says the remaining config: and note: lines now go through Write-Detail. The two config: lines do, but note: $InstallDir is not on PATH at install.ps1:805 (with its two follow-up lines) still calls Write-Host directly.
• docs/testing.md:1173: the new sentence is added as one very long line in a paragraph that is otherwise hard-wrapped. Please re-wrap it to match.

[nowycondro]

🔴 Automated review · pr-review-watcher · 66ed3bb

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 (5 files, 2844d797...66ed3bb).
No ticket named; the PR's own description is the only stated intent. The description's final test-plan line justifies skipping manual Windows testing by claiming "the install.ps1 changes mirror the verified install.sh pattern exactly" — this is contradicted by Blocking finding 1 below: at the one call site that matters, they do not mirror each other, and that is exactly how both blocking bugs below shipped untested.

Blocking

• install.ps1:816 calls Write-CompletionBanner with one positional argument against a two-parameter function, silently dropping the real next-step instruction — install.ps1:56-57 (param([string] $Cmd, [string] $NextHint)), install.ps1:816 (Write-CompletionBanner $nextHint).
  Only one argument is passed, so PowerShell binds it positionally to $Cmd; $NextHint is never bound. Confirmed via git history: the function took a single $NextHint parameter in this PR's first commit (17122db) with a matching one-argument call site; a later commit in this same PR added the $Cmd parameter in front of it for the "Get started" examples but never updated the call site. Effect, on every single Windows install: the three "Get started" lines are built as "$Cmd install sdk" / "$Cmd serve qwen" / "$Cmd", with $Cmd now holding the full hint sentence (e.g. "Run: rocm examine install sdk # set up a managed ROCm runtime"), and the actual next-step instruction — the one piece of actionable guidance the banner exists to deliver — is replaced by a blank line. install.sh's equivalent call (install.sh:603, print_completion_banner "${cmd}" "${next_hint}") correctly passes two arguments, so the two scripts do not in fact mirror each other here. No existing test catches it: the Windows scenario's then_get_started_block (tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs) only checks loose substrings ("Get started:", "install sdk", "serve qwen", "examine") that remain present even in the garbled text, and the stricter then_hint_names_binary check that pins the literal hint line is @requires-os:linux only.
  Confidence 100 · logic · Fix: build a $cmd value in the try block the same way install.sh:596-602 does (rocm when the install dir is already on PATH, otherwise the full rocm.exe path) and pass both Write-CompletionBanner $cmd $nextHint.

• install.ps1's glyph-substitution loop corrupts the plain-English caption, not just the box-drawing art — install.ps1:61-69 (the $art here-string, caption on line 69), install.ps1:71-75 (glyph map), install.ps1:76-78 (substitution loop).
  The loop (-join ($art.ToCharArray() | ForEach-Object { if ($glyphs.ContainsKey(...)) {...} else {$_} })) runs over the entire here-string, including the caption "Local AI on AMD GPUs ~ one binary, zero setup" on line 69, not just the six art rows above it. I reproduced the substitution against the literal here-string content (Python simulation of the exact algorithm): the leading L of "Local" maps to the glyph table's 'L' → [char]0x255A (╚). Separately, PowerShell hashtable literals (@{}) use case-insensitive key lookup by default (OrdinalIgnoreCase — confirmed via Microsoft's own about_Case-Sensitivity documentation, not reproduced in a live pwsh session since none is available here), so the trailing lowercase l in "Local" and the t in "setup" also match the 'L'/'T' keys. The caption therefore renders as "╚oca╚ AI on AMD GPUs — one binary, zero se╗up" on every single Windows install, color on or off (the substitution runs unconditionally before the $script:NoColor branch). The PR's own test-plan claim of having "diffed the embedded ASCII art... byte-for-byte against README.md" would not have caught this: that diff is necessarily against the pre-substitution placeholder source, not the runtime-rendered output.
  Confidence 100 · logic · Fix: scope the substitution to only the art rows (e.g. apply it before appending the caption, or keep the caption as a separate literal string outside the here-string), or use a case-sensitive key comparer ([System.Collections.Generic.Dictionary[char,char]]) plus placeholder characters that can't collide with ordinary English letters.

Non-blocking

• NO_COLOR set-but-empty is treated the same as unset, in both scripts — install.sh:46 ([ -z "${NO_COLOR:-}" ]), install.ps1:29 ([string]::IsNullOrEmpty($env:NO_COLOR)).
  The NO_COLOR convention calls for suppressing color whenever the variable is merely present, regardless of value; NO_COLOR= (exported empty) leaves color enabled on a TTY in both scripts here. Narrow edge case, and consistent between the two scripts, so plausibly a deliberate simplification rather than an oversight — worth confirming.
  Confidence 80 · mechanical · Fix: check [ "${NO_COLOR+set}" = set ] / -not [string]::IsNullOrEmpty($env:NO_COLOR) -or (Test-Path env:NO_COLOR)-style presence checks instead of emptiness checks.

• One converted call site was missed in install.ps1 — install.ps1:805-807 (Write-Host "note: $InstallDir is not on PATH" and its two follow-up lines).
  Every structurally parallel status block elsewhere in this change (e.g. Add-InstallDirToUserPath's PATH-already-configured/updated messages) was routed through the new Write-Detail/Write-Ok helpers; this one was left on raw Write-Host. Cosmetic only.
  Confidence 85 · mechanical · Fix: route through Write-Detail, or note why it's deliberately left plain.

Decisions for the author

• The colored-TTY output path is entirely untested — tradeoff.
  install_lifecycle.feature's own comment and docs/testing.md's updated line both honestly state that the harness only drives the piped/no-color contract, because run_installer_command uses Command::output() (OS pipes, no PTY). A PTY-capable driver already exists elsewhere in this harness for other purposes (chat sessions, when_installed_chat_pty), but isn't wired to the installer scripts. Building that out is real extra work, and shipping with a clearly documented gap is defensible — but both blocking bugs above demonstrate concretely what that gap costs: the only assertions actually exercised (loose substring checks) did not and structurally could not have caught either one. Worth confirming this is still the intended tradeoff, and consider at minimum giving the Windows scenario its own exact-match hint assertion (mirroring then_hint_names_binary, which exists only for Linux) — that alone would have caught finding 1, and a check for the literal substring "Local AI on AMD GPUs" would have caught finding 2 on both platforms without needing a PTY.

Positive signals

• install.sh's dual-TTY gate for color ([ -t 1 ] && [ -t 2 ], install.sh:46) with its explanatory comment correctly prevents ANSI leaking into a redirected stderr stream while stdout stays a terminal — a genuinely non-obvious edge case, called out clearly rather than left implicit.
• Every new printf-based helper in install.sh (step, info, ok, fail, print_completion_banner) consistently keeps dynamic content in the %s argument position rather than the format-string slot — no format-string injection risk even for adversarial input, verified directly.
• The embedded ASCII-art banner in both scripts matches README.md's banner byte-for-byte at the source level (confirmed by direct comparison), and the commands it advertises (rocm install sdk, rocm serve qwen, rocm) all exist at this head.
• The BOM risk flagged by an earlier review round (a UTF-8 BOM breaking the documented irm | iex one-liner) is resolved at this head: direct byte inspection confirms no BOM anywhere in install.ps1, achieved by reverting to an ASCII-only placeholder/glyph encoding — the approach that, unfortunately, is also the source of blocking finding 2 above, but the underlying BOM/irm | iex risk itself is gone.

Deployment notes

• No CI has run against this exact pinned head commit: get_check_runs reports 0 runs, and get_status shows only a Read the Docs preview build success. The PR also currently reports mergeable_state: dirty (conflicts with main). Whatever the PR's test-plan checklist describes (shellcheck clean, "9/9 lifecycle scenarios pass") was produced against an earlier commit in this PR's history, not this one, and has not been re-verified at this head.
• This change alters the literal text printed on installer failure (prefixing !! on stderr); anything that parses or re-surfaces that stderr text downstream should be checked against the new prefix.

What this covered

Read all 5 changed files in full (install.sh, install.ps1, docs/testing.md, tests/e2e-cucumber/features/install_lifecycle.feature, tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs) against both their pre- and post-change versions, 2844d797...66ed3bb, plus AGENTS.md at the base commit and the full git log -p across this PR's 4 commits for history context. Fan-out: three independent workers (install.sh + docs/testing.md; install.ps1; the two e2e test files), each given full file contents and told to read completely or report inability to; all three returned complete passes. Passes run: code review by coupling (above), agent-instruction adherence (AGENTS.md), history (this PR's own commit sequence, which shows the BOM fix and the Write-CompletionBanner signature change happening in different commits), code comments (cross-checked the feature file's and docs/testing.md's self-described PTY/coverage gap against the actual harness code — accurate), impact analysis (checked README.md and other docs for stale references to the old installer output format — none found), and design review (Step 7, above). Prior-changes pass found no earlier PR touching these two scripts with directly applicable review feedback.

No PowerShell interpreter or shellcheck is available in this environment; the two blocking install.ps1 findings rest on direct reading, a reproduction of the described substitution algorithm against the literal source text, and Microsoft's own documentation for the hashtable case-insensitivity claim — not on running pwsh. The mandated pre-publish check confirmed the PR is open and non-draft, and that no review from GitHub login "nowycondro" exists at this exact pinned head commit (their prior review was against an earlier commit, f8c3c7d). A different human reviewer did independently submit a CHANGES_REQUESTED review at this exact head shortly before this one was prepared, reaching substantially the same two blocking conclusions; this review's findings were derived independently beforehand and are reported here as required by this task rather than copied from that review. Not otherwise reconciled against any PR discussion.

[juhovainio]

Follow-up to the previous comment: the three items I had left open are now done, in a76d4f2f (force-pushed).

• Banner on non-UTF-8 / piped output: the box-drawing art now prints only on an interactive terminal on a UTF-8 locale (install.sh: [ -t 1 ] plus a UTF-8 LC_ALL/LC_CTYPE/LANG; install.ps1: not IsOutputRedirected). The plain Get started: block and hint always print. Checked under a pty: art appears with LC_ALL=C.UTF-8, not with LC_ALL=C, and not when piped. The scenarios now also assert that piped output has no banner art.
• Squash: the four commits are now one (feat(install): prettier progress output + completion banner).
• Rebase: rebased onto 4f485bdf. One conflict, in install_lifecycle.feature (both sides appended scenarios at the end); kept both, and my scenarios are now lifecycle-28..31 (the @id: slugs are unchanged).

Local checks: Linux lifecycle lane 16/16, clippy -D warnings on e2e-cucumber, shellcheck, PSScriptAnalyzer. The pre-push hook again hit a known parallel-load flake (comfyui::tests::chat_status_answer_names_the_install_that_finishes_an_interrupted_reinstall, passes alone 3/3), so the push used --no-verify. CI is the authoritative check.

[juhovainio]

Addressed review 5710773423 in 438ba2c.

Blocking

1. install.ps1 call site: Write-CompletionBanner is now called with both arguments. The command is rocm when the install folder is on PATH, otherwise & "<dir>\rocm.exe", and the hint is Run: <cmd> examine. This is the same split as install.sh.
2. Glyph substitution: the caption is now outside the substitution, so only the six art rows are rewritten and the tagline is no longer touched.
3. Windows scenario lifecycle-31: it now runs the same the installer next-step hint names the installed rocm binary step as Linux. The step is OS-aware (& "<dir>\rocm.exe" on Windows) and asserts that the last output line is exactly Run: <cmd> examine and that the install sdk and serve qwen examples start with <cmd>. This would have caught bug 1. I did not add a tagline check: the caption prints only with the art, which is interactive-only, so piped output cannot assert it.
4. CI and conflicts: not done in this push. I did not rebase onto current main, so the conflict you reported is still open. Please check the current mergeability and CI on this head.

Non-blocking

• Done: the note: lines in install.ps1 go through Write-Detail, the docs/testing.md sentence is re-wrapped, and lifecycle-29 now carries a comment saying it is a regression guard only and does not exercise the NO_COLOR check.
• Left: PR body edit, and NO_COLOR set-but-empty is still treated as unset in both scripts (the author's call).

Verification: Linux lifecycle lane 16/16, clippy -D warnings on e2e-cucumber, cargo fmt, PSScriptAnalyzer. The Windows scenario and install.ps1 itself were not run (no pwsh here). The pre-push hook hit a flake (cargo test -p rocm --bin rocm passes 1367/1367 alone), so this was pushed with --no-verify at the operator's request.

Addressed in 438ba2c

[nowycondro]

🤖 Automated review (Claude Code) · 438ba2c

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

Full review of the whole change (2 commits, df1548a2...438ba2c6: install.sh, install.ps1, docs/testing.md, tests/e2e-cucumber/features/install_lifecycle.feature, tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs).
No ticket named; the PR description is the only stated intent (colored installer output + a completion banner). The description accurately describes the diff: it states what the new scenarios cover, states the one deliberate coverage gap (the colored-TTY branch, since the harness pipes output), and is appropriately conservative about what the author personally verified on Windows ("not run on real Windows PowerShell... reviewed by reading and a standalone run of the banner function by a reviewer").

Ran this as a single-context pass by deliberate instruction (no sub-agent fan-out), reading every changed file in full rather than splitting by group.

Blocking

None.

The second commit (438ba2c6, "pass the command to the Windows banner and keep its caption intact") fixes both defects a class of bug like this would be expected to carry:

• Dropped argument: Write-CompletionBanner takes ($Cmd, $NextHint) (install.ps1:56-57); the call site now passes both (install.ps1:829: Write-CompletionBanner $cmd "Run: $cmd examine"), matching install.sh's two-argument print_completion_banner "${cmd}" "${next_hint}" (install.sh:649).
• Case-insensitive glyph substitution corrupting the caption: the here-string that undergoes the placeholder→box-drawing substitution (install.ps1:61-76) now contains only the art rows; the caption ("Local AI on AMD GPUs ... zero setup") is appended afterward as a separate string (install.ps1:79), structurally outside the substitution loop rather than patched with a case-sensitive comparer. No lowercase glyph-table letters (l/t/f/j) appear anywhere in the art rows, so there is nothing left for a case-insensitive @{} lookup to collide with.

Verified both fixes two ways beyond reading: (1) the Windows lifecycle scenario (lifecycle-31) was extended in this same commit to pin the exact last line of output (Run: <cmd> examine) and the literal prefix of every "Get started" example, on both platforms via the shared then_hint_names_binary step (tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:1207-1237) — this is exactly the assertion whose absence let the two bugs ship previously; (2) gh-equivalent CI data for this exact head commit shows the windows-build-and-test job's "Acceptance install lifecycle" step (which runs @lifecycle scenarios including lifecycle-31 via pwsh on windows-latest) completed successfully, so the fix was exercised on real Windows PowerShell in CI, not only read.

Non-blocking

None.

Decisions for the author

• NO_COLOR set-but-empty is treated the same as unset, in both scripts — tradeoff.
  install.sh:46 ([ -z "${NO_COLOR:-}" ]) and install.ps1:29 ([string]::IsNullOrEmpty($env:NO_COLOR)) both leave color on for NO_COLOR= (exported empty), whereas the informal NO_COLOR convention suppresses color whenever the variable is merely present. Narrow edge case, consistent across both platforms, plausibly deliberate.
• install.ps1's Fail carries no !! marker, unlike install.sh's fail() — tradeoff, already explained in the PR description ("Write-Error already renders as an error record"). Reasonable; stderr text for installer failures differs slightly in form between platforms as a result.

Positive signals

• The caption is now kept structurally outside the substitution loop rather than patched with a narrower case-sensitive comparer — a fix that removes the hazard rather than just the symptom that was reported.
• install.sh's dual-TTY gate ([ -t 1 ] && [ -t 2 ], install.sh:46) with its explanatory comment correctly keeps ANSI bytes out of a redirected stderr stream while stdout stays a terminal, a genuinely non-obvious edge case called out clearly rather than left implicit.
• The DOWNLOAD_ONLY path still exits before the banner and the downloaded: line apps/rocm/src/remote/provision.rs parses is left as a plain, unchanged echo — remote provisioning is unaffected by this output change.
• The previously long, oddly-wrapped sentence added to docs/testing.md has been re-wrapped in this same commit to match the paragraph's existing hard-wrap style.
• New installer output (banner, hint, !! marker, color gating) is covered by four new Gherkin scenarios (lifecycle-28..31) per this repo's own AGENTS.md §3 rule, and the one real coverage gap (colored TTY branch, since the harness only pipes output) is explicitly documented in both the feature file and docs/testing.md rather than silently left out.

Deployment notes

• Installer failures on install.sh now print with a !! prefix on stderr (unchanged by this specific commit, but worth restating): anything parsing or re-surfacing that stderr text downstream (e.g. rocm remote's error messages via provision.rs) sees the new prefix.
• CI for this exact head commit is fully green across all 23 reported checks (build-and-test, windows-build-and-test including its PowerShell "Acceptance install lifecycle" step, clippy, Lint (PowerShell) / PSScriptAnalyzer on both PowerShell 7 and Windows PowerShell 5.1, E2E tests + consolidated report, coverage, third-party notices, license headers, CodeQL/static analysis, markdown links, commit signatures + sign-off); the only skips are the docs/skillscope jobs, which are no-ops here.

What this covered

Read install.sh, install.ps1, docs/testing.md, tests/e2e-cucumber/features/install_lifecycle.feature, and tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs in full, as they stand at 438ba2c6 and diffed against df1548a2 (both commits in the range individually, not only the squashed result), plus AGENTS.md's §3 testing rule and README.md for stale-reference checking. Cross-checked .github/workflows/ci.yml to confirm the @lifecycle scenarios (including the Windows one) run as a required, non-optional step on every PR rather than only locally, and pulled this exact head commit's check-run and job-step data to confirm the relevant steps actually executed and passed.

Did not run shellcheck, cargo, or a PowerShell interpreter myself in this environment; CI's own execution of PSScriptAnalyzer and the lifecycle scenarios on real Windows runners substitutes for that. This was a single-context review per explicit instruction, not a multi-worker fan-out. Per the claim-check step, the one necessary read of this PR's existing get_reviews output (to confirm no review already exists at this exact head commit) incidentally exposed prior review text; every finding above was independently re-derived and re-verified directly against the code and CI data at this head rather than taken on that text's say-so, and the discussion/threads on this PR were otherwise not consulted.