Conversation
…date flow (fixes NousResearch#37423) gateway/run.py had 20 bare Path.read_text() / Path.write_text() calls with no encoding=. On Windows with a non-UTF-8 system locale (cp1252 on US installs, GBK/CP936 on Chinese installs), Python uses the locale codec instead of UTF-8, which crashes the gateway command handler with UnicodeEncodeError (when the user's gateway reply contains emoji/CJK) or UnicodeDecodeError/mojibake (when reading the hermes update subprocess's captured UTF-8 stdout). Both errors subclass ValueError, not OSError, so the surrounding except OSError guards don't catch them and the gateway command handler / update-stream coroutine dies. Both static guardrails (ruff PLW1514, scripts/check-windows-footguns.py) explicitly only flag builtin open() and don't see Path.read_text() on a variable receiver. The new tests/gateway/test_windows_utf8_file_io.py adds a focused AST-based regression guard that walks the gateway AST and asserts every .read_text() and .write_text() call site passes an explicit encoding= kwarg, plus a behavioural round-trip test that writes and reads an emoji/CJK payload under a fake cp1252 locale. Closes NousResearch#37423
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real Windows encoding failure: current gateway/run.py:9040 still writes an update response without an encoding, and hermes_bootstrap.py:32-37 confirms bootstrap cannot change current-process file defaults.
Problems
- The update IPC fix is incomplete:
hermes_cli/main.py:4600/:4608and platform response writers such asplugins/platforms/telegram/adapter.py:5668,plugins/platforms/feishu/adapter.py:2155,plugins/platforms/discord/adapter.py:7143, andgateway/platforms/qqbot/adapter.py:1204use the same.update_*protocol with bare text I/O. tests/gateway/test_windows_utf8_file_io.py:116fixes a source call-site count, which is a change-detector test rather than a behavior contract (AGENTS.md:80-87,AGENTS.md:1309-1355).tests/gateway/test_windows_utf8_file_io.py:171-194opens a separate cp1252 file but does not alter Path defaults or invoke gateway code; its tested calls already explicitly use UTF-8.
Suggested changes
- Cover all production update-IPC readers/writers, then replace the static count and standalone round trip with targeted temp-
HERMES_HOMEupdate-flow coverage for non-ASCII payloads.
Automated hermes-sweeper review.
| f"{missing[:5]}" | ||
| ) | ||
|
|
||
| def test_no_bare_read_text_or_write_text_remains( |
There was a problem hiding this comment.
This fixed minimum call-site count is a change-detector: moving or consolidating update I/O can fail the test without changing the UTF-8 contract. Please remove it and test the update IPC behavior instead.
|
|
||
| # Force Python to use cp1252-style behaviour for bare open() | ||
| # so we catch the regression even if a future refactor drops | ||
| # the encoding= kwarg. |
There was a problem hiding this comment.
This does not simulate a cp1252 default: opening a separate file with _io.open(..., encoding="cp1252") never changes Path.write_text() defaults, and this call explicitly passes UTF-8. Exercise the gateway/CLI update IPC path with non-ASCII data instead.
|
Closing as resolved by PR #71078 (merged, commit d372fda), which retired the bare read_text/write_text class codebase-wide: 139 sites guarded (11 contributor PRs salvaged with authorship preserved — earliest submitters credited — plus an AST sweep), with a CI linter rule and AST guard test preventing regressions. Every site this PR touches is guarded on current main — verified per-site before closing. Thanks for the fix; the number of independent PRs on this class is what escalated it to the campaign. |
What does this PR do?
Fixes
hermes update-over-gateway on Windows with a non-UTF-8 system locale. The 20 barePath.read_text()/Path.write_text()calls ingateway/run.py(noencoding=) were using Python's locale codec instead of UTF-8, which crashes the gateway command handler withUnicodeEncodeError(when the user's gateway reply contains emoji/CJK) orUnicodeDecodeError/ silent mojibake (when reading thehermes updatesubprocess's captured UTF-8 stdout). Both subclassValueError, notOSError, so the surroundingexcept OSErrorguards don't catch them.Changes
gateway/run.py— addsencoding="utf-8"to all 14.read_text()and 6.write_text()call sites in the update flow (lines 2113, 2140, 3813, 3843, 3889, 7354, 7374, 10690, 14770, 14910, 14944, 14982, 14992, 15017, 15035, 15128, 15142, 15148, 15082, 15201). Handles both single-line and multi-linewrite_text(...)shapes.tests/gateway/test_windows_utf8_file_io.py— new file with 5 tests:.read_text()and.write_text()call site ingateway/run.pymust passencoding=(catches the original bug, plus the inverse regression of someone usingencoding=locale.getpreferredencoding()).How to test
~/.hermes/hermes-agent/venv/bin/python -m pytest tests/gateway/test_windows_utf8_file_io.py -q— 5/5 pass.~/.hermes/hermes-agent/venv/bin/python -m pytest tests/gateway/ -q --timeout=60— all gateway tests pass, no regressions.encoding="utf-8"kwargs is reverted (manually walked the file, ran tests, restored).Notes
ruff PLW1514andscripts/check-windows-footguns.py) explicitly only flag builtinopen(), notPath.read_text()on a variable receiver. The new test fills that gap for the gateway module specifically.write_text(the file path + JSON payload span 3 lines); handled by passing theencoding="utf-8"on its own line in the closing tuple, matching the rest of the codebase's style.encoding="utf-8"kwarg is just noise on Linux. The change matters on Windows.Closes #37423