ROCMAI-83: extract common.rs, webhook.rs, and cli.rs from apps/rocmd/src/lib.rs

[jussielo-amd]

Summary

Second, third, and fourth PRs of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27), consolidated into one PR. Pulls common.rs, webhook.rs, and cli.rs out of apps/rocmd/src/lib.rs. These three were originally opened separately as #479/#480/#481; consolidated here to cut down the PR count for this phase, since all three are either genuinely standalone utility code or thin mechanical dispatch with no domain-logic interdependencies worth reviewing apart.

common.rs — helper groups shared across ≥2 of the still-inline sandbox/mcp/service/watchers clusters:

• 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.

webhook.rs — the local webhook source: axum routes, request validation, watcher-kind allow-list. Fully self-contained except one call into payload_string, which stays at the crate root until watchers.rs is extracted (#487) and will need to re-point that one call site.

cli.rs — the Cli/Command clap definitions, SandboxToolArg/SandboxToolPolicy, and the top-level dispatch (run_cli/run_bin_cli/run_from_args). Follows the mechanical-relocation pattern (per docs/architecture.md's convention) rather than full domain extraction: run_cli is thin dispatch, and its match arms call into clusters that haven't been extracted yet (sandbox, mcp, service, watchers) via crate::-qualified paths, updated as each lands in #483/#487. lib.rs re-exports run_bin_cli/run_from_args via pub use cli::{...}; — the crate's only two externally-consumed symbols are untouched. SandboxToolArg/SandboxToolPolicy are consumed pervasively by the still-inline sandbox.rs cluster and its test suite — all call sites repointed to cli::SandboxToolArg/cli::SandboxToolPolicy.

• Pure code motion across all three modules, no behavior change.
• Call sites repointed to common::.../webhook::.../cli::....
• The 29 tests that exercise these modules' own logic directly (8 common + 17 webhook + 4 cli) moved into each module's own #[cfg(test)] mod in this same PR.
• docs/architecture.md updated in this PR.

Targets main directly (not stacked on a sibling PR branch) so this PR can't be silently auto-closed if an intermediate branch gets deleted — the merge order relative to #483 and #487 is documented, not structural.

Supersedes #480 and #481, which are closed in favor of this PR.

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 · b7f2199

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.

Round 3, a full review of b7f2199d. The previous round's commit b843952a is no longer in the history after the rebase, so this round re-read the whole change instead of a delta. It also answers the latest update comment: the PR head has now moved to the fixed commit, and CI ran on it.

Summary

This PR moves the helpers shared by more than one part of the daemon out of apps/rocmd/src/lib.rs into a new apps/rocmd/src/common.rs. The moved helpers cover GPU/amd-smi snapshots, the bridge snapshot, command capture with timeouts, healthchecks and endpoint keys. The PR also documents the rule for what belongs in common.rs in docs/architecture.md. Outcome: no blocking findings. Reviewed: the whole change (main...b7f2199d, 1 commit; apps/rocmd/src/common.rs, apps/rocmd/src/lib.rs, docs/architecture.md), plus every caller of the moved names in lib.rs. Verified:

• The move is faithful. Each moved item matches its original body once the pub(crate) and common:: prefixes are stripped. #[cfg(test)] on find_engine_plugin_binary is kept.
• Build and tests pass. cargo clippy -p rocmd --all-targets -- -D warnings is clean, and cargo test -p rocmd gives 130 passed, 0 failed.
• It still builds on current main. main has moved two commits past this branch's base (975dd2c8). git merge-tree merges the two cleanly, and cargo check -p rocmd --all-targets passes on the merged tree.
• Non-Linux code is unaffected (static check only). No cfg(windows) or cfg(not(unix)) arm uses a moved name or a removed import. A real Windows cross-build was not run.

Blocking: 0 · Non-blocking: 7.

Previous round's items. The admission-rule mismatch has been non-blocking since the last round. Most of it is fixed: the four single-caller helpers went back to lib.rs, and run_rocm_capture_for_paths moved into common.rs. What remains is listed under Non-blocking. No change request is held on this PR.

CI. One check failed: E2E tests (MI300X). This PR did not cause it.

• 4 scenarios failed unexpectedly: serve-vllm-inference, chat-end-to-end-local-model, bench-load-real-serve and therock-next-09-live-install-reports-vllm-rocm10x-discovery-pins.
• The causes are vLLM's own "Engine core initialization failed" on the GPU host and failed llama.cpp/TheRock artifact downloads (HTTP 404/403).
• An unrelated PR failed the same way on the same lane in the same hour.
• Each failure happens inside vLLM's startup or in an external download, not in code this PR moved.

A re-run once the lane is healthy should clear it. Skill checks (skillscope) and Sphinx docs build (-W) finished as skipped, not failed.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• docs/architecture.md:34 — The rule says a helper goes into common.rs "only once a second still-inline cluster calls it directly". Four items there still don't meet it:

  • run_rocm_capture: only called from handle_mcp_tool_call.
  • wait_for_port: only endpoint_service_recovery_reason.
  • healthcheck_response_recoverable: only the recovery pair service_record_matches_recovery_event and find_recoverable_service.
  • healthcheck_response_ready: only engine_healthcheck_ready, now back in lib.rs.

  Either move them back, or loosen the sentence to match what was done.

• apps/rocmd/src/common.rs (healthcheck_response_ready) — engine_healthcheck_ready went back to lib.rs, but the one predicate only it uses stayed in common.rs. That splits a single-cluster pair across two files.

• apps/rocmd/src/common.rs (bridge_engine_inventory, apply_endpoint_key_env, run_rocm_capture_for_paths) — Each has only one direct caller in lib.rs and qualifies only through another common.rs helper. If that's intended, the doc's word "directly" should say so.

• apps/rocmd/src/common.rs:59,193,354 — gather_gpu_snapshot, run_command_with_timeout and engine_request are pub(crate) but nothing outside common.rs calls them. They were private before the move and can stay private.

• apps/rocmd/src/common.rs:169-170 — The test-only find_engine_plugin_binary is used only by a test in the same module, so it does not need pub(crate). It could live inside mod tests.

• apps/rocmd/src/common.rs:232 — The commit message says run_rocm_capture/run_rocm_capture_for_paths are "genuinely shared by the sandbox and MCP clusters". That's true only of the pair: run_rocm_capture is MCP-only and run_rocm_capture_for_paths has only a sandbox caller.

• apps/rocmd/src/lib.rs:1312 — run_process_with_timeout (sandbox-only) is still a near-copy of common::run_command_with_timeout. This predates the PR, and the new doc sentence explains why it stays. Worth tracking so the two copies get merged when the sandbox code is extracted.

[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.

[jussielo-amd]

Update since the reply above: mid-batch, #477 was squash-merged into main (commit 57b78c65). Because this repo squash-merges, every downstream branch — including this one — still carried the original pre-squash commits as ancestors, which broke each branch's diff against main and re-triggered the same architecture-doc staleness this review caught.

This branch has been corrective-rebased directly onto main at 57b78c65 (new tip b843952a). The architecture-doc wording and the dangling crate::load_managed_services fix described above both still hold under the new base — re-verified with cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, and the full test suite, all green. Pushed to both this fork and the upstream mirror.

Ready for another look.

[rominf]

🔴 Automated review · pr-review-watcher · b843952

Requesting changes for one defect. The rule this PR states for common.rs, "helpers shared across ≥2 of those remaining clusters" (in docs/architecture.md:34 and the commit message), does not match the code. parse_gpu_indices_arg, engine_healthcheck_ready and print_bridge_snapshot each have a single caller. run_rocm_capture_for_paths, which the sandbox and MCP code both use, stays in lib.rs. docs/architecture.md is what the remaining extraction PRs follow, so the stated rule needs to be true.

To resolve, choose one: reword the architecture line (and the PR text) to the rule actually applied, or move the helpers so the rule holds. Either way, add one sentence that sets the cluster boundaries. The earlier change request, about module files cited in the doc that do not exist, is resolved at this head and has been withdrawn. Details are in the review comment on this PR.

[r0x0r]

Re-checked this at b843952 after the restack. The extraction itself is clean and my original objection is resolved — I verified it as pure code motion rather than reading the diff: function bodies are identical, the item sets before and after match exactly (nothing lost, nothing added, tests included), doc-comment counts are unchanged, and the attribute census reconciles. The only deltas are pub(crate) markers, module re-qualification, rustfmt reflow and the SPDX header. The pub(crate) additions aren't a widening either — these were private items in the crate root, which was already crate-wide reachability.

docs/architecture.md now reads correctly for this point in the stack, so the doc line I held on is genuinely fixed.

One thing to sort before this is marked ready: CI has never run on this head. The only check present on b843952 is the Read the Docs build. The last real run was on the pre-restack sha 195678b5, and it failed the xtask architecture_doc gate on exactly the wording I'd complained about — six not-yet-existing .rs files. That failure is moot now, but the gate has never been exercised against the corrected text, so we have no signal that it passes.

Two related notes:

• The branch is 1 ahead / 5 behind main.
• The standing CHANGES_REQUESTED from the automated reviewer is pinned to 195678b5 and no longer reflects this head — I don't think it should be read as live.

A rebase onto main and a re-push would get CI onto the real head. Worth doing here first rather than downstream: #480, #481 and #483 all stack on this one and inherit the same gap.

Leaving this as a comment rather than a block — there's no code defect here, just a missing signal.

[jussielo-amd]

Fixed the branch mismatch flagged above: the round-2 fix (previously 801a98d2) had landed on a stray rocmai-83-common branch on ROCm/rocm-cli instead of on the fork branch this PR tracks (jussielo-amd/rocm-cli:rocmai-83-common), so the PR head never moved and CI never ran against it.

Rebased that fix onto current main (was 12 commits behind) and force-pushed to the correct fork branch — new head b7f2199. The stray upstream rocmai-83-common branch has been deleted.

Re-verified on the rebased tree: cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test -p rocmd --all-targets (130 passed, 1 pre-existing ignored), cargo xtask check-architecture-doc, cargo xtask check-crate-edges all green. This should now trigger CI properly on the real head.

[jussielo-amd]

Scope note: this PR now covers common.rs + webhook.rs + cli.rs (consolidated from #480 and #481, which are closed). Diff and description updated accordingly.

[rominf]

Round 3 is in the review report above (#479 (comment)), updated in place. The PR head has moved to b7f2199d, so the branch fix worked. The review found no blocking issues, and 7 non-blocking notes are listed there. The one failed check, E2E tests (MI300X), comes from the GPU lane's environment and not from this change. An unrelated PR failed the same way in the same hour, so a re-run should clear it.
🔴 Automated review · pr-review-watcher · b7f2199

[r0x0r]

Approving — the extraction holds up as pure code motion

I verified this mechanically rather than reading the diff by eye, because "pure code motion" is exactly the claim that's easy to assert and hard to eyeball across 700 moved lines. What I checked:

• Every moved item is byte-identical to its pre-move body, apart from the three expected deltas: fn → pub(crate) fn, persistence:: → crate::persistence::, and rocm_core::daemon_binary_path() → daemon_binary_path() (now covered by the import).
• Nothing was lost and nothing was duplicated. The item set before the change equals the item set after across lib.rs + common.rs, with an empty intersection between the two files — so a mis-repointed call site couldn't have silently resolved to a leftover copy; it would have been a compile error.
• All 16 touched functions in lib.rs differ only by common:: qualification on the call sites.
• All 8 moved tests have identical bodies (one drops a now-redundant super::), and the count reconciles: 127 → 119 + 8.
• No silent name re-resolution is possible here. There are no glob imports anywhere in apps/rocmd/src, and common.rs's trait-import set is a strict subset of lib.rs's — a subset can only lose a candidate, which is a compile error, never a quiet change of which method wins. cfg(test) gating survived intact on all four sites, and no moved item carries a platform cfg.

A couple of things I went looking for and that did not hold up as problems: gather_gpu_snapshot ends up pub(crate) with no callers outside common.rs, but it had exactly one caller before the move too (its own sibling), so no call site was dropped — it's just wider than it needs to be. And the failing E2E tests (MI300X) lane is vLLM Engine core initialization failed on the GPU host, which this change can't reach.

One non-blocking note, left inline

The admission rule the doc hunk adds is contradicted by two helpers in the module it describes. Not worth holding the merge over, but worth a follow-up so the next cluster PR isn't working from a rule the tree already disagrees with.

---

This reflects a read of the diff and the surrounding code, not a runtime check — the E2E lane above is still worth a re-run before merge.

[jussielo-amd]

Addressed in e403350:

• Moved run_rocm_capture, wait_for_port, healthcheck_response_recoverable, and healthcheck_response_ready back to lib.rs next to their sole callers — each has exactly one still-inline caller cluster (MCP for the first, the recovery/watcher pipeline for the rest), so none met common.rs's own ≥2-cluster admission rule. Their tests moved with them.
• As a side effect, run_rocm_capture_for_paths is now called directly by both the sandbox cluster (run_sandbox_tool) and the MCP cluster (run_rocm_capture), so the commit message's "genuinely shared by the sandbox and MCP clusters" claim is now accurate — no commit-message edit needed.
• Dropped pub(crate) on gather_gpu_snapshot, run_command_with_timeout, and engine_request — nothing outside common.rs calls them.
• Moved find_engine_plugin_binary inside its own #[cfg(test)] mod, since only a test there calls it.
• docs/architecture.md:34 — clarified the admission rule to cover transitive qualification through another common.rs helper that itself has direct ≥2-cluster usage (covers bridge_engine_inventory via build_bridge_snapshot and apply_endpoint_key_env via engine_request).

Not changing: the lib.rs:1312 run_process_with_timeout duplication — pre-existing debt the doc already explains, no action requested.

cargo check -p rocmd --all-targets, cargo clippy -p rocmd --all-targets -- -D warnings, cargo fmt -p rocmd -- --check, and cargo test -p rocmd (130 passed) all pass on the new commit.