[rominf]

Summary

#500 fixed the whitespace case of #443 in the dash approval previews by wrapping such values in double quotes. This PR finishes the job. It covers the CLI-side previews #500 did not touch, and it changes the dash quoter so its output reads back as the exact argv in a shell, not only for values containing spaces.

A preview is what the operator approves, and some previews are offered to be copied and run. So the rendered line has to split back, in a POSIX shell, to exactly the argv the CLI runs. Before this change that held only for values with no shell-significant characters.

CLI previews (not touched by #500)

• format_structured_tool_call renders the chat approval card, the advanced manual command: line, the tool_call: lines in rocm <request>'s request plan, and the copyable After the server is ready, run: line. It quoted a value only when it contained whitespace, and then used double quotes:
  • a lone " produced a line no shell can parse;
  • $HOME in a copyable rocm chat --prompt … expanded in the reader's own shell when pasted.
• therock::quote_display_arg renders the install dry-run's command: uv … && uv … line. It left /tmp/`id` completely bare and kept $(id) live inside its double quotes. Both executed id when the line was pasted. The install root comes from a --prefix the local assistant can choose, because install_sdk_dry_run is a model-callable tool.
• Both now use one POSIX single-quote quoter. It already existed for the remote command line and moves to apps/rocm/src/shell_quote.rs.

Dash previews (#500's helper)

• ui::exec::quote_display_arg keeps its name, its API and all of fix(dash-tui): quote whitespace in approval-preview args #500's call sites. Its body changes from "double-quote on whitespace and a short metacharacter list" to the same POSIX single-quote rule. Values that were still mis-rendered:
  • $HOME and `id` (left bare);
  • a value ending in \ (escapes its own closing quote);
  • a lone ';
  • #, which turns the rest of the line into a comment;
  • *, ? and ~.
• fix(dash-tui): quote whitespace in approval-preview args #500's example tests and its two TestBackend snapshot tests now expect '…'.
• The dash crate keeps a separate copy of the quoter because xtask's crate-edge contract forbids rocm-dash-tui -> rocm-core. Each copy is checked against a real sh by its own tests.

Control characters

• Quoting cannot make a terminal control character safe: '…' is literal to a shell, but not to the terminal that receives a \r or a CSI sequence. Model-supplied arguments carrying one are now refused before they reach a printed block.
• The guard applies to every chat tool whose arguments reach the terminal. That includes path_exists, whose path is echoed into an ungated block. The guarded list is pinned per entry by a test.
• rocm_command widens its own check from \0/\n/\r to every control character. ESC and the 8-bit CSI previously reached the approval card.
• Provider-planner args get the same rule. Planner notes, which are printed verbatim into the plan block, are dropped if they carry a control character, including \n and \t.

Behaviour change to be aware of

• Previews now read 'C:\Program Files\ROCm' rather than "C:\Program Files\ROCm":

  • bash and WSL read the line back exactly;
  • PowerShell reads it back exactly unless a value contains ';
  • cmd.exe does not strip single quotes, so a preview pasted there is no longer runnable as-is.

  On every platform the preview is now unambiguous to a reader, which is what an approval gate needs. The old double-quoted form was ambiguous everywhere.

• A model-supplied path containing a tab is now refused by the guarded tools.

Tests

• Property tests (proptest):

  • every rendered argv splits back through a POSIX word-splitter (shlex) to exactly that argv;
  • no argument adds a word to the line;
  • quoting twice still round-trips.

• Generator reach: arguments are drawn from a small alphabet that is mostly metacharacters. A test measures how often the generator reaches quotes, metacharacters, whitespace, empty arguments and backslashes, and fails below a floor.

• Real-sh round trips: shlex performs no expansion, so it calls $HOME or ~ left bare a clean round-trip. A property test on each quoter therefore hands the rendered line to /bin/sh and compares the words printf receives. The shell runs in a directory with glob bait and a sentinel HOME. The dry-run command: test asks a real sh for the same reason.

• Revert check: with the old quoters and the old control-character checks restored, 20 rocm tests fail. One example: minimal failing input: args = ["\""], rendered as rocm ", which does not parse. The new dash tests run against fix(dash-tui): quote whitespace in approval-preview args #500's quoter fail 10 of 15. One example: preview = "rocm #" splits back to ["rocm"], so the argument vanishes.

• e2e (AGENTS.md §3): two new mock-lane scenarios, both offline and untagged:

  • @id:runtime-freeform-prefix-with-quote-is-one-argument (runtime-19): a natural-language install into a folder named it's-here prints tool_call: lines that read back with that folder as one argument.
  • @id:therock-next-11-dry-run-command-keeps-a-quoted-folder-whole (therock-next-11): the dry-run command: line keeps that folder as one word.

  Both fail with the old quoters restored. The folder name carries a quote rather than a space because the old CLI quoter already wrapped whitespace, so a space would not distinguish old from new on this surface.

Not covered by a scenario, and why:

• Chat approval card: the e2e mock server's chat endpoint always returns a fixed text reply with no tool_calls, so no scenario can reach the approval card. Unit and property tests cover the renderer.
• Dash approval cards: the folder-browser flows cannot be driven to a hostile path until the harness can set the TUI's working directory (e2e: let TuiSession spawn with a controllable working directory #501). fix(dash-tui): quote whitespace in approval-preview args #500's TestBackend snapshot tests cover the rendered widgets.

Local verification

Command	Exit code
cargo test -p rocm --all-targets	0
cargo test -p rocm-core --lib	0
cargo test -p rocm-dash-tui --all-targets --no-fail-fast	101 (see below)
cargo clippy --locked --workspace --all-targets -- -D warnings	0
cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings	0
cargo fmt --all --check	0
cargo xtask manifest --check	0
cargo xtask tpn --check	0
cargo test -p e2e-cucumber --test feature_naming	0

• The rocm-dash-tui run fails only in 3 agent::clients tests. They cannot create a token cache under a read-only $HOME in my environment, and they are unrelated to this change. The 7 wave0_job_bridge tests, including fix(dash-tui): quote whitespace in approval-preview args #500's snapshots, pass.
• cargo xtask e2e -- -n 'runtime-|therock-next-|dash-': runtime-19 and therock-next-11 pass. The scenarios that fail locally need a detectable GPU family, live downloads, or a writable $HOME on this WSL host without a GPU, and none of them asserts preview text.
• GPU and Windows lanes were not run locally.

Dependencies

• proptest uses rocm-core's existing spec (default-features = false, features = ["std"]).
• shlex 2 is a dev-dependency. It was already in Cargo.lock, so there are no new crates and MANIFEST.md and THIRD_PARTY_NOTICES.txt are unchanged.

Fixes #443

Since the review of 55873c99

• One quoter. apps/rocm's shell_quote delegates to ui::exec::quote_display_arg, since apps/rocm already depends on rocm-dash-tui. The quoter's round-trip and real-sh tests live beside it.
• Deny-by-default guard. Every chat tool's arguments are screened, including tools added later, except rocm_command, which screens its own argv.
  • Tool names are screened too, and no refusal echoes the value it refused: an unsupported tool, a non-local host, or the planner's tool or intent. Planner failure notes are stripped of control characters.
  • Planner notes are trimmed, then dropped if they carry a control or format character, and the drop is reported as "N planner note(s) omitted".
  • The predicate also covers Unicode format characters (bidi overrides, zero-width characters) and U+2028/U+2029.
• Behaviour change. A model-supplied natural_language_plan request containing a newline, and any argument value containing bidi or zero-width characters, is now refused.
• Tests. The "no argument adds a word" tests are dropped, since the round-trip property implies them. There are new deny-by-default, nested-argument and no-echo tests; each check's removal turns one red.
• Not covered by a scenario.
  • The model tool-call refusal and planner-note dropping can't be reached offline: the mock chat endpoint returns fixed text with no tool_calls.
  • The rocm serve '<model>' placeholder is reachable offline but not yet covered; that is a follow-up.
  • The older unquoted hints (rocm logs <id>, runtimes activate, remote doctor, enable-provider) are a follow-up.

[jussielo-amd]

This is careful, well-tested security work. The POSIX single-quote quoter is correct (verified the escaping logic by hand and against the property tests), every preview site the description claims is actually wired to it with no leftover ad hoc quoting, and the control-character guard closes a real gap — rejecting CSI/ESC sequences reaching the terminal, not just NUL/CR/LF.

The test strategy is the standout here: round-tripping through both a POSIX word-splitter and a real /bin/sh (with glob bait and a sentinel $HOME) is exactly what's needed to catch a quoter that looks correct to a naive splitter but still lets $HOME or `id` expand on paste. The two new e2e scenarios are genuinely discriminating against the old quoters, not just plausible-looking. The documented coverage gaps for the chat and dash approval cards are real infrastructure limits (traced both against the mock server and issue #501), backed by solid property/unit coverage rather than hand-waved.

Checked this against the known auto_applicable iGPU/dGPU drift area from #149/#382/#413/#395 — this PR doesn't touch GPU/device-policy code at all, so no overlap there.

One small thing worth a follow-up fix whenever convenient: docs/architecture.md's new line says rocm-dash-tui's copy is ui::exec::quote_arg, but the function is actually named quote_display_arg.

Nice to have, not blocking: a short comment on shell_quote's inert predicate noting that allowing = bare relies on the quoted value never landing in argv[0] of a sh -c string — true at every current call site, just not structurally enforced.

[rominf]

Rebased onto current main (c1c2aeb3) and dropped the "merge main" commit, so the history is linear again. The head is now dc4a255d.

• Conflicts: three, all resolved:
  • the main.rs import block: main's new driver_install/engines_cmd/serve_cmd imports, plus this PR's shell_quote import;
  • the duplicate [dev-dependencies] in apps/rocm and rocm-dash-tui: resolved as before, so both files are byte-identical to the previous head;
  • the docs/architecture.md paragraph that main converted to links.
• No code moved. The functions this PR changes are still in main.rs, therock.rs, remote/ and ui/exec.rs. Apart from that docs paragraph, the PR's diff is the same as before.
• Nothing new to convert. I checked the code main added since this branch was cut, including the new driver_install.rs, engines_cmd.rs and serve_cmd.rs and the rocmd split. It adds no command preview.

On the review's two notes:

• The quote_arg name: c2522a43 renames it to ui::exec::quote_display_arg in docs/architecture.md and links exec.rs.
• Why inert leaves = bare: dc4a255d adds a comment at both quoter copies, apps/rocm/src/shell_quote.rs and crates/rocm-dash-tui/src/ui/exec.rs. It says = is inert only because no caller renders a value as argv[0], which nothing enforces. It also names one case outside the POSIX contract: zsh's default EQUALS option expands a word that starts with = to a command path. I left quoting a leading = out because it widens this PR; it is a one-line change to both copies if you'd rather close it here.

Re-verified on dc4a255d, all exiting 0:

• fmt
• tests for rocm --bin (1048), rocm-core (596), rocmd (131), rocm-dash-tui (858), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e, check-crate-edges and lychee
• both clippy invocations with -D warnings
• an x86_64-pc-windows-gnu check with -D warnings
• scenarios runtime-19 and therock-next-11

Reverting each guard or quoter one at a time still turns the matching tests red.

This approved e8b41f1. The head is now dc4a255, after a rebase onto main that also dropped the merge commit. c2522a4 and dc4a255 address the review's two notes: the docs name for quote_display_arg, and a comment explaining why '=' stays bare. The current head needs a fresh look.

[rominf]

Rebased onto main 0573d7fb to resolve a conflict with #562, which also edited the comment above proptest in apps/rocm/Cargo.toml. I took main's version of that comment, keeping this PR's shlex.workspace = true dev-dependency. The head is now 55873c99, and every other line of the diff is unchanged. It still merges cleanly with the current main, 34fbdfe9.

Verified locally on the new head, all exiting 0:

• fmt
• tests for rocm --bin (1058), rocm-dash-tui (858), rocmd (131), rocm-core (596), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e
• both clippy invocations
• a Windows-target cargo check -D warnings

[siloteemu]

🔴 Automated review · pr-review-watcher · 55873c9

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...55873c99, 13 commits, 16 files).
The description says the change makes every command preview denote the argv it runs, and it names #443 as fixed. The code does that for the previews. It also adds a control-character guard whose stated completeness does not hold (below).

Blocking

• [new] The control-character guard does not cover everything the description says it covers: three paths still print model-chosen strings raw. apps/rocm/src/main.rs:11160, apps/rocm/src/main.rs:11413, apps/rocm/src/main.rs:17720, apps/rocm/src/main.rs:17664-17666, apps/rocm/src/main.rs:17476
  The description says "The guard applies to every chat tool whose arguments reach the terminal". The doc on CHAT_TOOLS_REACHING_THE_OPERATORS_TERMINAL says every unlisted tool "constrains the one it takes". That holds for accepted values only; the refusal messages echo the rejected value.

  • A hostile tool name is echoed by unsupported ROCm tool {other}``. The guard checks only arguments, and an unlisted name skips it.
  • A hostile port_status host is echoed by non-local host {host}``. port_status is not on the list.
  • A hostile planner intent or tool is echoed by the planner errors. build_freeform_plan_with_context then pushes {error} into deterministic.notes, and the plan block prints notes verbatim. This bypasses both the new argv refusal and the new notes filter.
  • The first two errors reach the terminal through ? at main.rs:10973, then the top-level Error: {e:?} print. The third reaches it through the plan block's note: line. I traced these paths by reading the code and did not run a binary to the terminal.
  • I ran probe tests that call the validators directly. validate_chat_tool_call returned an error string containing U+001B for both a port_status host of x\u{1b}[2Ky and a tool named bad\u{1b}[2Ktool. provider_planner_response_to_plan with an ESC-bearing intent returned an error string containing the ESC.
  • Governing side: the stated guarantee ("the model, not this code, then chooses what the operator reads") governs. The allow-list and the "constrained" comment are what fall short.
    Confidence 90 · logic · Fix: refuse or escape control characters in call.name, in port_status's host, and in the planner intent/tool before they are echoed. Alternatively invert to deny-by-default with an explicit exemption list (natural_language_plan, rocm_command), so a new tool fails safe. If the gaps are meant to stay, narrow the description and the comment to say so.

• [new] The Array and nested-Object arms of reject_control_characters are exercised by no test. apps/rocm/src/main.rs:11260-11275 (and the tests at main.rs:34560-34740)
  The doc says the guard "walks the whole arguments object rather than naming fields, so a new argument … inherits the guarantee". No test sends an array or a nested object to a guarded tool, and no guarded tool reads one. I replaced the Array arm with Ok(()) and ran cargo test -p rocm --bin rocm command_preview_properties: 14 passed, 0 failed.
  Confidence 100 · logic · Fix: add cases such as install_sdk with {"prefix":"/x","extra":["a\u001b"]} and with {"extra":{"k":"a\r"}}, each asserting the guard's own wording. The existing per-tool test already asserts that wording.

• [new] a_windows_install_folder_keeps_its_separators passes with the production change reverted. crates/rocm-dash-tui/src/ui/exec.rs:~311-330
  The value C:\Program Files\ROCm contains a space, so the old quoter also wrapped it, and shlex reads the old double-quoted form back unchanged. I restored the old quote_display_arg body and ran the exec tests: this test passed while 10 of 15 others failed. The test is also the only Windows-side backslash check, because the real-sh property test is unix-only. It stays green if \ is added to the inert set, since the space forces quoting anyway.
  Confidence 100 · logic · Fix: use a space-free backslash path such as C:\ROCm. A bare backslash makes shlex return C:ROCm, so the test would then fail without the quoting.

• [new] an_argument_can_never_add_a_word (three copies) claims a guarantee it cannot fail on. crates/rocm-dash-tui/src/ui/exec.rs:~189-198, apps/rocm/src/shell_quote.rs (an_argument_can_never_add_a_word), apps/rocm/src/main.rs (an_argument_can_never_add_a_word_to_the_preview)
  The comments say a ;, | or > inside a value is "data, never syntax" and "can never read as a second command". shlex splits only on whitespace and quote or escape handling. It treats none of ; | & < > as operators.

  • In exec.rs I added ; | & < > to the inert set. an_argument_can_never_add_a_word stayed green, and so did the shlex round-trip properties. Only display_args_quotes_shell_metacharacters and the unix real-sh test failed.
  • The apps/rocm copies use the same shlex construction, so the same holds there. I did not mutate those separately.
  • Each copy is also implied by its round-trip test.
  • A test whose comment claims a branch it cannot fail on is ranked worse than no test, because it stops the next reviewer from looking.
    Confidence 100 for exec.rs, 85 for the other two · logic · Fix: narrow the comments to "no argument adds or loses a whitespace-delimited word", or drop the redundant tests and rely on the real-sh property tests, noting that those are unix-only.

Non-blocking

• [new] natural_language_plan's "echoed nowhere" claim is inaccurate for the model-called path. apps/rocm/src/main.rs:11197-11199, main.rs:12674-12695, main.rs:34879 (the test and comment)
  When the model calls the tool, request is model-supplied. It is rendered verbatim as request: and into tool_call: lines inside the quotes, and returned in structuredContent.text. I confirmed with a probe test that an ESC in request survives into that text. Which terminal surface prints structuredContent.text is not settled: rocm chat prints only the fixed summary, and the dash plan review reads it (crates/rocm-dash-tui/src/app/summary.rs:26).
  Confidence 70 · logic · Fix: correct the comment, and decide whether the plan text needs the same refusal or a render-boundary scrub.

• [new] has_terminal_control_character is char::is_control only, so it misses format characters. apps/rocm/src/shell_quote.rs:67-69
  U+202A-202E and U+2066-2069 (bidi), zero-width characters and U+2028/2029 pass the guard and land inside '…', where an RLO can reorder the displayed command. That contradicts the "preview denotes the argv" claim. rocm_core::terminal::is_control_or_format already exists (crates/rocm-core/src/terminal.rs:129) and apps/rocm depends on rocm-core.
  Confidence 75 · logic · Fix: use that predicate, and add a bidi case to the tests.

• [new] Planner notes are dropped silently, and on the untrimmed string. apps/rocm/src/main.rs:17608
  A benign note ending in \n, or a multi-line one, is dropped with no trace, and the operator gets no signal that anything was omitted.
  Confidence 70 · logic · Fix: trim before testing, and add a fixed "N planner note(s) omitted" note.

• [new] The justification for keeping a second quoter copy is false as stated. crates/rocm-dash-tui/src/ui/exec.rs:41-48, apps/rocm/src/shell_quote.rs:22-30, docs/architecture.md:28, commit a0b9f7d
  The text says "there is nowhere both can reach". rocm-dash-tui -> rocm-core is the only forbidden edge (xtask/src/crate_edges.rs). apps/rocm already depends on rocm-dash-tui, which exposes a pub quoter, and both crates depend on rocm-dash-core. The result is three byte-identical quoters, two copies of the hostile alphabet and two real-sh harnesses. Nothing ties the copies together, and shell_quote.rs:77-80 says "One copy, deliberately" while a second generator now exists.
  Confidence 80 · architectural · Fix: share one quoter, or reword the three comments to say the duplication is a choice rather than a constraint. Separately, docs/architecture.md calls shell_quote.rs the quoter for "any command line shown to an operator or handed to a shell". Many rocm services stop {} style hints remain unquoted and the doc should say which surfaces are covered.

• [new] Generator and tests leave gaps in which characters are exercised. apps/rocm/src/shell_quote.rs:104-107, exec.rs:153-156
  The alphabet has no [ ] { } % , @ +. Making [ and ] inert would stay green everywhere except the single package[extra] pin in therock.rs.

  • shell_quote.rs:102 says the generator has "No control characters", but it contains \t. a_validated_planner_argv_cannot_forge_a_plan_line goes red under revert only because of that accidental tab.
  • The approval property test returns early when validation refuses, so it can pass vacuously.
  • a_copyable_command_survives_being_pasted pins one spelling rather than paste behaviour, and stays green if $ becomes inert, because its value contains spaces.
    Confidence 70 · mechanical · Fix: add those characters to the alphabet and a fixed package[extra] example to the dash tests, and correct the comment.

• [new] Both new scenarios would pass on the old quoters on a Windows lane whose temp root contains whitespace. tests/e2e-cucumber/tests/e2e/runtime_steps.rs:1395-1405, tests/e2e-cucumber/tests/e2e/therock_steps.rs:~718-760
  The old quoters also wrapped on whitespace, and the lone ' does not discriminate once the whole path already has a space. Linux and spaceless roots do discriminate. The therock-next-11 step also matches any word whose file name equals it's-here, which is looser than the symlink reason requires.
  Confidence 55 · mechanical · Fix: assert the root has no whitespace, or compare the full path or its canonicalize().

• [new] Coverage gaps under AGENTS.md section 3, beyond what the description already declares. The description declares the chat approval card and the dash approval cards. It does not declare these, which have no scenario:

  • the new refusal of a model tool call carrying a control character, which is observable as an error;
  • dropped planner notes;
  • the rocm serve '<model>' placeholder text.
    I found no scenario asserting any of the three. State why each is unreachable offline, or add a scenario where one can run. The dash preview is separately uncovered by any scenario (dash-12 asserts only the title and the buttons).
    Confidence 75 · architectural.

• [new] The history-narrating comments in remote/mod.rs should go. apps/rocm/src/remote/mod.rs:1238-1244 and :1427-1430 record where shell_quote "used to" live. That is a changelog in the code. Confidence 70 · mechanical.

• [new] Commit hygiene. Confidence 85 · mechanical.

  • a0b9f7d has a truncated paragraph. The dash-TUI sentence starts mid-sentence ("fixes the whitespace case from #443 …"), which looks like a subject line eaten by comment stripping.
  • 829aba5 and 5cd2c1e are rustfmt-only commits, so earlier commits are not fmt-clean and not bisectable. 6ae6989, 093049d, 55873c9 and f7d5744 fix earlier commits of the same PR.
  • a0b9f7d is about 1,500 lines and bundles two logical changes: the quoting fix, and a new control-character refusal across nine tools, rocm_command, planner argv and planner notes. AGENTS.md section 11 asks for one logical change per PR. A split would also give the guard its own description.

• [new] quoter_properties reach test. shell_quote.rs:204-213 and the exec.rs twin create a rocm-…-glob-bait-<pid> directory in the temp dir and never remove it. One directory leaks per test process. Confidence 40, so it is below the reporting bar and listed for information only.

Decisions for the author

• POSIX single quotes on every platform, including Windows copy surfaces — tradeoff
  Both sides: for a reader the preview is now unambiguous, and the old double-quoted form was ambiguous everywhere. The description and shell_quote.rs:34-42 already disclose that cmd.exe does not strip single quotes and that PowerShell fails on a value containing '. The documented Windows shell is PowerShell (README.md:96, docs/manual-testing.md:10), so only the lone-' case and cmd.exe pastes break.
  Two further points: the dry-run command: uv … && uv … line already needed PowerShell 7 or cmd.exe for &&, and the Windows e2e lanes read the line back with POSIX shlex, so scenarios pass on a line a PowerShell user could not paste for the lone-' folder.
  rocm_core::runtime_is_windows() and comfyui.rs:680's powershell_quote exist as precedents for a platform-aware renderer. Confirm POSIX-only is the intended scope.

• Refuse at validation versus scrub at the render boundary — tradeoff
  Refusing is right for values that become argv, because the approval preview must equal what runs. It does not cover read-only tool output, which main.rs:10990-10993 prints raw line by line. That includes rocm_command output such as service logs. It also does not cover model prose (main.rs:9818-9820). Both predate this PR. The description's "the model chooses what the operator reads" framing therefore describes only the argument surface. Decide whether a single render-boundary scrub, using rocm_core::terminal, is the follow-up.

Positive signals

• Moving the remote quoter into one module and asking a real /bin/sh to read the rendered line back is the right oracle. The shlex-only version is shown, in the PR's own comment, to pass against the defect.
• generator_reaches_the_shapes_that_break_quoting measures the generator's reach and fails below a floor, so the alphabet cannot be tuned into harmlessness unnoticed.
• every_guarded_tool_refuses_a_control_character_in_its_rendered_argument asserts the guard's own wording rather than is_err(). Several tools would refuse the value for another reason, so a looser assertion would stay green with the guard deleted.
• The control-character cases pick a folder named with a lone ' rather than a space. They discriminate old from new on Linux, which a space would not.
• The Windows lanes and the real shell are both exercised by name through TEST_SH = "/bin/sh", avoiding a PATH race with other tests in the binary.

Deployment notes

• Copyable and printed command lines now use POSIX single quotes. Windows paths render as 'D:\ROCm\…', where they rendered bare before. The cmd.exe and PowerShell limits are described under Decisions.
• Model tool calls to guarded tools, rocm_command, and planner argv that carry any Unicode control character are now refused with an error. Previously only NUL, LF and CR were refused on rocm_command, and only NUL on planner argv. The error text is new and undocumented in docs/llm-tool-use.md (that doc was not made false, per AGENTS.md section 7).
• shlex becomes a dev-dependency in three crates. shlex 2.0.1 was already in Cargo.lock, so no new package enters the lockfile.

What this covered

Every changed file was read completely in its changed regions, plus the surrounding code. That includes apps/rocm/src/shell_quote.rs, remote/mod.rs, therock.rs, the main.rs validators, renderers and handlers, crates/rocm-dash-tui/src/ui/exec.rs and its callers, the e2e features and steps, the Cargo manifests, and docs/architecture.md. Read against prw-base (7a062ad7) to 55873c99. Six read-only Sonnet workers each covered one coupling group. A seventh question group covered the design questions: Windows rendering, completeness of the one-quoter claim, and guard placement. Findings were consolidated and verified by the driver.

Runs by the driver (one build at a time, one shared target dir, since removed):

• cargo test -p rocm --bin rocm command_preview_properties with the Array arm of reject_control_characters stubbed to Ok(()): 14 passed.
• cargo test -p rocm-dash-tui --lib exec:: with the old double-quote quote_display_arg restored: the Windows-folder test passed and 10 of 15 others failed.
• cargo test -p rocm-dash-tui --lib exec:: with ; | & < > added to the inert set: an_argument_can_never_add_a_word and the shlex properties passed; display_args_quotes_shell_metacharacters and the real-sh property failed.
• Probe tests, since reverted: port_status and unknown-tool refusal strings, a planner intent error, and natural_language_plan echo each carried the ESC byte.
• I did not run the full workspace test suite, clippy or the e2e lanes locally.

CI at head 55873c99 (live statusCheckRollup): every check that reported passed. That covers clippy, build-and-test, windows-build-and-test, Test (affected crates), Commit signatures + sign-off, E2E tests, the Strix Halo Ubuntu, Windows and WSL2 lanes, prek, Coverage (rocm-dash crates, ratcheted) and CodeQL. Regenerate manifests, Skill checks and Sphinx docs build were skipped. E2E tests (MI300X) was still queued, so it had not reported.
No check runs rustdoc, so cargo doc was not run. Possible rustdoc ambiguity on the [`shell_quote`] link at main.rs:17918 (both a module and a function) is unsettled, confidence about 60, so it is not scored.

The description and #443 were read at Step 9. The description matches the diff except for the guard-completeness claim and the "pinned per entry" claim under Blocking. #443 is OPEN, and the change is about it. The discussion on the PR was not read, and this report is not reconciled against it. Commit signature status was not checked locally, because %G? does not reflect the API's verified state here.

Did not run: the prior-changes pass (it needs earlier review comments, which this run may not read). History found nothing that the change undoes.

[rominf]

Rebased onto main 7a062ad7; the head is now 7850393c.

• Architecture doc: docs(architecture): reflow module-map paragraphs into per-module lists #624 reformatted docs/architecture.md into per-module lists, so the shell_quote.rs entry is now its own list item.
• Everything else: the code diff is unchanged, and it now sits on top of test(core): let e2e builds re-root the hardware probes #510.

Verified locally on the new head, all exiting 0:

• fmt
• tests for rocm --bin (1059), rocm-dash-tui (858), rocmd (131), rocm-core (603), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e
• both clippy invocations, on Linux and for x86_64-pc-windows-gnu

[rominf]

Rebased onto main 2844d797; the head is now a1872ef0. #625 moved the managed-service launch code into service_launch.rs, but none of this PR's changes moved with it. It still merges cleanly with current main.

Since this branch was cut, #604 added the shared rocm services <action> <id> --yes hint builders, and #424 added rocm services logs <id> and rocm runtimes activate <key> --restart-services --yes lines to the activation report. All of them printed the id or key without quoting. They now go through shell_quote:

• inside the hint builders, once. The remote stop hint quotes for its callers, so the line sent to the remote shell and the line shown match.
• at the two report lines, and in the service-remove message.

Ordinary ids print unchanged. New tests split each hint and both remote stop commands back to their argv. Removing the quoting from the builder fails a_hint_denotes_the_argv_it_names.

Older hints that predate this branch still print values bare:

• the logs: lines in service_launch.rs
• rocm logs --service in serve_summary.rs
• next step: rocm runtimes activate
• rocm remote doctor {target}
• rocm config enable-provider {provider}

I'll take those in a follow-up.

Verified locally:

• fmt
• tests for rocm --bin (1122), rocm-core (619), rocmd (131), rocm-dash-tui (859), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e and check-crate-edges
• both clippy invocations, on Linux and for x86_64-pc-windows-gnu

[rominf]

Thanks. I verified every finding before fixing it. They are addressed in dc25c5b4 and 855392fa, and the branch is rebased onto current main.

Blocking

1. Refusals echoing the refused value: confirmed all three. The guard is now deny-by-default:
   • Every tool's whole arguments object is screened first, with one exemption: rocm_command, which screens each argv entry itself after trimming. The opt-in list missed port_status's echoed host, and an exemption list fails safe when someone forgets it.
   • The tool name is screened before anything names it, and that refusal does not repeat it.
   • The planner's tool and intent refusals no longer echo a hostile value. Every planner failure now becomes a note through planner_failure_note, which strips control sequences.
   • Tests assert the guard's wording, and that the refusal carries no control character, for: an unknown tool, a tool name, the port_status host, the planner tool and intent, the failure note, and every accepted tool. Restoring the old allow-list, exempting port_status, or removing any single check turns a test red.
2. Array/nested-Object arms: added your two install_sdk cases, plus an array holding an object holding a bidi character. Changing the Array arm to Ok(()), or not recursing into objects, turns the test red.
3. The Windows folder test: it now uses C:\ROCm, and goes red with the old quoter restored.
4. an_argument_can_never_add_a_word: dropped all three copies. The round-trip test already implies one word per argument. Its doc now says that only the Unix real-sh property checks that ;, | and > stay data. Making those characters inert fails only that property and display_args_quotes_shell_metacharacters.

Non-blocking

• natural_language_plan: the comment was wrong. request is now screened like any other argument. The user's own /plan input comes from a single-line field, so this costs them nothing.
• The predicate: now rocm_core::terminal::is_control_or_format plus U+2028/2029, with bidi, zero-width and line-separator cases added. The dash needs no mirror, because its tool calls go through validate_chat_tool_call.
• Planner notes: trimmed before the check, and dropped notes are reported as "N planner note(s) omitted".
• The second quoter: shared rather than reworded. shell_quote delegates to rocm_dash_tui::ui::exec::quote_display_arg; apps/rocm already depends on that crate, so this adds no new edge. The quoter tests live once, in the dash crate. docs/architecture.md now names the covered surfaces and says that older hints are not covered.
• The generator: it gains [ ] { } % , @ + and a fixed package[extra] test, the \t comment is fixed, and the plan-line forge property now draws arguments that contain control characters on purpose.
• Smaller fixes: the history comments in remote/mod.rs are removed, and docs/llm-tool-use.md now states the refusal rule.

Not in this round:

• The older unquoted hints (rocm logs <id>, runtimes activate, remote doctor, enable-provider): about 15 sites, each needing its own coverage, so they go in a follow-up.
• Scenarios: model tool calls and planner notes can't be reached offline (the mock chat endpoint returns fixed text with no tool_calls). The rocm serve '<model>' placeholder is reachable offline but not yet covered; that is a follow-up.

Decisions

• POSIX-only quoting on Windows is the intended scope.
• A render-boundary scrub of read-only tool output and model prose is a separate follow-up.

Verified locally:

• fmt
• tests for rocm --bin (1120), rocm-dash-tui (860), rocm-core --features e2e-test-hooks (622), rocmd (131), xtask (252) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e and check-crate-edges
• both clippy invocations, on Linux and for x86_64-pc-windows-gnu

The one local failure was the known spinner timing flake, which passed on re-run.

This reviewed 55873c9. The head is now 855392f: dc25c5b shares one shell quoter, and 855392f makes the chat-tool guard deny-by-default (no refusal echoes the refused value), tests the nested Array/Object arms, fixes the Windows separator test and drops the tests that could not fail. All required checks are green on 855392f.

[rominf]

Rebased onto main 4629d531; the head is now be9a10bd.

• Scenario index: ROCMAI-542: detect gfx90c (Renoir/Cezanne Vega iGPUs) and stop mapping it to gfx90X-dcgpu #648 took runtime-19 in runtime_setup.feature, so this PR's quoted-folder scenario is now runtime-20. Its @id:runtime-freeform-prefix-with-quote-is-one-argument and its step text are unchanged.
• Diff survival: apart from that renumbering, the PR's added and removed lines are identical to the previous head 855392fa.
• New commit be9a10bd: fix(comfyui): reinstall replaces only ComfyUI's code, never the user's files #546 added ComfyUI advice in two places that tells the user to run rocm comfyui install --runtime-id <key>, with the key unquoted. Both now go through one builder that uses the shared quoter. A property test draws keys from the shared hostile generator and checks that both messages split back to exactly that argv; it fails if either message prints the key bare. Generated runtime keys are plain, so today's output is unchanged.
• Other hints added on main meanwhile: the vLLM --yes hints, the os-release install hints and the gfx90c message interpolate no value, so they need nothing.

Local gates, all green:

• fmt;
• tests: rocm bin (1191), rocm-core lib (661), rocm-dash-tui, xtask (252), e2e-cucumber lib (143) and feature_naming;
• cargo check --test e2e;
• clippy -D warnings for the workspace and the e2e target, on Linux and on the Windows target.

[rominf]

Rebased onto 03cdbccd (#665); the head is now 2a228d99.

• Moved check: ROCMAI-88: extract chat/MCP-bridge types and leaf helpers (1/3) #665 moved normalized_chat_rocm_command_args into chat.rs, so this PR's control-character check now applies there. It goes through crate::shell_quote::has_terminal_control_character, with no call back into main.rs.
• Test fix: the rebase reported no conflict there, but one test still called mcp_tool_result_text at its old place and would not compile. It now calls chat::mcp_tool_result_text. Every commit compiles on its own.
• Unchanged otherwise: the PR's added and removed lines differ only in those two lines.
• Check still bites: reverting the moved check to the old NUL/LF/CR test fails no_control_character_from_a_model_supplied_argv_reaches_the_preview.
• Other new hints: none of the helpers ROCMAI-88: extract chat/MCP-bridge types and leaf helpers (1/3) #665 moved, and nothing else main added since, builds a command preview from interpolated values.

Local checks all pass:

• fmt;
• tests for rocm, rocm-core, rocm-dash-tui, xtask and e2e-cucumber;
• the runtime-20 and therock-next-11 scenarios;
• clippy on Linux and the Windows target, both invocations.

[rominf]

Rebased onto a32d6320 (#587); the head is now 263f8b73. The only conflict was crates/rocm-dash-tui/Cargo.toml's dev-dependencies: #587 added the rig-core test-utils entry and this PR adds shlex, and both are kept. The PR's added and removed lines are identical before and after, and #587's agent-client files do not overlap this PR. Re-ran fmt, the rocm-dash-tui tests (848 + 20 + 5 + 7) and rocm tests (1257), and workspace clippy -D warnings on Linux and the Windows target: all pass.

[rominf]

Rebased onto main d4e7c751; the head is now c2764cbe.

• ROCMAI-88: extract chat/MCP-bridge function bodies (2/3) #667 moves: ROCMAI-88: extract chat/MCP-bridge function bodies (2/3) #667 moved the chat cluster's function bodies into chat.rs. This PR's hunks for validate_chat_tool_call, normalized_chat_rocm_command_args and the related functions now apply there. The command_preview_properties test module moved to chat.rs too, next to the cluster's other regression tests, rather than widening two now-private items to pub(crate). Apart from that move, the diff is unchanged except for a few imports.
• ASCII-output contract (fix(output): replace non-ASCII punctuation with ASCII in CLI output text #683): it flagged the U+2028/U+2029 char escapes in has_terminal_control_character. They duplicated rocm_core::terminal::is_control_or_line_separator, and the function now calls that instead. Behaviour is unchanged.
• New commit, quit-confirmation popup (feat(dash): confirm before quitting while a model is still serving #595): feat(dash): confirm before quitting while a model is still serving #595 added two rocm services stop {id} --yes lines to the dashboard's quit-confirmation popup, one in the registry fallback branch and one in the known-instance branch. Both now quote the id with quote_display_arg. quit_confirm_stop_lines_denote_the_argv_they_name_for_a_hostile_id requires each line to split back to the exact argv for a set of hostile ids, and it fails if either line goes back to a bare {id}. An ordinary id prints unchanged, and dash-26, dash-27 and dash-28 still pass.

Merged with current main: cargo test -p xtask (313), the rocm tests (1273) and the dash quit-confirm tests pass. On the head: fmt, rocm-dash-tui, e2e-cucumber lib and feature_naming, and clippy on Linux and the Windows target pass.

[rominf]

Rebased onto main 3067a103 (#630, #712); the head is now a27e68f3. #630 added scenarios therock-next-11 to 14, so this PR's quoted-folder dry-run scenario is now therock-next-15 (@id:therock-next-15-dry-run-command-keeps-a-quoted-folder-whole). Its step comment is updated to match. Nothing else changed: the PR's added and removed lines differ only in those three renumbered lines. Passing locally: fmt, feature_naming, cargo check --test e2e, cargo test -p xtask (313), and therock-next-11 to 15 (5 of 5).

[rominf]

Rebased onto main 1c33248c (#535, #566, #646, #647, #719); the head is now c5c148b5.

• One conflict, in docs/architecture.md: test: check that every advised rocm command parses; fix dashboard labels that don't #535 had changed the service_hints.rs line to stop|restart|remove. Both edits are kept.
• Two semantic conflicts with test: check that every advised rocm command parses; fix dashboard labels that don't #535's new advised-command checker, fixed in c5c148b5:
  • The checker lists the service hint's format string as not-an-invocation. Because this PR now quotes the id, that string reads rocm services {action} {} --yes, and the exclusion entry now matches it. The real hint is still checked by every_service_action_hint_verb_parses and service_action_retry_advice_parses_for_generated_ids.
  • The shell_quote.rs architecture note spelled the free-form request as rocm <request>, which the checker reads as a command. It now says "the plan for a free-form request".
• Unchanged otherwise: apart from the docs line and those two fixes, the PR's added and removed lines are unchanged.

Verified locally:

• fmt;
• tests: rocm (all advised_commands tests pass), rocm-dash-tui, xtask (317), and feature_naming;
• cargo check --test e2e;
• clippy, both invocations, and Windows clippy.

[nowycondro]

Automated review (medium/high-effort recall pass): 7 findings across correctness (two spots where this PR's own "quote the hint" fix wasn't applied to sibling occurrences of the identical string in the same file), architecture (four parallel hand-maintained control-character checks instead of one general gate; a silent-strip path inconsistent with the rest of the PR's refuse-don't-mangle design), and minor efficiency/simplification cleanups. See inline comments.

[nowycondro]

Four independent, hand-written "no control characters reach a terminal" checks instead of one general gate.

This call (reject_control_characters_in_tool_arguments) is one of four separately-maintained enforcement points this PR adds/widens for the same invariant:

1. this walk, for most chat tools;
2. normalized_chat_rocm_command_args's own inline has_terminal_control_character(arg) check for rocm_command's argv;
3. validate_provider_planner_tool_call's own .any(...) check for the planner's argv;
4. provider_planner_response_to_plan's inline loop for planner notes (which strips and prints via planner_failure_note/strip_terminal_control_sequences rather than refusing, unlike the other three).

None of the four is reachable by construction from a new call site — a future tool, a new planner field, or a new printed-preview path can be added without hitting any of them, which is exactly the "a list of exceptions fails safe when forgotten" risk this PR's own doc comment calls out for mechanism #1 specifically (CHAT_TOOLS_SCREENING_THEIR_OWN_ARGUMENTS is deny-by-default; the other three are not). A single choke point — e.g. sanitizing/refusing model- and provider-supplied strings once at deserialization, or routing all four through one shared validator — would not depend on every future call site remembering to opt in.

Related: planner_failure_note (main.rs) silently strips rather than refuses, which is also an inconsistency with the rest of this PR's "refuse, don't mangle" design — a hostile provider error (e.g. a forged "approval: not required" line) is silently cleaned and printed with no indication anything was altered.

[nowycondro]

Minor simplification: this quote_display_arg is now a pure 1-line wrapper around crate::shell_quote::shell_quote(value), with only two call sites (both in the same function, both inside .map(|arg| quote_display_arg(arg))). Consider deleting the wrapper and calling crate::shell_quote::shell_quote directly at both call sites — one less indirection for a reader to chase.

[nowycondro]

planner_failure_note strips control characters (rocm_core::terminal::strip_terminal_control_sequences) and silently prints the mangled result, unlike every other path this PR adds, which refuses on a control character (or, for planner notes a few lines below in provider_planner_response_to_plan, drops-and-counts rather than silently mangling in place). A hostile provider error containing a forged line (e.g. a fake "approval: not required") gets silently cleaned and printed with no indication it was altered — reintroducing the "operator can't tell original content from altered content" ambiguity that refusing (the pattern used everywhere else in this PR) is meant to eliminate.

[nowycondro]

Efficiency note: this recursive control-character walk now runs twice on the same unchanged call in at least two places that were already calling validate_chat_tool_call back-to-back before this PR, but which this PR makes each call more expensive (a full recursive JSON walk instead of the old cheap per-arg \0/\n/\r check):

• apps/rocm/src/dash_seam.rs's BinToolExecutor::execute calls chat::validate_chat_tool_call(&call) explicitly (line 37), then for a non-read-only tool calls chat::chat_tool_approval_request(&call, None) (line 46), whose first line re-runs validate_chat_tool_call on the same call.
• deterministic_mutating_tool_call_for_prompt validates a call, and the caller that wants its title (deterministic_mutating_tool_intro) immediately calls chat_tool_approval_request, which re-validates and rebuilds args/display_command via shell_quote, only to read approval.pending_title — the rest of the built request is discarded immediately, and the same call is validated/rendered again later for the real approval card.

Not a new bug, but this PR increases the cost of each of these now-redundant passes. Worth a cheap guard (e.g., a validated: bool the caller can skip, or reading the title without building the full approval request) if this path is latency-sensitive.

[nowycondro]

This PR fixes the exact wording `rocm services logs {service_id}` here (wrapping it in shell_quote), but two sibling occurrences of the identical string elsewhere in this same file are left unquoted:

• write_past_attempts_hint: writeln!(output, " Read the newest: rocm services logs {id}") (~main.rs:6610)
• render_services_text: writeln!(output, " logs: rocm services logs {}", record.service_id) (~main.rs:6671)

ManagedServiceRecord.service_id/newest_id are plain Strings with no inert-character guarantee once persisted, so an id with a space/quote/metacharacter makes these two lines split into multiple words on paste — exactly the defect class this PR closes everywhere else, missed two call sites away from where the identical string was just fixed.

Separately (lower confidence, possibly already covered by this PR's stated follow-up list): "Next step: rocm runtimes activate {}", manifest.runtime_key (~main.rs:2148) and " next step: rocm runtimes activate {}", installed.runtime_key (~main.rs:7710) have the same unquoted shape as the rocm runtimes activate <key> --restart-services --yes hint this PR did quote in runtime_services.rs. The PR description's "not covered by a scenario" section lists "runtimes activate" among older hints deferred as follow-up, so this may be intentional — flagging in case it wasn't meant to include these two.

Minor, unrelated: has_terminal_control_character (apps/rocm/src/shell_quote.rs) ORs rocm_core::terminal::is_control_or_format(c) and is_control_or_line_separator(c), both of which independently call c.is_control() first — for the common case (an ordinary character), this re-evaluates is_control() twice per character on what is now a hot path (every chat-tool argument string, walked recursively). Could be a single combined predicate in rocm_core::terminal doing one is_control() check.

[nowycondro]

🤖 Automated review (Claude Code) · c5c148b

Summary review pass at head c5c148b5 (medium effort). Findings below were posted as 5 inline comments plus a review summary just above this comment; restating here with the required format/disclosure.

Findings (most severe first):

1. Non-blocking, high confidence — apps/rocm/src/main.rs:4207: this PR fixes the unquoted rocm services logs {service_id} hint here, but two sibling occurrences of the identical string elsewhere in the same file (write_past_attempts_hint ~line 6610, render_services_text ~line 6671) are left bare — the exact defect class this PR exists to close, missed two call sites away from the fix it makes. Lower-confidence companions: two unquoted rocm runtimes activate {runtime_key} hints at ~lines 2148 and 7710 (possibly already tracked in the author's disclosed follow-up list of ~15 older unquoted hints — see their 2026-10-13 comment).
2. Non-blocking (architectural), medium confidence — apps/rocm/src/chat.rs:2117: four independent, hand-written "no control characters reach a terminal" checks (chat-tool args walk, rocm_command's own check, the planner-argv check, and the planner-notes loop) instead of one general, deny-by-default mechanism — only one of the four is actually deny-by-default.
3. Non-blocking, medium confidence — apps/rocm/src/main.rs:9058: planner_failure_note silently strips control characters and prints the mangled result, which is inconsistent with the rest of the PR's "refuse, don't mangle" design used elsewhere.
4. Non-blocking (efficiency), low-medium confidence — apps/rocm/src/chat.rs:2207: the now-heavier recursive control-character walk runs twice per call in dash_seam.rs::BinToolExecutor::execute and in the deterministic-planner title lookup — pre-existing double-validation made costlier by this PR, not a correctness bug.
5. Non-blocking (simplification), high confidence — apps/rocm/src/therock.rs:2408: quote_display_arg is now a pointless 1-line wrapper around shell_quote with only 2 call sites.

No blocking correctness regressions found at this head. Cross-file caller tracing for remote_service_stop_hint/service_action_hint/format_structured_tool_call found no double-quoting or incompatible-caller bugs, and the removed-behavior audit found every deleted check/test re-covered by an equal-or-stronger replacement (including strip_terminal_control_sequences already covering the bidi/zero-width/line-separator set raised in an earlier review round).

What I read / could not verify: Reviewed the diff at head c5c148b5 against its merge base via a local clone (git fetch origin pull/563/head), focused on files touched since the last visible review round (per the extensive rebase-log comment trail already on this PR) rather than re-litigating the full multi-month history. No Rust toolchain was available in this environment, so none of the above was confirmed by compiling or running cargo test/clippy — this is a static reading of the diff only. CI status at this head was not independently re-verified beyond what's visible on the PR checks tab.

🤖 Generated with Claude Code

[nowycondro]

🔴 Automated review · pr-review-watcher · c5c148b

Review — major concerns

Full review of the whole change: the three-dot diff between origin/main at d59f596f6e08297c207f008021eecb37defc3558 and the PR head c5c148b5801f3e5b53d15e5e0d86475b20839fd8 (23 files, +1961/-139), covering all changed files completely.
Implements ROCMAI-530 ("Command previews shown for approval do not denote the argv actually run," status In Progress) — the PR's described scope matches the ticket, and also names GitHub issue #443 ("Fixes #443") and builds on merged PR #500, consistent with what the diff does.

Blocking

• The PR currently has unresolved git conflicts with main and is forked from a stale point that predates a landed, overlapping refactor — repo/branch state (not a single line), corroborating evidence at apps/rocm/src/shell_quote.rs:63-64, crates/rocm-core/src/quoting.rs:92-100.
  Verified directly: gh pr view 563 --repo ROCm/rocm-cli --json mergeable,mergeStateStatus returns {"mergeable":"CONFLICTING","mergeStateStatus":"DIRTY"}. The PR's merge-base with origin/main is 1c33248c (2026-10-23), five days stale; origin/main since gained commit fe1dbabe ("ROCMAI-727: consolidate shell/PowerShell quoting into rocm-core (#743)"), which (a) extracted large portions of apps/rocm/src/main.rs into apps/rocm/src/freeform_planner.rs and sibling modules — confirmed freeform_planner.rs exists on origin/main but git show pr/563:apps/rocm/src/freeform_planner.rs fails ("exists on disk, but not in 'pr/563'"), while this PR edits the same planner functions in place inside main.rs — and (b) added rocm_core::posix_shell_quote, whose body (crates/rocm-core/src/quoting.rs:92-100) is now byte-identical to rocm_dash_tui::ui::exec::quote_display_arg, the function this PR's own new apps/rocm/src/shell_quote.rs:63-64 delegates to. rocm_core::quoting's own module doc explicitly says it deliberately leaves quote_display_arg unmerged because it "uses a materially different, currently-inconsistent escaping convention" — a premise this PR makes false without ever seeing that doc, because the two branches never met.
  Confidence 100 · architectural · Fix: rebase onto current main, re-home the edited planner logic into freeform_planner.rs (and any other extracted module this PR still edits in main.rs), and decide whether apps/rocm/src/shell_quote.rs should delegate to the now-identical rocm_core::posix_shell_quote instead of (or in addition to) rocm-dash-tui's copy.

• CI check "E2E tests (MI300X, current)" is failing on the reviewed head commit — gh pr checks 563 --repo ROCm/rocm-cli (re-verified immediately before this report).
  The job log could not be inspected (gh run view --log-failed → 403 Forbidden, self-hosted-runner logs not accessible to this account), so whether this is a regression from this change or an unrelated/infra flake on the self-hosted MI300X lane is not established either way — stated as a gap rather than assumed safe.
  Confidence 100 that the check is red · architectural · Fix: investigate the job, and if it is a known flake, re-run it or note that explicitly on the PR; a red check should not be left unexplained.

Non-blocking

• comfyui.rs's run_uv_logged_command still writes its "command: {uv} {args}" log line through a pre-existing, un-migrated quoter — apps/rocm/src/comfyui.rs:2298-2307 (quote_log_arg, unchanged by this diff — confirmed via git diff ...pr/563 -- apps/rocm/src/comfyui.rs | grep quote_log_arg, no hits) and its call site at apps/rocm/src/comfyui.rs:2065-2073.
  quote_log_arg falls back to Rust's {value:?} Debug formatting for anything non-trivial: Debug doubles backslashes, so a Windows install path such as D:\ROCm\therock_venvs (quoted correctly elsewhere in this very PR) would render here as "D:\\ROCm\\therock_venvs", a different, nonexistent path if pasted; Debug also never neutralizes $/backtick, so a value containing $(...) stays live inside the double quotes it produces. This is the identical bug class therock.rs's near-identical dry-run "command: uv … && uv …" line explicitly fixes in this same PR.
  Pre-existing and untouched by the diff, so not blocking by itself; non-blocking here because it's a diagnostic log line rather than an interactive approval-gate preview, and its exposure is unchanged by this PR. Flagged because of its direct pattern relevance to this PR's own stated purpose.
  Confidence 85 · logic · Fix: migrate quote_log_arg to the shared shell_quote in a follow-up.

• remote/mod.rs's copy-pasteable ssh {target} -- … hint lines still interpolate target bare, inconsistent with the rest of the same file — apps/rocm/src/remote/mod.rs:318, 422, 455, 600, 1120, 1144, 1229, 1237 (all pre-existing, untouched by this diff's 55/58-line change to this same file).
  target is documented elsewhere in this file as stored "exactly as the user typed it" (unlike session_id, which this PR did migrate to shell_quote in these very call sites). Practical exploitability is tempered because target must resolve to a live Tailscale peer name first, which in practice constrains it to DNS-safe characters — but the inconsistency sits inside a file this PR reworked for exactly this purpose, with no comment explaining why target was left out.
  Confidence 70 · logic · Fix: either quote target too, or add a one-line comment (matching the style already used elsewhere in this PR for other intentionally-bare values) explaining why it's safe to leave bare.

Decisions for the author

• Delegate shell_quote to rocm_core::posix_shell_quote instead of rocm-dash-tui's copy, once rebased — tradeoff.
  apps/rocm already depends on both rocm-core and rocm-dash-tui (apps/rocm/Cargo.toml lists both). Today's choice to delegate to rocm-dash-tui is justified by the crate-edge rule (rocm-dash-tui may not depend on rocm-core) and is well-documented in shell_quote.rs's module doc, but that reasoning predates rocm_core::posix_shell_quote existing at all. Once rebased, both delegation targets are equally available and implement the identical algorithm; keeping rocm-dash-tui's copy as the canonical one is defensible (it already carries the round-trip/real-sh tests), but routing through rocm_core instead would leave only two independently-tested copies (the unavoidable rocm-dash-tui one, plus whichever crate(s) cannot reach it) instead of the current three-way split. Neither option is wrong; worth a deliberate call once the conflict is resolved.

• A hand-maintained exemption list for self-screening tools — non-blocking-improvement.
  CHAT_TOOLS_SCREENING_THEIR_OWN_ARGUMENTS in apps/rocm/src/chat.rs has exactly one entry (rocm_command) and is correctly pinned by a test today, so a tool added here without equivalent self-screening would currently be caught. A compile-time or lint-level check tied to the tool registry itself would be more robust than a runtime test as this list grows, given how security-relevant it is.

• Automation-proposal title/message are echoed into printed tool-result text without going through the control-character guard — apps/rocm/src/chat.rs:3574, 3607, unverified/open.
  proposal_action's own argument screening covers proposal_id/action only; whether a proposal's stored title/message can ever be authored under model influence (vs. always system/user text) could not be traced from the files in this diff — proposal creation lives outside it. If it can be model-influenced, this is the same class of gap the guard elsewhere closes; if not, it's worth a one-line note recording that the scope decision was deliberate.

• Control-character guard uses a blocklist (reject any Unicode Cc/format codepoint, including a bare \t) rather than an allowlist — tradeoff.
  Safe-by-default and consistent with the guard's stated purpose, but could someday reject a legitimate value (e.g. a path containing a literal tab). Worth confirming this was weighed deliberately rather than merely convenient.

Positive signals

• The round-trip property tests go beyond word-splitter checks: they hand the rendered line to a real /bin/sh in a directory seeded with glob bait and a sentinel HOME, and one test case is a literal "x'; rm -rf ~; echo '" injection payload — this proves inertness under actual shell semantics, not just under a parser that performs no expansion.
• validate_chat_tool_call's control-character screening runs before the "is this tool even supported" check, so an unrecognized or future tool is screened by default rather than silently admitted — genuine fail-closed design, not merely a convention that happens to hold today.
• The two new e2e scenarios deliberately use a folder name containing a literal ' rather than a plain space, with an in-feature-file comment explaining that a space alone would already have passed under the pre-PR quoter — well-reasoned choice of adversarial test data rather than an accidental one.

Deployment notes

• This PR cannot currently be merged as-is; see Blocking above (git conflicts with main).
• Intentional, user-visible format change: previews now render '...' (POSIX single quote) instead of the old "...". This reads back correctly on bash/WSL/PowerShell (modulo a value containing ' on PowerShell) but is not paste-runnable as-is in cmd.exe, which does not strip single quotes — called out by the PR itself, repeated here for whoever writes release notes or support docs.

What this covered

Every file in the diff, read completely, via five coupling-grouped workers (shell_quote.rs + Cargo.toml/Cargo.lock + advised_commands.rs + therock.rs; chat.rs + docs/llm-tool-use.md; main.rs + remote/mod.rs + runtime_services.rs + service_hints.rs + comfyui.rs; rocm-dash-tui's ui/exec.rs + ui/mod.rs + wave0_job_bridge.rs + Cargo.toml; the e2e-cucumber feature/step files + Cargo.toml + docs/architecture.md), plus two source-reading passes (agent-instruction adherence against the repo's AGENTS.md; combined history + prior-changes-against-merged-PR-#500 + code-comment compliance). d59f596f6e08297c207f008021eecb37defc3558…c5c148b5801f3e5b53d15e5e0d86475b20839fd8.

One worker could not read apps/rocm/src/main.rs (~21,400 lines) byte-for-byte in a single pass; it read the full diff plus an exhaustive pattern-sweep of every command/preview-construction site with full surrounding context, and disclosed this as thorough-but-targeted rather than exhaustive-line-by-line for the file's untouched remainder.

I independently re-verified the highest-impact candidates against the source rather than taking worker reports at face value: the PR's live mergeable/mergeStateStatus, the live CI check list, the byte-identical bodies of rocm_core::posix_shell_quote and rocm_dash_tui::ui::exec::quote_display_arg, the absence of freeform_planner.rs from the PR's tree, and that both comfyui.rs's quote_log_arg and remote/mod.rs's bare target interpolations are untouched by this diff.

Not reconciled against any discussion on PR #563 — its review threads and comments were not read, per the scope given for this review. Ticket ROCMAI-530 was retrieved by identifier (title/status/type only, no comments).