fix(snapshot): make vfs_restore fail closed and apply atomically

crates/bashkit/src/fs/memory.rs

@@ -690,7 +690,7 @@ impl InMemoryFs {
     /// fs.write_file(Path::new("/tmp/new.txt"), b"new file").await?;
     ///
     /// // Restore
-    /// fs.restore(&snapshot);
+    /// fs.restore(&snapshot)?;
     ///
     /// // Original state is back
     /// let content = fs.read_file(Path::new("/tmp/data.txt")).await?;
@@ -701,32 +701,39 @@ impl InMemoryFs {
     /// # Ok(())
     /// # }
     /// ```
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if any snapshot entry violates the configured VFS
+    /// limits (path validation, file size, total bytes, or file count). On
+    /// error the existing filesystem contents are left untouched (issue
+    /// #1576) so callers can refuse the restore atomically.
     // THREAT[TM-ESC-012]: Enforce VFS limits to prevent bypass via restore()
-    pub fn restore(&self, snapshot: &VfsSnapshot) {
+    pub fn restore(&self, snapshot: &VfsSnapshot) -> crate::error::Result<()> {
         // Validate ALL snapshot entries before clearing existing state.
         // If any validation fails, return early WITHOUT clearing.
         let mut total_bytes = 0u64;
         let mut file_count = 0u64;
 
         for entry in &snapshot.entries {
-            if self.limits.validate_path(&entry.path).is_err() {
-                return;
-            }
+            self.limits
+                .validate_path(&entry.path)
+                .map_err(|e| IoError::other(e.to_string()))?;
             if let VfsEntryKind::File { content } = &entry.kind {
-                if self.limits.check_file_size(content.len() as u64).is_err() {
-                    return;
-                }
+                self.limits
+                    .check_file_size(content.len() as u64)
+                    .map_err(|e| IoError::other(e.to_string()))?;
                 total_bytes += content.len() as u64;
                 file_count += 1;
             }
         }
 
         if total_bytes > self.limits.max_total_bytes {
-            return;
-        }
-        if self.limits.check_file_count(file_count).is_err() {
-            return;
+            return Err(IoError::other("snapshot total bytes exceed limit").into());
         }
+        self.limits
+            .check_file_count(file_count)
+            .map_err(|e| IoError::other(e.to_string()))?;
 
         let mut entries = self.entries.write().unwrap();
         entries.clear();
@@ -796,6 +803,8 @@ impl InMemoryFs {
                 }
             }
         }
+
+        Ok(())
     }
 
     fn normalize_path(path: &Path) -> PathBuf {
@@ -1714,9 +1723,8 @@ impl FileSystemExt for InMemoryFs {
         Some(self.snapshot())
     }
 
-    fn vfs_restore(&self, snapshot: &VfsSnapshot) -> bool {
-        self.restore(snapshot);
-        true
+    fn vfs_restore(&self, snapshot: &VfsSnapshot) -> Result<()> {
+        self.restore(snapshot)
     }
 }
 
@@ -2134,7 +2142,9 @@ mod tests {
             max_file_size: 100,
             ..FsLimits::default()
         });
-        limited.restore(&snapshot);
+        // Issue #1576: restore must fail closed and not silently no-op.
+        let err = limited.restore(&snapshot);
+        assert!(err.is_err(), "restore must report the limit violation");
         assert!(!limited.exists(Path::new("/tmp/huge.bin")).await.unwrap());
     }
 

crates/bashkit/src/fs/mountable.rs

@@ -545,7 +545,7 @@ impl FileSystemExt for MountableFs {
         self.root.vfs_snapshot()
     }
 
