diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 6b0d0af..2472afe 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -226,6 +226,25 @@ impl SessionManager { let WorktreeSpec { base_repo, branch, .. } = spec; + // `run_add` adopts a leftover tree for this branch — the + // dirty-work guard refuses to delete unsaved work and the path + // is deterministic, so erroring would burn the branch forever. + // A tree a *live* session is using is a different matter: + // two sessions must not share one checkout, so reject it here + // where the session records are visible (#67 review). + let planned = self.inner.worktree_mgr.planned_path(base_repo, branch); + if self + .inner + .sessions + .lock() + .await + .values() + .any(|r| r.worktree_path.as_deref() == Some(planned.as_path())) + { + return Err(SessionError::Worktree( + crate::worktree::WorktreeError::PathExists { path: planned }, + )); + } let path = if spec.create_branch { self.inner.worktree_mgr.add_new(base_repo, branch) } else { diff --git a/crates/maestro-daemon/src/worktree.rs b/crates/maestro-daemon/src/worktree.rs index 7a1450d..1421260 100644 --- a/crates/maestro-daemon/src/worktree.rs +++ b/crates/maestro-daemon/src/worktree.rs @@ -53,6 +53,12 @@ impl WorktreeManager { /// The override root gets the repo's basename too: without it two repos /// spawning the same branch name target one directory and the second /// fails with `PathExists`. + /// Where [`Self::add_new`] / [`Self::add_existing`] would place this + /// branch's worktree. Callers use it to check ownership before creating. + pub fn planned_path(&self, base_repo: &Path, branch: &str) -> PathBuf { + self.target_path(base_repo, branch) + } + fn target_path(&self, base_repo: &Path, branch: &str) -> PathBuf { // An unusable branch name falls back to a digest rather than escaping // the root; `run_add` surfaces git's own error for it anyway. @@ -97,6 +103,16 @@ impl WorktreeManager { // path, so a previously-leaked entry doesn't block this branch. prune(base_repo)?; if path.symlink_metadata().is_ok() { + // A tree we left behind for this very branch — the dirty-work + // guard refuses to delete unsaved work, and the path is + // deterministic, so erroring here would burn the branch + // permanently: no spawn could ever produce a worktree for it + // again and the product offers no way to clear it. Adopt it + // instead; landing in the operator's own unsaved work on the + // branch they asked for is what they wanted (#67 review). + if registered_for(base_repo, &path, branch) { + return Ok(path); + } return Err(WorktreeError::PathExists { path }); } // The parent (`/worktrees` or the override root) may not exist @@ -176,6 +192,34 @@ fn main_checkout(path: &Path) -> PathBuf { .unwrap_or_else(|| path.to_path_buf()) } +/// True when `path` is already a registered worktree of `base_repo` holding +/// `branch` — i.e. one this daemon created for this branch and later declined +/// to remove. +fn registered_for(base_repo: &Path, path: &Path, branch: &str) -> bool { + let Ok(out) = Command::new("git") + .arg("-C") + .arg(base_repo) + .args(["worktree", "list", "--porcelain"]) + .output() + else { + return false; + }; + let want = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); + let text = String::from_utf8_lossy(&out.stdout); + let mut here = false; + for line in text.lines() { + if let Some(p) = line.strip_prefix("worktree ") { + let p = PathBuf::from(p); + here = p.canonicalize().unwrap_or(p) == want; + } else if here { + if let Some(r) = line.strip_prefix("branch ") { + return r.trim() == format!("refs/heads/{branch}"); + } + } + } + false +} + /// `Some(inner)` when a registered worktree lives under `path`. Git omits /// those directories from `git status`, so the dirty-work guard cannot see /// them and removal would delete them (#54 review). @@ -202,12 +246,20 @@ fn nested_worktree(path: &Path) -> Option { }) } -/// Short, stable digest used to disambiguate override roots. +/// Short digest used to disambiguate override roots. +/// +/// Hand-rolled FNV-1a rather than `DefaultHasher`: this value becomes a +/// directory name, and `DefaultHasher`'s algorithm is explicitly not +/// guaranteed stable across Rust releases — a toolchain bump would remap every +/// repo to a new directory and orphan the worktrees under the old one +/// (#67 review). fn short_hash(s: &str) -> String { - use std::hash::{Hash, Hasher}; - let mut h = std::collections::hash_map::DefaultHasher::new(); - s.hash(&mut h); - format!("{:08x}", h.finish() as u32) + let mut h: u32 = 0x811c_9dc5; + for b in s.as_bytes() { + h ^= u32::from(*b); + h = h.wrapping_mul(0x0100_0193); + } + format!("{h:08x}") } /// The relative directory a branch's worktree lives in. diff --git a/crates/maestro-daemon/tests/worktree.rs b/crates/maestro-daemon/tests/worktree.rs index cea834e..0c59bc9 100644 --- a/crates/maestro-daemon/tests/worktree.rs +++ b/crates/maestro-daemon/tests/worktree.rs @@ -399,3 +399,46 @@ fn similar_branch_names_get_distinct_directories() { assert_ne!(a, c); assert!(a.ends_with("feat/x"), "{a:?}"); } + +/// The dead end the cleanup guard created: it refuses to delete unsaved work, +/// the path is deterministic, and `PathExists` is a hard error — so one stray +/// untracked file used to burn that branch permanently, with no way to clear +/// it from the product. The leftover is adopted instead (#67 review). +#[test] +fn a_guarded_leftover_is_adopted_rather_than_burning_the_branch() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + let mgr = WorktreeManager::new(None); + 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. + mgr.remove(&repo, &first) + .expect_err("dirty tree is guarded"); + assert!(first.exists()); + + // Spawning that branch again lands in the operator's own work, not an error. + let again = mgr + .add_existing(&repo, "feat/resume") + .expect("leftover is adopted"); + assert_eq!(again, first); + assert!( + again.join("notes.txt").exists(), + "their work is still there" + ); +} + +/// Adoption must not extend to a directory that merely sits in the way. +#[test] +fn an_unrelated_directory_at_the_target_path_is_still_an_error() { + let repo_dir = TempDir::new().unwrap(); + let repo = repo_dir.path().to_path_buf(); + init_repo(&repo); + + std::fs::create_dir_all(repo.join("worktrees").join("feat").join("squatter")).unwrap(); + let mgr = WorktreeManager::new(None); + mgr.add_new(&repo, "feat/squatter") + .expect_err("a non-worktree directory must not be adopted"); +} diff --git a/crates/maestro-tui/src/app.rs b/crates/maestro-tui/src/app.rs index 1676c5d..96b5375 100644 --- a/crates/maestro-tui/src/app.rs +++ b/crates/maestro-tui/src/app.rs @@ -219,7 +219,7 @@ impl App { Effect::Spawn(req) => self.submit_spawn(req).await, Effect::Kill(id) => self.submit_kill(id).await, Effect::LoadDraft(tab_idx) => self.spawn_load_draft(tab_idx), - Effect::FastForward => self.fast_forward_spawn().await, + Effect::FastForward => self.spawn_fast_forward(), Effect::RefillNames(tab_idx) => self.spawn_refill_names(tab_idx), Effect::ProbeGitRoot { tab, seq, path } => { self.spawn_probe_git_root(tab, seq, path) @@ -395,43 +395,21 @@ impl App { effects } - /// One `/api/names` batch. [`DaemonClient`] has no typed call for this - /// route yet, so it goes through plain reqwest against the same base URL - /// and query contract (`routes::NAMES` + `routes::COUNT_QUERY`). - async fn fetch_names(&self, count: usize) -> Vec { - let url = format!( - "{}{}", - maestro_protocol::routes::normalize_base(self.daemon.base()), - routes::NAMES - ); - match self - .http - .get(url) - .query(&[(routes::COUNT_QUERY, count)]) - .send() - .await - { - Ok(resp) => resp - .json::() - .await - .map(|batch| batch.names) - .unwrap_or_default(), - Err(_) => Vec::new(), - } - } - /// Populate a freshly opened draft: presets from `/api/info` (falling back /// to the built-ins already in place), projects from `/api/projects`, and /// a first name batch. Applies only if that tab still shows the draft — /// a slow response must never clobber what replaced it. /// Kick off the draft's initial data load without blocking the loop. fn spawn_load_draft(&self, tab: usize) { + let Some(draft) = self.draft_id(tab) else { + return; + }; let (daemon, http, base, tx) = self.loader_handles(); tokio::spawn(async move { let (info, projects) = tokio::join!(daemon.daemon_info(), daemon.list_projects()); let names = fetch_names(&http, &base, crate::draft::NAME_BATCH_SIZE).await; let _ = tx.send(DraftUpdate::Data { - tab, + draft, info: info.ok(), projects: projects.ok(), names, @@ -440,14 +418,20 @@ impl App { } fn spawn_refill_names(&self, tab: usize) { + let Some(draft) = self.draft_id(tab) else { + return; + }; let (_, http, base, tx) = self.loader_handles(); tokio::spawn(async move { let names = fetch_names(&http, &base, crate::draft::NAME_BATCH_SIZE).await; - let _ = tx.send(DraftUpdate::Names { tab, names }); + let _ = tx.send(DraftUpdate::Names { draft, names }); }); } fn spawn_probe_git_root(&self, tab: usize, seq: u32, path: PathBuf) { + let Some(draft) = self.draft_id(tab) else { + return; + }; let (daemon, _, _, tx) = self.loader_handles(); tokio::spawn(async move { let root = daemon @@ -455,11 +439,14 @@ impl App { .await .ok() .and_then(|g| g.root); - let _ = tx.send(DraftUpdate::GitRoot { tab, seq, root }); + let _ = tx.send(DraftUpdate::GitRoot { draft, seq, root }); }); } fn spawn_load_branches(&self, tab: usize) { + let Some(draft) = self.draft_id(tab) else { + return; + }; let Some(cwd) = self .state .tabs @@ -475,10 +462,22 @@ impl App { let (_, http, base, tx) = self.loader_handles(); tokio::spawn(async move { let branches = fetch_branches(&http, &base, &cwd).await; - let _ = tx.send(DraftUpdate::Branches { tab, cwd, branches }); + let _ = tx.send(DraftUpdate::Branches { + draft, + cwd, + branches, + }); }); } + /// The draft pane living in `tab`, by identity. + fn draft_id(&self, tab: usize) -> Option { + match self.state.tabs.get(tab).and_then(|t| t.pane.as_ref()) { + Some(PaneRef::Draft(d)) => Some(d.id), + _ => None, + } + } + fn loader_handles( &self, ) -> ( @@ -497,11 +496,34 @@ impl App { /// Fold a finished background load into the draft that asked for it. async fn apply_draft_update(&mut self, update: DraftUpdate) { - let tab_idx = match &update { - DraftUpdate::Data { tab, .. } - | DraftUpdate::Names { tab, .. } - | DraftUpdate::GitRoot { tab, .. } - | DraftUpdate::Branches { tab, .. } => *tab, + if let DraftUpdate::FastForward { + info, + projects, + names, + } = update + { + self.fast_forward_spawn(info, projects, names).await; + return; + } + let want = match &update { + DraftUpdate::Data { draft, .. } + | DraftUpdate::Names { draft, .. } + | DraftUpdate::GitRoot { draft, .. } + | DraftUpdate::Branches { draft, .. } => *draft, + // Handled above; it targets no particular draft. + DraftUpdate::FastForward { .. } => unreachable!(), + }; + // Find it by identity: closing a tab shifts every index to its right, + // so an in-flight load addressed by index would be dropped or land on + // whatever pane inherited the slot (#67 review). + let Some(tab_idx) = self + .state + .tabs + .iter() + .position(|t| matches!(&t.pane, Some(PaneRef::Draft(d)) if d.id == want)) + else { + // The draft went away while the request was in flight. + return; }; let Some(PaneRef::Draft(draft)) = self .state @@ -509,7 +531,6 @@ impl App { .get_mut(tab_idx) .and_then(|t| t.pane.as_mut()) else { - // The pane went away while the request was in flight. return; }; let mut follow_up_branches = false; @@ -545,6 +566,7 @@ impl App { DraftUpdate::Branches { cwd, branches, .. } => { draft.set_branches(&cwd, branches); } + DraftUpdate::FastForward { .. } => unreachable!(), } if follow_up_branches { self.spawn_load_branches(tab_idx); @@ -557,11 +579,38 @@ impl App { /// branch named after that name. Anything unresolvable — no projects /// known yet, no command, no name — downgrades to a draft pane seeded /// with whatever did resolve. - async fn fast_forward_spawn(&mut self) { - let (info, projects) = tokio::join!(self.daemon.daemon_info(), self.daemon.list_projects()); - let names = self.fetch_names(crate::draft::NAME_BATCH_SIZE).await; + /// Load what the fast path needs without blocking the loop — the same + /// reason the draft loaders were moved off it (#67 review): a cold + /// `/api/projects` walks all of `$HOME`, and pressing the key must not + /// freeze keys, frames and attached panes until it returns. + fn spawn_fast_forward(&self) { + let (daemon, http, base, tx) = self.loader_handles(); + tokio::spawn(async move { + let (info, projects) = tokio::join!(daemon.daemon_info(), daemon.list_projects()); + let names = fetch_names(&http, &base, crate::draft::NAME_BATCH_SIZE).await; + let _ = tx.send(DraftUpdate::FastForward { + info: info.ok(), + projects: projects.ok(), + names, + }); + }); + } + + async fn fast_forward_spawn( + &mut self, + info: Option, + projects: Option>, + names: Vec, + ) { + // Keep the home dir: without it the pre-filled draft renders + // unabbreviated paths and "Other directory…" seeds `/` instead of + // home, so the two draft entry points disagreed (#67 review). + let home_dir = info + .as_ref() + .map(|i| i.home_dir.clone()) + .unwrap_or_default(); let presets = match info { - Ok(info) if !info.presets.is_empty() => info.presets, + Some(info) if !info.presets.is_empty() => info.presets, _ => Vec::new(), }; let discovered = projects.unwrap_or_default(); @@ -581,6 +630,7 @@ impl App { let Some(PaneRef::Draft(draft)) = self.state.active_tab_mut().pane.as_mut() else { return; }; + draft.home_dir = home_dir; if !presets.is_empty() { draft.set_presets(presets); } @@ -782,25 +832,31 @@ fn render_help_lines(cfg: &TuiConfig) -> Vec { #[derive(Debug)] enum DraftUpdate { Data { - tab: usize, + draft: u64, info: Option, projects: Option>, names: Vec, }, Names { - tab: usize, + draft: u64, names: Vec, }, GitRoot { - tab: usize, + draft: u64, seq: u32, root: Option, }, Branches { - tab: usize, + draft: u64, cwd: PathBuf, branches: Vec, }, + /// A fast-forward's data arrived; resolution and spawn happen on the loop. + FastForward { + info: Option, + projects: Option>, + names: Vec, + }, } /// `GET /api/names?count=N`. Free function: the spawned loaders have no `self`. @@ -1062,7 +1118,14 @@ mod tests { let mut app = make_app_at(&base); let effects = app.dispatch(Action::FastForwardSpawn); + // The fast path loads off the event loop now; the run loop drains the + // channel, so do that by hand. app.perform(effects).await; + let update = tokio::time::timeout(Duration::from_secs(5), app.draft_rx.recv()) + .await + .expect("load completed") + .expect("channel open"); + app.apply_draft_update(update).await; let posts = posted.lock().unwrap(); assert_eq!(posts.len(), 1, "one immediate POST, no form shown"); @@ -1106,7 +1169,14 @@ mod tests { let mut app = make_app_at(&base); let effects = app.dispatch(Action::FastForwardSpawn); + // The fast path loads off the event loop now; the run loop drains the + // channel, so do that by hand. app.perform(effects).await; + let update = tokio::time::timeout(Duration::from_secs(5), app.draft_rx.recv()) + .await + .expect("load completed") + .expect("channel open"); + app.apply_draft_update(update).await; assert!( matches!(app.state.active_tab().pane, Some(PaneRef::Draft(_))), diff --git a/crates/maestro-tui/src/draft.rs b/crates/maestro-tui/src/draft.rs index b20b99c..cf3a165 100644 --- a/crates/maestro-tui/src/draft.rs +++ b/crates/maestro-tui/src/draft.rs @@ -114,6 +114,11 @@ pub enum DraftInputResult { #[derive(Debug, Clone, PartialEq)] pub struct DraftPane { + /// Identity for routing async load results back to *this* draft. + /// + /// Tab indices shift when a tab closes, so an in-flight load addressed by + /// index could be dropped or land on the wrong pane (#67 review). + pub id: u64, /// Includes the implicit custom tile (empty `command`) last. pub presets: Vec, pub selected_preset: usize, @@ -164,6 +169,9 @@ pub struct DraftPane { pub home_dir: String, } +/// Source of [`DraftPane::id`]. +static NEXT_DRAFT_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); + impl Default for DraftPane { fn default() -> Self { // Built-ins until `/api/info` answers — the form must be usable even @@ -171,6 +179,7 @@ impl Default for DraftPane { let mut presets = builtin_presets(); presets.push(custom_preset()); Self { + id: NEXT_DRAFT_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed), presets, selected_preset: 0, custom_command: crate::text_input::TextInput::default(), @@ -256,10 +265,16 @@ impl DraftPane { /// Append a refill batch; the first name replaces the field (this only /// happens after an explicit reroll on an empty batch). - pub fn extend_names(&mut self, names: Vec) { - if let Some(first) = names.first() { - self.set_name(first.clone()); + /// + /// The shown name is *consumed*, not left at the head — queueing it too + /// made the next reroll produce the same name and the key look dead + /// (#67 review). + pub fn extend_names(&mut self, mut names: Vec) { + if names.is_empty() { + return; } + let first = names.remove(0); + self.set_name(first); self.name_queue.extend(names); } diff --git a/web/src/main.ts b/web/src/main.ts index 00e664d..a89bda9 100644 --- a/web/src/main.ts +++ b/web/src/main.ts @@ -307,7 +307,7 @@ async function requestFastForward(): Promise { // Mount it, the way the TUI's fast path attaches. Without this the // keystroke creates a session and leaves the UI untouched, so it reads // as a dead key (#67 review). - mv.openSession(session.id); + mv.openSessionBeside(session.id); updateShellRoute(); return; } catch { diff --git a/web/src/multiview.ts b/web/src/multiview.ts index c95daf7..b837c6e 100644 --- a/web/src/multiview.ts +++ b/web/src/multiview.ts @@ -49,6 +49,7 @@ export interface MultiView { /** Bring a session into the layout: focus it if already open, else fill the * focused pane (or open the first pane in an empty tab). */ openSession: (sessionId: string) => void; + openSessionBeside: (sessionId: string) => void; /** Drop the pane showing `sessionId` (the session was disconnected/killed). */ closeSession: (sessionId: string) => void; /** Every session currently shown in some pane, across all tabs. */ @@ -378,6 +379,39 @@ export function createMultiView(opts: MultiViewOptions): MultiView { render(); } + /// Mount a session without evicting one. + /// + /// `openSession` deliberately replaces the focused pane (VS Code style) for + /// a sidebar click, but the fast path must never do that: pressing the key + /// while attached would drop the running session out of the layout + /// entirely. Follows `openDraft`'s rule instead — reuse an empty pane, else + /// split beside the focused one (#67 review). + function openSessionBeside(sessionId: string): void { + const loc = locateSession(layout, sessionId); + if (loc) { + layout.activeTabId = loc.tab.id; + loc.tab.focusedLeafId = loc.leaf.id; + render(); + return; + } + const tab = activeTab(layout); + if (!tab.root || !tab.focusedLeafId || !findLeaf(tab.root, tab.focusedLeafId)) { + tab.focusedLeafId = splitLeaf(tab, "", sessionId, "row", true); + render(); + opts.onOpenSessionsChange?.(); + return; + } + const target = findLeaf(tab.root, tab.focusedLeafId)!; + if (!target.sessionId) { + target.sessionId = sessionId; + target.draft = null; + } else { + tab.focusedLeafId = splitLeaf(tab, target.id, sessionId, "row", true); + } + render(); + opts.onOpenSessionsChange?.(); + } + function openDraft(seed?: Partial): void { const tab = activeTab(layout); const draft = (): Draft => ({ command: "", cwd: "", name: "", ...seed }); @@ -693,6 +727,7 @@ export function createMultiView(opts: MultiViewOptions): MultiView { return { el, openSession, + openSessionBeside, openDraft, closeSession, openSessions: () => openSessions(layout), diff --git a/web/src/spawnForm.test.ts b/web/src/spawnForm.test.ts index 3481628..5deb9ea 100644 --- a/web/src/spawnForm.test.ts +++ b/web/src/spawnForm.test.ts @@ -9,6 +9,7 @@ const base: SpawnFormValues = { worktreeEnabled: false, worktreeBranch: "", worktreeBranchMode: "new", + worktreeBranchEdited: false, }; describe("buildSpawnRequest", () => { diff --git a/web/src/spawnForm.ts b/web/src/spawnForm.ts index 8e365c6..5a21569 100644 --- a/web/src/spawnForm.ts +++ b/web/src/spawnForm.ts @@ -25,6 +25,10 @@ export interface SpawnFormSeed { name?: string; worktreeEnabled?: boolean; worktreeBranch?: string; + /** Whether the operator typed the branch themselves, rather than it having + * been auto-synced from the session name. Carried separately because the + * value alone cannot distinguish the two. */ + worktreeBranchEdited?: boolean; worktreeBranchMode?: WorktreeBranchMode; cwdRawVisible?: boolean; } @@ -37,6 +41,7 @@ export interface SpawnFormValues { name: string; worktreeEnabled: boolean; worktreeBranch: string; + worktreeBranchEdited: boolean; /** "new" creates the branch; "existing" checks out a picked one (#61). */ worktreeBranchMode: WorktreeBranchMode; /** Whether the cwd lives in the raw text input (vs a picked recent row). */ @@ -230,6 +235,7 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { name: nameInput.value, worktreeEnabled: wtEnabled.checked, worktreeBranch: branchValue(), + worktreeBranchEdited: branchEdited, worktreeBranchMode: branchMode, cwdRawVisible: !cwdRawWrap.hidden, }); @@ -239,7 +245,11 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { // The worktree branch defaults to the session name — including a clone's // prefilled name — and tracks it on reroll/edit until the user types their // own branch. Only in "new" mode: an existing-branch pick is not a name. - let branchEdited = Boolean(prefill.worktreeBranch); + // True only once the operator types their own branch. It cannot be inferred + // from the seed: in "new" mode `emit()` publishes the auto-synced session + // name, so seeding from it froze the branch after any form rebuild + // (#67 review). `worktreeBranchEdited` is carried explicitly instead. + let branchEdited = Boolean(prefill.worktreeBranchEdited); const syncBranchToName = () => { if (!branchEdited && branchMode === "new") branchInput.value = nameInput.value; }; @@ -540,6 +550,16 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { /** Decide how the worktree section describes the current target. Called * once the initial load has settled `cwd`. */ function resolveBaseRepo(): void { + // Nothing to resolve when the operator turned the worktree off — the + // section would otherwise rebuild expanded under an unchecked box, and + // fire a `/api/gitroot` probe for a worktree nobody asked for + // (#67 review). + if (!wtEnabled.checked) { + wtBody.hidden = true; + wtHint.hidden = true; + inferredBaseRepo = null; + return; + } if (!cwd) { inferredBaseRepo = null; wtBody.hidden = true; @@ -688,6 +708,7 @@ export function createSpawnForm(opts: SpawnFormOptions): SpawnFormHandle { name: nameInput.value, worktreeEnabled: wtEnabled.checked, worktreeBranch: branchValue(), + worktreeBranchEdited: branchEdited, worktreeBranchMode: branchMode, }, inferredBaseRepo,