fix(gateway): add explicit utf-8 encoding to read_text/write_text in run.py (Windows) - #37424
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Windows compatibility fix. The underlying defect is still present on current main: gateway/run.py:9040 writes update responses with bare write_text(), and gateway/run.py:14589 reads update output with bare read_text().
Problems
- The
/updatecommand handler moved after this PR was opened. Commit619bd7827extracted it intogateway/slash_commands.py; its current pending-marker write atgateway/slash_commands.py:4562is still bare, so the PR’s oldgateway/run.pyedit does not cover that live sibling path. - The new guard only scans
gateway/run.py, so it would not protect the moved handler.
Suggested changes
- Salvage the
run.pychanges onto current HEAD and addencoding="utf-8"atgateway/slash_commands.py:4562. - Broaden the AST guard to cover both current files, or explicitly cover all current
/updatecoordination-file paths.
This is an automated hermes-sweeper review.
| @@ -14767,7 +14768,7 @@ async def _handle_update_command(self, event: MessageEvent) -> str: | |||
| if event.message_id: | |||
There was a problem hiding this comment.
This handler was extracted on current main by 619bd7827; mirror this encoding change in the live replacement at gateway/slash_commands.py:4562, which still calls _tmp_pending.write_text(json.dumps(pending)) without an explicit encoding.
There was a problem hiding this comment.
Confirmed and fixed. Rebuilt the branch on current main (89bd0fba9): the pending-marker write in the extracted handler now passes encoding="utf-8" at gateway/slash_commands.py:4586 (the line shifted from the 4562 you cited), and the AST guard now scans both gateway/run.py and gateway/slash_commands.py so the two halves of the /update path can't regress independently.
One correction on severity, for the record: that specific call is _tmp_pending.write_text(json.dumps(pending)), and json.dumps defaults to ensure_ascii=True — so it emits pure ASCII and was not a live crash on cp1252. The bytes on disk are unchanged by this edit. I fixed it anyway to match the "always pass encoding" policy and to keep it safe if ensure_ascii=False is ever introduced.
The two calls that do crash are still in run.py and are covered: the reply write at run.py:9122 (UnicodeEncodeError on an emoji/CJK reply — and it isn't an OSError, so the except OSError right below it doesn't catch it) and the subprocess-stdout reads at run.py:14671 / 14710 / 14842.
Verified on a cp1252 host — 20 offenders before, 0 after; ruff and scripts/check-windows-footguns.py --all clean; the /update and voice suites show identical pass/fail counts before and after.
f044610 to
95d8ac1
Compare
…ate path On native Windows with a non-UTF-8 locale (cp1252 on US installs, GBK/ CP936 on Chinese installs), the `hermes update` flow over the messaging gateway crashed: the gateway read and wrote its coordination files with bare Path.read_text()/write_text(), so Python used the locale codec instead of UTF-8. Writing the user's emoji/CJK update-prompt reply raised UnicodeEncodeError; reading the update subprocess's UTF-8 stdout raised UnicodeDecodeError (or produced mojibake). Both subclass ValueError, so the surrounding `except OSError` did not catch them and the gateway command handler / update-stream coroutine crashed. Add encoding="utf-8" to the 19 remaining read_text/write_text calls in gateway/run.py and to the pending-marker write in gateway/slash_commands.py, which 619bd78 extracted out of run.py into GatewaySlashCommandsMixin after this change was first written. That marker write goes through json.dumps (ensure_ascii=True), so it is ASCII-safe today and the bytes on disk are unchanged; it is fixed to match the project's "always pass encoding" policy and to stay safe if ensure_ascii=False is ever introduced. The static AST guard covers both gateway/run.py and gateway/slash_commands.py so the two halves of the update path cannot regress independently. Neither existing guardrail catches these: ruff PLW1514 does not resolve .read_text() on a variable/attribute receiver (ruff check --select PLW1514 passes on the unfixed file), and scripts/check-windows-footguns.py scans builtin open() only. Fixes NousResearch#37423
95d8ac1 to
2583cd4
Compare
|
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 & why
On native Windows with a non-UTF-8 locale (cp1252 on US installs, GBK/CP936 on Chinese installs), the
hermes update-over-gateway flow crashed. The gateway read/wrote its coordination files with barePath.read_text()/Path.write_text(), so Python used the locale codec instead of UTF-8:UnicodeEncodeError;hermes updatesubprocess's captured UTF-8 stdout →UnicodeDecodeError/ mojibake.Both subclass
ValueError, so the surroundingexcept OSErrordid not catch them and the handler / update-stream coroutine crashed.Fixes #37423
Rebased onto current HEAD (addresses @teknium1's review)
The original branch was cut before
619bd7827("extract 42 slash-command handlers into GatewaySlashCommandsMixin"), which moved the/updatehandler out ofrun.py. This PR is now rebuilt on currentmain(89bd0fba9):gateway/run.py—encoding="utf-8"on the 19 remaining bareread_text/write_textcalls. Both live crash sites are still here: the reply write (run.py:9122) and the subprocess-stdout reads (run.py:14671,14710,14842).gateway/slash_commands.py:4586— the pending-marker write in the extracted handler, which the old diff no longer covered. This one goes throughjson.dumps(ensure_ascii=True), so it is ASCII-safe today and the bytes on disk are unchanged; it is fixed to match the project's "always passencoding" policy and to stay safe ifensure_ascii=Falseis ever introduced.tests/gateway/test_update_encoding_footgun.py— the AST guard now scans both files, so the two halves of the/updatepath cannot regress independently. (Replaces the oldrun.py-only guard.)Why these aren't caught today
ruffPLW1514doesn't resolve.read_text()on a variable/attribute receiver —ruff check --select PLW1514 gateway/run.pyreports "All checks passed!" on the unfixed file.scripts/check-windows-footguns.pyscans builtinopen()only (its own comment: "…can be audited separately").Verification
Reproduced on Windows with
locale.getpreferredencoding() == 'cp1252':run.py:9122write reply"yes 🎉 更新"UnicodeEncodeError: 'charmap' codec can't encode '\U0001f389'— andisinstance(e, OSError)isFalse, soexcept OSErrormisses itrun.py:14671read UTF-8 stdoutОбновление→Ð�бновлениеruff checkon all three changed files → clean.python scripts/check-windows-footguns.py --all→ clean (760 files).tests/gateway/test_update_command.py,test_update_streaming.py,test_telegram_pending_update_probe.py,test_voice_command.pyproduce identical pass/fail counts before and after this change (the pre-existing failures are Windows-local dependency issues present on unmodifiedmaintoo).Scope
Limited to the two gateway-core files on the
/updatecoordination path. Complementary to #36828 (scripts/) and #50655 (hermes_cli/).Related (distinct) issues
#36649 / #36828 (
scripts/), #28579 (status.py::_read_json_file— missingexcept, already encoded), #34083 item 3 (subprocess stdout-pipe decode).