-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: handle Windows cmd encoding for ipconfig and other commands #7804
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
Conversation
- Set UTF-8 code page (65001) for Windows cmd before executing commands - Add proper UTF-8 encoding for PowerShell commands - Handle encoding fallback for Windows terminal output - Fix garbled output issue when running ipconfig through cmd Fixes #7803
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.
I reviewed my own code and found issues. Classic case of 'it worked on my machine' syndrome.
| if (process.platform === "win32") { | ||
| // Set code page to UTF-8 (65001) before running the command | ||
| // and restore it afterwards to avoid affecting other programs | ||
| actualCommand = `chcp 65001 >nul 2>&1 && ${command}` |
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.
Is this intentional? When using VSCode terminal integration, commands get wrapped with chcp twice - once here in ExecaTerminalProcess and once in TerminalProcess. This results in: chcp 65001 >nul 2>&1 && chcp 65001 >nul 2>&1 && ipconfig
Perhaps we should only apply this in ExecaTerminalProcess since TerminalProcess handles VSCode's integrated terminal?
| let actualCommand = command | ||
| if (process.platform === "win32") { | ||
| // Set code page to UTF-8 (65001) before running the command | ||
| // and restore it afterwards to avoid affecting other programs |
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.
The comment mentions "restore it afterwards" but I don't see where we're restoring the original code page. Should we either:
- Remove this misleading comment, or
- Actually implement code page restoration (e.g.,
chcp 65001 >nul 2>&1 && %s && chcp %original% >nul 2>&1)?
| yield new TextDecoder("utf-8", { fatal: true }).decode(chunk) | ||
| } catch { | ||
| // Fallback to Windows-1252 if UTF-8 decoding fails | ||
| yield new TextDecoder("windows-1252", { fatal: false }).decode(chunk) |
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.
When the fallback to Windows-1252 occurs, we're silently handling the encoding issue. Would it be helpful to add logging here to track when UTF-8 decoding fails? This could help debug encoding issues in production:
| // On Windows, set the console output code page to UTF-8 | ||
| // This helps with proper encoding of command outputs like ipconfig | ||
| if (process.platform === "win32") { | ||
| env.PYTHONIOENCODING = "utf-8" |
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 environment variable only affects new Python processes. Should we document that existing Python processes won't pick up this change? Also, is there a reason we're only setting this for Python and not other interpreters that might have similar issues?
| let commandToExecute = command | ||
|
|
||
| // Set UTF-8 encoding for PowerShell | ||
| commandToExecute = `[Console]::OutputEncoding = [System.Text.Encoding]::UTF8; ${commandToExecute}` |
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.
Could we add a comment explaining why PowerShell uses while cmd uses ? This would help future maintainers understand the different approaches:
[Console]::OutputEncoding = [System.Text.Encoding]::UTF8;
| terminal.shellIntegration.executeCommand(commandToExecute) | ||
| } else if (isCmd) { | ||
| // For Windows cmd, set code page to UTF-8 before executing the command | ||
| const commandToExecute = `chcp 65001 >nul 2>&1 && ${command}` |
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.
Same issue as in ExecaTerminalProcess - this creates duplicate chcp commands when both terminal processes are involved. Should we consolidate this logic to avoid redundancy?
|
This doesn't seem like a proper fix, the issue needs scoping |
Summary
This PR attempts to address Issue #7803 where running
ipconfigthrough cmd terminal results in garbled output in the chat and no content displayed in the terminal.Problem
When VSCode is configured to use cmd as the default terminal on Windows, commands like
ipconfigproduce garbled output because:Solution
The fix implements proper UTF-8 encoding handling for Windows terminals at multiple levels:
chcp 65001(UTF-8 code page) to commands executed in Windows cmdChanges
ExecaTerminalProcess.tsto wrap Windows commands with UTF-8 code page settingTerminalProcess.tsto detect and handle both cmd and PowerShell terminalsTerminal.tsTesting
Notes
Testing on an actual Windows environment with cmd terminal would be beneficial to confirm the fix works as expected across different Windows versions.
Fixes #7803
Feedback and guidance are welcome!
Important
Fixes Windows terminal encoding issues by ensuring UTF-8 encoding for command outputs in
ExecaTerminalProcess.ts,Terminal.ts, andTerminalProcess.ts.chcp 65001inExecaTerminalProcess.tsandTerminalProcess.tsto ensure UTF-8 output.TerminalProcess.ts.ExecaTerminalProcess.ts.PYTHONIOENCODINGtoutf-8for Windows inTerminal.ts.TerminalProcess.tsto handle both cmd and PowerShell terminals.Terminal.ts.This description was created by
for 2609ec7. You can customize this summary. It will automatically update as commits are pushed.