fix(bash): avoid duplicate trust warning after cd#8920
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Bash shell integration to prevent duplicate environment hooks when changing directories by introducing a tracking variable and separate prompt and directory-change hooks. It also adds several E2E tests for Bash. The review feedback highlights that the new test_bash_duplicate_trust_warning test script contains several Zsh-specific variable names that do not match the Bash implementation, which will cause the tests to fail.
Greptile SummaryThis PR fixes a duplicate The deactivation script is significantly improved: it now handles both string and array forms of
Confidence Score: 5/5Safe to merge — the core skip mechanism is correct and all previously reported issues are resolved No P0 or P1 issues found. The __MISE_BASH_CHPWD_RAN skip flag is set before hook-env runs and reset on the next precmd, correctly deduplicating the warning. Deactivation cleanup is thorough. The two P2 comments (fragile test strings, missing __MISE_ZSH_PRECMD_RUN unset) do not block merge. No files require special attention; minor style suggestions in e2e/cli/test_bash_duplicate_trust_warning and src/assets/bash/deactivate.sh Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant cd as cd (via __zsh_like_cd)
participant chpwd as _mise_hook_chpwd
participant PC as PROMPT_COMMAND
participant precmd as _mise_hook_prompt_command
participant hookenv as mise hook-env
User->>cd: cd untrusted-project
cd->>chpwd: fires chpwd_functions
chpwd->>chpwd: __MISE_BASH_CHPWD_RAN=1
chpwd->>hookenv: hook-env --reason chpwd
hookenv-->>User: trust warning (shown once)
Note over PC: shell renders next prompt
PC->>precmd: execute PROMPT_COMMAND
precmd->>precmd: CHPWD_RAN==1 → reset to 0, return early
precmd-->>User: no duplicate warning
Note over PC: subsequent prompts (no cd)
PC->>precmd: execute PROMPT_COMMAND
precmd->>hookenv: hook-env --reason precmd
hookenv-->>User: normal env check
Reviews (12): Last reviewed commit: "refactor(bash): extract hook scripts int..." | Re-trigger Greptile |
3941b57 to
14ea84b
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bash shell integration to prevent duplicate hook-env executions by coordinating between chpwd and PROMPT_COMMAND using a new skip flag (__MISE_BASH_CHPWD_RAN). It also adds support for PROMPT_COMMAND as an array and includes several new E2E tests to verify the behavior and cleanup. Feedback was provided regarding the deactivation logic for chpwd_functions, suggesting a more robust loop-based removal to avoid potential issues with substring replacement and empty array elements.
485106b to
fd1720d
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bash shell integration to prevent duplicate environment hooks, specifically addressing a double mise trust warning when changing directories. It introduces a state variable __MISE_BASH_CHPWD_RAN to coordinate between chpwd and PROMPT_COMMAND hooks. Additionally, it improves the robustness of PROMPT_COMMAND management by supporting both array and string formats and ensures thorough cleanup during deactivation. Several new E2E tests were added to verify these behaviors. I have no feedback to provide.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bash activation and deactivation logic by moving shell scripts into dedicated asset files and separating the environment hooks into _mise_hook_prompt_command and _mise_hook_chpwd. It introduces a skip flag to prevent duplicate trust warnings and adds comprehensive E2E tests for these scenarios. Feedback focuses on ensuring robust cleanup of legacy hook names (_mise_hook) from PROMPT_COMMAND and chpwd_functions during deactivation to prevent "command not found" errors, as well as using more idiomatic Bash variable expansion when handling PROMPT_COMMAND as a string.
c99434f to
459ab5d
Compare
Keep `PROMPT_COMMAND` persistent in bash and coordinate it with `chpwd` via `__MISE_BASH_CHPWD_RAN` so entering an untrusted project does not show the same `mise trust` warning twice for a single `cd`. Handle both string and array `PROMPT_COMMAND` forms so the hook logic works correctly across older and newer Bash versions.
459ab5d to
9ead5c5
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bash activation and deactivation logic by moving shell scripts from inline Rust strings into dedicated asset files and implementing a template rendering system. It introduces a state flag, __MISE_BASH_CHPWD_RAN, to prevent redundant environment checks and duplicate trust warnings when navigating directories. Feedback focuses on improving the deactivation process, specifically by restoring the original command_not_found_handle, ensuring all internal variables are unset, and refining the string manipulation used to clean up PROMPT_COMMAND. Additionally, a suggestion was made to set the skip flag during the initial activation hook to prevent duplicate warnings on shell startup.
| chpwd_functions+=(_mise_hook_chpwd) | ||
| fi | ||
|
|
||
| _mise_hook |
There was a problem hiding this comment.
To avoid a duplicate trust warning on shell startup when activating inside an untrusted project, the initial _mise_hook call should also set the __MISE_BASH_CHPWD_RAN flag. This ensures that the first PROMPT_COMMAND run immediately following activation will skip its redundant environment check.
| _mise_hook | |
| _mise_hook && __MISE_BASH_CHPWD_RAN=1 |
9ead5c5 to
aa5ee06
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the bash activation and deactivation logic by moving shell scripts into external asset files and implementing a template rendering system. It introduces a state flag to prevent duplicate environment hooks and trust warnings when both chpwd and PROMPT_COMMAND are triggered. New E2E tests verify these hook behaviors and deactivation cleanliness. Review feedback identifies a high-severity issue where double-quoting a pre-escaped executable path could lead to execution errors, and suggests a check to prevent duplicate hook registrations in the chpwd_functions array.
aa5ee06 to
43cbfb9
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bash shell integration by moving shell scripts from inline Rust strings into dedicated asset files and improving the hook mechanism. It introduces a state variable to prevent duplicate environment hooks and warnings when changing directories in Bash. Additionally, it enhances the deactivation logic to ensure a cleaner shell state upon removal and includes several new E2E tests to verify these improvements. I have no feedback to provide.
43cbfb9 to
c5a66b5
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bash shell integration by moving shell scripts into dedicated asset files and implementing a template rendering system in Rust. It addresses a bug where mise trust warnings were duplicated in Bash by introducing a state variable to synchronize the chpwd and PROMPT_COMMAND hooks. Several E2E tests were added to verify hook behavior and deactivation. A review comment suggests replacing an unwrap() call with expect() to provide better error context when parsing activation flags.
Move the bash hook logic out of `bash.rs` and into shell asset files so it is easier to reason about, run through shellcheck, and maintain. Keep `bash.rs` focused on rendering and assembling the generated shell script while preserving the current hook behavior.
c5a66b5 to
6aacfba
Compare
### 🚀 Features - **(config)** report env files in config ls and doctor output by @SamSoldatenko in [#8853](#8853) - add support for token sources in GitLab and Forgejo by @roele in [#8868](#8868) ### 🐛 Bug Fixes - **(aqua)** prevent double .exe extension when Windows override URL already ends in .exe by @yusei-wy in [#8863](#8863) - **(bash)** avoid duplicate trust warning after cd by @timothysparg in [#8920](#8920) - **(env)** prevent config root injection into PATH via _.source by @jdx in [#8936](#8936) - **(install)** suppress spurious dependency warning when tool is configured by @jdx in [#8923](#8923) ### 📚 Documentation - **(node)** add section on pinning npm version by @jdx in [#8925](#8925) - add Windows default paths and mise.toml examples alongside CLI commands by @jdx in [#8926](#8926) - clarify common sources of confusion from GitHub discussions by @jdx in [#8927](#8927) - clarify Python venv mechanisms, JAVA_HOME behavior, and activation performance by @jdx in [#8928](#8928) - add FAQ and troubleshooting entries based on common Discord questions by @jdx in [#8930](#8930) ### New Contributors - @SamSoldatenko made their first contribution in [#8853](#8853) - @yusei-wy made their first contribution in [#8863](#8863)
Upstream release notes: https://github.com/jdx/mise/releases/tag/v2026.4.5 Changes that matter for this repo: - **Spurious dependency warnings during install are suppressed** (jdx/mise#8923). This repo's mise.toml and .devcontainer/mise-system.toml both configure a language runtime plus packages from that ecosystem: - `node` + `npm:@google/gemini-cli`, `npm:@openai/codex`, `npm:agnix`, `npm:agents-lint` - `rust` + `cargo-binstall` v2026.4.4 warned that npm/cargo were "missing" during version resolution even though node/rust were configured and would be installed first. The warnings were cosmetic but noisy in CI build logs and local `mise run` output. v2026.4.5 suppresses the warning when the providing tool is present in the toolset. - **Bash duplicate trust warning fixed** (jdx/mise#8920). Entering an untrusted project directory in bash previously printed the trust warning twice per `cd` (once from the `chpwd` hook, once from `PROMPT_COMMAND`). Fixed in v2026.4.5. Directly improves the devcontainer bash experience. Changes NOT relevant to this repo (documented for future reference): - GitLab/Forgejo token support (this repo uses GitHub only). - `mise github token` renamed to `mise token github` (we don't invoke it). - Windows aqua backend `.exe` double-suffix fix (we're linux). - `_.source` PATH root injection fix (verified via grep: we don't use `_.source` in any mise config). - Env files in `mise config ls` / `mise doctor` (we don't use `MISE_ENV_FILE` or `_.file` directives today). Files touched: - docker-bake.hcl: `MISE_VERSION` variable default → v2026.4.5 - .devcontainer/Dockerfile: `ARG MISE_VERSION` → v2026.4.5 Both references must agree at every commit (bake passes MISE_VERSION to Dockerfile via ARG, so drift would manifest as a build cache miss + a downloaded mise version ≠ the Dockerfile's pinned version). Verification (CI-only per feedback_base_image_ci_only.md): - `HK_PKL_BACKEND=pkl hk run pre-commit --all --stash none` → 0 - `grep -n 'v2026\.4\.' docker-bake.hcl .devcontainer/Dockerfile` → both at v2026.4.5 - CI will download and cache v2026.4.5 on the next base-image rebuild; CI smoke-test validates end-to-end after merge. PR scope: commit 4 of PR-1 (base-image-cookbook-and-unblock). Follows commit 3 (cookbook refactor) so the version bump is a clean, separable bisect point. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When entering an untrusted project in bash, users currently see the same
mise trustwarningtwice for a single
cd.The demo shows the duplicate warning, then patches the hooks in-shell and shows the warning
appearing once.
This keeps
PROMPT_COMMANDpersistent in bash while coordinating it withchpwd, and preservessupport for both string and array
PROMPT_COMMANDvalues.