Skip to content

feat(core): pick the right shell for a command instead of hardcoding cmd /C - #3618

Closed
rhishi99 wants to merge 1 commit into
rtk-ai:developfrom
rhishi99:feat/windows-shell-selection
Closed

rhishi99 wants to merge 1 commit into
rtk-ai:developfrom
rhishi99:feat/windows-shell-selection

Conversation

@rhishi99

Copy link
Copy Markdown
Contributor

Problem

rtk err, rtk test and rtk summary take a command as one string and run it through a shell. On Windows that shell was hardcoded to cmd /C — in two separate copies of the same helper (cmds/rust/runner.rs, cmds/system/summary.rs). Every PowerShell cmdlet or pipeline handed to them fails:

> rtk summary "Get-ChildItem | Measure-Object"
'Get-ChildItem' is not recognized as an internal or external command

PowerShell is the default shell on Windows, so this is the common case, not an edge case.

Fix

Both copies collapse into one core::shell module which resolves the shell rather than assuming it:

  1. RTK_SHELL wins when set — cmd, powershell, pwsh, sh, bash, or any executable name or path.
  2. On Windows, if the command's leading word is neither a cmd builtin nor a program on PATH, it is PowerShell syntax → route to pwsh if installed, else powershell.
  3. Otherwise the platform default: cmd on Windows, sh elsewhere.
rtk summary "dir /b"                          # cmd.exe
rtk summary "Get-ChildItem | Measure-Object"  # PowerShell
$env:RTK_SHELL = 'pwsh'                       # pin it explicitly

Non-Windows behaviour is unchanged — without RTK_SHELL it is still sh -c, exactly as before.

Why not detect the parent shell

That is the obvious approach and it does not work here:

  • Walking parent processes needs Win32 calls this crate forbids (unsafe_code = "deny").
  • The environment carries no usable signal — PSModulePath is inherited by cmd children, so its presence proves nothing.
  • Parsing cmd.exe's is not recognized as an internal or external command is locale-dependent.

Deciding from the command text needs no parent process, costs one PATH lookup, and is locale-independent. cmd builtins are listed explicitly because no file on PATH backs them, so a PATH lookup alone would misread dir or echo as PowerShell.

Testing

Unit tests in core::shell cover RTK_SHELL precedence, builtin detection, PATH-program detection, the PowerShell fallback, quoted leading words, and the empty-command case. Docs added to docs/guide/getting-started/configuration.md.

Full suite on x86_64-pc-windows-gnu: 2626 passed. cargo fmt --check and cargo clippy --all-targets clean. The single unrelated failure on this toolchain is fixed in #3615.

🤖 Generated with Claude Code

…cmd /C

`rtk err`, `rtk test` and `rtk summary` take a command as one string and
run it through a shell. On Windows that shell was hardcoded to `cmd /C`,
in two separate copies of the same helper, so every PowerShell cmdlet and
pipeline handed to them failed:

    rtk summary "Get-ChildItem | Measure-Object"
    'Get-ChildItem' is not recognized as an internal or external command

Consolidate both copies into `core::shell` and resolve the shell instead:

1. `RTK_SHELL` wins when set (`cmd`, `powershell`, `pwsh`, `sh`, `bash`,
   or any executable name or path).
2. On Windows, if the command's leading word is neither a `cmd` builtin
   nor a program on PATH, it is PowerShell syntax -- route it to `pwsh`
   if installed, else `powershell`.
3. Otherwise the platform default: `cmd` on Windows, `sh` elsewhere.

Non-Windows behaviour is unchanged: without `RTK_SHELL` it is still
`sh -c`, same as before.

Step 2 is deliberately not "detect the parent shell". That needs Win32
calls the crate forbids (`unsafe_code = "deny"`), and the environment
carries no usable signal -- `PSModulePath` is inherited by `cmd`
children, so it proves nothing. Reading the command text costs one PATH
lookup and is not locale-dependent the way parsing cmd.exe's error
message would be.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@CLAassistant

CLAassistant commented Aug 20, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@KuSh

KuSh commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

Thanks for working on this, rhishi99. #4125 (merged 2026-09-26) replaced the shell handling in rtk err, test and summary: positional commands now run directly, and a script goes through an explicit --shell, which accepts pwsh and powershell with -Command from a shared src/core/shell.rs. The duplicated cmd /C helpers this PR merges into one are already gone on develop. What develop still lacks is a way to reach PowerShell without --shell, such as an RTK_SHELL default or detection of PowerShell syntax; if you still want that, an issue or a narrow PR on the --shell model would be welcome. If I missed something this PR covers, please comment or reopen.

@KuSh KuSh closed this Oct 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants