fix(doctor): decode subprocess output as UTF-8 on non-UTF-8 (GBK) locales - #49510
fix(doctor): decode subprocess output as UTF-8 on non-UTF-8 (GBK) locales#49510briandevans wants to merge 2 commits into
Conversation
…ales hermes doctor crashes on Chinese Simplified Windows (CP936/GBK) with UnicodeDecodeError: 'gbk' codec can't decode byte 0xaa raised inside subprocess._readerthread. The text-mode subprocess.run(..., text=True) calls in doctor decode child output using the system locale; on a GBK locale a child emitting UTF-8 bytes triggers the crash, taking down the first command users reach for to diagnose their environment. Pin encoding="utf-8", errors="replace" on the two text-mode subprocess sites in doctor (the SSH connectivity probe and the npm audit --json call). The docker-info and gh-auth-status calls are bytes-mode and never decode, so they are unaffected. This is a sibling follow-up to NousResearch#18637, which fixed the .env file-read GBK path but did not touch the subprocess decode path. Fixes NousResearch#49499
There was a problem hiding this comment.
Pull request overview
Fixes hermes doctor crashing on GBK/CP936 Windows locales by forcing UTF-8 decoding (with replacement) for subprocess output in doctor’s text-mode subprocess.run probes.
Changes:
- Pin
encoding="utf-8", errors="replace"on doctor’s SSH connectivity probe subprocess call. - Pin
encoding="utf-8", errors="replace"on doctor’snpm audit --jsonsubprocess call. - Add a regression test that simulates a GBK-locale
UnicodeDecodeErrorat the subprocess text-decoding boundary.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/doctor.py |
Forces UTF-8 decoding for the two text=True subprocess call sites to avoid GBK-locale decode crashes. |
tests/hermes_cli/test_doctor.py |
Adds a regression test simulating GBK subprocess decode failures to ensure doctor survives and exits normally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Drive the SSH connectivity probe (the first text-mode subprocess | ||
| # site in run_doctor) by selecting the ssh terminal backend. | ||
| monkeypatch.setenv("TERMINAL_ENV", "ssh") | ||
| monkeypatch.setenv("TERMINAL_SSH_HOST", "example.invalid") | ||
|
|
||
| hermes_home = tmp_path / ".hermes" | ||
| hermes_home.mkdir() | ||
| monkeypatch.setattr(doctor_mod, "HERMES_HOME", hermes_home) | ||
|
|
alt-glitch
added
type/bug
… Copilot review)
The existing TestDoctorSubprocessEncoding test drives only the SSH probe
text-mode subprocess site; it never reaches the second text-mode site, the
`npm audit --json` call, because _safe_which("npm") is not stubbed and
PROJECT_ROOT/node_modules does not exist. Stripping encoding="utf-8",
errors="replace" from the npm-audit subprocess.run would therefore not fail
any test.
Add a sibling test that stubs _safe_which for node/npm, monkeypatches
PROJECT_ROOT to a tmp dir with a node_modules/ subdir so run_doctor()
actually executes the npm-audit subprocess under the same GBK decode
simulation. Because that call is wrapped in a broad `except Exception: pass`,
a UnicodeDecodeError there is swallowed rather than crashing, so the test
asserts on the observable outcome — the "deps (no known vulnerabilities)"
result line — which is present only when the audit decodes successfully.
Verified it fails if the npm-audit encoding/errors kwargs are removed.
|
@copilot Addressed in 38b67a3 (test-only). Added One nuance worth flagging: unlike the SSH probe, the npm-audit call is wrapped in a broad |
|
Draining our contribution queue to keep it lean and reviewable \u2014 closing this as stale (no reviewer traction in 2+ weeks). If the underlying issue is still live on current main, we'll re-file a fresh, focused fix. |
This is a sibling follow-up to #18637 (GBK locale crash, bug 3)
.envfile read — pinnedencoding="utf-8"onPath.read_textso a UTF-8.envno longer crashes a GBK-locale doctor.text=Truesubprocess.runstill decodes child output via the GBK system locale.encoding="utf-8", errors="replace"on the two text-mode subprocess sites (SSH probe + npm audit) so a child emitting UTF-8 bytes no longer crashes doctor insubprocess._readerthread.What does this PR do?
hermes doctorcrashes on Chinese Simplified Windows (CP936/GBK) withUnicodeDecodeError: 'gbk' codec can't decode byte 0xaaraised insidesubprocess._readerthread. The text-modesubprocess.run(..., text=True)calls in doctor decode child output using the system locale; on a GBK locale a child emitting UTF-8 bytes triggers the crash, taking down the first command users reach for to diagnose their environment. This pins UTF-8 decoding witherrors="replace"on every text-mode subprocess site in doctor.Related Issue
Fixes #49499
Type of Change
Changes Made
hermes_cli/doctor.py: addedencoding="utf-8", errors="replace"to the SSH connectivity probe and thenpm audit --jsonsubprocess calls (the twotext=Truesites —grep text=True hermes_cli/doctor.pyconfirms these are the only two; the docker-info and gh-auth-status calls are bytes-mode and never decode). Covers all text-mode subprocess decode sites in doctor.tests/hermes_cli/test_doctor.py: regression test simulating a GBK locale (UnicodeDecodeError unless UTF-8 is pinned), proving doctor's subprocess path survives — fails before / passes after.How to Test
uv run --with pytest --with pytest-asyncio python3 -m pytest tests/hermes_cli/test_doctor.py -vChecklist
Code
Documentation & Housekeeping