goose icon indicating copy to clipboard operation
goose copied to clipboard

fix: resolve mcp-hermit cleanup path expansion issue

Open sheikhlimon opened this issue 3 weeks ago • 7 comments

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
  • [x] Bug fix
  • [ ] Refactor / Code quality
  • [ ] Performance improvement
  • [ ] Documentation
  • [ ] Tests
  • [ ] Security fix
  • [ ] Build / Release
  • [ ] Other (specify below)

AI Assistance

  • [x] This PR was created or reviewed with AI assistance

Related Issues

Relates to #5944

sheikhlimon avatar Dec 02 '25 20:12 sheikhlimon

@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.

sheikhlimon avatar Dec 02 '25 20:12 sheikhlimon

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

taniandjerry avatar Dec 02 '25 20:12 taniandjerry

@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! :grinning:

The-Best-Codes avatar Dec 03 '25 04:12 The-Best-Codes

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. 😄

sheikhlimon avatar Dec 03 '25 06:12 sheikhlimon

@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!

The-Best-Codes avatar Dec 03 '25 14:12 The-Best-Codes

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.

sheikhlimon avatar Dec 03 '25 19:12 sheikhlimon

Yeah that sounds good!

The-Best-Codes avatar Dec 03 '25 20:12 The-Best-Codes