[r0x0r]

Summary

Fixes ROCMAI-543: rocm model (and several other commands) wrote a UTF-8 em dash (U+2014) to stdout/stderr. A Windows console on the OEM code page (437) mis-renders that as ΓÇö instead of an em dash (seen in Windows PowerShell 5.1).

• Replaces non-ASCII punctuation — em dashes, plus an ellipsis (U+2026) and a left arrow (U+2190) of the same bug class — with plain ASCII in every genuinely user-facing output string across apps/ and crates/: CLI prints, clap doc-comment --help text, and error/remediation messages.
• Adds xtask/src/ascii_output_contract.rs, a repo-wide contract test that makes the invariant structural rather than per-site. See Why the coverage shape changed.
• Adds a Gherkin scenario (serve-model-list-output-is-ascii) per AGENTS.md §3, which requires a BDD scenario (not just a unit test) for changes to observable CLI output.
• Updates docs/manual-testing.md's quoted CLI wording to match the new output.

Correction: two earlier claims in this description were false

Two headline claims in previous revisions of this description did not hold, and both were correctly called out in review. Stating plainly what was wrong:

"A repo-wide scan of info!/warn!/error!/debug!/trace! call sites finds zero remaining non-ASCII." False. Two em dashes remained:

• apps/rocm/src/dash.rs — inside a tracing::warn! whose own doc comment says it goes to the file-only client log rocm-cli.log.
• crates/rocm-dash-daemon/src/runner.rs — in a string pushed into Snapshot.warnings, which broadcast_and_persist → SessionWriter::append serializes into an NDJSON session file whenever persist_dir is wired, and apps/rocm/src/dash.rs wires it unconditionally.

Why the scan missed them. It was anchored on the macro name. In both cases the em dash sits on a continuation line of a multi-line string literal, several lines below the warn! the grep matched on, so the match window never contained the dash. This is not a near-miss: any finder keyed to a call site has this blind spot for every multi-line literal in the tree, and this repo has hit it before. Both are now converted, and the re-scan that found them is byte-level and literal-oriented, cross-checked against a second independent implementation (the Rust one that ships in this PR) which agrees site-for-site.

"Every converted site now has a regression test that fails if the conversion alone is reverted." False as written — several converted strings had no assertion that reached the converted character. Real numbers are in Regression coverage below.

Scope: what is deliberately not converted

The bug is a byte-level misdecode. ΓÇö is the UTF-8 bytes E2 80 94 read as code page 437, which can only happen once raw bytes leave the process — i.e. on a redirected stream or into a file another tool reads back. A write to a real Windows console never takes that path: the handle is a console, so Rust emits UTF-16 via WriteConsoleW and no code page is consulted. The sweep therefore follows the stream, not the character.

A byte-level scan of every string/char literal in the tree finds 711 containing non-ASCII, across 98 files. Classified:

Class	Count	In scope?
crates/rocm-dash-tui	336	No — console-only; box drawing, Braille sparklines and arrows are its visual language. Byte-identical to main.
Test modules and test crates	306	No — assertion text, not product output.
crates/e2e-report	29	No — see below.
xtask	12	No — developer/CI tooling, never in a released binary.
TTY-gated spinner labels	7	No — cli_progress::Spinner/AnimatedSpinner are hard-gated on stderr().is_terminal() and render nothing when disabled.
cli_progress internals (Braille frames, overflow marker)	12	No — same gate.
JobStatus::glyph() in rocm-dash-core	4	No — every caller is in rocm-dash-tui.
Genuinely in scope	5	Yes — all converted in this revision.

The five: the two above, two CollectorError::Unsupported guards in rocm-dash-collectors (unreachable in production, converted anyway so there is no argument left about where the bytes go), and an em dash in a Python comment inside the embedded torch alignment probe script.

On crates/e2e-report (raised in review). Checked and left alone, deliberately. It is a CI report generator, not CLI output: crate_edges::ALLOWLIST confirms it is reachable only from xtask and the e2e harness, never from the shipped rocm binary. What it writes is HTML carrying an explicit meta charset="utf-8" (consolidated.rs:941, single_report.rs:61) plus GitHub-flavoured Markdown for the Actions job summary — both decoded as UTF-8 by declaration, not by whatever code page a console is on. The review named one em dash at consolidated.rs:1316; the same file holds 24 further non-ASCII literals including the ✅/❌/⚠️ status vocabulary and ×/·, which would mojibake identically under the premise given. If that premise held the fix would be all 25 and the report would lose its glyph vocabulary — the same trade already settled for the TUI. Converting one and leaving twenty-four is the inconsistency this PR was already criticised for in bench_run.rs.

The daemon's log output is in scope, because the log is not a console. apps/rocm/src/logging.rs routes rocm_dash_daemon=info into a daily-rotated plain-UTF-8 file via tracing_appender::rolling::daily, with no is_terminal() gate anywhere.

Why the coverage shape changed

Per-site assertions were the wrong shape for this invariant, and this PR is the evidence: they failed twice, both times because nothing enumerated the sites. Adding more per-site assertions could not have caught either leftover — the problem was never assertion strength.

xtask/src/ascii_output_contract.rs enumerates them. It follows the house pattern for whole-tree invariants (crate_edges, env_mutation_contract, workflow_contract) and adds no dependency:

• It lexes each .rs file and inspects literals, not lines. A literal's continuation lines are part of the literal, so a dash hiding on one is found like any other — the exact blind spot that let the two leftovers ship.
• It also checks the data files include_str! compiles into a binary. The label fields in crates/rocm-core/src/model_catalog.json are what rocm model prints, and rocm model is the command this ticket was filed against; being JSON, no Rust-source scan would see them.
• Comments are skipped deliberately (a © in a licence header is not output). Clap --help text is doc comments, and is covered instead by every_help_text_in_the_command_tree_is_ascii, which asserts the rendered help and so also covers #[arg(help = ...)].

On allowlist rot, which is the standing objection to a scan like this and the reason an earlier round rejected one. The check is bidirectional, the same discipline crate_edges::ALLOWLIST already uses here: an exemption that matches nothing fails as loudly as an unexempted literal. Convert a spinner label, or delete a file an exemption names, and the test goes red until the stale entry is removed, so the list can only ever describe literals that are really there. A further test requires every exemption to carry a reason. The list is 11 literals (7 spinner labels, 4 TUI glyphs), 5 path prefixes and 2 embedded files.

The lexer is itself the risk, so every shape that defeated an earlier finder has a fixture: the multi-line continuation line, raw strings at any hash count, lifetimes vs. char literals, nested block comments, escaped quotes, and the #[cfg(test)] use a::{b, c}; brace group that a naive "first #[cfg(test)] wins" rule would let exempt an entire file.

This is a source-literal guard, not a sanitizing wrapper at the point bytes leave the process. A wrapper was considered and is out of scope here: it would have to sit on every print!/write!/tracing sink in the workspace, and silently transliterating output at runtime hides the mistake rather than failing on it.

Regression coverage

74 strings are converted by this PR. Each was reverted individually on the devbox and the covering test observed red, then green again on restore:

Mechanism	Sites	Falsified red
every_literal_that_can_reach_a_redirectable_sink_is_ascii (new)	50	50
every_help_text_in_the_command_tree_is_ascii (clap doc comments)	13	13
render_model_registry_text_lists_builtin_recipes (model_catalog.json labels)	3	3
The owning test itself (test-module expectation text)	8	8
Total	74	74

The last 8 are .contains(...) expectations and fixtures inside #[cfg(test)] modules, not product output. They are self-enforcing: reverting one makes its own test fail, because the production string it is compared against is ASCII. Verified rather than assumed — two were reverted and render_model_registry_text_lists_builtin_recipes and model_catalog::tests went red as predicted.

Separately falsified, each red with the failure naming the offending text, green on restore:

Property	Reverted	Result
File-only client log stays ASCII	dash.rs test-clock warning	red
NDJSON session record stays ASCII	runner.rs amd-smi warning	red
Embedded catalog stays ASCII	a model_catalog.json label	red
Exemption list cannot go stale	a spinner label converted to ASCII	red
service_launch.rs (review item 3)	"detached - server still running"	red
ROLLBACK_RECOVERY_HINT (review item 4)	"(no history - undoes only ...)"	red

The last two are the gaps review items 3–6 named — service_launch.rs, ROLLBACK_RECOVERY_HINT, the ~15 remote/{mod,publish,doctor,install,provision,tailnet}.rs sites, dash.rs's bench strings, engines_cmd.rs and runtimes_cmd.rs. All of them are now covered by the contract rather than by 25 individually weak assertions, and the per-file red counts above show each file's sites failing.

The two registry-driven tests from the earlier revision are kept, because a source scan and a rendered-output walk catch different mistakes: every_message_the_diagnose_catalog_can_emit_is_ascii walks CHECKERS and asserts both the text report and the --json payload, and every_help_text_in_the_command_tree_is_ascii walks the clap tree.

Test plan

• cargo test --workspace --all-targets --no-fail-fast (remote Linux box) — 33 groups, 3869 passed, 16 ignored. One failure, cli_progress::tests::animated_spinner_keeps_ticking_without_progress_calls, is a known timing flake under parallel load; it passes in isolation and is unrelated to this change (it asserts the background ticker advances frames).
• cargo clippy --workspace --all-targets -- -D warnings (remote Linux box) — clean
• cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings (remote Linux box) — clean
• cargo fmt --all --check
• Falsification: all 74 conversions reverted one at a time, each observed red then green on restore

Review follow-up (non-blocking items)

Item 9 — a shared assert_ascii(value: &str) helper in main.rs's test module. Not taken; the premise does not hold. The seven is_ascii() assertions are not copy-paste: each names a different surface and interpolates a different value — rocm services prune output, the remote-session warning, the setup-reset prompt, the host-GPU marker, {path} --help, rocm model, rocm model --verbose. What repeats is is_ascii() and the ticket id, i.e. the shared concept. Collapsing them into one helper would replace seven specific diagnostics with one generic message, so a failure would no longer say which surface broke — that message is the value, not the boilerplate.

Item 10 — tighten every_message_the_diagnose_catalog_can_emit_is_ascii to respect the OS-family filter. Not taken, and tightening it would weaken the invariant in the direction that matters. ROCMAI-543 is a Windows bug, and CI runs on Linux: filtering by OS family means a Linux CI run would never check any Windows-only message in the catalog — precisely the strings most likely to reach an OEM code page. The test already calls the real diagnose() and asserts the rendered report, so the production path is covered; the CHECKERS loop is an additional, deliberately broader net over text that is compiled into the binary regardless of host. The hypothesised false failure needs a checker that misbehaves when it reads a foreign os_family from a plain data struct, which would be a bug worth failing on.

Known scenario-id collision

tests/e2e-cucumber/features/model_serving.feature gains one Scenario: serve-25. Two other open PRs independently claim the same index in the same file:

• feat(vllm): enhance OOM diagnostics and guidance in serve summary (eai-8059) #284 (feat(vllm): enhance OOM diagnostics…) adds serve-25 through serve-28
• test(serve): cover the restart path's startup death end to end (serve-25) #581 (test(serve): cover the restart path's startup death end to end (serve-25)) adds serve-24 and serve-25

Nothing can be reserved or gapped around: tests/e2e-cucumber/tests/feature_naming.rs::scenario_names_are_indexed_sequentially_per_feature asserts {key}-{NN:02} - by position+1 in declaration order, so whichever of the three merges last renumbers. Flagged for coordination, not fixed here.

[juhovainio]

Reviewed with a review-code fan-out pass plus a separate Standards/Spec pass. Overall this is a clean, well-scoped fix: single commit, properly signed/DCO'd, accurately described, and the new e2e scenario genuinely exercises the real non-TTY trigger condition (piped stdout) rather than a TTY/PTY false positive. CI is green on every required lane.

Two things are worth fixing before merge, one of which I've left as an inline comment:

1. crates/rocm-dash-tui/src/ui/bench_run.rs contradicts the PR's own stated scope (inline comment below): the PR says rocm-dash-tui is intentionally left untouched because it renders via crossterm and bypasses the OEM code page entirely, but this file has one em dash fixed while three sibling em-dash strings in the identical render path are left as-is (bench_run.rs:64, :270, :280). Either the crate is exempt (and this one edit should be reverted) or it isn't (and the file should be finished).

2. The ticket's second acceptance criterion ("check that no other commands show the same problem") isn't fully met, and the gap sits on the bug's actual trigger condition. Three spinner labels still use a raw ellipsis: apps/rocm/src/serve_cmd.rs:858 ("Starting {model} on {selected_engine}…"), and apps/rocm/src/therock.rs:2912/:2921 ("Downloading {}…", "Extracting {}…"). These render via cli_progress::Spinner/AnimatedSpinner to stderr, gated only by IsTerminal - meaning they print precisely when stderr is an interactive console, which is the ticket's reported scenario (Windows PowerShell 5.1, a TTY), not a redirected stream. The PR's stated TTY exemption doesn't actually cover these three call sites. These aren't in this diff so I can't anchor a line comment to them, but they're easy to fix alongside the rest.

Non-blocking, for a follow-up if you'd rather not scope-creep this PR further: the --help text changes (the largest single block of this diff) have no test coverage at all; the new "<- your GPU" heading branch in main.rs isn't exercised by any test in the default no-GPU CI lane; and a handful of specific main.rs string edits (the rollback-recovery hint, the service-prune skip message, the uninstall remote-session warning, the onboarding-reopen line) are only covered by substring assertions that wouldn't catch a revert of just the punctuation. None of this is a correctness bug, just coverage gaps against a self-described "replace every em dash" change.

[juhovainio]

This is the only em-dash fix in this crate, but the PR describes rocm-dash-tui as intentionally untouched (renders via crossterm/WriteConsoleW, bypasses the OEM code page). Three sibling em-dash strings in this same render path are left unfixed: line 64 ("{s} (daemon-tailed — live updates)"), line 270 ("(optional — auto-detected from /v1/models)"), and line 280 ("1,2,4,8,16,32,64,128 — stops at saturation"). Likely cause: this exact string also appears in apps/rocm/src/dash.rs, which was fixed, so a literal-string grep probably patched both copies without applying the "crate excluded" rule per-file. Please either revert this one line (keeping the crate genuinely untouched as described) or finish converting the other three.

[r0x0r]

Reverted — your diagnosis was exactly right: this string is byte-identical to one in apps/rocm/src/dash.rs, so the literal-string replacement caught both copies without applying the crate-exclusion rule per file.

Landed on "revert" rather than "finish the file" on the merits: the crate holds 1483 non-ASCII occurrences across 65 files (box drawing, Braille sparklines, arrows), so converting the other three em dashes would fix 3 of 1483 and leave it no more ASCII than before. The exemption's rationale also checks out — ΓÇö is E2 80 94 decoded as CP437, a byte-level misdecode that needs raw bytes on a redirected stream, and this crate only renders to a real console (UTF-16 via WriteConsoleW, no code page). crates/rocm-dash-tui is now byte-identical to main.

[r0x0r]

Thanks @juhovainio — both findings were accurate about the code; I landed on a different conclusion on the second and want to lay out why. Pushed ad585cc8..2a118bd5.

1. bench_run.rs — reverted, crate is exempt.

You were right about the cause: the string is byte-identical to one in apps/rocm/src/dash.rs, so the literal-string replacement patched both copies. crates/rocm-dash-tui is now byte-identical to main.

I went with "exempt" on the merits rather than because it is less work. The crate carries 1483 non-ASCII occurrences across 65 files (box drawing, Braille sparklines, arrows); converting the other three em dashes would have fixed 3 of 1483 and left the crate no more ASCII than before, while breaking the TUI's visual language if carried through. And the exemption's rationale holds: ΓÇö is the bytes E2 80 94 decoded as CP437, a byte-level misdecode that only happens once raw bytes leave the process. The TUI only ever renders to a real console, which receives UTF-16 via WriteConsoleW and consults no code page.

2. Spinner labels — kept, and this is where I differ.

Your reading of the gating is exactly right: Spinner::new sets enabled = stderr().is_terminal() and render_current returns early when disabled, so these labels print only when stderr is a console.

That is the reason I think they are safe rather than exposed. Writing to a console is the one path that cannot produce the reported mojibake, for the same reason the TUI exemption holds — UTF-16 out, no code page. I think the two findings rest on opposite premises about the same mechanism: #1 accepts "console rendering is immune" as a valid basis for exemption, #2 requires it to be false. The reported artifact is byte-level, so the immunity premise is the one the evidence supports.

Two further things that pushed me this way:

• The cited three are not a complete set. There are eight such labels — serve_cmd.rs:858,902, therock.rs:2912,2921, comfyui.rs:587,1767,2401, main.rs:4698 — all behind the same gate. Fixing three would have reproduced precisely the partial-conversion inconsistency you flagged in bench_run.rs.
• cli_progress emits Braille frames and appends U+2026 as its own overflow marker on every repaint, so transliterating labels alone changes nothing observable on that line.

I have recorded all of this in cli_progress's module docs so the exemption is justified by mechanism rather than left as an assertion. Happy to convert all eight if you still disagree — but it should be all eight plus the frames and the overflow marker, which is a visual-design change rather than this fix.

Worth noting the PR already applies this same rule in the other direction: println!("stopping server…") was converted, because that one is redirectable.

3. Title/description — yes, widened. The title said "em dash" while the change also covers an ellipsis and a left arrow; it is now "replace non-ASCII punctuation with ASCII in CLI output text". The description now states the stream-based rule explicitly instead of asserting the exemptions. Your point that the ticket is about mojibake generally is what prompted the audit; the existing rendered.is_ascii() assertion already covers ellipses and arrows, not just U+2014.

4. Non-blocking — took two, left one.

• --help coverage — taken. Added a test walking the entire clap command tree, asserting both short and long renderings. A top-level --help assertion would have covered almost none of this diff, since every help edit sits on a nested subcommand's field or variant. It passes today, which confirms the help sweep was complete, and it picks up the rollback after_help string you listed under the third item.
• <- your GPU — taken. Split the heading into a helper so the branch is reachable without matching host hardware, and asserted the exact string, so a punctuation-only revert fails. This was the real hole: the is_ascii() guard never covered it on the no-GPU lane.
• Remaining substring-only assertions — left. Your point stands that they would not catch a punctuation revert. I did sweep for anything still at risk: outside the TUI and the gated spinners, there is no non-ASCII left in redirectable CLI output — only test assertion messages, tracing logs, and the e2e report generator, none of which reach a Windows console.

Verified on a Linux box: clippy --workspace --all-targets, clippy -p e2e-cucumber --test e2e, both clean at -D warnings; cargo test --workspace --all-targets --no-fail-fast → 33 groups, 3847 passed. The one failure was animated_spinner_keeps_ticking_without_progress_calls, a known timing flake under load that passes 3/3 in isolation and is untouched here (my cli_progress change is 100% doc-comment lines).

Also: no feature file changed in these commits, so this still claims a single serve-25 and the numbering collision with #284 is no worse.

[jussielo-amd]

Reviewed at 2a118bd5. Summary of a full pass over the diff (fanned out across the files touching apps/rocm/src/main.rs, the smaller apps/rocm files, the apps/rocm/src/remote/ subsystem, crates/rocm-core/rocm-dash-daemon/lemonade, and the e2e/docs changes).

Fixes ROCMAI-543 ("em dash shows as mojibake in Windows PowerShell 5.1"). The approach is sound and precisely described: converting non-ASCII punctuation to ASCII in output reachable on a redirectable stream, while correctly leaving crates/rocm-dash-tui and the TTY-gated spinner labels in cli_progress.rs untouched (verified directly — Spinner/AnimatedSpinner are hard-gated on stderr().is_terminal() with no bypass path, so they can never hit a redirected stream). The two new tests (every_help_text_in_the_command_tree_is_ascii, and the rocm model is_ascii() assertions) are well-designed and would catch a revert.

Requesting changes

Several of the individual ASCII conversions have no regression test that would fail if reverted, which doesn't meet AGENTS.md §3's "every bug fix ships with a regression test... test fails before fix and passes after fix":

• crates/rocm-core/src/diagnose.rs:1185,1681-1682
• crates/rocm-core/src/examine.rs:1667
• crates/rocm-core/src/fix.rs:1331
• crates/rocm-dash-daemon/src/server.rs:125
• engines/lemonade/src/runtime_dir.rs:468
• apps/rocm/src/main.rs:4397 (service-prune skip message) and :13698 (uninstall remote-session warning) — no test references these strings at all
• apps/rocm/src/main.rs:1845, tested at :23562 — the existing assertion (rendered.contains("Onboarding will not reopen automatically")) stops short of the changed text, so it still passes with the em dash reintroduced

The PR already shows the right pattern for this (the clap-help-tree walk and the rocm model is_ascii() checks) — it'd be great to extend the same idea to these remaining sites, e.g. an .is_ascii() or exact-string assertion added to whichever existing test already exercises each function.

Non-blocking

• tests/e2e-cucumber/features/model_serving.feature:333 adds Scenario: serve-25 - .... The PR body discloses the known collision with #284, but open PR #581 ("cover the restart path's startup death end to end (serve-25)") also independently claims serve-25 in the same file and isn't mentioned. Not a defect here — the repo's sequential-numbering test will force a renumber on whichever of the three merges last — but worth a heads-up for coordination.
• Minor style inconsistency: this PR's conversions use " - " consistently, while some untouched comments nearby (e.g. engines_cmd.rs:622,632) use " -- " for the same role. Not wrong, just worth knowing.
• One test-only em dash remains at apps/rocm/src/remote/mod.rs:2429 (inside a #[test] assertion message, never shipped) — consistent with this PR leaving test code untouched elsewhere, not an oversight.

Nice work on the cli_progress.rs module doc and the model_catalog_platform_heading extraction (makes the previously CI-unreachable "your GPU" branch unit-testable) — both are genuinely good engineering beyond the minimum needed here.

[r0x0r]

Thanks @jussielo-amd — the blocking finding was right on every count. I checked each of the eight sites against 2a118bd5 before touching anything, and grepped each surrounding string literal across the whole tree: none of them was referenced by any test, so each could have been reverted with the suite staying green. Pushed as f504c57a.

On shape: one invariant where a registry exists, extensions everywhere else

You suggested adding an .is_ascii() or exact-string assertion to whichever existing test already exercises each function, and that is what most of these got. But for rocm diagnose I went broader, because eight pinned sentences would mostly have restated the constants they check — and AGENTS.md §3 says as much ("a test that only pins the wording certifies the string, not the truth of it").

crates/rocm-core/src/diagnose.rs has a real registry: CHECKERS is the catalog of everything the command can say. So instead of two assertions on two sentences, there is now one every_message_the_diagnose_catalog_can_emit_is_ascii that walks CHECKERS over a corpus of Examinations and asserts both redirectable surfaces are ASCII — the rendered text report and the --json payload, which serde_json emits as raw UTF-8 rather than \u escapes. A check_20 added next month is covered the day it lands, for free.

The risk with a broad invariant is that it quietly stops reaching the branch it was written for and then passes for the wrong reason, so it ends with a coverage guard asserting the corpus still lights check_9's dGPU/APU branch and check_16's does-not-fit wording. Those markers sit beside the converted punctuation without containing it, so they stay true whichever way the dash goes.

I did consider going all the way to a single repo-wide source scan (walk the .rs sources, extract string literals, assert ASCII). I rejected it: outside rocm-dash-tui/e2e-report and #[cfg(test)] modules there are still ~20 legitimate non-ASCII literals — the eight gated spinner labels, the Braille frames, rocm-dash-core's status glyphs, a couple of panic! messages — so it would need a hand-rolled Rust lexer plus an allowlist encoding the judgment "is this user-facing output?". The allowlist becomes the thing that rots. Happy to revisit if you see it differently.

Three sites no unit test can reach

server.rs:125, runtime_dir.rs:468 and fix.rs:1331 are not reachable in-process: the first two need a directory owned by a second uid (a limitation your own file already records, on only_root_owned_sticky_dir_is_safe_shared), and fail! writes straight to the process's stderr. I did look at reaching the chmod branches for real against a root-owned path — it works unprivileged, but it either skips silently when CI runs as root, or (worse) succeeds and chmods a system directory. Not worth it.

So those three messages are lifted into named functions (unsecurable_socket_dir_context, unsecurable_runtime_dir_context, diagnosis_position_refusal) and asserted there — the same split you praised on model_catalog_platform_heading. Each test also asserts the remediation survives (the directory name, $XDG_RUNTIME_DIR/$HOME, the id:/apply with: pointer), so it is not purely an encoding pin.

Falsification

Each conversion reverted in isolation on the Linux devbox, named test observed red, restored, observed green. All nine:

Site	Test	Reverted alone
diagnose.rs:1185	every_message_the_diagnose_catalog_can_emit_is_ascii	FAILED — fix-9-igpu-dgpu ... discrete GPU — do not assume
diagnose.rs:1681-1682	same test	FAILED — fix-16-vllm-oom ... will not help — use a smaller
examine.rs:1667	the_report_lists_the_gpus_the_kernel_exposes_not_every_card_on_the_bus	FAILED — this is expected — only the passed-through GPUs
fix.rs:1331	a_position_argument_is_refused_with_the_same_exit_code	FAILED — against that cause — \rocm diagnose``
server.rs:125	the_unsecurable_socket_dir_refusal_is_ascii_and_names_the_remedy	FAILED — to mode 0700 — it is owned by another user
runtime_dir.rs:468	the_unsecurable_runtime_dir_context_is_ascii_and_names_the_directory	FAILED — to mode 0700 — it is most likely owned by
main.rs:4397	services_prune_skips_running_records_and_reports_the_count	FAILED — prune output
main.rs:13698	uninstall_warns_about_remote_sessions_in_ascii_and_names_the_stop_command (new)	FAILED — loses track of them — run \rocm remote stop``
main.rs:1845	setup_reset_cli_output_is_plain_and_persists_first_time_prompt	FAILED — reopen automatically — open it from

Done in three passes so attribution stays clean (the two diagnose.rs sites share a test, so they were reverted separately from each other). Each red run was 0 passed; 1 failed; each restore was 1 passed; 0 failed.

Gates on f504c57a, all on the remote Linux box:

• cargo test --workspace --all-targets --no-fail-fast — 33 groups, 3852 passed, 0 failed, 16 ignored (was 3847 before this commit; +4 new tests, the rest are extensions to existing ones)
• cargo clippy --workspace --all-targets -- -D warnings — exit 0, zero warnings
• cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings — exit 0
• cargo fmt --all

Non-blocking items

• test(serve): cover the restart path's startup death end to end (serve-25) #581 / serve-25 — confirmed and added to the PR description. test(serve): cover the restart path's startup death end to end (serve-25) #581 adds serve-24 and serve-25; feat(vllm): enhance OOM diagnostics and guidance in serve summary (eai-8059) #284 adds serve-25 through serve-28; this PR adds one serve-25. Nothing renumbered here, since feature_naming.rs::scenario_names_are_indexed_sequentially_per_feature checks position+1 in declaration order, so an index cannot be reserved or gapped — whichever merges last renumbers. Flagged for coordination.
• " - " vs " -- " — left alone. Those are untouched comments; normalising them would widen a diff that is already repo-wide, and the two never appear in the same rendered line.
• remote/mod.rs:2429 — agreed, left. It is a #[test] assertion message, never shipped, and converting it would be inconsistent with the test code this PR leaves untouched everywhere else.

[jussielo-amd]

Review: ROCMAI-543 — replace non-ASCII punctuation with ASCII in CLI output

Two things worth addressing before merge:

1. Internal tracking link in the PR description (blocking)

This repo's AGENTS.md section 2 ("Company Identity Required, Internal Content Forbidden") permits a bare ticket reference like ROCMAI-543 on upstream-facing surfaces, but not a link to the internal tracking system. The PR description currently links to the internal Atlassian instance in two places:

• the inline Fixes [ROCMAI-543](https://amd.atlassian.net/browse/ROCMAI-543): near the top
• the footnote-style reference definition near the bottom ([ROCMAI-543]: https://amd.atlassian.net/browse/ROCMAI-543?atlOrigin=...), whose query string also carries an embedded internal session/product identifier

Could you replace both with a plain ROCMAI-543 text reference (no hyperlink)? Requesting changes for this one since it's a leak onto a public PR rather than a style preference.

2. One remaining em dash not covered by the sweep (non-blocking, but worth a decision)

crates/rocm-dash-daemon/src/server.rs:287 still has:

info!(?cmd, "pause/resume — runner does not honor yet");

This is pre-existing code this PR doesn't touch (it falls outside every diff hunk in this file, so I can't attach it as an inline suggestion), but it's the same bug class: the line flows through apps/rocm/src/logging.rs's rocm_dash_daemon=info routing into a daily-rotated plain UTF-8 log file — a writer that, unlike the TTY-gated spinner ellipses this PR correctly exempts, is never gated on is_terminal(). On Windows, a tool assuming the OEM code page when reading that log file would hit the exact ROCMAI-543 mis-rendering bug this PR sets out to fix, and none of the new is_ascii() regression tests cover this call site (the nearest one, at line 342, covers a different message).

Could you either convert this one too, or note the gap explicitly in the PR's Scope section if it's intentionally out of scope?

[jussielo-amd]

Nit, non-blocking: this assert!(x.is_ascii(), "... (ROCMAI-543): {x}") pattern (here and the two similar asserts right below it) recurs across this PR's regression tests — similar asserts appear in examine.rs, fix.rs, main.rs, server.rs, and runtime_dir.rs. Given each site already extends an existing test per the PR's own Regression coverage table, this is a minor suggestion rather than something to block on — but a shared assert_ascii_output!(expr) test helper could keep the message from drifting in wording across sites independently.

[r0x0r]

Thanks — both items addressed in 80130961.

1. Internal tracking links — done

Both are gone from the description: the inline Fixes [ROCMAI-543](...) link and the footnote reference definition whose query string carried the atlOrigin identifier. Both are now a plain ROCMAI-543 text reference. Verified zero amd.atlassian.net occurrences remain in the rendered body.

2. server.rs:287 — converted, not excluded

I verified the whole chain you described before deciding, and it holds in every part:

• The line is there. crates/rocm-dash-daemon/src/server.rs:287, exactly as quoted.
• It is outside this PR's hunks. Combined main...HEAD diff for that file touches @@ -67,0 +68,18, @@ -123,9 +141, @@ -322 +332,23. Line 287 falls in handle_client (228–318), between the second and third hunk — so your inline-comment constraint was real, not a tooling quirk.
• The writer is not TTY-gated. apps/rocm/src/logging.rs sets DEFAULT_FILTER = "warn,rocm=info,rocm_dash_tui=info,rocm_dash_daemon=info" and builds the subscriber over tracing_appender::rolling::daily(...) wrapped in non_blocking. grep -c is_terminal apps/rocm/src/logging.rs returns 0 — there is no TTY gate anywhere in that file. So this is stronger than the redirectable-stream case the rest of the sweep targets: a log line takes the raw-bytes path by construction, never via WriteConsoleW.
• Nothing covered it. "pause/resume" appears at exactly one place in the tree (the call site itself); no test references it. The nearby test at 342 covers unsecurable_socket_dir_context, as you said.

Decision: convert. My Scope exemption is specifically "the bytes reach a real console, so no code page is consulted" — and that argument simply does not apply to a plain UTF-8 file. Excluding this would have made the Scope section state a rule the code then violated, which is worse than either converting it or genuinely narrowing the PR. It is also one line with no behavioural change.

It was the only one. A repo-wide scan of every info!/warn!/error!/debug!/trace! call site for non-ASCII found exactly one hit — this line. It is now zero, so a crate-wide invariant would have had nothing further to guard; a single assertion was the right size.

Coverage. The emitting branch sits inside handle_client's select! loop, which a unit test cannot drive without a live socket peer and a runner — the same reachability problem as the chmod branches. So the wording is lifted into const PAUSE_RESUME_UNSUPPORTED, mirroring the unsecurable_socket_dir_context split, and server::tests::the_pause_resume_notice_is_ascii pins both .is_ascii() and the exact string.

Falsified, per the same protocol as the other nine rows: reverting the em dash alone turned the suite red — 6 passed; 1 failed, panicking with the pause/resume log notice must be ASCII-only (ROCMAI-543): pause/resume — runner does not honor yet. Restored: 7 passed; 0 failed.

Gates (devbox, Linux, toolchain 1.96.0 per rust-toolchain.toml)

Gate	Result
cargo fmt --all	clean
cargo test --workspace --all-targets --no-fail-fast	3852 passed, 1 failed, 33 suites
cargo clippy --workspace --all-targets -- -D warnings	clean
cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings	clean

The one failure is cli_progress::tests::animated_spinner_keeps_ticking_without_progress_calls, a known timing flake under parallel load; re-run in isolation it passes (1 passed; 0 failed). It is unrelated to this change.

Description reconciled

The Scope section now states the log-file rule explicitly rather than leaving it implicit, the coverage table gains the server.rs pause/resume row, and the falsification count reads "all ten".

Commit: 80130961edb4412ff0286cf7b25706cb9ef36958

[jussielo-amd]

🔴 Automated review · pr-review-watcher · 8013096

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: fix(output): replace non-ASCII punctuation with ASCII in CLI output text

Checked out the branch locally, read every hunk plus enclosing functions, and ran
multiple independent finder passes followed by direct verification against the
actual working tree. The mechanical punctuation substitutions themselves are
correct everywhere checked, and the TTY-gated rocm-dash-tui spinner exemption
reasoning is sound. The issue is that the PR description's two headline claims are
each falsifiable, confirmed directly against the code rather than taken on trust:

• "a repo-wide scan of info!/warn!/error!/debug!/trace! call sites finds
  zero remaining [non-ASCII]"
• "every converted site now has a regression test that fails if the conversion
  alone is reverted"

Blocking

1. apps/rocm/src/dash.rs:150 — A leftover em dash remains inside a
   tracing::warn! message that the function's own doc comment confirms goes to
   the file-only client log (rocm-cli.log), not stdout/stderr — exactly the
   non-console sink class this PR exists to fix. A Windows tool reading that log
   on the OEM code page renders the em dash as ΓÇö, reproducing ROCMAI-543
   directly.

2. crates/rocm-dash-daemon/src/runner.rs:490 — A leftover em dash remains in
   a warning string pushed into Snapshot.warnings, which broadcast_and_persist
   → SessionWriter::append serializes to an NDJSON file on disk whenever
   persist_dir is wired — and apps/rocm/src/dash.rs:63 wires it
   unconditionally. On WSL/Windows hosts where amd-smi is unresolvable, every
   rocm dash session persists this exact warning byte-for-byte, reproducing the
   same mojibake class on replay/inspection of the session file.

3. apps/rocm/src/service_launch.rs:1640 — Of the four converted strings in
   this file (lines 679, 945, 953, 967), only one has any test, and that test's
   .contains("shown only now") assertion stops exactly before the converted
   hyphen; the other three ("streaming engine logs", "detached", "stopping
   server") have zero coverage. Reverting any of them to an em dash/ellipsis
   passes cargo test --workspace unnoticed, silently reintroducing mojibake on
   rocm serve/attach messages — among the most frequently seen CLI output.

4. apps/rocm/src/main.rs:11809 — ROLLBACK_RECOVERY_HINT's em-dash-to-hyphen
   conversion isn't listed in the PR's coverage table, and its only test
   (update_activate_summary_hints_rollback_only_when_a_previous_runtime_exists,
   main.rs:26938) splits two .contains() checks exactly around the converted
   character, so reinstating the em dash leaves the test green.

5. apps/rocm/src/remote/mod.rs:455 — All ~15 converted sites across
   remote/{mod,publish,doctor,install,provision,tailnet}.rs have zero
   is_ascii/exact-string regression coverage (confirmed: grep for is_ascii in
   that directory returns only unrelated identifier-validation code). A revert of
   any em dash in this module's remote-serve error/warning paths is invisible to
   CI despite the PR's blanket claim that every conversion was falsified
   red-then-green.

6. apps/rocm/src/dash.rs:742 — Same gap for the bench-command conversions
   (dash.rs:742, 762, 817, 838, 852) plus engines_cmd.rs's engine-shell strings
   (637, 640) and runtimes_cmd.rs's uninstall-failure message — roughly a third
   of the touched files have no test that would catch a silent revert.

7. crates/e2e-report/src/consolidated.rs:1316 — A leftover em dash
   (format!(" — {r}")) was not converted, despite the PR's description
   stating the sweep covers all genuinely user-facing output in crates/. This
   generator's output is always written to a file — matching the PR's own
   "follow the stream, not the character" rule for in-scope sites — so if that
   HTML is ever cat'd to a redirected Windows terminal or logged through a CP437
   pipe, the same mojibake class recurs in the one crates/ file the sweep
   missed.

Non-blocking

8. crates/rocm-core/src/diagnose.rs:2467 (altitude) — Correctness is
   enforced only by two narrow catalog-walk tests (diagnose CHECKERS, the clap
   help tree) rather than a durable, repo-wide mechanism (e.g. a clippy
   non-ASCII-literal lint, or a central output-sanitizing wrapper applied once at
   the point bytes leave the process — matching the PR's own root-cause framing).
   A future PR adding a message with a stray em dash anywhere outside those two
   catalogs is not caught by anything in CI — the same mechanism that let findings
   1 and 2 above ship inside this very PR.

9. apps/rocm/src/main.rs:22185 (simplification) —
   assert!(value.is_ascii(), "...must be ASCII-only (ROCMAI-543): {value}")
   boilerplate is copy-pasted at least five times in this module's own test code
   with no shared helper. A single fn assert_ascii(value: &str) helper would
   remove the duplication.

10. crates/rocm-core/src/diagnose.rs:866 (minor, test design) —
    every_message_the_diagnose_catalog_can_emit_is_ascii iterates all of
    CHECKERS directly, bypassing the OS-family filter that production's
    run_all_checks always applies, so it exercises checker/platform
    combinations that can never occur through the real diagnose() entry point.
    A future linux-only checker that assumes its declared OS family holds could
    fail this test against a synthetic "windows" Examination no real user can
    construct, producing a false failure unrelated to ASCII correctness.

What this covered

Full diff review of all files touched by this PR (base fee201c..head
8013096, 23 files), plus the enclosing functions and test modules for every
converted call site, cross-checked against the PR description's two coverage
claims by direct grep/read rather than by trusting the description.

[r0x0r]

Both headline claims in the description were wrong. They are now corrected in the description itself rather than only here.

Why the scan missed them — not a near-miss. The finder pass was anchored on the macro name (warn!, println!, ...). In both leftovers the em dash sits on a continuation line of a multi-line string literal, several lines below the macro the grep matched, so the match window never contained the dash. That blind spot applies to every multi-line literal in the tree, which is why two of them shipped inside the fix itself. No amount of extra per-site assertions would have caught either; the problem was never assertion strength, it was that nothing enumerated the sites.

The re-scan is byte-level and literal-oriented, and was cross-checked against a second independent implementation (the Rust one that ships here) which agrees site-for-site: 711 literals across 98 files contain non-ASCII. Classification is in the description; 5 were genuinely in scope and all 5 are converted.

1, 2 — confirmed, fixed (e6465265). dash.rs (file-only client log) and runner.rs (Snapshot.warnings → NDJSON session file). Both reverted individually, both red naming the string, green on restore.

3, 4, 5, 6 — confirmed real. I verified each: service_launch.rs's .contains("shown only now") does stop exactly before the converted hyphen, and update_activate_summary_hints_rollback_only_when_a_previous_runtime_exists does split its two .contains() checks around ROLLBACK_RECOVERY_HINT's converted character.

I did not fix them with 25 more per-site assertions. Item 8 is the right diagnosis, so the fix is the mechanism: xtask/src/ascii_output_contract.rs (a3f47b82) lexes every .rs file and inspects literals, so continuation lines are part of the literal. It also checks the data files include_str! compiles in — the label fields in model_catalog.json are what rocm model prints, and no Rust-source scan would see them.

On the earlier round's objection that an allowlist rots: the check is bidirectional, the same discipline crate_edges::ALLOWLIST already uses here. An exemption matching nothing fails as loudly as an unexempted literal, so the list can only ever describe what is really there, and a test requires every exemption to carry a reason. Every shape that defeated an earlier finder has a fixture, including the continuation line and the #[cfg(test)] use a::{b, c}; brace group that a naive "first #[cfg(test)] wins" rule would let exempt a whole file.

Falsification — all 74 conversions, reverted one at a time, red then green on restore:

Mechanism	Sites	Red
every_literal_that_can_reach_a_redirectable_sink_is_ascii (new)	50	50
every_help_text_in_the_command_tree_is_ascii (clap doc comments)	13	13
render_model_registry_text_lists_builtin_recipes (model_catalog.json)	3	3
The owning test itself (test-module expectation text)	8	8

That includes every site items 3–6 named: service_launch.rs 4/4, remote/{mod,publish,doctor,install,provision,tailnet}.rs 17/17, dash.rs 6/6, engines_cmd.rs 2/2, runtimes_cmd.rs 1/1, ROLLBACK_RECOVERY_HINT red.

7 — checked, and respectfully not taken. crates/e2e-report is a CI report generator, not CLI output: crate_edges::ALLOWLIST confirms it is reachable only from xtask and the e2e harness, never from the shipped rocm binary. Its HTML carries an explicit meta charset="utf-8" (consolidated.rs:941, single_report.rs:61), so it is decoded by declaration, not by a console code page. The decisive point is internal consistency: consolidated.rs holds 24 further non-ASCII literals besides the one at 1316 — the ✅/❌/⚠️ status vocabulary, ×, ·. Under the premise given, the fix is all 25 and the report loses its glyph vocabulary. Converting one and leaving twenty-four is exactly the inconsistency this PR was already criticised for in bench_run.rs.

9 — not taken; the premise doesn't hold. The seven is_ascii() assertions in main.rs are not copy-paste. Each names a different surface and interpolates a different value: rocm services prune output, the remote-session warning, the setup-reset prompt, the host-GPU marker, {path} --help, rocm model, rocm model --verbose. What repeats is is_ascii() and the ticket id — the shared concept. A single assert_ascii(value) would replace seven specific diagnostics with one generic message, so a failure would stop saying which surface broke. The message is the value, not the boilerplate.

10 — not taken; tightening it would weaken the invariant where it matters. ROCMAI-543 is a Windows bug and CI runs on Linux. Applying run_all_checks's OS-family filter means a Linux CI run never checks any Windows-only message in the catalog — precisely the strings most likely to meet an OEM code page. The test already calls the real diagnose() and asserts the rendered report, so the production path is covered; the CHECKERS loop is a deliberately broader net over text compiled into the binary regardless of host. The hypothesised false failure needs a checker that misbehaves reading a foreign os_family out of a plain data struct, which is a bug worth failing on.

Gate (remote Linux box, at a3f47b82): cargo clippy --workspace --all-targets -- -D warnings clean; cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings clean; cargo test --workspace --all-targets --no-fail-fast 33 groups, 3869 passed, 16 ignored, 1 failure — cli_progress::tests::animated_spinner_keeps_ticking_without_progress_calls, a known timing flake under parallel load, confirmed passing in isolation and untouched by this change. cargo fmt --all --check clean.

Verified at head a3f47b8: the whole-tree xtask/src/ascii_output_contract.rs contract added in the final commit (a3f47b8) lexically scans every non-test, non-exempt literal across apps/ and crates/ (confirmed: diagnose.rs, examine.rs, fix.rs, server.rs, runtime_dir.rs, and main.rs are not in its EXEMPT_PREFIXES list). That structurally satisfies "fails before fix, passes after fix" for every site this review named, without needing a named per-site assertion. Dismissing as resolved.

[jussielo-amd]

🔴 Automated review · pr-review-watcher · a3f47b8

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.

Before this round: two earlier change-request reviews on this account (at 2a118bd5 and f504c57a) are dismissed as resolved, verified against the current head rather than assumed —

• the "no regression test would catch a revert" findings (diagnose.rs, examine.rs, fix.rs, server.rs, runtime_dir.rs, main.rs) are structurally covered by the whole-tree xtask/src/ascii_output_contract.rs contract added in the final commit: confirmed none of those paths are in its EXEMPT_PREFIXES, so a reverted literal in any of them now fails that test directly.
• the internal amd.atlassian.net link is no longer in the PR description (confirmed by re-reading the current body).
• the server.rs:287 pause/resume em dash was converted in commit 80130961.

This round is a fresh, independent pass over the full diff (fanned out across the same file groupings as before, plus a dedicated pass on xtask/src/ascii_output_contract.rs itself and an AGENTS.md-adherence pass), with new findings below.

Review — needs work

Full review of the whole change (071664500405f62095fea1310b46b9f0a56e22f6…a3f47b82fe90bb34a638baff700c12363a7c20a1). Retrieved ROCMAI-543 from Jira directly: still "In Progress" (expected, PR open); both its acceptance criteria (ASCII-only text; check other commands) are addressed by this change's scope methodology.

Blocking

• \u{XXXX} Unicode escapes defeat the new ASCII output-contract lexer entirely — xtask/src/ascii_output_contract.rs:513-518 (quoted_string_at), xtask/src/ascii_output_contract.rs:541-546 (char_literal_at)
  Both functions handle a backslash escape generically by copying exactly \ + the next byte into the recorded literal and resuming scanning there. For \n, \", etc. that's correct, but \u{2014} is longer: only \u is consumed as "the escape," and the remaining {2014} falls through as plain ASCII digits/braces, which get pushed into literal unchanged. The recorded text is therefore all-ASCII even though the escape denotes U+2014 (em dash), so the literal.is_ascii() check in record() never fires. Traced by hand against the exact code: for "a \u{2014} b", the loop consumes \u as the 2-byte escape at line 516-518, then pushes {, 2, 0, 1, 4, } one at a time via the fallback branch — never decoding to the actual scalar value. This directly contradicts the function's own comment at :513-515 ("an escape sequence never is [non-ASCII]"), and the fixture escaped_quotes_and_escape_sequences_do_not_confuse_the_scanner only exercises \" and \u{41} (ASCII 'A'), never a non-ASCII escape target, so the gap ships with a passing, confidently-named test. A future (or reverted) fix written as "\u{2014}" instead of a literal em dash would stay invisible to this guard while the UTF-8 bytes it exists to block still reach the stream.
  Confidence 100 · logic · Fix: decode \u{...} to its scalar value before the ASCII check (or conservatively treat any \u{...} escape as non-ASCII), and add a fixture asserting "\u{2014}" is detected.

• #[cfg(test)] directly above a brace-initialized const/static is misclassified, which can wrongly flag legitimate test-only literals as production violations — xtask/src/ascii_output_contract.rs:376-396, specifically the tail.starts_with(';') check at :389
  The brace-group/body disambiguation decides "is this a use {...}; group (spent attribute) or an item body (push a test span)?" by checking whether the closing } is immediately followed by ;. That correctly separates #[cfg(test)] use a::{b, c}; from #[cfg(test)] fn t() { ... } (function bodies don't trail ;) — but #[cfg(test)] const X: Foo = Foo { field: "…" }; also closes its brace group with ;, so it takes the same "spent attribute" branch and is not pushed as a test span. Verified against the exact disambiguation logic: a const/static of this shape is read as production code, so non-ASCII content inside it would be wrongly flagged, forcing an unnecessary EXEMPT_LITERALS entry for legitimate test fixture data rather than hiding a real violation. No such pattern exists in the tree today (confirmed by grep), so this is currently dormant rather than live, but it's an objectively wrong classification in new code.
  Confidence 78 · logic · Fix: track the keyword between the attribute and the brace (use vs. const/static) instead of inferring it from the trailing punctuation.

• The e2e scenario's stated check is narrower than what it actually enforces — tests/e2e-cucumber/features/model_serving.feature:335, tests/e2e-cucumber/tests/e2e/serving_steps.rs:1314,1322
  The Gherkin reads "the model list output contains no non-ASCII punctuation," but the step implements assert!(output.is_ascii(), ...), which fails on any non-ASCII character — a letter, digit, CJK text, an accented name — not just punctuation. It's harmless today (model_catalog.json at PR head is fully ASCII), but the scenario's prose and its enforced invariant diverge: a future legitimate non-punctuation non-ASCII value would trip this test for a reason the Gherkin text doesn't describe, misleading whoever debugs the failure from the scenario alone.
  Confidence 72 · logic · Fix: reword the Then step to "...contains only ASCII characters" to match what's actually enforced (simplest), or narrow the assertion to punctuation-class characters to match the current wording.

Non-blocking

None.

Decisions for the author

• Keep the per-site extracted helpers/tests now that the whole-tree lexer exists, or retire them? — tradeoff. crates/rocm-dash-daemon/src/server.rs extracts unsecurable_socket_dir_context/PAUSE_RESUME_UNSUPPORTED into named items specifically to get per-site unit tests, even though xtask/src/ascii_output_contract.rs now makes "revert the ASCII conversion → red" structural rather than per-site. Both mechanisms coexist in this diff. Keeping both has a real reason (the per-site tests also pin remediation wording like $XDG_RUNTIME_DIR/$HOME, which the ASCII-only scanner can't check), but worth confirming that's the intended going-forward convention rather than transitional duplication.
• Is the rocm model Gherkin scenario meant to be the sole BDD-level coverage for this whole fix? — tradeoff. The new scenario is deliberately scoped to rocm model ("the flagship instance of that bug class" per its own comment), while the underlying fix touches ~15+ other files/commands, none of which get their own scenario. The regression-coverage table shows each of those sites has a falsified unit-level test instead, and AGENTS.md's Gherkin requirement is scoped to "user-observable behavior," not every output site — plausibly a deliberate, defensible choice, but worth stating explicitly rather than leaving implicit.

Positive signals

• EXEMPT_LITERALS/EXEMPT_PREFIXES/EXEMPT_EMBEDDED require a reason per entry and are bidirectionally staleness-checked (an exemption matching nothing fails as loudly as an unexempted literal) — confirmed against the code and against the existing crate_edges::ALLOWLIST house pattern it's modeled on; it is not just claimed, it's real.
• The two spinner labels in apps/rocm/src/therock.rs ("Downloading {}…" / "Extracting {}…") that one of this round's own fanned-out passes initially flagged as missed conversions turned out to be deliberately and correctly exempted: traced Spinner::render_current and confirmed it returns before any write when stderr().is_terminal() is false, and both exact literals are already present in EXEMPT_LITERALS with that reasoning.
• The PR description transparently retracts two of its own earlier false claims (an incomplete grep sweep, and an overclaimed "every conversion has a reverting test") with concrete falsification evidence for the correction, rather than quietly fixing and re-asserting completeness.

Deployment notes

None — this adds a new xtask CI-side lint and converts string literals; no runtime migration, API, or shipped-binary behavior change beyond the text itself.

What this covered

Reviewed the full diff (29 files, 1523 insertions / 101 deletions) via 7 independent blind fan-out passes grouped by coupling (xtask ASCII-contract lexer + model_catalog.{json,rs}; apps/rocm core CLI + dash; the smaller apps/rocm command files; core diagnostics/fix catalog; dash daemon + collectors; engines/lemonade runtime_dir + docs; the e2e BDD scenario), plus 2 additive source-reading passes (AGENTS.md adherence across the whole diff; git history + search for related prior PRs across the touched files) — 9 workers total, each reading its assigned files completely at the PR head plus relevant surrounding code. The driver then independently re-verified the three highest-stakes candidates directly against the PR-head source: confirmed both xtask lexer bugs by hand-tracing the exact byte-level logic, and refuted a worker's claim about the two therock.rs spinner labels by tracing Spinner's is_terminal() gate and finding both literals already in the contract's own exemption table. Checked gh pr view --json statusCheckRollup for the current head: every check (build-and-test, windows-build-and-test, Test (affected crates), clippy, coverage ratchet, E2E tests, CodeQL, commit-signatures) is SUCCESS. Retrieved ROCMAI-543 from Jira directly by identifier, and verified the two now-dismissed prior change requests against the current head before dismissing either.

Did not run: cargo test/cargo clippy/the xtask binary directly (relied on direct source tracing plus the PR's own green CI). Not reconciled against this PR's own GitHub review comments or revision history beyond what was needed to verify the two dismissed reviews — the findings above were reached blind to that discussion.

[r0x0r]

Addressed in aba1c4cd (merged with main in f367866f) and 80addb44.

1. \u{XXXX} escapes defeated the lexer — confirmed, and it was worse than one escape form. The scanner copied \ plus one byte and resumed, so \u{2014} left {2014} to be read as ordinary ASCII and is_ascii() never fired. Fixed by escape_denotes_non_ascii, which walks escapes in order and decodes them: \u{...} is decoded to its scalar and compared against 0x80, and \xNN above 7F counts too, since that is legal in a byte string and puts the byte straight on the wire. Walking in order rather than searching is what makes "\\u{2014}" read correctly — the \\ is consumed as one escape, so the following u{2014} is the ASCII text it really is. Anything malformed counts as a finding rather than being skipped.

Raw strings were a sibling hole in the same shape, found while enumerating the literal forms rather than from the review: r"..." has no escapes at all, so an Escapes::Literal variant now records them without escape interpretation. The fixture that previously claimed to cover escapes only exercised \" and \u{41} — which is ASCII A — and has been replaced accordingly.

2. #[cfg(test)] before a brace-initialised const/static — confirmed. The disambiguation keyed off a trailing ;, which const X: Foo = Foo { .. }; also has, so it took the use-group branch and was read as production code. It now tracks the keyword between the attribute and the brace, which is what actually distinguishes the two. Fixture added for the crowded single-line form.

3. Gherkin prose vs assertion — confirmed. The step asserted output.is_ascii(), which fails on any non-ASCII character, not only punctuation. Reworded to "the model list output contains only ASCII characters" to match what is enforced; the assertion is unchanged, so the coverage is the same and only the description is now accurate.

4. A clippy failure of our own. cargo clippy --workspace --all-targets -- -D warnings was red on the contract's own tests (two unnested or-patterns, two needless borrows) — fixed in 80addb44. Worth noting plainly: the branch had not been run against that gate since the contract landed.

Gate at 80addb44 (Linux devbox): cargo test --workspace --all-targets --no-fail-fast exit 0 at the pre-clippy-fix tree; cargo test -p xtask --all-targets 286 passed / 0 failed; cargo clippy --workspace --all-targets -- -D warnings exit 0; cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings exit 0. Both commits GPG-signed with DCO sign-off; the merge commit carries its sign-off too.

On the two decisions you raised. The per-site extracted helpers in server.rs should stay: their tests also pin remediation wording ($XDG_RUNTIME_DIR, $HOME) that an ASCII-only scanner cannot check, so retiring them would lose coverage the contract does not replace — the two mechanisms are complementary rather than transitional. And yes, the rocm model scenario is intended as the sole BDD-level coverage: it is the ticket's own reproduction, every other converted site is covered structurally by the contract plus its falsified unit tests, and AGENTS.md scopes the Gherkin requirement to user-observable behaviour rather than every output site.

[jussielo-amd]

🔴 Automated review · pr-review-watcher · 80addb4

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.

Before this round

The three blocking items from the standing change request on this account (review submitted at a3f47b82) are verified resolved at the current head (80addb44), checked directly against source rather than assumed:

• \u{XXXX} escape decoding — xtask/src/ascii_output_contract.rs now has a test fixture asserting "\u{2014}" is detected (found[0].1.contains("\\u{2014}")), and the surrounding comment confirms the escape is consumed correctly before the ASCII check. Fixed.
• #[cfg(test)] const/static misclassification — the disambiguation logic now explicitly reads the keyword between the attribute and the brace (use vs. anything else) instead of inferring it from the trailing ;, with a comment naming the exact const X: Foo = Foo { .. }; case this fixes. Fixed.
• Gherkin step wording vs. enforced assertion — the step is now #[then("the model list output contains only ASCII characters")], matching the output.is_ascii() assertion it wraps; the feature file's own comment explains the "ASCII-only, not punctuation-only" scoping. Fixed.

This round is a fresh pass over the current diff, focused on the new xtask/src/ascii_output_contract.rs lexer (the PR's central safety mechanism), with one new blocking finding and two non-blocking ones.

Review — needs work

Blocking

1. xtask/src/ascii_output_contract.rs:595 (bracket_group_end) — byte-level bracket counting desyncs on a ] inside a string literal, and the corruption cascades past the literal it occurs in. bracket_group_end is called (at :434) specifically to skip the [&str; 2]-style bracket group between a #[cfg(test)] attribute and its body/initializer, so the scanner's use-vs-item state isn't confused by a ; inside an array type. It counts [/] at the raw byte level with no awareness of string contents. Traced by hand against the exact logic: for #[cfg(test)] static X: [&str; 2] = ["a]b", "\u{2014}"];, the ] inside "a]b" is hit before the real closing ], so depth reaches 0 one character early and the function returns a position still inside the first string literal (at the b right after it). The main loop resumes from there, which means the real closing " of "a]b" is now read as if it opened a new string, pairing with the next " to produce a spurious literal (", "); the resumed scan then hits the { and } of \u{2014} as if they were real code braces, which — given pending_test is still true and open_test is still None at that point — sets open_test from that position rather than the item's real body, corrupting the test-span tracking state for everything that follows in the file. The practical risk isn't limited to this one fixture: once the state machine desyncs, the scan can mis-track test-span boundaries for subsequent code in the same file, which can mask a genuinely non-ASCII production literal later on — the exact failure mode the whole-tree contract exists to make impossible. No existing fixture puts a ] inside an array element's string, so this path is untested, and nothing in CI would currently catch it. Fix: track string/char literal boundaries while walking the bracket group (reuse quoted_string_at/char_literal_at/raw_string_at inside bracket_group_end the same way the outer loop does), or have it bail out and fall back to a full re-scan from at if it crosses anything that looks like a string start.

Non-blocking

2. xtask/src/ascii_output_contract.rs:878 (embedded_data_files) — the test-only exemption is scoped by the including file's path, not by the include_str! call site, unlike non_ascii_literals's test-span skip, which is scoped per call site. crates/rocm-dash-tui/src/skills.rs is a live instance of the gap: it is a production file (not test-only, not under tests/), and its RELEASE_FIXTURE (include_str!("../tests/fixtures/lemonade_release_latest.json") at skills.rs:451) sits inside a #[cfg(test)] mod tests block starting at :415. It doesn't fail today only because that fixture happens to be pure ASCII. A future fixture carrying a real non-ASCII character (e.g. a captured API response with a unicode name), added the same way, would trip every_embedded_data_file_is_ascii as a false positive, forcing an unnecessary EXEMPT_EMBEDDED entry for data that never reaches a user-facing stream. Not in this PR's diff, but exposed by the new test this PR adds.

3. xtask/src/ascii_output_contract.rs:582 (starts_test_attribute / the comment-skip logic) — only //-style comments (including /// doc comments) are recognized as exempt; the equivalent #[doc = "..."] or #[deprecated(note = "...")] attribute-argument form isn't. Semantically identical prose-about-code is scanned inconsistently depending on syntax. No instance exists in the tree today, so this is dormant rather than live — worth a one-line note for the next contributor who hits it, not worth holding the PR for.

Before merge (process, not a diff defect)

CI currently shows E2E tests (Strix Halo, Windows) failing on diagnose-fix-path-prepends-with-apostrophe-intact. Traced: that scenario exercises ps_single_quote_escape/ps_env_scope in fix.rs, none of which this diff touches, and the scenario already exists on origin/main (introduced by already-merged commit fee201cc) — this reads as a pre-existing flaky/stateful-runner issue rather than a regression caused by this change. Per this repo's own CI-ownership policy, a red check still needs to be driven green or explicitly triaged as a known flake in the PR before merge, rather than left unaddressed.

What this covered

Full re-read of xtask/src/ascii_output_contract.rs at the current head (80addb44), cross-checked against the standing change request's three blockers by diffing the relevant functions against what they looked like at a3f47b82. Hand-traced the bracket_group_end bug byte-by-byte against a constructed fixture rather than trusting a prior pass's description of it, and confirmed the embedded_data_files gap against a real instance already in the tree (crates/rocm-dash-tui/src/skills.rs). Checked current statusCheckRollup directly via gh pr view --json statusCheckRollup, and confirmed the Windows E2E failure's scenario and implicated code predate this PR via git show/git log on origin/main. Did not re-run cargo test/cargo clippy directly for this round; relied on direct source tracing plus the PR's own CI results for everything else.

[r0x0r]

Thanks — the blocking finding is real, and the traced consequence is worse than an off-by-one. Fixed in c40afd74.

Blocking: bracket_group_end desync — verified, and what I demonstrated

I did not take the trace on trust. Because the lexer in xtask/src/ascii_output_contract.rs is self-contained (Finding through is_ident_byte depend on nothing outside themselves), I lifted that range verbatim out of the file at 80addb44 and compiled it standalone under the pinned 1.96.0, so I could run the shipped code against constructed fixtures and read the actual output rather than reason about it.

The reviewer's index trace is right. The consequence I measured is the one that matters:

#[cfg(test)]
static X: [&str; 2] = ["a]b", "c"];

fn f() {
    let _ = "<em dash>";          // line 5, production output
}

Against the code at 80addb44 this reports zero findings. The em dash on line 5 is production text in a non-test item, and the guard does not see it.

The mechanism, confirmed byte by byte: the ] inside "a]b" drives depth to 0 early, so bracket_group_end returns at the b. The main loop resumes there, quoted_string_at reads b" as a byte-string opener, and from that point every quote in the file is paired off by one — the scan then consumes "];\n\nfn f() {\n let _ = as a single literal, lands on the em dash as bare code, skips it as a non-ident byte, and opens yet another string on its closing quote that runs to EOF. The literal is never recorded. The test-span tracking is corrupted too (in the "\u{2014}" variant the { and } of the escape set open_test), but it is the quote-pairing cascade that does the actual hiding. This is the silent miss the whole-tree contract exists to make impossible, so it is correctly blocking.

Five spellings all fail the same way at 80addb44:

array between #[cfg(test)] and the item	findings at 80addb44	expected
["a]b", "c"]	0	1
["a]b", "\u{2014}"]	0	1
[r#"a]b"#, "c"]	0	1
[b"a]b", b"c"]	0	1
["a\"]b", "c"]	0	1

5/5 report zero findings where one is required. Directly on the helper, 8/8 tricky groups return the wrong end offset (e.g. ["a]b", "c"]! → 4, want 12; [\n // a ] in prose\n "a",\n]! → 12, want 32).

Fix chosen, and why not the other one

Walk the group with the same recognisers the outer loop already uses (raw_string_at / quoted_string_at / char_literal_at, plus // and nested /* */), rather than the suggested bail-out-and-re-scan.

Two reasons. First, re-scanning from at gives up the one thing this function exists to do — step over the ; in [&str; 2] — so it would reintroduce the bug an_array_type_semicolon_does_not_end_a_test_item_early was added for; trading a false negative for a false positive in a function whose whole job is the semicolon is not a trade. Second, the recognisers are already the file's tested authority on every string form the language has (raw at any hash count, b/c/br/cr, escaped quotes, chars vs. lifetimes) — a second, weaker "looks like a string start" heuristic would be a new thing to keep in sync. Passing line = 0 makes the returned line the newline count, so the existing line accounting is unchanged.

Falsification

Both new fixtures fail completely against 80addb44 and pass against c40afd74:

• a_bracket_inside_a_literal_does_not_hide_later_production_text — 5/5 shapes fail before (zero findings each), 5/5 pass after. It asserts the consequence you flagged as the real risk: a later production literal in the same file, after a tricky array, is still reported, at the right line and with the right text. Not an offset check.
• bracket_group_end_counts_brackets_on_code_bytes_only — 8/8 groups fail before, 8/8 pass after. Pins the helper's own offsets and newline count.

One thing I will not overclaim, given this PR's history with confidently-named fixtures: I could not reproduce a hidden literal through a comment. A ] in a // or /* */ comment inside the group did return a mid-comment offset before the fix, but the main loop resynchronised on the group's real ] in every shape I tried (unbalanced quote in the comment, a { in the comment, extra trailing }s — all still reported the production literal). Comments are handled for completeness and the fixture's doc comment says exactly that rather than claiming a consequence I did not observe.

All pre-existing fixtures that touch this path still pass unchanged (an_array_type_semicolon_does_not_end_a_test_item_early, the cfg_test_const pair, an_escaped_quote_char_literal_is_consumed_whole, a_multi_line_test_attribute_keeps_the_line_count_straight).

Non-blocking 2 — embedded_data_files call-site scoping: verified, recorded, deliberately not changed

The instance is real: crates/rocm-dash-tui/src/skills.rs:451 is an include_str! inside the #[cfg(test)] mod tests that starts at :415, in a production file that embedded_data_files does walk (it filters on is_test_only / tests/ prefix, not on EXEMPT_PREFIXES), and it passes only because the fixture is ASCII.

I am leaving the scoping as it is, and documenting why in the function's docs. Scoping by call site means asking the test-span tracker in non_ascii_literals which embeds to drop — and the finding above is a demonstration that that tracker can desync. Wiring it into every_embedded_data_file_is_ascii would give this test a way to fail open on a real model_catalog.json-class embed. Path-based scoping is independent of the lexer and cannot. As you note, a false positive is the cheaper error here: its cost is one EXEMPT_EMBEDDED entry with a one-line reason, which is bounded and visible. The doc note names the skills.rs instance and says to prefer the entry, and to revisit call-site scoping only if they start accumulating.

Non-blocking 3 — #[doc = "..."] prose: noted

Confirmed dormant (no instance in the tree). Added the one-line note to non_ascii_literals's doc comment, telling the next contributor who hits it to widen the comment skip rather than add an EXEMPT_LITERALS entry for prose.

Process: the red Windows check — triage, not a fix

E2E tests (Strix Halo, Windows) is red and I am not claiming it is fixed. Triage:

The lane fails four scenarios: diagnose-38, serve-01, serve-02, runtime-02 (142 scenarios, 138 passed, 4 failed; 3 are xfail, diagnose-38 is the unexpected one).

#447 (fix(serve): run the KFD pre-flight at every amd-smi launch site, an unrelated change) fails the identical four on the same lane — job 116597624858, 141 scenarios, 137 passed, 4 failed, same three xfail, same FAIL: 'diagnose-fix-path-prepends-with-apostrophe-intact', same four failing step texts. Two unrelated PRs failing the identical set is evidence about the lane, not about either diff.

diagnose-38 comes from already-merged fee201cc ("stop fix-6-path from wiping Windows User PATH on a failed read", #691), which introduced both the scenario and the step. This PR touches none of it.

The assertion it trips on is over-broad, and the CI log shows it plainly. tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1495 asserts !output.contains("setx ") over the whole command output — but that output includes the catalog's own advice block, and crates/rocm-core/src/fix.rs:377 is literally:

"setx PATH \"%PATH%;C:\\Program Files\\AMD\\ROCm\\<version>\\bin\"",

The same captured output shows the actual PATH write doing the right thing:

Plan: prepend ...\fake-hip-sdk\bin to your User PATH:
  [Environment]::SetEnvironmentVariable('PATH', '...;C:\Users\O''Brien\bin;...', 'User')

So the behaviour under test is correct — apostrophe intact, PowerShell write, no setx — and the assertion is matching the advice string in the recipe listing instead of the write. That is a defect in merged test code, owned by fee201cc/#691, not by this diff and not something this diff can turn green. Recording it here per the CI-ownership policy.

Gate

All four, on the Linux devbox at c40afd74 (macOS cannot compile this tree):

• cargo fmt --all --check — clean
• cargo test --workspace --all-targets --no-fail-fast — rc 0, 33 binaries, 3974 passed, 0 failed
• cargo clippy --workspace --all-targets -- -D warnings — rc 0, 0 warnings
• cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings — rc 0, 0 warnings

cargo test -p xtask --all-targets on its own: 288 passed, 0 failed.

Commit: c40afd74.

[r0x0r]

Merged origin/main (c9e9a327, which carries #676) in a6a6c5ca.

One conflict, and it was structure-vs-text rather than two edits to the same logic. In apps/rocm/src/remote/install.rs, #676 replaced the NeedsAHuman arm with a much richer one — bullets(findings), a separate without_a_fix branch, and a new NothingToInstall variant — while this PR had only converted the old arm's punctuation. Resolved by taking #676's structure wholesale and re-applying this PR's conversion to the em dashes inside it.

#676's new text carried three em dashes in string literals, which this PR's own contract would have failed on:

• "...first — the suggested fixes are below.{}"
• "...Also reported, with no fix to apply — these describe the machine..."
• "...no install command clears it — running one on that machine..." (in NothingToInstall, outside the conflict region entirely)

All three converted. The third is the interesting one: it auto-merged cleanly and sits nowhere near the conflict markers, so it would have been easy to miss by only resolving what git flagged. A file-wide non-ASCII scan after resolving is what caught it. Every remaining non-ASCII in that file is in //, /// or //! comments, which the contract does not scan.

Gate at a6a6c5ca (Linux devbox): cargo test -p xtask --all-targets 310 passed / 0 failed — this is the one that matters here, since it includes every_output_bearing_literal_is_ascii, and it is what proves the three conversions above were both necessary and sufficient. Then cargo test --workspace --all-targets --no-fail-fast exit 0 across 33 suites; cargo clippy --workspace --all-targets -- -D warnings exit 0; cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings exit 0; cargo fmt --all clean. Merge commit is GPG-signed and carries its DCO sign-off.

Scenario numbering is untouched by this merge; serve-25 is still contested with #284 and #581, and whoever merges last renumbers.