From d203db3251af086ed1af81beff8eb8daf9ac86b6 Mon Sep 17 00:00:00 2001 From: dawn <93917549+dawNotPoi@users.noreply.github.com> Date: Fri, 4 Sep 2026 13:09:36 +0800 Subject: [PATCH 01/11] fix(worktree): recover empty cleanup shells --- src-tauri/src/work_task/engine.rs | 96 ++++++++++++++++++++++++++-- src-tauri/src/work_task/git.rs | 103 +++++++++++++++++++++++++++--- 2 files changed, 183 insertions(+), 16 deletions(-) diff --git a/src-tauri/src/work_task/engine.rs b/src-tauri/src/work_task/engine.rs index b2c6bfa716..125ca38632 100644 --- a/src-tauri/src/work_task/engine.rs +++ b/src-tauri/src/work_task/engine.rs @@ -98,17 +98,23 @@ pub(crate) const RETRY_THE_CLEANUP: &str = "Remove them, then retry the cleanup. /// The probe every worktree-removal gate shares: does this checkout still hold /// work (tracked edits or files git has never seen)? /// -/// FAILS CLOSED. Exactly one reason for git being unable to answer is safe to -/// read as "clean" — the checkout is already off disk, so there is nothing left -/// to lose, and the removal paths handle a missing directory on their own. Any -/// OTHER failure (a corrupt index, a permission error, a transient git fault) -/// means we could not prove the directory is safe to destroy, and the operation -/// waiting on this answer is `git worktree remove --force`. Guessing "clean" +/// FAILS CLOSED. Only two reasons for git being unable to answer are safe to +/// read as "clean": the checkout is already off disk, or git removed its +/// registration and contents but left a readable, strictly empty directory +/// behind. Any entry in that shell (ignored and hidden files included), a +/// corrupt index, a permission error, or another transient filesystem failure +/// means we could not prove the directory is safe to destroy. The operation +/// waiting on this answer is `git worktree remove --force`, so guessing "clean" /// there trades a recoverable stall for unrecoverable files. async fn path_holds_uncommitted(path: &str) -> bool { match task_git::has_changes(path).await { Ok(dirty) => dirty, - Err(_) => Path::new(path).exists(), + Err(_) => match std::fs::read_dir(path) { + // `Some(Err(_))` is deliberately still "holds work": even learning + // whether an entry exists has to succeed before removal is safe. + Ok(mut entries) => entries.next().is_some(), + Err(e) => e.kind() != std::io::ErrorKind::NotFound, + }, } } @@ -10131,6 +10137,82 @@ mod tests { assert!(!f.engine.index.lock().await.contains_key(ZOMBIE), "retired"); } + /// Git for Windows can finish the destructive part of `worktree remove` + /// (registration and checkout contents) but fail to remove the now-empty + /// directory. That first pass leaves the task flagged for cleanup; its retry + /// must recognize the harmless shell, finish the branch/DB cleanup, and not + /// report files that do not exist. + #[tokio::test] + async fn worktree_cleanup_retry_converges_an_empty_detached_shell() { + let f = delivery_fixture(FakeForge::default()).await; + let worktree = f.worktree.to_str().expect("utf-8 worktree"); + git_run(&f.root, &["worktree", "remove", "--force", worktree]); + std::fs::create_dir(&f.worktree).expect("empty shell"); + work_task_service::set_cleanup_state( + &f.engine.db.conn, + f.task_id, + true, + Some("the first removal stopped after git detached it".into()), + ) + .await + .expect("flag failed cleanup"); + + f.engine + .cleanup_task(f.task_id) + .await + .expect("retry cleanup"); + + let task = row(&f.engine, f.task_id).await; + assert_eq!(task.cleanup_state, None, "the retry is fully settled"); + assert_eq!( + task.worktree_folder_id, None, + "folder bookkeeping converged" + ); + assert!(!f.worktree.exists(), "the empty shell is gone"); + assert!( + task_git::rev_parse(f.root.to_str().unwrap(), "refs/heads/task/7") + .await + .is_err(), + "the requested work branch goes too" + ); + } + + /// A missing `.git` file alone is not proof that the directory is a harmless + /// post-removal shell. Files may have appeared after the partial teardown, + /// and none of them is recoverable through git, so the retry must preserve + /// both the sentinel and the branch. + #[tokio::test] + async fn worktree_cleanup_retry_preserves_a_file_in_a_detached_shell() { + let f = delivery_fixture(FakeForge::default()).await; + let worktree = f.worktree.to_str().expect("utf-8 worktree"); + git_run(&f.root, &["worktree", "remove", "--force", worktree]); + std::fs::create_dir(&f.worktree).expect("empty shell"); + std::fs::write(f.worktree.join("sentinel.txt"), "not in git\n").expect("sentinel"); + + let err = f + .engine + .cleanup_task(f.task_id) + .await + .expect_err("a non-empty shell is not removable"); + + assert!(err.holds_work, "the retry is refused as a data-safety gate"); + assert_eq!( + std::fs::read_to_string(f.worktree.join("sentinel.txt")).expect("read sentinel"), + "not in git\n" + ); + let task = row(&f.engine, f.task_id).await; + assert!( + task.worktree_folder_id.is_some(), + "the retry remains available" + ); + assert!( + task_git::rev_parse(f.root.to_str().unwrap(), "refs/heads/task/7") + .await + .is_ok(), + "the branch survives with the sentinel" + ); + } + /// The removal underneath is `worktree remove --force`, and a stop pressed /// while the agent is editing is exactly when a checkout is dirty. Nothing /// the user clicked named those files — the cancel dialog's checkbox offers diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index ce436d60df..d9f92e68d1 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -564,9 +564,10 @@ pub async fn branch_holds_unlanded_work( } /// Remove a task worktree directory + its branch. Runs from the project repo. -/// Tolerant of a directory already gone (prunes the stale registration) and of -/// a branch already deleted; `-D` is required because a squash-landed branch is -/// unmerged in git's eyes. +/// Tolerant of a directory already gone (prunes the stale registration), an +/// empty directory shell left by a partially successful removal, and a branch +/// already deleted; `-D` is required because a squash-landed branch is unmerged +/// in git's eyes. /// /// `expected_tip` turns the branch delete into a COMPARE-AND-DELETE: the ref /// goes only if it still points at that commit, and a branch that moved is an @@ -581,13 +582,48 @@ pub async fn remove_worktree_and_branch( work_branch: Option<&str>, expected_tip: Option<&str>, ) -> Result<(), AppCommandError> { - let removed = run_git(repo_path, &["worktree", "remove", "--force", worktree_path]).await?; - if !removed.status.success() { - if std::path::Path::new(worktree_path).exists() { - return Err(git_command_error("worktree remove", &removed.stderr)); + // A real checkout keeps the established git removal path. Only a missing + // marker identifies a detached shell that is safe to consume directly, + // and even then `remove_dir` must atomically prove the shell is still empty. + let marker_exists = + match std::fs::symlink_metadata(std::path::Path::new(worktree_path).join(".git")) { + Ok(_) => true, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => false, + Err(e) => return Err(AppCommandError::io(e)), + }; + let shell_already_gone = if marker_exists { + false + } else { + match std::fs::remove_dir(worktree_path) { + Ok(()) => true, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => true, + Err(e) => return Err(AppCommandError::io(e)), } - // Directory already gone — drop the stale registration so the branch - // delete below isn't blocked by a phantom checkout. + }; + + let mut needs_prune = shell_already_gone; + if !shell_already_gone { + let removed = run_git(repo_path, &["worktree", "remove", "--force", worktree_path]).await?; + if !removed.status.success() { + // On Windows git can remove the registration and every checkout + // file, then fail its final directory removal because a process + // briefly holds the directory open. Recover that exact empty shell, + // including on a later retry. `remove_dir` is intentionally + // non-recursive: an ignored artifact or a file created after the + // clean probe makes it fail closed, preserving both the file and the + // branch below. + match std::fs::remove_dir(worktree_path) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(_) => return Err(git_command_error("worktree remove", &removed.stderr)), + } + needs_prune = true; + } + } + if needs_prune { + // Directory already gone (or its empty shell removed) — drop any stale + // registration so the branch delete below isn't blocked by a phantom + // checkout. let prune = run_git(repo_path, &["worktree", "prune"]).await?; if !prune.status.success() { return Err(git_command_error("worktree prune", &prune.stderr)); @@ -1093,6 +1129,55 @@ mod tests { .expect("a second pass finds nothing to do and says so quietly"); } + /// The engine normally catches contents before calling this layer, but a + /// file can appear after git has removed the checkout marker but before the + /// task retries. Git may still have the path registered and would then + /// recursively remove it, so the missing `.git` marker must fail closed + /// whenever the shell is no longer empty. + #[tokio::test] + async fn registered_worktree_without_git_marker_never_takes_a_sentinel() { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = dir.path().join("repo"); + std::fs::create_dir(&repo).expect("mkdir"); + let repo_path = repo.to_str().expect("utf-8 path"); + git_run(&repo, &["init", "-q", "-b", "main"]); + std::fs::write(repo.join("a.txt"), "one\n").expect("write"); + git_run(&repo, &["add", "-A"]); + git_run(&repo, &["commit", "-q", "-m", "base"]); + + let worktree = dir.path().join("wt-sentinel"); + let worktree_path = worktree.to_str().expect("utf-8 worktree"); + git_run( + &repo, + &[ + "worktree", + "add", + "-q", + "-b", + "task/sentinel", + worktree_path, + ], + ); + std::fs::remove_file(worktree.join(".git")).expect("remove worktree marker"); + std::fs::remove_file(worktree.join("a.txt")).expect("remove tracked contents"); + std::fs::write(worktree.join("sentinel.txt"), "keep me\n").expect("sentinel"); + + remove_worktree_and_branch(repo_path, worktree_path, Some("task/sentinel"), None) + .await + .expect_err("a non-empty detached shell must fail closed"); + + assert_eq!( + std::fs::read_to_string(worktree.join("sentinel.txt")).expect("read sentinel"), + "keep me\n" + ); + assert!( + rev_parse(repo_path, "refs/heads/task/sentinel") + .await + .is_ok(), + "the branch is not deleted after the directory refusal" + ); + } + /// A retry after the checkout was removed must get the SAME branch back, /// prior commits included — not a fresh tree on a fresh base. #[tokio::test] From 8d8cab6516e9cfe3461baf0c263e5614801bd8e5 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 15:53:21 +0800 Subject: [PATCH 02/11] fix(worktree): read a detached shell without asking the enclosing repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `path_holds_uncommitted` measured a post-removal shell with `git status` run from the shell itself, and git walks up from there. A worktree root configured inside the project — a relative `worktree_root` — puts the shell under the project's own `.git`, so git answered about the PROJECT: a project with a stray file of its own made an empty shell read as "holds uncommitted files" and the cleanup retry never converged. Decide from the directory alone whenever the `.git` marker is gone, since a path git no longer speaks for cannot be described by its answer. Report the filesystem half of `remove_worktree_and_branch` through a message that names the path and the OS reason: `AppCommandError`'s `Display` is its `message` alone, so `AppCommandError::io` left bare "I/O operation failed" on the task card. Cover the probe directly, in the DEFAULT worktree layout (beside the project, outside every repository) — the existing fixture nests its worktree inside the repo, where `git status` always answers and the empty-shell branch is never reached. --- src-tauri/src/work_task/engine.rs | 129 ++++++++++++++++++++++++++++-- src-tauri/src/work_task/git.rs | 27 +++++-- 2 files changed, 144 insertions(+), 12 deletions(-) diff --git a/src-tauri/src/work_task/engine.rs b/src-tauri/src/work_task/engine.rs index 125ca38632..6d22ac9e4f 100644 --- a/src-tauri/src/work_task/engine.rs +++ b/src-tauri/src/work_task/engine.rs @@ -106,15 +106,37 @@ pub(crate) const RETRY_THE_CLEANUP: &str = "Remove them, then retry the cleanup. /// means we could not prove the directory is safe to destroy. The operation /// waiting on this answer is `git worktree remove --force`, so guessing "clean" /// there trades a recoverable stall for unrecoverable files. +/// +/// The `.git` marker is checked FIRST, and not as an optimization: `has_changes` +/// runs `git status` with the path as its working directory, and git walks UP +/// from there. A shell with no marker left is not a checkout git can speak for, +/// so whatever it answers is about the repository that ENCLOSES the shell — the +/// project itself whenever the folder's worktree root is a path inside it. That +/// answer is a clean `Ok(false)` or a dirty `Ok(true)` about somebody else's +/// files, and neither is this path's; only the directory probe below is. async fn path_holds_uncommitted(path: &str) -> bool { + match std::fs::symlink_metadata(Path::new(path).join(".git")) { + Ok(_) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return shell_holds_entries(path), + // Could not even look at the marker — no proof of anything. Fail closed. + Err(_) => return true, + } match task_git::has_changes(path).await { Ok(dirty) => dirty, - Err(_) => match std::fs::read_dir(path) { - // `Some(Err(_))` is deliberately still "holds work": even learning - // whether an entry exists has to succeed before removal is safe. - Ok(mut entries) => entries.next().is_some(), - Err(e) => e.kind() != std::io::ErrorKind::NotFound, - }, + Err(_) => shell_holds_entries(path), + } +} + +/// The one thing left to ask about a directory git has stopped speaking for: is +/// it strictly empty? Anything else — an entry of any kind, or a failure to read +/// the directory at all — is "holds work", because a `--force` removal is what +/// waits on the answer. Only a path that is gone reads as nothing to lose. +fn shell_holds_entries(path: &str) -> bool { + match std::fs::read_dir(path) { + // `Some(Err(_))` is deliberately still "holds work": even learning + // whether an entry exists has to succeed before removal is safe. + Ok(mut entries) => entries.next().is_some(), + Err(e) => e.kind() != std::io::ErrorKind::NotFound, } } @@ -10213,6 +10235,101 @@ mod tests { ); } + /// A repository with `layout` applied to it, for the probe tests below: + /// returns `(tempdir, repo path, worktree path)`. The two tests that pass + /// `"sibling"` reproduce the DEFAULT worktree layout — beside the project, + /// outside every repository — which is the only layout in which a shell has + /// no enclosing repository for `git status` to answer about instead. + fn probe_repo(layout: &str) -> (tempfile::TempDir, std::path::PathBuf, std::path::PathBuf) { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = dir.path().join("repo"); + std::fs::create_dir(&repo).expect("mkdir"); + git_run(&repo, &["init", "-q", "-b", "main"]); + std::fs::write(repo.join("a.txt"), "one\n").expect("write"); + git_run(&repo, &["add", "-A"]); + git_run(&repo, &["commit", "-q", "-m", "base"]); + let worktree = match layout { + "sibling" => dir.path().join("wt"), + _ => repo.join("wt"), + }; + (dir, repo, worktree) + } + + /// THE probe behind issue #642's dead-end retry, exercised where the bug + /// actually happens. `has_changes` runs `git status` from the path itself, + /// so a shell in the default layout — beside the project, no repository + /// above it — makes git error, and reading that error as "dirty" is what + /// made the retry offer the same refusal forever. An empty shell has, by + /// definition, nothing a `--force` removal could take. + #[tokio::test] + async fn an_empty_shell_beside_a_project_reads_as_clean() { + let (_dir, _repo, worktree) = probe_repo("sibling"); + std::fs::create_dir(&worktree).expect("empty shell"); + + assert!( + !path_holds_uncommitted(worktree.to_str().expect("utf-8")).await, + "an empty post-removal shell holds nothing" + ); + } + + /// The other half of the same answer: outside a repository git errors on a + /// shell whatever is in it, so emptiness is the ONLY thing separating a + /// harmless leftover from files nothing can give back. + #[tokio::test] + async fn a_file_in_a_shell_beside_a_project_still_reads_as_dirty() { + let (_dir, _repo, worktree) = probe_repo("sibling"); + std::fs::create_dir(&worktree).expect("shell"); + std::fs::write(worktree.join("sentinel.txt"), "not in git\n").expect("sentinel"); + + assert!( + path_holds_uncommitted(worktree.to_str().expect("utf-8")).await, + "a shell with an entry in it is not removable" + ); + } + + /// A worktree root configured INSIDE the project (a relative + /// `worktree_root`, which the setting takes at face value) puts the shell + /// under the project's own `.git`. `git status` then answers happily — about + /// the PROJECT — so a project with a stray file of its own would answer + /// "dirty" for a shell holding nothing, and #642's retry would still never + /// converge. Without a `.git` marker the path is not a checkout git speaks + /// for, and its answer must not be read as this path's. + #[tokio::test] + async fn an_empty_shell_inside_a_dirty_project_reads_as_clean() { + let (_dir, repo, worktree) = probe_repo("nested"); + std::fs::write(repo.join("stray.txt"), "the project's own mess\n").expect("stray"); + std::fs::create_dir(&worktree).expect("empty shell"); + + assert!( + !path_holds_uncommitted(worktree.to_str().expect("utf-8")).await, + "the project's dirt is not the shell's" + ); + } + + /// And the marker check must not short-circuit the case it is guarding: a + /// checkout that still HAS its `.git` is exactly what `git status` is for, + /// uncommitted work included. + #[tokio::test] + async fn an_intact_worktree_is_still_measured_by_git() { + let (_dir, repo, worktree) = probe_repo("sibling"); + let worktree_path = worktree.to_str().expect("utf-8"); + git_run( + &repo, + &["worktree", "add", "-q", "-b", "task/probe", worktree_path], + ); + assert!( + !path_holds_uncommitted(worktree_path).await, + "a fresh checkout holds nothing" + ); + + std::fs::write(worktree.join("edit.txt"), "unstaged\n").expect("edit"); + + assert!( + path_holds_uncommitted(worktree_path).await, + "an uncommitted file in a live checkout is work" + ); + } + /// The removal underneath is `worktree remove --force`, and a stop pressed /// while the agent is editing is exactly when a checkout is dirty. Nothing /// the user clicked named those files — the cancel dialog's checkbox offers diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index d9f92e68d1..e88fadb08d 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -563,6 +563,19 @@ pub async fn branch_holds_unlanded_work( } } +/// The failure of [`remove_worktree_and_branch`]'s filesystem half reports +/// through the SAME channel its git half does — `cleanup_state`, rendered +/// verbatim on the task card — and `AppCommandError`'s `Display` is its +/// `message` alone, detail dropped. So the message has to carry the path and +/// the OS reason itself: `AppCommandError::io` would leave "I/O operation +/// failed" on that card and nothing else, which is the dead end this whole +/// path exists to get users out of. +fn shell_error(worktree_path: &str, what: &str, err: &std::io::Error) -> AppCommandError { + AppCommandError::io_error(format!( + "the worktree directory '{worktree_path}' {what}: {err}" + )) +} + /// Remove a task worktree directory + its branch. Runs from the project repo. /// Tolerant of a directory already gone (prunes the stale registration), an /// empty directory shell left by a partially successful removal, and a branch @@ -589,7 +602,7 @@ pub async fn remove_worktree_and_branch( match std::fs::symlink_metadata(std::path::Path::new(worktree_path).join(".git")) { Ok(_) => true, Err(e) if e.kind() == std::io::ErrorKind::NotFound => false, - Err(e) => return Err(AppCommandError::io(e)), + Err(e) => return Err(shell_error(worktree_path, "could not be read", &e)), }; let shell_already_gone = if marker_exists { false @@ -597,7 +610,7 @@ pub async fn remove_worktree_and_branch( match std::fs::remove_dir(worktree_path) { Ok(()) => true, Err(e) if e.kind() == std::io::ErrorKind::NotFound => true, - Err(e) => return Err(AppCommandError::io(e)), + Err(e) => return Err(shell_error(worktree_path, "could not be removed", &e)), } }; @@ -1130,10 +1143,12 @@ mod tests { } /// The engine normally catches contents before calling this layer, but a - /// file can appear after git has removed the checkout marker but before the - /// task retries. Git may still have the path registered and would then - /// recursively remove it, so the missing `.git` marker must fail closed - /// whenever the shell is no longer empty. + /// file can appear after git has removed the checkout marker and before the + /// task retries. Git is not the risk here — it validates the `.git` marker + /// and refuses the removal outright, contents untouched. The risk is the + /// marker-less path this function grew FOR empty shells, which is the only + /// code that will delete such a directory at all: it must stay + /// non-recursive, so that a file makes it fail closed and keep the branch. #[tokio::test] async fn registered_worktree_without_git_marker_never_takes_a_sentinel() { let dir = tempfile::tempdir().expect("tempdir"); From 95de195002dc9c9c5dc3825dfe132afcccde29ab Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 16:55:32 +0800 Subject: [PATCH 03/11] fix(worktree): probe the directory git would act on, and say why it stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the removal's filesystem probes from the project repository, the way git resolves the same argument: `std::fs` would have used the app's own working directory, so a project sitting at a filesystem root — where `sibling_path` has nothing to prefix and returns a bare name — could have `remove_dir` reach for a namesake beside the app while git still held the real checkout. Keep the OS error code on the filesystem failures instead of flattening every one to `IoError`, and correct where that message is read: it lands in the task's `cleanup_failed` timeline event, not on the card. Make the two probe tests measure a modified TRACKED file. `has_changes` spawns git through the inherited environment, so an untracked fixture is only as visible as the developer's global ignore rules allow. --- src-tauri/src/work_task/engine.rs | 18 ++++++++--- src-tauri/src/work_task/git.rs | 52 ++++++++++++++++++++----------- 2 files changed, 48 insertions(+), 22 deletions(-) diff --git a/src-tauri/src/work_task/engine.rs b/src-tauri/src/work_task/engine.rs index 6d22ac9e4f..cb67b1f2d7 100644 --- a/src-tauri/src/work_task/engine.rs +++ b/src-tauri/src/work_task/engine.rs @@ -10294,10 +10294,17 @@ mod tests { /// "dirty" for a shell holding nothing, and #642's retry would still never /// converge. Without a `.git` marker the path is not a checkout git speaks /// for, and its answer must not be read as this path's. + /// + /// The project's dirt is an edit to a TRACKED file, not a stray untracked + /// one: `has_changes` spawns git through `crate::process`, which inherits + /// the real environment, so an untracked fixture is only as visible as the + /// developer's global `core.excludesFile` lets it be — and a fixture git + /// silently ignores would make this test pass against the very bug it + /// exists to pin. No ignore rule can hide a modified tracked file. #[tokio::test] async fn an_empty_shell_inside_a_dirty_project_reads_as_clean() { let (_dir, repo, worktree) = probe_repo("nested"); - std::fs::write(repo.join("stray.txt"), "the project's own mess\n").expect("stray"); + std::fs::write(repo.join("a.txt"), "the project's own mess\n").expect("dirty project"); std::fs::create_dir(&worktree).expect("empty shell"); assert!( @@ -10308,7 +10315,10 @@ mod tests { /// And the marker check must not short-circuit the case it is guarding: a /// checkout that still HAS its `.git` is exactly what `git status` is for, - /// uncommitted work included. + /// uncommitted work included. Tracked and modified for the reason above — + /// an untracked fixture would read as clean under a global ignore rule that + /// happens to match it, and fail this assertion on that developer's machine + /// alone. #[tokio::test] async fn an_intact_worktree_is_still_measured_by_git() { let (_dir, repo, worktree) = probe_repo("sibling"); @@ -10322,11 +10332,11 @@ mod tests { "a fresh checkout holds nothing" ); - std::fs::write(worktree.join("edit.txt"), "unstaged\n").expect("edit"); + std::fs::write(worktree.join("a.txt"), "unstaged\n").expect("edit"); assert!( path_holds_uncommitted(worktree_path).await, - "an uncommitted file in a live checkout is work" + "an uncommitted edit in a live checkout is work" ); } diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index e88fadb08d..940949f80e 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -3,7 +3,7 @@ //! (mirroring `commands::folders`), composed by the task engine which owns the //! per-folder git mutex. -use crate::app_error::AppCommandError; +use crate::app_error::{AppCommandError, AppErrorCode}; use crate::commands::folders::{detect_conflicts, git_command_error}; use crate::models::WorkTaskChangedFile; @@ -564,16 +564,23 @@ pub async fn branch_holds_unlanded_work( } /// The failure of [`remove_worktree_and_branch`]'s filesystem half reports -/// through the SAME channel its git half does — `cleanup_state`, rendered -/// verbatim on the task card — and `AppCommandError`'s `Display` is its -/// `message` alone, detail dropped. So the message has to carry the path and -/// the OS reason itself: `AppCommandError::io` would leave "I/O operation -/// failed" on that card and nothing else, which is the dead end this whole -/// path exists to get users out of. +/// through the SAME channel its git half does: the caller stringifies it into +/// the task's `cleanup_failed` timeline event, which the detail sheet renders +/// verbatim as the reason the cleanup stopped. `AppCommandError`'s `Display` +/// is its `message` alone — `detail` never reaches that line — so the message +/// has to carry the path and the OS reason itself. `AppCommandError::io` would +/// leave "I/O operation failed" there and nothing else, which is the dead end +/// this whole path exists to get users out of. The code still gets mapped, +/// because a refusal the OS blamed on permissions is not a generic fault. fn shell_error(worktree_path: &str, what: &str, err: &std::io::Error) -> AppCommandError { - AppCommandError::io_error(format!( - "the worktree directory '{worktree_path}' {what}: {err}" - )) + let code = match err.kind() { + std::io::ErrorKind::PermissionDenied => AppErrorCode::PermissionDenied, + _ => AppErrorCode::IoError, + }; + AppCommandError::new( + code, + format!("the worktree directory '{worktree_path}' {what}: {err}"), + ) } /// Remove a task worktree directory + its branch. Runs from the project repo. @@ -595,19 +602,28 @@ pub async fn remove_worktree_and_branch( work_branch: Option<&str>, expected_tip: Option<&str>, ) -> Result<(), AppCommandError> { + // The filesystem probes below have to land on the DIRECTORY GIT WOULD ACT + // ON, and git resolves `worktree_path` from `repo_path` (that is where + // `run_git` runs) while `std::fs` would resolve it from the process's own + // working directory. Absolute paths — every path `worktree_path_in` builds + // from an absolute project — make this join a no-op; a relative one (a + // project sitting at a filesystem root leaves `sibling_path` with nothing + // to prefix) would otherwise have the probe answer about a namesake beside + // the app, and a `remove_dir` reaching for a directory nobody asked about + // is the one mistake this function must not make. + let target = std::path::Path::new(repo_path).join(worktree_path); // A real checkout keeps the established git removal path. Only a missing // marker identifies a detached shell that is safe to consume directly, // and even then `remove_dir` must atomically prove the shell is still empty. - let marker_exists = - match std::fs::symlink_metadata(std::path::Path::new(worktree_path).join(".git")) { - Ok(_) => true, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => false, - Err(e) => return Err(shell_error(worktree_path, "could not be read", &e)), - }; + let marker_exists = match std::fs::symlink_metadata(target.join(".git")) { + Ok(_) => true, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => false, + Err(e) => return Err(shell_error(worktree_path, "could not be read", &e)), + }; let shell_already_gone = if marker_exists { false } else { - match std::fs::remove_dir(worktree_path) { + match std::fs::remove_dir(&target) { Ok(()) => true, Err(e) if e.kind() == std::io::ErrorKind::NotFound => true, Err(e) => return Err(shell_error(worktree_path, "could not be removed", &e)), @@ -625,7 +641,7 @@ pub async fn remove_worktree_and_branch( // non-recursive: an ignored artifact or a file created after the // clean probe makes it fail closed, preserving both the file and the // branch below. - match std::fs::remove_dir(worktree_path) { + match std::fs::remove_dir(&target) { Ok(()) => {} Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} Err(_) => return Err(git_command_error("worktree remove", &removed.stderr)), From bb51a25bddd1150cec0dbc28b398ee9a2397cb5a Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 16:58:05 +0800 Subject: [PATCH 04/11] fix(worktree): name the resolved directory in the removal's io failures --- src-tauri/src/work_task/git.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index 940949f80e..c9d7506380 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -572,14 +572,16 @@ pub async fn branch_holds_unlanded_work( /// leave "I/O operation failed" there and nothing else, which is the dead end /// this whole path exists to get users out of. The code still gets mapped, /// because a refusal the OS blamed on permissions is not a generic fault. -fn shell_error(worktree_path: &str, what: &str, err: &std::io::Error) -> AppCommandError { +/// Takes the RESOLVED path, so the sentence names the directory the call +/// actually touched rather than the argument it started from. +fn shell_error(target: &std::path::Path, what: &str, err: &std::io::Error) -> AppCommandError { let code = match err.kind() { std::io::ErrorKind::PermissionDenied => AppErrorCode::PermissionDenied, _ => AppErrorCode::IoError, }; AppCommandError::new( code, - format!("the worktree directory '{worktree_path}' {what}: {err}"), + format!("the worktree directory '{}' {what}: {err}", target.display()), ) } @@ -618,7 +620,7 @@ pub async fn remove_worktree_and_branch( let marker_exists = match std::fs::symlink_metadata(target.join(".git")) { Ok(_) => true, Err(e) if e.kind() == std::io::ErrorKind::NotFound => false, - Err(e) => return Err(shell_error(worktree_path, "could not be read", &e)), + Err(e) => return Err(shell_error(&target, "could not be read", &e)), }; let shell_already_gone = if marker_exists { false @@ -626,7 +628,7 @@ pub async fn remove_worktree_and_branch( match std::fs::remove_dir(&target) { Ok(()) => true, Err(e) if e.kind() == std::io::ErrorKind::NotFound => true, - Err(e) => return Err(shell_error(worktree_path, "could not be removed", &e)), + Err(e) => return Err(shell_error(&target, "could not be removed", &e)), } }; From 3ed2897111aae35839a00fe5c0177cd583a744bd Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 17:32:58 +0800 Subject: [PATCH 05/11] fix(worktree): correct the rationale the removal probes carry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The join was justified with a project sitting at a filesystem root, but that project cannot have a worktree at all: `basename` is empty there, so the generated name starts with a dash and `git worktree add` reads it as options. State what the join actually buys — the removal and its own probe agreeing on one directory — and say plainly that it does not make a relative folder path coherent, because the engine's gates still read that path from the app's working directory. Map `AlreadyExists` too, which POSIX allows `rmdir` to return for a non-empty directory, so replacing `AppCommandError::io` costs only its message. Stop the probe fixture from claiming a guarantee it cannot give about repositories above the system temp directory. --- src-tauri/src/work_task/engine.rs | 9 +++++++-- src-tauri/src/work_task/git.rs | 24 +++++++++++++++--------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src-tauri/src/work_task/engine.rs b/src-tauri/src/work_task/engine.rs index cb67b1f2d7..a360791b38 100644 --- a/src-tauri/src/work_task/engine.rs +++ b/src-tauri/src/work_task/engine.rs @@ -10238,8 +10238,13 @@ mod tests { /// A repository with `layout` applied to it, for the probe tests below: /// returns `(tempdir, repo path, worktree path)`. The two tests that pass /// `"sibling"` reproduce the DEFAULT worktree layout — beside the project, - /// outside every repository — which is the only layout in which a shell has - /// no enclosing repository for `git status` to answer about instead. + /// with no repository of the fixture's own above it, which is the layout in + /// which a shell has no enclosing repository for `git status` to answer + /// about instead. (Nothing here can rule out a repository ABOVE the system + /// temp directory; in that environment the sibling pair still asserts the + /// right answers, but it is `an_empty_shell_inside_a_dirty_project_reads_as_clean` + /// — which builds its own enclosing repository — that pins the regression + /// unconditionally.) fn probe_repo(layout: &str) -> (tempfile::TempDir, std::path::PathBuf, std::path::PathBuf) { let dir = tempfile::tempdir().expect("tempdir"); let repo = dir.path().join("repo"); diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index c9d7506380..c545e659a6 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -575,8 +575,13 @@ pub async fn branch_holds_unlanded_work( /// Takes the RESOLVED path, so the sentence names the directory the call /// actually touched rather than the argument it started from. fn shell_error(target: &std::path::Path, what: &str, err: &std::io::Error) -> AppCommandError { + // The same kinds `AppCommandError::io` distinguishes, so replacing it costs + // only the message. `AlreadyExists` is in the list because POSIX lets + // `rmdir` report a non-empty directory as `EEXIST` rather than `ENOTEMPTY`. let code = match err.kind() { + std::io::ErrorKind::NotFound => AppErrorCode::NotFound, std::io::ErrorKind::PermissionDenied => AppErrorCode::PermissionDenied, + std::io::ErrorKind::AlreadyExists => AppErrorCode::AlreadyExists, _ => AppErrorCode::IoError, }; AppCommandError::new( @@ -604,15 +609,16 @@ pub async fn remove_worktree_and_branch( work_branch: Option<&str>, expected_tip: Option<&str>, ) -> Result<(), AppCommandError> { - // The filesystem probes below have to land on the DIRECTORY GIT WOULD ACT - // ON, and git resolves `worktree_path` from `repo_path` (that is where - // `run_git` runs) while `std::fs` would resolve it from the process's own - // working directory. Absolute paths — every path `worktree_path_in` builds - // from an absolute project — make this join a no-op; a relative one (a - // project sitting at a filesystem root leaves `sibling_path` with nothing - // to prefix) would otherwise have the probe answer about a namesake beside - // the app, and a `remove_dir` reaching for a directory nobody asked about - // is the one mistake this function must not make. + // The filesystem probes below have to land on the DIRECTORY THE GIT CALL + // BELOW WOULD ACT ON. Git resolves `worktree_path` from `repo_path` — that + // is where `run_git` runs — while `std::fs` resolves it from the app's own + // working directory, and folder paths are stored exactly as they were + // given. A no-op for an ordinary absolute path, and for anything else it + // keeps the one destructive call in this function from reaching a namesake + // beside the app instead of the checkout the caller named. It does not make + // a relative folder path COHERENT — the engine's own gates read that same + // path from the app's working directory, so they would still be measuring + // somewhere else — it only keeps the removal and its own probe together. let target = std::path::Path::new(repo_path).join(worktree_path); // A real checkout keeps the established git removal path. Only a missing // marker identifies a detached shell that is safe to consume directly, From f4988da34f71b20d70ad9a3c10268609f6966c2c Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 20:49:36 +0800 Subject: [PATCH 06/11] refactor(worktree): one removal path instead of a pre-check that predicts git MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-check existed to keep a marker-less directory away from `git worktree remove`, but git needs no such help: it validates the `.git` marker before deleting anything and refuses outright, contents untouched — measured across all four shapes (registered or not, empty or not). Asking git first is therefore free, and the fallback it already had recovers every leftover the pre-check did. What made that safe to collapse is `remove_dir` being non-recursive: it can only ever succeed on an empty directory, so a shell holding a file fails closed whichever call reaches it. The one thing the pre-check did carry is which failure to report, so that moves to where it belongs. A checkout git still speaks for keeps git's reporting; a detached shell has none worth keeping — git's only word about it names a `.git` the user never heard of, while the removal that failed was ours. Both directions are pinned, the second by a locked worktree. --- src-tauri/src/work_task/git.rs | 157 +++++++++++++++++++++++---------- 1 file changed, 112 insertions(+), 45 deletions(-) diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index c545e659a6..cfdb30352f 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -563,18 +563,34 @@ pub async fn branch_holds_unlanded_work( } } -/// The failure of [`remove_worktree_and_branch`]'s filesystem half reports -/// through the SAME channel its git half does: the caller stringifies it into +/// Which of the two refusals to report once git has declined a removal AND the +/// leftover directory could not be consumed either. +/// +/// A checkout git still speaks for keeps git's established reporting — stderr +/// in `detail`, exactly as every other git failure in this function does it. +/// A DETACHED shell has no such thing to keep: git's only word about it is a +/// stock sentence naming a `.git` the user never heard of, and the removal that +/// actually failed was OURS. So this path owns its message, and it says the two +/// things a user can act on — which directory, and what is still in it. +/// +/// That has to travel in `message`, because the caller stringifies this into /// the task's `cleanup_failed` timeline event, which the detail sheet renders -/// verbatim as the reason the cleanup stopped. `AppCommandError`'s `Display` -/// is its `message` alone — `detail` never reaches that line — so the message -/// has to carry the path and the OS reason itself. `AppCommandError::io` would -/// leave "I/O operation failed" there and nothing else, which is the dead end -/// this whole path exists to get users out of. The code still gets mapped, -/// because a refusal the OS blamed on permissions is not a generic fault. -/// Takes the RESOLVED path, so the sentence names the directory the call -/// actually touched rather than the argument it started from. -fn shell_error(target: &std::path::Path, what: &str, err: &std::io::Error) -> AppCommandError { +/// verbatim, and `AppCommandError`'s `Display` is its `message` alone — +/// `detail` never reaches that line. `AppCommandError::io` would leave "I/O +/// operation failed" there and nothing else, the same dead end this path exists +/// to get users out of. Takes the RESOLVED path, so the sentence names the +/// directory the call actually touched rather than the argument it started +/// from. +fn removal_refused( + target: &std::path::Path, + git_stderr: &[u8], + err: &std::io::Error, +) -> AppCommandError { + // An unreadable marker counts as detached: this only picks a message, and + // whatever stopped the read is the more useful of the two either way. + if std::fs::symlink_metadata(target.join(".git")).is_ok() { + return git_command_error("worktree remove", git_stderr); + } // The same kinds `AppCommandError::io` distinguishes, so replacing it costs // only the message. `AlreadyExists` is in the list because POSIX lets // `rmdir` report a non-empty directory as `EEXIST` rather than `ENOTEMPTY`. @@ -586,7 +602,10 @@ fn shell_error(target: &std::path::Path, what: &str, err: &std::io::Error) -> Ap }; AppCommandError::new( code, - format!("the worktree directory '{}' {what}: {err}", target.display()), + format!( + "the worktree directory '{}' could not be removed: {err}", + target.display() + ), ) } @@ -620,43 +639,32 @@ pub async fn remove_worktree_and_branch( // path from the app's working directory, so they would still be measuring // somewhere else — it only keeps the removal and its own probe together. let target = std::path::Path::new(repo_path).join(worktree_path); - // A real checkout keeps the established git removal path. Only a missing - // marker identifies a detached shell that is safe to consume directly, - // and even then `remove_dir` must atomically prove the shell is still empty. - let marker_exists = match std::fs::symlink_metadata(target.join(".git")) { - Ok(_) => true, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => false, - Err(e) => return Err(shell_error(&target, "could not be read", &e)), - }; - let shell_already_gone = if marker_exists { + // Git first, always — including for a directory it will refuse. Refusing is + // ALL it does: `worktree remove` validates the `.git` marker before it + // deletes anything, so a shell git will not speak for arrives at the + // recovery below exactly as it was. Asking it first is therefore free of + // risk, and it leaves ONE removal path here instead of a pre-check that + // has to re-derive what git is about to say. + let removed = run_git(repo_path, &["worktree", "remove", "--force", worktree_path]).await?; + let needs_prune = if removed.status.success() { false } else { + // Every shape of leftover recovers the same way, so they share a line: + // a directory already gone, the empty shell that outlives a detached + // registration (#642), and the one Windows leaves when a process holds + // the directory open through git's final rmdir. + // + // `remove_dir` is what makes that safe to say. It is non-recursive, so + // it can only ever succeed on an EMPTY directory: an ignored artifact, + // or a file that appeared after the engine's own check, fails closed + // and keeps both the file and the branch below. match std::fs::remove_dir(&target) { - Ok(()) => true, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => true, - Err(e) => return Err(shell_error(&target, "could not be removed", &e)), + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(removal_refused(&target, &removed.stderr, &e)), } + true }; - - let mut needs_prune = shell_already_gone; - if !shell_already_gone { - let removed = run_git(repo_path, &["worktree", "remove", "--force", worktree_path]).await?; - if !removed.status.success() { - // On Windows git can remove the registration and every checkout - // file, then fail its final directory removal because a process - // briefly holds the directory open. Recover that exact empty shell, - // including on a later retry. `remove_dir` is intentionally - // non-recursive: an ignored artifact or a file created after the - // clean probe makes it fail closed, preserving both the file and the - // branch below. - match std::fs::remove_dir(&target) { - Ok(()) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(_) => return Err(git_command_error("worktree remove", &removed.stderr)), - } - needs_prune = true; - } - } if needs_prune { // Directory already gone (or its empty shell removed) — drop any stale // registration so the branch delete below isn't blocked by a phantom @@ -1201,7 +1209,7 @@ mod tests { std::fs::remove_file(worktree.join("a.txt")).expect("remove tracked contents"); std::fs::write(worktree.join("sentinel.txt"), "keep me\n").expect("sentinel"); - remove_worktree_and_branch(repo_path, worktree_path, Some("task/sentinel"), None) + let err = remove_worktree_and_branch(repo_path, worktree_path, Some("task/sentinel"), None) .await .expect_err("a non-empty detached shell must fail closed"); @@ -1215,6 +1223,65 @@ mod tests { .is_ok(), "the branch is not deleted after the directory refusal" ); + // `Display` is `message` alone, and this is the string the cleanup + // event shows. Git's own attempt failed first, but with a sentence + // about a missing `.git` — the reason cleanup actually stopped is that + // OUR removal found something in the directory, so that is what has to + // come out the other end. + let msg = err.to_string(); + assert!( + msg.contains(worktree_path) && msg.contains("could not be removed"), + "the refusal names the directory and its reason: {msg}" + ); + } + + /// The other side of that choice: a worktree git still speaks for is git's + /// to refuse, and its refusal must not be overwritten by ours. The + /// `remove_dir` recovery still runs here and still fails — a live checkout + /// is not empty — so this pins that failing SECOND does not make it the + /// story. A lock is the cleanest way to make git decline a healthy + /// checkout, and it is exactly the case where git's own text carries + /// something no filesystem error could ("use 'remove -f -f' to override"). + #[tokio::test] + async fn a_locked_worktree_keeps_gits_own_refusal() { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = dir.path().join("repo"); + std::fs::create_dir(&repo).expect("mkdir"); + let repo_path = repo.to_str().expect("utf-8 path"); + git_run(&repo, &["init", "-q", "-b", "main"]); + std::fs::write(repo.join("a.txt"), "one\n").expect("write"); + git_run(&repo, &["add", "-A"]); + git_run(&repo, &["commit", "-q", "-m", "base"]); + + let worktree = dir.path().join("wt-locked"); + let worktree_path = worktree.to_str().expect("utf-8 worktree"); + git_run( + &repo, + &["worktree", "add", "-q", "-b", "task/locked", worktree_path], + ); + git_run(&repo, &["worktree", "lock", worktree_path]); + + let err = remove_worktree_and_branch(repo_path, worktree_path, Some("task/locked"), None) + .await + .expect_err("a locked worktree is not removed"); + + assert_eq!( + err.message, "git worktree remove failed", + "git's refusal is reported as git's, not as a directory we failed to remove" + ); + assert!( + err.detail.as_deref().unwrap_or_default().contains("locked"), + "git's reason is kept where this file always puts it: {:?}", + err.detail + ); + assert!( + worktree.join("a.txt").exists(), + "the locked checkout is left standing" + ); + assert!( + rev_parse(repo_path, "refs/heads/task/locked").await.is_ok(), + "and so is its branch" + ); } /// A retry after the checkout was removed must get the SAME branch back, From 28709698bc98a3b3f4abc6be09b0a4a6b3c6f09e Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 21:15:05 +0800 Subject: [PATCH 07/11] fix(worktree): resolve the removal path before git can suffix-match it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git looks a `` argument up by unique path suffix BEFORE it resolves it as a path, so a relative name is not scoped to the repository it is handed to. With a checkout registered at `/tmp/elsewhere/repo-task-7`, `git worktree remove --force repo-task-7` run from an unrelated repository deletes that one, uncommitted files included — measured, and now pinned by a test that loses `precious.txt` without this change. Folder paths are stored exactly as they were given, so the argument was only ever as absolute as whoever made the folder. Resolving it here also collapses the last gap between the two halves of this function: git and `std::fs` now act on one directory that neither can reinterpret. --- src-tauri/src/work_task/git.rs | 88 +++++++++++++++++++++++++++++----- 1 file changed, 76 insertions(+), 12 deletions(-) diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index cfdb30352f..dc66628ff3 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -628,24 +628,34 @@ pub async fn remove_worktree_and_branch( work_branch: Option<&str>, expected_tip: Option<&str>, ) -> Result<(), AppCommandError> { - // The filesystem probes below have to land on the DIRECTORY THE GIT CALL - // BELOW WOULD ACT ON. Git resolves `worktree_path` from `repo_path` — that - // is where `run_git` runs — while `std::fs` resolves it from the app's own - // working directory, and folder paths are stored exactly as they were - // given. A no-op for an ordinary absolute path, and for anything else it - // keeps the one destructive call in this function from reaching a namesake - // beside the app instead of the checkout the caller named. It does not make - // a relative folder path COHERENT — the engine's own gates read that same - // path from the app's working directory, so they would still be measuring - // somewhere else — it only keeps the removal and its own probe together. - let target = std::path::Path::new(repo_path).join(worktree_path); + // ONE resolved directory, shared by both halves of this function, and it + // has to be ABSOLUTE before git sees it. Git resolves a `` + // argument by unique path SUFFIX first and only then as a path, so a + // relative name reaches a registered worktree anywhere on disk and + // `--force` deletes it: with a checkout registered at + // `/tmp/elsewhere/repo-task-7`, `git worktree remove --force repo-task-7` + // run from an unrelated repo deletes THAT one, uncommitted files included + // (measured; `a_relative_path_cannot_reach_a_worktree_somewhere_else` + // pins it). An absolute path suffix-matches nothing but itself. + // + // Joining from `repo_path` is what git does with a relative path — that is + // `run_git`'s working directory — and `std::path::absolute` then applies + // the same process working directory the child would inherit. So the git + // call and the `std::fs` call below cannot land on different directories, + // which for the one destructive call here is the whole point. Folder paths + // are stored exactly as they were given, so neither is hypothetical. + let target = std::path::absolute(std::path::Path::new(repo_path).join(worktree_path)) + .map_err(AppCommandError::io)?; + // Lossy only to hand git a `&str`. A mangled path is still absolute, so the + // worst it can do is make git refuse; every removal below uses `target`. + let target_arg = target.to_string_lossy().into_owned(); // Git first, always — including for a directory it will refuse. Refusing is // ALL it does: `worktree remove` validates the `.git` marker before it // deletes anything, so a shell git will not speak for arrives at the // recovery below exactly as it was. Asking it first is therefore free of // risk, and it leaves ONE removal path here instead of a pre-check that // has to re-derive what git is about to say. - let removed = run_git(repo_path, &["worktree", "remove", "--force", worktree_path]).await?; + let removed = run_git(repo_path, &["worktree", "remove", "--force", &target_arg]).await?; let needs_prune = if removed.status.success() { false } else { @@ -1235,6 +1245,60 @@ mod tests { ); } + /// Git looks a `` argument up by unique path SUFFIX before it + /// resolves it as a path, so a RELATIVE name is not scoped to the + /// repository it is handed to — it reaches a registered checkout anywhere + /// on disk, and `--force` deletes that one, uncommitted files included. + /// Folder paths are stored exactly as they were given, so the argument is + /// only as absolute as whoever created the folder made it. This is the one + /// call in this file that can destroy a checkout, so it resolves the path + /// itself rather than trusting git to scope it. + #[tokio::test] + async fn a_relative_path_cannot_reach_a_worktree_somewhere_else() { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = dir.path().join("repo"); + std::fs::create_dir(&repo).expect("mkdir"); + let repo_path = repo.to_str().expect("utf-8 path"); + git_run(&repo, &["init", "-q", "-b", "main"]); + std::fs::write(repo.join("a.txt"), "one\n").expect("write"); + git_run(&repo, &["add", "-A"]); + git_run(&repo, &["commit", "-q", "-m", "base"]); + + // A real, registered checkout that shares only its LAST path component + // with the directory the caller names. + let elsewhere = dir.path().join("elsewhere").join("repo-task-7"); + std::fs::create_dir_all(elsewhere.parent().expect("parent")).expect("mkdir"); + let elsewhere_path = elsewhere.to_str().expect("utf-8 path"); + git_run( + &repo, + &["worktree", "add", "-q", "-b", "task/7", elsewhere_path], + ); + std::fs::write(elsewhere.join("precious.txt"), "not yours\n").expect("precious"); + + // The caller names `repo-task-7` beside the project — which does not + // exist. Nothing here may travel to the checkout that does. + remove_worktree_and_branch(repo_path, "repo-task-7", None, None) + .await + .expect("a path that is not a worktree is nothing to do"); + + assert_eq!( + std::fs::read_to_string(elsewhere.join("precious.txt")).expect("read precious"), + "not yours\n", + "the unrelated checkout keeps its uncommitted work" + ); + assert!( + elsewhere.join("a.txt").exists(), + "and the rest of its tree" + ); + let list = run_git(repo_path, &["worktree", "list"]) + .await + .expect("list worktrees"); + assert!( + String::from_utf8_lossy(&list.stdout).contains(elsewhere_path), + "and its registration: the prune must not have swept it either" + ); + } + /// The other side of that choice: a worktree git still speaks for is git's /// to refuse, and its refusal must not be overwritten by ours. The /// `remove_dir` recovery still runs here and still fails — a live checkout From fb7947f6cabeedb42f32e8f9823b474b76a6f1ef Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 21:32:48 +0800 Subject: [PATCH 08/11] fix(worktree): refuse a removal path that resolves two ways Two inputs could still send git somewhere the filesystem half was not looking. A Windows drive-relative path makes `join` drop `repo_path` entirely, leaving `C:trees` for a per-drive current directory the app and git do not share. And a path the OS holds as non-UTF-8 came out of `to_string_lossy` as a DIFFERENT absolute path, which git would have gone and deleted while every removal here still used the original bytes. Both now stop the removal rather than guess at it. Handing git a directory other than the one we probed is the single thing this resolution exists to prevent, so it cannot be the thing it falls back to. --- src-tauri/src/work_task/git.rs | 36 ++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index dc66628ff3..dd0cc391f5 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -644,18 +644,42 @@ pub async fn remove_worktree_and_branch( // call and the `std::fs` call below cannot land on different directories, // which for the one destructive call here is the whole point. Folder paths // are stored exactly as they were given, so neither is hypothetical. - let target = std::path::absolute(std::path::Path::new(repo_path).join(worktree_path)) - .map_err(AppCommandError::io)?; - // Lossy only to hand git a `&str`. A mangled path is still absolute, so the - // worst it can do is make git refuse; every removal below uses `target`. - let target_arg = target.to_string_lossy().into_owned(); + let joined = std::path::Path::new(repo_path).join(worktree_path); + // `join` drops the base when the right-hand side brings its own prefix. On + // Unix that means an absolute path, which is what we want. On Windows it + // also means a DRIVE-RELATIVE one (`C:trees`), which names a directory only + // a per-drive current directory can finish — and the app's is not the one + // git would use from `repo_path`, so the two halves would resolve it apart. + // There is no safe guess about which directory that is, and this function + // deletes directories, so it stops instead. + if joined.is_relative() && !joined.starts_with(repo_path) { + return Err(AppCommandError::new( + AppErrorCode::InvalidInput, + format!( + "the worktree path '{worktree_path}' is relative to a drive rather than \ + to '{repo_path}', so it does not name one directory" + ), + )); + } + let target = std::path::absolute(&joined).map_err(AppCommandError::io)?; + // Refused rather than made lossy: a `\u{FFFD}` substituted into an absolute + // path is still an absolute path, and git would go delete THAT one while + // every removal below still used the bytes in `target`. Handing git a + // different directory than the one we probed is the single thing this + // resolution exists to prevent. + let Some(target_arg) = target.to_str() else { + return Err(AppCommandError::new( + AppErrorCode::InvalidInput, + format!("the worktree path '{worktree_path}' does not resolve to valid UTF-8"), + )); + }; // Git first, always — including for a directory it will refuse. Refusing is // ALL it does: `worktree remove` validates the `.git` marker before it // deletes anything, so a shell git will not speak for arrives at the // recovery below exactly as it was. Asking it first is therefore free of // risk, and it leaves ONE removal path here instead of a pre-check that // has to re-derive what git is about to say. - let removed = run_git(repo_path, &["worktree", "remove", "--force", &target_arg]).await?; + let removed = run_git(repo_path, &["worktree", "remove", "--force", target_arg]).await?; let needs_prune = if removed.status.success() { false } else { From 9fe1e1dee4d05191dd6d0ffd61b8349f3fa88570 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 21:40:21 +0800 Subject: [PATCH 09/11] fix(worktree): name the ambiguous path form instead of inferring it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous guard asked whether `join` had dropped its base, which a drive-relative PROJECT path answers wrongly: `C:repo` joined with `C:repo\..\victim` still starts with `C:repo`, so the check passed a path whose meaning the process and the git child would each finish from their own per-drive current directory. Ask the question that was always meant instead — is either side relative to a drive rather than rooted on one — and refuse whichever side carries it. Unix has no prefixes, so the predicate is false for every path there; what a Unix run pins is that no shape a real folder row holds is mistaken for the Windows form. --- src-tauri/src/work_task/git.rs | 82 +++++++++++++++++++++++++++------- 1 file changed, 65 insertions(+), 17 deletions(-) diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index dd0cc391f5..4511db4b2a 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -563,6 +563,22 @@ pub async fn branch_holds_unlanded_work( } } +/// A Windows path that names a drive without rooting on it — `C:trees`, as +/// opposed to `C:\trees`. Windows finishes such a path from the current +/// directory OF THAT DRIVE, which a process keeps per drive and does not pass +/// to a child the way it passes its working directory, so the same string can +/// name two directories on one machine. `Path::join` reads it as a fresh base +/// and drops whatever it was joined onto, which is what makes it dangerous to a +/// caller that thought it had scoped a path to a repository. +/// +/// Unix has no such form: it has no prefixes, so this is `false` for every path +/// there, relative ones included. +fn is_drive_relative(path: &std::path::Path) -> bool { + let mut components = path.components(); + matches!(components.next(), Some(std::path::Component::Prefix(_))) + && !matches!(components.next(), Some(std::path::Component::RootDir)) +} + /// Which of the two refusals to report once git has declined a removal AND the /// leftover directory could not be consumed either. /// @@ -644,24 +660,25 @@ pub async fn remove_worktree_and_branch( // call and the `std::fs` call below cannot land on different directories, // which for the one destructive call here is the whole point. Folder paths // are stored exactly as they were given, so neither is hypothetical. - let joined = std::path::Path::new(repo_path).join(worktree_path); - // `join` drops the base when the right-hand side brings its own prefix. On - // Unix that means an absolute path, which is what we want. On Windows it - // also means a DRIVE-RELATIVE one (`C:trees`), which names a directory only - // a per-drive current directory can finish — and the app's is not the one - // git would use from `repo_path`, so the two halves would resolve it apart. - // There is no safe guess about which directory that is, and this function - // deletes directories, so it stops instead. - if joined.is_relative() && !joined.starts_with(repo_path) { - return Err(AppCommandError::new( - AppErrorCode::InvalidInput, - format!( - "the worktree path '{worktree_path}' is relative to a drive rather than \ - to '{repo_path}', so it does not name one directory" - ), - )); + // A drive-relative path on EITHER side is refused before anything resolves + // it. It is the one form whose meaning depends on a per-drive current + // directory, which this process and the git child do not share, so the two + // halves would resolve it apart — and the half that runs second deletes a + // directory. Whichever side carries it, the answer is the same: this does + // not name one directory, so nothing here may act on it. + for (label, path) in [("project", repo_path), ("worktree", worktree_path)] { + if is_drive_relative(std::path::Path::new(path)) { + return Err(AppCommandError::new( + AppErrorCode::InvalidInput, + format!( + "the {label} path '{path}' is relative to a drive rather than rooted on \ + one, so it does not name a single directory" + ), + )); + } } - let target = std::path::absolute(&joined).map_err(AppCommandError::io)?; + let target = std::path::absolute(std::path::Path::new(repo_path).join(worktree_path)) + .map_err(AppCommandError::io)?; // Refused rather than made lossy: a `\u{FFFD}` substituted into an absolute // path is still an absolute path, and git would go delete THAT one while // every removal below still used the bytes in `target`. Handing git a @@ -1269,6 +1286,37 @@ mod tests { ); } + /// The refusal above guards a Windows-only path form, so what a Unix run + /// can still pin is the half that would break everyone: that no shape a + /// real folder row holds is mistaken for it. `Path` has no prefixes here, + /// so every one of these must read as ordinary — including the strings + /// that LOOK drive-relative, which on this platform are just filenames. + #[test] + fn only_a_drive_relative_path_is_refused() { + for ordinary in [ + "/Users/x/work/repo", + "/Users/x/work/repo/", + "repo", + "rel/proj", + "./rel/proj", + "../sibling/repo", + "/", + "", + // Windows forms that ARE rooted, and so are not this. + r"C:\repo", + r"\\server\share\repo", + r"\\?\C:\repo", + // Drive-relative on Windows; a plain filename on Unix. Either way + // this platform must not see a prefix in it. + "C:repo", + ] { + assert!( + !is_drive_relative(std::path::Path::new(ordinary)), + "unix has no path prefixes, so {ordinary:?} is an ordinary path here" + ); + } + } + /// Git looks a `` argument up by unique path SUFFIX before it /// resolves it as a path, so a RELATIVE name is not scoped to the /// repository it is handed to — it reaches a registered checkout anywhere From 0ae2b844e43cd78935f43dc7c846ec046b3ac1b1 Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 4 Sep 2026 22:42:31 +0800 Subject: [PATCH 10/11] docs(worktree): put the resolution note back above the line it describes --- src-tauri/src/work_task/git.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index 4511db4b2a..af685894d9 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -654,12 +654,6 @@ pub async fn remove_worktree_and_branch( // (measured; `a_relative_path_cannot_reach_a_worktree_somewhere_else` // pins it). An absolute path suffix-matches nothing but itself. // - // Joining from `repo_path` is what git does with a relative path — that is - // `run_git`'s working directory — and `std::path::absolute` then applies - // the same process working directory the child would inherit. So the git - // call and the `std::fs` call below cannot land on different directories, - // which for the one destructive call here is the whole point. Folder paths - // are stored exactly as they were given, so neither is hypothetical. // A drive-relative path on EITHER side is refused before anything resolves // it. It is the one form whose meaning depends on a per-drive current // directory, which this process and the git child do not share, so the two @@ -677,6 +671,12 @@ pub async fn remove_worktree_and_branch( )); } } + // Joining from `repo_path` is what git does with a relative path — that is + // `run_git`'s working directory — and `std::path::absolute` then applies + // the same process working directory the child would inherit. So the git + // call and the `std::fs` call below cannot land on different directories, + // which for the one destructive call here is the whole point. Folder paths + // are stored exactly as they were given, so none of this is hypothetical. let target = std::path::absolute(std::path::Path::new(repo_path).join(worktree_path)) .map_err(AppCommandError::io)?; // Refused rather than made lossy: a `\u{FFFD}` substituted into an absolute From ea9a8e2d869a894370ff010676d675c5a542337b Mon Sep 17 00:00:00 2001 From: xintaofei Date: Sat, 5 Sep 2026 07:25:11 +0800 Subject: [PATCH 11/11] test(worktree): ask the platform instead of assuming it is this one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI caught both of these, and the first one caught it fairly: the predicate test asserted `C:repo` was ordinary "because unix has no path prefixes", which is true of the machine it was written on and not of the one the guard exists for. Windows read the drive prefix and refused the path, exactly as designed — so the assertion now states that outcome per platform, and the branch I could not exercise locally is covered. The second was a test measuring the platform rather than the code: `git worktree list` prints forward slashes on Windows while the fixture path holds backslashes, and a temp directory can come back short-named, so the string never matched even though nothing had been pruned. Ask git inside the checkout instead — a swept worktree cannot answer for its own HEAD — which compares nothing and says the same thing. Both are test-only. The removal behavior they cover was already correct on Windows: everything asserted before the failing lines passed there. --- src-tauri/src/work_task/git.rs | 46 ++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src-tauri/src/work_task/git.rs b/src-tauri/src/work_task/git.rs index af685894d9..e2029e58aa 100644 --- a/src-tauri/src/work_task/git.rs +++ b/src-tauri/src/work_task/git.rs @@ -1286,11 +1286,16 @@ mod tests { ); } - /// The refusal above guards a Windows-only path form, so what a Unix run - /// can still pin is the half that would break everyone: that no shape a - /// real folder row holds is mistaken for it. `Path` has no prefixes here, - /// so every one of these must read as ordinary — including the strings - /// that LOOK drive-relative, which on this platform are just filenames. + /// The predicate the refusal is built on, asked of whichever platform is + /// running rather than of the one that happened to write the test. + /// + /// `C:repo` is the whole reason it exists, and it is the case that cannot + /// be stated platform-blind: Windows reads a drive prefix there and has to + /// refuse it, while a platform without path prefixes reads the same bytes + /// as an ordinary filename and must not. Everything else names a single + /// directory on BOTH — a rooted drive, a UNC share, a verbatim path, and + /// every Unix shape a folder row actually holds — so a refusal there would + /// break cleanup for real users. #[test] fn only_a_drive_relative_path_is_refused() { for ordinary in [ @@ -1306,13 +1311,23 @@ mod tests { r"C:\repo", r"\\server\share\repo", r"\\?\C:\repo", - // Drive-relative on Windows; a plain filename on Unix. Either way - // this platform must not see a prefix in it. - "C:repo", + // Rooted on the current drive rather than naming one. + r"\wt", ] { assert!( !is_drive_relative(std::path::Path::new(ordinary)), - "unix has no path prefixes, so {ordinary:?} is an ordinary path here" + "{ordinary:?} names one directory, so nothing may refuse it" + ); + } + // A drive with a path that is not rooted on it, a drive with nothing + // after it at all, and the shape that walks back out of the directory + // it names — each finished by a per-drive current directory on Windows, + // each an ordinary filename anywhere without prefixes. + for drive_relative in ["C:repo", "C:", r"C:repo\..\victim"] { + assert_eq!( + is_drive_relative(std::path::Path::new(drive_relative)), + cfg!(windows), + "{drive_relative:?} is drive-relative on Windows and a plain name off it" ); } } @@ -1362,11 +1377,16 @@ mod tests { elsewhere.join("a.txt").exists(), "and the rest of its tree" ); - let list = run_git(repo_path, &["worktree", "list"]) - .await - .expect("list worktrees"); + // Asked OF git rather than matched against `worktree list`: that + // listing prints forward slashes on Windows while the fixture path + // holds backslashes, and a temp directory can come back short-named, + // so comparing the two strings tests the platform rather than the + // code. Running git inside the checkout answers the same question + // without comparing anything — a swept worktree leaves its `.git` + // file pointing at an administrative directory that is gone, and + // every git command in it fails. assert!( - String::from_utf8_lossy(&list.stdout).contains(elsewhere_path), + rev_parse(elsewhere_path, "HEAD").await.is_ok(), "and its registration: the prune must not have swept it either" ); }