From 8376122dfe2b43e111686b009d9ab0c17cc23725 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sun, 23 Aug 2026 20:54:21 +0100 Subject: [PATCH] fix(maestro): second-review findings across daemon, tui and web Ten findings from reviewing the accumulated blitz/spawn-v2 diff. The two that could wedge a repo: - The cleanup guard refused any worktree whose commits were on no other ref, which is every ordinary unpushed branch. Paths are deterministic now, so the leftover tree blocked the next spawn and each branch became a one-shot. `worktree remove` keeps refs/heads/, so committed work is never at risk: guard on an unclean working tree alone. - A worktree whose directory had vanished failed the git probe and was reported as unsaved work, stranding the registration forever. Prune it instead, and prune before `worktree add` tests the target path. Also: - The CLI hardcoded create_branch: true, so the daemon's existing-branch path was unreachable. Added --existing-branch. - Pastes were forwarded without their DECSET 2004 markers, so a remote shell ran a multi-line paste line by line. - spawnForm emitted before its seed was restored and after destroy(), so a rebuilt draft blanked its cwd and a dead form clobbered the live one. - showRawCwd and the probe's worktree untick never reached the leaf. - A late autocomplete fetch reopened the popup under a blurred input. - The "remove worktree when killed" checkbox promised something the daemon no longer honours; removed. --- crates/maestro-cli/src/cli.rs | 30 ++++++++++ crates/maestro-cli/src/sessions.rs | 2 +- crates/maestro-daemon/src/worktree.rs | 75 +++++++++++-------------- crates/maestro-daemon/tests/worktree.rs | 50 ++++++++++++++--- crates/maestro-tui/src/app.rs | 21 +++++-- web/src/dirAutocomplete.ts | 8 +++ web/src/spawnForm.test.ts | 5 +- web/src/spawnForm.ts | 42 +++++++------- 8 files changed, 154 insertions(+), 79 deletions(-) diff --git a/crates/maestro-cli/src/cli.rs b/crates/maestro-cli/src/cli.rs index 3f497e6..877d344 100644 --- a/crates/maestro-cli/src/cli.rs +++ b/crates/maestro-cli/src/cli.rs @@ -67,6 +67,11 @@ pub struct SpawnArgs { /// Branch name for the worktree (required with --worktree-from). #[arg(long)] pub branch: Option, + /// Check out an existing branch instead of creating one. Without it the + /// daemon runs `worktree add -b`, which fails if the branch is already + /// there. + #[arg(long)] + pub existing_branch: bool, /// 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. @@ -170,6 +175,31 @@ mod tests { } } + /// Review fix: without a flag the CLI always sent `create_branch: true`, + /// so the daemon's existing-branch path was unreachable and spawning onto + /// a branch that already existed just failed. + #[test] + fn parses_spawn_onto_an_existing_branch() { + let cli = Cli::try_parse_from([ + "maestro", + "spawn", + "--cwd", + "/tmp", + "--command", + "bash", + "--worktree-from", + "/repo", + "--branch", + "feat/existing", + "--existing-branch", + ]) + .unwrap(); + let Command::Spawn(a) = cli.command else { + panic!("expected spawn") + }; + assert!(a.existing_branch); + } + #[test] fn spawn_worktree_requires_branch() { let err = Cli::try_parse_from([ diff --git a/crates/maestro-cli/src/sessions.rs b/crates/maestro-cli/src/sessions.rs index e3ca37e..1a087dc 100644 --- a/crates/maestro-cli/src/sessions.rs +++ b/crates/maestro-cli/src/sessions.rs @@ -44,7 +44,7 @@ pub async fn run_spawn(client: &DaemonClient, args: SpawnArgs) -> Result None, }; diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index 0e8ad5e..c723066 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -69,6 +69,9 @@ impl WorktreeManager { create_branch: bool, ) -> Result { let path = self.target_path(base_repo, branch); + // Clear registrations whose directory is gone before testing the + // path, so a previously-leaked entry doesn't block this branch. + prune(base_repo)?; if path.symlink_metadata().is_ok() { return Err(WorktreeError::PathExists { path }); } @@ -97,6 +100,14 @@ impl WorktreeManager { /// 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> { + // 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 + // work, stranding the registration — and since paths are + // deterministic, `worktree add` would then refuse that branch forever + // with "missing but already registered" (#56 review). + if !path.exists() { + return prune(base_repo); + } if let Some(reason) = unsafe_to_remove(path)? { return Err(WorktreeError::Guarded { path: path.to_path_buf(), @@ -128,55 +139,35 @@ 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). +/// `Some(reason)` when deleting `path` would destroy work. +/// +/// 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> { - let status = run_git(path, ["status", "--porcelain"])?; + // `--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())); } - - let head = String::from_utf8_lossy(&run_git(path, ["rev-parse", "HEAD"])?.stdout) - .trim() - .to_string(); - let branch = - String::from_utf8_lossy(&run_git(path, ["rev-parse", "--abbrev-ref", "HEAD"])?.stdout) - .trim() - .to_string(); - - // Every ref except this worktree's own branch counts as a safe home for - // its commits. Detached HEAD excludes everything. - let refs = - String::from_utf8_lossy(&run_git(path, ["for-each-ref", "--format=%(refname)"])?.stdout) - .lines() - .filter(|r| match branch.as_str() { - "HEAD" => true, - b => *r != format!("refs/heads/{b}"), - }) - .map(str::to_string) - .collect::>(); - - let mut cmd = Command::new("git"); - cmd.arg("-C").arg(path).args(["rev-list", "--count", &head]); - if !refs.is_empty() { - cmd.arg("--not").args(&refs); - } - let out = cmd.output()?; - check(out.clone())?; - let count: usize = String::from_utf8_lossy(&out.stdout) - .trim() - .parse() - .unwrap_or(0); - if count > 0 { - return Ok(Some(format!( - "has {count} commit(s) not present on any other ref" - ))); - } Ok(None) } +/// `git -C worktree prune` — drops registrations whose directory +/// no longer exists. +fn prune(base_repo: &Path) -> Result<(), WorktreeError> { + let out = Command::new("git") + .arg("-C") + .arg(base_repo) + .args(["worktree", "prune"]) + .output()?; + check(out) +} + fn run_git( dir: &Path, args: impl IntoIterator, diff --git a/crates/maestro-daemon/tests/worktree.rs b/crates/maestro-daemon/tests/worktree.rs index fdb6945..722875a 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/crates/maestro-daemon/tests/worktree.rs @@ -206,18 +206,50 @@ fn remove_refuses_worktree_with_uncommitted_changes() { .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… + // 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", "unsaved"]); + run_git(&path, &["commit", "-q", "-m", "saved"]); + + mgr.remove(&repo, &path).expect("committed work can go"); + assert!(!path.exists()); + assert!( + worktree_branches(&repo).contains(&"main".to_string()), + "the branch survives removal" + ); + let branches = String::from_utf8( + Command::new("git") + .arg("-C") + .arg(&repo) + .args(["for-each-ref", "--format=%(refname:short)", "refs/heads"]) + .output() + .unwrap() + .stdout, + ) + .unwrap(); + assert!(branches.contains("feat/dirty"), "{branches}"); +} + +/// A worktree whose directory vanished must be pruned, not misread as holding +/// unsaved work — otherwise the registration is stranded and, because paths +/// are deterministic, that branch can never be spawned again (#56 review). +#[test] +fn remove_prunes_a_worktree_whose_directory_vanished() { + 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/gone").expect("add"); + std::fs::remove_dir_all(&path).unwrap(); 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()); + .expect("a vanished worktree prunes"); + // The registration is gone too, so the same branch can be spawned again. + mgr.add_existing(&repo, "feat/gone") + .expect("branch is spawnable again"); } fn run_git(dir: &std::path::Path, args: &[&str]) { diff --git a/crates/maestro-tui/src/app.rs b/crates/maestro-tui/src/app.rs index aae1d75..98eac6c 100644 --- a/crates/maestro-tui/src/app.rs +++ b/crates/maestro-tui/src/app.rs @@ -464,7 +464,16 @@ fn encode_key(key: &InputKey) -> Option> { InputKey::Home => Some(b"\x1b[H".to_vec()), InputKey::End => Some(b"\x1b[F".to_vec()), InputKey::Delete => Some(b"\x1b[3~".to_vec()), - InputKey::Paste(text) => Some(text.as_bytes().to_vec()), + InputKey::Paste(text) => { + // Wrap in the DECSET 2004 markers. Raw bytes look like typed keys + // to an app that enabled bracketed paste, so a multi-line paste + // would execute line by line instead of arriving as one block. + let mut out = Vec::with_capacity(text.len() + 12); + out.extend_from_slice(b"\x1b[200~"); + out.extend_from_slice(text.as_bytes()); + out.extend_from_slice(b"\x1b[201~"); + Some(out) + } InputKey::Mouse { .. } => None, } } @@ -697,12 +706,14 @@ mod tests { /// Regression review fix: `Paste(_)` used to be silently dropped, so a /// paste into an attached session vanished once bracketed paste was - /// enabled — the payload must be forwarded verbatim. + /// enabled. It also has to keep its DECSET 2004 markers — without them a + /// remote shell reads the payload as typed keys and runs a multi-line + /// paste line by line. #[test] - fn a_bracketed_paste_is_forwarded_verbatim_to_the_session() { + fn a_bracketed_paste_reaches_the_session_with_its_markers() { assert_eq!( - encode_key(&InputKey::Paste("cd /tmp/foo".into())), - Some(b"cd /tmp/foo".to_vec()) + encode_key(&InputKey::Paste("cd /tmp/foo\nls".into())), + Some(b"\x1b[200~cd /tmp/foo\nls\x1b[201~".to_vec()) ); } diff --git a/web/src/dirAutocomplete.ts b/web/src/dirAutocomplete.ts index 0eb5efe..3ec9114 100644 --- a/web/src/dirAutocomplete.ts +++ b/web/src/dirAutocomplete.ts @@ -109,6 +109,14 @@ export function attachDirAutocomplete(input: HTMLInputElement): DirAutocompleteH } function applyFilter(entries: string[], prefix: string): void { + // A fetch can outlive the dismissal that was meant to close this popup + // (Tab-commit re-arms a refresh, then blur dismisses 120ms later). Showing + // it now would anchor a dropdown under an unfocused input that nothing + // closes until the next interaction. + if (dismissed || document.activeElement !== input) { + close(); + return; + } const lower = prefix.toLowerCase(); filtered = entries.filter((e) => e.toLowerCase().startsWith(lower)); highlight = filtered.length > 0 ? 0 : -1; diff --git a/web/src/spawnForm.test.ts b/web/src/spawnForm.test.ts index 4b7dd1b..4559526 100644 --- a/web/src/spawnForm.test.ts +++ b/web/src/spawnForm.test.ts @@ -8,7 +8,6 @@ const base: SpawnFormValues = { name: "amber-fox", worktreeEnabled: false, worktreeBranch: "", - worktreeCleanup: false, }; describe("buildSpawnRequest", () => { @@ -24,9 +23,9 @@ describe("buildSpawnRequest", () => { expect(res).toEqual({ req: { cwd: base.cwd, command: base.command, metadata: {} } }); }); - it("attaches a trimmed branch and cleanup flag when worktree is enabled", () => { + it("attaches a trimmed branch when worktree is enabled, and always cleans up", () => { const res = buildSpawnRequest( - { ...base, worktreeEnabled: true, worktreeBranch: " feat/foo ", worktreeCleanup: true }, + { ...base, worktreeEnabled: true, worktreeBranch: " feat/foo " }, "/home/user/project", ); expect(res).toEqual({ diff --git a/web/src/spawnForm.ts b/web/src/spawnForm.ts index 7b78eb4..d28d295 100644 --- a/web/src/spawnForm.ts +++ b/web/src/spawnForm.ts @@ -14,7 +14,6 @@ export interface SpawnFormSeed { name?: string; worktreeEnabled?: boolean; worktreeBranch?: string; - worktreeCleanup?: boolean; cwdRawVisible?: boolean; } @@ -26,7 +25,6 @@ export interface SpawnFormValues { name: string; worktreeEnabled: boolean; worktreeBranch: string; - worktreeCleanup: boolean; /** Whether the cwd lives in the raw text input (vs a picked recent row). */ cwdRawVisible?: boolean; } @@ -55,7 +53,9 @@ export function buildSpawnRequest( req.worktree = { base_repo: baseRepo, branch, - cleanup_on_kill: values.worktreeCleanup, + // The daemon always cleans up, guarded against unsaved work (#56); + // the wire field stays for older daemons. + cleanup_on_kill: true, }; } @@ -130,10 +130,6 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { Branch - @@ -155,9 +151,6 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { const wtBody = form.querySelector(".wt-body") as HTMLElement; const wtRepo = form.querySelector(".wt-repo") as HTMLElement; const branchInput = form.querySelector("input[name=worktree_branch]") as HTMLInputElement; - const cleanupInput = form.querySelector( - "input[name=worktree_cleanup_on_kill]", - ) as HTMLInputElement; const nameInput = form.querySelector("input[name=name]") as HTMLInputElement; // ---- form state ------------------------------------------------------ @@ -172,14 +165,22 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { // settles — so the fetch can't race past and clobber the seeded directory. let seedCwd: string | null = prefill.cwd ?? null; + // `emit` stays silent until the initial load settles and goes silent again + // once the form is destroyed. Construction runs with `cwd === ""` and the + // seed is only restored two fetches later, so emitting in between would + // blank the leaf's draft; and `multiview.render()` rebuilds forms while the + // old one's fetches are still in flight, so a destroyed form emitting would + // clobber the live one's leaf (#57 review). + let ready = false; + let destroyed = false; const emit = () => { + if (!ready || destroyed) return; opts.onChange?.({ command, cwd, name: nameInput.value, worktreeEnabled: wtEnabled.checked, worktreeBranch: branchInput.value, - worktreeCleanup: cleanupInput.checked, cwdRawVisible: !cwdRawWrap.hidden, }); }; @@ -296,6 +297,9 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { cwdRaw.focus(); cwdRaw.select(); } + // Both the visibility and the directory are draft state: without this a + // render before the operator types snaps back to the recents picker. + emit(); } cwdRaw.addEventListener("input", () => { @@ -357,10 +361,9 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { } else { cwdRawWrap.hidden = true; } - // The seeded cwd is in place — a restored worktree can probe now. + // Everything seeded: the form may publish from here on. + ready = true; if (wtEnabled.checked) void probeGitRoot(); - // Publish it: construction ran `emit()` with an empty cwd, so without this - // the leaf's draft keeps `cwd: ""` and the next render reseeds it blank. emit(); } @@ -423,7 +426,10 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { wtRepo.textContent = `repo: ${root} (inferred)`; } else { inferredBaseRepo = null; + // Publish the untick too, or the next render seeds the box checked with + // no inferred repo and submitting fails. wtEnabled.checked = false; + emit(); wtBody.hidden = true; wtHint.hidden = false; wtHint.textContent = "Not inside a git repository — pick a dir in a repo to use a worktree."; @@ -440,14 +446,12 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { wtHint.hidden = true; } }); - cleanupInput.addEventListener("change", emit); // A seeded worktree restores its checkbox/body up front; its gitroot probe // waits for loadRecents to apply the seeded cwd. - if (prefill.worktreeEnabled || prefill.worktreeBranch || prefill.worktreeCleanup) { + if (prefill.worktreeEnabled || prefill.worktreeBranch) { wtEnabled.checked = Boolean(prefill.worktreeEnabled); if (prefill.worktreeBranch) branchInput.value = prefill.worktreeBranch; - cleanupInput.checked = Boolean(prefill.worktreeCleanup); wtBody.hidden = !wtEnabled.checked; wtHint.hidden = true; emit(); @@ -471,8 +475,7 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { name: nameInput.value, worktreeEnabled: wtEnabled.checked, worktreeBranch: branchInput.value, - worktreeCleanup: cleanupInput.checked, - }, + }, inferredBaseRepo, ); if ("error" in built) { @@ -494,6 +497,7 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { return { el: form, destroy() { + destroyed = true; autocomplete.destroy(); form.remove(); },