[jussielo-amd]

Why

ROCMAI-720. rocmd (common.rs, mcp.rs) and rocm's chat tool bridge (chat.rs) each carried their own copy of the same plumbing around rocm_core::process::run_with_timeout: the CommandCapture { argv, exit_status, stdout, stderr } struct, the wrapper that spawns the rocm binary and fills it in, and the command_capture_text formatter (which rocmd had twice). The copies were identical apart from how the child's environment was set, so any future change to one had to be remembered in the others.

What changes

• A new rocm_core::command_capture module owns CommandCapture, command_capture_text, and run_rocm_capture_with_env(args, env, timeout). Both apps use these instead of their own copies.
• The environment is a parameter (&[(&str, &Path)], the same shape as process.rs's env_overrides) rather than something rocm-core decides. rocmd passes its three ROCM_CLI_CONFIG_DIR/DATA_DIR/CACHE_DIR overrides; rocm passes app_path_env_vars(paths) from main.rs. Each app keeps a one-line run_rocm_capture_for_paths that supplies its own list, so the existing call sites and the comments that name that function stay as they were.
• docs/architecture.md gains an entry for the new module, and the chat.rs, rocmd common.rs and mcp.rs entries no longer claim ownership of what moved.

No behavior change: the spawned binary (daemon_binary_path()), the three env vars, the argv, the unwrap_or(1) exit-status mapping, the lossy UTF-8 decoding and the failed to run … error context are the same as in both removed copies.

An alternative was to move app_path_env_vars into rocm-core as well and have the shared function take &AppPaths. It was not done because which variables a rocm child should get is each caller's decision, and taking the list as a parameter keeps that decision with the caller.

Verification

• cargo test --workspace --all-targets --no-fail-fast: 4324 passed, 0 failed, 17 ignored. An earlier full run without --no-fail-fast hit one failure in therock::archive_properties::sdk_tarball_unpack_extracts_a_benign_tarball_into_the_install_root (failed to launch tar: No such file or directory). That test is in a module this change does not touch, and it passed when re-run on its own and in the full --no-fail-fast run.
• cargo clippy --workspace --all-targets -- -D warnings: clean. cargo fmt --all --check: clean.
• python scripts/smoke_local.py: smoke: ok.
• The three command_capture_text unit tests moved with the formatter, unchanged. A new test, a_failing_child_is_captured_with_the_env_it_was_given, runs a real shell child (sh on Linux, cmd on Windows) through the shared capture path. It checks that the caller's env var reaches the child and that a non-zero exit comes back as a CommandCapture with argv, exit status and both streams rather than as an error. dash_seam's seam_execute_approved_captures_a_failing_command_as_a_result still drives the real chat replay chain end to end against a rocm subprocess, and passes.
• Not verified: the Windows (cmd) branch of the new test was not run locally (Linux host only). It follows the same cmd /C pattern as process.rs's existing a_command_that_succeeds_reports_its_output.

Scope

This is a refactor with no user-observable change, so it adds no Gherkin scenario (AGENTS.md §3). It does not touch the bounded-execution logic in run_with_timeout. It also does not converge the other capture helpers tracked in #623.

---

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (Not a bug fix.)
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md (the clap declaration and dispatch wiring staying in main.rs/lib.rs is expected, not a violation). (No new subcommand; the shared code is its own rocm-core module.)
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. Covers claims of an outcome, remediation advice naming a command, and promises that something will not happen. (No user-facing message changed.)

[Copilot]

🟢 Approval recommended

The narrow extraction preserves existing subprocess semantics and caller-specific environments, with no blocking issues identified.

No open issues 🎉

What changed in this PR

Centralizes subprocess capture in rocm-core so rocm and rocmd share the same implementation while retaining caller-specific environment settings and existing behavior.

Changes:

• Extracts CommandCapture, subprocess execution, and output formatting into a shared module.
• Updates both apps and test fixtures to use the shared implementation.
• Moves formatter tests, adds a child-process capture test, and updates architecture documentation.

Verification reported in the PR was not rerun; the new test’s Windows branch remains locally unverified.

File	Description
docs/​architecture.md	Documents shared capture ownership and app wrappers.
crates/​rocm-core/​src/​lib.rs	Exports the capture module and shared API.
crates/​rocm-core/​src/​command_capture.rs	Implements shared capture and formatting with tests.
apps/​rocmd/​src/​watchers.rs	Uses the shared capture type in test fixtures.
apps/​rocmd/​src/​sandbox.rs	Imports the shared capture type.
apps/​rocmd/​src/​mcp.rs	Reuses the shared type and formatter.
apps/​rocmd/​src/​common.rs	Delegates capture while preserving daemon environment overrides.
apps/​rocm/​src/​chat.rs	Replaces duplicate capture code with shared helpers.

_{Review effort: Balanced}

[nowycondro]

🔴 Automated review · pr-review-watcher · 299e97a

Review — needs work

Full review of the whole diff (8 files, +219/-132), split into two independent blind sub-reviews — the new shared crates/rocm-core/src/command_capture.rs module, and the five consumer call-sites plus docs/architecture.md — alongside a driver pass covering history, CI status, and design review.
Implements ROCMAI-720 (the ticket itself could not be retrieved — no issue tracker tool is available in this environment, so its contents are not assumed); the PR correctly scopes itself out of issue #623 (converging the other duplicate subprocess-capture helpers), and that reference checks out against #623's actual content.

Blocking

• A required check is failing on the current head commit — E2E tests (MI300X, current), https://github.com/ROCm/rocm-cli/actions/runs/40349430611/job/122039400043
  gh pr view 734 --json statusCheckRollup shows this job concluded FAILURE on commit 299e97af; every other check (build/test, clippy, fmt-equivalent, the other E2E hardware lanes, CodeQL, docs) is green. The job's logs could not be inspected from here (gh api .../actions/runs/.../jobs returned 403 Forbidden), so whether this is a real regression or a self-hosted-runner/hardware flake is undetermined from this review alone — the job also started roughly 15 hours after its sibling jobs in the same workflow run, consistent with a hardware-queue delay rather than a code-triggered failure, but that is circumstantial, not a finding of innocence.
  Confidence 85 (a failing check on the reviewed commit is directly observed, not inferred) · ci · Fix: inspect the job log with repo-write/maintainer access, and either re-run it if infra-flaky or fix the regression if real, before merge.

Non-blocking

None

Decisions for the author

• The new command_capture.rs test exercises capture_program (private) rather than the public run_rocm_capture_with_env — non-blocking-improvement
  This is explained in the test's own comment (daemon_binary_path() isn't scriptable from a rocm-core unit test), and it's a reasonable compromise — the untested surface is run_rocm_capture_with_env's one-line pass-through to capture_program, which is thin enough that it's unlikely to hide a bug. Still, the author may want to confirm that's an acceptable permanent gap rather than a temporary one.

Positive signals

• run_rocm_capture_with_env takes the child's environment as a caller-supplied &[(&str, &Path)] instead of the crate deciding it — this lets rocmd's 3-variable case and rocm's app_path_env_vars case share one function with no adapter code on either side, and the PR description explicitly documents the rejected alternative (baking app_path_env_vars into rocm-core) and why it wasn't taken.
• The new unit test a_failing_child_is_captured_with_the_env_it_was_given spawns a real shell child and asserts env-propagation, non-zero-exit-as-data (not Err), argv shape, and both streams in one pass — stronger coverage of the spawn/env path than existed in any of the three duplicated copies it replaces, which only tested the text formatter.
• The module's doc comment draws an explicit, verified-accurate boundary with process.rs's bounded-execution logic ("the bounded-execution half ... is run_with_timeout's, not this module's"), which is the kind of comment that prevents the next change from re-duplicating timeout/draining logic here.

Deployment notes

None — this is an internal refactor with no behavior, API-surface-for-end-users, or configuration change; the PR's own "no behavior change" claim held up against every call site checked (exit-status mapping, UTF-8 decoding, argv construction, error-context text, and the three ROCM_CLI_*_DIR / app_path_env_vars environment sets, all traced to be identical to what they replaced).

What this covered

Every file in the diff, completely, plus surrounding code needed to judge it in place: crates/rocm-core/src/process.rs and binary_paths.rs (unchanged, but what the new module calls into and documents), and apps/rocm/src/main.rs's apply_app_path_env/app_path_env_vars (unchanged, confirmed behavior-equivalent to the function the chat-tool-bridge call site switched to). Commit range 63ecab2d26ffb569ed857a0e7b0ab26a12bd5853...299e97afa49c0a2717a0ac374e5207dc68f3cd25 (single commit, DCO-signed). Also checked: repo-wide grep for any leftover reference to the old module-local CommandCapture type; the apps/rocmd crate-layering guardrail (no import of anything from the rocm crate); and the linked issue #623 for an accurate scope boundary.

Did not run: cargo test/clippy/fmt locally — no cargo toolchain available in this review environment; relied on the PR's green CI (build-and-test, windows-build-and-test, clippy, Test (affected crates) all SUCCESS on this head commit) in place of a local run, per the one failing check noted above. Ticket ROCMAI-720 named but not retrievable from here (no tracker tool available) — judged the change only against its own description and the diff. Not reconciled against any prior review discussion on this PR.

[rominf]

🔴 Automated review · pr-review-watcher · 299e97a

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

Review — no blocking findings

Full review of the whole change.
Implements ROCMAI-720 as the description states it. It does not converge the capture helpers tracked in #623 and says so, and #623 is still open. The ROCMAI-720 ticket was not retrieved from here.

Blocking

None

Non-blocking

None

Decisions for the author

• The ROCM_CLI_*_DIR list still exists twice, once in each app — tradeoff
  The PR removes the duplicated capture code, but the child-environment list is still written out in two places: inline in apps/rocmd/src/common.rs:224-228, and in app_path_env_vars in apps/rocm/src/main.rs:11263. The two lists are identical today. The process that reads them is AppPaths::discover, in crates/rocm-core/src/paths.rs:34, so the producer lists sit in two apps and the consumer sits in rocm-core.
  • For the current design: the description says it considered moving the list and chose to keep "which variables a rocm child should get" with each caller. Taking the env as a parameter also matches the shape of process.rs's env_overrides.
  • Against it: if a fourth ROCM_CLI_* override is added to paths.rs, someone has to remember both app lists. main.rs already appends ROCM_CLI_ENGINE_ENVS_ROOT in another path. An AppPaths method in rocm-core that returns the list would put producer and consumer side by side, and a caller could still extend it.
    Please confirm the split is deliberate, given that duplicated implementations are where this repo's bugs tend to collect.

Positive signals

• The new test drives the private capture_program with a real sh/cmd child. That avoids depending on daemon_binary_path(), which could resolve to the test harness, and the test explains why. Mutation check: when the command.env(key, value) loop was removed, a_failing_child_is_captured_with_the_env_it_was_given failed (left: "", right: "marker-value"). So the test does pin the one parameter that differs between the old copies.
• Behaviour is the same as before at every call site. I compared both removed copies with the shared one: same binary resolution, run_with_timeout(command, timeout, "process"), stdio handling, unwrap_or(1) exit mapping, lossy decoding and failed to run … context. app_path_env_vars sets the same three variables that apply_app_path_env used to set.

Deployment notes

None. rocm_core gains a public module and three re-exports, all consumed only inside the workspace.

What this covered

Read in full: every file in the diff (base c586618…299e97af, one commit). I also read the surrounding code: app_path_env_vars/apply_app_path_env in main.rs, the run_rocm_command_for_paths hand-built capture in chat.rs, the dash_seam.rs replay test, paths.rs's env reads, process.rs's env_overrides, and the other uses of daemon_binary_path.

What I ran, all inside the checkout with its own target/:

• cargo test -p rocm-core --lib command_capture: 4 passed.
• The env-loop mutation described above: it failed as expected.
• cargo clippy -p rocm-core -p rocm -p rocmd --all-targets -D warnings, after touching the sources: clean.
• cargo xtask check-crate-edges: pass.
• cargo test -p rocmd --lib: 154 passed.
• cargo build -p rocm, then cargo test -p rocm --bin rocm -- chat:: dash_seam::: 114 passed. This included seam_execute_approved_captures_a_failing_command_as_a_result, which ran against the built binary and was not skipped.
• Sign-off check: exit 0, 1 commit checked, none missing.

CI at this head: every check is green or skipped except "E2E tests (MI300X, current)". That job fails the same way on main's latest runs: the lemonade/TheRock downloads return 404/403. So it does not bear on this change. windows-build-and-test passed, which covers the cmd branch the description lists as not run locally.

Did not run:

• The local Windows-target cargo check. The x86_64-pc-windows-gnu target is installed, but ring's build script needs x86_64-w64-mingw32-gcc, which is absent. The diff has no #[cfg(windows)] arms; there is only a cfg!(windows) runtime branch in the test, and that compiles on both platforms.
• The full workspace test suite and smoke_local.py.
• Fan-out. I used a single pass because the change is small enough for one context to read completely, and I worked through the design questions in that same context, so they were not independent.
• Prior changes. No prior reviews were consulted.
• The ROCMAI-720 ticket was not retrieved, because no tracker tool was provided for that project.

This report was not reconciled against the PR's discussion.

[nowycondro]

🔴 Automated review · pr-review-watcher · 2f2d409

Review

Full review of the whole diff (8 files, +219/-132) against the current head, reviewed as a single pass (the change is one tightly-coupled mechanical move, well under the size where splitting into independent sub-reviews would add coverage). Covers correctness, cross-file consistency, test quality, CI status, and the description/ticket relationship.
Implements ROCMAI-720. The ticket itself could not be retrieved (no issue-tracker tool is available in this environment), so it is judged only against its own description and the diff; the description's claim that this does not converge the other duplicated capture helpers checks out against issue #623, which is open and describes exactly those four other helpers.

