From 85e7749e5af9677366da84b78f75629d396eaaa0 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sun, 23 Aug 2026 20:05:26 +0100 Subject: [PATCH] feat(maestro): worktree specs can check out an existing branch --- crates/maestro-cli/src/sessions.rs | 1 + crates/maestro-daemon/src/session.rs | 18 ++++--- crates/maestro-daemon/src/worktree.rs | 36 ++++++++++---- crates/maestro-daemon/tests/worktree.rs | 66 +++++++++++++++++++++++-- crates/maestro-e2e/tests/e2e.rs | 35 +++++++++++++ crates/maestro-protocol/src/lib.rs | 35 +++++++++++++ 6 files changed, 169 insertions(+), 22 deletions(-) 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-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 2372aef..41845eb 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -213,14 +213,16 @@ 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), diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index b5ffa63..77f38b8 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -31,17 +31,35 @@ impl WorktreeManager { &self.root } - /// `git -C worktree add -b `. - /// Returns the path of the new worktree on success. - pub fn add(&self, base_repo: &Path, branch: &str) -> Result { + /// `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 { 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()?; + 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) } diff --git a/crates/maestro-daemon/tests/worktree.rs b/crates/maestro-daemon/tests/worktree.rs index 1087ac6..c82e085 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/crates/maestro-daemon/tests/worktree.rs @@ -33,7 +33,7 @@ fn add_creates_worktree_on_new_branch() { let wt_root = TempDir::new().unwrap(); let mgr = WorktreeManager::new(wt_root.path()); - 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())); @@ -51,8 +51,8 @@ fn add_twice_generates_distinct_paths() { let wt_root = TempDir::new().unwrap(); let mgr = WorktreeManager::new(wt_root.path()); - 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()); @@ -67,7 +67,7 @@ fn remove_prunes_worktree() { let wt_root = TempDir::new().unwrap(); let mgr = WorktreeManager::new(wt_root.path()); - 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 +79,62 @@ 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(wt_root.path()); + + 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 add_fails_when_branch_already_exists() { let repo_dir = TempDir::new().unwrap(); @@ -97,7 +153,7 @@ fn add_fails_when_branch_already_exists() { let wt_root = TempDir::new().unwrap(); let mgr = WorktreeManager::new(wt_root.path()); - 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..f3fe278 100644 --- a/crates/maestro-e2e/tests/e2e.rs +++ b/crates/maestro-e2e/tests/e2e.rs @@ -129,6 +129,7 @@ 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, }), }; @@ -160,6 +161,40 @@ async fn cleanup_on_kill_removes_worktree() { ); } +#[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..4b6fac9 100644 --- a/crates/maestro-protocol/src/lib.rs +++ b/crates/maestro-protocol/src/lib.rs @@ -113,6 +113,15 @@ pub struct WorktreeSpec { pub branch: String, #[serde(default)] 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 +402,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 +420,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_create_branch_true_on_legacy_payload() { + let json = serde_json::json!({ + "base_repo": "/home/naps62/projects/maestro", + "branch": "feat/foo", + "cleanup_on_kill": true, + }); + let spec: WorktreeSpec = serde_json::from_value(json).unwrap(); + assert!(spec.create_branch); + } + + #[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();