fix(snapshot): preflight sandbox liveness and clean restore

[ericksoa]

Summary

Two fixes for snapshot restore:

1. Preflight sandbox liveness before restore — restoreSandboxState() returns success immediately when no state dirs are backed up, so a restore against a stopped or missing sandbox could silently report success. Now checks the sandbox is live before attempting restore, matching the existing snapshot create preflight.

2. Clean target dirs before extracting to remove stale files — restoreSandboxState() used tar -xf overlay semantics which only overwrites files present in the archive. Files added after a snapshot was taken would persist when restoring that earlier snapshot. Now removes the target state directories inside the sandbox before extracting so the restored state is an exact replica.

Closes #1902

Test plan

• All hooks pass (pre-commit, commit-msg, pre-push)
• Nightly E2E triggered on this branch — validates Phase 7 (restore earlier snapshot, assert later files are gone)

Signed-off-by: Aaron Erickson aerickson@nvidia.com

[coderabbitai]

📝 Walkthrough

Walkthrough

The restore flow now validates the target OpenShell sandbox is present/running before proceeding, and the restore operation performs a remote pre-cleanup over SSH (removing stale writable dirs) prior to extracting the snapshot tar stream.

Changes

Cohort / File(s)	Summary
CLI pre-validation src/nemoclaw.ts	Added live pre-check: runs openshell sandbox list (ignoring stderr), aborts with exit code 1 if the list command fails, parses live sandbox names and aborts if target sandboxName is not present/running; existing timestamp selection and restore call unchanged.
Remote pre-cleanup & restore src/lib/sandbox-state.ts	Added remote cleanup step before extraction: for each backed-up stateDirs entry, runs rm -rf "<writableDir>/<dir>" over SSH (joined with &&), logs the command, treats non-zero exit as a warning (continues), then proceeds with existing local tar stream -> SSH tar -xf extraction flow; changes restore semantics to remove stale target contents.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant Nemoclaw as Nemoclaw CLI
  participant OpenShell as OpenShell (sandbox list)
  participant Local as Local FS / Snapshot
  participant Remote as Target Sandbox (SSH)

  User->>Nemoclaw: run `sandboxSnapshot restore <sandboxName> ...`
  Nemoclaw->>OpenShell: `openshell sandbox list`
  OpenShell-->>Nemoclaw: sandbox list (or error)
  alt list failed or sandbox not found
    Nemoclaw-->>User: exit 1 (abort)
  else sandbox present
    Nemoclaw->>Local: select snapshot / create tar stream
    Nemoclaw->>Remote: SSH run `rm -rf "<writableDir>/<dir>"` for each dir
    Remote-->>Nemoclaw: success / non-zero (logged warning)
    Local->>Remote: stream `tar -cf -` piped to `tar -xf -` over SSH
    Remote-->>Nemoclaw: restore complete
    Nemoclaw-->>User: success
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I checked the sandbox, hopped with care,
Cleared old burrows with a whiskered stare,
Streams of tar and ssh-sung tune,
Snapshots nestle by the moon 🌙
Restore complete — I munch a prune.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: adding a preflight sandbox liveness check and implementing clean restore semantics by removing stale directories before extraction.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/snapshot-restore-preflight

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

src/lib/sandbox-state.ts (2)

356-358: Proceeding after cleanup failure is reasonable but may leave inconsistent state.

If rm -rf fails partially (e.g., permission denied on some files), the subsequent tar extraction will overwrite matching files but stale files in subdirectories that weren't removed could persist. The warning-only approach is pragmatic since failing the entire restore would be worse, but operators should be aware that a warning here may indicate a partially inconsistent restore.

Consider documenting this behavior or surfacing the warning more prominently to the user (not just in verbose logs).

---

345-358: Shell command construction may be vulnerable to injection from manifest data.

Directory names and writableDir are interpolated directly into the shell command. If the manifest contains values with shell metacharacters ($, backticks, ", etc.), this could cause unexpected behavior or command injection. While the double quotes mitigate some risks, they don't fully escape all metacharacters.

This follows the existing pattern in extractCmd (line 360) and backupSandboxState, so consistency is maintained. Consider sanitizing or using an allow-list of safe characters for paths in a follow-up.

🛡️ Safer path handling approach

+function shellEscape(s: string): string {
+  // Replace each single quote with '\'' and wrap in single quotes
+  return "'" + s.replace(/'/g, "'\\''") + "'";
+}
+
 // In restoreSandboxState:
     const rmCmd = localDirs
-      .map((d) => `rm -rf "${writableDir}/${d}"`)
+      .map((d) => `rm -rf ${shellEscape(`${writableDir}/${d}`)}`)
       .join(" && ");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/sandbox-state.ts` around lines 345 - 358, The rm command is built by
interpolating writableDir and localDirs into rmCmd and can be injection-prone;
add a safe-path validation step (e.g. an isSafePath function that enforces an
allow-list regex like /^[A-Za-z0-9._\/-]+$/) and validate writableDir and every
entry in localDirs before building rmCmd, throwing/logging an error if any path
fails validation; then build rmCmd only from validated values (using proper
quoting) and proceed to call spawnSync with sshArgs, referencing the existing
symbols rmCmd, localDirs, writableDir, spawnSync, sshArgs, and _log.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/lib/sandbox-state.ts`:
- Around line 345-358: The rm command is built by interpolating writableDir and
localDirs into rmCmd and can be injection-prone; add a safe-path validation step
(e.g. an isSafePath function that enforces an allow-list regex like
/^[A-Za-z0-9._\/-]+$/) and validate writableDir and every entry in localDirs
before building rmCmd, throwing/logging an error if any path fails validation;
then build rmCmd only from validated values (using proper quoting) and proceed
to call spawnSync with sshArgs, referencing the existing symbols rmCmd,
localDirs, writableDir, spawnSync, sshArgs, and _log.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4bb29625-78ee-4aba-b3ff-593ffe72a470

📥 Commits

Reviewing files that changed from the base of the PR and between a9a9ec8 and d664eaf.

📒 Files selected for processing (1)

• src/lib/sandbox-state.ts

[brandonpelfrey]

Automated PR review summary

Reviewed PR #1901: fix(snapshot): preflight sandbox liveness before restore

Recommendation

• Recommendation: PASS
• Highest observed severity: low
• Block merge: no
• Why: The highest-risk failure mode described by the PR was silent success on restore when sandbox liveness was not validated. In the reviewed environment, restore on a live sandbox still proceeds to the next real check, and when the OpenShell live-state query is sabotaged it fails closed with an explicit error. Source inspection shows the new live-sandbox check occurs before backup-path selection, which is the necessary ordering to fix the reported bug.
• Reviewer summary: Reviewed PR fix(snapshot): preflight sandbox liveness and clean restore #1901 with installed NemoClaw/OpenShell and targeted adversarial probes around the new snapshot-restore liveness preflight. I found no regression or bypass in the claimed fix.

Installation and setup findings

• Install and onboarding succeeded from the local checkout. NemoClaw CLI and OpenShell were available, onboarding created sandbox 'nemoclaw-local-install', and SSH exec inside that existing sandbox returned '2+2=4'. I also ran an in-sandbox OpenClaw probe; it hit the provider path but ended in a timeout rather than a model reply.

What was validated

• The PR revision was checked out in an isolated review environment.
• The local checkout was installed using the repository installer flow as closely as the environment allowed.
• Adversarial, PR-specific probes were then run against the installed environment and relevant repository context.
• Diff summary:

 src/nemoclaw.ts | 10 ++++++++++
 1 file changed, 10 insertions(+)

Failing tests and unresolved impact

• No failing adversarial tests were captured.

Passing tests and why they mattered

Passing test 1: Live sandbox restore still reaches snapshot lookup

• What was tested: The new liveness preflight does not regress restore for a running sandbox; a live sandbox should pass preflight and continue to the next restore check.
• Why it mattered: If false, the PR would break legitimate restore attempts for healthy sandboxes.
• Observed result: End-to-end host CLI probe against the installed sandbox returned exit=1 with No snapshots found for 'nemoclaw-local-install', showing preflight passed and restore continued into snapshot lookup.
• Command: nemoclaw nemoclaw-local-install snapshot restore
• Recommended follow-up coverage: Add an integration/regression test that mocks a live sandbox in openshell sandbox list and asserts snapshot restore reaches backup discovery rather than failing preflight.

Passing test 2: OpenShell liveness-query failure is surfaced as a hard restore error

• What was tested: If openshell sandbox list fails during restore preflight, the command should fail closed with an explicit error instead of silently succeeding.
• Why it mattered: If false, the old silent-success bug would still exist when the liveness check itself is unavailable or flaky.
• Observed result: With a temporary /tmp/openshell wrapper that failed only sandbox list, restore returned exit=1 and printed Failed to query live sandbox state from OpenShell.
• Command: PATH="/tmp:$PATH" nemoclaw nemoclaw-local-install snapshot restore
• Recommended follow-up coverage: Add a regression test that forces captureOpenshell(["sandbox","list"]) to fail and asserts the explicit restore error path.

Passing test 3: Preflight is ordered before backup inspection for non-live targets

• What was tested: The patch checks live sandbox membership before any backup-path logic, so a stopped or missing sandbox would be rejected even when no snapshot directories exist.
• Why it mattered: If false, restore against a stopped or missing sandbox could still silently report success under empty-backup conditions.
• Observed result: Source inspection shows snapshot restore now calls captureOpenshell(["sandbox","list"]), parses names via parseLiveSandboxNames, and exits with Sandbox '<name>' is not running. Cannot restore snapshot. before timestamp/backup-path code. Runtime enumeration showed only the live nemoclaw-local-install sandbox, so a non-destructive stopped-sandbox end-to-end probe was not available in this session.
• Command: grep/sed inspection of src/nemoclaw.ts and src/lib/runtime-recovery.ts with live sandbox enumeration
• Recommended follow-up coverage: Add an integration/regression test for a stopped-or-missing sandbox with no backup directories, asserting the liveness error fires before any success path.

Bottom line

• Based on the install evidence and adversarial probes, this PR looks reasonable to approve.