fix: don't name shell variables "path" - zsh ties it to $PATH - #7
Merged
Conversation
In zsh the `path` array is tied to `PATH`, so a `local path` (or a bare `read -r path`) empties PATH for that function's scope and everything it calls. `wt merged --rm` hit this at `local main_wt path branch`: every subsequent git/awk call failed with "command not found", _wt_resolve returned nothing, and removal aborted with "no worktree matching". Rename to wt_path at all four sites - declaration and every use. Renaming only the declaration would be worse: the leftover `read -r path` would then write into zsh's tied array and clobber PATH permanently rather than for the function's scope. WT_PATH, the hook env var, is not a special name and is unchanged. Also drop the claim that the `claude` CLI mutates the caller's PATH - a misdiagnosis of this same bug (a subprocess cannot alter its parent's environment). The up-front binary resolution stays; the one-shot session fetch is still worth it. Tests run under bash, where this bug class is invisible, so add a behavioural zsh test (skipped when zsh is absent) plus a static grep guard. Both fail against the pre-fix wt.sh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a zsh-specific failure mode where naming a shell variable path (via local path or read -r path) can clobber PATH due to zsh’s tied path array, causing downstream commands like git/awk to stop resolving during wt merged --rm.
Changes:
- Renames shell variables previously named
pathtowt_pathin affected functions to avoid zsh’spath/PATHcoupling. - Removes an incorrect comment about the
claudeCLI mutating the caller’sPATH. - Adds zsh-focused tests: a behavioral
wt merged --rmrun under real zsh (skipped if absent) plus a static guard to prevent reintroducingpathas a shell variable name.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| wt.sh | Renames shell variable path usages to wt_path and updates related comments to prevent zsh PATH clobbering. |
| test/zsh.bats | Adds a zsh behavioral regression test and a static grep-based guard against path shell variables. |
| test/helpers.bash | Exposes WT_SH path for tests so zsh can source the same script location reliably. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
wt merged --rmfails partway through under zsh, leaving the worktree in place:In zsh the
patharray is tied toPATH. Alocal path(or a bareread -r path) emptiesPATHfor that function's scope and every function it calls, sogit/awkstop resolving mid-run.This explains the failure ordering exactly: in
_wt_mergedthe awk/grep/confirm prompt all run beforelocal main_wt path branch failed=0; the first command after it isgit worktree list | awk— the first error seen._wt_rm→_wt_resolvethen fail for the same reason, producing the "no worktree matching" tail.PATHis restored when the function returns, which is why the shell keeps working afterwards.The fix
Rename to
wt_pathat all four sites — declaration and every use:_wt_claude_tableand_wt_merged— the two reachable from the reported failure_wt_claude_rm_sessions,_wt_run_adhoc_hook— same latent bugRenaming only the declaration would be worse: the leftover
read -r pathwould then write straight into zsh's tied array and clobberPATHpermanently rather than for the function's scope.WT_PATH, the hook env var, is not a special name and is unchanged — the hook contract is untouched.Also drops the comment claiming the
claudeCLI mutates the caller'sPATH. That was a misdiagnosis of this same bug (a subprocess cannot alter its parent's environment); the real reason the jq/column lookups had to happen early waslocal pathin_wt_claude_table. The up-front binary resolution stays — the one-shot session fetch is still worth it.Tests
The suite runs under bash, where this bug class is completely invisible, so
test/zsh.batsadds:wt merged --rmrun under a real zsh (skips when zsh is absent)local/readnaming a variablepath, covering functions with no zsh coverageBoth fail against the pre-fix
wt.shand pass after. Full suite: 55/55 green.🤖 Generated with Claude Code