fix: character encoding issues in shell command processor - #1949
Conversation
Fixes google-gemini#1945 - Add system encoding detection for Windows (chcp) and Unix-like systems (locale) - Replace StringDecoder with TextDecoder for better multi-byte character support - Add comprehensive Windows code page to encoding mappings - Remove deprecated decoder.end() calls that are not needed with TextDecoder This resolves issues with garbled output when shell commands produce non-UTF-8 encoded text, particularly on Windows systems with different code pages.
|
Today I found that when entering Chinese, only the last character will be displayed in the terminal, and the way of copying text will also only show the last character.... |
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request addresses character encoding issues in the shell command processor by adding system encoding detection and replacing StringDecoder with TextDecoder. However, there are potential issues with the fallback mechanisms for encoding detection and handling unsupported code pages. Additionally, the use of stripAnsi might remove valuable formatting information. Addressing these issues will improve the robustness and user experience of the shell command processor.
- Add NaN check for parsed code page value in getSystemEncoding() - Rename unused catch variable to indicate intentional non-use - Prevent potential issues from invalid code page values Resolves PR feedback in google-gemini#1949
- Implement locale charmap fallback for Unix-like systems when env vars are missing (Addresses code review feedback from PR google-gemini#1949)
Fall back to utf-8 instead of `cp${cp}` for unknown code pages
and log warning to prevent TextDecoder errors.
jacob314
left a comment
There was a problem hiding this comment.
Thank you for the pull request! This is a very important polish item.
- Add cachedSystemEncoding variable to store detected encoding - Add getCachedSystemEncoding() function to return cached value or detect once - Update executeShellCommand to use cached encoding instead of calling getSystemEncoding() repeatedly
…ream handling
- Add { stream: true } option to decoder.decode() calls for proper streaming
- Handle final bytes by calling decode() at end-of-stream
- Based on WHATWG Encoding specification: https://encoding.spec.whatwg.org/#interface-textdecoder
- Add chardet dependency for robust encoding detection - Enhance getCachedSystemEncoding to use chardet as fallback - Improve system encoding detection with better error handling - Initialize TextDecoders lazily after encoding detection - Return null instead of fallback encoding when detection fails - Add detectEncodingFromBuffer function for buffer-based detection - Update Windows code page mapping with better error messages - Improve comment formatting and documentation
…eShellCommand function
…for testing Includes function comments and types for clarity and a cache reset utility for test support.
- Windows code page mapping tests - Buffer encoding detection tests - Cross-platform system encoding tests - Edge cases and error handling tests
…initialized state for fixing the tests
@jacob314 Thanks so much for the thorough code review and your patience! All comments have been addressed, and I’ve added tests with mocked command output to ensure cross-platform compatibility. Please feel free to let me know if you spot any other issues or have further suggestions. |
Co-authored-by: Jacob Richman <jacob314@gmail.com> Co-authored-by: Sandy Tao <sandytao520@icloud.com>
Co-authored-by: Jacob Richman <jacob314@gmail.com> Co-authored-by: Sandy Tao <sandytao520@icloud.com>
…ini#1949) Co-authored-by: Jacob Richman <jacob314@gmail.com> Co-authored-by: Sandy Tao <sandytao520@icloud.com>
…ini#1949) Co-authored-by: Jacob Richman <jacob314@gmail.com> Co-authored-by: Sandy Tao <sandytao520@icloud.com>
…ini#1949) Co-authored-by: Jacob Richman <jacob314@gmail.com> Co-authored-by: Sandy Tao <sandytao520@icloud.com>

Fixes #1945
This resolves issues with garbled output when shell commands produce non-UTF-8 encoded text, particularly on Windows systems with different code pages.
TLDR
This PR improves the handling of shell command outputs with non-UTF-8 encodings by dynamically detecting system encoding on Windows and Unix-like OSes. It replaces
StringDecoderwithTextDecoderto properly decode multi-byte characters such as Chinese, fixing the garbled text problem.Dive Deeper
Previously, shell command outputs containing Chinese or other non-UTF-8 characters appeared corrupted due to improper decoding. The original implementation used
StringDecoder, which does not support Big5 or other non-UTF-8 encodings well. On Windows, code pages vary and can be detected viachcp, while on Unix-like systems,localeprovides the encoding info. By integrating these detection methods and switching toTextDecoder(which supports multi-byte decoding natively), this change ensures the correct interpretation of command outputs across different environments. Deprecated calls todecoder.end()were removed becauseTextDecoderdoes not require them.Reviewer Test Plan
geminion Windows with a code page like 950 (Big5) or 936 (GBK) that outputs Chinese characters, and verify the output is displayed correctly without garbled text.Testing Matrix
Linked issues / bugs
#1945: Garbled output from shell commands containing Chinese characters due to encoding issues