Repository navigation
fix(shell): use command rm in wrapper cleanup to bypass rm aliases - #3714
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
The change itself is correct and complete for the templates — command rm -f bypasses a user's rm alias baked in at function-definition parse time, matching the existing command rm -f in templates/fish.fish and the builtin cd precedent from #2643. nushell/powershell are rightly left alone.
But code-coverage is failing on integration_tests::init::test_init case_1 (bash) and case_3 (zsh): the command rm -f change was applied to the --lib snapshots (src/shell/snapshots/) but not to the integration-test snapshots under tests/snapshots/, which are a separate set generated with --features shell-integration-tests. Two files still contain the old rm -f:
tests/snapshots/integration__integration_tests__init__init_bash.snaptests/snapshots/integration__integration_tests__init__init_zsh.snap
(init_fish.snap there already had command rm -f, so it passes — consistent with the intended change.)
cargo test --lib shell doesn't cover these; the pre-merge hook does (it runs cargo insta test --accept --test integration). Regenerating with:
cargo insta test --accept --test integration -- init::test_init
should close it out. Want me to push that snapshot regeneration to the branch?
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f45144f to
7c803a6
Compare
|
Done in 7c803a6 — regenerated the two integration snapshots ( Note I trimmed the accept run's rewrite of the |
## Problem The nushell wrapper's temp-file cleanup called a bare `rm -f`. Nushell resolves aliases at parse time, and `config.nu` runs before the `$nu.vendor-autoload-dirs` file the wrapper installs into — so a user's `alias rm = ...` is already in scope when the wrapper's `def` is parsed, and intercepts the cleanup. #3714 fixed the same shadowing in the bash and zsh wrappers with `command rm`; nushell has no `command` builtin, so it was left out. Two failures, both verified end-to-end against nushell 0.114.1 (the version CI pins) driving the real rendered wrapper: | `alias rm =` | before | after | |---|---|---| | *(none)* | exit 0, stdout returned, 0 temp files left | unchanged | | `^false` | exit 1, **stdout empty**, **3 temp files leaked** | exit 0, stdout returned, 0 left | | `^echo TRASHED` | `TRASHED -f /tmp/...` lines **injected into stdout**, **3 temp files leaked** | exit 0, stdout returned, 0 left | The `^false` row is the serious one. Cleanup sits between `let output = (open $stdout_file --raw)` and the function's return, and nushell 0.98+ raises `ShellError` on a non-zero external exit — so an `rm` alias that fails, prompts, or isn't installed on that machine aborts the wrapper before it returns, and the command's stdout is silently discarded. Only stdout is affected; stderr has already streamed to the terminal, which is why the symptom hides on `wt switch` (its output is on stderr) and shows on `wt config show`. ## Solution Consolidate the two cleanup calls into one, after the last read of any temp file, and branch at runtime on `$nu.os-info.family`: ```nu if $nu.os-info.family == "windows" { try { rm -f $cd_file $exec_file $stdout_file } } else { try { ^rm -f $cd_file $exec_file $stdout_file } } ``` `^rm` bypasses alias expansion entirely — the Unix fix is complete, not partial. Windows keeps the builtin because `^rm` is external-only and Windows has no external `rm`; `try` covers both branches so cleanup can never abort the wrapper again. Branching at *runtime* rather than forking the template by build platform is what keeps this cheap: `wt config shell init nu` renders one text on every platform, so there's a single `init_nu` snapshot and no platform-specific rendering to maintain. <details><summary>Options considered and rejected</summary> - **`` `rm` `` (backtick-quoted)** — bypasses the alias, but `` `rm` `` with `PATH` emptied reports ``Command `rm` not found``, and `` `rm` `` with no args prints `/usr/bin/rm: missing operand`. It resolves to the *external*, so it's just `^rm` spelled differently — same Windows gap, no benefit. - **`hide rm`** — works inside the `def`, but `hide` is a parse-time keyword that leaks into the enclosing scope. For an autoloaded file that scope is the user's session, so installing shell integration would silently unbind their own `rm` alias. Same leak inside a `do {}` closure. Worse than the bug. - **`try` alone, no `^rm`** — cross-platform and kills the abort, but leaves the alias running: a `trash` alias still trashes worktrunk's temp files on every invocation, and an `rm -i`-style alias still prompts. - **Forking the template by build platform** — same end state on each platform, at the cost of platform-specific `init_nu` snapshots. The runtime branch gets there without them. </details> ## Testing `test_nu_wrapper_cleanup_survives_rm_alias` in `tests/integration_tests/shell_wrapper.rs` runs the real wrapper through a PTY with `alias rm = ^false` declared ahead of it, against a dedicated `TMPDIR`, and asserts both symptoms: the wrapper's stdout comes back non-empty, and the temp dir is empty afterwards. It fails on the current template (marker absent — the wrapper aborted) and passes with the fix. Also run locally: `cargo test --lib --bins`, and the full `shell_wrapper` + `config_show` + `test_docs_are_in_sync` integration set with `--features shell-integration-tests` (201 passed). ## Not fixed here A user alias to a *custom command or builtin* whose signature rejects `-f` — e.g. `alias rm = print "…"` — makes the whole wrapper file fail to parse, leaving `wt` undefined rather than merely broken. That's pre-existing (today's template has two bare `rm -f` calls) and survives this change, because the Windows branch is still parsed on Unix even though it never runs. Eliminating it means emitting no bare `rm` at all on Unix, which is the template fork above. Flagged on #3730 rather than bundled in here. --- Closes #3730 — automated triage Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
worktrunk 0.72.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## Release Notes ### Improved - **A host carrying a forge's name anywhere resolves to that forge again**: 0.71.0 required `github`, `gitlab`, or `gitea` as a whole dot-separated label, which read as an ownership check but wasn't one — an attacker controls their own DNS — while shutting out self-hosters with hyphenated names. `github-enterprise.acme.com`, `mygithub.life-white.uk`, and the `github-personal` SSH alias classify again, so CI status, `wt switch --prs`, and `repo.provider` work with no config. ([#3673](max-sixty/worktrunk#3673)) - **One `[projects."…"]` entry can cover every repository on a host, and can set the forge**: A key containing `*` matches any run of characters, `/` included, so `[projects."git.company.example/*"]` covers a whole host; every matching entry applies, least- to most-specific. The table also gained `forge.platform` and `forge.hostname`, so a self-hosted host needs one entry here rather than a block in every repo. [Docs](https://worktrunk.dev/config/#user-project-specific-settings) ([#3701](max-sixty/worktrunk#3701), thanks @chrishas35 for the request and @witt-bit for the workspace-scoped case it partly serves) - **`wt merge` and `wt step push` leave the target worktree's uncommitted changes in place**: The autostash that held a dirty target's changes restored them as unstaged; both now advance the target with a compare-and-swap `update-ref` and `read-tree -m -u`, which leaves uncommitted work untouched. (Breaking: the fast-forward path no longer runs `git push`, so `pre-push` and the receive-side hooks no longer fire, and a failed sync errors with the ref rolled back.) ([#3703](max-sixty/worktrunk#3703), [#3684](max-sixty/worktrunk#3684), [#3693](max-sixty/worktrunk#3693), thanks @gubasso for reporting) - **Approval state and branch-removal outcomes are machine-readable**: `wt config approvals list --format=json` reports whether a non-interactive run would stop for approval. `wt remove` and `wt step prune` replace `branch_deleted` with `branch_outcome`: `deleted`, `deferred`, `not_attempted`, `retained_unmerged`, `retained_checked_out`, `retained_raced`, `retained_failed`. (Breaking.) ([#3710](max-sixty/worktrunk#3710), thanks @NathanaelRea for the requests) - **Every commit hash worktrunk prints follows `core.abbrev`**: The `wt list` table, `wt switch --prs`'s `log` tab, and `wt config state`'s CI cache table sliced to 8 characters while `--format=json` carried git's `%h`. All now ask git how wide it abbreviates in this repo. ([#3676](max-sixty/worktrunk#3676), [#3677](max-sixty/worktrunk#3677)) - **`wt list --format=json` schema 2 has a published JSON Schema**: [worktrunk.dev/schema/list-v2.json](https://worktrunk.dev/schema/list-v2.json) holds the contract, and `wt list --print-schema` prints the same document. Four fields that were bare strings are now enumerated vocabularies; the emitted JSON is unchanged. ([#3747](max-sixty/worktrunk#3747)) - **A detached worktree is named by its commit, not `-`**: The Branch cell hardcoded `-`, which reads as missing data rather than a state; it now carries the row's abbreviated HEAD, and the picker and statusline name the worktree the same way. ([#3675](max-sixty/worktrunk#3675)) ### Fixed - **Piped output is plain and no longer panics**: `wt list | head -3` exited 101 with `failed printing to stdout: Broken pipe`, and `wt list` wrote ANSI to a pipe unconditionally. Every stdout surface now exits cleanly, and the human-read ones are plain when piped unless `CLICOLOR_FORCE=1`. ([#3746](max-sixty/worktrunk#3746), [#3766](max-sixty/worktrunk#3766)) - **A CI check that hasn't finished no longer reads as passed**: Each forge's status parser missed documented values, so a GitHub PR parked on an approval gate showed green, and GitLab's `canceling` and Azure DevOps's `postponed` read as no CI at all. ([#3741](max-sixty/worktrunk#3741), [#3740](max-sixty/worktrunk#3740)) - **The shell wrappers survive an `rm` alias and a failing `--execute`**: Aliases bake into the wrapper at parse time, so `alias rm='rm -v'` reached its cleanup — noise on zsh and bash, and on nushell an abort that leaked three temp files, as a failing `--execute` body also did. ([#3714](max-sixty/worktrunk#3714), thanks @Ar4l), ([#3732](max-sixty/worktrunk#3732), [#3734](max-sixty/worktrunk#3734)) - **An alias or hook wrapping `wt switch` or `wt remove` keeps your subdirectory**: The user's position came from the `wt` process's cwd, which inside an alias body is the worktree root, so an aliased `wt remove` from `feature/apps/gateway` landed at the primary worktree's root. Fixes [#3723](max-sixty/worktrunk#3723). ([#3724](max-sixty/worktrunk#3724), thanks @vivienm for reporting) - **`wt merge` and `wt step push` refuse a target worktree parked mid-operation**: The two-tree sync refuses an unmerged index but not a stopped cherry-pick or rebase whose conflict was already staged, so the push range could land in a paused target and be committed by `--continue`. ([#3759](max-sixty/worktrunk#3759)) - **The Claude plugin's worktree-remove hook resolves against the worktree path**: The hook anchored at `CLAUDE_PROJECT_DIR`, which the `claude agents` view routinely leaves outside any repository, so `wt remove` died with `not a git repository` and the session became undeletable. Its guard now also requires a `.git` entry. ([#3754](max-sixty/worktrunk#3754), [#3767](max-sixty/worktrunk#3767), thanks @judewang for the fix and the report) - **A Gitea API error is reported as one, not as a parse failure**: `tea api` exits 0 whatever the HTTP status, so both call sites guessed from the body's shape and blamed an API change for an API error. `--include` surfaces the status instead. ([#3713](max-sixty/worktrunk#3713), [#3600](max-sixty/worktrunk#3600)) - **`--print-schema` and the doc-generation help flags name the right command**: All three found the subcommand by scanning `argv` for a `/wt` suffix, which never matches `wt.exe` under a backslash path, so on Windows they read the binary's own path as the command. ([#3762](max-sixty/worktrunk#3762)) - **A multibyte shell name no longer panics**: `extract_filename_from_path` sliced at `len() - 4` to test for `.exe` with no char-boundary check: `SHELL=/bin/日本語 wt config show` panicked, and on macOS every process name goes through it during shell detection. ([#3727](max-sixty/worktrunk#3727)) - **`wt` installed under a dotted name generates shell integration for that name**: `binary_name` used `file_stem`, which cuts at the last dot, so `wt config shell init bash` under `wt.old` emitted a wrapper for `wt`. It now strips only the executable suffix. ([#3719](max-sixty/worktrunk#3719)) - **Concurrent `wt step prune` removals no longer race the worktree registry**: `git worktree remove` reads every sibling under `.git/worktrees/`, so two overlapping removals could have one read a sibling mid-teardown. Registry-mutating removals now serialize behind a second lock. ([#3692](max-sixty/worktrunk#3692)) - **The `wt switch` first-run offer previews the legacy files it removes**: Accepting "Install shell integration?" could delete a deprecated worktrunk-managed wrapper the prompt never named. What gets removed is unchanged. ([#3656](max-sixty/worktrunk#3656)) - **`wt config create --project` writes a resolvable link**: The comment it writes into `.config/wt.toml` carried a raw Zola target, because the link-conversion regex stopped at the first `]` — here the one closing a nested code span. ([#3731](max-sixty/worktrunk#3731)) - **`wt list --branches` counts a local branch containing `/` as local**: The summary tally classified branch-only rows by `branch.contains('/')`, so a local `feature/login` counted under "N remote branches". ([#3687](max-sixty/worktrunk#3687)) - **`wt step relocate`'s human summary counts template-error branches as skipped**: `--format=json` already folded them into `skipped`; the human tally undercounted by the number of branches whose `worktree-path` template failed to expand. ([#3688](max-sixty/worktrunk#3688)) ### Documentation - **SignPath attribution appears with the artifacts it describes**: The code-signing notice and a route to the policy now sit in the install section's Windows block on the README and the docs landing page, as SignPath Foundation's OSS program requires. ([#3709](max-sixty/worktrunk#3709)) - **`wt step copy-ignored`'s `--require-include` example renders as a terminal block**: It was the only `console` block in the command's long help missing the `$ ` prefix. ([#3706](max-sixty/worktrunk#3706)) ### Internal - **Library API rework** (Breaking library API): `cargo-semver-checks` fails five lints — `LegacyForgeAlias` and `Repository::legacy_forge_alias` removed with the forge-classification revert, `Repository::forge_platform_override` removed for one shared resolver, `stage_worktree_removal` gained two parameters, `UserProjectOverrides` gained a `forge` field, and the `real-repo-benches` feature was removed. ([#3673](max-sixty/worktrunk#3673), [#3694](htt See merge request: Harmonybrew/homebrew-core!16275
zsh (and bash with
expand_aliases) expand aliases at function-definition parse time. If a user hasalias rm='rm -v'defined beforeeval "$(wt config shell init zsh)"in their rc file — a common ordering, since alias blocks usually come first — the wrapper's cleanup bakes inrm -v -f …, and everywtinvocation prints two lines of noise:This changes the zsh and bash wrapper cleanup from
rm -ftocommand rm -f, mirroring:command rm -fintemplates/fish.fishbuiltin cdguard added for the same alias-expansion reason in the zsh template (Bug Report:wt switchfails withzoxide: no match foundwhencdis aliased to__zoxide_z#2643)nushell and powershell are unaffected (
rmthere resolves to shell builtins with different alias semantics).Insta snapshots regenerated;
cargo test --lib shellpasses (236 tests).🤖 Generated with Claude Code