Skip to content

Conversation

eddy-win
Copy link
Contributor

@eddy-win eddy-win commented Aug 14, 2025

What? Why? How?

  • When running on Windows, codex often tries to invoke bash commands, which commonly fail (unless WSL is installed)
  • Fix: Detect if powershell is available and, if so, route commands to it
    • Also add a shell_name property to environmental context for codex to default to powershell commands when running in that environment

Testing

  • Tested within WSL and powershell (e.g. get top 5 largest files within a folder and validated that commands generated were powershell commands)
  • Tested within Zsh
  • Updated unit tests

Copy link

github-actions bot commented Aug 14, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@eddy-win
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@eddy-win
Copy link
Contributor Author

recheck

@eddy-win eddy-win force-pushed the eddy-oai/powershell-command branch from b694b04 to 22e29cc Compare August 14, 2025 23:24
@eddy-win
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@eddy-win
Copy link
Contributor Author

recheck

@eddy-oai
Copy link
Contributor

I have read the CLA Document and I hereby sign the CLA

@eddy-oai
Copy link
Contributor

recheck

github-actions bot added a commit that referenced this pull request Aug 15, 2025
@eddy-win eddy-win force-pushed the eddy-oai/powershell-command branch 4 times, most recently from e56de43 to 9a957d1 Compare August 19, 2025 21:56
Copy link
Collaborator

@bolinfest bolinfest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, overall this seems great! I do worry a bit about the escaping in the one place I called out. Could we at least get more unit tests to try to flag edge cases like newlines and other types of whitespace that might make parsing funny?

let first = command.first().map(String::as_str);
if first != Some(ps.exe.as_str()) {
// TODO (CODEX_2900): Handle escaping newlines.
if command.iter().any(|a| a.contains('\n') || a.contains('\r')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit dicey?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, flagged CODEX-2900 for follow-up

return Some(command);
}

let joined = shlex::try_join(command.iter().map(|s| s.as_str())).ok();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is shlex's quoting rules appropriate for Windows?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not the best, flagged in CODEX-2900 for follow-up

use tokio::process::Command;

// Prefer PowerShell 7+ (`pwsh`) if available, otherwise fall back to Windows PowerShell.
let has_pwsh = Command::new("pwsh")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider tokio::join! to run these in parallel since this is on the startup path via Session::new()?

@eddy-win eddy-win force-pushed the eddy-oai/powershell-command branch from 9a957d1 to 84cee01 Compare August 20, 2025 21:25
@eddy-oai eddy-oai merged commit 050b9ba into openai:main Aug 20, 2025
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants