-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: resolve mcp-hermit cleanup path expansion issue #5953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: resolve mcp-hermit cleanup path expansion issue #5953
Conversation
|
@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 |
|
Let me tag @block/goose-core-maintainers as well! |
|
@sheikhlimon The issue was with |
|
Thanks for the clarification! Yes, using My point about quotes was more about general shell safety rather than the tilde issue. 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. 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 |
|
@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. |
|
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: These cases are uncommon, but quoting removes that class of issues completely and matches what the standard shell style guides recommend: I am happy to update it for consistency by quoting all path-related expansions and using the ${VAR} form if that sounds good. |
|
Yeah that sounds good! |
c41b117 to
bea7639
Compare
Signed-off-by: sheikhlimon <[email protected]>
bea7639 to
191097b
Compare
The-Best-Codes
left a comment
There was a problem hiding this 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
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
AI Assistance
Related Issues
Relates to #5944