docs: document auto-injected GOOSE_SHELL flags - #10759
Conversation
The shell flags goose injects based on shell type (-NoProfile -NonInteractive -Command for PowerShell, /C for cmd, -c for POSIX shells) were undocumented. A PR reviewer noted they searched the docs expecting to add these flags manually, only to find they are injected automatically. - Fix a misplaced doc comment on windows_shell() that described Unix shell behavior (Flatpak, basename auto-detection) instead of Windows - Add a doc comment to build_shell_command() documenting the automatic flag mapping for PowerShell/cmd/POSIX shells - Add a user-facing note in the environment-variables guide Co-Authored-By: goose <opensource@block.xyz>
![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: f4562b8f9d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| :::note | ||
| You only ever set `GOOSE_SHELL` to a shell executable path or name. goose injects the command-line flags automatically based on the shell, so there is no need to add them yourself: | ||
|
|
||
| - **PowerShell** (`pwsh`, `powershell`) → `-NoProfile -NonInteractive -Command` |
There was a problem hiding this comment.
When a Unix user sets GOOSE_SHELL=pwsh, the #[cfg(not(windows))] branch does not perform this PowerShell-specific mapping: unix_shell_command_args supplies only -c, leaving profiles enabled and omitting -NonInteractive. This note therefore gives cross-platform PowerShell users an incorrect account of the launched process; scope these three mappings to Windows and clarify that all Unix overrides receive -c (and make the same correction in the new Rustdoc).
Useful? React with 👍 / 👎.
| ``` | ||
|
|
||
| :::note | ||
| You only ever set `GOOSE_SHELL` to a shell executable path or name. goose injects the command-line flags automatically based on the shell, so there is no need to add them yourself: |
There was a problem hiding this comment.
Saying there is no need to add them yourself seems odd when we document how to do so multiple times in this doc seems a bit odd to me...
We should take a stance on whether we document how to use this env var or keep it internal to goose and remove all mentions in docs.
Thoughts?
There was a problem hiding this comment.
Fair point — the note is awkwardly worded. It's telling users not to do something the examples never suggested, so I'll rephrase it to state the behavior factually instead.
On the broader question: I'd keep GOOSE_SHELL documented. It's a real config knob (force zsh, Cygwin bash on Windows, custom pwsh), and hiding it would hurt discoverability. The flag-mapping detail is better served by the build_shell_command rustdoc this PR already adds, so I'll trim the user-facing note to a one-liner and avoid duplicating the verbose list in both places.
* origin/main: (58 commits) Handle MCP tool list change notifications fix: restore final_output_tool when agent is recreated after LRU eviction (#10793) feat(ui): bring back make sidebar resizable with drag handle and persisted width (#10795) docs: document auto-injected GOOSE_SHELL flags (#10759) feat: surface output-token-limit info (#10831) fix(desktop): avoid O(n²) cloning during session load (#10665) feat: add interactive menu for single select elicitations (#10327) fix(acp): prefix child stderr log target so it passes the default goose=info filter (#10901) chore (codex-acp): migrate to @agentclientprotocol/codex-acp (#10923) style: add dark mode inline code styling for message bubbles (#10861) docs: clarify macOS sandbox feature was removed in post-v1.25.0 cleanup (#10900) fall back to static model list only for non-models payloads (#10189) Remove CLI project support (#10838) fix(anthropic): add claude-sonnet-5 and claude-fable-5 to known models list (#10865) fix: stdio extensions silently skipped when name missing or env: used in config (#10773) feat(desktop): show session metadata on sidebar chat hover (#10663) fix(serve): start scheduler at startup instead of first ACP connection (#10766) fix: respect disabled builtin extensions (developer) at session start (#10223) fix: parse PyPI requirements for OSV checks (#10510) fix(telegram): contain voice file extensions (#10456) ... # Conflicts: # Cargo.lock # crates/goose/src/agents/extension_manager.rs # crates/goose/tests/mcp_replays/npx-y@modelcontextprotocol_server-everything@2026.1.14
2 participants
Summary
build_shell_commandbased on shell type, but this was undocumented — a reviewer noted they searched the docs expecting to add these flags manually, only to find they're injected automatically (feat: add GOOSE_SHELL env var to configure preferred shell #7909 (comment)).windows_shell()that incorrectly described Unix behavior (Flatpak, basename auto-detection) instead of Windows.build_shell_command()documenting the automatic flag mapping: PowerShell →-NoProfile -NonInteractive -Command, cmd →/C, POSIX shells →-c.:::notein the environment-variables guide so the behavior is discoverable where users setGOOSE_SHELL.Review Notes
cargo fmtclean, clippy shows only pre-existing unrelated warnings, 21/21 shell module tests pass.Decision Log
Hardest decision: Whether to fix the misplaced
windows_shell()doc comment in the same PR or leave it. It was describing Unix behavior (Flatpak, PATH basename resolution) on a#[cfg(windows)]function — clearly a copy-paste fromunix_shell(). Since the whole point of this PR is accurateGOOSE_SHELLdocs, leaving a wrong doc comment right next to the new one would undermine it, so I fixed it.Alternatives rejected:
environment-variables.md): rejected becausecargo doc/ IDE users readingbuild_shell_commandwould still hit the same dead end the reviewer did. Added the rustdoc too.windows_shell,shell_basename): rejected as duplication; the singlebuild_shell_commanddoc is the canonical source and the others point to it.Least confident about: The wording "POSIX shells (bash, zsh, … via Cygwin/MSYS2) →
-c". The Windows catch-all_arm matches any non-PowerShell/non-cmd shell, so something exotic (fish, csh) on Windows would also get-c, which is correct for bash/zsh but not for those. The original code already had this behavior unchanged by this PR; I documented the common cases rather than over-promising on the catch-all.Test plan
cargo doc -p goose --librenders the new doc comments without warnings:::noteadmonition correctly