fix(shell): resolve binary in fish package completions to avoid wrapper recursion

[worktrunk-bot]

Problem

On a fresh fish shell, tab-completing wt <TAB> before running any wt command recurses into the lazy-load wt wrapper function until fish aborts with "The call stack limit has been exceeded" (#3240).

Root cause: wt config shell completions fish — the script package managers ship as the vendored share/fish/vendor_completions.d/wt.fish (e.g. Homebrew's generate_completions_from_executable) — emitted clap's raw registration, which calls the bare wt command at TAB time:

complete --keep-order --exclusive --command wt --arguments "(COMPLETE=fish wt -- (commandline …))"

When worktrunk's lazy-load wrapper function shadows the binary (the state before the first wt invocation), that bare wt re-enters the wrapper. fish has already exported COMPLETE=fish, so the wrapper's command wt config shell init fish | source makes the binary emit a completion registration instead of the init script — the real wt function is never defined — and the wrapper's trailing wt $argv recurses into the stub to the call-stack limit.

The install-path completion (wt config shell install) was already safe: it resolves the binary via type -P wt, bypassing the wrapper. Only the package-manager path was exposed.

Solution

Make wt config shell completions fish emit the same binary-resolving registration as wt config shell install, reusing configure_shell::fish_completion_content (via type -P, with the WORKTRUNK_BIN override). This mirrors the existing make_zsh_autoload_safe transform applied to the zsh package registration.

Changed files: src/commands/init.rs (emit the safe form for fish, drop the now-unused EnvFish import, update the doc comment), src/commands/configure_shell.rs (fish_completion_content → pub(crate)).

Testing

• Reproduced the bug end-to-end with fish 3.7.0: installed the lazy-load stub plus the old bare-wt vendor completion, then fish -c 'complete -C "wt remove "' recursed to the call-stack limit (in function 'wt' with arguments '-- wt remove ""' repeating). The install-path type -P completion did not recurse, isolating the bare-wt registration as the cause.
• Verified the fix: after rebuilding, the same complete -C "wt remove " and the issue's minimal repro fish -c 'complete -C "wt switch "' both produce completions with no recursion.
• Added a regression assertion to test_completions_for_all_shells (tests/integration_tests/completion.rs): the fish registration must resolve the binary (type -P / WORKTRUNK_BIN) and must not call the bare wt command. It fails against the old output and passes with the fix.
• cargo test --test integration completion (66) and configure_shell (48) pass; cargo clippy --bin wt is clean.

---

Closes #3240 — automated triage

[max-sixty]

can you break down the sequence more? why does this only apply before having run wt?

because running wt loads the lazy function? but tab-completing doesn't?

[worktrunk-bot]

The fix itself reads correctly: routing wt config shell completions fish through fish_completion_content makes the package-manager / Homebrew-vendored registration resolve the real binary via type -P (with WORKTRUNK_BIN override), the same form wt config shell install already wrote, so the bare-wt registration that recurses through the lazy-load wrapper is gone. It parallels the existing make_zsh_autoload_safe transform, the dropped EnvFish import is genuinely unused, and the regression assertions in test_completions_for_all_shells pin the type -P / WORKTRUNK_BIN form and reject the bare COMPLETE=fish wt call. Bash/zsh don't share the bug because their wrapper has an explicit COMPLETE guard that calls command "${WORKTRUNK_BIN:-wt}".

Flagging for human sign-off per worktrunk's data-loss-surface policy: this PR edits src/commands/configure_shell.rs, which already contains fs::remove_file (the shell-config uninstall/install paths). The diff there is a one-token visibility change (fn fish_completion_content → pub(crate) fn) at a location far from the removal logic, so it can't alter deletion behavior — but the policy holds a change touching that file for maintainer review rather than letting an agent judge it harmless. Requesting @max-sixty.

Not approving — self-authored PR (GitHub blocks self-approval), and the data-safety surface is the maintainer's call regardless.

[worktrunk-bot]

Good question — the "before having run wt" framing is slightly off, and pinning down exactly why is the clearest way to explain it. Tab-completing does trigger the lazy-load wrapper. The asymmetry isn't "loads the function vs. doesn't" — it's the COMPLETE=fish env var, which only the completion path sets, and which sabotages the wrapper's self-replacement step.

The two wt definitions in play:

• The wrapper stub (~/.config/fish/functions/wt.fish, templates/fish_wrapper.fish) — what fish autoloads the first time wt is referenced in a session. Its whole job is to run command wt config shell init fish | source (replacing itself with the real function) and then re-dispatch via wt $argv.
• The full function (templates/fish.fish) — emitted by wt config shell init fish, sourced in by the stub. It resolves the binary with type -P wt into $WORKTRUNK_BIN and invokes $WORKTRUNK_BIN directly, never the bare wt command — so it can't re-enter itself.

Why running wt normally is safe (no COMPLETE set):

1. fish autoloads the stub, calls it.
2. Stub runs command wt config shell init fish. COMPLETE is unset, so the binary emits the init script (the full function).
3. source redefines wt → the in-memory wt is now the full function.
4. Stub's trailing wt $argv hits the full function → works. The stub is gone for the rest of the session.

Why tab-completing in that same fresh session recurses (COMPLETE=fish set):

The old vendored completion registered complete --command wt --arguments "(COMPLETE=fish wt -- …)". On <TAB>, fish:

1. Exports COMPLETE=fish and evaluates wt -- <tokens>. That wt reference autoloads the stub and calls it (same as above — completion is not exempt from loading the function).
2. Stub runs command wt config shell init fish. But COMPLETE=fish is now in the environment, so clap_complete's CompleteEnv intercepts dispatch before config shell init fish ever runs and emits a completion registration instead of the init script (the same intercept the test harness simulates).
3. source therefore sources a completion registration, not the full function — so wt is still the stub.
4. Stub's trailing wt $argv re-enters the stub. COMPLETE=fish is still set, so step 2 repeats identically → infinite recursion → "The call stack limit has been exceeded".

So the precise condition isn't "before having run wt" but "while the in-memory wt is still the stub and COMPLETE is set." Running any normal wt first replaces the stub with the full function, which (a) is what completion would call thereafter, and (b) dispatches through $WORKTRUNK_BIN directly rather than re-entering wt, so even with COMPLETE=fish set there's nothing to recurse into. Each new fish session re-autoloads the stub, so the window reopens until the first wt of that session.

The fix sidesteps the whole chain: the package-manager completion now resolves type -P wt → $WORKTRUNK_BIN and calls the binary directly, exactly like wt config shell install already did — so the bare wt is never invoked at TAB time and the stub is never re-entered, regardless of COMPLETE.

[worktrunk-bot]

Heads-up for review: test (windows) is red, but the failure is an unrelated flake, not from this change.

The failing test is test_switch_picker_alt_x_keeps_cursor_sticky — a timing-sensitive interactive picker PTY test (assertion left == right failed: switch after alt-x should exit 0). It passed on both test (linux) and test (macos), and this PR touches only fish completion output (init.rs, configure_shell.rs) plus a completion test — nothing in the picker path. A Windows rerun should clear it.

All other required checks, including codecov/patch, are green.

[max-sixty]

@worktrunk-bot rerun windows test and create a new PR to fix the flake pls

[worktrunk-bot]

Done — fix PR and reruns:

Fix PR: #3242 de-flakes test_switch_picker_alt_x_keeps_cursor_sticky. The failing run showed the switch verifiably succeeded (the --format=json result and Switched to worktree for wt-keep were both on screen) yet wt exited 1 on Windows. The test's actual subject — alt-x keeping the cursor on the slid-up row — is already proven by those screen assertions; the exit_code == 0 pin coupled it to the interactive skim session's process exit code, which is a separate racy signal on Windows (this file's assert_valid_abort_exit_code already treats skim PTY exit codes as non-deterministic). #3242 keeps the screen-based success check and accepts skim's 0/1 codes.

Reruns: the first windows rerun cleared …alt_x_keeps_cursor_sticky, but a different picker PTY test flaked — test_switch_picker_worktree_row_comments_tab_shows_thread timed out after 30s waiting for diff --git in the preview pane. That's a distinct preview-content symptom, so I left it out of #3242 (one concern per PR) and kicked off another windows rerun.

Why de-flake the test rather than chase the exit code

The failing run finished in 2.74s — under the test's 5s exit wait — so the child exited on its own (not killed by the timeout). The switch pipeline ran, emitted its JSON payload, and printed the success message; the non-zero exit comes from somewhere after that success output, on Windows only. I couldn't reproduce it on the Linux CI runner to root-cause it, so #3242 de-flakes the cursor-stickiness test (which was never meant to assert process-exit hygiene) rather than shipping an unverifiable product change. The repeated, differing Windows symptoms across reruns point to a broader class of picker-PTY flakiness worth a separate look.