naps62/maestro
Archived
1
0
Fork 0
Code Issues 9 Pull Requests Actions Packages Projects Releases Wiki Activity

merge: worktree placement, existing branches, guarded cleanup (#56)

Browse Source
This commit is contained in:
Miguel Palhas
2026-08-23 20:21:08 +01:00
parent 6f8de8de7c 431f9def6b
commit 6d21061799
14 changed files with 574 additions and 95 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/maestro-cli/src/cli.rs
+25 -2
View File
@@ -67,8 +67,10 @@ pub struct SpawnArgs {
/// Branch name for the worktree (required with --worktree-from).
#[arg(long)]
pub branch: Option<String>,
/// 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([
crates/maestro-cli/src/sessions.rs
+1
View File
@@ -44,6 +44,7 @@ pub async fn run_spawn(client: &DaemonClient, args: SpawnArgs) -> Result<Session
base_repo: repo.clone(),
branch: branch.clone(),
cleanup_on_kill: args.cleanup_on_kill,
create_branch: true,
}),
_ => None,
};
crates/maestro-config/src/lib.rs
+3 -2
View File
@@ -87,8 +87,9 @@ pub struct DaemonSection {
/// Root for everything daemon-local: sidecars, FIFOs, etc.
#[serde(default)]
pub state_dir: Option<PathBuf>,
/// 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 `<repo>/worktrees/<branch>` (#56).
#[serde(default)]
pub worktree_root: Option<PathBuf>,
/// Per-session PTY replay buffer cap in bytes (NAP-229). Each new attach
crates/maestro-daemon/src/cli.rs
+2 -1
View File
@@ -47,7 +47,8 @@ async fn run(config_override: Option<PathBuf>) -> Result<(), Box<dyn Error>> {
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());
crates/maestro-daemon/src/config.rs
+6 -6
View File
@@ -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 `<repo>/worktrees/<branch>`;
/// operators who want them elsewhere set this.
pub worktree_root: Option<PathBuf>,
/// 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<DaemonConfig, Confi
Some(p) => 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<DaemonConfig, Confi
home_dir,
bind_addr,
state_dir,
worktree_root,
worktree_root: daemon.worktree_root,
pty_replay_bytes,
tmux_tmpdir,
spawn_history_days,
@@ -188,7 +188,7 @@ mod tests {
let cfg = load_config(&path, EnvOverrides::default()).unwrap();
assert_eq!(cfg.bind_addr, default_bind_addr());
assert!(cfg.state_dir.ends_with("maestro"));
assert!(cfg.worktree_root.ends_with("worktrees"));
assert!(cfg.worktree_root.is_none());
assert!(!cfg.hostname.is_empty());
}
crates/maestro-daemon/src/session.rs
+27 -24
View File
@@ -38,13 +38,12 @@ pub const DEFAULT_LIVENESS_INTERVAL: Duration = Duration::from_secs(2);
pub struct SessionRecord {
pub session: Session,
pub fifo_path: PathBuf,
/// Worktree we created at spawn time and that should be cleaned up on
/// kill (if `cleanup_worktree_on_kill`).
/// Worktree we created at spawn time; removed on kill unless the
/// dirty-work guard refuses (#56).
pub worktree_path: Option<PathBuf>,
/// `base_repo` from the original `WorktreeSpec`. Needed because `git
/// worktree remove` is invoked from the base repo.
pub worktree_base_repo: Option<PathBuf>,
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<Session, SessionError> {
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<Inner>, 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 });
crates/maestro-daemon/src/worktree.rs
+143 -29
View File
@@ -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 `<repo>/worktrees/<branch>` (#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
/// `<repo>/worktrees/<branch>` placement.
root_override: Option<PathBuf>,
}
impl WorktreeManager {
pub fn new(root: impl Into<PathBuf>) -> Self {
Self { root: root.into() }
pub fn new(root_override: Option<PathBuf>) -> Self {
Self { root_override }
}
pub fn root(&self) -> &Path {
&self.root
pub fn root(&self) -> Option<&Path> {
self.root_override.as_deref()
}
/// `git -C <base_repo> worktree add -b <branch> <generated-path>`.
/// Returns the path of the new worktree on success.
pub fn add(&self, base_repo: &Path, branch: &str) -> Result<PathBuf, WorktreeError> {
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
/// `<base_repo>/worktrees/<sanitized branch>` 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 <base_repo> worktree add -b <branch> <path>` — creates the
/// branch. Returns the path of the new worktree on success.
pub fn add_new(&self, base_repo: &Path, branch: &str) -> Result<PathBuf, WorktreeError> {
self.run_add(base_repo, branch, true)
}
/// `git -C <base_repo> worktree add <path> <branch>` — checks out a branch
/// that must already exist (#56).
pub fn add_existing(&self, base_repo: &Path, branch: &str) -> Result<PathBuf, WorktreeError> {
self.run_add(base_repo, branch, false)
}
fn run_add(
&self,
base_repo: &Path,
branch: &str,
create_branch: bool,
) -> Result<PathBuf, WorktreeError> {
let path = self.target_path(base_repo, branch);
if path.symlink_metadata().is_ok() {
return Err(WorktreeError::PathExists { path });
}
// The parent (`<repo>/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 <path> <branch>` with no `-b`.
cmd.arg(&path).arg(branch);
}
let out = cmd.output()?;
check(out)?;
Ok(path)
}
/// `git -C <base_repo> worktree remove --force <path>`.
/// `git -C <base_repo> worktree remove <path>` — 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<Option<String>, 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::<Vec<_>>();
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<Item = &'static str>,
) -> Result<Output, WorktreeError> {
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(())
crates/maestro-daemon/tests/common/mod.rs
+2 -4
View File
@@ -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<u32>) -> 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,
}
}
crates/maestro-daemon/tests/session_decisions.rs
+2 -2
View File
@@ -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();
crates/maestro-daemon/tests/session_manager.rs
+70 -6
View File
@@ -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();
crates/maestro-daemon/tests/worktree.rs
+164 -9
View File
@@ -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}");
}
crates/maestro-e2e/tests/e2e.rs
+87 -7
View File
@@ -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
// `<state_dir>/worktrees/<branch>`. 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 `<repo>/worktrees/<branch>`
// (#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 <repo>/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;
crates/maestro-protocol/src/lib.rs
+39 -1
View File
@@ -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();
docs/INSTALL.md
+3 -2
View File
@@ -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 `<repo>/worktrees/<branch>`;
set `[daemon].worktree_root` only to override that.)
## Non-Linux hosts