Skip to content

Conversation

@sheikhlimon
Copy link
Contributor

@sheikhlimon sheikhlimon commented Dec 2, 2025

Summary

Issue: mcp-hermit cleanup fails with "No such file or directory" error - this was already fixed recently by replacing tilde with ${HOME} variable

Solution: Add proper quoting to the cleanup marker path to prevent shell expansion issues and ensure robust path handling

The root cause was already addressed in a recent PR, but this adds additional safety with proper quoting around path variables.

Type of Change

  • Feature
  • Bug fix
  • Refactor / Code quality
  • Performance improvement
  • Documentation
  • Tests
  • Security fix
  • Build / Release
  • Other (specify below)

AI Assistance

  • This PR was created or reviewed with AI assistance

Related Issues

Relates to #5944

@sheikhlimon sheikhlimon marked this pull request as draft December 2, 2025 20:36
@sheikhlimon
Copy link
Contributor Author

sheikhlimon commented Dec 2, 2025

@The-Best-Codes I just noticed the previous PR addressing this. But shouldn't quotes be the standard approach?

Edit: #5868 I guess this was a linux and ui problem since it's placed on ui/desktop /src/bin, not just fedora... the main issue was probably the ~, not the quotes. I don't have the whole picture but cleanup logic could be moved to /scripts/mcp-hermit-cleanup.sh for using broadly. Using consistent quotes for the whole script probably would be more robust.

@taniandjerry
Copy link
Contributor

Let me tag @block/goose-core-maintainers as well!

@The-Best-Codes
Copy link
Collaborator

The-Best-Codes commented Dec 3, 2025

@sheikhlimon The issue was with ~ expansion inside quotes in various shells, to err on the side of caution PR #5868 removes quotes and uses the ${HOME} variable. I don't think adding the quotes back is a good idea, but I'd be happy to hear why if you think it is! 😀

@sheikhlimon
Copy link
Contributor Author

Thanks for the clarification! Yes, using ${HOME} instead of ~ absolutely fixes the original issue. ~ does not expand inside quotes in POSIX shells, so switching to ${HOME} was the right call. 👍

My point about quotes was more about general shell safety rather than the tilde issue.
Even after fixing ~ -> ${HOME}, quoting variables is still considered best practice because:
Paths under $HOME can legally contain spaces (macOS is especially common).
Without quotes, something like:

cd ${HOME}/.config/goose/mcp-hermit

would turn into multiple arguments if the user’s home directory contains spaces.

Even if we expect users not to have unusual paths, quoting ensures consistent behavior across shells and environments.
So the corrected form:

cd "${HOME}/.config/goose/mcp-hermit"

is both tilde-safe and robust against those other issues.

I completely understand the fix in #5868, removing quotes was a fast way to eliminate the tilde-expansion bug. However, now that ${HOME} is being used everywhere, adding quotes back shouldn’t reintroduce the original problem and would make the script more defensive and portable overall. 😄

@The-Best-Codes
Copy link
Collaborator

The-Best-Codes commented Dec 3, 2025

@sheikhlimon Hmm, I'm still just not seeing the need for quotes. A home directory with spaces is pretty rare, and that's basically the only thing that would be user-controlled in this script. The rest of the commands involving paths are hard-coded and don't contain spaces.
If you want, though, I do see some opportunity for standardization and consistency improvements in the script. You could migrate all the commands involving a path to be wrapped in quotes (I don't think it will hurt anything at least), and maybe standardize all the environment variables to be ${VAR} instead of $VAR (for example, the CLEANUP_MARKER var could be updated to use that pattern). Let me know what you think!

@sheikhlimon
Copy link
Contributor Author

Good point. The rest of the paths are hard-coded, but quoting is still recommended because ${HOME} itself is user-controlled and the entire expansion is subject to word splitting or globbing if it is unquoted. Even without spaces, characters like (, ), *, ?, or brackets in a home directory can change how the shell interprets commands like:

cd ${HOME}/...
mkdir -p ${HOME}/...
cp ${HOME}/...

These cases are uncommon, but quoting removes that class of issues completely and matches what the standard shell style guides recommend: always quote variable expansions unless you specifically want word splitting. This script never relies on splitting, so quoting makes it more consistent and predictable.

I am happy to update it for consistency by quoting all path-related expansions and using the ${VAR} form if that sounds good.

@The-Best-Codes
Copy link
Collaborator

Yeah that sounds good!

@sheikhlimon sheikhlimon force-pushed the fix/mcp-hermit-cleanup-path-expansion branch from c41b117 to bea7639 Compare December 3, 2025 20:57
@sheikhlimon sheikhlimon marked this pull request as ready for review December 3, 2025 21:01
@sheikhlimon sheikhlimon force-pushed the fix/mcp-hermit-cleanup-path-expansion branch from bea7639 to 191097b Compare December 3, 2025 21:17
Copy link
Collaborator

@The-Best-Codes The-Best-Codes left a comment

Choose a reason for hiding this comment

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

All looks good to me, I haven't tested it locally yet though

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.

3 participants