ROCMAI-756: untangle the rocmd module cycles and move the service, common and part of the watcher code into focused modules by jussielo-amd · Pull Request #831 · ROCm/rocm-cli

jussielo-amd · 2026-11-02T11:41:30Z

Why

apps/rocmd/src/service.rs, common.rs, watchers.rs and lib.rs each hold several unrelated clusters of the daemon (service supervision, readiness probing, the daemon loop, engine IPC, host snapshots), and the sandbox tool vocabulary and the service-record loader sit in the wrong modules, which is the source of the cycles between them. This change moves each cluster into a module of its own, as pure moves, so that the production module graph of rocmd has no cycles and each concern can change without touching the others. Delivers steps PR 1 to PR 10 and PR 13 of ROCMAI-756 only; the remaining steps (PR 11 and 12, PR 13b, PR 14 to 18) are tracked as ROCMAI-822.

What changes

Each step below is one commit, so the branch can be reviewed commit by commit:

The sandbox-tool vocabulary moves into sandbox_tool.rs.
Stop-server result shaping moves into sandbox.rs.
load_service_record moves into persistence.rs.
The supervisor and readiness code moves into supervise.rs, and 5. the recovery respawn joins it, so supervise_service and its recovery live in one module.
The daemon loop and status move into daemon.rs.
payload_string moves into common.rs, and 8. single-caller helpers are returned to their callers (run_rocm_capture to mcp.rs, wait_for_port to watchers.rs, gather_gpu_snapshot made private).
Engine IPC and the endpoint-key guards move into engine_ipc.rs.
The host and bridge snapshot move into host_snapshot.rs.
print_json moves into cli.rs.

The crate's public API is unchanged and apps/rocm's in-process daemon path compiles untouched. Production code is acyclic after step 11 (checked with a strongly-connected-component pass over the crate:: edges); a few test-only edges remain, as the ticket anticipated.

Besides the relocations, the commits make these edits: items that had a single caller are made private (sandbox_stop_server_value, ARTIFACT_PREFETCH_TIMEOUT, run_rocm_capture, update_supervised_record, WATCHER_TICK_INTERVAL, wait_for_port, gather_gpu_snapshot, print_json); a few moved tests drop their path prefixes and take their own imports; the Windows taskkill block that stays in service.rs has its process imports gated with cfg(windows) and its test module's copy with cfg(unix), and one import that only a Linux-only test uses is gated; comment pins that named the moved code now point at it (advised_commands.rs, two registry.rs comments, two rocm-core comments, including the rocm_install.rs one, which now names host_snapshot.rs); and docs/architecture.md gets an entry per new file, with the supervise_service description corrected (it said "spawn-and-recover" while the recovery lived in watchers.rs) and one sentence that became false dropped.

Verification

cargo fmt --check is clean, and cargo clippy -D warnings --all-targets is clean for rocmd, rocm-core, rocm-dash-daemon and rocm.
The leaf-name test lists are identical before and after (162 tests, 1 ignored), and every commit compiles without unused or dead-code warnings.
cargo xtask manifest --check and cargo xtask check-crate-edges pass, and the 20 advised_commands tests pass.
Not run locally: the full workspace tests, cargo test -p xtask, the tests/e2e-cucumber suite, the coverage floors and any Windows build (the Windows target cannot compile core here). The cfg(not(unix)) variants of detached_rocmd_command (now in supervise.rs) and shutdown_signal (now in daemon.rs) moved and are exercised only on the Windows lane; every cfg gate and import was read for the unix, windows and not-unix configurations.

Scope

This is an internal refactor with no user-observable change, so no end-to-end scenario is added; the unmodified tests/e2e-cucumber suite covers the unchanged behaviour. The eleven steps are bundled in one pull request on purpose, each as its own pure-move commit, so that the production module graph of rocmd becomes acyclic as one reviewable unit; the ticket itself only suggests that some of them could be combined, so this goes beyond that and can be split if the maintainers prefer. Not part of this PR, and tracked under ROCMAI-822: the artifact-prefetch move, which needs the atomic writer that PR #610 relocates and would otherwise create a cycle with sandbox; the conditional atomic-writer module; the optional mcp_argv.rs step; and the watchers/ directory split. Because the watchers/ split is deferred, watchers.rs shrinks (from about 3,840 to about 3,440 lines) but remains the largest module in the crate until that ticket lands. The no-op let _ = daemon_binary_path(); in supervise_service and its error wording are left as they were. Open PRs that edit the same files (#610, #811, #799, #817, #447 and the drafts #723 to #725) will need rebasing onto the new layout.

