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

[jussielo-amd]

Summary

First PR of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27): extracts record_event/load_managed_services out of apps/rocmd/src/lib.rs's flat 9,980-line body into a new persistence.rs module. These two helpers (automation-event/audit-log writes and managed-service-registry reads) are reached into by every other cluster slated for later PRs in this phase (sandbox, mcp, service, watchers), so this lands first per the ticket's explicit sequencing.

• Pure code motion, no behavior change.
• Call sites repointed to persistence::record_event/persistence::load_managed_services.
• The one test that exercises record_event directly moved into persistence.rs's own #[cfg(test)] mod tests in this same PR (per the epic's rule against deferring test-splitting).
• docs/architecture.md updated in this PR to reflect progress on this phase.

Test plan

• cargo build (rocmd, and rocm+rocmd together to confirm the external API surface — rocmd::run_bin_cli/rocmd::run_from_args — 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

🟢 Approval recommended

The extraction preserves implementation and test coverage while consistently updating all references.

Review effort: Balanced
Findings: None

What changed in this PR

Extracts daemon persistence helpers from lib.rs into a focused module without changing behavior.

Changes:

• Moves event recording and managed-service loading into persistence.rs.
• Updates all call sites and relocates the associated unit test.
• Documents the modularization progress.

File	Description
apps/​rocmd/​src/​lib.rs	Registers and uses the persistence module.
apps/​rocmd/​src/​persistence.rs	Contains persistence helpers and their test.
docs/​architecture.md	Records current and planned modularization.

---

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

[r0x0r]

Review: clean mechanical relocation — approving

Verified as pure code motion. record_event and load_managed_services are byte-for-byte identical to their originals at lib.rs:5110 and lib.rs:5224 on main — no changed error handling, no dropped .context(...), no reordered statements, no altered literals.

What I checked

• Call sites — all 42 previously-bare calls are now persistence::-qualified; no unqualified call survives in lib.rs.
• Imports — append_automation_event is correctly dropped from lib.rs (its only non-test user moved), and AutomationEventRecord is correctly re-gated behind #[cfg(test)] since its only remaining uses in lib.rs are in tests. Both matter: the workspace gates on clippy -- -D warnings, where an unused import fails the build. I re-checked every dropped import against the remaining body and found no symbol left referenced without one.
• Visibility — pub(crate) on both items, which is exactly what the cross-module calls need and no wider.
• Tests — the test attribute count is conserved at 130 across the branch (129 in lib.rs + 1 in persistence.rs), matching the PR's claim precisely. record_event_mirrors_watcher_actions_to_audit_log moved intact.
• Docs — the docs/architecture.md text accurately describes this branch: persistence.rs is the only module present, and it is the only one claimed as extracted. (This is worth calling out because the sibling PRs in this phase do not have that property — see below.)
• Blast radius — nothing outside apps/rocmd references either symbol. apps/rocm/src/main.rs has its own, different load_managed_services; it is untouched. The rocm-dash-daemon mention is a prose comment with no code dependency.
• Leak scan (AGENTS.md §2) — clean.

Non-blocking: the test-helper duplication will compound

temp_app_paths, unique_test_root and workspace_test_artifact_dir are now byte-identical copies in both persistence.rs's and lib.rs's test modules. That is a reasonable call for one module, and it is unavoidable without a shared helper module since each #[cfg(test)] mod tests is a separate private scope.

The reason to raise it here rather than later: the same three helpers are also duplicated into common.rs on #479 and into sandbox.rs on #483. Phase 5 has seven target modules, so on the current trajectory this ends at roughly eight copies of workspace_test_artifact_dir, each independently able to drift on the next change to the artifact path. A #[cfg(test)] mod test_support; at the crate root, introduced once, would cost one small PR now and save seven divergence risks later. Worth deciding before the remaining cluster PRs land, not after.

Note on merge order for this phase

This PR is the only one of the five whose architecture-doc hunk matches its own branch. #479, #480, #481 and #483 each rewrite the same single line of docs/architecture.md with a cumulative list that assumes everything earlier in the sequence has already merged. Since all five are branched independently from main, that line is a guaranteed conflict for whichever four merge later — and, more importantly, any of them merging before this one would apply cleanly while planting a description of files that do not exist on main. Landing this PR first is what makes the rest of the sequence coherent.

---

🤖 by agent-hub on AMD AgentHub

[r0x0r]

Phase 5 merge-order note — posting here because this is the PR that lands first

My approval above stands: this PR is clean on its own. Since then the rest of Phase 5 has opened (#479, #480, #481, #483, #484, #487, #489) and I've reviewed all eight. Two problems only show up when you look across them, so I'm recording them here rather than repeating them on each.

1. The eight PRs do not compile once merged, and nothing conflicts to warn you

Each new module reaches back into the crate root via crate:: for items that are still in lib.rs on its own branch. That is correct against main today. But several of those items are exactly what a sibling PR moves somewhere else, so the path goes dangling the moment that sibling lands.

Checking every crate:: reference in all eight new module files against what each sibling relocates, 52 break:

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

The reason this is worth raising loudly is where the breakage sits. Take the clearest case: #489 moves payload_string into watchers.rs and correctly updates its own call sites, while #480's webhook.rs calls crate::payload_string at lines 164 and 168. The lib.rs side does conflict — #480 edits that function's line, #489 deletes it — so someone will be asked to resolve it. But the two broken call sites are in webhook.rs, which #489 never opens. Resolve the lib.rs conflict the obvious way and you get a tree with no conflict markers that doesn't build.

Every one of the 52 has that shape: a dangling reference in a file the relocating PR never touches.

Nothing is wrong with the crate::-reach-back technique — it is the right call for one PR against main. It just doesn't survive eight of them sharing a base. Either:

1. Stack them so each branches off its predecessor and sees the real location of everything already moved. AGENTS.md §11 already prescribes this shape — keep stacked PRs in draft until dependencies merge upstream, then rebase and mark ready.
2. Keep them independent and rebase serially — after each lands on main, rebase the next and repoint its crate:: paths before merging.

2. Three symbols are claimed by two PRs each

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

These aren't dangling-path problems — both definitions would exist, one unused, which the workspace's -D warnings gate fails on, and the call sites each PR repointed disagree about which is canonical.

CommandCapture has a right answer by the phase's own admission rule for common.rs ("used by ≥2 of the sandbox/mcp/service/watchers clusters"). On main it has exactly two consumer groups — sandbox (sandbox_check_updates_value, update_check_status, sandbox_driver_plan_value) and MCP (tool_result_from_command, command_capture_text, run_rocm_capture) — so common.rs is correct and #484's placement would give sandbox.rs a dependency on mcp.rs. Written up in full on #484.

3. The architecture-doc line, across all eight

Every PR rewrites the same single line of docs/architecture.md with a cumulative list that is only true at merge order 477 → 479 → 480 → 481 → 483 → 484 → 487 → 489. Out of order, a PR applies cleanly and leaves main describing files that aren't there. For the record this is not pre-existing drift: main currently reads `lib.rs` is **not yet modularized** — see EAI-7768 at line 34. Scoping each hunk to its own module makes every PR correct at any merge order, and turns eight whole-line rewrites into eight appends.

Where that leaves the eight

Reviewed clean on their own merits, blocked only on the above: #479, #480, #481, #483, #484, #487, #489. This one (#477) is approved and is the right thing to land first — it is the only one whose doc hunk already matches its own branch, and the only one with no broken crate:: references in either direction.

---

🤖 by agent-hub on AMD AgentHub

[jussielo-amd]

Re test-helper duplication (r0x0r's note on the approved review): fixed. temp_app_paths/unique_test_root/workspace_test_artifact_dir now live in a single shared apps/rocmd/src/test_support.rs (#[cfg(test)] mod test_support; declared in lib.rs), and both lib.rs's and persistence.rs's test modules import from there instead of keeping their own copies. 130 tests still pass (129 + 1 pre-existing ignored), clippy and fmt clean.

Sibling PRs in this phase can adopt crate::test_support::{temp_app_paths, unique_test_root, workspace_test_artifact_dir} as they rebase onto this stack, rather than re-adding their own copies.

[jussielo-amd]

Re the Phase 5 merge-order note: confirmed plan is to stack the remaining seven PRs rather than keep them independent. #477 merges first, independently, staying based on `main` (no base change needed here — it already has no broken `crate::` references and its `docs/architecture.md` hunk already matches its own branch, as you noted).

#479 → #480 → #481 → #483 → #484 → #487 → #489 will each be restacked as drafts dependent on their predecessor per AGENTS.md §11 (kept in draft until the predecessor merges upstream, then rebased and marked ready). Rebasing each onto its already-merged/already-fixed predecessor resolves both issues you flagged mechanically: the dangling `crate::` references see the real post-move location of everything already relocated, and the cumulative `docs/architecture.md` line conflicts/resolves naturally one hunk at a time instead of needing the line scoped to an append per-module.

Will flag the doubly-claimed-symbol conflicts (`CommandCapture`, `run_command_with_timeout`, `update_check_message`) for resolution on the specific PRs that introduce them (#479/#483/#484) as each is restacked.

[jussielo-amd]

Following up on the #479-specific finding above (common.rs's crate::load_managed_services going dangling once this PR's persistence.rs lands): #479 (rocmai-83-common) is now rebased on top of this branch (rocmai-83-persistence @ a6b6e35) and the reference has been repointed to crate::persistence::load_managed_services in commit 195678b. It also now uses crate::test_support::temp_app_paths instead of its own duplicate copy of the test helpers.

#479's base is conceptually this branch now — GitHub doesn't let a fork-based PR's base ref point at a branch that only exists on the fork, so its base field still shows main, but the branch itself is rebased on these commits and the PR is back in draft until this one merges.