hook, shell: cd-tracking and shell-not-found robustness fixes - #3002
Open
alurm wants to merge 10 commits into
Open
hook, shell: cd-tracking and shell-not-found robustness fixes#3002alurm wants to merge 10 commits into
alurm wants to merge 10 commits into
Conversation
sandydoo
added
bug
_DEVENV_HOOK_DIR marks the one shell devenv hook's activation function spawned, so it knows to `exit` on cd-out (763dd26). But it's a plain process environment variable, so it wasn't scoped to that one shell — any descendant process forked from it afterward (a new tmux/zellij pane, a manually started nested shell) inherited it too. If that descendant re-sources the hook script, as any ordinary interactive rc file would, it wrongly concludes it's also hook-spawned, and calls `exit` on cd-out with nothing set up to catch it — closing the pane/session outright. Capture the marker into a non-exported variable as soon as each hook script reads it, then erase the exported copy, so it can't propagate past the one process it was meant for. Reproduced against a real tmux + patched devenv binary before and after to confirm the fix; added regression tests for bash/zsh/fish/nu that fail without the fix (spawn a shim `devenv shell` from a child process) and pass with it. Fixes cachix#2861. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Cd-out detection for a hook-spawned shell lived only in the outer hook.fish/posix.sh/nu script, and depended on that same script being re-sourced a second time inside the spawned shell via the user's own rc file. That's not guaranteed: a hook-spawned `fish -i` is non-login, and a common fish idiom gates rc content behind `status is-login` — which never re-arms the hook there. Confirmed live: with such a config, `cd ..` out of an active devenv shell did nothing at all; the environment just stayed active. Move cd-out detection into devenv-shell's own generated init file for each dialect (bash/zsh/fish/nu), gated on the same _DEVENV_HOOK_DIR marker. This runs unconditionally as part of `devenv shell` itself, independent of whatever the user's rc does, so it's not dependent on that shell's own rc doing the right thing. For bash, zsh, and nu, consume the exported marker before sourcing user configuration so it still cannot leak, but defer registering the prompt/PWD callback until afterward; fish's -C integration already runs after config.fish. Preserve any existing user callbacks while installing devenv's own, with regressions that deliberately replace each shell's callback configuration and verify both sides still run. Manual/nested `devenv shell` invocations are unaffected: the new code only activates when _DEVENV_HOOK_DIR is set, which is only ever true for the hook's own spawn call, never a manual one. Verified live, including a hook-spawned shell with a manually-nested devenv shell inside it. This leaves a duplicate copy of the exit-on-cd-out logic in the outer hook scripts (harmless — whichever copy runs first wins, the other becomes a no-op) that could be removed as a follow-up now that this copy is unconditionally reliable. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Fish defers acting on an activation decision to the next prompt (8eff3cd, to dodge zoxide's recursion guard): _devenv_hook decides on a PWD change, _devenv_hook_activate spawns on the following prompt. Nothing re-checked, at spawn time, whether something else — direnv loading `.envrc`'s `use devenv`, or a manually entered devenv shell — had activated the same directory in between. Reproduced directly: export DEVENV_ROOT between the decision and the deferred prompt, and _devenv_hook_activate still spawns a redundant nested devenv shell on top. Recheck DEVENV_ROOT immediately before spawning and bail if it's already set. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
) _devenv_hook_activate follows the user out of a project with `builtin cd`, not `cd` (9486594, to dodge "zoxide: infinite loop detected" when the user overrides `cd`, e.g. `zoxide init --cmd=cd`). `builtin cd` bypasses fish's own `cd` *function* entirely though, and that function is where fish records directory history for `cd -`/ `prevd`/`nextd` (`$dirprev`) — it isn't a PWD-change hook. So a subsequent `cd -` silently skipped over the project directory instead of returning to it. Root cause confirmed with a minimal, zero-devenv-code repro: a function-internal `builtin cd` never updates $dirprev, while the same function using plain `cd` does — and using a wrapped `cd` (to simulate zoxide) makes it visible that `builtin cd` correctly avoids calling it. Add _devenv_builtin_cd_with_history, which replicates fish's own `cd` function's $dirprev/$dirnext bookkeeping around `builtin cd`, without calling `cd` (and thus without invoking any override). Confirmed live that it neither re-triggers a user's `cd` wrapper nor regresses the "infinite loop" fix. Fixes cachix#2853. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Exiting a hook-spawned devenv shell always dropped back into the outer shell that spawned it — even when that outer shell never had a prompt of its own. A fresh terminal opened straight into a trusted project (e.g. a new Zed/tmux tab) auto-activates on the shell's very first prompt (_devenv_hook_init / the first-ever PROMPT_COMMAND firing), before the user ever sees or uses that outer shell. Exiting the inner shell there just leaves an empty vehicle shell behind, requiring a second exit/Ctrl-D to actually close the terminal. Distinguish that case from an activation that happens after the user already interacted with the outer shell (a later `cd` into the project). Both signals needed already exist: whether the inner shell exited outright vs. cd'd out (exit-dir file present or not), and whether this is the shell's first-ever activation (_devenv_hook_init/first PROMPT_COMMAND firing) vs. a later one. Combining them: a plain exit from a first-activation shell now propagates, closing the outer shell too; a later activation never propagates, since the user may want that shell back. nu is not touched: env_change.PWD does not fire for the starting directory when nu starts already inside a trusted project (verified directly), so there is no zero-interaction activation path to key off there in the first place — nothing regresses, but nu still needs two. Verified live end-to-end in tmux: a fresh pane rooted in a trusted project closes entirely on one Ctrl-D; a pane where the outer shell was used first and then cd'd into the project survives one Ctrl-D as before, only the inner shell exiting. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Found while manually verifying the exit-propagation feature under /tmp: macOS symlinks /tmp -> /private/tmp (and similarly elsewhere). DEVENV_ROOT is canonicalized by devenv's Rust side (fs::canonicalize), while $PWD preserves whatever symlinked path the user actually cd'd through — every cd-out check in the codebase compares them as raw strings, so a project living under such a path had the hook-spawned shell immediately (and wrongly) conclude the user had left the project the moment it started, writing an exit-dir file and exiting with nothing having actually changed. This affected both the outer hook scripts (hook.fish/posix.sh/nu) and the dialect-embedded copies added earlier. Resolve both sides before comparing, using builtins only (no `realpath` dependency, keeping with devenv hook's "no external tools required" design): fish's `path resolve`, a portable `cd -P`/`pwd` subshell shared by bash and zsh (extracted into one function, `exit_on_cd_out_snippet`, since the two dialects' copies were byte-for-byte identical), and nu's `path expand`. Falls back to the raw value if resolution fails (e.g. the directory was removed out from under the shell). The existing test suite didn't catch this: it sets DEVENV_ROOT directly to the same tempfile path used for `cd`, so both sides were always identical strings — never exercising the mismatch. Added dedicated regression tests that canonicalize the temp directory themselves (skipping gracefully on platforms where it isn't symlinked, e.g. most Linux CI) and set DEVENV_ROOT to the canonical form while `cd`-ing through the symlinked one, for all four dialects. Verified live end-to-end with the real binary in tmux (a project under /tmp activates and stays active instead of silently bouncing back to a plain prompt) and confirmed real cd-out still works correctly afterward. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Without this, devenv falls back to $SHELL (the login shell), which is frequently stale and can disagree with the shell the hook was actually loaded into (e.g. a fish user whose $SHELL is still /bin/bash) — silently activating the wrong dialect's environment. Each hook script now passes its own known dialect explicitly. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
"set SHELL to an absolute path" read as if any valid $SHELL would work, when it specifically means "point SHELL at this dialect's own binary" — misleading when $SHELL is already a different, valid shell. Also fixed, independent of the wording: `resolve_shell_path` returned the bare, unresolved shell name (e.g. "nu") instead of signaling failure when a dialect binary couldn't be found, and that name flowed straight into `apply_shell_env`, which unconditionally overwrote SHELL on the intermediate bash process with it — before the Nix environment's own enterShell hook even runs, since env sourcing happens earlier in the generated rcfile than this check. Any shellHook that branches on $SHELL would have seen a bogus, nonexistent path instead of the user's real shell. Fixed by having `resolve_shell_path` return `Option<String>`, and only overriding SHELL in the child environment when resolution actually succeeded; otherwise the caller's real SHELL passes through untouched. An earlier version of this commit also added a $SHELL fallback (spawn $SHELL, with a warning, when the requested dialect's own binary can't be found) for hook-triggered spawns. Removed after live-testing turned up a real gap in it: if $SHELL was fish, fish's own vendor_conf.d-autoloaded hook.fish would still read the leftover _DEVENV_HOOK_DIR marker and wrongly treat the fallback shell as fully hook-spawned (enabling cd-out-exit detection) despite the warning's claim of "without dialect-specific integration" — a half-integrated state that depends on happenstance (whether $SHELL's own hook loading reaches for that marker) rather than anything deliberately designed. Concluded a plain, clearly-worded hard error is more predictable than a fallback that silently substitutes a different, only-sometimes- still-hooked shell for what was actually requested. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
`cd /; cd <trusted-project>; cd -` silently failed to return anywhere right after activation. Root cause differs per dialect: - bash/zsh discard an inherited OLDPWD unconditionally, even across their own `exec` — confirmed directly against plain `bash`/`zsh`/ `sh`, unrelated to devenv. Re-derived by actually cd-ing to the previous directory and back once the real shell has started, which is the only way to make OLDPWD meaningful to them. - fish's `cd -` doesn't consult OLDPWD at all; it tracks `$dirprev`, a shell variable that can't cross a process boundary. Seeded directly, matching the MAX_DIR_HIST bookkeeping `_devenv_builtin_cd_with_history` already replicates from fish's own `cd` function. - nu is the only one of the four where a plain freshly-exported OLDPWD just works. Each hook script now passes the previous directory through via `_DEVENV_PREV_PWD`; each dialect's own init file consumes it. Along the way, found a separate, more significant gap while trying to verify the bash fix at all: `devenv shell`'s non-TUI interactive path special-cased bash through `bash_init_script`, an older, much simpler generator that predates the hook-dir/cd-out/symlink fixes earlier in this stack — none of them ever ran for bash outside the TUI (e.g. `--no-tui`, non-interactive terminals, CI). zsh/fish/nu were never affected; they already went through the shared `dialect.rcfile_content()` path here. Bash now does too, closing that gap. Also found (via live-testing the exact repro of re-entering a project via `cd -` itself, rather than a fresh `cd <name>`): fish's own `cd -` pops the entry `_devenv_hook_activate` reads from `$dirprev` and moves it to `$dirnext` instead, flipping `$__fish_cd_direction` to `next` — so reactivating by typing `cd -` left nothing in `$dirprev` for the next activation to capture, and `cd -` inside the newly-spawned shell was lost. `_devenv_hook_activate` now checks `$__fish_cd_direction` to read whichever of `$dirprev`/`$dirnext` actually holds the entry. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Found while manually verifying the pr-hook-fixes bundle end-to-end: cd into a trusted project, cd out, cd right back in — the second entry silently failed to reactivate (bash and zsh; fish and nu use event-driven PWD hooks with no equivalent cache, so they're unaffected). Pre-existing bug, unrelated to anything else in this stack. `_devenv_hook` short-circuits when `$_DEVENV_HOOK_PWD` still equals `$PWD`, to avoid re-running `hook-should-activate` on every prompt when nothing has changed. Following the user out on cd-out (`cd "$target_dir"`) never updated that cache, so it stayed at the old project directory. Re-entering that same project later coincidentally matched it again, wrongly short-circuiting the check. First fix attempt (set the cache to the new location after following out) broke the existing sibling-activation test: it made the immediate follow-up check treat the new directory as already-known too, skipping activation there instead. Clearing the cache instead of setting it handles both cases: the next call always re-evaluates, whether that's the immediate sibling check or a much later re-entry. Verified live via tmux with the real binary (bash and zsh) and with a new regression test that fails without the fix and passes with it. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
alurm
force-pushed
the
pr-hook-robustness
branch
from
July 16, 2026 12:01
33f851b to
e23b2ba
Compare
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
2 participants
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.
Depends on #3001.
Five more fixes found while further testing the hook:
devenv shellexplicitly which dialect to spawn.devenv shell --shell zshwhen zsh isn't installed anywhere (not indevenv.nixpackages, not onPATH) printeddevenv: add zsh to your devenv.nix packages, or set SHELL to an absolute path— the second half named no shell, so it read like pointing$SHELLat any installed shell (e.g./bin/bash) would fix it; it now saysset SHELL to zsh's absolute path, naming the specific binary that's actually required.cd -silently failed to return you to the previous directory right after a devenv shell first activates, across all four dialects, each for a different underlying reason (bash/zsh discard OLDPWD unconditionally, fish tracks$dirprevinstead which doesn't cross a process boundary). Found along the way: devenv shell's non-TUI interactive path used an older script generator that predated the earlier fixes in this series, so bash alone was missing all of them — the hook-dir leak fix, cd-out detection, and symlink resolution — now unified with zsh/fish/nu's path. Separately (fish only): reactivating a project by typingcd -itself, rather than a freshcd <name>, also brokecd -inside the newly-spawned shell — fish's owncd -moves the directory-history entry the hook needs from$dirprevto$dirnext, and the hook was only ever looking at$dirprev.Assisted by Claude Sonnet 5.