feat(maestro): always clean up worktrees on kill behind a dirty-work guard

crates/maestro-cli/src/cli.rs

@@ -67,8 +67,10 @@ pub struct SpawnArgs {
     /// Branch name for the worktree (required with --worktree-from).
     #[arg(long)]
     pub branch: Option<String>,
-    /// Remove the worktree when the session is killed.
-    #[arg(long)]
+    /// Remove the worktree when the session is killed. Kept for wire
+    /// compatibility (#56): the daemon now always cleans up on kill, guarded
+    /// against unsaved work.
+    #[arg(long, default_value_t = true)]
     pub cleanup_on_kill: bool,
     /// After spawning, attach immediately.
     #[arg(long, short = 'a')]
@@ -147,6 +149,27 @@ mod tests {
         }
     }
 
+    #[test]
+    fn cleanup_on_kill_defaults_true_for_the_wire() {
+        let cli = Cli::try_parse_from([
+            "maestro",
+            "spawn",
+            "--cwd",
+            "/tmp",
+            "--command",
+            "bash",
+            "--worktree-from",
+            "/repos/foo",
+            "--branch",
+            "feat/x",
+        ])
+        .unwrap();
+        match cli.command {
+            Command::Spawn(a) => assert!(a.cleanup_on_kill),
+            _ => panic!("wrong subcommand"),
+        }
+    }
+
     #[test]
     fn spawn_worktree_requires_branch() {
         let err = Cli::try_parse_from([

crates/maestro-daemon/src/session.rs

@@ -38,13 +38,12 @@ pub const DEFAULT_LIVENESS_INTERVAL: Duration = Duration::from_secs(2);
 pub struct SessionRecord {
     pub session: Session,
     pub fifo_path: PathBuf,
-    /// Worktree we created at spawn time and that should be cleaned up on
-    /// kill (if `cleanup_worktree_on_kill`).
+    /// Worktree we created at spawn time; removed on kill unless the
+    /// dirty-work guard refuses (#56).
     pub worktree_path: Option<PathBuf>,
     /// `base_repo` from the original `WorktreeSpec`. Needed because `git
     /// worktree remove` is invoked from the base repo.
     pub worktree_base_repo: Option<PathBuf>,
-    pub cleanup_worktree_on_kill: bool,
     pub bridge: PtyBridge,
 }
 
@@ -227,12 +226,6 @@ impl SessionManager {
             }
             None => (req.cwd.clone(), None, None),
         };
-        let cleanup_worktree_on_kill = req
-            .worktree
-            .as_ref()
-            .map(|w| w.cleanup_on_kill)
-            .unwrap_or(false);
-
         let id = Uuid::new_v4();
         // Build the FIFO + reader task *before* asking tmux to spawn the
         // command — otherwise the child's first bytes race the separate
@@ -317,7 +310,6 @@ impl SessionManager {
                     fifo_path,
                     worktree_path,
                     worktree_base_repo,
-                    cleanup_worktree_on_kill,
                     bridge,
                 },
             );
@@ -359,8 +351,9 @@ impl SessionManager {
         }
     }
 
-    /// Kill a session and remove its on-disk artifacts. The worktree is only
-    /// pruned if the create request set `cleanup_on_kill: true`.
+    /// Kill a session and remove its on-disk artifacts. The worktree is
+    /// always pruned -- unless it holds uncommitted changes or commits not on
+    /// any other ref, in which case the guard leaves it on disk (#56).
     ///
     /// This is also how an already-dead session is dismissed. tmux is only
     /// asked to kill something it still has — calling `kill-session` on a
@@ -456,7 +449,6 @@ impl SessionManager {
                     fifo_path,
                     worktree_path: None,
                     worktree_base_repo: None,
-                    cleanup_worktree_on_kill: false,
                     bridge,
                 },
             );
