Defer the COW branch decision past process exit

README.md

@@ -246,7 +246,7 @@ sandlock run --no-supervisor -r /proc -r /usr -r /lib -r /lib64 -r /bin -r /etc
 ### Python API
 
 ```python
-from sandlock import Sandbox, confine
+from sandlock import Sandbox, BranchAction, confine
 
 sandbox = Sandbox(
     fs_writable=["/tmp/sandbox"],
@@ -286,10 +286,18 @@ sb = Sandbox(port_remap=True, fs_readable=["/usr", "/lib", "/etc"], name="api.lo
 confine(Sandbox(fs_readable=["/usr", "/lib"], fs_writable=["/tmp"]))
 
 # Dry-run: see what files would change, then discard
-sandbox = Sandbox(fs_writable=["."], workdir=".", fs_readable=["/usr", "/lib", "/bin", "/etc"])
-result = sandbox.dry_run(["make", "build"])
+sandbox = Sandbox(fs_writable=["."], workdir=".", fs_readable=["/usr", "/lib", "/bin", "/etc"],
+                  on_exit=BranchAction.ABORT)
+result = sandbox.run(["make", "build"])
 for c in result.changes:
     print(f"{c.kind}  {c.path}")  # A=added, M=modified, D=deleted
+
+# Defer: inspect the changes, then commit or abort
+sandbox = Sandbox(fs_writable=["."], workdir=".", fs_readable=["/usr", "/lib", "/bin", "/etc"],
+                  on_exit=BranchAction.DEFER)
+result = sandbox.run(["make", "build"])
+if sandbox.pending:
+    sandbox.commit() if approve(result.changes, sandbox.upper_dir) else sandbox.abort()
 ```
 
 ### Pipeline

crates/sandlock-cli/src/main.rs

@@ -718,35 +718,15 @@ async fn run_command(args: RunArgs) -> Result<i32> {
         policy
     };
 
-    let result = if args.dry_run {
+    if args.dry_run {
         if policy.workdir.is_none() {
             return Err(anyhow!("--dry-run requires --workdir"));
         }
-        let dr = if let Some(secs) = args.timeout {
-            match tokio::time::timeout(
-                std::time::Duration::from_secs(secs),
-                policy.dry_run_interactive(&cmd_strs),
-            ).await {
-                Ok(r) => r?,
-                Err(_) => {
-                    eprintln!("sandlock: timeout after {}s", secs);
-                    return Ok(124);
-                }
-            }
-        } else {
-            policy.dry_run_interactive(&cmd_strs).await?
-        };
+        policy.on_exit = BranchAction::Abort;
+        policy.on_error = BranchAction::Abort;
+    }
 
-        if dr.changes.is_empty() {
-            eprintln!("sandlock: dry-run: no filesystem changes");
-        } else {
-            eprintln!("sandlock: dry-run: filesystem changes:");
-            for change in &dr.changes {
-                eprintln!("{}", change);
-            }
-        }
-        dr.run_result
-    } else if let Some(secs) = args.timeout {
+    let result = if let Some(secs) = args.timeout {
         match tokio::time::timeout(
             std::time::Duration::from_secs(secs),
             policy.run_interactive(&cmd_strs),
@@ -761,6 +741,17 @@ async fn run_command(args: RunArgs) -> Result<i32> {
         policy.run_interactive(&cmd_strs).await?
     };
 
+    if args.dry_run {
+        if result.changes.is_empty() {
+            eprintln!("sandlock: dry-run: no filesystem changes");
+        } else {
+            eprintln!("sandlock: dry-run: filesystem changes:");
+            for change in &result.changes {
+                eprintln!("{change}");
+            }
+        }
+    }
+
     if let Some(fd) = args.status_fd {
         use std::io::Write as _;
         use std::os::unix::io::FromRawFd;

crates/sandlock-cli/tests/cli_test.rs

@@ -684,3 +684,28 @@ fn test_help_shows_ps_and_inspect() {
         "--help should NOT show 'list' command (renamed to 'ps')"
     );
 }
+
+/// `--dry-run` is a run with both branch actions forced to `abort`: the
+/// change list is reported on stderr and nothing lands in the workdir.
+#[test]
+fn test_dry_run_reports_changes_and_writes_nothing() {
+    let workdir = tempfile::tempdir().expect("tempdir");
+    let sentinel = workdir.path().join("planned.txt");
+
+    let cmd = format!("echo planned > {}", sentinel.display());
+    let output = sandlock_bin()
+        .args(args_for_host(&[
+            "run", "--dry-run",
+            "-r", "/usr", "-r", "/lib", "-r", "/lib64", "-r", "/bin", "-r", "/etc",
+            "-w", workdir.path().to_str().unwrap(),
+            "--workdir", workdir.path().to_str().unwrap(),
+            "--", "sh", "-c", &cmd,
+        ]))
+        .output()
+        .expect("failed to run sandlock");
+    let stderr = String::from_utf8_lossy(&output.stderr);
+    assert!(output.status.success(), "sandlock exit={:?}, stderr: {stderr}", output.status.code());
+
+    assert!(!sentinel.exists(), "a dry run must not write the workdir");
+    assert!(stderr.contains("A  planned.txt"), "change list missing from stderr: {stderr}");
+}

crates/sandlock-core/src/cow/seccomp.rs

@@ -1591,28 +1591,24 @@ impl SeccompCowBranch {
     }
 
     /// List all filesystem changes in the COW layer.
-    pub fn changes(&self) -> Result<Vec<crate::dry_run::Change>, BranchError> {
-        use crate::dry_run::{Change, ChangeKind};
+    pub fn changes(&self) -> Result<Vec<crate::result::Change>, BranchError> {
+        use crate::result::{Change, ChangeKind};
 
         let mut result = Vec::new();
 
-        // Walk upper directory for added/modified files
+        // The kind compares the two trees as they stand, not the branch's
+        // history: a whiteouted-then-recreated path still has its old bytes
+        // in the workdir, and that is what a caller diffing the sides needs.
         for entry in walkdir::WalkDir::new(&self.upper).min_depth(1) {
             let entry = entry.map_err(|e| BranchError::Operation(format!("walk: {}", e)))?;
-            if entry.file_type().is_dir() {
+            let rel = entry.path().strip_prefix(&self.upper).unwrap();
+            let lower = self.workdir.join(rel).symlink_metadata().ok();
+            // Copy-up recreates a modified file's parents in the upper; a
+            // directory the workdir already has is scaffolding, not a change.
+            if entry.file_type().is_dir() && lower.as_ref().is_some_and(|m| m.is_dir()) {
                 continue;
             }
-            let rel = entry.path().strip_prefix(&self.upper).unwrap();
-            let lower = self.workdir.join(rel);
-            // A covered path's lower entry is logically gone, so a re-created
-            // upper entry is an addition even though lower bytes still exist.
-            let kind = if self.deleted.covers(&rel.to_string_lossy()) {
-                ChangeKind::Added
-            } else if lower.exists() {
-                ChangeKind::Modified
-            } else {
-                ChangeKind::Added
-            };
+            let kind = if lower.is_some() { ChangeKind::Modified } else { ChangeKind::Added };
             result.push(Change { kind, path: rel.to_path_buf() });
         }
 
@@ -3117,7 +3113,7 @@ mod tests {
             "the deletion was applied before the failure",
         );
 
-        let mut outstanding: Vec<(crate::dry_run::ChangeKind, String)> = branch
+        let mut outstanding: Vec<(crate::result::ChangeKind, String)> = branch
             .changes()
             .unwrap()
             .into_iter()
@@ -3129,8 +3125,8 @@ mod tests {
             vec![
                 // b.txt is "modified" because the obstructing symlink is still
                 // there in the workdir; c.txt was never reached.
-                (crate::dry_run::ChangeKind::Modified, "b.txt".to_string()),
-                (crate::dry_run::ChangeKind::Added, "c.txt".to_string()),
+                (crate::result::ChangeKind::Modified, "b.txt".to_string()),
+                (crate::result::ChangeKind::Added, "c.txt".to_string()),
             ],
             "changes() after a partial merge must report the remainder only",
         );
@@ -3672,7 +3668,7 @@ mod tests {
         fs::write(&upper, "new content").unwrap();
         let changes = branch.changes().unwrap();
         assert_eq!(changes.len(), 1);
-        assert_eq!(changes[0].kind, crate::dry_run::ChangeKind::Added);
+        assert_eq!(changes[0].kind, crate::result::ChangeKind::Added);
         assert_eq!(changes[0].path, std::path::PathBuf::from("brand_new.txt"));
     }
 
@@ -3684,7 +3680,7 @@ mod tests {
         fs::write(&upper, "modified content").unwrap();
         let changes = branch.changes().unwrap();
         assert_eq!(changes.len(), 1);
-        assert_eq!(changes[0].kind, crate::dry_run::ChangeKind::Modified);
+        assert_eq!(changes[0].kind, crate::result::ChangeKind::Modified);
         assert_eq!(changes[0].path, std::path::PathBuf::from("existing.txt"));
     }
 
@@ -3695,7 +3691,7 @@ mod tests {
         branch.mark_deleted("existing.txt");
         let changes = branch.changes().unwrap();
         assert_eq!(changes.len(), 1);
-        assert_eq!(changes[0].kind, crate::dry_run::ChangeKind::Deleted);
+        assert_eq!(changes[0].kind, crate::result::ChangeKind::Deleted);
         assert_eq!(changes[0].path, std::path::PathBuf::from("existing.txt"));
     }
 
@@ -3720,11 +3716,11 @@ mod tests {
         let mut changes = branch.changes().unwrap();
         changes.sort_by(|a, b| a.path.cmp(&b.path));
         assert_eq!(changes.len(), 3);
-        assert_eq!(changes[0].kind, crate::dry_run::ChangeKind::Modified);
+        assert_eq!(changes[0].kind, crate::result::ChangeKind::Modified);
         assert_eq!(changes[0].path, std::path::PathBuf::from("existing.txt"));
-        assert_eq!(changes[1].kind, crate::dry_run::ChangeKind::Added);
+        assert_eq!(changes[1].kind, crate::result::ChangeKind::Added);
         assert_eq!(changes[1].path, std::path::PathBuf::from("new.txt"));
-        assert_eq!(changes[2].kind, crate::dry_run::ChangeKind::Deleted);
+        assert_eq!(changes[2].kind, crate::result::ChangeKind::Deleted);
         assert_eq!(changes[2].path, std::path::PathBuf::from("subdir/nested.txt"));
     }
 
@@ -4523,13 +4519,13 @@ mod tests {
         let upper = branch.ensure_cow_copy("existing.txt").unwrap();
         fs::write(&upper, "recreated").unwrap();
         let changes = branch.changes().unwrap();
-        // The recreated file is a single Added entry, not Deleted + Modified.
+        // The recreated file is a single Modified entry, not Deleted + Modified.
         let for_path: Vec<_> = changes
             .iter()
             .filter(|c| c.path == std::path::Path::new("existing.txt"))
             .collect();
         assert_eq!(for_path.len(), 1);
-        assert_eq!(for_path[0].kind, crate::dry_run::ChangeKind::Added);
+        assert_eq!(for_path[0].kind, crate::result::ChangeKind::Modified);
     }
 
     #[test]
@@ -5429,7 +5425,7 @@ mod tests {
                 .changes()
                 .unwrap()
                 .iter()
-                .all(|c| c.kind != crate::dry_run::ChangeKind::Deleted),
+                .all(|c| c.kind != crate::result::ChangeKind::Deleted),
             "a whiteout the upper re-created must not be reported as a deletion",
         );
 
@@ -5549,7 +5545,7 @@ mod tests {
                 .changes()
                 .unwrap()
                 .into_iter()
-                .filter(|c| c.kind == crate::dry_run::ChangeKind::Deleted)
+                .filter(|c| c.kind == crate::result::ChangeKind::Deleted)
                 .map(|c| c.path)
                 .collect::<Vec<_>>(),
             vec![PathBuf::from("link/x.txt")],
@@ -5668,7 +5664,7 @@ mod tests {
                 .into_iter()
                 .map(|c| (c.kind, c.path))
                 .collect::<Vec<_>>(),
-            vec![(crate::dry_run::ChangeKind::Modified, PathBuf::from("f.txt"))],
+            vec![(crate::result::ChangeKind::Modified, PathBuf::from("f.txt"))],
             "precondition: the run reports the chmod as a recorded change",
         );
 
@@ -5831,7 +5827,7 @@ mod tests {
     /// reading a dry run or a recovery report is actually being told.
     #[test]
     fn changes_labels_an_entry_against_the_workdir_as_it_stands_now() {
-        use crate::dry_run::ChangeKind;
+        use crate::result::ChangeKind;
         let workdir = tempfile::tempdir().unwrap();
         let storage = tempfile::tempdir().unwrap();
 
@@ -5851,6 +5847,72 @@ mod tests {
         );
     }
 
+    /// A path that exists on both sides is Modified even when a whiteout
+    /// covers it: `sed -i`, `mv over` and `rm; recreate` all unlink first,
+    /// and a caller diffing the two trees needs the old bytes it can still
+    /// read in the workdir, not a claim that the file is new.
+    #[test]
+    fn changes_labels_a_recreated_entry_modified_while_the_workdir_still_has_it() {
+        use crate::result::ChangeKind;
+        let workdir = tempfile::tempdir().unwrap();
+        let storage = tempfile::tempdir().unwrap();
+        fs::write(workdir.path().join("f.txt"), "before").unwrap();
+
+        let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap();
+        branch.mark_deleted("f.txt");
+        fs::write(branch.upper.join("f.txt"), "after").unwrap();
+
+        let changes: Vec<_> = branch
+            .changes()
+            .unwrap()
+            .into_iter()
+            .map(|c| (c.kind, c.path.display().to_string()))
+            .collect();
+        assert_eq!(changes, vec![(ChangeKind::Modified, "f.txt".to_string())]);
+    }
+
+    /// The commit creates every directory the upper holds, so an empty one the
+    /// run made is a change and must be reported like any other addition.
+    #[test]
+    fn changes_reports_an_added_empty_directory() {
+        use crate::result::ChangeKind;
+        let workdir = tempfile::tempdir().unwrap();
+        let storage = tempfile::tempdir().unwrap();
+
+        let branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap();
+        fs::create_dir(branch.upper.join("newdir")).unwrap();
+
+        let changes: Vec<_> = branch
+            .changes()
+            .unwrap()
+            .into_iter()
+            .map(|c| (c.kind, c.path.display().to_string()))
+            .collect();
+        assert_eq!(changes, vec![(ChangeKind::Added, "newdir".to_string())]);
+    }
+
+    /// Copy-up recreates the parents of a modified file in the upper; those
+    /// mirror directories the workdir already has and are not changes.
+    #[test]
+    fn changes_skips_upper_directories_the_workdir_already_has() {
+        use crate::result::ChangeKind;
+        let workdir = tempfile::tempdir().unwrap();
+        let storage = tempfile::tempdir().unwrap();
+        fs::create_dir(workdir.path().join("sub")).unwrap();
+
+        let branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap();
+        fs::create_dir(branch.upper.join("sub")).unwrap();
+        fs::write(branch.upper.join("sub/a.txt"), "new").unwrap();
+
+        let changes: Vec<_> = branch
+            .changes()
+            .unwrap()
+            .into_iter()
+            .map(|c| (c.kind, c.path.display().to_string()))
+            .collect();
+        assert_eq!(changes, vec![(ChangeKind::Added, "sub/a.txt".to_string())]);
+    }
+
     // ---- Names, symlinks and the confined path helpers ----
 
     /// `safe_rel` normalises the spellings that name the same entry, rejects an
@@ -6242,7 +6304,7 @@ mod tests {
     /// run do" with half the truth.
     #[test]
     fn changes_on_a_kept_branch_still_reports_the_whole_change_set() {
-        use crate::dry_run::ChangeKind;
+        use crate::result::ChangeKind;
         let workdir = tempfile::tempdir().unwrap();
         let storage = tempfile::tempdir().unwrap();
         fs::write(workdir.path().join("gone.txt"), "still here").unwrap();