From 898c3cc763d532e824d18d6ed54bf147fb66b71f Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Mon, 24 Aug 2026 10:59:08 +0100 Subject: [PATCH 1/2] feat(maestro): shared unsaved-work probe for worktrees (#79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One function, unsaved_work(), returns the structured breakdown — uncommitted tracked changes, untracked non-ignored files, unpushed commits — so the kill guard and the natural-exit classifier (#82) answer from the same place. Ignored files never count: every Rust checkout has a target/, and counting those would leave no session killable. WorktreeManager::remove drops its own dirty-tree guard; git itself still refuses a dirty tree, and the real guard moves onto the kill. --- crates/maestro-daemon/src/worktree.rs | 83 ++++++++----- crates/maestro-daemon/tests/common/mod.rs | 32 +++++ crates/maestro-daemon/tests/worktree.rs | 136 ++++++++++++++++++++-- crates/maestro-protocol/src/lib.rs | 62 ++++++++++ 4 files changed, 274 insertions(+), 39 deletions(-) diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index d698606..d68640e 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -10,7 +10,9 @@ pub enum WorktreeError { Spawn(#[from] std::io::Error), #[error("git exited with code {code}: {stderr}")] NonZero { code: i32, stderr: String }, - /// The worktree holds unsaved work; removal was refused (#56). + /// A registered worktree lives under the tree about to be removed; git + /// hides its contents from `git status`, so removal would delete its + /// uncommitted work unseen (#54 review). #[error("left {} on disk: {}", .path.display(), .reason)] Guarded { path: PathBuf, reason: String }, } @@ -126,12 +128,13 @@ impl WorktreeManager { Ok(path) } - /// `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. + /// `git -C worktree remove `. + /// + /// Unconditional (#79): the guard moved off the worktree and onto the + /// kill, so by the time removal runs the caller has already chosen to + /// destroy the tree — the kill only gets here after + /// [`unsaved_work`] came back empty. Still refused for a tree holding a + /// nested registered worktree, which `git status` cannot see. pub fn remove(&self, base_repo: &Path, path: &Path) -> Result<(), WorktreeError> { // Gone from disk already (operator `rm -rf`, or a create that failed // half-way). Probing it with git would fail and be misread as unsaved @@ -147,12 +150,6 @@ impl WorktreeManager { reason: format!("holds a nested worktree at {}", inner.display()), }); } - 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) @@ -251,22 +248,49 @@ fn branch_relpath(branch: &str) -> Option { Some(candidate.to_path_buf()) } -/// `Some(reason)` when deleting `path` would destroy work. +/// The unsaved-work probe (#79): what would be lost if `path` were deleted. /// -/// Only an unclean working tree qualifies. `git worktree remove` deletes the -/// checkout but keeps `refs/heads/`, so committed work — pushed or -/// not — stays reachable and is never at risk. Guarding on "commits on no -/// other ref" instead would refuse every ordinary unpushed feature branch, -/// and because the path is deterministic that branch could then never be -/// spawned again (#56 review). -fn unsafe_to_remove(path: &Path) -> Result, WorktreeError> { - // `--ignored=no` keeps build output out of it; untracked sources count, - // since removal would delete them. - let status = run_git(path, ["status", "--porcelain", "--ignored=no"])?; - if !status.stdout.is_empty() { - return Ok(Some("has uncommitted changes".into())); +/// One function, three answers, shared by every caller that must make the +/// same decision — the kill guard refuses on any non-empty result and the +/// natural-exit classifier (#82) retains on it — so the two paths cannot +/// drift. A worktree whose directory is already gone holds nothing to save. +pub fn unsaved_work(path: &Path) -> Result { + if std::fs::symlink_metadata(path).is_err() { + return Ok(maestro_protocol::UnsavedWork::default()); } - Ok(None) + // `--ignored=no` keeps build output out of it; ignored files never block + // (#79), or no Rust checkout would ever be killable. + let out = run_git(path, &["status", "--porcelain", "--ignored=no"])?; + let status = String::from_utf8_lossy(&out.stdout); + let mut dirty = 0u32; + let mut untracked = 0u32; + for line in status.lines() { + if line.starts_with("??") { + untracked += 1; + } else { + dirty += 1; + } + } + + // Detached HEAD has no branch; the tip itself still works as a starting + // point for the reachability query below. + let head = run_git(path, &["rev-parse", "--abbrev-ref", "HEAD"])?; + let branch = String::from_utf8_lossy(&head.stdout).trim().to_string(); + let tip: &str = if branch == "HEAD" { "HEAD" } else { &branch }; + let count = run_git(path, &["rev-list", "--count", tip, "--not", "--remotes"])?; + let unpushed: u32 = String::from_utf8_lossy(&count.stdout) + .trim() + .parse() + .map_err(|e| WorktreeError::NonZero { + code: 0, + stderr: format!("unparseable rev-list count: {e}"), + })?; + + Ok(maestro_protocol::UnsavedWork { + dirty_tracked_files: dirty, + untracked_files: untracked, + unpushed_commits: unpushed, + }) } /// `git -C worktree prune` — drops registrations whose directory @@ -280,10 +304,7 @@ fn prune(base_repo: &Path) -> Result<(), WorktreeError> { check(out) } -fn run_git( - dir: &Path, - args: impl IntoIterator, -) -> Result { +fn run_git(dir: &Path, args: &[&str]) -> Result { let mut cmd = Command::new("git"); cmd.arg("-C").arg(dir).args(args); let out = cmd.output()?; diff --git a/crates/maestro-daemon/tests/common/mod.rs b/crates/maestro-daemon/tests/common/mod.rs index cf077cf..fc382d7 100644 --- a/crates/maestro-daemon/tests/common/mod.rs +++ b/crates/maestro-daemon/tests/common/mod.rs @@ -93,6 +93,38 @@ fn run(cmd: &mut Command) { ); } +/// An empty bare repository at `dir`, usable as a push target (#79). +pub fn init_bare_repo(dir: &Path) { + run(Command::new("git") + .arg("init") + .arg("-q") + .arg("--bare") + .arg(dir.to_str().expect("non-utf8 bare repo path"))); +} + +/// Push every branch of `repo` to `origin`, creating remote-tracking refs. +/// +/// The unsaved-work probe (#79) counts commits reachable from no +/// remote-tracking ref as unpushed, so tests that want a killable worktree +/// must give its branch one. Safe to call again: re-adding an existing +/// remote is ignored. +pub fn push_all_to_origin(repo: &Path, origin: &Path) { + let _ = Command::new("git") + .arg("-C") + .arg(repo) + .args([ + "remote", + "add", + "origin", + origin.to_str().expect("non-utf8 origin path"), + ]) + .status(); + run(Command::new("git") + .arg("-C") + .arg(repo) + .args(["push", "-q", "--all", "origin"])); +} + /// Async polling sibling of [`wait_for`] for tokio tests. pub async fn wait_for_async(timeout: Duration, mut cond: F) where diff --git a/crates/maestro-daemon/tests/worktree.rs b/crates/maestro-daemon/tests/worktree.rs index 36615a5..5dd2967 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/crates/maestro-daemon/tests/worktree.rs @@ -6,8 +6,8 @@ mod common; use std::path::PathBuf; use std::process::Command; -use common::init_repo; -use maestro_daemon::worktree::WorktreeManager; +use common::{init_bare_repo, init_repo, push_all_to_origin}; +use maestro_daemon::worktree::{unsaved_work, WorktreeManager}; use tempfile::TempDir; fn worktree_branches(repo: &PathBuf) -> Vec { @@ -134,6 +134,123 @@ fn add_twice_generates_distinct_paths() { assert!(p2.exists()); } +/// The unsaved-work probe (#79) counts uncommitted tracked changes. +#[test] +fn the_probe_counts_uncommitted_tracked_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(); + + let w = unsaved_work(&path).expect("probe"); + assert_eq!(w.dirty_tracked_files, 1); + assert_eq!(w.untracked_files, 0); +} + +/// Untracked sources count; a staged new file is also unsaved tracked work. +#[test] +fn the_probe_counts_untracked_files() { + 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/untracked").expect("add"); + std::fs::write(path.join("notes.txt"), b"wip\n").unwrap(); + std::fs::write(path.join("more.txt"), b"also wip\n").unwrap(); + + let w = unsaved_work(&path).expect("probe"); + assert_eq!(w.untracked_files, 2); + assert_eq!(w.dirty_tracked_files, 0); + + run_git(&path, &["add", "notes.txt"]); + let w = unsaved_work(&path).expect("probe"); + assert_eq!(w.dirty_tracked_files, 1, "staged counts as uncommitted"); + assert_eq!(w.untracked_files, 1); +} + +/// Ignored files NEVER block: every Rust checkout has a `target/`, and +/// counting ignored output would leave no session killable (#79). +#[test] +fn the_probe_ignores_ignored_files() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + // .gitignore must be committed, or it would itself read as untracked. + std::fs::write(repo.join(".gitignore"), b"/target\n").unwrap(); + run_git(&repo, &["add", ".gitignore"]); + run_git(&repo, &["commit", "-q", "-m", "ignore target"]); + + let mgr = WorktreeManager::new(None); + let path = mgr.add_new(&repo, "feat/builds").expect("add"); + std::fs::create_dir_all(path.join("target")).unwrap(); + std::fs::write(path.join("target").join("app.o"), b"binary\n").unwrap(); + + let w = unsaved_work(&path).expect("probe"); + assert_eq!(w.dirty_tracked_files, 0); + assert_eq!(w.untracked_files, 0); + + // With the branch pushed too, an ignored-only tree reads fully clean. + let origin_dir = TempDir::new().unwrap(); + init_bare_repo(origin_dir.path()); + push_all_to_origin(&repo, origin_dir.path()); + let w = unsaved_work(&path).expect("probe after push"); + assert!(w.is_empty(), "ignored-only worktree must read clean: {w:?}"); +} + +/// Commits reachable from no remote-tracking ref block; pushing the branch +/// clears them. With no remote at all every commit is unreachable from one, +/// which is exactly why feature branches almost always refuse (#79). +#[test] +fn the_probe_counts_unpushed_commits_until_the_branch_is_pushed() { + 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/branch").expect("add"); + + let w = unsaved_work(&path).expect("probe"); + assert!( + w.unpushed_commits >= 1, + "a fresh branch has no remote-tracking ref: {w:?}" + ); + + let origin_dir = TempDir::new().unwrap(); + init_bare_repo(origin_dir.path()); + push_all_to_origin(&repo, origin_dir.path()); + + let w = unsaved_work(&path).expect("probe after push"); + assert_eq!(w.unpushed_commits, 0, "{w:?}"); + assert!(w.is_empty(), "pushed and clean must be empty: {w:?}"); +} + +/// The full breakdown names each blocker with its count. +#[test] +fn the_probe_reports_every_blocker_at_once() { + 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/mixed").expect("add"); + std::fs::write(path.join("README"), b"edited\n").unwrap(); + std::fs::write(path.join("scratch.txt"), b"untracked\n").unwrap(); + + let w = unsaved_work(&path).expect("probe"); + assert_eq!(w.dirty_tracked_files, 1); + assert_eq!(w.untracked_files, 1); + assert!(w.unpushed_commits >= 1); + let summary = w.to_string(); + assert!(summary.contains("1 uncommitted file"), "{summary}"); + assert!(summary.contains("1 untracked file"), "{summary}"); + assert!(summary.contains("unpushed commit"), "{summary}"); +} + #[test] fn remove_prunes_worktree() { let repo_dir = TempDir::new().unwrap(); @@ -211,8 +328,14 @@ fn add_existing_checks_out_branch_without_creating_it() { ); } +/// Removal is guarded upstream now (#79): the kill refuses unsaved work +/// before anything is torn down, so by the time `remove` runs the tree is +/// clean. Git still refuses a dirty tree on its own — this asserts that last +/// line of defense — and committed-but-unpushed work does not block removal, +/// because `worktree remove` keeps `refs/heads/` and the commits stay +/// reachable. #[test] -fn remove_refuses_worktree_with_uncommitted_changes() { +fn remove_still_relies_on_git_refusing_a_dirty_tree() { let repo_dir = TempDir::new().unwrap(); let repo = repo_dir.path().to_path_buf(); init_repo(&repo); @@ -225,10 +348,6 @@ fn remove_refuses_worktree_with_uncommitted_changes() { .expect_err("dirty worktree must be refused"); assert!(path.exists(), "guarded worktree must stay on disk"); - // Committing it makes removal safe: `worktree remove` keeps - // `refs/heads/feat/dirty`, so the work is still reachable. Refusing here - // instead would make every unpushed branch a one-shot, since the path is - // deterministic and the leftover tree blocks the next spawn (#56 review). run_git(&path, &["add", "."]); run_git(&path, &["commit", "-q", "-m", "saved"]); @@ -410,7 +529,8 @@ fn a_guarded_leftover_is_never_adopted() { let first = mgr.add_new(&repo, "feat/resume").expect("add"); std::fs::write(first.join("notes.txt"), b"unsaved\n").unwrap(); - // The guard refuses, leaving it on disk. + // Removal is refused — git itself refuses a dirty tree — leaving it on + // disk; the daemon-side guard lives on the kill (#79). mgr.remove(&repo, &first) .expect_err("dirty tree is guarded"); diff --git a/crates/maestro-protocol/src/lib.rs b/crates/maestro-protocol/src/lib.rs index 77979d8..c79204b 100644 --- a/crates/maestro-protocol/src/lib.rs +++ b/crates/maestro-protocol/src/lib.rs @@ -143,6 +143,62 @@ pub struct SessionWorktree { pub base_repo: PathBuf, } +// --------------------------------------------------------------------------- +// Unsaved-work breakdown (#79) +// --------------------------------------------------------------------------- + +/// What a worktree holds that would be lost if its directory were destroyed. +/// +/// The kill guard refuses `DELETE /api/sessions/:id` with this breakdown, and +/// natural exits (#82) classify with the same daemon-side probe, so the two +/// paths cannot drift. Ignored files never count: every Rust checkout has a +/// `target/`, and counting those would leave no session killable. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct UnsavedWork { + /// Tracked files with uncommitted modifications or staged changes. + #[serde(default)] + pub dirty_tracked_files: u32, + /// Untracked files that are not ignored. + #[serde(default)] + pub untracked_files: u32, + /// Commits on the branch reachable from no remote-tracking ref. + #[serde(default)] + pub unpushed_commits: u32, +} + +impl UnsavedWork { + pub fn is_empty(&self) -> bool { + self.dirty_tracked_files == 0 && self.untracked_files == 0 && self.unpushed_commits == 0 + } +} + +impl std::fmt::Display for UnsavedWork { + /// "3 uncommitted files, 2 untracked files, 2 unpushed commits" — zero + /// parts are omitted; all-zero renders "no unsaved work". + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let parts = [ + (self.dirty_tracked_files, "uncommitted file"), + (self.untracked_files, "untracked file"), + (self.unpushed_commits, "unpushed commit"), + ]; + let said: Vec = parts + .into_iter() + .filter(|(n, _)| *n > 0) + .map(|(n, unit)| { + if n == 1 { + format!("1 {unit}") + } else { + format!("{n} {unit}s") + } + }) + .collect(); + if said.is_empty() { + return write!(f, "no unsaved work"); + } + write!(f, "{}", said.join(", ")) + } +} + /// HTTP `POST /api/sessions` body. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CreateSessionRequest { @@ -248,6 +304,10 @@ pub struct BranchInfo { pub struct ErrorInfo { pub code: String, pub message: String, + /// Set only by the kill guard (#79): the unsaved work that blocked + /// `DELETE /api/sessions/:id`, so a client can render counts directly. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub unsaved_work: Option, } /// The envelope every `/api/*` failure is wrapped in: `{"error": {…}}`. @@ -268,6 +328,7 @@ impl ApiError { error: ErrorInfo { code: code.into(), message: message.into(), + unsaved_work: None, }, } } @@ -578,6 +639,7 @@ mod tests { ClientControl::Error(ErrorInfo { code: "bad_frame".into(), message: "decode failed".into(), + unsaved_work: None, }), resize, ] { From a575b8bef1558a2844be99ac64590ccc23b03f25 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Mon, 24 Aug 2026 11:03:43 +0100 Subject: [PATCH 2/2] feat(maestro): refuse a kill when the worktree holds unsaved work (#79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard moves off the worktree and onto the kill. DELETE /api/sessions/:id now runs the shared unsaved_work probe before tearing anything down and answers 409 kill_guarded with the breakdown on the wire, so a client can say "3 uncommitted files, 2 unpushed commits". The session keeps running and the tree is untouched. WorktreeManager::remove is unconditional now — git itself still refuses a dirty tree as a last line of defense. --- crates/maestro-daemon/src/dirlist.rs | 1 + crates/maestro-daemon/src/local_api.rs | 13 +- crates/maestro-daemon/src/session.rs | 58 +++++-- crates/maestro-daemon/tests/local_api.rs | 49 +++++- .../maestro-daemon/tests/session_manager.rs | 155 ++++++++++++++++-- crates/maestro-e2e/tests/e2e.rs | 58 +++++-- 6 files changed, 295 insertions(+), 39 deletions(-) diff --git a/crates/maestro-daemon/src/dirlist.rs b/crates/maestro-daemon/src/dirlist.rs index cae1c96..19a14ec 100644 --- a/crates/maestro-daemon/src/dirlist.rs +++ b/crates/maestro-daemon/src/dirlist.rs @@ -111,6 +111,7 @@ fn io_error(dir: &Path, e: &std::io::Error) -> ErrorInfo { ErrorInfo { code: code.into(), message: format!("{message}: {}", dir.display()), + unsaved_work: None, } } diff --git a/crates/maestro-daemon/src/local_api.rs b/crates/maestro-daemon/src/local_api.rs index 69a4494..089400c 100644 --- a/crates/maestro-daemon/src/local_api.rs +++ b/crates/maestro-daemon/src/local_api.rs @@ -94,6 +94,8 @@ fn session_error_status(code: &str) -> StatusCode { match code { "not_found" => StatusCode::NOT_FOUND, "invalid_env" => StatusCode::BAD_REQUEST, + // A guard refusal is the daemon doing its job, not a failure (#79). + "kill_guarded" => StatusCode::CONFLICT, _ => StatusCode::BAD_GATEWAY, } } @@ -168,16 +170,17 @@ async fn create_session( /// `kill` already reports a missing session as `SessionError::NotFound`, which /// maps to 404. Checking first would only add a window in which the liveness /// watcher reaps the session between the two awaits, turning a 404 into a 502. +/// +/// The full error info is passed through rather than rebuilt from +/// code + message: a `kill_guarded` refusal (#79) carries the unsaved-work +/// breakdown clients render as "3 uncommitted files, 2 unpushed commits". async fn kill_session(State(s): State, Path(id): Path) -> Response { match s.manager.kill(id).await { Ok(()) => StatusCode::NO_CONTENT.into_response(), Err(e) => { let info = e.to_error_info(); - error_response( - session_error_status(&info.code), - info.code.as_str(), - info.message.as_str(), - ) + let status = session_error_status(&info.code); + (status, Json(ApiError { error: info })).into_response() } } } diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 72ae8f9..04fd3aa 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -38,8 +38,8 @@ 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; removed on kill unless the - /// dirty-work guard refuses (#56). + /// Worktree we created at spawn time; removed on kill, but only after the + /// kill guard (#79) found nothing worth keeping in it. pub worktree_path: Option, /// `base_repo` from the original `WorktreeSpec`. Needed because `git /// worktree remove` is invoked from the base repo. @@ -61,6 +61,11 @@ pub enum SessionError { NotFound(Uuid), #[error("invalid environment variable name: {0:?}")] InvalidEnv(String), + /// The kill was refused (#79): the session's worktree holds unsaved work. + /// Nothing happened — the session keeps running and the tree is untouched, + /// so it stays the operator's handle on their own unfinished work. + #[error("kill refused: the session's worktree holds unsaved work ({0})")] + KillGuarded(maestro_protocol::UnsavedWork), } impl SessionError { @@ -72,10 +77,16 @@ impl SessionError { SessionError::Io(_) => "io_failed", SessionError::NotFound(_) => "not_found", SessionError::InvalidEnv(_) => "invalid_env", + SessionError::KillGuarded(_) => "kill_guarded", + }; + let unsaved_work = match self { + SessionError::KillGuarded(b) => Some(*b), + _ => None, }; ErrorInfo { code: code.into(), message: self.to_string(), + unsaved_work, } } } @@ -370,20 +381,48 @@ impl SessionManager { } } - /// 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). + /// Kill a session and remove its on-disk artifacts. + /// + /// Refused (#79) when the session's worktree holds unsaved work — + /// uncommitted tracked changes, untracked non-ignored files, or commits + /// reachable from no remote-tracking ref. Nothing is killed and nothing + /// is removed; the error names what blocked it and how much of it there + /// is. The session stays as the operator's handle on their own unfinished + /// work, so ending a session that still matters becomes a deliberate + /// `--force`, which retains rather than destroys (WORKTREES.md). /// /// 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 /// session tmux already lost fails, and that failure is not interesting. pub async fn kill(&self, id: Uuid) -> Result<(), SessionError> { + let (worktree, alive) = { + let sessions = self.inner.sessions.lock().await; + match sessions.get(&id) { + None => return Err(SessionError::NotFound(id)), + Some(record) => ( + record + .worktree_base_repo + .clone() + .zip(record.worktree_path.clone()), + record.session.status == SessionStatus::Running, + ), + } + }; + // The guard runs before anything is torn down: refusing must leave the + // session running, exactly as the operator left it. + if let Some((_, path)) = worktree { + let probe = tmux_blocking(move || crate::worktree::unsaved_work(&path)) + .await + .map_err(SessionError::Worktree)?; + if !probe.is_empty() { + return Err(SessionError::KillGuarded(probe)); + } + } let removed = { let mut sessions = self.inner.sessions.lock().await; sessions.remove(&id) }; let record = removed.ok_or(SessionError::NotFound(id))?; - let alive = record.session.status == SessionStatus::Running; cleanup_record(&self.inner, id, record, alive).await; Ok(()) } @@ -720,10 +759,9 @@ 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"); } - // 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. + // The kill already passed the unsaved-work guard (#79), so this removal + // is unconditional; the manager's own nested-worktree refusal is all that + // can still leave a tree on disk, and it says where. 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!( diff --git a/crates/maestro-daemon/tests/local_api.rs b/crates/maestro-daemon/tests/local_api.rs index b04abeb..8d9b2d4 100644 --- a/crates/maestro-daemon/tests/local_api.rs +++ b/crates/maestro-daemon/tests/local_api.rs @@ -9,7 +9,7 @@ use std::time::Duration; use common::DaemonFixture; use futures_util::{SinkExt, StreamExt}; use maestro_daemon::local_api; -use maestro_protocol::{CreateSessionRequest, Session}; +use maestro_protocol::{ApiError, CreateSessionRequest, Session, WorktreeSpec}; use tokio::net::TcpListener; use tokio::sync::oneshot; use tokio_tungstenite::tungstenite::Message as WsMessage; @@ -338,6 +338,53 @@ async fn delete_owned_session_kills_it() { assert!(!h.manager.has(session.id).await); } +/// The kill guard on the wire (#79): `DELETE /api/sessions/:id` answers 409 +/// with the structured breakdown, and the session keeps running. +#[tokio::test] +async fn delete_a_session_whose_worktree_holds_unsaved_work_returns_409_with_the_breakdown() { + let h = spawn("naps-laptop").await; + let repo = tempfile::TempDir::new().unwrap(); + common::init_repo(repo.path()); + let session = h + .manager + .create(CreateSessionRequest { + cwd: repo.path().to_path_buf(), + command: "sleep 60".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 + .unwrap(); + std::fs::write( + repo.path() + .join("worktrees") + .join("feat") + .join("guarded") + .join("README"), + b"unsaved edits\n", + ) + .unwrap(); + + let resp = reqwest::Client::new() + .delete(url(h.addr, &format!("/api/sessions/{}", session.id))) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), reqwest::StatusCode::CONFLICT); + let body: ApiError = resp.json().await.unwrap(); + assert_eq!(body.error.code, "kill_guarded"); + let breakdown = body.error.unsaved_work.expect("breakdown on the wire"); + assert_eq!(breakdown.dirty_tracked_files, 1); + + assert!(h.manager.has(session.id).await, "the session keeps running"); +} + #[tokio::test] async fn attach_ws_streams_pty_bytes() { let h = spawn("naps-laptop").await; diff --git a/crates/maestro-daemon/tests/session_manager.rs b/crates/maestro-daemon/tests/session_manager.rs index 15e9aa2..e9c5ad7 100644 --- a/crates/maestro-daemon/tests/session_manager.rs +++ b/crates/maestro-daemon/tests/session_manager.rs @@ -4,10 +4,12 @@ mod common; use std::collections::HashMap; use std::path::PathBuf; +use std::process::Command; use std::sync::Arc; use std::time::Duration; -use common::{init_repo, wait_for_async, DaemonFixture}; +use common::{init_bare_repo, init_repo, push_all_to_origin, wait_for_async, DaemonFixture}; +use maestro_daemon::SessionError; use maestro_protocol::CreateSessionRequest; use maestro_protocol::{CatalogEvent, SessionStatus, WorktreeSpec}; use tempfile::TempDir; @@ -363,8 +365,9 @@ 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. +/// Cleanup on kill is unconditional once the guard passes (#79): even a spec +/// that says "don't clean up" loses its worktree — the branch was pushed, so +/// nothing in the tree is unique and removal goes ahead. #[tokio::test] async fn kill_removes_worktree_even_when_spec_says_keep() { let fx = DaemonFixture::new(); @@ -390,14 +393,21 @@ async fn kill_removes_worktree_even_when_spec_says_keep() { .expect("create"); assert!(wt_path.exists()); + // Push the branch so its commits are reachable from a remote-tracking ref. + let origin = TempDir::new().unwrap(); + init_bare_repo(origin.path()); + push_all_to_origin(repo.path(), origin.path()); + 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. +/// The kill guard (#79): a worktree holding uncommitted changes refuses the +/// kill outright. Nothing happens — the session keeps running as the +/// operator's handle on their unfinished work, and the tree is untouched. +/// The refusal names what blocked it and how much of it there is. #[tokio::test] -async fn kill_leaves_worktree_with_unsaved_work_on_disk() { +async fn killing_refuses_a_worktree_with_unsaved_work_and_keeps_the_session() { let fx = DaemonFixture::new(); let repo = TempDir::new().unwrap(); init_repo(repo.path()); @@ -420,13 +430,138 @@ async fn kill_leaves_worktree_with_unsaved_work_on_disk() { .await .expect("create"); std::fs::write(wt_path.join("README"), b"unsaved edits\n").unwrap(); + std::fs::write(wt_path.join("notes.txt"), b"untracked\n").unwrap(); - fx.manager.kill(s.id).await.expect("kill"); + let err = fx.manager.kill(s.id).await.expect_err("kill must refuse"); + let SessionError::KillGuarded(breakdown) = err else { + panic!("expected KillGuarded, got {err:?}"); + }; + assert_eq!(breakdown.dirty_tracked_files, 1); + assert_eq!(breakdown.untracked_files, 1); assert!( - wt_path.exists(), - "dirty worktree must be left on disk, not destroyed" + breakdown.unpushed_commits >= 1, + "a fresh branch has no remote-tracking ref" + ); + let message = breakdown.to_string(); + assert!(message.contains("1 uncommitted file"), "{message}"); + assert!(message.contains("1 untracked file"), "{message}"); + + // Nothing happened: the session survives and so does the tree. + assert!(fx.manager.has(s.id).await, "the session must keep running"); + assert!(wt_path.exists(), "the worktree must be untouched"); + + // A second kill after the work is saved goes through. + run_git_in(&wt_path, &["add", "."]); + run_git_in(&wt_path, &["commit", "-q", "-m", "saved"]); + let origin = TempDir::new().unwrap(); + init_bare_repo(origin.path()); + push_all_to_origin(repo.path(), origin.path()); + fx.manager.kill(s.id).await.expect("kill after saving"); + assert!(!fx.manager.has(s.id).await); +} + +/// Unpushed commits alone block the kill (#79): a feature branch almost +/// always has them, which is why `--force` (which retains) is the ordinary +/// way to end such a session. +#[tokio::test] +async fn killing_refuses_until_the_branch_is_pushed() { + let fx = DaemonFixture::new(); + let repo = TempDir::new().unwrap(); + init_repo(repo.path()); + let wt_path = repo.path().join("worktrees").join("feat").join("pushed"); + + 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/pushed".into(), + cleanup_on_kill: true, + create_branch: true, + }), + }) + .await + .expect("create"); + + let err = fx.manager.kill(s.id).await.expect_err("unpushed blocks"); + let SessionError::KillGuarded(breakdown) = err else { + panic!("expected KillGuarded, got {err:?}"); + }; + assert_eq!(breakdown.dirty_tracked_files, 0); + assert_eq!(breakdown.untracked_files, 0); + assert!(breakdown.unpushed_commits >= 1); + + let origin = TempDir::new().unwrap(); + init_bare_repo(origin.path()); + push_all_to_origin(repo.path(), origin.path()); + + fx.manager.kill(s.id).await.expect("pushed branch kills"); + assert!(!wt_path.exists()); + assert!(!fx.manager.has(s.id).await); +} + +/// Ignored files NEVER block (#79): build output like `target/` exists in +/// every Rust checkout, and an ignored-only worktree stays killable. +#[tokio::test] +async fn killing_an_ignored_only_worktree_succeeds() { + let fx = DaemonFixture::new(); + let repo = TempDir::new().unwrap(); + init_repo(repo.path()); + // .gitignore must be committed before the worktree branches off, or it + // would itself read as an untracked file. + std::fs::write(repo.path().join(".gitignore"), b"/target\n").unwrap(); + run_git_in(repo.path(), &["add", ".gitignore"]); + run_git_in(repo.path(), &["commit", "-q", "-m", "ignore target"]); + let origin = TempDir::new().unwrap(); + init_bare_repo(origin.path()); + + let wt_path = repo.path().join("worktrees").join("feat").join("rusty"); + 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/rusty".into(), + cleanup_on_kill: true, + create_branch: true, + }), + }) + .await + .expect("create"); + std::fs::create_dir_all(wt_path.join("target")).unwrap(); + std::fs::write(wt_path.join("target").join("app.o"), b"binary\n").unwrap(); + + // Branch pushed; the only dirt is ignored output. + push_all_to_origin(repo.path(), origin.path()); + + fx.manager + .kill(s.id) + .await + .expect("ignored files never block"); + assert!(!wt_path.exists()); + assert!(!fx.manager.has(s.id).await); +} + +fn run_git_in(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) ); - assert!(fx.manager.snapshot().await.is_empty()); } #[tokio::test] diff --git a/crates/maestro-e2e/tests/e2e.rs b/crates/maestro-e2e/tests/e2e.rs index cbb213a..b049014 100644 --- a/crates/maestro-e2e/tests/e2e.rs +++ b/crates/maestro-e2e/tests/e2e.rs @@ -160,6 +160,7 @@ async fn kill_removes_clean_worktree() { "expected a worktree for feature/e2e under /worktrees, got: {worktrees:?}" ); + push_to_throwaway_origin(base.path()); client.kill_session(s.id).await.expect("kill"); // Give the daemon a moment to remove the worktree, then verify. @@ -177,11 +178,12 @@ async fn kill_removes_clean_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. +/// The kill guard (#79): killing a session whose worktree holds uncommitted +/// changes is refused outright. Nothing happens — the session keeps running +/// as the operator's handle on their unfinished work, the tree stays on disk, +/// and the refusal names what blocked it and how much. #[tokio::test(flavor = "multi_thread")] -async fn kill_spares_worktree_with_uncommitted_changes() { +async fn kill_refuses_worktree_with_uncommitted_changes() { let (_daemon, client) = bring_up().await; let base = TempDir::new().unwrap(); @@ -209,17 +211,25 @@ async fn kill_spares_worktree_with_uncommitted_changes() { 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; + let err = client + .kill_session(s.id) + .await + .expect_err("kill must refuse"); + match err { + maestro_e2e::ClientError::Api(maestro_protocol::ClientError::Api { info, .. }) => { + assert_eq!(info.code, "kill_guarded", "{info:?}"); + let breakdown = info.unsaved_work.expect("breakdown on the wire"); + assert_eq!(breakdown.dirty_tracked_files, 1); } - tokio::time::sleep(Duration::from_millis(100)).await; + other => panic!("expected a kill_guarded refusal, got {other:?}"), } + + // The session survives and the dirty worktree stays findable on disk. + let sessions = client.list_sessions().await.unwrap_or_default(); + assert!( + sessions.iter().any(|x| x.id == s.id), + "a refused kill must keep the session alive" + ); assert!( wt_path.exists(), "guarded worktree must be left on disk after kill" @@ -270,6 +280,7 @@ async fn spawn_on_existing_branch_checks_it_out() { "expected a worktree checked out at feature/existing, got: {worktrees:?}" ); + push_to_throwaway_origin(base.path()); client.kill_session(s.id).await.expect("kill"); } @@ -317,6 +328,7 @@ async fn spawn_new_branch_creates_worktree_and_runs_in_it() { ) .await; + push_to_throwaway_origin(base.path()); client.kill_session(s.id).await.expect("kill"); } @@ -364,6 +376,7 @@ async fn second_spawn_on_same_branch_is_rejected() { "original worktree must be untouched" ); + push_to_throwaway_origin(base.path()); client.kill_session(first.id).await.expect("kill"); } @@ -406,6 +419,7 @@ async fn spawn_from_linked_worktree_places_in_main_checkout() { "worktrees must never nest inside another worktree" ); + push_to_throwaway_origin(base.path()); client.kill_session(s.id).await.expect("kill"); } @@ -774,6 +788,24 @@ fn git_worktree_list(dir: &std::path::Path) -> Vec { .collect() } +/// Push every branch of `repo` to a throwaway bare remote, so its commits are +/// reachable from remote-tracking refs. Without one the kill guard (#79) +/// reads every commit as unpushed, and any worktree session refuses to die. +fn push_to_throwaway_origin(repo: &std::path::Path) { + let origin = tempfile::TempDir::new().unwrap(); + run_git( + &["init", "-q", "--bare", origin.path().to_str().unwrap()], + repo, + ); + let _ = Command::new("git") + .arg("-C") + .arg(repo) + .args(["remote", "add", "origin", origin.path().to_str().unwrap()]) + .output() + .expect("git remote add"); + run_git(&["push", "-q", "--all", "origin"], repo); +} + #[tokio::test(flavor = "multi_thread")] async fn daemon_events_ws_streams_snapshot_then_session_added() { // NAP-248: subscribe to the daemon's WS /api/events, expect an initial