Skip to content

fix: add explicit UTF-8 encoding to Path.read_text()/write_text() (P1 — silent data corruption on Windows) - #58840

Closed
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/path-read-text-encoding
Closed

fix: add explicit UTF-8 encoding to Path.read_text()/write_text() (P1 — silent data corruption on Windows)#58840
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/path-read-text-encoding

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

Add explicit encoding="utf-8" to all Path.read_text() and Path.write_text() calls that omit it.

Problem

Path.read_text() and Path.write_text() without the encoding parameter default to the system locale encoding. On Windows this is typically cp1252, which silently corrupts non-ASCII content (accented characters, CJK, emoji, etc.) when the file actually contains UTF-8 data.

This is NOT caught by ruff rule PLW1514 (which only flags bare open() calls) and is a common source of cross-platform bugs.

Changes

58 files fixed across the entire codebase:

  • Path.read_text() -> Path.read_text(encoding="utf-8") (21 files)
  • Path.write_text(...) -> Path.write_text(..., encoding="utf-8") (58 files total)

Key areas:

  • gateway/run.py (9 read_text calls)
  • tools/skills_hub.py (read + write)
  • hermes_cli/ (auth, main, profiles, service_manager, etc.)
  • plugins/platforms/ (discord, telegram, slack, whatsapp, feishu)
  • agent/ (auxiliary_client, copilot_acp_client, curator)

Impact

  • P1: Silent data corruption on Windows for any file containing non-ASCII text
  • JSON config files, session data, skill definitions, and logs are all affected
  • UTF-8 is the correct encoding per RFC 8259 (JSON) and W3C standards

Testing

All 58 changed files pass python3 -m py_compile verification.

… calls (P1)

Path.read_text() and write_text() without encoding parameter default
to the system locale encoding (cp1252 on Windows, UTF-8 on Linux).
This causes silent data corruption when reading/writing non-ASCII
content on Windows systems.

This is NOT caught by ruff PLW1514 (which only flags open() calls)
and is a common source of cross-platform bugs.

Fixed:
- 21 files with .read_text() -> .read_text(encoding="utf-8")
- 58 files total including .write_text() -> .write_text(..., encoding="utf-8")

Key areas:
- gateway/run.py (9 read_text calls)
- tools/skills_hub.py (read + write)
- hermes_cli/auth.py, hermes_cli/main.py
- plugins/platforms/ (discord, telegram, slack, whatsapp, feishu)
- agent/ (auxiliary_client, copilot_acp_client, curator)
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/gateway Gateway runner, session dispatch, delivery comp/cli CLI entry point, hermes_cli/, setup wizard comp/tools Tool registry, model_tools, toolsets comp/plugins Plugin system and bundled plugins tool/skills Skills system (list, view, manage) platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows P3 Low — cosmetic, nice to have labels Jul 5, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment (LGTM)

P1 critical fix: adds explicit encoding="utf-8" to all Path.read_text() and Path.write_text() calls across 58 files. This prevents silent data corruption on Windows where the default encoding is platform-dependent (often cp1252). The fix is mechanical and thorough — every instance was updated, not just the ones that happened to fail on Windows in testing.

What looks good:

  • Comprehensive sweep covering all Path I/O in the codebase
  • Backwards compatible (utf-8 is what the files already are in practice)
  • Prevents a real silent-corruption class of bugs

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as resolved by PR #71078 (merged, commit d372fda): the class-wide close-out salvaged your #50655/#54241/#56385/#66856/#65440 series as the backbone (authorship preserved in git log) and swept the remaining sites, so every read_text/write_text call this PR touches is now guarded on current main — verified per-site. A CI linter rule in check-windows-footguns.py plus the AST guard test now prevent regressions. Your overlapping/split variants of the same series are being closed together; the credit for the class rests on your commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants