From d8acc3adde5b68d2a9da74e1e347f76702cf955e Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sun, 23 Aug 2026 20:17:08 +0100 Subject: [PATCH] feat(maestro): always clean up worktrees on kill behind a dirty-work guard --- crates/maestro-cli/src/cli.rs | 27 +++++- crates/maestro-daemon/src/session.rs | 33 ++++---- crates/maestro-daemon/src/worktree.rs | 83 ++++++++++++++++++- .../maestro-daemon/tests/session_manager.rs | 70 +++++++++++++++- crates/maestro-daemon/tests/worktree.rs | 42 ++++++++++ crates/maestro-e2e/tests/e2e.rs | 47 ++++++++++- crates/maestro-protocol/src/lib.rs | 9 +- 7 files changed, 285 insertions(+), 26 deletions(-) diff --git a/crates/maestro-cli/src/cli.rs b/crates/maestro-cli/src/cli.rs index 660f494..3f497e6 100644 --- a/crates/maestro-cli/src/cli.rs +++ b/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, - /// 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([ diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 41845eb..7ae544c 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/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, /// `base_repo` from the original `WorktreeSpec`. Needed because `git /// worktree remove` is invoked from the base repo. pub worktree_base_repo: Option, - 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, 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 }); diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index 73a9097..0e8ad5e 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/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 worktree remove --force `. + /// `git -C worktree remove ` — 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, 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::>(); + + 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, +) -> Result { + 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(()) diff --git a/crates/maestro-daemon/tests/session_manager.rs b/crates/maestro-daemon/tests/session_manager.rs index 17dfd1e..deb2cc7 100644 --- a/crates/maestro-daemon/tests/session_manager.rs +++ b/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(); diff --git a/crates/maestro-daemon/tests/worktree.rs b/crates/maestro-daemon/tests/worktree.rs index 5740f73..fdb6945 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/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(); diff --git a/crates/maestro-e2e/tests/e2e.rs b/crates/maestro-e2e/tests/e2e.rs index 647f663..ccf9da3 100644 --- a/crates/maestro-e2e/tests/e2e.rs +++ b/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; diff --git a/crates/maestro-protocol/src/lib.rs b/crates/maestro-protocol/src/lib.rs index 4b6fac9..82c73b7 100644 --- a/crates/maestro-protocol/src/lib.rs +++ b/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]