Skip to content

fix: properly quote exe in bash activate#7070

Closed
agriffis wants to merge 1 commit into
jdx:mainfrom
agriffis:fix/bash-exe-quoting
Closed

fix: properly quote exe in bash activate#7070
agriffis wants to merge 1 commit into
jdx:mainfrom
agriffis:fix/bash-exe-quoting

Conversation

@agriffis

@agriffis agriffis commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

Blindly wrapping an interpolated value in quotes is an anti-pattern. Properly quote the value prior to interpolation, instead.

Replaces commit 4a9ec4e from #7002


Note

Escapes the interpolated exe for bash activation and updates command invocations to use the escaped value without extra quotes.

  • Shell (bash):
    • Escape exe via shell_escape::unix::escape before interpolation in src/shell/bash.rs.
    • Update generated scripts to call {exe} without surrounding quotes:
      • In mise() function (zero-arg path, eval path, and general command path).
      • In command_not_found_handle hook.

Written by Cursor Bugbot for commit 728e8ae. This will update automatically on new commits. Configure here.

Blindly wrapping an interpolated value in quotes is an anti-pattern.
Properly quote the value prior to interpolation, instead.

Replaces commit 4a9ec4e from jdx#7002
Copilot AI review requested due to automatic review settings November 26, 2025 14:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes shell escaping in bash activation scripts by properly escaping the executable path before interpolation, rather than blindly wrapping it in quotes during interpolation. The change uses shell_escape::unix::escape() to handle special characters in the exe path correctly.

Key changes:

  • Added pre-escaping of the exe variable using shell_escape::unix::escape()
  • Removed hard-coded quotes around all {exe} interpolations in the generated bash script
  • Applied the same escaping pattern already used by set_env() and unset_env() methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jdx

jdx commented Nov 26, 2025

Copy link
Copy Markdown
Owner

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no bugs!


@agriffis

Copy link
Copy Markdown
Contributor Author

@cspotcode Could you check if this alternative implementation also solves the problem for you?

@agriffis

Copy link
Copy Markdown
Contributor Author

@jdx would be good to wait for someone with Windows to confirm. I just saw this while working in this area for a separate reason

@cspotcode

Copy link
Copy Markdown
Contributor

I can't, don't have time. I don't see why it wouldn't work.

A failing test we can try is if the path to mise contains a single-quote in the path, like if someone installs it into a directory named mise'10, since single-quotes are the character we're concerned about; all others are handled.

We can also update the escaping used for other mise activate templates at the same time.

@agriffis

Copy link
Copy Markdown
Contributor Author

I'm confident this handles the single quote. I just don't have any experience in the windows world that inspired the other pr.

@agriffis

agriffis commented Nov 26, 2025

Copy link
Copy Markdown
Contributor Author

We can also update the escaping used for other mise activate templates at the same time.

I looked at this but each shell has its own quoting rules, and they aren't all compatible with shell_escape::unix::escape(), and I don't care enough to dig into them. Especially xonsh and pwsh are unknowns to me, but each is questionable enough that it would make sense to give the appropriate attention by a shell-specific expert.

This PR is just to fix a recently-introduced wart in the bash implementation. I am confident in the correctness of this change for Bash. My reason for pause is that the previous fix was targeted at a Windows-specific problem. But having said that, I'm pretty sure it's fine.

@jdx jdx marked this pull request as draft November 27, 2025 21:26
@jdx jdx closed this Dec 15, 2025
@agriffis

Copy link
Copy Markdown
Contributor Author

@jdx I thought we were still going to merge this. Did you just close it because of apparent staleness?

@agriffis

Copy link
Copy Markdown
Contributor Author

Oh, I see, this was replaced by a357fa9. Got it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants