ROCMAI-83: extract common.rs from apps/rocmd/src/lib.rs

[jussielo-amd]

Summary

Second PR of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27). Pulls helper groups that are shared across ≥2 of the still-inline sandbox/mcp/service/watchers clusters into a new common.rs module, ahead of extracting those clusters themselves in later PRs:

• GPU/amd-smi snapshotting (gather_gpu_snapshot*, capture_amd_smi_json, run_command_with_timeout) — used by sandbox, mcp, and watchers
• The bridge-snapshot diagnostic (build_bridge_snapshot/print_bridge_snapshot/bridge_engine_inventory/rocmd_engine_inventory) — used by the CLI's BridgeSnapshot command and the MCP dispatcher
• CommandCapture + its construction plumbing — constructed in both sandbox and MCP code
• Small cross-cluster utilities: optional_arg, wait_for_port, parse_gpu_indices_arg, and the engine-healthcheck/endpoint-key-guard family — used by both service and watchers

This module wasn't in the original ticket (which only named persistence.rs) — these cross-cutting dependencies surfaced during boundary verification against current main. Adding common.rs keeps each later cluster PR from having to reach across into a sibling cluster file.

• Pure code motion, no behavior change.
• Call sites repointed to common::....
• The 8 tests that exercise these helpers directly (not mixed with a not-yet-extracted cluster's own domain logic) moved into common.rs's own #[cfg(test)] mod tests in this same PR.
• docs/architecture.md updated in this PR.

Independent of #477 (persistence.rs) — branched separately from main, not stacked, per the one-off PR approach for this phase.

Test plan

• cargo build (rocmd, and rocm+rocmd together to confirm the external API surface is untouched)
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test -p rocmd — 130 tests, same count as before this change (129 passed + 1 pre-existing ignored)
• cargo xtask manifest --check
• cargo fmt / prek hooks clean
• tests/e2e-cucumber (CI)

[Copilot]

Copilot review overview

🟡 Changes recommended

The architecture document incorrectly reports the still-open persistence.rs extraction as already present.

Review effort: Balanced
Findings: 1

Open (1)

• Document common.rs extraction; defer persistence.rs until landed · New

What changed in this PR

Extracts shared daemon helpers from lib.rs into common.rs without intended behavior changes.

Changes:

• Moves shared snapshot, command, healthcheck, and argument utilities.
• Relocates eight associated unit tests.
• Updates daemon architecture documentation.

File	Description
apps/​rocmd/​src/​common.rs	Adds shared helpers and tests.
apps/​rocmd/​src/​lib.rs	Repoints callers to common.
docs/​architecture.md	Updates modularization status.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jussielo-amd]

Addressed in 195678b: this branch is now rebased/restacked on top of #477 (rocmai-83-persistence) per AGENTS.md §11 (stacked PRs stay draft until the dependency merges), and the PR has been moved back to draft. By the time this PR's own commit lands, persistence.rs genuinely exists in the branch's history alongside common.rs, so the doc line is now accurate for this branch's actual content rather than describing a sibling PR's files.

Note: GitHub won't let this fork-based PR's base ref point at rocmai-83-persistence directly (that branch only exists on the fork, not on ROCm/rocm-cli), so the base field still shows main even though the branch is rebased on #477's commits — flagging in case that needs a different fix (e.g. pushing the branch to the upstream repo).

[r0x0r]

Review: code is clean; holding approval on one doc line

The extraction itself verified as pure code motion and I have no concerns with it. One issue in docs/architecture.md is the only thing between this and an approval, and it is a one-line fix.

The doc hunk describes a state this branch is not in

The new text reads "Extracted so far: persistence.rs (…) and common.rs (…)", but apps/rocmd/src/persistence.rs does not exist in this branch — git ls-tree pr-head apps/rocmd/src/ returns only common.rs, lib.rs, main.rs. record_event and load_managed_services are still sitting in lib.rs at lines 4890 and 5004 here.

This matters more than a normal doc nit because of how the five PRs in this phase interact, which is not visible from inside any one of them:

• All five rewrite the same single line of docs/architecture.md, and all five are branched independently from main. Four of them are therefore guaranteed to conflict on that line.
• Each one's text is a cumulative prefix that assumes the whole earlier sequence has already merged. That only resolves to a true statement if they merge in exactly the order 477 → 479 → 480 → 481 → 483.
• The failure case is quiet rather than loud: if this PR merges before #477, there is nothing on that line to conflict with, so it applies cleanly and main ends up asserting that persistence.rs exists when it does not. The conflict that would have caught it only appears once something else has touched the line.

docs/architecture.md opens by saying it is updated in the same PR as the code it documents and that "a stale-but-plausible-looking note is worse than an explicit prompt to check" — a concrete filename plus a symbol list for a file that is not there is exactly that.

Either of these resolves it:

1. Scope the hunk to this branch — say common.rs only, and let #480/#481/#483 each append their own module as they land. Self-consistent regardless of merge order, and the conflicts become trivial appends rather than whole-line rewrites.
2. Keep the cumulative text but gate the merge order, stating the dependency in the PR body (Depends on #477) so it cannot land first.

(1) is the more robust of the two; (2) relies on reviewers remembering the order at merge time.

Verified clean

• Pure code motion — every moved symbol compared against its original on main: error handling, ? propagation, .context(...) strings, timeout constants, match arms, and the #[must_use] on apply_endpoint_key_env all preserved verbatim.
• No silent CPU fallback introduced (AGENTS.md §6) — I looked at this specifically, since this PR moves the GPU-snapshot path. gather_gpu_snapshot_for_config is byte-identical; the config.telemetry.local_inspection_enabled() gate and the amd_smi_available: false note-only branch are unchanged, and capture_amd_smi_json keeps its early return on !output.status.success().
• Call sites — all 15 moved symbols repointed to common::; no missed site.
• Imports — all 15 dropped from lib.rs are genuinely unreferenced there. TcpStream looked like a survivor on a first pass but both remaining uses are fully-qualified std::net::TcpStream in test code, so there is no missing-import failure under -D warnings. I checked this one by hand rather than trusting the grep.
• Visibility — pub(crate) throughout, CommandCapture's four fields included; find_engine_plugin_binary correctly #[cfg(test)]-gated; rocmd_engine_inventory correctly stays private to the module.
• Tests — 130 test attributes conserved across the branch (122 + 8), matching the claimed 8 moved exactly.
• The ≥2-cluster rule holds for every item — I was ready to flag anything in here used by only one cluster, since that would contradict the module's own stated admission criterion, but each one checks out: update_check_message (sandbox + watchers), gather_gpu_snapshot_for_config (MCP + watchers), CommandCapture (sandbox + MCP), engine_healthcheck_ready (watchers + service lifecycle), optional_arg (three clusters). Defining the module by a stated rule and then actually honouring it is what keeps common.rs from becoming a junk drawer, which is the usual fate of a module with that name.
• Leak scan (AGENTS.md §2) — the one hit is a pre-existing comment containing the English word "private", moved verbatim. Clean.

Non-blocking: test-helper duplication

temp_app_paths, unique_test_root and workspace_test_artifact_dir are byte-identical copies in both common.rs and lib.rs test modules. Same note as on #477: #483 makes a third copy, and Phase 5 has seven target modules, so this ends around eight copies that can each drift independently. One #[cfg(test)] mod test_support; now is cheaper than reconciling them later.

---

🤖 by agent-hub on AMD AgentHub

[jussielo-amd]

Addressing the review (195678b):

• Architecture-doc ordering: restacked this branch on top of ROCMAI-83: extract persistence.rs from apps/rocmd/src/lib.rs #477 (base branch rocmai-83-persistence locally; GitHub won't let this fork-based PR's base ref point at a branch that only exists on the fork rather than on ROCm/rocm-cli, so the PR's base field still shows main — flagging in case that needs a separate fix). Moved back to draft per AGENTS.md §11 until ROCMAI-83: extract persistence.rs from apps/rocmd/src/lib.rs #477 merges. The doc line is now accurate for this branch's actual history.
• Dangling crate::load_managed_services reference: fixed — common.rs's build_bridge_snapshot now calls crate::persistence::load_managed_services, matching where ROCMAI-83: extract persistence.rs from apps/rocmd/src/lib.rs #477 relocated it.
• Test-helper duplication (non-blocking note): common.rs's test module now uses crate::test_support::temp_app_paths (the shared helper ROCMAI-83: extract persistence.rs from apps/rocmd/src/lib.rs #477 introduced) instead of its own byte-identical copies of temp_app_paths/unique_test_root/workspace_test_artifact_dir.

cargo build/clippy -D warnings/test -p rocmd all green on the rebased tree (129 passed, 1 pre-existing ignored, count conserved).

[rominf]

🔴 Automated review · pr-review-watcher · 195678b

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

Summary

This PR moves the rocmd helpers that several parts of the daemon use from apps/rocmd/src/lib.rs into a new apps/rocmd/src/common.rs, moves 8 tests with them, and updates docs/architecture.md. The code move is clean, but the docs line it rewrote still breaks CI: Needs work. Reviewed: the whole delta rocmai-83-persistence...195678b5 (the PR's own commit only): apps/rocmd/src/common.rs, apps/rocmd/src/lib.rs, docs/architecture.md, plus every call site and cfg arm in lib.rs that the move touches. Verified: cargo clippy -p rocmd --all-targets -- -D warnings is clean and cargo test -p rocmd passes (129 passed, 1 ignored) on Linux. Every moved item was diffed byte-for-byte against the code it replaced: the only changes are pub(crate) visibility, persistence:: becoming crate::persistence::, rustfmt re-wrapping, and the 8 tests dropping their super:: prefix. Every remaining lib.rs reference, including the cfg(windows) and cfg(not(unix)) arms, now goes through common::. Blocking: 1 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

• docs/architecture.md:34 — The rewritten line still cites `cli.rs`, `sandbox.rs`, `mcp.rs`, `service.rs`, `webhook.rs` and `watchers.rs` in backticks under the ### `apps/rocmd` heading. None of those files exist under apps/rocmd.
  • Check 1: xtask/src/architecture_doc.rs treats a bare backticked .rs name as a path and checks it against its heading's directory (citation_exists). The cargo xtask check-architecture-doc step therefore fails, which is the "Architecture doc path citations are current" check. Its log on this head lists exactly these six paths.
  • Check 2: the same file's run_passes_against_the_real_doc unit test calls that checker, and windows-build-and-test runs the workspace tests including xtask. The Windows job log on this head confirms it: its only failing test is architecture_doc::tests::run_passes_against_the_real_doc (200 passed, 1 failed). The Windows failure is not caused by any Windows code.
  • Where it came from: the parent branch (7fe516d) introduced the "still pending" list, which also cited common.rs; CI on the base tip a6b6e35 fails both checks too, with 7 stale citations. This PR fixes the common.rs entry (7 down to 6) but rewrote the line and kept the other six.
  • Why it blocks: the doc check is red on this head because of text this PR wrote.
  • Fix: name the pending modules without backticks, e.g. "still pending: the cli, sandbox, mcp, service, webhook and watchers modules".
  • Windows cfg arms: none of them in lib.rs call the moved items, and common.rs has no platform-specific code. Nothing else in the delta is Windows-specific.

Non-blocking

• apps/rocmd/src/common.rs:83,194,299,373 — gather_gpu_snapshot, find_engine_plugin_binary (test-only), healthcheck_response_ready and engine_request are pub(crate), but nothing outside common.rs uses them. Make them private, as was already done for capture_amd_smi_json and rocmd_engine_inventory.
• docs/architecture.md:34 / commit message — Both describe common.rs as helpers "shared across ≥2 of those remaining clusters". That is not true of three items, which each have a single caller in one part of the daemon: parse_gpu_indices_arg (only supervise_service, service code), engine_healthcheck_ready (only wait_for_service_ready, service code) and wait_for_port (only endpoint_service_recovery_reason, watchers/recovery code). Either move them later with their part of the daemon (service.rs/watchers.rs), or reword the rule so the doc does not state a criterion the module does not follow.
• apps/rocmd/src/common.rs:30,49,155 — print_bridge_snapshot, build_bridge_snapshot and bridge_engine_inventory are only used by the BridgeSnapshot CLI command and the MCP tool handler. They belong to the MCP/bridge code rather than being shared helpers, which makes common.rs a mixed bag that the planned mcp.rs extraction will have to pull apart again.
• PR text — this is a pure refactor with no observable behaviour change, so AGENTS.md §3 needs no scenario. But §3 asks for the reason to be stated in the PR text; the commit message only says "No behavior change", so add one sentence explaining why no scenario is needed.

Stack state

The parent PR #477 has since merged into main, after being rebased, so its final head is not the rocmai-83-persistence tip this PR is based on. This PR still targets that branch. It needs a rebase onto main and a retarget to main. When rebasing, take main's version of the architecture line as the starting point; the blocker above applies to whatever text this PR adds on top of it. Both red checks were run against the old base.

[rominf]

🔴 Automated review · pr-review-watcher · 195678b

Requesting changes for one defect: docs/architecture.md:34 still cites cli.rs, sandbox.rs, mcp.rs, service.rs, webhook.rs and watchers.rs in backticks under the apps/rocmd heading, and none of those files exist there. That keeps "Architecture doc path citations are current" red. It also fails windows-build-and-test, through the architecture_doc::tests::run_passes_against_the_real_doc test.

To resolve: name the pending modules without backticks, or drop the list. Also rebase onto main and retarget there now that #477 has merged. Details are in the review comment on this PR.