fix: remove setx calls to not permanently edit the windows shell PATH #5821
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the persistPathForUser function that was permanently modifying the Windows user's PATH environment variable using setx commands. The change ensures that PATH modifications only affect the Goose application and its child processes, not the user's persistent shell environment.
Key changes:
- Removed the
persistPathForUserfunction and its PowerShell script that usedsetx - Removed the unused
spawnimport fromchild_process - Added
uv.exeto the list of shims to copy - Updated comments to clarify that PATH changes are temporary and process-scoped only
| ); | ||
|
|
||
| // Prepend to PATH **for this process & all children**. | ||
| // Make sure our bin directory is at the VERY BEGINNING of PATH |
There was a problem hiding this comment.
This comment explains it - it injects at the beginning of PATH and as such is the first valid command, as PATH search path is executed from left to right. If it would just append at the end it wouldn't make such problem, but not changing PATH environment variable w/o user consent would be even greater.
There was a problem hiding this comment.
yep, thanks for bringing it up @klonuo (and goose made it clear it was bad, and even when I switched models a few times, as I didn't believe that it would do that!). Its been decades since I was a windows dev and the registry gives me chills (not the good kind).
| } | ||
| } | ||
|
|
||
| // Optional: Persist PATH for user's external PowerShell/CMD sessions |
There was a problem hiding this comment.
The "Optional" here says it all: this was probably inserted by llm and barely glanced at. We should definitely not be adding these to the user's PATH.
* main: (48 commits) [fix] generic check for gemini compat (#5842) Add scheduler to diagnostics (#5849) Cors and token (#5850) fix sessions coming back with empty messages (#5841) markdown export from URL (#5830) Next camp refactor live (#5706) Add out of context compaction test via error proxy (#5805) fix: Add backward compatibility for conversationCompacted message type (#5819) Add /agent/stop endpoint, make max active agents configurable (#5826) Handle 404s (#5791) Persist provider name and model config in the session (#5419) Comment out the flaky mcp callers (#5827) Slash commands (#5718) fix: remove setx calls to not permanently edit the windows shell PATH (#5821) fix: Parse maas models for gcp vertex provider (#5816) fix: support Gemini 3's thought signatures (#5806) chore: Add Adrian Cole to Maintainers (#5815) [MCP-UI] Proxy and Better Message Handling (#5487) Release 1.15.0 Document New Window menu in macOS dock (#5811) ...
…block#5821) Signed-off-by: Blair Allan <[email protected]>
fixes: #5777
setxseems like it is not a great thing to call, it should not mess with users env, even if it limits what it can do.@jamadeo not sure who is best equipped with windows to think on this, but it seems like we shoulen't be messing with PATH that exists outside of goose.