-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Don't load user's shell env on app startup #4681
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
Conversation
|
@jamadeo not sure how it worked on your machine yesterday, but trying it packaged today:
Longer: |
|
@jamadeo I think worth continuing this, but still couldn't work out how to make sure goosed and thus developer MCP has a |
|
@jamadeo actually scratch that last bit - I think on main I still see that, so may be my environment if you don't see it? Main thing is that it doesn't load any old variable from config.yaml (in the occasional case you want to set provider + api key as a variable I guess? not sure what edge cases other than that, but probably not much). |
|
@michaelneale I can't reproduce any issue with the working directory (I'm asking the model to run pwd or echo $PWD in shell) For the config, what do we really want to do there? I typically wouldn't expect a desktop app to execute by bash/zsh configs to read their environment. I'd expect to put everything in Goose's config.yaml or UI directly (for secret values e.g.) If we definitely want a way to set environment variables in the Goose process, we could consider having an |
|
@jamadeo yeah - I think its just my environment. On variables - it is pretty common for other systems to have an The example I saw fail was anthropic, which uses /// Get a secret value.
///
/// This will attempt to get the value from:
/// 1. Environment variable with the exact key name
/// 2. System keyring
I'm not sure this will work with environment as there is no environment when run as an .app (but again may be my env! maybe can double check that?) worth checking out. In that specific case, secret store is probably ok, but if it trips up this, could be a ton of other unseen things. |
|
ok after clearing out Application Support/Goose dir, I can run this (and is fast) - however, even with shell commands it doesn't have access to the users shell variables (ie add some variable to .zshrc - then ask goose "tell me the value of $THING" ). I think that may be perhaps the bigger issue than config environment variables? (ie a developer using the developer SDK would have reason to think it would have a similar environment to their default/preferred shell?). It can source it if needed I guess... interestingly in the developer toolkit: // Use bash on Unix/macOS (keep existing behavior)
Self {
executable: "bash".to_string(),
args: vec!["-c".to_string()],
}even if I change that to zsh (hard coded - although it should honour $SHELL ideally), it doesn't automatically source .zshrc (!) when running shell commands, which seems really odd (this wouldn't be noticed when we forcibly source .zshrc so if this is a bug in developer mcp, then it is being hidden and now exposed). In any case, this behaves differently to goose cli (given it starts from a shell of course). Obviously a non issue on linux (and windows) and for non zsh macos users as it was always this way - but a bit of a departure otherwise I think.. interesting.. |
|
I wonder what we want to support here. What are the main use cases for passing env from shell config files? I think for Goose itself, you mainly want to pass vars to either MCP extensions or providers, and both of those have other (better) mechanisms. The developer shell is an interesting use case. I wonder if it's possible for the goose shell tool to start a single login shell using $SHELL, and spawn all other shells from that? Or at least just do this login shell env reading in the developer MCP instead of app startup. That is, if we do want to make this work. |
|
@michaelneale I think BASH_ENV might be the key to all of this. If set, any bash process sources from this file. We can set it to a location in Goose's config dir, letting the user set up their Goose shell env however they want. |
|
Opened #4695 to allow a specific discussion |
|
@jamadeo on mac I think everyone uses zsh these days, would BASH_ENV work for that the same? I'm not sure about changing cli behavior here though. |
| workspace = true | ||
|
|
||
| [dependencies] | ||
| goose = { path = "../goose" } |
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.
hrm - is this an issue adding in the goose dep to mcp?
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.
(feels circular but isn't...)
|
@jamadeo updated this to main - cli works still as expected, that is good to know. Do you think this should use the users default $SHELL and load it appropriately in |
|
@jamadeo ah - I think I learned something, good find on BASH_ENV - yes that is idiomatic way, and I think it is fine for GUI to expect to load it from somewhere like you suggested. I now get there is interactive, login shells, and we don't load either (for GUI) but need an option to load something. Bash doesn't have a standard other than what you set BASH_ENV to be. however zsh, which is now I believe standard on macos, does: My suggestion, not sure if harmful is a PR to this PR: #4702 An additional option, use the built in $SHELL value and delegate to that (so it isn't hard coded to bash). That allows it to load appropriately if zsh etc, fall back to bash. So bash requires that goose location, but zsh DOES read from ~/.zshenv which is the correct and idiomatic place to put non interactive and non login values (which work for GUI and cli the same). Bash will require the goose specific location as you found - just a thought given that zsh is now default on macos, so we don't have to source zshrc (nor should we!) but we can use that one. thouights? |
|
"I wonder if it's possible for the goose shell tool to start a single login shell using $SHELL, and spawn all other shells from that?" - yeah I think it is probably right to not use the login shell, or even interactive, as this is not what it was designed for. I am sure that is possible however, probably probably messy state wise... |
michaelneale
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.
@jamadeo I think this is ok thinking about it. The dependency on goose core crate if it is in issue we can pull out the get_config_dir to somewhere else if the direction of dependencies are wrong, but is likely ok.
main downside of this may be people needing to know to put a .bash_env in the goose config dir (or if using the other PR - .zshenv in their home dir). Would it make sense to have a .goose_env option in home dir or getting weird and kludgy now?
…ovements * 'main' of github.com:block/goose: Remove now unused mcp-server crate (#4773) Release/1.9.0 (#4703) chore(mcp): convert computercontroller server to use the rust sdk (#4772) Docs: Delete sessions from UI and edit has changed (#4785) Don't load user's shell env on app startup (#4681) Docs: Chrome Dev Tools Extension Tutorial (#4783) Add Hacktoberfest 2025 Leaderboard Workflow (#4776) [docs] Add gotoHuman MCP server Tutorial (#4764) fix: adjust nightly builds to first push a tag (#4704) temp file for batch issue creation fix: view can recognise a dir (#4701) goosed standalone works with providers (#4698) Compact session automatically for streaming providers on Context Length Exceeded (#4565) When the developer extension gets a cancellation message, it should kill any running processes that it owns. (#4604) Remove some unused stuff (#4388) Add I Ching MCP to extension catalog (#4525) Offer to summarize or clear conversation when it has gotten too long … (#4688)
…-unification * 'main' of github.com:block/goose: 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) shave and code split (#4630) docs: acp support (#4793) Add Take Action for Hacktoberfest (#4791) Remove now unused mcp-server crate (#4773) Release/1.9.0 (#4703) chore(mcp): convert computercontroller server to use the rust sdk (#4772) Docs: Delete sessions from UI and edit has changed (#4785) Don't load user's shell env on app startup (#4681) Docs: Chrome Dev Tools Extension Tutorial (#4783) Add Hacktoberfest 2025 Leaderboard Workflow (#4776) # Conflicts: # crates/goose-server/src/routes/recipe.rs # ui/desktop/openapi.json # ui/desktop/src/api/types.gen.ts # ui/desktop/src/hooks/useRecipeManager.ts # ui/desktop/src/recipe/index.ts
…se into zane/recipe-param-values-resume * 'zane/create-recipe-unification' of github.com:block/goose: fix recipe issues from upstream changes and regenerate types 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) shave and code split (#4630) docs: acp support (#4793) Add Take Action for Hacktoberfest (#4791) fix recipe instructions from session metadata not being injected Remove now unused mcp-server crate (#4773) Release/1.9.0 (#4703) chore(mcp): convert computercontroller server to use the rust sdk (#4772) Docs: Delete sessions from UI and edit has changed (#4785) Don't load user's shell env on app startup (#4681) Docs: Chrome Dev Tools Extension Tutorial (#4783) Add Hacktoberfest 2025 Leaderboard Workflow (#4776) # Conflicts: # ui/desktop/src/hooks/useAgent.ts # ui/desktop/src/utils/providerUtils.ts
…ose into zane/create-edit-recipe-tests * 'zane/recipe-param-values-resume' of github.com:block/goose: fix recipe issues from upstream changes and regenerate types 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) shave and code split (#4630) docs: acp support (#4793) Add Take Action for Hacktoberfest (#4791) fix recipe instructions from session metadata not being injected Remove now unused mcp-server crate (#4773) Release/1.9.0 (#4703) chore(mcp): convert computercontroller server to use the rust sdk (#4772) Docs: Delete sessions from UI and edit has changed (#4785) Don't load user's shell env on app startup (#4681) Docs: Chrome Dev Tools Extension Tutorial (#4783) Add Hacktoberfest 2025 Leaderboard Workflow (#4776)
Co-authored-by: Michael Neale <[email protected]>
Co-authored-by: Michael Neale <[email protected]> Signed-off-by: HikaruEgashira <[email protected]>
This reverts commit 44115a6.
This speeds up startup quite a lot. I don't think we need to do it (and definitely not three times)