Skip to content

fix(doctor): pin utf-8/replace decoding for captured subprocesses - #49512

Closed
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:fix/49499-doctor-subprocess-encoding
Closed

fix(doctor): pin utf-8/replace decoding for captured subprocesses#49512
Bartok9 wants to merge 2 commits into
NousResearch:mainfrom
Bartok9:fix/49499-doctor-subprocess-encoding

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • hermes doctor crashed with UnicodeDecodeError on non-UTF-8 Windows locales (CP936/GBK).
  • Captured subprocess output now decodes deterministically and survives undecodable bytes.

Motivation

Closes #49499.

On Chinese Simplified Windows (default locale CP936/GBK), hermes doctor crashed during the connectivity/diagnostic checks:

UnicodeDecodeError: 'gbk' codec can't decode byte 0xaa in position 1753: illegal multibyte sequence

subprocess.run(..., text=True) decodes the child's output with the system locale codec. When a captured child emits UTF-8 bytes that are invalid under GBK, the decode raises inside subprocess's reader thread and takes down the command that's supposed to be the first troubleshooting step.

Fix

Add a small helper _run_text_subprocess() that pins encoding="utf-8" and errors="replace" (both caller-overridable), and route the two text-capturing doctor probes — the SSH connectivity check and the npm audit check — through it. Undecodable bytes now degrade to the replacement character instead of crashing, exactly as the issue's "Suggested Fix" requested.

The two byte-only captures (docker info, gh auth status) don't decode text and are left unchanged.

Verification

  • python3 -m pytest tests/hermes_cli/test_doctor.py — 70 passed

Real behavior proof

gbk -> CRASH (reproduces #49499): 'gbk' codec can't decode byte 0xaa in position 2: illegal multibyte sequence
  • After — same bytes through _run_text_subprocess(...):
helper -> survives: 'ok\ufffd\ufffd tail'
  • Regression test: tests/hermes_cli/test_doctor.py::TestRunTextSubprocess::test_survives_bytes_invalid_under_gbk spawns a child that writes b'ok\xaa\xff tail' and asserts the helper returns cleanly (returncode 0, output preserved) — it would raise UnicodeDecodeError under a GBK locale with the old text=True path.
  • What was NOT tested: the live GBK Windows environment itself (no Windows runner locally); the test reproduces the decode mechanism cross-platform by exercising the exact byte pattern.

Closes NousResearch#49499

Root cause: subprocess.run(..., text=True) decodes child output with
the system locale codec. On non-UTF-8 Windows locales (Chinese
CP936/GBK), a child that emits UTF-8 bytes triggers UnicodeDecodeError
inside subprocess's reader thread, crashing 'hermes doctor' mid-run
(the reported 'gbk codec can't decode byte 0xaa' traceback).

Fix: add _run_text_subprocess() that pins encoding='utf-8' and
errors='replace' (caller-overridable) and route the two text-capturing
doctor probes (SSH connectivity, npm audit) through it. Undecodable
bytes now degrade to the replacement character instead of raising, so
doctor keeps running and prints a readable diagnostic.

Tests: TestRunTextSubprocess covers the utf-8/replace default, text
kwarg normalization, caller override, and an end-to-end run with bytes
(0xaa 0xff) that raise UnicodeDecodeError under GBK but decode cleanly
through the helper. 70 tests pass in tests/hermes_cli/test_doctor.py.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jun 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #49510 — both fix #49499 by pinning encoding="utf-8", errors="replace" on the same two text-mode subprocess sites (SSH probe + npm audit) in hermes_cli/doctor.py. #49510 was opened earlier (the canonical fix); this PR factors the same logic into a _run_text_subprocess() helper, but the behavior change is identical.

The _run_text_subprocess wrapper passed timeout via **kwargs, which the
static subprocess-timeout guardrail (tests/hermes_cli/test_subprocess_timeouts.py)
cannot see, so CI failed on doctor.py:47. Make timeout a keyword-only param
forwarded literally to subprocess.run so the AST check passes; callers
already supply it.
@Bartok9

Bartok9 commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @alt-glitch — you're right, this duplicates #49510 (opened earlier by @briandevans), which is the canonical fix for #49499. Same two text-mode subprocess sites, same encoding="utf-8", errors="replace" behavior. Deferring to the earlier PR — closing this in its favor.

@briandevans if it's useful, the only delta here was factoring the two call sites into a shared _run_text_subprocess() helper to avoid the duplicated kwargs; feel free to fold that in or ignore. Either way #49510 should be the one that lands. 👍

@Bartok9 Bartok9 closed this Jun 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hermes doctor crashes with UnicodeDecodeError on Chinese Windows GBK locale

2 participants