-
Notifications
You must be signed in to change notification settings - Fork 2.3k
prefer users SHELL #4702
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
prefer users SHELL #4702
Conversation
|
We could totally do this, but I could see some downsides. The model might expect to be using bash specifically, unless we update system prompts/tool descriptions. So if it isn't instructed to do so specifically, it could generate invalid shell commands thinking it's bash |
|
@jamadeo oh... hrm... that is a great point. zsh is kind of a superset of bash so I expect would work the same. Conversely, bash on macos is getting old (bash 3.2 when linux is > 4) and is lagging modern bash so it could go the other way where it will try to do things that won't work (especially if it ends up deprecated), not that I Have seen that. so if zsh is a reasonable updated shell, then probably better to do this (but then also is a change). If it is not, probably better to stay with bash. Zsh is a posix shell which I would think LLMs are mostly trained with (due to linux). We don't prompt either way I believe, so the LLM will just see "shell" and think whatever is most idiomatic on that platform, which could go either way I guess (and probably drift over time). |
|
I looked at how claude code works, and while its tool is always called bash, it does use zsh on my system. Presumably it's using $SHELL then? So, yeah, let's go for it. Maybe though, should we tell the model in the tool description which shell executable it will use? I suppose if it wants to it can always specify by running |
|
I think it is enough to say it says to use the system SHELL - it should be able to work that out. |
|
we could still inject it into the system prompt, yeah? |
|
yeah - not sure if it is necessary, as it can work it out itself as it goes if it needs to? |
Co-authored-by: Jack Amadeo <[email protected]>
…ovements * 'main' of github.com:block/goose: (23 commits) blog post on subagents vs subrecipes (#4829) fix chat button alignment and spacing for attachments (#4794) fix: remove nested double quotes in windows automation_script tool description (#4824) fix: a few things with the mcp snapshot test (#4818) Revert "fix(compaction): try to catch more context limit exceeded erors and compact" (#4820) test: add test coverage for Tools Inspector (#4700) feat: Parse and use retryDelay from Google API RateLimitExceeded errors (#4124) cleanup: remove unused link preview and goose response form components (#4795) fix build: latest bedrock version (#4812) prefer users SHELL (#4702) feat: update aws-sdk-bedrockruntime to enable AWS_BEARER_TOKEN_BEDROCK auth (#4327) correct the tests from an odd merge (#4804) docs: import yaml recipe (#4799) docs: Add openmetadata extension to goose mcp docs (#4547) Add elapsed time to the CLI output. (#4609) fix: Fix cell coordinate ordering in XlsxTool and add unit tests (#4551) Use gemini flash for summarization on open router (#4290) chore(deps): bump xcb from 1.5.0 to 1.6.0 (#4289) feat(shell): throw errors on interactive commands (#4788) feat: AgentManager - foundation for unified execution (#4389) (#4684) ...
Co-authored-by: Jack Amadeo <[email protected]> Signed-off-by: HikaruEgashira <[email protected]>
This is one option - use the $SHELL value and delegate to that. That allows it to load appropriately if zsh etc, fall back to bash