-    fn vfs_restore(&self, snapshot: &super::VfsSnapshot) -> bool {
+    fn vfs_restore(&self, snapshot: &super::VfsSnapshot) -> crate::Result<()> {
         // Delegate to root filesystem
         self.root.vfs_restore(snapshot)
     }

crates/bashkit/src/fs/overlay.rs

@@ -953,9 +953,8 @@ impl FileSystemExt for OverlayFs {
         Some(self.upper.snapshot())
     }
 
-    fn vfs_restore(&self, snapshot: &super::VfsSnapshot) -> bool {
-        self.upper.restore(snapshot);
-        true
+    fn vfs_restore(&self, snapshot: &super::VfsSnapshot) -> crate::Result<()> {
+        self.upper.restore(snapshot)
     }
 }
 

crates/bashkit/src/fs/traits.rs

@@ -165,10 +165,14 @@ pub trait FileSystemExt: Send + Sync {
 
     /// Restore filesystem contents from a snapshot.
     ///
-    /// Returns `false` if this filesystem doesn't support restore.
-    /// The default implementation returns `false`.
-    fn vfs_restore(&self, _snapshot: &super::VfsSnapshot) -> bool {
-        false
+    /// Returns `Err(...)` if this filesystem doesn't support restore, or if
+    /// the snapshot fails validation (path, size, or count limits). On error
+    /// the filesystem MUST be left untouched so callers can refuse the
+    /// restore atomically (issue #1576).
+    ///
+    /// The default implementation returns `Err(Unsupported)`.
+    fn vfs_restore(&self, _snapshot: &super::VfsSnapshot) -> Result<()> {
+        Err(IoError::new(ErrorKind::Unsupported, "vfs_restore not supported").into())
     }
 }
 

crates/bashkit/src/snapshot.rs

@@ -324,12 +324,17 @@ impl crate::Bash {
     }
 
     fn restore_snapshot_inner(&mut self, snap: &Snapshot) -> crate::Result<()> {
+        // Issue #1576: validate everything that can fail before mutating
+        // either the shell or the VFS, so a malformed/forged snapshot can't
+        // leave the instance in a half-restored state.
         self.interpreter
             .validate_shell_state_restore_limits(&snap.shell)?;
-        self.interpreter.restore_shell_state(&snap.shell);
         if let Some(ref vfs) = snap.vfs {
-            self.fs.vfs_restore(vfs);
+            self.fs.vfs_restore(vfs)?;
         }
+        // Shell state cannot fail past validation, and the VFS has already
+        // been restored atomically (or rejected) above.
+        self.interpreter.restore_shell_state(&snap.shell);
         Ok(())
     }
 

crates/bashkit/tests/snapshot_tests.rs

@@ -24,7 +24,7 @@ async fn vfs_snapshot_restores_file_content() {
         .unwrap();
 
     // Restore
-    fs.restore(&snapshot);
+    fs.restore(&snapshot).unwrap();
 
     let content = fs.read_file(Path::new("/tmp/test.txt")).await.unwrap();
     assert_eq!(content, b"original");
@@ -42,7 +42,7 @@ async fn vfs_snapshot_removes_new_files() {
     assert!(fs.exists(Path::new("/tmp/new.txt")).await.unwrap());
 
     // Restore
-    fs.restore(&snapshot);
+    fs.restore(&snapshot).unwrap();
     assert!(!fs.exists(Path::new("/tmp/new.txt")).await.unwrap());
 }
 
@@ -60,7 +60,7 @@ async fn vfs_snapshot_restores_deleted_files() {
     assert!(!fs.exists(Path::new("/tmp/keep.txt")).await.unwrap());
 
     // Restore
-    fs.restore(&snapshot);
+    fs.restore(&snapshot).unwrap();
     let content = fs.read_file(Path::new("/tmp/keep.txt")).await.unwrap();
     assert_eq!(content, b"keep me");
 }
@@ -79,7 +79,7 @@ async fn vfs_snapshot_preserves_directories() {
     fs.remove(Path::new("/data"), true).await.unwrap();
     assert!(!fs.exists(Path::new("/data")).await.unwrap());
 
-    fs.restore(&snapshot);
+    fs.restore(&snapshot).unwrap();
     assert!(fs.exists(Path::new("/data/sub")).await.unwrap());
     let content = fs.read_file(Path::new("/data/sub/file.txt")).await.unwrap();
     assert_eq!(content, b"content");
@@ -97,7 +97,7 @@ async fn vfs_snapshot_serialization_roundtrip() {
     let restored: bashkit::VfsSnapshot = serde_json::from_str(&json).unwrap();
 
     let fs2 = Arc::new(InMemoryFs::new());
-    fs2.restore(&restored);
+    fs2.restore(&restored).unwrap();
 
     let content = fs2.read_file(Path::new("/tmp/data.txt")).await.unwrap();
     assert_eq!(content, b"serialize me");
@@ -187,7 +187,7 @@ async fn combined_snapshot_restore_multi_turn() {
         .unwrap();
 
     // Rollback to turn 1
-    fs.restore(&vfs_snap);
+    fs.restore(&vfs_snap).unwrap();
     bash.restore_shell_state(&shell_snap);
 
     let result = bash
@@ -593,6 +593,66 @@ async fn snapshot_restore_rejects_tampered_shell_state_that_exceeds_memory_limit
     );
 }
 
+// ==================== Atomic restore (Issue #1576) ====================
+
+/// A malformed VFS snapshot (path validation failure) must cause
+/// `restore_snapshot` to fail closed. The previous-tenant shell state
+/// must not be replaced and the previous-tenant VFS must not be
+/// observable, otherwise a forged unkeyed snapshot can leave a reused
+/// instance in a half-restored, cross-tenant state.
+#[tokio::test]
+async fn snapshot_restore_fails_closed_on_malformed_vfs() {
+    // Tenant A: real, valid snapshot.
+    let mut tenant_a = Bash::new();
+    tenant_a.exec("greeting=alpha").await.unwrap();
+    tenant_a
+        .fs()
+        .write_file(Path::new("/tmp/secret-a.txt"), b"alpha-secret")
+        .await
+        .unwrap();
+    let bytes = tenant_a.snapshot().unwrap();
+
+    // Patch the snapshot JSON so the VFS section contains a poisoned
+    // path (rejected by limits::validate_path) and the shell variable
+    // changes — both clearly distinguish "applied" from "not applied".
+    let mut json: serde_json::Value = serde_json::from_slice(&bytes[32..]).unwrap();
+    json["shell"]["variables"]["greeting"] = serde_json::json!("BETA");
+    json["vfs"] = serde_json::json!({
+        "entries": [
+            {
+                "path": "/poison\u{0}bad",
+                "kind": { "File": { "content": [120] } },
+                "mode": 0o644,
+            }
+        ]
+    });
+    let forged_snap: Snapshot = serde_json::from_value(json).unwrap();
+    let forged = forged_snap.to_bytes().unwrap();
+
+    let result = tenant_a.restore_snapshot(&forged);
+    assert!(
+        result.is_err(),
+        "restore must fail closed when VFS validation fails (#1576)"
+    );
+
+    // Shell state must not be mutated.
+    let r = tenant_a.exec("echo $greeting").await.unwrap();
+    assert_eq!(
+        r.stdout.trim(),
+        "alpha",
+        "shell state must remain unchanged after a failed restore"
+    );
+
+    // Pre-existing VFS must still match tenant A's contents (no clobber
+    // by partial restore, no tenant-B injection).
+    let secret = tenant_a
+        .fs()
+        .read_file(Path::new("/tmp/secret-a.txt"))
+        .await
+        .unwrap();
+    assert_eq!(secret, b"alpha-secret");
+}
+
 // ==================== Integrity verification (Issue #977) ====================
 
 #[tokio::test]