Skip to content

fix(cli): add explicit encoding to read_text/write_text calls - #50655

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

fix(cli): add explicit encoding to read_text/write_text calls#50655
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix-encoding-read-write-text

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

Path.read_text() and Path.write_text() without explicit encoding default to the system locale encoding. On Windows this is typically cp1252, which causes UnicodeDecodeError for UTF-8 content (JSON configs, user data, service scripts).

Add encoding="utf-8" to all read_text() and write_text() calls across 8 CLI files, matching the pattern established in PR #50534 and ruff rule PLW1514.

Fixed files:

  • main.py: 4 read_text calls
  • auth.py: 3 read_text calls
  • banner.py: 1 read_text + 1 write_text
  • service_manager.py: 1 read_text + 4 write_text
  • container_boot.py: 1 read_text + 4 write_text
  • doctor.py: 3 read_text calls
  • uninstall.py: 2 read_text calls
  • gateway.py: 1 write_text call

Why this matters

Hermes runs on Windows, macOS, and Linux. On Windows, the default encoding for Path.read_text() is cp1252, not utf-8. JSON files written with UTF-8 content (emojis, non-ASCII usernames, i18n strings) will fail to read back on Windows without explicit encoding.

Test plan

  • CI passes
  • ruff check --select PLW1514 hermes_cli/ passes

Path.read_text() and Path.write_text() without explicit encoding
default to the system locale encoding. On Windows this is typically
cp1252, which causes UnicodeDecodeError for UTF-8 content (JSON
configs, user data, service scripts).

Add encoding="utf-8" to all read_text() and write_text() calls
across 8 CLI files, matching the pattern established in PR NousResearch#50534
(security_audit_startup.py) and ruff rule PLW1514.

Fixed files:
- main.py: 4 read_text calls
- auth.py: 3 read_text calls
- banner.py: 1 read_text + 1 write_text
- service_manager.py: 1 read_text + 4 write_text
- container_boot.py: 1 read_text + 4 write_text
- doctor.py: 3 read_text calls
- uninstall.py: 2 read_text calls
- gateway.py: 1 write_text call
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists labels Jun 22, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related: part of the systemic encoding='utf-8' rollout for bare read_text()/write_text() calls (Windows cp1252/GBK UnicodeDecodeError). Sibling PRs cover other subsystems: #37424 / #37647 (gateway run.py, issue #37423), #36828 (scripts, issue #36649). This PR covers the hermes_cli/* CLI files (main.py, auth.py, banner.py, service_manager.py, container_boot.py, doctor.py, uninstall.py, gateway.py) — a distinct file set, so it complements rather than duplicates those. (Note: the body cites PR #50534 as the pattern source, but #50534 is actually a terminal-PATH fix; the relevant precedent is ruff rule PLW1514 / #36649.)

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing the Windows locale issue. The underlying premise still holds on current main, but this commit needs targeted salvage before it can be used.

Problems

  • gh pr diff 50655 changes four already-encoded gateway writes into duplicate keyword arguments. Current main has valid single-keyword calls at hermes_cli/gateway.py:2997, :3179, :4003, and :4189; applying the PR versions would be invalid Python.
  • The current CLI surface has additional bare sibling calls that the commit does not cover: hermes_cli/container_boot.py:222,441,449,453,460, hermes_cli/service_manager.py:387,990,994,998,1005, and hermes_cli/uninstall.py:90.

Suggested changes

  • Preserve only the still-needed bare gateway write at hermes_cli/gateway.py:4139; drop the four duplicate-keyword gateway hunks.
  • Re-scan current hermes_cli/ for bare read_text()/write_text() calls and cover the remaining related service and uninstall paths.
  • Add a narrow non-ASCII persistence regression test for a representative read/write path.

Automated hermes-sweeper review.

Comment thread hermes_cli/gateway.py
@@ -2708,7 +2708,7 @@ def refresh_systemd_unit_if_needed(system: bool = False) -> bool:
if _refuse_temp_home_service_write(new_unit, "systemd unit"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This call already has encoding="utf-8" on current main (hermes_cli/gateway.py:2997). Adding it a second time produces duplicate keyword arguments and invalid Python; this hunk should be dropped during salvage.

@teknium1 teknium1 added 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 sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #71078 — your commit(s) were cherry-picked onto current main with your authorship preserved in git log (rebase merge). This PR was part of the class-wide close-out of bare read_text/write_text calls: all 139 remaining sites now pass explicit encoding, and a new CI linter rule prevents regressions. Thanks for the contribution!

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 P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants