Skip to content

fix: add explicit UTF-8 encoding to Path.read_text() for JSON files (P1) - #56321

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

fix: add explicit UTF-8 encoding to Path.read_text() for JSON files (P1)#56321
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/read-text-encoding-json

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

Add explicit encoding="utf-8" to 29 Path.read_text() calls that feed into json.loads() across 14 files.

Problem

Path.read_text() without an encoding parameter defaults to the system locale encoding. On Windows with cp1252 locale (common on US-locale installs), this silently corrupts any non-ASCII content in JSON files, which are always UTF-8 per RFC 8259.

This exact issue caused at least 3 separate Windows sandbox regressions before PLW1514 was added to ruff config. However, PLW1514 only covers open() calls — Path.read_text() was left unguarded.

# BAD — defaults to cp1252 on Windows, corrupts non-ASCII:
data = json.loads(path.read_text())

# GOOD — explicit UTF-8, matches JSON spec:
data = json.loads(path.read_text(encoding="utf-8"))

Changes (14 files, 29 instances)

File Count
gateway/run.py 9
tools/skills_hub.py 6
hermes_cli/auth.py 3
agent/auxiliary_client.py 1
agent/shell_hooks.py 1
gateway/dead_targets.py 1
hermes_cli/banner.py 1
hermes_cli/container_boot.py 1
hermes_cli/doctor.py 1
hermes_cli/main.py 1
hermes_cli/service_manager.py 1
tools/managed_tool_gateway.py 1
tools/skills_sync.py 1
tools/xai_http.py 1

Testing

All 14 files pass python3 -m py_compile.

Path.read_text() without encoding defaults to the system locale encoding.
On Windows with cp1252 locale, this silently corrupts non-ASCII content
in JSON files (which are always UTF-8 per RFC 8259).

This caused at least 3 separate Windows sandbox regressions before the
PLW1514 rule was added for open() calls. However, Path.read_text() was
not covered by that rule, leaving 29 instances vulnerable.

Fix all 29 json.loads(Path.read_text()) calls across 14 files to
explicitly pass encoding="utf-8".

Files changed:
- agent/auxiliary_client.py (1)
- agent/shell_hooks.py (1)
- tools/skills_sync.py (1)
- tools/managed_tool_gateway.py (1)
- tools/xai_http.py (1)
- tools/skills_hub.py (6)
- gateway/run.py (9)
- gateway/dead_targets.py (1)
- hermes_cli/banner.py (1)
- hermes_cli/service_manager.py (1)
- hermes_cli/container_boot.py (1)
- hermes_cli/doctor.py (1)
- hermes_cli/main.py (1)
- hermes_cli/auth.py (3)
@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/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets area/config Config system, migrations, profiles 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 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.read_text() across 14 files. Same defensive fix as #56385 (write_text), applied to read_text calls. Prevents UnicodeDecodeError on non-UTF-8 locales. Mechanical and safe.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused cross-platform fix. Current main still has the unencoded JSON readers this PR changes, including hermes_cli/auth.py:1087, gateway/run.py:3227, and tools/skills_hub.py:1137; the replacement with encoding="utf-8" matches the repository policy in pyproject.toml:381-386.

Problems

  • The diff contains no test files (gh pr diff 56321 --name-only); py_compile cannot exercise locale-dependent decoding behavior.

Suggested changes

  • Add one focused regression test using UTF-8 JSON with non-ASCII content and a representative production loader such as _load_auth_store (hermes_cli/auth.py:1081).

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label 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

area/config Config system, migrations, profiles 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/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants