-
Notifications
You must be signed in to change notification settings - Fork 14
Use SHELL environment variable before falling back to /bin/bash #81
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: swissspidy <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
/gemini review |
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.
Code Review
This pull request correctly implements the fallback to the SHELL environment variable before using the hardcoded /bin/bash path. The changes are logical and the new test case in features/shell.feature provides good coverage for the new behavior. I have one minor suggestion in src/WP_CLI/Shell/REPL.php to make the code more concise and efficient.
| if ( getenv( 'WP_CLI_CUSTOM_SHELL' ) ) { | ||
| $shell_binary = getenv( 'WP_CLI_CUSTOM_SHELL' ); | ||
| } elseif ( getenv( 'SHELL' ) ) { | ||
| $shell_binary = getenv( 'SHELL' ); | ||
| } else { | ||
| $shell_binary = '/bin/bash'; | ||
| } |
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.
This if/elseif/else block can be simplified and made more efficient by avoiding repeated calls to getenv(). You can use the ternary operator shorthand (?:) to achieve the same logic in a single, more concise line. This avoids calling getenv() multiple times for the same variable.
$shell_binary = getenv( 'WP_CLI_CUSTOM_SHELL' ) ?: getenv( 'SHELL' ) ?: '/bin/bash';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.
Pull request overview
Adjusts wp shell’s shell binary selection to work on systems where bash isn’t located at /bin/bash (e.g. NixOS), by considering environment variables before falling back.
Changes:
- Update REPL shell selection to prefer
WP_CLI_CUSTOM_SHELL, thenSHELL, then/bin/bash. - Add a Behat scenario intended to verify
SHELLis respected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/WP_CLI/Shell/REPL.php |
Changes shell binary detection order to include $SHELL before /bin/bash. |
features/shell.feature |
Adds a feature scenario covering $SHELL behavior and invalid $SHELL erroring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( getenv( 'WP_CLI_CUSTOM_SHELL' ) ) { | ||
| $shell_binary = getenv( 'WP_CLI_CUSTOM_SHELL' ); | ||
| } elseif ( getenv( 'SHELL' ) ) { | ||
| $shell_binary = getenv( 'SHELL' ); | ||
| } else { | ||
| $shell_binary = '/bin/bash'; |
Copilot
AI
Feb 2, 2026
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.
Using the SHELL environment variable as the default shell can break wp shell for users whose login shell is not bash (e.g. zsh/fish). create_prompt_cmd() builds a bash-specific command (history -r/-s/-w, read -re), so selecting a non-bash SHELL will pass the is_file/is_readable check but then fail at runtime. Consider preferring a known bash binary first (e.g. /bin/bash when it exists), and only falling back to SHELL when /bin/bash is unavailable or when SHELL points to a bash binary (e.g. basename is bash), or probe for bash on PATH.
| Scenario: Use SHELL environment variable as fallback | ||
| Given a WP install | ||
|
|
||
| And a session file: | ||
| """ | ||
| return true; | ||
| """ | ||
|
|
||
| When I try `SHELL=/bin/bash wp shell --basic < session` | ||
| Then STDOUT should contain: | ||
| """ | ||
| bool(true) | ||
| """ | ||
| And STDERR should be empty |
Copilot
AI
Feb 2, 2026
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.
This scenario doesn't clearly validate the new behavior of preferring $SHELL, because setting SHELL=/bin/bash matches the existing hardcoded fallback and would succeed even without the code change. Consider asserting a bash path that differs from /bin/bash (e.g. via a resolved bash path) or otherwise structuring the test so it fails on the old behavior but passes on the new one.
| Then STDOUT should be empty | ||
| And STDERR should contain: | ||
| """ | ||
| Error: The shell binary '/nonsense/path' is not valid. | ||
| """ |
Copilot
AI
Feb 2, 2026
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.
With the new precedence order, an invalid SHELL now causes wp shell to error even if /bin/bash is present and usable (previous behavior would have ignored SHELL and worked). The second half of this scenario locks in that behavior; please confirm this breaking change is intended. If not intended, adjust the selection logic to fall back to /bin/bash when SHELL is unset/invalid/non-bash, and update the test accordingly.
| Then STDOUT should be empty | |
| And STDERR should contain: | |
| """ | |
| Error: The shell binary '/nonsense/path' is not valid. | |
| """ | |
| Then STDOUT should contain: | |
| """ | |
| bool(true) | |
| """ | |
| And STDERR should be empty |
On systems like NixOS, bash is not located at
/bin/bash. Thewp shellcommand was hardcoded to this path, causing failures despite the shell being available at$SHELL.Changes
Shell detection priority in
REPL.php:WP_CLI_CUSTOM_SHELL(explicit override)SHELL(system default) ← new/bin/bash(fallback)Test coverage: Added scenario verifying
SHELLenvironment variable is respectedAligns with how
wp --infodetects the shell.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.