[codex] refine shell prompt and powershell invocation#2284
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors Windows shell invocation logic, adding the -NonInteractive flag to PowerShell commands, simplifying shell_program_stem to handle full paths (such as cmd.exe), and adding corresponding tests. However, the changes also remove the fallback to Windows PowerShell (powershell.exe) when PowerShell Core (pwsh.exe) is missing, which forces standard Windows environments to fall back directly to cmd.exe. The reviewer recommends restoring the powershell_on_path field, its detection, the fallback logic, and the accidentally deleted test windows_falls_back_to_comspec_cmd_with_utf8_prefix to prevent this regression.
|
@Hmbown I want to confirm the intended direction before I make another change here. In your #1779 / #2279 comments, you said that if Can you confirm which behavior you want for this branch?
I’ll align the branch to your preference. Paulo Aboim Pinto |
|
pwsh.exe → cmd.exe - thank you so much! |
|
@Hmbown aligned this with your preference: the default Windows fallback stays I also restored coverage for the COMSPEC full-path Verified locally:
Paulo Aboim Pinto |
Summary
-NonInteractiveto PowerShell invocationcmd.exefull-path raw-argument handling fix and its regression coverageshell_program_stemhelper reusable inside the crateThis is a follow-up branch because I cannot push directly to the maintainer branch from this environment.
Paulo Aboim Pinto
Greptile Summary
This PR refines Windows shell handling in three focused ways: the system-prompt environment block now shows a shell stem (
pwsh,cmd) instead of a full executable path; PowerShell invocations gain-NonInteractive; andpush_eval_shell_argsineval.rscorrectly matches full-pathcmd.exereferences by routing throughshell_program_stemrather than a plain case-insensitive string compare.powershell.exe(Windows PowerShell 5.x) auto-detection fallback is intentional and is now documented with a code comment; users can still select it explicitly via theSHELLenvironment variable.-NonInteractiveis added to everypowershell_invocationcall (bothpwsh.exeand explicitpowershell.exeviaSHELL), preventing PowerShell from waiting for user input in automated contexts.shell_program_stemis promoted topub(crate)and reused consistently acrosseval.rsandprompts.rs, eliminating duplicated path-normalisation logic.Confidence Score: 5/5
Safe to merge; all behaviour changes are intentional, documented, and covered by new tests.
The three changed files make tightly scoped improvements. The powershell.exe auto-detection removal is explicitly documented in a code comment and a new test confirms the correct fallback path. The shell_program_stem refactor produces identical output for all edge cases. The -NonInteractive addition and stem-only prompt display have targeted regression tests. No logic regressions or data-correctness issues were identified.
No files require special attention.
Important Files Changed
Comments Outside Diff (1)
crates/tui/src/shell_invocation.rs, line 129-131 (link)powershell.exeauto-detection fallbackThe
powershell_on_pathfield and its associated fallback branch were removed, which means Windows users who only havepowershell.exe(Windows PowerShell 5.x) installed — but notpwsh(PowerShell 7) — will now silently fall through tocmd.exe. This is a non-trivial behaviour change for any Windows machine that ships PowerShell 5.x exclusively (e.g. Windows Server 2019, unmanaged enterprise desktops). The PR description does not mention this removal, so it is unclear whether it is intentional. Ifpowershell.exeis being deliberately dropped, a short comment or changelog entry would help reviewers and users understand the new minimum requirement.Reviews (3): Last reviewed commit: "test(tui): lock Windows shell fallback b..." | Re-trigger Greptile