refactor(dirstack): move directory stack to typed interpreter state#2103
Merged
Conversation
pushd/popd/dirs stored the whole directory stack in the user variable namespace as _DIRSTACK_SIZE / _DIRSTACK_N, guarded by is_internal_variable() — a data structure leaked into shell variables that scripts could forge. Move it to a typed dir_stack: Arc<Vec<String>> on ScopedState (so it is subshell-isolated via the scoped snapshot like aliases/traps) and expose it to the pushd/popd/dirs builtins via ShellRef::dir_stack. Add it to ShellState (serde default) + restore so it roundtrips through session snapshots, and drop _DIRSTACK_ from the is_internal_variable() blocklist and its test list. The DoS size cap now applies to pushd growth instead of clamping a user-forgeable size variable. Replace the magic-variable-inspecting unit tests with interpreter-level spec coverage (dirstack.test.sh), adding dirs -v and a regression that the former _DIRSTACK_* names are now inert ordinary variables. Adds a ShellState dir-stack roundtrip test. Concludes the #1 migration of magic-variable channels to typed state.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
bashkit | ab80a7d | Commit Preview URL Branch Preview URL |
Jun 19 2026, 08:34 PM |
There was a problem hiding this comment.
Pull request overview
Moves the directory stack backing pushd/popd/dirs out of user-forgeable shell variables (_DIRSTACK_*) and into typed, subshell-scoped interpreter state, and ensures it roundtrips through shell_state() / restore_shell_state().
Changes:
- Add
dir_stacktoScopedState(Arc-wrapped, subshell-isolated) and plumb it throughShellRef. - Refactor
crates/bashkit/src/builtins/dirstack.rsto usectx.shell.dir_stackand enforce a DoS cap on push growth. - Update tests: add spec coverage for
dirs -vand_DIRSTACK_*now being ordinary inert vars; add a unit test for shell-state roundtrip.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/bashkit/tests/spec_cases/bash/dirstack.test.sh | Adds spec coverage for dirs -v output and verifies _DIRSTACK_* vars no longer affect the directory stack. |
| crates/bashkit/src/interpreter/mod.rs | Introduces typed dir_stack in scoped interpreter state and includes it in shell-state snapshot/restore + unit test. |
| crates/bashkit/src/builtins/dirstack.rs | Rewrites pushd/popd/dirs to use typed dir_stack instead of _DIRSTACK_* variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review nits: - pushd used exists() + stat() with unwrap_or(false), misreporting IO/TOCTOU errors as 'Not a directory'. Use one stat() and branch on Ok(dir)/Ok(non-dir)/Err(not found). - dirs cloned the whole stack into a Vec even on the default path that doesn't use it. Return early for the default output and iterate the borrowed dir_stack (reversed) for -p/-v without allocating.
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.
Why
Concludes the #1 series (interpreter-internal state out of the user variable namespace). This is the clearest "leaked abstraction" of the bunch:
pushd/popd/dirsstored the entire directory stack as_DIRSTACK_SIZE/_DIRSTACK_Nshell variables, guarded byis_internal_variable()— a data structure living in (and forgeable from) the user namespace.What
dir_stack: Arc<Vec<String>>toScopedState(so it's subshell-isolated via the scoped snapshot, exactly likealiases/traps), and expose it to thepushd/popd/dirsbuiltins throughShellRef::dir_stack(the established pattern for internal-builtin state).dirstack.rsto operate onctx.shell.dir_stackinstead of_DIRSTACK_*variables.dir_stacktoShellState(serde default) + populate + restore so it roundtrips throughshell_state()/restore_shell_state()session snapshots._DIRSTACK_from theis_internal_variable()blocklist and its test list. The DoS size cap (DIRSTACK_MAX_SIZE) now applies topushdgrowth instead of clamping a user-forgeable_DIRSTACK_SIZE.Tests
dirstack.test.sh(the existing suite already covered pushd/popd/dirs/swap/clear/per-line behavior through the real interpreter). Addeddirs -vanddirstack_internal_names_are_ordinary_vars(setting_DIRSTACK_SIZE/_DIRSTACK_0is now inert).test_shell_state_roundtrips_dir_stackunit test (dir stack survivesshell_state()+restore_shell_state()).bash_spec_tests, 2477 lib unit tests, 44 snapshot tests green. fmt + clippy clean.