Template checklist: not a bug fix, so there are no stale xfail rows to narrow; no new subcommand or subsystem; no user-facing message is added or changed.

Move SandboxToolArg (with its Linux-only writes_data/writes_cache), SandboxToolPolicy and resolve_huggingface_token out of cli.rs into a new leaf module. sandbox.rs, service.rs, watchers.rs and the lib.rs tests now import it from the new module, and cli.rs imports it for its Command fields, so those modules no longer depend on cli.rs; no re-export is left behind in cli.rs. Part of the rocmd module-cycle follow-up work. Other than the moved lines, the module declaration and the import-path edits, the only change is the docs/architecture.md entry for the new file (and dropping the moved types from the cli.rs entry). No tests move: none target these types directly. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Move sandbox_stop_server_value out of service.rs and ARTIFACT_PREFETCH_TIMEOUT out of lib.rs into sandbox.rs, together with the two lib.rs tests that exercise the stop_server tool (sandbox_tool_stop_server_updates_manifest_and_skips_current_pid and sandbox_tool_stop_server_reports_an_unconfirmed_stop). run_sandbox_tool was the only production caller of the function, and it was the only thing that made service.rs depend on the sandbox-tool types; stop_report_confirmed stays in service.rs because mcp.rs also reads it. Part of the rocmd module-cycle follow-up work. Besides the moved lines and import wiring, this commit changes: sandbox_stop_server_value and ARTIFACT_PREFETCH_TIMEOUT become private (their only users are now in sandbox.rs); the two moved tests drop the cli::/sandbox::/watchers:: path prefixes now that they sit beside the code; lib.rs loses its emptied test module; docs/architecture.md gains stop_server in the sandbox.rs entry and drops the constant and test module from the lib.rs entry. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Move load_service_record, the single-manifest read, out of watchers.rs into persistence.rs next to load_managed_services. update_supervised_record in service.rs calls it, and watchers.rs calls update_supervised_record, so leaving it in watchers.rs would make the supervisor code that is about to leave service.rs and watchers.rs depend on each other. Production code in service.rs and watchers.rs, and the test imports in service.rs, mcp.rs, sandbox.rs and watchers.rs, now point at persistence.rs; persistence.rs still uses no other module of the crate. Part of the rocmd module-cycle follow-up work. Besides the moved lines and import wiring, the only other change is the docs/architecture.md entry for persistence.rs, which now names the function. No tests move: none target this function directly. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Move the code behind `rocmd supervise` out of service.rs into a new supervise.rs: parse_gpu_indices_arg, supervise_service, update_supervised_record, record_supervised_exit, engine_serve_http_args, last_cr_segment, classify_startup_phase, read_new_log_phase, engine_healthcheck_ready and wait_for_service_ready, with the tests that exercise only those symbols (the three key-guard tests and their helpers, the startup-phase and log-tail tests, the two engine_serve_http_args tests, the supervisor identity-token, first-write and exit-write tests). cli.rs and watchers.rs call the moved items at their new path. service.rs keeps the stop code and the daemon loop. Part of the rocmd module-cycle follow-up work. Besides the moved lines, module and import wiring, this commit changes: the Windows-only taskkill block that stays in service.rs keeps its process imports, now gated with cfg(windows) because nothing else in that file uses them on Linux, and the service.rs test module takes its own cfg(unix) copy for the tests that spawn helper processes; two comments in rocm-dash-daemon registry.rs that named a lib.rs path for supervise_service now name supervise.rs; docs/architecture.md gets an entry for the new file and a reworded service.rs entry. The doc comments on supervise_service and update_supervised_record that refer to a stop's re-read of the record were checked against the stop code left in service.rs and need no change. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Move restart_managed_service, record_spawned_supervisor, recovery_supervise_args and both cfg variants of detached_rocmd_command out of watchers.rs into supervise.rs, next to supervise_service, which is what recovery_supervise_args builds the arguments for. The six tests that exercise only these symbols move with them (recovery_supervise_args_preserve_engine_recipe_json, restart_managed_service_persists_the_supervisor_identity_token, restart_managed_service_drops_the_replaced_runs_engine_pid, restart_failure_write_keeps_a_stop_marker_set_meanwhile, restart_records_its_supervisor_without_erasing_a_stop_marker, restart_leaves_a_record_naming_another_supervisor_or_none; three are Linux-only). watchers.rs and sandbox.rs now import restart_managed_service from supervise.rs, so supervise.rs no longer depends on watchers.rs in production code. Part of the rocmd module-cycle follow-up work. Besides the moved lines and import wiring, this commit changes: update_supervised_record becomes private, since its only remaining callers are now inside supervise.rs; the imports that only the moved code used are dropped from watchers.rs, and its test module takes its own use std::fs; the supervise.rs test module imports UNCONFIRMED_STOP_PID only on Linux, since only a Linux-only test uses it; docs/architecture.md describes the recovery respawn in the supervise.rs entry. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Move run_daemon, print_status, build_runtime_state and both cfg variants of shutdown_signal out of service.rs, and WATCHER_TICK_INTERVAL out of lib.rs, into a new daemon.rs, together with the one test that exercises run_daemon on its own (local_webhook_requires_enabled_automation_loop). cli.rs calls the two entry points at their new path. service.rs now holds only the managed-service stop code, and with this commit the production modules of the crate no longer form an import cycle. Part of the rocmd module-cycle follow-up work. Besides the moved lines and import wiring, this commit changes: WATCHER_TICK_INTERVAL becomes a private constant of daemon.rs, its only user; the two path pins in apps/rocm/src/advised_commands.rs for the "rocmd automation supervisor started/stopped" log lines now name daemon.rs, where those lines live; docs/architecture.md gets an entry for the new file, rewords the service.rs entry to the stop code that remains, and drops the constant from the lib.rs entry. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Move payload_string, the trimmed non-empty string read from a JSON payload, out of watchers.rs into common.rs. Both watchers.rs and webhook.rs call it, so it belongs in the module the other modules share; webhook.rs now reaches it there and no longer imports watchers.rs. Part of the rocmd module-cycle follow-up work. Besides the moved lines and import wiring, the only other change is the docs/architecture.md common.rs entry, which now names the helper. No tests move: none target this function directly. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Move run_rocm_capture out of common.rs into mcp.rs, its only caller, and wait_for_port into watchers.rs beside endpoint_service_recovery_reason, its only caller; gather_gpu_snapshot, which only common.rs itself calls, becomes private. The shared run_rocm_capture_for_paths stays in common.rs because both mcp.rs and sandbox.rs use it. Part of the rocmd module-cycle follow-up work. Besides the moved lines and import wiring, this commit changes: wait_for_port becomes private in its new home and gather_gpu_snapshot is narrowed from pub(crate) to private; the imports only the moved code needed move with it (std::net, std::thread and format_host_port into watchers.rs, std::time::Duration into mcp.rs); docs/architecture.md drops the common.rs sentence about wait_for_port having a single caller, now false, and names the capture wrapper that remains there. No tests move: none target these functions directly. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…ipc.rs Move engine_request, engine_healthcheck_response, healthcheck_response_ready, healthcheck_response_recoverable, apply_endpoint_key_env and ensure_public_service_has_endpoint_key out of common.rs into a new engine_ipc.rs that uses no other module of the crate, together with the six tests that exercise only those symbols (recovery_refuses_a_service_that_lost_a_key_it_was_launched_with, recovery_respawn_fails_closed_for_a_public_service_without_a_key, apply_endpoint_key_env_sets_var_only_when_key_file_present, healthcheck_readiness_requires_ready_loaded_model, healthcheck_recoverability_tracks_failed_endpoint_state, healthcheck_readiness_withheld_while_the_model_only_lists). service.rs, supervise.rs and watchers.rs call them at the new path. Part of the rocmd module-cycle follow-up work. Besides the moved lines and import wiring, the only other change is docs/architecture.md: an entry for the new file, and the common.rs and supervise.rs entries adjusted to match. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Move AMD_SMI_PROBE_TIMEOUT, build_bridge_snapshot, gather_gpu_snapshot, gather_gpu_snapshot_for_config, capture_amd_smi_json, bridge_engine_inventory and rocmd_engine_inventory out of common.rs into a new host_snapshot.rs, with gpu_snapshot_respects_disabled_telemetry_policy. The new module uses only persistence.rs; cli.rs, mcp.rs and watchers.rs call it at the new path. Part of the rocmd module-cycle follow-up work. Besides the moved lines and import wiring, this commit changes: two comments in rocm-core that named an rocmd path now name host_snapshot.rs. In process.rs the old comment said both of the test's callers were in apps/rocmd/src/common.rs, which had already stopped being true when the rocm capture moved into rocm-core, so it now names the amd-smi probe in host_snapshot.rs and the rocm capture that common.rs still makes through run_rocm_capture_with_env. In rocm_install.rs the comment pointed at apps/rocmd/src/lib.rs for the daemon's serialised snapshot; the snapshot containing the examine summary is built in host_snapshot.rs, so it points there (the old path never held it). docs/architecture.md gets an entry for the new file and the common.rs entry is reworded to what remains. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Move print_json out of mcp.rs into cli.rs, where all four of its callers are. It becomes private, and cli.rs no longer reaches into mcp.rs for a printer. Part of the rocmd module-cycle follow-up work. Besides the moved lines and import wiring, the only other changes are that print_json is narrowed from pub(crate) to private and that the docs/architecture.md cli.rs entry names it. No tests move: none target this function directly. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Copilot

