From 838826b9718bb566e1f31781e9c3ec7c1c948b3e Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Sat, 25 Jul 2026 13:26:48 -0700 Subject: [PATCH] fix(shell): reclaim the legacy wrapper paths instead of inspecting them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `wt config shell install` decided whether the file at a legacy location was worktrunk's by reading it — a substring test for the wrapper header, falling back to "every code line looks like an integration line". Anything that didn't match was left in place. Fish sources `conf.d` at startup, so whatever `conf.d/{cmd}.fish` defines is already loaded by the time fish would autoload the `functions/{cmd}.fish` the install just wrote — the stale definition wins and the new wrapper may never load. Ownership is now the path. `conf.d/{cmd}.fish` and the stranded nushell `{cmd}.nu` candidates are paths worktrunk itself computes for the command name being installed, so install takes them back whole without reading them. Only that exact filename is touched: a neighbour under another name is not worktrunk's, and each removal is reported. `wt config shell uninstall` still reads the header, because it takes no `--cmd` and so genuinely doesn't know the name — it lists the shell-owned directories and has to tell our `{cmd}.fish` from the user's own files beside it. That is the one place ownership can't come from the path, and it prompts and previews every file before removing it, which install does neither of. The two tests that pinned the old conservatism now pin the new boundary: a headerless file at the reclaimed path goes, a differently-named neighbour stays. Co-Authored-By: Claude Opus 5 (1M context) --- docs/content/faq.md | 4 +- .../skills/worktrunk/reference/faq.md | 4 +- skills/worktrunk/reference/faq.md | 4 +- src/commands/configure_shell.rs | 83 ++++++++----------- tests/integration_tests/configure_shell.rs | 70 +++++++++------- 5 files changed, 82 insertions(+), 83 deletions(-) diff --git a/docs/content/faq.md b/docs/content/faq.md index f58050f1e1..b0e8845a7b 100644 --- a/docs/content/faq.md +++ b/docs/content/faq.md @@ -195,8 +195,8 @@ Use `-D` to force-delete branches with unmerged changes. Use `--no-delete-branch - `wt remove` — besides the target worktree, two cleanup mechanisms run. The removed worktree's own `git fsmonitor--daemon` (git's per-worktree filesystem watcher under `core.fsmonitor=true`, which would leak once its worktree is gone) is sent `git fsmonitor--daemon stop`, then force-terminated (`SIGTERM`, then `SIGKILL`) via the PID resolved from its IPC socket if it didn't exit. A background sweep then deletes `.git/wt/trash/` entries older than 24 hours (directories orphaned when a previous background removal was interrupted) and terminates fsmonitor daemons whose worktree no longer exists (orphans from `git worktree remove`, `rm -rf`, or a crashed `wt`) - `wt config state clear` — removes all worktrunk data from `.git/` (config keys, caches, markers, hints, variables, logs, stale trash) -- `wt config shell install` — when migrating an integration to a new location, removes the worktrunk-managed file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). Recognition is the same as for uninstall below, so a file of the user's own sitting at that path is left in place -- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Files are recognized by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after +- `wt config shell install` — when migrating an integration to a new location, removes the file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). The old path is where worktrunk's own wrapper lived and is named after the command being installed, so it's taken back whole without reading it — a `conf.d/wt.fish` left in place would be sourced at startup and shadow the new wrapper anyway. Only that exact filename is touched, and each removal is printed +- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Uninstall takes no command name, so it lists those directories and recognizes files by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after See [What files does Worktrunk create?](#what-files-does-worktrunk-create) for details. diff --git a/plugins/worktrunk/skills/worktrunk/reference/faq.md b/plugins/worktrunk/skills/worktrunk/reference/faq.md index fffae01e4f..63517fb487 100644 --- a/plugins/worktrunk/skills/worktrunk/reference/faq.md +++ b/plugins/worktrunk/skills/worktrunk/reference/faq.md @@ -190,8 +190,8 @@ Use `-D` to force-delete branches with unmerged changes. Use `--no-delete-branch - `wt remove` — besides the target worktree, two cleanup mechanisms run. The removed worktree's own `git fsmonitor--daemon` (git's per-worktree filesystem watcher under `core.fsmonitor=true`, which would leak once its worktree is gone) is sent `git fsmonitor--daemon stop`, then force-terminated (`SIGTERM`, then `SIGKILL`) via the PID resolved from its IPC socket if it didn't exit. A background sweep then deletes `.git/wt/trash/` entries older than 24 hours (directories orphaned when a previous background removal was interrupted) and terminates fsmonitor daemons whose worktree no longer exists (orphans from `git worktree remove`, `rm -rf`, or a crashed `wt`) - `wt config state clear` — removes all worktrunk data from `.git/` (config keys, caches, markers, hints, variables, logs, stale trash) -- `wt config shell install` — when migrating an integration to a new location, removes the worktrunk-managed file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). Recognition is the same as for uninstall below, so a file of the user's own sitting at that path is left in place -- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Files are recognized by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after +- `wt config shell install` — when migrating an integration to a new location, removes the file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). The old path is where worktrunk's own wrapper lived and is named after the command being installed, so it's taken back whole without reading it — a `conf.d/wt.fish` left in place would be sourced at startup and shadow the new wrapper anyway. Only that exact filename is touched, and each removal is printed +- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Uninstall takes no command name, so it lists those directories and recognizes files by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after See [What files does Worktrunk create?](#what-files-does-worktrunk-create) for details. diff --git a/skills/worktrunk/reference/faq.md b/skills/worktrunk/reference/faq.md index fffae01e4f..63517fb487 100644 --- a/skills/worktrunk/reference/faq.md +++ b/skills/worktrunk/reference/faq.md @@ -190,8 +190,8 @@ Use `-D` to force-delete branches with unmerged changes. Use `--no-delete-branch - `wt remove` — besides the target worktree, two cleanup mechanisms run. The removed worktree's own `git fsmonitor--daemon` (git's per-worktree filesystem watcher under `core.fsmonitor=true`, which would leak once its worktree is gone) is sent `git fsmonitor--daemon stop`, then force-terminated (`SIGTERM`, then `SIGKILL`) via the PID resolved from its IPC socket if it didn't exit. A background sweep then deletes `.git/wt/trash/` entries older than 24 hours (directories orphaned when a previous background removal was interrupted) and terminates fsmonitor daemons whose worktree no longer exists (orphans from `git worktree remove`, `rm -rf`, or a crashed `wt`) - `wt config state clear` — removes all worktrunk data from `.git/` (config keys, caches, markers, hints, variables, logs, stale trash) -- `wt config shell install` — when migrating an integration to a new location, removes the worktrunk-managed file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). Recognition is the same as for uninstall below, so a file of the user's own sitting at that path is left in place -- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Files are recognized by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after +- `wt config shell install` — when migrating an integration to a new location, removes the file left at the old one: fish `conf.d/wt.fish` (now `functions/wt.fish`) and nushell wrappers stranded under `/vendor/autoload` (now `/vendor/autoload`). The old path is where worktrunk's own wrapper lived and is named after the command being installed, so it's taken back whole without reading it — a `conf.d/wt.fish` left in place would be sourced at startup and shadow the new wrapper anyway. Only that exact filename is touched, and each removal is printed +- `wt config shell uninstall` — removes integration lines from bash/zsh/PowerShell rc files, and deletes worktrunk's wrapper and completion files (fish `functions/`, `conf.d/`, and `completions/`; nushell `vendor/autoload`). Uninstall takes no command name, so it lists those directories and recognizes files by worktrunk's own content markers, whatever binary name they were installed under; files without the markers are left alone. An rc file belongs to the user, so a line qualifies only where it runs the init command: one that merely mentions it, inside a comment, an `echo`, or an alias body, stays. Every line uninstall does take is printed, before removal and again after See [What files does Worktrunk create?](#what-files-does-worktrunk-create) for details. diff --git a/src/commands/configure_shell.rs b/src/commands/configure_shell.rs index 366215fab2..649d7aebcf 100644 --- a/src/commands/configure_shell.rs +++ b/src/commands/configure_shell.rs @@ -132,9 +132,15 @@ const WRAPPER_MARKER: &str = "worktrunk shell integration for"; /// The header every fish completion file worktrunk has shipped opens with. const COMPLETION_MARKER: &str = "# worktrunk completions for"; -/// Whether a whole file is one worktrunk generated, and so is worktrunk's to -/// delete — by `uninstall`, and by the install-time cleanup of the legacy fish -/// `conf.d` wrapper. +/// Whether a whole file is one worktrunk generated, and so is `uninstall`'s to +/// delete. +/// +/// This is the one place ownership is read out of a file's contents, because +/// it's the one place worktrunk doesn't know the name. `uninstall` takes no +/// `--cmd`, so it lists the shell-owned directories and has to tell worktrunk's +/// `{cmd}.fish` from the user's own files sitting beside it. Wherever the +/// command name *is* known — install, and the legacy-location cleanups that +/// accompany it — the path names the file worktrunk owns and nothing reads it. /// /// The answer is independent of the binary name embedded in the file, so a /// wrapper installed as `wt.fish`, `git-wt.fish`, or `git-wt.nu` is recognized @@ -145,10 +151,10 @@ const COMPLETION_MARKER: &str = "# worktrunk completions for"; /// (per the rc-file line detector, keeping one definition of "an integration /// line"). /// -/// Both callers walk directories the user owns, so the question is asked of -/// every line rather than of the file as a blob. A user's own `wt.fish` that -/// runs `wt config shell init` amid other code — or merely mentions it in a -/// comment — survives; a whole-file substring test would delete it. +/// The directories walked are the user's, so the question is asked of every +/// line rather than of the file as a blob. A user's own `wt.fish` that runs +/// `wt config shell init` amid other code — or merely mentions it in a comment +/// — survives; a whole-file substring test would delete it. fn is_worktrunk_managed_content(content: &str) -> bool { if content.contains(WRAPPER_MARKER) { return true; @@ -160,21 +166,18 @@ fn is_worktrunk_managed_content(content: &str) -> bool { .all(shell::is_shell_integration_line_for_uninstall_any_cmd) } -/// Check if a Nushell wrapper file is worktrunk-managed. +/// Take back the fish wrapper's legacy `conf.d` location after installing to +/// `functions/`. /// -/// The Nushell wrapper is a complete autoload file (not a `source` line), so it -/// carries no `config shell init` marker — the header comment is the whole -/// signal that the file is ours to remove during stranded-file cleanup -/// (issue #2878). -fn is_worktrunk_managed_nushell(content: &str) -> bool { - content.contains(&format!("{WRAPPER_MARKER} nushell")) -} - -/// Clean up legacy fish conf.d file after installing to functions/ +/// Fish integration used to install to `~/.config/fish/conf.d/{cmd}.fish`, +/// which loads before Homebrew's PATH setup in `config.fish` (issue #566); +/// installs now write `functions/{cmd}.fish`, autoloaded on first use. /// -/// Previously, fish shell integration was installed to `~/.config/fish/conf.d/{cmd}.fish`. -/// This caused issues with Homebrew PATH setup (see issue #566). We now install to -/// `functions/{cmd}.fish` instead. This function removes the legacy file if it exists. +/// The path names the command being installed, so it's worktrunk's and the file +/// goes whole — the contents are never read. Leaving it would also break the +/// install it accompanies: `conf.d` is sourced at startup, so a `function +/// {cmd}` defined there is already loaded by the time fish would autoload +/// `functions/{cmd}.fish`, and the stale wrapper wins every time. /// /// Returns the paths of files that were cleaned up, each paired with `Shell::Fish`. fn cleanup_legacy_fish_conf_d(configured: &[ConfigureResult], cmd: &str) -> Vec<(Shell, PathBuf)> { @@ -198,16 +201,6 @@ fn cleanup_legacy_fish_conf_d(configured: &[ConfigureResult], cmd: &str) -> Vec< return cleaned; } - // Only remove if the file contains worktrunk integration markers - // to avoid deleting user's custom wt.fish that isn't from worktrunk - let Ok(content) = fs::read_to_string(&legacy_path) else { - return cleaned; - }; - - if !is_worktrunk_managed_content(&content) { - return cleaned; - } - match fs::remove_file(&legacy_path) { Ok(()) => { cleaned.push((Shell::Fish, legacy_path)); @@ -231,9 +224,13 @@ fn cleanup_legacy_fish_conf_d(configured: &[ConfigureResult], cmd: &str) -> Vec< /// /// Older worktrunk installed the wrapper under `/vendor/autoload`, /// which Nushell never autoloads (issue #2878). After installing to the correct -/// vendor-autoload dir (`/vendor/autoload`), this removes any -/// worktrunk-managed wrapper left at the other candidate paths so a stale, -/// never-loaded copy isn't left behind. +/// vendor-autoload dir (`/vendor/autoload`), this removes the wrapper +/// left at the other candidate paths so a stale, never-loaded copy isn't left +/// behind. +/// +/// Every path considered is one worktrunk itself computes for this command name +/// (`config_paths`), so — as with the fish `conf.d` cleanup — the path settles +/// ownership and the file is removed whole, unread. /// /// Returns the paths removed, each paired with `Shell::Nushell`. fn cleanup_stranded_nushell(configured: &[ConfigureResult], cmd: &str) -> Vec<(Shell, PathBuf)> { @@ -254,14 +251,6 @@ fn cleanup_stranded_nushell(configured: &[ConfigureResult], cmd: &str) -> Vec<(S if &path == canonical || !path.exists() { continue; } - // Only remove files that are clearly worktrunk's, to avoid deleting a - // user's own `wt.nu`. - let Ok(content) = fs::read_to_string(&path) else { - continue; - }; - if !is_worktrunk_managed_nushell(&content) { - continue; - } match fs::remove_file(&path) { Ok(()) => cleaned.push((Shell::Nushell, path)), Err(e) => { @@ -1633,12 +1622,12 @@ mod tests { assert!(is_worktrunk_managed_content(content), "{content}"); } - // Not ours: both callers walk directories the user owns, so a file that - // only mentions the command must survive — as must a user's own file - // that runs the init line amid other code, which a per-line any-match - // would delete whole. The second and third also carry the init command - // and a `| source` somewhere, which is all a whole-file substring test - // asks before deleting. + // Not ours: the uninstall scan walks directories the user owns and has + // only the contents to go on, so a file that merely mentions the command + // must survive — as must a user's own file that runs the init line amid + // other code, which a per-line any-match would delete whole. The second + // and third also carry the init command and a `| source` somewhere, + // which is all a whole-file substring test asks before deleting. for content in [ "function notes\n echo run wt config shell init fish to set up\nend\n", "# reminder: wt config shell init fish\nfunction helpers\n cat ~/.aliases | source\nend\n", diff --git a/tests/integration_tests/configure_shell.rs b/tests/integration_tests/configure_shell.rs index 30e9f3bcd6..f0aa51c55c 100644 --- a/tests/integration_tests/configure_shell.rs +++ b/tests/integration_tests/configure_shell.rs @@ -408,20 +408,23 @@ fn test_configure_shell_fish_legacy_conf_d_cleanup(repo: TestRepo, temp_home: Te ); } -/// Installing fish integration leaves a user's own `conf.d/wt.fish` alone. +/// Installing fish integration reclaims `conf.d/{cmd}.fish` whatever it holds. /// -/// The legacy cleanup deletes a whole file the user never asked it to touch, so -/// it has to recognize worktrunk's own wrapper rather than any file that -/// mentions the init command. This one names it in a comment and pipes -/// something unrelated to `source` — both substrings a whole-file test would -/// take as proof the file was worktrunk's. +/// The path names the command being installed, so it's worktrunk's; the file's +/// contents don't enter into it. Leaving this one would break the install +/// besides — `conf.d` is sourced at startup, so a `function wt` defined there +/// is already loaded when fish would otherwise autoload `functions/wt.fish`. #[rstest] -fn test_configure_shell_fish_preserves_user_conf_d_file(repo: TestRepo, temp_home: TempDir) { +fn test_configure_shell_fish_reclaims_conf_d_path(repo: TestRepo, temp_home: TempDir) { let conf_d = temp_home.path().join(".config/fish/conf.d"); fs::create_dir_all(&conf_d).unwrap(); - let user_file = conf_d.join("wt.fish"); - let user_content = "# reminder: set up with wt config shell init fish\nfunction wt_helpers\n cat ~/.aliases | source\nend\n"; - fs::write(&user_file, user_content).unwrap(); + let stale = conf_d.join("wt.fish"); + // No worktrunk header, and nothing a content test would recognize. + fs::write(&stale, "function wt\n command wt-old $argv\nend\n").unwrap(); + // A neighbour under another name is untouched: only `{cmd}.fish` is ours. + let neighbour = conf_d.join("aliases.fish"); + let neighbour_content = "# reminder: wt config shell init fish\nalias ll 'ls -l'\n"; + fs::write(&neighbour, neighbour_content).unwrap(); let mut cmd = wt_command(); repo.configure_wt_cmd(&mut cmd); @@ -437,16 +440,22 @@ fn test_configure_shell_fish_preserves_user_conf_d_file(repo: TestRepo, temp_hom .path() .join(".config/fish/functions/wt.fish") .exists(), - "install should still write functions/wt.fish" + "install should write functions/wt.fish" ); assert!( - user_file.exists(), - "install deleted the user's own conf.d/wt.fish" + !stale.exists(), + "install should reclaim conf.d/wt.fish: {stale:?}" ); assert_eq!( - fs::read_to_string(&user_file).unwrap(), - user_content, - "user's own conf.d/wt.fish must survive install untouched" + fs::read_to_string(&neighbour).unwrap(), + neighbour_content, + "a conf.d file under another name is not worktrunk's" + ); + // The removal is reported, not silent. + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("deprecated"), + "install should report the conf.d cleanup:\n{stderr}" ); } @@ -2774,21 +2783,23 @@ fn test_nushell_install_cleans_stranded_legacy(repo: TestRepo, temp_home: TempDi ); } -/// Data safety: install must NOT delete a `wt.nu` at a legacy location that -/// isn't worktrunk-managed (no worktrunk header) — it could be the user's own -/// file. Only files carrying the worktrunk header are cleaned up (issue #2878). +/// The stranded-file cleanup is scoped by path, so it reclaims `{cmd}.nu` at a +/// legacy autoload dir whatever the file holds — and touches nothing else in +/// that directory (issue #2878). #[rstest] -fn test_nushell_install_keeps_unmanaged_legacy_file(repo: TestRepo, temp_home: TempDir) { +fn test_nushell_install_reclaims_only_the_command_name(repo: TestRepo, temp_home: TempDir) { let home = canonical_temp_home(&temp_home); let autoload = home.join(".local/share/nushell/vendor/autoload"); - // A user-authored wt.nu at the legacy config-dir location — no worktrunk - // header, so it must be left untouched. let legacy_dir = home.join(".config/nushell/vendor/autoload"); fs::create_dir_all(&legacy_dir).unwrap(); + // No worktrunk header: the path is what makes it worktrunk's. let legacy = legacy_dir.join("wt.nu"); - let user_content = "# my own wt helper\ndef wt [] { echo hi }\n"; - fs::write(&legacy, user_content).unwrap(); + fs::write(&legacy, "def wt [] { echo hi }\n").unwrap(); + // A neighbour under another name stays put. + let neighbour = legacy_dir.join("helpers.nu"); + let neighbour_content = "def hi [] { echo hi }\n"; + fs::write(&neighbour, neighbour_content).unwrap(); let mut cmd = wt_command(); repo.configure_wt_cmd(&mut cmd); @@ -2805,16 +2816,15 @@ fn test_nushell_install_keeps_unmanaged_legacy_file(repo: TestRepo, temp_home: T String::from_utf8_lossy(&output.stderr) ); - // Canonical wrapper written, user's legacy file preserved verbatim. assert!(autoload.join("wt.nu").exists(), "canonical wrapper missing"); assert!( - legacy.exists(), - "unmanaged legacy wt.nu must be preserved: {legacy:?}" + !legacy.exists(), + "install should reclaim the legacy wt.nu: {legacy:?}" ); assert_eq!( - fs::read_to_string(&legacy).unwrap(), - user_content, - "unmanaged legacy file must be left unchanged" + fs::read_to_string(&neighbour).unwrap(), + neighbour_content, + "a legacy-dir file under another name is not worktrunk's" ); }