fix(shell): decode command output as UTF-8 instead of the platform codepage - #395
Open
LHMQ878 wants to merge 1 commit into
Open
fix(shell): decode command output as UTF-8 instead of the platform codepage#395LHMQ878 wants to merge 1 commit into
LHMQ878 wants to merge 1 commit into
Conversation
…depage
`text=True` with no `encoding=` decodes the child's output with the
platform's preferred encoding, which is a legacy codepage on a stock
Windows install (cp936 on a Chinese system, cp1252 on a Western one).
Command output is the least trustworthy byte stream the shell handles --
`cat` on a binary file, a compiler quoting a snippet in another encoding,
curl echoing a response body -- and under a codepage those bytes fail two
ways:
* bytes the codepage rejects raise UnicodeDecodeError inside the reader
thread. The `finally` pushes the EOF sentinel, `run()` reads that as
"the shell died", and the session is gone along with every line
already buffered -- including the ASCII either side of the bad bytes.
* bytes the codepage accepts but maps differently decode to mojibake
with no exception at all, handing the model a corrupted string it
cannot distinguish from the real one.
Pass `encoding="utf-8", errors="replace"` to both Popen calls, matching
how `tools/files.py` already reads files. The `pgrep` call is left as is:
POSIX-only and emits nothing but PID digits.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
platform/coworker/tools/shell.pyspawns the persistent shell withtext=Trueand noencoding=, so the child's output is decoded with the platform's preferred encoding — a legacy codepage on a stock Windows install (cp936 on a Chinese system, cp1252 on a Western one), not UTF-8. The file already has explicit Windows/PowerShell support, so this path is reached in normal use.Command output is the least trustworthy byte stream this module handles —
caton a binary file, a compiler quoting a snippet in another encoding,curlechoing a response body, a test runner printing a non-ASCII assertion message. Under a codepage those bytes fail in two ways, and the first one is severe:1. Bytes the codepage rejects kill the whole shell session.
_read_loopis a daemon thread iteratingproc.stdout. AUnicodeDecodeErrorthere escapes the loop, thefinallypushes the EOF sentinel, andrun()reads that sentinel as "the shell died". The session is gone — and so is every line already buffered, including the plain ASCII on either side of the offending bytes.Reproduced with the exact structure of
_read_loop(text=True, reader in a daemon thread, EOF sentinel infinally), a child writingb"ok-before\n\x81 raw\nok-after\n":Note
lines: [].ok-beforehad already been written by the child and was lost too.2. Bytes the codepage accepts but maps differently become silent mojibake.
"café".encode("utf-8")decodes under cp936 to"café"with no exception at all. The model receives a corrupted string it cannot distinguish from the real one — no error to notice, no signal that anything went wrong.More Information
Two sites, both
subprocess.Popen(..., text=True)with noencoding=:_BackgroundTask.__init__(~line 96)shell_task_outputLocalExecutor._spawn(~line 194)Both now get
encoding="utf-8", errors="replace"via a shared module constant that documents the reasoning:errors="replace"rather thanstrictis the point of the fix: undecodable bytes degrade toU+FFFDand the stream stays in sync, instead of a decode error taking down the reader thread and the session with it.The
subprocess.runcall forpgrep(~line 372) is deliberately left alone: it is POSIX-only, emits nothing but PID digits, and is already guarded byValueErroronint(pid).This is also a consistency fix. The repo already names UTF-8 with a permissive error handler where it reads untrusted text:
tools/files.py:76(encoding="utf-8", errors="replace"),tools/search.py:165(errors="ignore"), and four call sites intools/email_tools.py. The shell was the one place reading external bytes with the locale default.Validation
Three tests added to
platform/tests/test_shell.py, in a new "output decoding" section, following the file's existing per-OS parameterized command style:test_undecodable_output_does_not_kill_the_sessiontest_utf8_output_is_not_mojibake"café" in outputand"café" not in output)test_background_task_undecodable_output_survivesPopencall, reached only by background tasksThe mojibake test needs the bytes to arrive at the pipe unmodified, so it writes them straight to the stdout handle (
[Console]::OpenStandardOutput().Write(...)on Windows,printf 'caf\303\251\n'on POSIX). PowerShell'sWrite-Outputwould re-encode through its own output encoding first, which is a different concern from the decoding under test.Control experiment —
shell.pyreverted to the unpatchedPopencalls, new tests kept. The three failures map one-to-one onto the three failure modes:The 10 that stay green are the existing tests — the fix does not change any behavior they cover.
With the fix:
tests/test_shell.py→ 13 passed. Together with the neighbouring tool suites (test_shell.py test_code_tools.py test_tools_permissions.py) → 39 passed.No regressions. Full
platform/tests/suite:mainThe same 12 failures on both trees —
test_fake_slack.py(6),test_slack_relay.py(4),test_github_installs.py(1),test_ui_refresh_e2e.py(1), mostlyTimeoutErrorfrom tests that need a live socket. Pre-existing and unrelated. The delta is exactly the 3 new tests.ruff check→ All checks passed.ruff format --check→ 2 files already formatted.On a UTF-8 host the previous behavior and the new one coincide, so nothing that worked before changes.
Linked Issues
None — I did not find an existing issue for this (searched
encoding,UnicodeDecodeError,mojibake,codepage,shell decode: 0 relevant results). Happy to open one if you'd prefer the issue-first flow.