Skip to content

fix(shell): bypass rm aliases in the nushell wrapper's cleanup - #3732

Merged
max-sixty merged 1 commit into
mainfrom
fix/issue-3730
Aug 5, 2026
Merged

max-sixty merged 1 commit into
mainfrom
fix/issue-3730

Conversation

@worktrunk-bot

Copy link
Copy Markdown
Collaborator

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:

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.

Options considered and rejected
  • `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.

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

The nushell wrapper's temp-file cleanup called a bare `rm -f`. Nushell
resolves aliases at parse time and runs config.nu before the vendor
autoload dir 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. A trash-style alias leaked all three temp files; an alias that
exits non-zero raised a ShellError that aborted the wrapper before it
returned the command's stdout, swallowing the output entirely.

#3714 fixed the same shadowing in bash and zsh with `command rm`.
Nushell has no `command` builtin, so cleanup branches at runtime on
$nu.os-info.family: `^rm` bypasses aliases on Unix, Windows keeps the
builtin (the only `rm` there), and `try` covers both so cleanup can
never abort the wrapper.

Closes #3730

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-authored, so this is a COMMENT rather than an approval.

One gap the consolidated cleanup doesn't close, verified against nushell 0.114.1: the exec-directive call a few lines above it — ^sh -c $script, inside the if ($exec_file | path exists) and (open $exec_file --raw | str trim | is-not-empty) block in templates/nushell.nu — isn't wrapped in try. A --execute body that exits non-zero raises the same ShellError the new comment describes, aborting the wrapper before cleanup runs: all three temp files leak and the command's stdout is discarded. That's the same abort-before-cleanup shape this PR fixes for the rm case, but on a supported path rather than an alias-dependent one.

It's pre-existing and not a regression here — cleanup already sat after the exec call. What makes it worth raising alongside this change is that test_wrapper_execute_exit_code_propagation already drives it for nu (--execute "exit 42"), and passes only because the abort happens to carry exit 42 out to the shell, not because the wrapper handled it. So the leak is on a path that's covered by a green test.

Repro — post-fix template shape, stub binary writing exit 42 into the exec file, dedicated TMPDIR
exit=42
leftover in TMPDIR: 3
tmp.BVI9gUzbux
tmp.R5lDm07YIU
tmp.ziwKEYB5JV

OUT=… (the print $"OUT=($out)" after the call) never runs — the function's return value is discarded along with the rest of the script.

Bare mechanism, no wrapper:

def wrapper [] {
    let f = (mktemp --tmpdir)
    if true { ^sh -c "exit 42" }
    print "REACHED_AFTER_EXEC"     # never prints
    "RETVAL"                       # never returned
}

The fix is to capture the exec exit the way the bash and zsh wrappers do with src_exit rather than letting the abort carry it — let exec_exit = (try { ^sh -c $script; 0 } catch { $env.LAST_EXIT_CODE }), folded into $exit_code when that's still 0. The end of the function then propagates it through the existing ^sh -c $"exit ($exit_code)" path, which keeps the 42 the test asserts and additionally prints the stdout that's currently dropped. That's a change to exit-code propagation, so it belongs in its own PR rather than bolted onto this one — happy to open it.

@worktrunk-bot

Copy link
Copy Markdown
Collaborator Author

Confirmed against nushell 0.114.1, both the bare mechanism and the real rendered wrapper: wt switch --create execfail --yes --execute "exit 42" leaves all three mktemp files in TMPDIR and never reaches the statement after the wt call, so the rest of the calling script dies with it. And test_wrapper_execute_exit_code_propagation does pass off the abort rather than off handling — its assertion is satisfied by the failure mode.

Split out to #3733 rather than folded in here, for the reason the review gives: deciding which code wins when both wt and the --execute body fail is a user-visible contract, not a mechanical port. The issue carries the repro, the src_exit-shaped fix, and a note that the existing test needs an "still executing after the failing body" assertion so the next regression can't pass the same way.

max-sixty pushed a commit that referenced this pull request Aug 5, 2026
…innable (#3736)

Daily `review-runs` sweep of the 2026-08-03/04 window. One maintainer
correction in the window is generalizable repo knowledge, so it gets
codified here.

## What happened

Triaging [#3696](#3696)
("Allow selecting the `wt list` JSON schema on the command line"), the
bot read
[`resolve_json_schema`](https://github.com/max-sixty/worktrunk/blob/286004f1ebe5f6563c772be24987394144a29215/src/commands/list/mod.rs#L220),
saw it reads `repo.config().list.json_schema`, and
[posted](#3696 (comment))
that "there's no way to pin it per-invocation independent of that
config" — then went further and said the usual alias deflection "can't
substitute, since there's no composable way to select the schema without
this flag existing first".

max-sixty [corrected
it](#3696 (comment)):
"the need is real, but it's already met, so I'm not going to add the
flag" — `wt --config-set 'list.json-schema = 2' list --format=json` pins
it per invocation, and `WORKTRUNK_LIST__JSON_SCHEMA` does the same a
layer down. The proposed `--schema` flag was declined as "a third
spelling of the same thing on a surface I deliberately keep small". The
issue was closed on that basis.

`--config-set` has been documented since
[#3138](#3138) (2026-06-19),
in `after_long_help` in
[`src/cli/mod.rs`](https://github.com/max-sixty/worktrunk/blob/286004f1ebe5f6563c772be24987394144a29215/src/cli/mod.rs#L2558)
and its rendered mirror. It was available to read; the triage just never
looked above the resolver.

## Why codify it rather than log it

The specific miss is what the gates would call a single occurrence, but
the shape isn't a behavioural nudge that risks noise elsewhere — it's a
factual property of worktrunk's config layering, and the failure mode it
prevents is a **publicly posted false capability claim**, which is the
class `running-in-ci` treats as the most corrosive output failure. It's
also invisible: nothing fails, the reply just reads as authoritative and
wrong until a maintainer happens to catch it.

It sits directly above **Suggesting Aliases for Niche Feature Requests**
because it serves the same flag-growth limit that section already
encodes, and is the cheaper answer when the ask is "select an existing
config value per invocation" — the alias deflection is the fallback when
it isn't.

<details><summary>Window summary</summary>

46 token-bearing runs, **0 failed**, 0 near-timeout (longest job 68 min
against the 360-min cap). 3 bot PRs opened
([#3731](#3731),
[#3732](#3732),
[#3734](#3734)), 2 merged
([#3722](#3722),
[#3726](#3726)), 0 closed
without merge. [#3721](#3721)
— the PR stranded by the 08-03 outage — was reviewed and approved,
confirming yesterday's recovery worked. Full evidence in the tracking
issue [#3691](#3691).

</details>

---------

Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
@max-sixty
max-sixty merged commit 7801016 into main Aug 5, 2026
40 of 50 checks passed
@max-sixty
max-sixty deleted the fix/issue-3730 branch August 5, 2026 19:24
social4hyq pushed a commit to social4hyq/homebrew-core that referenced this pull request Sep 20, 2026
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

This branch was previously deployed

1 inactive deployment
tend — a62500e9 Deployed Aug 4, 2026 by worktrunk-bot via review #3697
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated-fix Automated CI fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nushell wrapper's cleanup rm is shadowable by a user alias, and a failing alias swallows wt's output

2 participants