fix(developer): run shell tool under bash/sh regardless of login shell#8658
fix(developer): run shell tool under bash/sh regardless of login shell#8658jh-block wants to merge 1 commit into
Conversation
Before, the developer extension's shell tool ran commands under the user's $SHELL when /bin/bash wasn't present. Users with a non-POSIX login shell (fish, csh, tcsh, nu, ...) would then see failures whenever the model emitted common shell idioms like heredoc redirection (`cat <<EOF > file`), since those shells don't support that syntax. Now we always run commands under bash (when it's on PATH) or fall back to sh. GOOSE_SHELL still overrides. The tool description also tells the model which shell it's writing for, so it can tailor syntax (and so GOOSE_SHELL users get the dialect they asked for). Along the way, simplify the shell-selection logic: use `which` to probe for bash instead of hard-coded paths, drop the redundant Flatpak short-circuit (the `flatpak-spawn --host` wrapping is still there), always store shells as basenames so PATH resolution does the work, and factor the Windows/Unix basename handling behind a shared helper. Fixes #7348 Signed-off-by: jh-block <jhugo@block.xyz>
|
superseded by #8659 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2a0832d20
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if which::which("bash").is_ok() { | ||
| "bash".to_string() | ||
| } else { | ||
| std::path::Path::new(shell) | ||
| .file_name() | ||
| .and_then(|s| s.to_str()) | ||
| .unwrap_or("bash") | ||
| "sh".to_string() |
There was a problem hiding this comment.
Returning bare command names ("bash"/"sh") here introduces a bootstrap failure when Goose starts with a restricted PATH that does not include the shell binary locations (common in GUI/service launches): resolve_login_shell_path() calls Command::new(&shell) before the login-path recovery has run, so it can no longer start the shell to recover PATH, and later shell execution fails for the same reason. The previous absolute /bin/bash fallback avoided this class of failure. Consider keeping an absolute fallback (or storing which::which("bash")’s resolved path) for non-Flatpak execution.
Useful? React with 👍 / 👎.
1 participant
Problem
The developer extension's
shelltool previously fell back to the user's$SHELLwhen/bin/bashwasn't present. Users with a non-POSIX login shell (fish, csh, tcsh, nu, ...) would then see failures whenever the model emitted common idioms like heredoc redirection (cat <<EOF > file), which those shells don't support.Fixes #7348.
Fix
bash(when on PATH) orsh;GOOSE_SHELLstill overrides.shelltool description so the model tailors syntax accordingly (andGOOSE_SHELLusers get the dialect they asked for).whichto probe for bash instead of hard-coded paths, drop a redundant Flatpak short-circuit (theflatpak-spawn --hostwrapping is unchanged), store shells as basenames so PATH resolution does the work, and factor Windows/Unix basename handling behind a shared helper.