🟢 Approval recommended

The rocmd module refactor has no active review issues.

No open issues 🎉

What changed in this PR

Refactors rocmd into focused modules to remove production dependency cycles and separate daemon execution, service supervision, engine communication, and host snapshots while preserving its public API and existing behavior.

Changes:

Extracts five modules and moves shared helpers to their appropriate owners.
Updates callers, relocates existing tests, and narrows helper visibility and platform-specific imports.
Documents the new layout and updates source-path references.

File	Description
`docs/architecture.md`	Documents module responsibilities and dependencies.
`crates/rocm-dash-daemon/src/registry.rs`	Updates supervisor paths in comments only.
`crates/rocm-core/src/rocm_install.rs`	Updates the snapshot source path in documentation only.
`crates/rocm-core/src/process.rs`	Updates caller locations in test comments only.
`apps/rocmd/src/webhook.rs`	Uses the relocated shared payload helper.
`apps/rocmd/src/watchers.rs`	Delegates recovery, persistence, and snapshots; owns port probing.
`apps/rocmd/src/supervise.rs`	Houses supervision, readiness, recovery, and associated tests.
`apps/rocmd/src/service.rs`	Retains service-stop logic and scopes process imports by platform.
`apps/rocmd/src/sandbox.rs`	Owns stop-result shaping, its tests, and the prefetch timeout.
`apps/rocmd/src/sandbox_tool.rs`	Houses sandbox tool names and download policy types.
`apps/rocmd/src/persistence.rs`	Takes ownership of service-record loading.
`apps/rocmd/src/mcp.rs`	Uses extracted snapshots and owns its capture wrapper.
`apps/rocmd/src/lib.rs`	Reduces crate-root code to module declarations and public exports.
`apps/rocmd/src/host_snapshot.rs`	Houses bridge snapshots, GPU probes, and engine inventory.
`apps/rocmd/src/engine_ipc.rs`	Houses engine requests, health checks, and endpoint-key guards.
`apps/rocmd/src/daemon.rs`	Houses the daemon loop, status output, and shutdown handling.
`apps/rocmd/src/common.rs`	Retains shared utilities and receives payload parsing.
`apps/rocmd/src/cli.rs`	Dispatches to extracted modules and owns JSON printing.
`apps/rocm/src/advised_commands.rs`	Updates test exclusions to the daemon messages’ new location.

