Repository navigation
fix(shell): catch a failing --execute body in the nushell wrapper - #3734
Conversation
Nushell 0.98+ raises a ShellError on a non-zero external exit, so the wrapper's unguarded `^sh -c $script` unwound the whole `def` — skipping cleanup (all three mktemp files leaked) and discarding the stdout the function still had to return. Wrap it in `try`, the same way the binary call above it already is, and fold the body's exit code into `$exit_code` with the precedence the bash, zsh and fish wrappers apply to their `src_exit`: wt's own failure wins, otherwise the body's. The existing `^sh -c $"exit ($exit_code)"` at the end then carries the code out, so propagation is the wrapper's doing rather than a side effect of the unwind. Closes #3733
worktrunk-bot
left a comment
There was a problem hiding this comment.
Verified the fix itself against nushell 0.114.1 rather than taking the description's word for it — pre-fix the def unwinds before cleanup, post-fix cleanup runs, the body's stdout still streams ahead of the wrapper's own return, and the let exit_code = if $exit_code != 0 { … } shadowing resolves the previous binding on the RHS as intended (wt's code wins, body's code otherwise). No defect in the diff. Two things adjacent to it:
The "should merge without conflict" claim about #3732 is wrong. git merge-tree on the two PR heads reports content conflicts in both files:
CONFLICT (content): Merge conflict in src/shell/snapshots/worktrunk__shell__tests__init_nu.snap
CONFLICT (content): Merge conflict in templates/nushell.nu
On the base, ^sh -c $script is line 123 and rm -f $cd_file $exec_file is line 126 — three apart, so each PR's changed lines sit inside the other's context. This PR rewrites the former and inserts the let exit_code fold immediately above the latter; #3732 rewrites the rm lines and hoists let output above them. Resolution is mechanical (keep both, fold goes before the cleanup block), but whichever lands second needs a manual rebase. I've corrected the description.
The same abort-before-cleanup shape sits at a third site 15 lines up, unfixed. In the cd-directive block (# cd file holds a raw path — no shell parsing needed), cd $target_dir raises on a missing or inaccessible directory and unwinds the whole def exactly as the exec call did:
def wrapper_cd [target: string] {
cd $target
print "CLEANUP_RAN" # never prints
"RETVAL"
}On nu 0.114.1 that catches as an error with no CLEANUP_RAN — same three leaked temp files, same discarded stdout. So the function has three abort sites: this PR fixes exec, #3732 fixes rm, cd remains. Separately, bash, zsh and fish fold a cd_exit into exit_code under the same first-non-zero-wins rule this PR adopts for src_exit; the nushell wrapper folds neither. Both are out of scope here — I'm not opening a third overlapping nushell PR while these two already conflict, and it wants its own test either way. Happy to follow up once these land.
The reachability of the cd case is low (wt writes a path it just resolved; it takes a concurrent removal or a permissions change to miss), which is probably why it hasn't surfaced — noting it so the merge isn't read as closing out the whole function.
…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>
|
@worktrunk-bot resolve conflicts |
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewed the conflict resolution in 938976ec8, not just the description's account of it. It holds: git diff origin/main..938976ec8 -- templates/nushell.nu is exactly this PR's own hunk and nothing else, so #3732's consolidated try { ^rm -f $cd_file $exec_file $stdout_file } block — and its hide rm / ^rm-vs-builtin rationale — came through the merge intact, with the let exit_code fold landing above the let output read where it needs to be. The regenerated init_nu snapshot is byte-identical to the merged template with {{ cmd }} → wt substituted, which is the only substitution the render performs, so the snapshot can't be silently stale.
That closes the one live point from my earlier review — the "should merge without conflict" claim, which the description now records as resolved. The cd $target_dir site remains the third abort path in this function, still unfixed by either PR and still out of scope here; it's now unblocked for a follow-up, since the two overlapping PRs have converged.
Not approving: self-authored, so GitHub blocks it — the merge call is yours.
|
Conflicts resolved — |
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
Problem
The nushell wrapper's
--executedirective call wasn't wrapped intry. Nushell 0.98+ raises aShellErroron a non-zero external exit, so a--executebody that exits non-zero unwound the wholedef— cleanup never ran, all threemktempfiles leaked, and the stdout the function still had to return was discarded. Cleanup has always sat after the exec call, so this is pre-existing.Verified end-to-end against nushell 0.114.1 (the version CI pins) driving the real rendered wrapper in a dedicated
TMPDIR:wt switch --create … --execute "echo BODY; exit 42"wt switch --definitely-not-a-flag(wt itself fails)--execute "echo OK", thenwt config show)The bare mechanism, with no wrapper involved:
def wrapper [] { let f = (mktemp --tmpdir) if true { ^sh -c "exit 42" } print "REACHED_AFTER_EXEC" # never prints "RETVAL" # never returned }Why the existing test stayed green
test_wrapper_execute_exit_code_propagationalready drives this exact command fornuand passes — but it passes because the unwind happens to carry exit 42 out to the shell, not because the wrapper handled it. The assertion the test makes is satisfied by the failure mode, so the leak sat on a path that looked covered.Solution
Wrap the exec call in
try, the same way the binary call above it already is, and fold the body's exit code into$exit_code:The end of the function then propagates it through the existing
^sh -c $"exit ($exit_code)"path, so the 42 is the wrapper's doing rather than a side effect of the unwind — and cleanup runs on the way.#3733 flagged the precedence question — when both
wtand the body fail, which code wins — as a user-visible contract rather than a mechanical port. It turns out the contract is already set: bash, zsh and fish all guard theirsrc_exitassignment withif $exit_code -eq 0, i.e. first non-zero wins, wt before the body. This mirrors that rather than inventing a nushell-specific rule, which is what took it from an issue to a PR.^sh -c $scriptkeeps its non-terminal position inside thetryblock, so the body's stdout still goes straight to the terminal — verified above (BODYappears in both columns).Testing
test_nu_wrapper_execute_failure_runs_cleanupintests/integration_tests/shell_wrapper.rsruns the real wrapper through a PTY against a dedicatedTMPDIRand asserts both halves: the body's exit code still reaches the caller, and the temp dir is empty afterwards. It fails on the current template withleaked temp files: ["tmp.zyJKCYvPHq", "tmp.eF7z3IFox0", "tmp.0sJXkEnCXb"]and passes with the fix.The temp-dir assertion is the part that distinguishes the two states —
WT_EXIT:42is present in the failing run too, which is exactly why the pre-existing test couldn't catch this. The driving script wraps thewtcall in nushell's owntry, since the wrapper's terminal^sh -c $"exit …"is meant to abort the caller; without that the intended propagation would hide whether cleanup ran first.Also run locally:
cargo test --lib --bins(2,450 passed, including the regeneratedinit_nusnapshot), the fullshell_wrappermodule with--features shell-integration-tests(62 passed), andtest_docs_are_in_sync+shell_init+config_show(129 passed).Relationship to #3732
Same abort-before-cleanup shape, different site: #3732 fixes the cleanup
rmcall, this fixes the exec call. Neither fixes the other's site, and they're separately revertable.Resolved. #3732 merged first, so
origin/mainis now merged into this branch (938976ec8). The conflict was intemplates/nushell.nuand theinit_nusnapshot, exactly as predicted — resolution was mechanical: this PR'slet exit_codefold stays, and the oldrm -f $cd_file $exec_fileline it sat above is gone, replaced by #3732's consolidatedtry { ^rm -f … }block after thelet outputread.cargo test --lib shell::tests::test_shell_init_generateconfirms the regenerated snapshot matches the resolved template.A third site in the same function is not covered by either PR:
cd $target_dirin the cd-directive block raises on a missing or inaccessible directory and unwinds thedefthe same way, leaking the same three temp files. Left for a follow-up so this PR stays one concern.Closes #3733 — automated triage