Skip to content

fix: add encoding="utf-8" to Path.write_text() calls (P1) - #56385

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

fix: add encoding="utf-8" to Path.write_text() calls (P1)#56385
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/write-text-encoding-utf8

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

Add explicit encoding="utf-8" to 39 Path.write_text() calls across 16 files.

Problem

Path.write_text() without encoding defaults to system locale. On Windows (cp1252), this corrupts non-ASCII content. Same root cause as PLW1514 but for the write path.

This is the write-side counterpart to the read_text() encoding fix (PR #56115).

Changes (16 files, 39 instances)

File Count
tools/skills_hub.py 8
hermes_cli/service_manager.py 5
hermes_cli/container_boot.py 5
gateway/run.py 5
hermes_cli/profiles.py 3
gateway/delivery.py 2
hermes_cli/main.py 2
agent/copilot_acp_client.py 1
tools/web_tools.py 1
tools/xai_http.py 1
gateway/slash_commands.py 1
gateway/dead_targets.py 1
gateway/platforms/qqbot/adapter.py 1
hermes_cli/gateway.py 1
hermes_cli/banner.py 1
hermes_cli/uninstall.py 1

Testing

All 16 files pass py_compile verification. Zero duplicate encodings.

Path.write_text() without encoding defaults to system locale encoding.
On Windows (cp1252), this silently corrupts non-ASCII content written
to JSON files, config files, and cache files.

This is the write-side counterpart to the read_text() encoding fix
(PR NousResearch#56115). PLW1514 only covers open() calls — Path methods are
unguarded by ruff.

39 instances across 16 files, all passing py_compile.

Files changed:
- agent/copilot_acp_client.py (1)
- tools/web_tools.py (1)
- tools/xai_http.py (1)
- tools/skills_hub.py (8)
- gateway/slash_commands.py (1)
- gateway/run.py (5)
- gateway/dead_targets.py (1)
- gateway/delivery.py (2)
- gateway/platforms/qqbot/adapter.py (1)
- hermes_cli/gateway.py (1)
- hermes_cli/banner.py (1)
- hermes_cli/service_manager.py (5)
- hermes_cli/container_boot.py (5)
- hermes_cli/uninstall.py (1)
- hermes_cli/main.py (2)
- hermes_cli/profiles.py (3)
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets 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 sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 1, 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.

LGTM — explicit encoding='utf-8' for Path.write_text() across 16 files. This is a defensive fix for non-UTF-8 locales (Windows especially). Each change adds the encoding parameter to a write_text call — no behavioral difference on UTF-8 systems, but prevents UnicodeDecodeError on systems with non-UTF-8 default encoding. File count is 16 (borderline) but the changes are mechanical and safe.

@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: LGTM

Gates read-dedup and staleness detection to local backend only. Prevents false positives on remote backends where host mtime is meaningless.

Looks Good

  • _is_host_local_env cleanly checks both active environment and config fallback
  • Read-dedup correctly skips on remote backends (prevents serving stale content)
  • Staleness warning correctly skips on remote backends (prevents false positives)
  • Comprehensive test coverage: 6 tests covering local, remote, config fallback, and exception cases

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused cross-platform cleanup. The current-main call sites still use locale-default Path.write_text()—for example, agent/copilot_acp_client.py:734 and gateway/delivery.py:360—so the central premise remains valid. The explicit UTF-8 arguments are mechanically appropriate.

Suggested changes

  • Please add one focused non-ASCII persistence regression test. This PR modifies no test files, and GitHub currently reports no checks for the branch. A representative cache or delivery path would validate the Windows-specific behavior described by the change.
  • When salvaging, reapply against current main: 11 touched files changed since the PR base, including gateway/run.py, hermes_cli/main.py, hermes_cli/profiles.py, tools/skills_hub.py, and tools/web_tools.py. The tools/web_tools.py hunk is correctly a stale-comment correction; the underlying writer already uses encoding="utf-8" on current main at tools/web_tools.py:509.

Automated hermes-sweeper review.

@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

Merged via PR #71078 — your commit was cherry-picked onto current main with your authorship preserved in git log (rebase merge). Your 5-PR series (#50655 was authored under your earlier account, #54241, #56385, #66856, #65440) formed the backbone of the class-wide close-out: 68 of the 139 bare sites came from your commits, and the campaign's structure followed your directory-by-directory split. The remaining 71 sites were swept on top and a CI linter rule now prevents regressions. Thanks for the sustained, methodical work on this class.

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 comp/gateway Gateway runner, session dispatch, delivery 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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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