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-cli/src/sessions.rs b/crates/maestro-cli/src/sessions.rs index 2e3729a..e3ca37e 100644 --- a/crates/maestro-cli/src/sessions.rs +++ b/crates/maestro-cli/src/sessions.rs @@ -44,6 +44,7 @@ pub async fn run_spawn(client: &DaemonClient, args: SpawnArgs) -> Result None, }; diff --git a/crates/maestro-config/src/lib.rs b/crates/maestro-config/src/lib.rs index 57b2a6d..4780720 100644 --- a/crates/maestro-config/src/lib.rs +++ b/crates/maestro-config/src/lib.rs @@ -87,8 +87,9 @@ pub struct DaemonSection { /// Root for everything daemon-local: sidecars, FIFOs, etc. #[serde(default)] pub state_dir: Option, - /// Root for ephemeral worktrees the daemon creates on `CreateSession` - /// with a `worktree` spec. + /// Override root for ephemeral worktrees the daemon creates on + /// `CreateSession` with a `worktree` spec. Unset by default: worktrees + /// then live inside their base repo at `/worktrees/` (#56). #[serde(default)] pub worktree_root: Option, /// Per-session PTY replay buffer cap in bytes (NAP-229). Each new attach diff --git a/crates/maestro-daemon/src/cli.rs b/crates/maestro-daemon/src/cli.rs index 542440b..a836059 100644 --- a/crates/maestro-daemon/src/cli.rs +++ b/crates/maestro-daemon/src/cli.rs @@ -47,7 +47,8 @@ async fn run(config_override: Option) -> Result<(), Box> { let _ = default_state_dir(); std::fs::create_dir_all(config.sidecar_dir())?; std::fs::create_dir_all(config.fifo_dir())?; - std::fs::create_dir_all(&config.worktree_root)?; + // No worktree dir to pre-create: without an override the worktrees live + // under their base repos, created on demand (#56). let sidecar = SidecarStore::new(config.sidecar_dir()); let worktree_mgr = WorktreeManager::new(config.worktree_root.clone()); diff --git a/crates/maestro-daemon/src/config.rs b/crates/maestro-daemon/src/config.rs index 88ded4b..dccb4f5 100644 --- a/crates/maestro-daemon/src/config.rs +++ b/crates/maestro-daemon/src/config.rs @@ -53,7 +53,10 @@ pub struct DaemonConfig { pub home_dir: String, pub bind_addr: SocketAddr, pub state_dir: PathBuf, - pub worktree_root: PathBuf, + /// `[daemon].worktree_root` override (#56). `None` — the default — places + /// worktrees inside their base repo at `/worktrees/`; + /// operators who want them elsewhere set this. + pub worktree_root: Option, /// Per-session PTY replay buffer cap in bytes (NAP-229). Each new attach /// receives this much of the prior PTY output before the live stream. /// Default `pty::DEFAULT_REPLAY_BYTES`; `0` disables. @@ -131,9 +134,6 @@ pub fn load_config(path: &Path, env: EnvOverrides) -> Result p, None => default_state_dir()?, }; - let worktree_root = daemon - .worktree_root - .unwrap_or_else(|| state_dir.join("worktrees")); let pty_replay_bytes = daemon .pty_replay_bytes @@ -152,7 +152,7 @@ pub fn load_config(path: &Path, env: EnvOverrides) -> Result, /// `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, } @@ -213,24 +212,20 @@ impl SessionManager { /// Spawn a new session. On success returns the freshly-created `Session`. pub async fn create(&self, req: CreateSessionRequest) -> Result { let (effective_cwd, worktree_path, worktree_base_repo) = match &req.worktree { - Some(WorktreeSpec { - base_repo, branch, .. - }) => { - let path = self - .inner - .worktree_mgr - .add(base_repo, branch) - .map_err(SessionError::Worktree)?; + Some(spec) => { + let WorktreeSpec { + base_repo, branch, .. + } = spec; + let path = if spec.create_branch { + self.inner.worktree_mgr.add_new(base_repo, branch) + } else { + self.inner.worktree_mgr.add_existing(base_repo, branch) + } + .map_err(SessionError::Worktree)?; (path.clone(), Some(path), Some(base_repo.clone())) } 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 @@ -315,7 +310,6 @@ impl SessionManager { fifo_path, worktree_path, worktree_base_repo, - cleanup_worktree_on_kill, bridge, }, ); @@ -357,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 @@ -454,7 +449,6 @@ impl SessionManager { fifo_path, worktree_path: None, worktree_base_repo: None, - cleanup_worktree_on_kill: false, bridge, }, ); @@ -703,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 b5ffa63..0e8ad5e 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -4,64 +4,113 @@ use std::path::{Path, PathBuf}; use std::process::{Command, Output}; -use uuid::Uuid; - #[derive(Debug, thiserror::Error)] pub enum WorktreeError { #[error("failed to spawn git: {0}")] Spawn(#[from] std::io::Error), #[error("git exited with code {code}: {stderr}")] NonZero { code: i32, stderr: String }, + /// A worktree already occupies the target path. The caller decides what + /// 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 }, } -/// Generates a unique path under `root` and runs `git worktree add` rooted at -/// `base_repo`. Each call appends a UUID-suffixed directory so concurrent -/// `add()`s never collide. +/// Runs `git worktree add` rooted at `base_repo`. By default worktrees land +/// inside the repository itself at `/worktrees/` (#56) so they +/// are findable from the repo; an operator-set root overrides that. #[derive(Debug, Clone)] pub struct WorktreeManager { - root: PathBuf, + /// `[daemon].worktree_root` override. `None` uses the default + /// `/worktrees/` placement. + root_override: Option, } impl WorktreeManager { - pub fn new(root: impl Into) -> Self { - Self { root: root.into() } + pub fn new(root_override: Option) -> Self { + Self { root_override } } - pub fn root(&self) -> &Path { - &self.root + pub fn root(&self) -> Option<&Path> { + self.root_override.as_deref() } - /// `git -C worktree add -b `. - /// Returns the path of the new worktree on success. - pub fn add(&self, base_repo: &Path, branch: &str) -> Result { - std::fs::create_dir_all(&self.root)?; - let path = self.generate_path(branch); - let out = Command::new("git") - .arg("-C") - .arg(base_repo) - .args(["worktree", "add", "-b", branch]) - .arg(&path) - .output()?; + /// Where a worktree for `branch` will be created: the override root, or + /// `/worktrees/` by default. The path is + /// predictable (no UUID suffix), so an existing directory at the target + /// is an error rather than a silently different location. + fn target_path(&self, base_repo: &Path, branch: &str) -> PathBuf { + let slug = sanitize_branch_for_path(branch); + match &self.root_override { + Some(root) => root.join(slug), + None => base_repo.join("worktrees").join(slug), + } + } + + /// `git -C worktree add -b ` — creates the + /// branch. Returns the path of the new worktree on success. + pub fn add_new(&self, base_repo: &Path, branch: &str) -> Result { + self.run_add(base_repo, branch, true) + } + + /// `git -C worktree add ` — checks out a branch + /// that must already exist (#56). + pub fn add_existing(&self, base_repo: &Path, branch: &str) -> Result { + self.run_add(base_repo, branch, false) + } + + fn run_add( + &self, + base_repo: &Path, + branch: &str, + create_branch: bool, + ) -> Result { + let path = self.target_path(base_repo, branch); + if path.symlink_metadata().is_ok() { + return Err(WorktreeError::PathExists { path }); + } + // The parent (`/worktrees` or the override root) may not exist + // yet; the target itself must not. + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + let mut cmd = Command::new("git"); + cmd.arg("-C").arg(base_repo).args(["worktree", "add"]); + if create_branch { + cmd.arg("-b").arg(branch).arg(&path); + } else { + // Existing branch: `worktree add ` with no `-b`. + cmd.arg(&path).arg(branch); + } + let out = cmd.output()?; check(out)?; 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) } - - fn generate_path(&self, branch: &str) -> PathBuf { - let slug = sanitize_branch_for_path(branch); - let unique = Uuid::new_v4().simple(); - self.root.join(format!("{slug}-{unique}")) - } } fn sanitize_branch_for_path(branch: &str) -> String { @@ -79,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/common/mod.rs b/crates/maestro-daemon/tests/common/mod.rs index 2e1e96a..cf077cf 100644 --- a/crates/maestro-daemon/tests/common/mod.rs +++ b/crates/maestro-daemon/tests/common/mod.rs @@ -115,7 +115,6 @@ pub struct DaemonFixture { pub tmux: TmuxFixture, pub manager: SessionManager, pub state_dir: TempDir, - pub worktree_root: TempDir, } impl DaemonFixture { @@ -132,9 +131,9 @@ impl DaemonFixture { fn build(history_days: Option) -> Self { let tmux = TmuxFixture::new(); let state_dir = TempDir::new().unwrap(); - let worktree_root = TempDir::new().unwrap(); let sidecar = SidecarStore::new(state_dir.path().join("sessions")); - let worktree_mgr = WorktreeManager::new(worktree_root.path().to_path_buf()); + // Default placement: worktrees land under their base repo (#56). + let worktree_mgr = WorktreeManager::new(None); let mut manager = SessionManager::new( Arc::new(tmux.tmux.clone()), sidecar, @@ -149,7 +148,6 @@ impl DaemonFixture { tmux, manager, state_dir, - worktree_root, } } diff --git a/crates/maestro-daemon/tests/session_decisions.rs b/crates/maestro-daemon/tests/session_decisions.rs index 967f6fd..fb48862 100644 --- a/crates/maestro-daemon/tests/session_decisions.rs +++ b/crates/maestro-daemon/tests/session_decisions.rs @@ -38,7 +38,7 @@ impl Fx { let manager = SessionManager::new( fake.clone(), SidecarStore::new(pick(tmp.path())), - WorktreeManager::new(tmp.path().join("worktrees")), + WorktreeManager::new(Some(tmp.path().join("worktrees"))), fifo_dir, ); Self { manager, fake, tmp } @@ -387,7 +387,7 @@ fn attaching_spawn_history_after_cloning_panics() { let manager = SessionManager::new( fake, SidecarStore::new(tmp.path().join("sidecars")), - WorktreeManager::new(tmp.path().join("worktrees")), + WorktreeManager::new(Some(tmp.path().join("worktrees"))), tmp.path().join("fifos"), ); let _clone = manager.clone(); diff --git a/crates/maestro-daemon/tests/session_manager.rs b/crates/maestro-daemon/tests/session_manager.rs index 6b1df55..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] @@ -123,8 +123,7 @@ async fn recover_adopts_existing_tmux_sessions() { // build a parallel manager pointing at the same dirs + same tmux server let sidecar = maestro_daemon::sidecar::SidecarStore::new(fx.sidecar_dir()); - let worktree_mgr = - maestro_daemon::worktree::WorktreeManager::new(fx.worktree_root.path().to_path_buf()); + let worktree_mgr = maestro_daemon::worktree::WorktreeManager::new(None); let revived = maestro_daemon::SessionManager::new( Arc::new(fx.tmux.tmux.clone()), sidecar, @@ -214,7 +213,6 @@ async fn recover_re_pipes_pane_so_output_reaches_subscribers() { let sidecar_dir = fx.sidecar_dir(); let fifo_dir = fx.fifo_dir(); let tmux = fx.tmux.tmux.clone(); - let worktree_root = fx.worktree_root.path().to_path_buf(); // Drop the original manager. Its PtyBridge closes the FIFO's only // reader, so tmux's `cat >> fifo` writer hits SIGPIPE and exits — but @@ -223,7 +221,7 @@ async fn recover_re_pipes_pane_so_output_reaches_subscribers() { drop(fx.manager); let sidecar = maestro_daemon::sidecar::SidecarStore::new(sidecar_dir); - let worktree_mgr = maestro_daemon::worktree::WorktreeManager::new(worktree_root); + let worktree_mgr = maestro_daemon::worktree::WorktreeManager::new(None); let revived = maestro_daemon::SessionManager::new(Arc::new(tmux), sidecar, worktree_mgr, fifo_dir); assert_eq!(revived.recover().await.expect("recover"), 1); @@ -365,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 1087ac6..fdb6945 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/crates/maestro-daemon/tests/worktree.rs @@ -31,9 +31,9 @@ fn add_creates_worktree_on_new_branch() { init_repo(&repo); let wt_root = TempDir::new().unwrap(); - let mgr = WorktreeManager::new(wt_root.path()); + let mgr = WorktreeManager::new(Some(wt_root.path().to_path_buf())); - let path = mgr.add(&repo, "feat/example").expect("add worktree"); + let path = mgr.add_new(&repo, "feat/example").expect("add worktree"); assert!(path.exists()); assert!(path.join("README").exists()); assert!(path.starts_with(wt_root.path())); @@ -42,6 +42,63 @@ fn add_creates_worktree_on_new_branch() { assert!(branches.iter().any(|b| b == "feat/example"), "{branches:?}"); } +#[test] +fn add_places_worktree_inside_the_base_repo_by_default() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + // No override: the worktree must land where the repository keeps its + // own worktrees (#56), not under some daemon state dir. + let mgr = WorktreeManager::new(None); + + let path = mgr.add_new(&repo, "feat/example").expect("add worktree"); + assert_eq!(path, repo.join("worktrees").join("feat-example")); + assert!(path.exists()); + assert!(path.join("README").exists()); + + let branches = worktree_branches(&repo); + assert!(branches.iter().any(|b| b == "feat/example"), "{branches:?}"); +} + +#[test] +fn worktree_root_override_redirects_placement() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + let wt_root = TempDir::new().unwrap(); + let mgr = WorktreeManager::new(Some(wt_root.path().to_path_buf())); + + let path = mgr.add_new(&repo, "feat/example").expect("add worktree"); + assert_eq!(path, wt_root.path().join("feat-example")); + assert!(path.exists()); +} + +#[test] +fn existing_directory_at_target_path_is_an_error_carrying_the_path() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + // A predictable path means collisions are possible; they must be + // reported, never silently relocated. + std::fs::create_dir_all(repo.join("worktrees").join("feat-collide")).unwrap(); + + let mgr = WorktreeManager::new(None); + let err = mgr.add_new(&repo, "feat/collide").unwrap_err(); + let msg = format!("{err}"); + assert!( + msg.contains( + repo.join("worktrees") + .join("feat-collide") + .to_str() + .unwrap() + ), + "{msg}" + ); +} + #[test] fn add_twice_generates_distinct_paths() { let repo_dir = TempDir::new().unwrap(); @@ -49,10 +106,10 @@ fn add_twice_generates_distinct_paths() { init_repo(&repo); let wt_root = TempDir::new().unwrap(); - let mgr = WorktreeManager::new(wt_root.path()); + let mgr = WorktreeManager::new(Some(wt_root.path().to_path_buf())); - let p1 = mgr.add(&repo, "feat/a").expect("add a"); - let p2 = mgr.add(&repo, "feat/b").expect("add b"); + let p1 = mgr.add_new(&repo, "feat/a").expect("add a"); + let p2 = mgr.add_new(&repo, "feat/b").expect("add b"); assert_ne!(p1, p2); assert!(p1.exists()); assert!(p2.exists()); @@ -65,9 +122,9 @@ fn remove_prunes_worktree() { init_repo(&repo); let wt_root = TempDir::new().unwrap(); - let mgr = WorktreeManager::new(wt_root.path()); + let mgr = WorktreeManager::new(Some(wt_root.path().to_path_buf())); - let path = mgr.add(&repo, "feat/gone").expect("add"); + let path = mgr.add_new(&repo, "feat/gone").expect("add"); assert!(path.exists()); mgr.remove(&repo, &path).expect("remove"); @@ -79,6 +136,104 @@ fn remove_prunes_worktree() { assert_eq!(branches, vec!["main".to_string()]); } +#[test] +fn add_existing_checks_out_branch_without_creating_it() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + // Pre-create the branch with its own commit so we can tell a checkout + // from an accidental `-b`. + let created = Command::new("git") + .arg("-C") + .arg(&repo) + .args(["checkout", "-q", "-b", "feat/dup"]) + .status() + .unwrap(); + assert!(created.success()); + std::fs::write(repo.join("FEATURE"), b"dup\n").unwrap(); + let staged = Command::new("git") + .arg("-C") + .arg(&repo) + .args(["add", "."]) + .status() + .unwrap(); + assert!(staged.success()); + let committed = Command::new("git") + .arg("-C") + .arg(&repo) + .args(["commit", "-q", "-am", "wip"]) + .status() + .unwrap(); + assert!(committed.success()); + let back = Command::new("git") + .arg("-C") + .arg(&repo) + .args(["checkout", "-q", "main"]) + .status() + .unwrap(); + assert!(back.success()); + + let wt_root = TempDir::new().unwrap(); + let mgr = WorktreeManager::new(Some(wt_root.path().to_path_buf())); + + let path = mgr.add_existing(&repo, "feat/dup").expect("add existing"); + assert!(path.exists()); + assert!(path.join("README").exists()); + + // The branch set is unchanged: `feat/dup` was checked out, not recreated. + let branches = worktree_branches(&repo); + let mut sorted = branches.clone(); + sorted.sort(); + assert_eq!( + sorted, + vec!["feat/dup".to_string(), "main".to_string()], + "{branches:?}" + ); +} + +#[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(); @@ -95,9 +250,9 @@ fn add_fails_when_branch_already_exists() { assert!(status.success()); let wt_root = TempDir::new().unwrap(); - let mgr = WorktreeManager::new(wt_root.path()); + let mgr = WorktreeManager::new(Some(wt_root.path().to_path_buf())); - let err = mgr.add(&repo, "feat/dup").unwrap_err(); + let err = mgr.add_new(&repo, "feat/dup").unwrap_err(); let msg = format!("{err}"); assert!(msg.contains("git exited"), "{msg}"); } diff --git a/crates/maestro-e2e/tests/e2e.rs b/crates/maestro-e2e/tests/e2e.rs index 5cb166a..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(); @@ -129,18 +129,19 @@ async fn cleanup_on_kill_removes_worktree() { base_repo: base.path().to_path_buf(), branch: "feature/e2e".into(), cleanup_on_kill: true, + create_branch: true, }), }; let s = client.spawn_session(&req).await.expect("spawn worktree"); - // The daemon's WorktreeManager puts new worktrees under - // `/worktrees/`. We can't peek the daemon's state dir - // directly from here, but we can verify by asking git: a worktree must - // exist that lives outside the original repo. + // Worktrees live inside the base repo at `/worktrees/` + // (#56), so `git worktree list` must show the predictable in-repo path. let worktrees = git_worktree_list(base.path()); assert!( - worktrees.iter().any(|w| w.contains("feature/e2e")), - "expected a worktree for feature/e2e branch, got: {worktrees:?}" + worktrees + .iter() + .any(|w| w.contains("worktrees/feature-e2e")), + "expected a worktree for feature/e2e under /worktrees, got: {worktrees:?}" ); client.kill_session(s.id).await.expect("kill"); @@ -160,6 +161,85 @@ 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; + + let base = TempDir::new().unwrap(); + init_repo(base.path()); + run_git(&["branch", "feature/existing"], base.path()); + + 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/existing".into(), + cleanup_on_kill: true, + create_branch: false, + }), + }; + + let s = client + .spawn_session(&req) + .await + .expect("spawn on existing branch"); + let worktrees = git_worktree_list(base.path()); + assert!( + worktrees.iter().any(|w| w.contains("feature/existing")), + "expected a worktree checked out at feature/existing, got: {worktrees:?}" + ); + + client.kill_session(s.id).await.expect("kill"); +} + #[tokio::test(flavor = "multi_thread")] async fn daemon_restart_recovers_session() { let (mut daemon, client) = bring_up().await; diff --git a/crates/maestro-protocol/src/lib.rs b/crates/maestro-protocol/src/lib.rs index fe001b8..82c73b7 100644 --- a/crates/maestro-protocol/src/lib.rs +++ b/crates/maestro-protocol/src/lib.rs @@ -111,8 +111,20 @@ 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 + /// the field existed) the daemon creates it with `worktree add -b`. + #[serde(default = "default_true")] + pub create_branch: bool, +} + +fn default_true() -> bool { + true } /// HTTP `POST /api/sessions` body. @@ -393,6 +405,7 @@ mod tests { base_repo: PathBuf::from("/home/naps62/projects/maestro"), branch: "feat/foo".into(), cleanup_on_kill: true, + create_branch: true, }), }; assert_eq!(req, round_trip(&req)); @@ -410,6 +423,31 @@ mod tests { assert!(req.metadata.is_empty()); } + /// A client that predates the new/existing discriminator sends no + /// `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_on_legacy_payload() { + let json = serde_json::json!({ + "base_repo": "/home/naps62/projects/maestro", + "branch": "feat/foo", + }); + let spec: WorktreeSpec = serde_json::from_value(json).unwrap(); + assert!(spec.create_branch); + assert!(spec.cleanup_on_kill); + } + + #[test] + fn worktree_spec_round_trips_existing_branch_mode() { + let spec = WorktreeSpec { + base_repo: PathBuf::from("/work"), + branch: "feat/foo".into(), + cleanup_on_kill: true, + create_branch: false, + }; + assert_eq!(spec, round_trip(&spec)); + } + #[test] fn catalog_event_round_trips_and_tags() { let session = sample_session(); diff --git a/docs/INSTALL.md b/docs/INSTALL.md index ad02b5f..a6b2a63 100644 --- a/docs/INSTALL.md +++ b/docs/INSTALL.md @@ -280,8 +280,9 @@ maestro daemon # foreground; or wire your own service manager ``` Every setting has a default, so `~/.config/maestro/config.toml` is optional — -create it only to change the bind address, worktree root, spawn presets, or TUI -keymap. +create it only to change the bind address, spawn presets, or TUI +keymap. (Worktrees live inside their base repos at `/worktrees/`; +set `[daemon].worktree_root` only to override that.) ## Non-Linux hosts