fix: shell ACP providers on desktop - #10907
Conversation
![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: bcf0e601fe
ℹ️ 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".
|
The auto codex review comment seems correct, this won't propagate PATH down which will matter for e.g. Beyond that, |
|
I think we might be better to find the login shell PATH (ensuring that we use the user's actual login shell) once (in TS) at startup of the desktop app and set the PATH environment variable that we run |
bcf0e60 to
bd57713
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd5771309a
ℹ️ 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".
bd57713 to
62e5b58
Compare
jbg
left a comment
jbg
left a comment
There was a problem hiding this comment.
a few comments, looks good to me once they are resolved (or declared invalid!)
also acpConnection.ts is still sending goose/useLoginShellPath: true which I think is probably no longer needed? (duplicates the effect of this)
| return Promise.resolve(null); | ||
| } | ||
|
|
||
| const shell = process.env.GOOSE_SHELL || process.env.SHELL || 'bash'; |
There was a problem hiding this comment.
I think this shouldn't use GOOSE_SHELL, which is an override for the Developer extension.
| * launchd, so goosed can't find CLI-backed providers (claude, etc.). Sourcing | ||
| * the user's profile via a login+interactive shell recovers the real PATH. | ||
| * Doing this here rather than in goosed keeps the plain `goose` CLI on the | ||
| * ambient PATH. Returns null on Windows, timeout, or any failure. |
There was a problem hiding this comment.
I think maybe this should return null on Linux too? I don't actually know whether any Linux desktop environment scrubs the environment of apps like macOS does, but I would think they probably run with a login shell's environment? I don't have easy access to a suitable machine to test this, but the conservative approach would probably be to make this macOS-only for now.
| ...process.env, | ||
| HOME: homeDir, | ||
| [pathKey]: `${path.dirname(binaryPath)}${path.delimiter}${currentPath}`, | ||
| [pathKey]: [path.dirname(binaryPath), currentPath, loginShellPath] |
There was a problem hiding this comment.
The comment above says that loginShellPath gets prepended, but this seems to be appending
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ef1cdc034
ℹ️ 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".
| }); | ||
|
|
||
| const timer = setTimeout(() => { | ||
| child.kill(); |
There was a problem hiding this comment.
When a macOS user's interactive login shell or startup script hangs long enough to hit this timeout, child.kill() sends the default SIGTERM only to the shell process. Interactive bash shells ignore SIGTERM by default (Bash manual), and any commands started from rc files are in the detached child's process group, so the resolver can return null while the stuck shell/process tree keeps running and holding its pipe; repeated desktop starts can leave orphaned shell readers around. Kill the detached process group, or use a non-ignorable fallback, before resolving the timeout.
Useful? React with 👍 / 👎.
* main: (32 commits) fix: keep turn-context in place for OpenAI Responses-stack models so prompt caching works (#10993) fix(developer): byte-bound the shell truncation preview (#10992) fix(openrouter): stop silently ignoring thinking effort off (#10991) fix: dispatch edited queued messages (#10933) fix: contain recipe template paths (#10930) fix: make shell approval titles faithful (#10986) fix: block MCP app form submissions (#10985) fix: migrate desktop routing to React Router 8.3.0 (#10971) fix: sanitize Bedrock tool errors (#10934) fix implicit OAuth during model discovery (#10929) fix: update React Router to 7.18.2 (#10967) test: early-exit code-exec smoke tests once tool invocation is observed (#10954) fix: keep ACP session naming out of live conversations (#10963) Bind MCP apps to trusted ownership metadata (#10747) tests: add recursion_limit attribute to remaining ACP test files (#10559) Sanitize Unicode tags in MCP resources (#10746) fix(oauth): preserve RFC 9207 iss from MCP OAuth callback (#10678) feat(installer): detect Termux and select musl portable build (#10568) feat: add Celeris provider (#10714) fix: shell ACP providers on desktop (#10907) ...
* main: (101 commits) fix: keep turn-context in place for OpenAI Responses-stack models so prompt caching works (#10993) fix(developer): byte-bound the shell truncation preview (#10992) fix(openrouter): stop silently ignoring thinking effort off (#10991) fix: dispatch edited queued messages (#10933) fix: contain recipe template paths (#10930) fix: make shell approval titles faithful (#10986) fix: block MCP app form submissions (#10985) fix: migrate desktop routing to React Router 8.3.0 (#10971) fix: sanitize Bedrock tool errors (#10934) fix implicit OAuth during model discovery (#10929) fix: update React Router to 7.18.2 (#10967) test: early-exit code-exec smoke tests once tool invocation is observed (#10954) fix: keep ACP session naming out of live conversations (#10963) Bind MCP apps to trusted ownership metadata (#10747) tests: add recursion_limit attribute to remaining ACP test files (#10559) Sanitize Unicode tags in MCP resources (#10746) fix(oauth): preserve RFC 9207 iss from MCP OAuth callback (#10678) feat(installer): detect Termux and select musl portable build (#10568) feat: add Celeris provider (#10714) fix: shell ACP providers on desktop (#10907) ...
2 participants
Summary
When goose runs as a desktop app, Electron on macOS launches goosed with a minimal PATH inherited from launchd, so CLI-backed providers like claude-code can't find their binary (SearchPaths fails to resolve claude), the provider never gets built, and the agent reports "Provider not set."
Testing
Manual usage
Related Issues
Found this bug when working to resolve #10807
Screenshots/Demos (for UX changes)
Before:
After:
