diff --git a/crates/maestro-e2e/src/client.rs b/crates/maestro-e2e/src/client.rs index f89cd2c..6ba4410 100644 --- a/crates/maestro-e2e/src/client.rs +++ b/crates/maestro-e2e/src/client.rs @@ -12,7 +12,8 @@ use std::time::Duration; use futures_util::{SinkExt, StreamExt}; use maestro_protocol::{ - BranchInfo, CatalogEvent, CreateSessionRequest, DaemonClient, Project, Session, + BranchInfo, CatalogEvent, CreateSessionRequest, DaemonClient, KillOutcome, Project, + RetainedWorktree, Session, }; use tokio_tungstenite::tungstenite::client::IntoClientRequest; use tokio_tungstenite::tungstenite::Message as WsMessage; @@ -64,6 +65,20 @@ impl TestClient { Ok(self.daemon.kill_session(id, false).await.map(|_| ())?) } + /// Kill with `force=true`: overrides a kill refusal and retains the + /// guarded worktree (#81). Answers with what was kept. + pub async fn kill_session_forced(&self, id: Uuid) -> Result { + Ok(self.daemon.kill_session(id, true).await?) + } + + pub async fn list_worktrees(&self) -> Result, ClientError> { + Ok(self.daemon.list_worktrees().await?) + } + + pub async fn reclaim_worktree(&self, id: Uuid) -> Result<(), ClientError> { + Ok(self.daemon.reclaim_worktree(id).await?) + } + pub async fn list_projects(&self) -> Result, ClientError> { Ok(self.daemon.list_projects().await?) } diff --git a/crates/maestro-e2e/tests/e2e.rs b/crates/maestro-e2e/tests/e2e.rs index af64f16..be5e092 100644 --- a/crates/maestro-e2e/tests/e2e.rs +++ b/crates/maestro-e2e/tests/e2e.rs @@ -35,6 +35,17 @@ fn create_req(cwd: &std::path::Path, command: &str) -> CreateSessionRequest { } } +/// A worktree spec built through its wire shape. The daemon and clients ship +/// together (#87 dropped `cleanup_on_kill` from the struct), so the JSON +/// object carries only the fields the lifecycle actually honours. +fn worktree_spec(base_repo: &std::path::Path, branch: &str, create_branch: bool) -> WorktreeSpec { + WorktreeSpec { + base_repo: base_repo.to_path_buf(), + branch: branch.into(), + create_branch, + } +} + /// Spawn request carrying a worktree spec against `base`. fn worktree_req(base: &std::path::Path, branch: &str, create_branch: bool) -> CreateSessionRequest { CreateSessionRequest { @@ -42,11 +53,7 @@ fn worktree_req(base: &std::path::Path, branch: &str, create_branch: bool) -> Cr command: "sleep 30".into(), env: HashMap::new(), metadata: HashMap::new(), - worktree: Some(WorktreeSpec { - base_repo: base.to_path_buf(), - branch: branch.into(), - create_branch, - }), + worktree: Some(worktree_spec(base, branch, create_branch)), } } @@ -140,11 +147,7 @@ async fn kill_removes_clean_worktree() { command: "sleep 30".into(), env: HashMap::new(), metadata: HashMap::new(), - worktree: Some(WorktreeSpec { - base_repo: base.path().to_path_buf(), - branch: "feature/e2e".into(), - create_branch: true, - }), + worktree: Some(worktree_spec(base.path(), "feature/e2e", true)), }; let s = client.spawn_session(&req).await.expect("spawn worktree"); @@ -158,7 +161,7 @@ async fn kill_removes_clean_worktree() { "expected a worktree for feature/e2e under /worktrees, got: {worktrees:?}" ); - push_to_throwaway_origin(base.path()); + let _origin = throwaway_origin(base.path()); client.kill_session(s.id).await.expect("kill"); // Give the daemon a moment to remove the worktree, then verify. @@ -197,11 +200,7 @@ async fn kill_refuses_worktree_with_uncommitted_changes() { command: "sleep 30".into(), env: HashMap::new(), metadata: HashMap::new(), - worktree: Some(WorktreeSpec { - base_repo: base.path().to_path_buf(), - branch: "feature/unsaved".into(), - create_branch: true, - }), + worktree: Some(worktree_spec(base.path(), "feature/unsaved", true)), }; let s = client.spawn_session(&req).await.expect("spawn worktree"); @@ -259,11 +258,7 @@ async fn spawn_on_existing_branch_checks_it_out() { command: "sleep 30".into(), env: HashMap::new(), metadata: HashMap::new(), - worktree: Some(WorktreeSpec { - base_repo: base.path().to_path_buf(), - branch: "feature/existing".into(), - create_branch: false, - }), + worktree: Some(worktree_spec(base.path(), "feature/existing", false)), }; let s = client @@ -276,7 +271,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()); + let _origin = throwaway_origin(base.path()); client.kill_session(s.id).await.expect("kill"); } @@ -324,7 +319,7 @@ async fn spawn_new_branch_creates_worktree_and_runs_in_it() { ) .await; - push_to_throwaway_origin(base.path()); + let _origin = throwaway_origin(base.path()); client.kill_session(s.id).await.expect("kill"); } @@ -372,7 +367,7 @@ async fn second_spawn_on_same_branch_is_rejected() { "original worktree must be untouched" ); - push_to_throwaway_origin(base.path()); + let _origin = throwaway_origin(base.path()); client.kill_session(first.id).await.expect("kill"); } @@ -415,10 +410,481 @@ async fn spawn_from_linked_worktree_places_in_main_checkout() { "worktrees must never nest inside another worktree" ); - push_to_throwaway_origin(base.path()); + let _origin = throwaway_origin(base.path()); client.kill_session(s.id).await.expect("kill"); } +// --------------------------------------------------------------------------- +// Worktree lifecycle: collisions, retention, reclaim (#88) +// --------------------------------------------------------------------------- + +/// #77/#88: the target path `/worktrees/` is occupied by a real +/// registered worktree the operator made by hand. The daemon sidesteps to the +/// suffixed directory and never touches the occupant — and both trees keep +/// working: two registered worktrees, two sessions, each running in its own. +#[tokio::test(flavor = "multi_thread")] +async fn occupied_path_gets_suffixed_directory_and_both_worktrees_work() { + let (_daemon, client) = bring_up().await; + + let base = TempDir::new().unwrap(); + init_repo(base.path()); + + // An operator-made worktree sits exactly where `feature/collide` would go. + // Its branch is a different one, so the requested branch stays creatable. + let squatter = base + .path() + .join("worktrees") + .join("feature") + .join("collide"); + run_git( + &[ + "worktree", + "add", + "-q", + "-b", + "operator/manual", + &squatter.to_string_lossy(), + ], + base.path(), + ); + + let s = client + .spawn_session(&worktree_req(base.path(), "feature/collide", true)) + .await + .expect("spawn onto occupied path"); + + let suffixed = base + .path() + .join("worktrees") + .join("feature") + .join("collide-2"); + assert_eq!( + s.cwd, suffixed, + "the daemon must take the suffixed directory, not the occupied one" + ); + assert!(suffixed.is_dir(), "suffixed worktree must exist on disk"); + + // Both trees are registered, distinct, and the squatter is byte-intact. + let registrations = git_worktree_list(base.path()); + assert!( + registrations + .iter() + .any(|l| l == &format!("worktree {}", squatter.canonicalize().unwrap().display())), + "operator's worktree must stay registered: {registrations:?}" + ); + assert!( + registrations.iter().any(|l| l.contains("collide-2")), + "suffixed worktree must be registered too: {registrations:?}" + ); + + // Both sessions run inside their own tree at the same time. + let sessions = client.list_sessions().await.expect("list"); + assert!( + sessions.iter().any(|x| x.id == s.id && x.cwd == suffixed), + "session must keep running in its suffixed tree" + ); + + let _origin = throwaway_origin(base.path()); + client.kill_session(s.id).await.expect("kill"); + + // Only the daemon-made trees go; the operator's manual one survives. + let after = git_worktree_list(base.path()); + assert!( + after + .iter() + .any(|l| l == &format!("worktree {}", squatter.canonicalize().unwrap().display())), + "operator's worktree must outlive the sessions: {after:?}" + ); +} + +/// #77/#88: the target path holds a plain directory the daemon never made and +/// git has never heard of. The daemon neither adopts it nor errors — it builds +/// beside it and leaves every byte of it alone. +#[tokio::test(flavor = "multi_thread")] +async fn foreign_directory_is_sidestepped_never_adopted() { + let (_daemon, client) = bring_up().await; + + let base = TempDir::new().unwrap(); + init_repo(base.path()); + + let squatter = base + .path() + .join("worktrees") + .join("feature") + .join("foreign"); + std::fs::create_dir_all(&squatter).unwrap(); + std::fs::write(squatter.join("operator-notes.txt"), b"mine\n").unwrap(); + + let s = client + .spawn_session(&worktree_req(base.path(), "feature/foreign", true)) + .await + .expect("spawn must sidestep, not adopt or fail"); + + let suffixed = base + .path() + .join("worktrees") + .join("feature") + .join("foreign-2"); + assert_eq!(s.cwd, suffixed, "session must run in the suffixed tree"); + assert_eq!( + std::fs::read(squatter.join("operator-notes.txt")).unwrap(), + b"mine\n", + "the foreign directory must be untouched" + ); + let registrations = git_worktree_list(base.path()); + let registered_paths: Vec<&str> = registrations + .iter() + .filter_map(|l| l.strip_prefix("worktree ")) + .collect(); + assert!( + !registered_paths + .iter() + .any(|p| *p == squatter.canonicalize().unwrap().display().to_string()), + "a plain directory must never be adopted as a worktree: {registered_paths:?}" + ); + + let _origin = throwaway_origin(base.path()); + client.kill_session(s.id).await.expect("kill"); + assert!(squatter.join("operator-notes.txt").exists()); +} + +/// #79/#88: killing with uncommitted changes is refused; committing and +/// pushing that work clears the guard and the very same kill then succeeds +/// and removes the tree. The session is the handle on the work: while it is +/// unsaved the session stays, once it is saved the tree goes. +#[tokio::test(flavor = "multi_thread")] +async fn guarded_kill_then_commit_and_push_makes_the_same_kill_succeed() { + let (_daemon, client) = bring_up().await; + + let base = TempDir::new().unwrap(); + init_repo(base.path()); + let _origin = throwaway_origin(base.path()); + + let wt = base.path().join("worktrees").join("feature").join("guard"); + let s = client + .spawn_session(&worktree_req(base.path(), "feature/guard", true)) + .await + .expect("spawn"); + std::fs::write(wt.join("README"), b"work in progress\n").unwrap(); + + // Refused: the edit is uncommitted. + 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.uncommitted_files, 1); + assert_eq!(breakdown.unpushed_commits, 0); + } + other => panic!("expected a kill_guarded refusal, got {other:?}"), + } + assert!( + client + .list_sessions() + .await + .unwrap() + .iter() + .any(|x| x.id == s.id), + "a refused kill keeps the session running" + ); + assert_eq!( + std::fs::read(wt.join("README")).unwrap(), + b"work in progress\n", + "the worktree is untouched by the refusal" + ); + + // The operator does exactly what the refusal asked for: save and push. + commit_all(&wt, "wip"); + push_all(base.path()); + + client.kill_session(s.id).await.expect("kill after saving"); + + let deadline = Instant::now() + Duration::from_secs(5); + while Instant::now() < deadline { + if !wt.exists() { + break; + } + tokio::time::sleep(Duration::from_millis(100)).await; + } + assert!( + !wt.exists(), + "a cleared worktree must be removed after kill" + ); + let registrations = git_worktree_list(base.path()); + assert!( + !registrations.iter().any(|l| l.contains("feature/guard")), + "removal must deregister the tree: {registrations:?}" + ); +} + +/// #81/#88: commits that no remote has refuse the kill; `--force` ends the +/// session anyway but RETAINS the worktree — force never destroys blocked +/// work, it parks it in the retained store with reason `force-killed`. +#[tokio::test(flavor = "multi_thread")] +async fn unpushed_commits_block_kill_and_force_retains_the_tree() { + let (_daemon, client) = bring_up().await; + + let base = TempDir::new().unwrap(); + init_repo(base.path()); + let _origin = throwaway_origin(base.path()); + + let wt = base.path().join("worktrees").join("feature").join("local"); + let s = client + .spawn_session(&worktree_req(base.path(), "feature/local", true)) + .await + .expect("spawn"); + std::fs::write(wt.join("README"), b"local only\n").unwrap(); + commit_all(&wt, "not pushed yet"); + + // Refused: the commit is on no remote-tracking ref. + 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.unpushed_commits, 1); + assert_eq!(breakdown.uncommitted_files, 0); + } + other => panic!("expected a kill_guarded refusal, got {other:?}"), + } + + // Force overrides the refusal and nothing else: session gone, tree kept. + let outcome = client.kill_session_forced(s.id).await.expect("forced kill"); + let retained = outcome + .retained + .expect("force must retain the guarded tree"); + assert_eq!( + retained.reason, + maestro_protocol::RetainedReason::ForceKilled + ); + assert_eq!(retained.path, wt); + assert_eq!(retained.branch, "feature/local"); + assert_eq!(retained.in_the_way.unpushed_commits, 1); + + assert!( + !client + .list_sessions() + .await + .unwrap() + .iter() + .any(|x| x.id == s.id), + "the forced kill ends the session" + ); + assert!(wt.is_dir(), "a force-retained worktree must stay on disk"); + assert!( + git_worktree_list(base.path()) + .iter() + .any(|l| l.contains("feature/local")), + "the retained tree stays registered in git" + ); +} + +/// #82/#88: a session that exits on its own never reaches the kill path, but +/// its worktree follows the same rule. Dirty → retained with reason +/// `exited-with-unsaved-work`; pristine → removed, leaving nothing behind. +#[tokio::test(flavor = "multi_thread")] +async fn natural_exit_settles_dirty_and_pristine_worktrees_differently() { + let (_daemon, client) = bring_up().await; + + // Two repos so the two cases cannot see each other's state. + let dirty_repo = TempDir::new().unwrap(); + init_repo(dirty_repo.path()); + let _origin = throwaway_origin(dirty_repo.path()); + let pristine_repo = TempDir::new().unwrap(); + init_repo(pristine_repo.path()); + let _origin_pristine = throwaway_origin(pristine_repo.path()); + + let dirty_wt = dirty_repo + .path() + .join("worktrees") + .join("feature") + .join("messy"); + let pristine_wt = pristine_repo + .path() + .join("worktrees") + .join("feature") + .join("tidy"); + + let _dirty = client + .spawn_session(&CreateSessionRequest { + cwd: dirty_repo.path().to_path_buf(), + command: "sh -c 'echo unsaved > scratch.txt'".into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: Some(worktree_spec(dirty_repo.path(), "feature/messy", true)), + }) + .await + .expect("spawn dirty"); + let _pristine = client + .spawn_session(&create_req_cmd( + pristine_repo.path(), + "true", + "feature/tidy", + )) + .await + .expect("spawn pristine"); + + assert!(dirty_wt.is_dir() && pristine_wt.is_dir()); + + // The watcher reaps dead tmux sessions every couple of ticks and settles + // each worktree accordingly. Poll for both outcomes. + wait_for_async( + "dirty exit retains its worktree", + Duration::from_secs(20), + || { + let client = client.clone(); + let path = dirty_wt.clone(); + async move { + client + .list_worktrees() + .await + .ok() + .map(|wts| { + wts.iter().any(|w| { + w.path == path + && w.reason + == maestro_protocol::RetainedReason::ExitedWithUnsavedWork + }) + }) + .unwrap_or(false) + } + }, + ) + .await; + wait_for_async( + "pristine exit removes its worktree entirely", + Duration::from_secs(20), + || { + let client = client.clone(); + let repo = pristine_repo.path().to_path_buf(); + let path = pristine_wt.clone(); + async move { + if path.exists() { + return false; + } + let listed = client.list_worktrees().await.ok(); + let listed_clear = listed + .map(|wts| !wts.iter().any(|w| w.path == path)) + .unwrap_or(false); + let registered = git_worktree_list(&repo); + !registered.iter().any(|l| l.contains("feature/tidy")) && listed_clear + } + }, + ) + .await; + + // The dirty tree is still fully there, file included. + assert!(dirty_wt.is_dir()); + assert_eq!( + std::fs::read(dirty_wt.join("scratch.txt")).unwrap(), + b"unsaved\n" + ); +} + +fn create_req_cmd(cwd: &std::path::Path, command: &str, branch: &str) -> CreateSessionRequest { + CreateSessionRequest { + cwd: cwd.to_path_buf(), + command: command.into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: Some(worktree_spec(cwd, branch, true)), + } +} + +/// #80/#88: `GET /api/worktrees` lists what the daemon is holding, with the +/// reason and a live in-the-way breakdown; reclaim removes the directory and +/// the store entry but NEVER the branch — its commits outlive the reclaim. +#[tokio::test(flavor = "multi_thread")] +async fn worktrees_endpoint_lists_reasons_and_reclaim_keeps_the_branch() { + let (_daemon, client) = bring_up().await; + + let base = TempDir::new().unwrap(); + init_repo(base.path()); + let _origin = throwaway_origin(base.path()); + + let wt = base.path().join("worktrees").join("feature").join("parked"); + let s = client + .spawn_session(&worktree_req(base.path(), "feature/parked", true)) + .await + .expect("spawn"); + std::fs::write(wt.join("README"), b"worth keeping\n").unwrap(); + commit_all(&wt, "worth keeping"); + let sha = branch_sha(&wt, "feature/parked"); + + let outcome = client.kill_session_forced(s.id).await.expect("force kill"); + assert!(outcome.retained.is_some()); + + let listed = client.list_worktrees().await.expect("list worktrees"); + assert_eq!(listed.len(), 1, "exactly one retained entry: {listed:?}"); + let entry = &listed[0]; + assert_eq!(entry.path, wt); + assert_eq!(entry.branch, "feature/parked"); + assert_eq!(entry.reason, maestro_protocol::RetainedReason::ForceKilled); + assert_eq!(entry.in_the_way.unpushed_commits, 1); + + client.reclaim_worktree(entry.id).await.expect("reclaim"); + assert!(!wt.exists(), "reclaim removes the directory"); + assert!( + client.list_worktrees().await.unwrap().is_empty(), + "reclaim drops the store entry" + ); + + // The whole point of reclaim: the branch and its commits survive. + assert_eq!( + branch_sha(base.path(), "feature/parked"), + sha, + "the branch must keep pointing at the same commit after reclaim" + ); +} + +/// The retained store is state-dir backed (#80): a daemon restart must not +/// orphan the trees it is holding — they are still on disk with entries +/// pointing at them, and still reclaimable afterwards. +#[tokio::test(flavor = "multi_thread")] +async fn retained_store_survives_a_daemon_restart() { + let (mut daemon, client) = bring_up().await; + + let base = TempDir::new().unwrap(); + init_repo(base.path()); + let _origin = throwaway_origin(base.path()); + + let wt = base + .path() + .join("worktrees") + .join("feature") + .join("durable"); + let s = client + .spawn_session(&worktree_req(base.path(), "feature/durable", true)) + .await + .expect("spawn"); + std::fs::write(wt.join("README"), b"survives restarts\n").unwrap(); + commit_all(&wt, "survives restarts"); + client.kill_session_forced(s.id).await.expect("force kill"); + + daemon.restart().await; + + let listed = client.list_worktrees().await.expect("list after restart"); + assert_eq!(listed.len(), 1, "store must survive restart: {listed:?}"); + assert_eq!(listed[0].path, wt); + assert_eq!( + listed[0].reason, + maestro_protocol::RetainedReason::ForceKilled + ); + assert!(wt.is_dir(), "the tree itself must survive the restart too"); + + client + .reclaim_worktree(listed[0].id) + .await + .expect("reclaim"); + assert!(!wt.exists()); +} + // --------------------------------------------------------------------------- // GET /api/projects + /api/branches (#54/#55/#66) // --------------------------------------------------------------------------- @@ -784,10 +1250,12 @@ 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) { +/// Bare remote registered as `origin` on `repo`, seeded with every current +/// branch, so existing 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. The caller must keep the `TempDir` alive +/// while the remote is still needed; later pushes go through [`push_all`]. +fn throwaway_origin(repo: &std::path::Path) -> TempDir { let origin = tempfile::TempDir::new().unwrap(); run_git( &["init", "-q", "--bare", origin.path().to_str().unwrap()], @@ -799,9 +1267,34 @@ fn push_to_throwaway_origin(repo: &std::path::Path) { .args(["remote", "add", "origin", origin.path().to_str().unwrap()]) .output() .expect("git remote add"); + push_all(repo); + origin +} + +fn push_all(repo: &std::path::Path) { run_git(&["push", "-q", "--all", "origin"], repo); } +fn commit_all(dir: &std::path::Path, message: &str) { + run_git(&["add", "-A"], dir); + run_git(&["commit", "-q", "-m", message], dir); +} + +fn branch_sha(repo: &std::path::Path, branch: &str) -> String { + let out = Command::new("git") + .arg("-C") + .arg(repo) + .args(["rev-parse", "--verify", &format!("refs/heads/{branch}")]) + .output() + .expect("git rev-parse"); + assert!( + out.status.success(), + "branch {branch} missing: {}", + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).trim().to_string() +} + #[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