_{Review effort: Balanced}

juhovainio

The moves look behavior-preserving: every removed line reappears in the new modules apart from import/call-site paths, licence headers, comment pins and docs, and visibility only narrows. I found no functional defects. Requested changes:

PR body: state that there is no user-observable change, so no e2e scenario applies. Also say it delivers PR 1-10 and 13 of ROCMAI-756 only, with the remainder (11/12, 13b, 14-18) deferred to ROCMAI-822, without claiming the whole ticket.
Scope: this bundles 10 clusters while the epic rule is one cluster per PR. Please either split it or note the agreed exception in the PR body so reviewers know the bundling is intentional.
Nit: run_rocm_capture is still pub(crate) in mcp.rs with a single caller; narrow it to private.

Also, the E2E lanes and windows-build-and-test were still pending when I looked; they need to be green before merge.

nowycondro

🤖 Automated review (Claude Code) · 40e8737

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. Implements ROCMAI-756's steps PR1-10 and PR13 (the cycle-breaking moves and the misplaced-printer move); the remaining ROCMAI-756 scope items (PR11 atomic-writer, PR12 artifact-prefetch, PR13b mcp_argv, PR14-18 watchers/ directory split) are deferred to ROCMAI-822, which itself states it exists so that "ticket 756 and its pull request describe the same, complete work."

Blocking

docs/architecture.md's 11 new module citations use bare backtick paths instead of markdown links — docs/architecture.md:88-96 (new bullet entries for daemon.rs, engine_ipc.rs, host_snapshot.rs, sandbox_tool.rs, supervise.rs)
AGENTS.md (the repo's root agent-instruction file) requires citing a tracked repo file as a markdown link relative to the citing file, and explicitly says the rule "covers citations you add or edit." Every one of the 5 new bullets this PR adds cites sibling files as bare backtick paths even though the same files are linked elsewhere in the same list — e.g. line 89: "service.rs, supervise.rs and watchers.rs call it" should be [service.rs](../apps/rocmd/src/service.rs) etc. 11 such citations total (1 in the daemon.rs entry, 3 each in engine_ipc.rs/host_snapshot.rs/supervise.rs, 1 in sandbox_tool.rs), confirmed directly against the diff. Because lychee only validates links, not the absence of one, this slips past the "Markdown links resolve (lychee)" CI check entirely.
Confidence 85 · mechanical · Fix: wrap each bare file-name mention in these 5 new bullets in a markdown link relative to docs/architecture.md, matching the style already used elsewhere in the same entries.
The commit-8 "single-caller helpers are returned to their callers" claim is incomplete: run_rocm_capture was not actually moved — apps/rocmd/src/common.rs:215, apps/rocmd/src/mcp.rs:570,673,684,692,702,712,722,732,750,768
The PR description's step 8 and the named ticket's own scope table both list run_rocm_capture as a single-caller helper to be "returned to" mcp.rs (alongside wait_for_port→watchers.rs and gather_gpu_snapshot→private, both of which were done correctly). In the actual diff, run_rocm_capture is still defined in common.rs as pub(crate) fn run_rocm_capture(...), and every one of its 10 call sites is in mcp.rs (all via common::run_rocm_capture(...)) — confirmed by a repo-wide grep showing zero callers outside mcp.rs. The function was neither moved into mcp.rs nor narrowed from pub(crate) to private, unlike every other single-caller helper this PR touches.
Confidence 85 · mechanical · Fix: move run_rocm_capture into mcp.rs as a private fn (as done for the other single-caller helpers in this PR), or correct the description if it's being deliberately left for the follow-up.

Non-blocking

None

Decisions for the author

This PR bundles 10+ of the ticket's clusters into a single PR, commit-by-commit — tradeoff
ROCMAI-756 restates its epic rule as "One PR per cluster; a large cluster may take 2-3 PRs," and lays out the work as 13+ separate PRs. This PR instead delivers PR1-10 and PR13 as one PR with 11 commits, explicitly structured "so the branch can be reviewed commit by commit." That's a reasonable reviewability tradeoff for a mechanical, low-risk move, but it is a deviation from the ticket's stated process; confirm the deviation is an agreed exception rather than an oversight.
watchers.rs remains the single largest module in the crate after this split (3438 lines, barely down from 3843) — tradeoff
The PR's title frames this as splitting "the oversized service, common and watcher modules," but the watcher-specific split (into a watchers/ directory) is explicitly deferred to ROCMAI-822 and flagged there as only "medium confidence; confirm whether still wanted." Shipping now leaves the stated goal only partially met for watchers.rs specifically; this is disclosed, not hidden, so it's the author/maintainers' call whether to hold for the follow-up or land this incremental step.
"Completes ROCMAI-756" — answered-by-intent
ROCMAI-822 (also named in this PR's description): "Phase 8.1-D delivered its steps PR 1 to PR 10 and PR 13 (#831). This ticket owns the steps that were left out, so that ticket 756 and its pull request describe the same, complete work." Would otherwise have been blocking under "the change claims to close a ticket it does not close," since ROCMAI-756's own scope table lists the artifact-prefetch move (PR12) as in-scope without the "conditional"/"optional"/"medium confidence" hedges it gives the atomic-writer, mcp_argv, and watchers-split items — but ROCMAI-822's text confirms this was a deliberate, already-ticketed re-scoping, not an oversight. Confirm that framing is still current.

Positive signals

Every moved function and its tests were verified byte-for-byte identical to the pre-move source by independent passes over each file group — a genuinely clean mechanical move, not just a claimed one.
cargo xtask check-crate-edges (.github/workflows/ci.yml:1176) is a real, enforced CI gate, so the acyclicity this PR establishes won't silently regress in a future change.

Deployment notes

None.

What this covered

Full diff 11a60691...40e87375 (19 files, 2513/2445 +/-), read completely across 5 independent file-coupling groups (service/sandbox/persistence; supervise/watchers; engine_ipc/host_snapshot/mcp; common/daemon/cli/lib; docs-and-comment-only files), each cross-checked against the BASE source and repo-wide caller greps, plus a direct impact-analysis sweep for stale path/symbol references and an independent visibility/caller check on run_rocm_capture. Read ROCMAI-756 and ROCMAI-822 (both named in the PR description) by identifier, title+body+status only. CI checks at the reviewed commit: all completed checks (clippy, tests, coverage, CodeQL, lychee, commit-signatures, Linux and Windows build-and-test, several E2E hardware lanes) report success; no check reports failure; 3 hardware-gated E2E lanes (MI300X, Strix Halo Ubuntu release, W7900) were still queued/in-progress at review time.

Did not run as separate fan-out workers: the history and prior-changes source-reading passes (folded into the driver's own analysis given the small, mechanical nature of the change); no local cargo test/cargo clippy execution (relied on CI's reported results per the method). The mandatory claim-check fetch of this PR's reviews (required to check for a prior "nowycondro" review at this head) incidentally surfaced the body of an existing human review (juhovainio, CHANGES_REQUESTED, same commit) before this report was finalized; none of this report's findings were adopted from or anchored to that review's wording — each was independently re-derived from the diff, the ticket text, or direct source reads — but reconciliation against that review's specific requests was not performed here and is left to the human reviewers.

🤖 Generated with Claude Code

run_rocm_capture moved into mcp.rs with its only callers, so nothing outside the module needs crate-wide visibility any more. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

The five new module entries cited sibling files as bare backtick paths. Link each one relative to the doc so the docs-links check follows it when a file moves. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd · 2026-11-02T14:08:49Z

Thanks for the reviews. Pushed 5ceb77f on top of the branch (two new commits, no rewrite) and updated the PR description in place.

On the points from juhovainio's review (5736302589) that are settled:

The description now says the PR delivers steps PR 1 to PR 10 and PR 13 of the ticket only, with the remaining steps (PR 11 and 12, PR 13b, PR 14 to 18) tracked in the follow-up ticket, and no longer says it completes the ticket. The Scope section already states that this is an internal refactor with no user-observable change and so adds no e2e scenario; that stays as it was.
run_rocm_capture in mcp.rs is now private (ab57ea0), since all its callers are in that module. cargo clippy -p rocmd --all-targets -- -D warnings, cargo check --workspace --all-targets and the 19 mcp tests pass locally, and the leaf test-name list is still 162 tests with 1 ignored.

On the points from nowycondro's review (5736493495):

The docs/architecture.md finding is right: the five new rocmd entries named sibling files as bare backtick paths. All 11 are now markdown links relative to the doc (5ceb77f), and lychee --offline on the file reports no errors.
The run_rocm_capture finding does not hold at the reviewed head. Step 8 already moved the function out of common.rs: common.rs at 40e8737 holds only run_rocm_capture_for_paths, and the definition sits in mcp.rs next to its ten callers (the common.rs:215 and common::run_rocm_capture(...) references in the report match the base tree, not the PR head). What was left over was its pub(crate) visibility, which is narrowed as described above.
The report's "Completes ROCMAI-756" point is corrected in the description as described above.

fixed

juhovainio

I went through the whole diff line by line. The moves are verbatim: apart from import and path repoints, visibility and cfg lines, rustfmt re-wraps and comment pins, no logic line changed. Every test name appears exactly once removed and once added, the unix/non-unix/linux gates moved intact, and I found no production dependency cycle reintroduced. CI is green, including the Windows job.

Two non-blocking notes:

The title names ROCMAI-756, so merging will close it, but the remaining scope (artifact prefetch, atomic_write, mcp_argv, the watchers/ split) is deferred to ROCMAI-822. That is disclosed in the PR body; just confirm the auto-close is intended.
A few test-only cycle edges remain (supervise and service tests import from watchers), as the PR body already says.

This reflects a diff read plus CI results, not a local build.

nowycondro

Review — no blocking findings

Full review of the whole change. Implements ROCMAI-756's steps PR1-10 and PR13 (the cycle-breaking and cleanup moves); the remaining scope (PR11 atomic-writer, PR12 artifact-prefetch, PR13b mcp_argv, PR14-18 watchers/ directory split) is tracked under ROCMAI-822, confirmed by the ticket author's own comment on ROCMAI-756.

Blocking

None

Non-blocking

None

Decisions for the author

This PR bundles 10+ of ROCMAI-756's clusters (PR1-10, PR13) into one PR across 13 commits — tradeoff
The ticket's epic rule is "One PR per cluster; a large cluster may take 2-3 PRs," and its own PR sequence lists these as 10+ separate PRs. This PR delivers them together, disclosed in the body as structured "so the branch can be reviewed commit by commit." Reasonable for a mechanical, low-risk move; confirm the bundling is an agreed exception.
watchers.rs shrinks only ~11% (3843→3438 lines) despite the PR title naming "watcher modules" among what's being split — tradeoff
The bulk of its size — the per-watcher watchers/ directory split (~1550 lines, PR14-18) — is explicitly deferred to ROCMAI-822, where it is itself flagged "medium confidence; confirm whether still wanted." The PR body's own Scope section discloses this accurately; only the title reads a little broader than what lands here.
PR title names ROCMAI-756 — answered-by-intent
ROCMAI-756's own "Automation warning" says only the final PR of the ticket may name its key, since naming it otherwise auto-closes the ticket early. This PR is not the ticket's literal last listed PR (PR11-18 remain). That would otherwise be blocking under "claims to close a ticket it does not close" — but the ticket's own comment thread (same author, same day) states: "This ticket is delivered by #831 (steps PR 1 to PR 10 and PR 13). The steps it left out are now tracked as ROCMAI-822... ROCMAI-822 starts once that pull request has merged." This is a deliberate, already-documented re-scoping, not an oversight. Confirm that framing is still current.
No automated CI check protects the module-level acyclicity this PR establishes — non-blocking-improvement
cargo xtask check-crate-edges (xtask/src/crate_edges.rs), the only automated dependency-graph gate in CI, validates solely inter-crate Cargo dependency edges against a hardcoded allowlist (e.g. "rocmd must never depend on rocm") — its own doc comment says a literal cycle check would be a no-op since Cargo's resolver already rejects those. It does not parse or check intra-crate crate::/mod edges within apps/rocmd at all, which is the exact kind of cycle this PR removes. ROCMAI-756 itself says as much ("cargo xtask check-crate-edges (crate level only)") and names a manual process as the only safeguard against regression ("re-grep crate:: :: per module and compare with the target graph"). Since the whole value of this refactor is the new module-level DAG, consider having a follow-up add an xtask check for intra-crate module cycles in apps/rocmd, so this property is enforced by CI rather than by every future author remembering to re-grep by hand.

Positive signals

Every one of the ~10 moved clusters was verified line-for-line (or script-diffed) identical to its pre-move source across five independent file-coupling review passes, including the security-relevant endpoint-key guard logic in engine_ipc.rs (apply_endpoint_key_env/ensure_public_service_has_endpoint_key), which keeps both bail! branches intact.
Test-leaf-name count is exactly conserved across the moved file groups (e.g. 72 before/after for the service.rs/watchers.rs/supervise.rs split alone), with every cfg(windows)/cfg(unix)/cfg(target_os = "linux") gate carried over correctly, including on test modules.
Commit structure is exemplary: 13 small, independently buildable, DCO-signed commits, each named for exactly the cluster it moves, with no "wip"/"fix" noise.
docs/architecture.md's new/changed bullets were checked link-by-link and symbol-by-symbol against the head tree and are accurate; AGENTS.md's markdown-citation rule (relative links, not bare backtick paths, for files you add or edit a citation for) is fully satisfied at this head.

Deployment notes

None — internal refactor; lib.rs's public surface (parse_args, run_bin_cli, run_from_args) is confirmed byte-for-byte unchanged, and no configuration, CLI behavior, or user-facing message changes.

What this covered

Full diff 11a60691...5ceb77ff (19 files, +2513/-2445, 13 commits), read completely across five independent file-coupling worker passes (sandbox_tool/sandbox/service/persistence/webhook; supervise/watchers/service; engine_ipc/host_snapshot/mcp; common/daemon/cli/lib; docs/architecture.md plus four comment-only files), each worker reading every assigned file completely at both base and head and cross-checking callers repo-wide. I separately read AGENTS.md's citation rule and xtask/src/crate_edges.rs's actual scope directly from source rather than taking either on faith. Read ROCMAI-756 and ROCMAI-822 (both named in the PR description) by identifier — full title, body, status and comments. CI at the reviewed commit (5ceb77ff): all checks completed, zero failures, zero in-progress/queued, including the Windows build-and-test lane and all E2E lanes.

Did not run as a separate pass: git-blame/history deep-dive on the moved regions (verified instead by direct base/head diff per file, the stronger check here for a pure-move PR). A prior-review-aware "prior changes" pass was not performed: this PR's review history reached me unavoidably via the mandatory claim-check fetch rather than a deliberate search, and none of this report's findings were adopted from or anchored to that content's wording — each was independently re-derived from the diff, the two tickets, or direct source reads — but reconciliation against any prior review's specific wording was not attempted here. No local cargo test/cargo clippy/cargo xtask check-crate-edges execution — relied on CI's reported results.

jussielo-amd · 2026-11-02T16:21:11Z

Thanks for the approvals and the notes. On the points raised: the title naming ROCMAI-756 is intentional, because the remaining steps (artifact prefetch, the atomic-writer module, mcp argv and the watchers/ split) are tracked as ROCMAI-822, so that ticket and this pull request describe the same, complete piece of work. The remaining test-only import edges between supervise, service and watchers are as described in the body. On bundling: it is a deliberate choice by the author, with one pure-move commit per step so each can be reviewed on its own; the ticket only suggests that some steps could be combined, so I have said that plainly in the Scope section instead of calling it an agreed exception, and I am happy to split the pull request if you would rather. On watchers.rs: it shrinks only from about 3,840 to about 3,440 lines until the watchers/ split in ROCMAI-822, so I reworded the title to say that the service and common modules are split and the watcher code is moved out in part, and added that to the Scope section.

jussielo-amd added 11 commits November 2, 2026 11:31

jussielo-amd requested a balanced review from Copilot November 2, 2026 11:51

Copilot started reviewing on behalf of jussielo-amd November 2, 2026 11:52

View session

jussielo-amd changed the title ~~refactor(rocmd): untangle the module cycles and split the oversized service, common and watcher modules~~ ROCMAI-756: untangle the rocmd module cycles and split the oversized service, common and watcher modules Nov 2, 2026

Copilot AI reviewed Nov 2, 2026

View reviewed changes

jussielo-amd marked this pull request as ready for review November 2, 2026 11:57

jussielo-amd requested a review from a team as a code owner November 2, 2026 11:57

jussielo-amd requested a review from siloteemu November 2, 2026 11:57

juhovainio previously requested changes Nov 2, 2026

View reviewed changes

nowycondro reviewed Nov 2, 2026

View reviewed changes

jussielo-amd added 2 commits November 2, 2026 14:04

refactor(rocmd): make mcp.rs's rocm capture wrapper private

ab57ea0

run_rocm_capture moved into mcp.rs with its only callers, so nothing outside the module needs crate-wide visibility any more. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

docs(architecture): link the callers named in the new rocmd entries

5ceb77f

The five new module entries cited sibling files as bare backtick paths. Link each one relative to the doc so the docs-links check follows it when a file moves. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd requested a review from juhovainio November 2, 2026 14:13

juhovainio approved these changes Nov 2, 2026

View reviewed changes

nowycondro reviewed Nov 2, 2026

View reviewed changes

jussielo-amd changed the title ~~ROCMAI-756: untangle the rocmd module cycles and split the oversized service, common and watcher modules~~ ROCMAI-756: untangle the rocmd module cycles and move the service, common and part of the watcher code into focused modules Nov 2, 2026

jussielo-amd added this pull request to the merge queue Nov 2, 2026

Merged via the queue into ROCm:main with commit f52cc5b Nov 2, 2026
24 checks passed

jussielo-amd deleted the refactor-rocmd-cycles-and-splits branch November 2, 2026 16:45

Navigation Menu

Repository navigation

ROCMAI-756: untangle the rocmd module cycles and move the service, common and part of the watcher code into focused modules - #831

ROCMAI-756: untangle the rocmd module cycles and move the service, common and part of the watcher code into focused modules#831
jussielo-amd merged 13 commits into
ROCm:mainfrom
jussielo-amd:refactor-rocmd-cycles-and-splits

jussielo-amd commented Nov 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

juhovainio left a comment

Uh oh!

nowycondro left a comment

Uh oh!

jussielo-amd commented Nov 2, 2026

Uh oh!

juhovainio left a comment

Uh oh!

nowycondro left a comment

Uh oh!

jussielo-amd commented Nov 2, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jussielo-amd commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changes

Verification

Scope

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

juhovainio left a comment

Choose a reason for hiding this comment

Uh oh!

nowycondro left a comment

Choose a reason for hiding this comment

Review — needs work

Blocking

Non-blocking

Decisions for the author

Positive signals

Deployment notes

What this covered

Uh oh!

jussielo-amd commented Nov 2, 2026

Uh oh!

juhovainio left a comment

Choose a reason for hiding this comment

Uh oh!

nowycondro left a comment

Choose a reason for hiding this comment

Review — no blocking findings

Blocking

Non-blocking

Decisions for the author

Positive signals

Deployment notes

What this covered

Uh oh!

jussielo-amd commented Nov 2, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jussielo-amd commented Nov 2, 2026 •

edited

Loading