@@ -705,9 +697,18 @@ async fn cleanup_record(inner: &Arc<Inner>, id: Uuid, record: SessionRecord, kil
     if let Err(e) = inner.sidecar.delete(id) {
         tracing::warn!(error = ?e, %id, "cleanup: failed to delete sidecar");
     }
-    if record.cleanup_worktree_on_kill {
-        if let (Some(base), Some(path)) = (&record.worktree_base_repo, &record.worktree_path) {
-            let _ = inner.worktree_mgr.remove(base, path);
+    // Cleanup is unconditional now (#56): `cleanup_on_kill` is no longer a
+    // user-facing choice. The manager's guard is what protects unsaved work --
+    // when it refuses, leave the tree on disk and say where it is so the
+    // operator can find it.
+    if let (Some(base), Some(path)) = (&record.worktree_base_repo, &record.worktree_path) {
+        if let Err(e) = inner.worktree_mgr.remove(base, path) {
+            tracing::warn!(
+                %id,
+                path = %path.display(),
+                error = %e,
+                "left worktree on disk"
+            );
         }
     }
     let _ = inner.events.send(CatalogEvent::SessionRemoved { id });

crates/maestro-daemon/src/worktree.rs

@@ -14,6 +14,9 @@ pub enum WorktreeError {
     /// to do; the daemon never relocates to a different directory (#56).
     #[error("worktree path already exists: {}", .path.display())]
     PathExists { path: PathBuf },
+    /// The worktree holds unsaved work; removal was refused (#56).
+    #[error("left {} on disk: {}", .path.display(), .reason)]
+    Guarded { path: PathBuf, reason: String },
 }
 
 /// Runs `git worktree add` rooted at `base_repo`. By default worktrees land
@@ -87,12 +90,23 @@ impl WorktreeManager {
         Ok(path)
     }
 
-    /// `git -C <base_repo> worktree remove --force <path>`.
+    /// `git -C <base_repo> worktree remove <path>` — but only when the
+    /// worktree holds nothing unique (#56). A worktree with uncommitted
+    /// changes, or with commits reachable from no other ref, is refused and
+    /// left on disk so the operator can rescue it; cleanup is unconditional,
+    /// which makes this guard the only thing standing between a stray kill
+    /// and destroyed work.
     pub fn remove(&self, base_repo: &Path, path: &Path) -> Result<(), WorktreeError> {
+        if let Some(reason) = unsafe_to_remove(path)? {
+            return Err(WorktreeError::Guarded {
+                path: path.to_path_buf(),
+                reason,
+            });
+        }
         let out = Command::new("git")
             .arg("-C")
             .arg(base_repo)
-            .args(["worktree", "remove", "--force"])
+            .args(["worktree", "remove"])
             .arg(path)
             .output()?;
         check(out)
@@ -114,6 +128,71 @@ fn sanitize_branch_for_path(branch: &str) -> String {
     out
 }
 
+/// `Some(reason)` when deleting `path` would destroy work: uncommitted
+/// changes, or commits reachable from no ref other than the worktree's own
+/// branch (`git worktree remove` keeps the branch, but nothing checks it out
+/// anymore — treat its commits as at risk anyway, per #56).
+fn unsafe_to_remove(path: &Path) -> Result<Option<String>, WorktreeError> {
+    let status = run_git(path, ["status", "--porcelain"])?;
+    if !status.stdout.is_empty() {
+        return Ok(Some("has uncommitted changes".into()));
+    }
+
+    let head = String::from_utf8_lossy(&run_git(path, ["rev-parse", "HEAD"])?.stdout)
+        .trim()
+        .to_string();
+    let branch =
+        String::from_utf8_lossy(&run_git(path, ["rev-parse", "--abbrev-ref", "HEAD"])?.stdout)
+            .trim()
+            .to_string();
+
+    // Every ref except this worktree's own branch counts as a safe home for
+    // its commits. Detached HEAD excludes everything.
+    let refs =
+        String::from_utf8_lossy(&run_git(path, ["for-each-ref", "--format=%(refname)"])?.stdout)
+            .lines()
+            .filter(|r| match branch.as_str() {
+                "HEAD" => true,
+                b => *r != format!("refs/heads/{b}"),
+            })
+            .map(str::to_string)
+            .collect::<Vec<_>>();
+
+    let mut cmd = Command::new("git");
+    cmd.arg("-C").arg(path).args(["rev-list", "--count", &head]);
+    if !refs.is_empty() {
+        cmd.arg("--not").args(&refs);
+    }
+    let out = cmd.output()?;
+    check(out.clone())?;
+    let count: usize = String::from_utf8_lossy(&out.stdout)
+        .trim()
+        .parse()
+        .unwrap_or(0);
+    if count > 0 {
+        return Ok(Some(format!(
+            "has {count} commit(s) not present on any other ref"
+        )));
+    }
+    Ok(None)
+}
+
+fn run_git(
+    dir: &Path,
+    args: impl IntoIterator<Item = &'static str>,
+) -> Result<Output, WorktreeError> {
+    let mut cmd = Command::new("git");
+    cmd.arg("-C").arg(dir).args(args);
+    let out = cmd.output()?;
+    if !out.status.success() {
+        return Err(WorktreeError::NonZero {
+            code: out.status.code().unwrap_or(-1),
+            stderr: String::from_utf8_lossy(&out.stderr).into_owned(),
+        });
+    }
+    Ok(out)
+}
+
 fn check(out: Output) -> Result<(), WorktreeError> {
     if out.status.success() {
         Ok(())

crates/maestro-daemon/tests/session_manager.rs

@@ -7,9 +7,9 @@ use std::path::PathBuf;
 use std::sync::Arc;
 use std::time::Duration;
 
-use common::{wait_for_async, DaemonFixture};
+use common::{init_repo, wait_for_async, DaemonFixture};
 use maestro_protocol::CreateSessionRequest;
-use maestro_protocol::{CatalogEvent, SessionStatus};
+use maestro_protocol::{CatalogEvent, SessionStatus, WorktreeSpec};
 use tempfile::TempDir;
 
 #[tokio::test]
@@ -363,6 +363,72 @@ async fn liveness_watcher_marks_dead_sessions_and_keeps_them_until_dismissed() {
     assert!(!fx.fifo_dir().join(format!("{}.fifo", s.id)).exists());
 }
 
+/// Cleanup is unconditional (#56): even a spec that says "don't clean up"
+/// loses its worktree on kill — the guard, not the flag, protects work.
+#[tokio::test]
+async fn kill_removes_worktree_even_when_spec_says_keep() {
+    let fx = DaemonFixture::new();
+    let repo = TempDir::new().unwrap();
+    init_repo(repo.path());
+    let wt_path = repo.path().join("worktrees").join("feat-keep");
+
+    let s = fx
+        .manager
+        .create(CreateSessionRequest {
+            cwd: repo.path().to_path_buf(),
+            command: "sleep 30".into(),
+            env: HashMap::new(),
+            metadata: HashMap::new(),
+            worktree: Some(WorktreeSpec {
+                base_repo: repo.path().to_path_buf(),
+                branch: "feat/keep".into(),
+                cleanup_on_kill: false,
+                create_branch: true,
+            }),
+        })
+        .await
+        .expect("create");
+    assert!(wt_path.exists());
+
+    fx.manager.kill(s.id).await.expect("kill");
+    assert!(!wt_path.exists(), "worktree must be removed on kill");
+}
+
+/// The guard (#56): a worktree holding uncommitted changes survives the kill,
+/// left on disk where the operator can find it.
+#[tokio::test]
+async fn kill_leaves_worktree_with_unsaved_work_on_disk() {
+    let fx = DaemonFixture::new();
+    let repo = TempDir::new().unwrap();
+    init_repo(repo.path());
+    let wt_path = repo.path().join("worktrees").join("feat-guarded");
+
+    let s = fx
+        .manager
+        .create(CreateSessionRequest {
+            cwd: repo.path().to_path_buf(),
+            command: "sleep 30".into(),
+            env: HashMap::new(),
+            metadata: HashMap::new(),
+            worktree: Some(WorktreeSpec {
+                base_repo: repo.path().to_path_buf(),
+                branch: "feat/guarded".into(),
+                cleanup_on_kill: true,
+                create_branch: true,
+            }),
+        })
+        .await
+        .expect("create");
+    std::fs::write(wt_path.join("README"), b"unsaved edits\n").unwrap();
+
+    fx.manager.kill(s.id).await.expect("kill");
+    assert!(
+        wt_path.exists(),
+        "dirty worktree must be left on disk, not destroyed"
+    );
+    assert!(fx.manager.snapshot().await.is_empty());
+}
+
 #[tokio::test]
 async fn liveness_watcher_leaves_running_sessions_alone() {
     let fx = DaemonFixture::new();

crates/maestro-daemon/tests/worktree.rs

@@ -192,6 +192,48 @@ fn add_existing_checks_out_branch_without_creating_it() {
     );
 }
 
+#[test]
+fn remove_refuses_worktree_with_uncommitted_changes() {
+    let repo_dir = TempDir::new().unwrap();
+    let repo = repo_dir.path().to_path_buf();
+    init_repo(&repo);
+
+    let mgr = WorktreeManager::new(None);
+    let path = mgr.add_new(&repo, "feat/dirty").expect("add");
+    std::fs::write(path.join("README"), b"unsaved edits\n").unwrap();
+
+    mgr.remove(&repo, &path)
+        .expect_err("dirty worktree must be refused");
+    assert!(path.exists(), "guarded worktree must stay on disk");
+
+    // Committing the change doesn't help while it is the only copy…
+    run_git(&path, &["add", "."]);
+    run_git(&path, &["commit", "-q", "-m", "unsaved"]);
+
+    mgr.remove(&repo, &path)
+        .expect_err("unique commit must be refused");
+    assert!(path.exists());
+
+    // …but once the commit is reachable from main, removal is safe.
+    run_git(&repo, &["merge", "-q", "--no-edit", "feat/dirty"]);
+    mgr.remove(&repo, &path).expect("merged worktree can go");
+    assert!(!path.exists());
+}
+
+fn run_git(dir: &std::path::Path, args: &[&str]) {
+    let out = Command::new("git")
+        .arg("-C")
+        .arg(dir)
+        .args(args)
+        .output()
+        .expect("git spawn");
+    assert!(
+        out.status.success(),
+        "git {args:?} failed: {}",
+        String::from_utf8_lossy(&out.stderr)
+    );
+}
+
 #[test]
 fn add_fails_when_branch_already_exists() {
     let repo_dir = TempDir::new().unwrap();

crates/maestro-e2e/tests/e2e.rs

@@ -114,7 +114,7 @@ async fn multi_attach_sees_same_output() {
 }
 
 #[tokio::test(flavor = "multi_thread")]
-async fn cleanup_on_kill_removes_worktree() {
+async fn kill_removes_clean_worktree() {
     let (_daemon, client) = bring_up().await;
 
     let base = TempDir::new().unwrap();
@@ -161,6 +161,51 @@ async fn cleanup_on_kill_removes_worktree() {
     );
 }
 
+/// The guard (#56): killing a session whose worktree holds uncommitted
+/// changes must leave the tree on disk — cleanup is unconditional, so this
+/// guard is what keeps a stray kill from destroying work.
+#[tokio::test(flavor = "multi_thread")]
+async fn kill_spares_worktree_with_uncommitted_changes() {
+    let (_daemon, client) = bring_up().await;
+
+    let base = TempDir::new().unwrap();
+    init_repo(base.path());
+    let wt_path = base.path().join("worktrees").join("feature-unsaved");
+
+    let req = CreateSessionRequest {
+        cwd: base.path().to_path_buf(),
+        command: "sleep 30".into(),
+        env: HashMap::new(),
+        metadata: HashMap::new(),
+        worktree: Some(WorktreeSpec {
+            base_repo: base.path().to_path_buf(),
+            branch: "feature/unsaved".into(),
+            cleanup_on_kill: true,
+            create_branch: true,
+        }),
+    };
+
+    let s = client.spawn_session(&req).await.expect("spawn worktree");
+    assert!(wt_path.exists());
+    std::fs::write(wt_path.join("README"), b"unsaved edits\n").unwrap();
+
+    client.kill_session(s.id).await.expect("kill");
+
+    // The session is gone but the dirty worktree stays findable on disk.
+    let deadline = Instant::now() + Duration::from_secs(5);
+    while Instant::now() < deadline {
+        let sessions = client.list_sessions().await.unwrap_or_default();
+        if sessions.is_empty() {
+            break;
+        }
+        tokio::time::sleep(Duration::from_millis(100)).await;
+    }
+    assert!(
+        wt_path.exists(),
+        "guarded worktree must be left on disk after kill"
+    );
+}
+
 #[tokio::test(flavor = "multi_thread")]
 async fn spawn_on_existing_branch_checks_it_out() {
     let (_daemon, client) = bring_up().await;

crates/maestro-protocol/src/lib.rs

@@ -111,7 +111,10 @@ pub enum ActivityState {
 pub struct WorktreeSpec {
     pub base_repo: PathBuf,
     pub branch: String,
-    #[serde(default)]
+    /// Kept on the wire for CLI compatibility (#56) but no longer a
+    /// user-facing choice: the daemon always cleans up on kill, guarded
+    /// against unsaved work. Defaults to `true`.
+    #[serde(default = "default_true")]
     pub cleanup_on_kill: bool,
     /// When false, `branch` must already exist and is checked out as-is
     /// (#56); when true (the default, and what every payload meant before
@@ -424,14 +427,14 @@ mod tests {
     /// `create_branch`; the daemon must read that as "create a new branch" —
     /// exactly what every payload meant before the field existed (#56).
     #[test]
-    fn worktree_spec_defaults_create_branch_true_on_legacy_payload() {
+    fn worktree_spec_defaults_on_legacy_payload() {
         let json = serde_json::json!({
             "base_repo": "/home/naps62/projects/maestro",
             "branch": "feat/foo",
-            "cleanup_on_kill": true,
         });
         let spec: WorktreeSpec = serde_json::from_value(json).unwrap();
         assert!(spec.create_branch);
+        assert!(spec.cleanup_on_kill);
     }
 
     #[test]