Blocking

• A check is failing on the current head commit, and two hardware E2E lanes had not finished — E2E tests (Strix Halo, Windows, current), https://github.com/ROCm/rocm-cli/actions/runs/40509170172/job/122206371032
  gh pr view 734 --json statusCheckRollup shows this job FAILURE on commit 2f2d4091, with E2E tests (MI300X, current) and E2E tests (W7900, current) still pending at review time; every other check (build-and-test, windows-build-and-test, clippy, Test (affected crates), Coverage, CodeQL, docs) is green. The job's own logs could not be inspected from here (gh api .../actions/jobs/... returns 403 Forbidden). Checking the same job name on other recent PRs found it also FAILURE on #762 (an unrelated, already-merged PR touching only therock/test-locking code), which is evidence this is a pre-existing self-hosted-runner flake rather than something this diff caused — but that is circumstantial, not a finding of innocence, and the PR's mergeStateStatus is currently BLOCKED.
  Confidence 85 (the failure on the reviewed commit is directly observed, not inferred) · ci · Fix: confirm against the job log (needs maintainer/repo-write access) whether this is the known Strix-Halo-Windows flake or a real regression, and wait for the two still-pending hardware lanes before merging either way.

Non-blocking

None

Decisions for the author

• The new command_capture.rs test exercises capture_program (private) rather than the public run_rocm_capture_with_env — non-blocking-improvement
  The test's own comment explains why: run_rocm_capture_with_env resolves daemon_binary_path(), which in a rocm-core unit test is either the built rocm binary next to the test harness or the harness itself — neither a child this test can script against. The untested sliver is run_rocm_capture_with_env's one-line delegation to capture_program, thin enough that it is unlikely to hide a bug, but the author may want to confirm that gap is permanent rather than temporary.

Positive signals

• run_rocm_capture_with_env takes the child's environment as a caller-supplied &[(&str, &Path)] instead of rocm-core deciding it — this lets rocmd's 3-variable case and rocm's app_path_env_vars case share one function with no adapter on either side, and the PR description documents the rejected alternative (baking app_path_env_vars into rocm-core) and the actual reason it was rejected (which caller should own the decision), not just that an alternative existed.
• The new unit test a_failing_child_is_captured_with_the_env_it_was_given spawns a real shell child and in one pass asserts env-propagation, non-zero-exit-as-data (not Err), argv shape, and both streams — this is more coverage of the spawn/env path than any of the three duplicated copies it replaces had (they only tested the text formatter). Verified it is a meaningful test, not a vacuous one: deleting the for (key, value) in env { command.env(key, value); } loop would make the child echo an empty string instead of marker-value, failing the assertion.
• Verified "no behavior change" end-to-end rather than taking the description's word for it: the argv construction, unwrap_or(1) exit-status mapping, lossy UTF-8 decoding, "failed to run {}" error context, and all three ROCM_CLI_CONFIG_DIR/DATA_DIR/CACHE_DIR env assignments in rocmd (plus rocm's app_path_env_vars, itself unchanged by this PR) are byte-for-byte identical to what they replaced, just relocated.
• Repo-wide grep for every removed symbol (CommandCapture, command_capture_text, apply_app_path_env at the old call site) turned up no stale references anywhere, including tests/e2e-cucumber/: the migration is complete, not partial.

Deployment notes

None. Internal refactor with no user-observable behavior, API-surface, or configuration change — consistent with the PR's own "Scope" section, and confirmed by tracing every call site rather than assuming it.

What this covered

Every file in the diff, completely, plus the surrounding code needed to judge it in place: crates/rocm-core/src/process.rs (run_with_timeout's signature) and binary_paths.rs (daemon_binary_path's signature), apps/rocm/src/main.rs's app_path_env_vars/apply_app_path_env (unchanged, confirmed still used correctly at the one call site this PR touches and unaffected everywhere else), and all five call sites across chat.rs, common.rs, mcp.rs, sandbox.rs and watchers.rs. Diffed against the commit's actual parent, 24e405a7...2f2d4091 (single DCO-signed commit).

Did not run: cargo test/clippy/fmt locally — no cargo toolchain in this environment; relied on CI's build-and-test, windows-build-and-test, clippy and Test (affected crates), all SUCCESS on this head commit, in place of a local run (see the one failing/two-pending checks noted above). History and prior-review passes did not run as separate workers — folded into the single pass above given the diff's size. Ticket ROCMAI-720 named but not retrievable from here (no tracker tool available). Not reconciled against any prior review discussion on this PR, including this reviewer's own earlier pass against commit 299e97af, which targeted different code and is superseded by this one.