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

[jussielo-amd]

Summary

Fifth PR of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27). Pulls bubblewrap/native sandbox execution, the atomic-write helper family, artifact prefetch policy gating, and the check_updates/driver_plan sandbox-tool result shaping into their own sandbox.rs module.

• No behavior change. Reaches back into still-crate-root items (CommandCapture, SandboxToolArg/SandboxToolPolicy, load_managed_services, restart_managed_service, stop_managed_service, run_rocm_capture_for_paths) via crate::, since the modules that will eventually own those (common.rs/cli.rs/persistence.rs/watchers.rs/mcp.rs/service.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).
• 37 tests that exercise this module's own logic (atomic-write semantics, sandbox-tool dispatch and output shaping, prefetch policy gating, huggingface-URL classification) moved into sandbox.rs's own #[cfg(test)] mod in this same PR.
• A handful of interleaved tests that are really about the managed-service-registry stop/restart lifecycle (service.rs), or that use this module's sandbox_check_updates_value/sandbox_driver_plan_value purely as mock watcher-event data, stayed in lib.rs to move with service.rs/watchers.rs in their own later PRs.
• docs/architecture.md updated in this PR.

Independent of #477, #479, #480, #481 — 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): the sandbox isolation is intact; one doc line to fix

Reviewed as a draft, so this is a comment rather than an approval — mark it ready and I'll re-review. The substance is in good shape: I treated the bubblewrap and policy-gate code as the real risk in this diff and checked it flag by flag rather than relying on the test count.

Security-critical motion: verified identical

This is the part of Phase 5 where a reordered line would be a sandbox escape rather than a compile error, so stating the result explicitly:

• Bubblewrap argument construction matches lib.rs:467–510 on main exactly, flags and order both: --die-with-parent, --new-session, --unshare-ipc, --unshare-uts, --proc /proc, --dev /dev, --tmpfs /tmp, --tmpfs /run, then the conditional --unshare-net. The network-unshare condition if !(matches!(tool, SandboxToolArg::PrefetchArtifact) && policy.allow_artifact_download) is unchanged, as is the bind sequence (ro-binds for system dirs / exe_dir / config_dir, conditional bind vs ro-bind for data_dir and cache_dir).
• Native fallback — the allow_native_fallback gate is unchanged; no new path skips the sandbox.
• Prefetch policy gate — allow_artifact_download, artifact_max_bytes and allow_huggingface_download are byte-identical, and no default flipped from deny to allow. huggingface_token is still only ever passed as a bearer header; it is not logged, not folded into an error message, and not reachable through captured stdout/stderr. resolve_huggingface_token correctly stays in lib.rs with SandboxToolPolicy::from_cli.
• HuggingFace URL classification — is_huggingface_url and http_url_host unchanged, including the huggingface.co / .huggingface.co / hf.co / .hf.co host matching.
• Atomic writes — write_file_atomically_with_publish preserves its ordering exactly: reserve temp → write_all → drop(file) → before_publish() → publish(&tmp, path).inspect_err(|_| fs::remove_file(&tmp)). Cleanup on both write failure and publish failure survives, and the publish_temp_file / replace_file_windows split is identical.

The doc hunk describes a state this branch is not in

docs/architecture.md now claims five extracted modules; this branch has one. persistence.rs, common.rs, webhook.rs and cli.rs are all absent, and the items they are documented as owning — load_managed_services, CommandCapture, SandboxToolArg/SandboxToolPolicy — are still in lib.rs here.

Across the phase this is a pattern rather than an isolated slip, and the cross-PR shape is not visible from inside any single one:

• All five PRs rewrite the same single line, each branched independently from main, so four are guaranteed to conflict on it.
• Each text is a cumulative prefix that is only true at merge order 477 → 479 → 480 → 481 → 483.
• The quiet failure is a PR other than #477 merging first: nothing to conflict with, applies cleanly, and main ends up asserting files that do not exist. Being last in the sequence, this PR carries the largest such claim.

Scoping the hunk to sandbox.rs alone and letting each sibling append its own module as it lands fixes it at any merge order and reduces the conflicts to trivial appends. Suggested replacement for this branch:

lib.rs modularization is in progress (ROCMAI-83, Phase 5 of EAI-7768's sequencing). Extracted so far: sandbox.rs (bubblewrap/native sandbox execution, atomic-write helpers, artifact prefetch policy gating, and the sandbox-tool result shaping for check_updates/driver_plan).

Verified clean

• Visibility of the seven crate-root back-references — CommandCapture (including its private fields argv/exit_status/stdout/stderr), SandboxToolArg, SandboxToolPolicy, ARTIFACT_PREFETCH_TIMEOUT, load_managed_services, restart_managed_service, stop_managed_service and run_rocm_capture_for_paths are all private in lib.rs, which is fine: mod sandbox; makes this a direct child of the crate root, and private items are visible to descendant modules. Both the production reads in sandbox_check_updates_value/sandbox_driver_plan_value and the test struct literals work.
• Call sites — all six pub(crate) functions in sandbox.rs are called sandbox::-qualified from lib.rs; none missed.
• Imports — sha2::{Digest, Sha256} correctly dropped from lib.rs (no reference survives) and re-added to sandbox.rs under #[cfg(test)], consistent with sha256_hex itself being test-only. OsStr correctly followed temp_sibling_path across. Nothing fails under -D warnings.
• Tests — 130 attributes conserved across the branch (93 + 37), matching the claimed 37 exactly, and write_file_atomically_cleans_up_temp_on_write_failure kept its #[ignore = "fills /dev/shm to provoke ENOSPC; not safe to run concurrently"]. That attribute surviving a 16.9k-line move is easy to lose and would have turned a deliberately-skipped destructive test into one that runs in CI. The tests left in lib.rs do use sandbox_check_updates_value/sandbox_driver_plan_value only as mock runner results feeding handle_therock_update_event_with_runner, so the split rationale holds.
• No TODOs, commented-out code or half-moved clusters — nothing in the diff looks unfinished despite the draft status.
• Blast radius — nothing outside apps/rocmd/src/ references the moved symbols; no e2e scenario touches them.
• Leak scan (AGENTS.md §2) — the one hit is the pre-existing phrase "restricted internal tool API" in an error string, moved verbatim. Clean.

Non-blocking: test-helper duplication, third copy

temp_app_paths, unique_test_root and workspace_test_artifact_dir are duplicated byte-identically into sandbox.rs while remaining in lib.rs. This is the third copy in the phase (#477 and #479 make the others) and Phase 5 has seven target modules. A single #[cfg(test)] mod test_support; at the crate root, added once, would stop this at three rather than eight — each copy being independently able to drift the next time the artifact path changes.

---

🤖 by agent-hub on AMD AgentHub

[jussielo-amd]

Thanks for the thorough review. Both points addressed in e92b913 (rebased onto #481 / rocmai-83-cli per AGENTS.md §11 stacking):

• docs/architecture.md hunk: resolved by stacking. The base is now rocmai-83-cli, which already carries the correct cumulative doc text for persistence.rs/common.rs/webhook.rs/cli.rs; this PR's diff is now just the incremental append of sandbox.rs to that list, so it's correct at this merge position regardless of whether a sibling lands first.
• Test-helper duplication: fixed using the shared test_support module ROCMAI-83: extract persistence.rs from apps/rocmd/src/lib.rs #477 introduced. sandbox.rs's test module now does use crate::test_support::temp_app_paths; instead of keeping its own copies of temp_app_paths/unique_test_root/workspace_test_artifact_dir.

Since the rebase lands this on top of common.rs/cli.rs/persistence.rs, it also picked up a few things only visible once those modules exist: the seven crate-root back-references now resolve to their real owners (common::CommandCapture, cli::SandboxToolArg/SandboxToolPolicy, persistence::load_managed_services); update_check_message turned out to have been independently duplicated into both common.rs and this PR's sandbox.rs, so sandbox.rs now calls common's copy instead of keeping its own; and cli.rs's two sandbox-dispatch call sites were updated to the sandbox:: path. Still a draft stacked on rocmai-83-cli — ready once that merges.