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

[jussielo-amd]

Summary

Eighth and final PR of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27). Pulls event collection/dispatch for all built-in watchers (TheRock update, GPU metrics/thermal-pressure, cache-warm, driver-upgrade, server-recover), managed-service recovery classification, and automation-proposal queuing into their own watchers.rs module.

• Non-contiguous in the source: most of the cluster is one block, but queue_proposal/queue_proposal_with_arguments/proposal_tool_for_action/proposal_arguments_for_action sit sandwiched between record_event and load_managed_services (persistence.rs territory, stays in lib.rs on this branch), and detached_rocmd_command sits at the tail of the file right before the test module. All three sub-blocks moved in this PR. Watcher-only consts (SERVER_RECOVER_BACKOFF_MS, SERVER_TRANSIENT_STALE_MS, ENDPOINT_HEALTH_TIMEOUT, THEROCK_UPDATE_INTERVAL_MS, GPU_METRICS_INTERVAL_MS, and the GPU thermal/VRAM pressure thresholds) moved with it, since nothing else in lib.rs used them.
• No behavior change. Reaches back into still-crate-root items (record_event/load_managed_services, run_sandbox_tool + SandboxToolArg/SandboxToolPolicy, update_check_message/record_notification_audit, the engine-healthcheck/endpoint-key/wait_for_port/optional_arg/gather_gpu_snapshot_for_config family) via crate::, since the modules that will eventually own those (persistence.rs/sandbox.rs/cli.rs/common.rs) are separate, independent PRs not present on this branch (see ROCMAI-83: extract persistence.rs from apps/rocmd/src/lib.rs #477, ROCMAI-83: extract common.rs from apps/rocmd/src/lib.rs #479, ROCMAI-83: extract webhook.rs from apps/rocmd/src/lib.rs #480, ROCMAI-83: extract cli.rs from apps/rocmd/src/lib.rs #481, ROCMAI-83: extract sandbox.rs from apps/rocmd/src/lib.rs #483, ROCMAI-83: extract mcp.rs from apps/rocmd/src/lib.rs #484, ROCMAI-83: extract service.rs from apps/rocmd/src/lib.rs #487).
• 34 tests that exercise this module's own logic (event-collector/gpu/cache-warm/driver-upgrade/server-recover dispatch, therock-update handling, watcher-policy mode mapping, recovery-reason classification) moved into watchers.rs's own #[cfg(test)] mod in this same PR. Tests that are fundamentally about Cli parsing, run_daemon, sandbox-tool dispatch shape, or the persistence-layer record_event itself stayed in lib.rs for their own PRs.
• docs/architecture.md updated in this PR — this completes Phase 5: lib.rs is now top-level glue (module declarations and the two externally-consumed entry points re-exported from cli.rs).

Independent of #477, #479, #480, #481, #483, #484, #487 — 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)

[r0x0r]

Review (draft): code motion is clean; this PR exposes a phase-wide merge problem

The extraction itself is good — I audited every moved constant's value and the watcher decision logic, and found no behavior change. But reviewing this one alongside its seven siblings surfaced something none of them shows on its own, and it is the reason I'd hold the whole phase rather than this PR specifically.

payload_string silently breaks #480

This PR moves payload_string out of the crate root into watchers.rs:1101 and correctly repoints its own two call sites in lib.rs to watchers::payload_string.

#480 adds webhook.rs, which calls crate::payload_string at webhook.rs:164 and :168, and keeps the function at the crate root (lib.rs:4516, promoted to pub(crate) for exactly that purpose).

After both merge, crate::payload_string does not exist and rocmd does not compile.

What makes this one dangerous rather than merely annoying is where the breakage lives. The lib.rs side will conflict — #480 edits the payload_string line while this PR deletes it, which git reports as an edit/delete. But the two broken call sites are inside webhook.rs, a file this PR never touches. Resolving the lib.rs conflict by taking this PR's deletion produces a tree with no conflict markers, no merge warning, and a compile error two files away.

This is systemic, not a one-off

I checked every crate::-qualified reach-back in all eight new module files against the symbols each sibling moves out of lib.rs. 52 of them break on merge, across six of the eight PRs:

PR	module	crate:: refs broken by a sibling
#489	watchers.rs	18 (incl. CommandCapture, SandboxToolArg/Policy, run_sandbox_tool, record_event)
#487	service.rs	13 (incl. evaluate_watchers, start_local_webhook_source, load_service_record)
#481	cli.rs	11 (incl. run_daemon, supervise_service, run_mcp_server, handle_mcp_tool_call)
#484	mcp.rs	5 (incl. build_bridge_snapshot, stop_managed_service)
#480	webhook.rs	1 (payload_string)
#479	common.rs	1 (load_managed_services)

Every one has the same shape as the payload_string case: the dangling reference sits in a new file that the PR relocating the symbol never opens, so there is nothing for git to conflict on.

The crate::-reach-back design is sound for one PR against main — the problem is only that eight of them are in flight against the same base. Two ways out:

1. Stack them. Each PR branches off its predecessor, so each sees the real location of everything already moved and crate:: paths are correct by construction. AGENTS.md §11 already prescribes this ("keep stacked PRs in draft until dependencies merge upstream, then rebase and move out of draft").
2. Keep them independent but rebase serially — after each lands on main, rebase the next and repoint its crate:: paths before merging.

Either works; what does not is merging them as-is in any order.

Three symbols are claimed by two PRs each

Separately from the dangling references, these are defined in two different modules by two open PRs:

symbol
CommandCapture	#479 common.rs:204	#484 mcp.rs:846
run_command_with_timeout	#479 common.rs	#484 mcp.rs
update_check_message	#479 common.rs:64	#483 sandbox.rs:848

If two of these land, the symbol exists in both modules — the copies compile, but one becomes dead code, which the workspace's -D warnings gate treats as a failure, and the call sites each PR repointed differently disagree about which one is canonical. CommandCapture has a correct answer by the phase's own rule (two consumer clusters → common.rs); I've written that up on #484.

This branch does not complete Phase 5

The PR body and the doc hunk both say "this completes Phase 5: lib.rs is now top-level glue (module declarations and the two externally-consumed entry points re-exported from cli.rs)." On this branch:

• lib.rs is 6,903 lines (down from 9,980 — so ~31% of the file moved, not all of it). It still contains the CLI definition, sandbox tooling, the MCP server, service lifecycle and GPU snapshot collection.
• There is no cli.rs here, so nothing is re-exported from it; run_bin_cli and run_from_args are still defined directly in lib.rs at lines 250 and 255.
• The doc lists all eight modules as extracted; apps/rocmd/src/ contains lib.rs, main.rs, watchers.rs.

That claim is true of the phase, not of this PR. Being the last one, it carries the largest such gap — and for the record, main's current text is plainly `lib.rs` is **not yet modularized** — see EAI-7768 (line 34), so none of this is inherited from an already-drifted doc.

Verified clean

• Every moved constant's value is unchanged — I checked these individually because a changed numeric literal here silently alters when the daemon fires recovery or declares thermal pressure, and no test-count check would catch it:

  SERVER_RECOVER_BACKOFF_MS 30_000 · SERVER_TRANSIENT_STALE_MS 5 * 60 * 1_000 · ENDPOINT_HEALTH_TIMEOUT Duration::from_millis(250) · THEROCK_UPDATE_INTERVAL_MS 6 * 60 * 60 * 1000 · GPU_METRICS_INTERVAL_MS 60 * 1000 · GPU_THERMAL_HOTSPOT_PRESSURE_C 95.0 · GPU_THERMAL_MEMORY_PRESSURE_C 95.0 · GPU_MEMORY_VRAM_PRESSURE_PERCENT 95.0

  The "nothing else in lib.rs used them" claim also holds — zero remaining references on this branch.

• GPU pressure evaluation — all three comparisons are >= before and after; boundary behavior unchanged, and no CPU fallback introduced (AGENTS.md §6).

• Recovery classification — service_recovery_event_kind's arms and prefixes (healthcheck_status_, endpoint_status_, fallback) unchanged in content and order.

• Proposal mappings — all five arms of proposal_tool_for_action / proposal_arguments_for_action identical, same tools and same JSON shapes.

• The non-contiguous move is correct — all three sub-blocks landed, nothing between them was swept in, and record_event / load_managed_services correctly did not move (they remain at lib.rs:3640 and :3683 for #477). Given the middle sub-block sits sandwiched between those two functions, that is the easy mistake here and it was not made.

• Tests — 130 attributes conserved (96 + 34), matching the claimed 34 exactly.

• Imports/visibility — all crate:: targets resolve on this branch; no unused imports left in lib.rs; the rocm_core items that became test-only are correctly #[cfg(test)]-scoped.

• Blast radius — nothing outside apps/rocmd/src/ references any moved symbol.

---

🤖 by agent-hub on AMD AgentHub

[jussielo-amd]

Thanks for the thorough audit — replying to the phase-wide concerns from this review (#5380436537), both of which are resolved by how the stack has landed:

crate:: reach-backs / merge-order problem. Per AGENTS.md §11, all eight Phase 5 PRs are now stacked in merge order (#477 → #479 → #480 → #481 → #483 → #484 → #487 → #489), rather than independently branched off the same pre-stack main. This PR's base is now rocmai-83-service (#487), and I rebased watchers.rs onto it: all ~18 crate:: reach-backs that the pre-stack diff left dangling (CommandCapture, SandboxToolArg/SandboxToolPolicy, run_sandbox_tool, record_event, load_managed_services, update_check_message, wait_for_port, payload_string, etc.) now point at their real homes — common::, cli::, sandbox::, persistence::, webhook:: — verified against each module's actual current content rather than assumed from the original diff. I also repointed the stale bare crate:: calls the review's table doesn't cover because they live in this PR's own direction: service.rs's evaluate_watchers/evaluate_watchers_for_events/reconcile_watcher_snapshots/load_service_record and webhook.rs's payload_string now call crate::watchers::, and sandbox.rs's restart_managed_service import now comes from crate::watchers. cargo check/clippy -D warnings/fmt --check/test are all green on the rebased branch (129 passed, 1 pre-existing ignore).

On the duplicate-ownership front: CommandCapture/update_check_message are resolved in common.rs's favor per the note on #484 — watchers.rs imports both from crate::common rather than redefining them, and I did the same systematic check for every symbol this PR moves (no other duplicates found). Also deduped the test-only temp_app_paths/unique_test_root/workspace_test_artifact_dir trio in watchers.rs's test module in favor of the shared crate::test_support.

"This completes Phase 5" / doc overclaim. Correct that it wasn't true against this PR's pre-stack diff alone — fixed now that the base is #487: lib.rs on this branch is 94 lines of module declarations, two re-exported entry points, two shared consts, and one surviving test; all seven sibling modules (persistence.rs, common.rs, webhook.rs, cli.rs, sandbox.rs, mcp.rs, service.rs) plus watchers.rs itself exist. docs/architecture.md's hunk now lists all eight as extracted with no "still pending" clause, which is genuinely accurate once the stack lands in this order.

PR is back in draft with base rocmai-83-service, matching the rest of the stack, and will come out of draft once #487 and its predecessors merge.