fix: add explicit UTF-8 encoding to all subprocess text=True calls (#53428) by Stoltemberg · Pull Request #55339 · NousResearch/hermes-agent

Stoltemberg · 2026-06-30T01:39:27Z

What does this PR do?

Fixes #53428 — adds explicit encoding='utf-8', errors='replace' to all subprocess.run() and subprocess.Popen() calls that use text=True across 146 non-test Python files.

Related Issue

Fixes #53428 (master tracker for Windows GBK locale crash)

Type of Change

🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

Added encoding='utf-8', errors='replace' to every subprocess.run(text=True) call in non-test files
Covers all 21+ crash sites identified in subprocess.run(text=True) without encoding param triggers GBK crash on Chinese Windows (21 locations) #53428
No changes to merge_text=True parameters (different API)
Preserves existing encoding= values where already specified

How to Test

Set Windows locale to Chinese (GBK) or any non-UTF-8 locale
Run hermes and exercise any tool that spawns subprocesses
Verify no UnicodeDecodeError crashes occur

Platforms Tested

Windows 11

Checklist

I have searched for existing PRs before submitting
I have tested my changes
My changes do not introduce new warnings

alt-glitch · 2026-06-30T01:52:04Z

This was generated by AI during triage.

Related: #53428 (master tracker) and #52249 (the focused open sibling fixing the same GBK/CP936 subprocess-encoding crash). This PR is the broadest rollout in that family.

Heads-up for reviewers: the diff is not subprocess-encoding-only. Beyond the encoding='utf-8', errors='replace' additions, it also bundles unrelated changes from the branch — agent/credential_pool.py adds credential-input sanitization (security), and apps/desktop/electron/windows-user-env.cjs adds an 8.3-short-path→long-path resolver (resolveLongPath/GetLongPathNameW, refs #38773/#52842). The genuine UTF-8 fix is real, but the actual fix likely needs isolating from the bundled cruft before merge — or maintainers may prefer the cleaner #52249 for that scope.

alt-glitch · 2026-06-30T01:52:51Z

@christian-byrne Tagging you on this ComfyUI item.

This was generated by AI during triage. This PR is a repo-wide subprocess-encoding sweep (encoding='utf-8' on text=True calls) that incidentally touches skills/creative/comfyui/scripts/auto_fix_deps.py and hardware_check.py — flagging per ComfyUI ownership in case the change affects those scripts.

tonydwb

COMMENT: High surface area (147 files, +670/-472). The encoding fix (adding encoding=utf-8, errors=replace to subprocess calls) is well-motivated and consistent. However, the credential_pool.py addition (sanitize_credential_input on add_entry) is a separate security concern bundled into the same PR. Recommend splitting: (1) the encoding fix across 146 files is mechanical and safe, (2) the credential sanitization in credential_pool.py should be its own PR for focused review. The two changes address different failure modes and have different risk profiles.

…ousResearch#53428) On Windows with Chinese locale (GBK), subprocess.run(text=True) without explicit encoding causes UnicodeDecodeError crashes. This fix adds encoding='utf-8', errors='replace' to all subprocess.run() and subprocess.Popen() calls that use text=True across 76 non-test Python files. Fixes NousResearch#53428 (master tracker for Windows GBK locale crash). Note: credential_pool.py and electron changes excluded per reviewer request — those will be submitted as separate focused PRs.

Stoltemberg · 2026-06-30T02:45:03Z

Thanks for the review! I've addressed your feedback:

Changes Made

Removed bundled changes — force-pushed a clean commit that contains ONLY the subprocess encoding fix (76 files, +276/-276). Removed:
- agent/credential_pool.py (credential sanitization) → will be a separate PR
- apps/desktop/electron/*.cjs (unrelated electron changes) → removed
- scripts/tests/*.ps1 (unrelated PowerShell test) → removed
Scope — The PR now purely addresses subprocess.run(text=True) without encoding param triggers GBK crash on Chinese Windows (21 locations) #53428: adding encoding='utf-8', errors='replace' to all subprocess.run(text=True) calls in non-test Python files.
Credential sanitization — The credential_pool.py input validation will be submitted as a separate focused PR for security review.

Verification

$ git diff --stat upstream/main..HEAD | tail -3
 76 files changed, 276 insertions(+), 276 deletions(+)

All changes are mechanical text=True → text=True, encoding='utf-8', errors='replace' substitutions.

tonydwb · 2026-06-30T03:07:49Z

Code Review Summary

Verdict: Comment — high surface area, human review recommended

This is a large mechanical change across 76 files adding explicit UTF-8 encoding to subprocess text=True calls. While each individual change is simple and correct, the sheer number of files warrants human review to ensure no unintended behavioral changes.

⚠️ High Surface Area

76 files changed — too large for automated review confidence
Mechanical transformation across the codebase
Recommend verifying no encoding-related regressions in subprocess-heavy paths

Reviewed by Hermes Agent

@alt-glitch

…esearch#53428) PR NousResearch#55339 adds encoding='utf-8', errors='replace' to 26 subprocess.run(text=True) call sites across the codebase. The triage review (thanks @alt-glitch) diffed this PR against NousResearch#55339 and found that 5 of the 6 originally-touched call sites are already covered there byte-identically: - hermes_cli/main.py::_probe_container - hermes_cli/setup.py SSH probe - tools/tts_tool.py::_generate_neutts - tools/transcription_tools.py::_prepare_local_audio - tools/transcription_tools.py::_transcribe_local_command (both branches) The one genuinely net-new site — hermes_cli/onepassword_secrets_cli.py::_op_whoami (the 1Password op CLI whoami probe) — is NOT in NousResearch#55339 and is fixed here. Without explicit encoding=, text=True decodes child output with locale.getpreferredencoding(False) — cp936 on Chinese Windows — which crashes _readerthread on non-GBK bytes, cascading into pipe buffer fills, event loop stalls, and TUI freezes (issues NousResearch#47939, NousResearch#53428, NousResearch#57238). Scope narrowed per triage feedback: the other 5 sites should land via NousResearch#55339. Refs NousResearch#53428 (together with NousResearch#55339).

teknium1 · 2026-07-24T18:46:21Z

Merged via PR #70875 — your commit was cherry-picked onto current main with your authorship preserved in git log (rebase merge). Thanks for the original codebase-wide sweep; it was the base of the class-closing PR. Your 61-site commit was de-conflicted against three weeks of main drift, and the salvage PR extended it with the remaining ~136 sites that had landed since, plus @jinglun010-cpu's linter rule (#60751) so the bug class can't regress. Credit noted in the PR body.

@alt-glitch

…esearch#53428) PR NousResearch#55339 adds encoding='utf-8', errors='replace' to 26 subprocess.run(text=True) call sites across the codebase. The triage review (thanks @alt-glitch) diffed this PR against NousResearch#55339 and found that 5 of the 6 originally-touched call sites are already covered there byte-identically: - hermes_cli/main.py::_probe_container - hermes_cli/setup.py SSH probe - tools/tts_tool.py::_generate_neutts - tools/transcription_tools.py::_prepare_local_audio - tools/transcription_tools.py::_transcribe_local_command (both branches) The one genuinely net-new site — hermes_cli/onepassword_secrets_cli.py::_op_whoami (the 1Password op CLI whoami probe) — is NOT in NousResearch#55339 and is fixed here. Without explicit encoding=, text=True decodes child output with locale.getpreferredencoding(False) — cp936 on Chinese Windows — which crashes _readerthread on non-GBK bytes, cascading into pipe buffer fills, event loop stalls, and TUI freezes (issues NousResearch#47939, NousResearch#53428, NousResearch#57238). Scope narrowed per triage feedback: the other 5 sites should land via NousResearch#55339. Refs NousResearch#53428 (together with NousResearch#55339).

…debase-wide AST-driven pass over every subprocess.run/Popen/check_output/check_call/call with text=True (or universal_newlines=True) and no explicit encoding=: append encoding='utf-8', errors='replace' at the kwarg site. 136 call sites across 28 files (cli.py, hermes_cli/main.py, tools_config.py, environments, computer_use, gateway, scripts, skills helpers, agent/*). Together with the salvaged NousResearch#55339/NousResearch#60741 commits this closes out issue NousResearch#53428's bug class; the salvaged NousResearch#60751 linter rule in check-windows-footguns.py now enforces it repo-wide (verified: 807 files scanned, zero findings).

… call sites, kwarg-snapshot tests - Strip the salvaged commit's inline encoding kwargs where main had since gained its own (process_registry, local env, cua doctor, gateway, commands, gateway_windows — the latter keeps its locale-aware _schtasks_encoding() from NousResearch#38186) - Revert encoding kwargs mistakenly applied to non-subprocess APIs (exa get_contents, tempfile.mkstemp in webhook.py) - Guard the ddgs worker Popen (new on main since NousResearch#55339) - Update two kwarg-snapshot test assertions for the new kwargs

@alt-glitch

…esearch#53428) PR NousResearch#55339 adds encoding='utf-8', errors='replace' to 26 subprocess.run(text=True) call sites across the codebase. The triage review (thanks @alt-glitch) diffed this PR against NousResearch#55339 and found that 5 of the 6 originally-touched call sites are already covered there byte-identically: - hermes_cli/main.py::_probe_container - hermes_cli/setup.py SSH probe - tools/tts_tool.py::_generate_neutts - tools/transcription_tools.py::_prepare_local_audio - tools/transcription_tools.py::_transcribe_local_command (both branches) The one genuinely net-new site — hermes_cli/onepassword_secrets_cli.py::_op_whoami (the 1Password op CLI whoami probe) — is NOT in NousResearch#55339 and is fixed here. Without explicit encoding=, text=True decodes child output with locale.getpreferredencoding(False) — cp936 on Chinese Windows — which crashes _readerthread on non-GBK bytes, cascading into pipe buffer fills, event loop stalls, and TUI freezes (issues NousResearch#47939, NousResearch#53428, NousResearch#57238). Scope narrowed per triage feedback: the other 5 sites should land via NousResearch#55339. Refs NousResearch#53428 (together with NousResearch#55339).

…debase-wide AST-driven pass over every subprocess.run/Popen/check_output/check_call/call with text=True (or universal_newlines=True) and no explicit encoding=: append encoding='utf-8', errors='replace' at the kwarg site. 136 call sites across 28 files (cli.py, hermes_cli/main.py, tools_config.py, environments, computer_use, gateway, scripts, skills helpers, agent/*). Together with the salvaged NousResearch#55339/NousResearch#60741 commits this closes out issue NousResearch#53428's bug class; the salvaged NousResearch#60751 linter rule in check-windows-footguns.py now enforces it repo-wide (verified: 807 files scanned, zero findings).

… call sites, kwarg-snapshot tests - Strip the salvaged commit's inline encoding kwargs where main had since gained its own (process_registry, local env, cua doctor, gateway, commands, gateway_windows — the latter keeps its locale-aware _schtasks_encoding() from NousResearch#38186) - Revert encoding kwargs mistakenly applied to non-subprocess APIs (exa get_contents, tempfile.mkstemp in webhook.py) - Guard the ddgs worker Popen (new on main since NousResearch#55339) - Update two kwarg-snapshot test assertions for the new kwargs

@alt-glitch

…esearch#53428) PR NousResearch#55339 adds encoding='utf-8', errors='replace' to 26 subprocess.run(text=True) call sites across the codebase. The triage review (thanks @alt-glitch) diffed this PR against NousResearch#55339 and found that 5 of the 6 originally-touched call sites are already covered there byte-identically: - hermes_cli/main.py::_probe_container - hermes_cli/setup.py SSH probe - tools/tts_tool.py::_generate_neutts - tools/transcription_tools.py::_prepare_local_audio - tools/transcription_tools.py::_transcribe_local_command (both branches) The one genuinely net-new site — hermes_cli/onepassword_secrets_cli.py::_op_whoami (the 1Password op CLI whoami probe) — is NOT in NousResearch#55339 and is fixed here. Without explicit encoding=, text=True decodes child output with locale.getpreferredencoding(False) — cp936 on Chinese Windows — which crashes _readerthread on non-GBK bytes, cascading into pipe buffer fills, event loop stalls, and TUI freezes (issues NousResearch#47939, NousResearch#53428, NousResearch#57238). Scope narrowed per triage feedback: the other 5 sites should land via NousResearch#55339. Refs NousResearch#53428 (together with NousResearch#55339).

…debase-wide AST-driven pass over every subprocess.run/Popen/check_output/check_call/call with text=True (or universal_newlines=True) and no explicit encoding=: append encoding='utf-8', errors='replace' at the kwarg site. 136 call sites across 28 files (cli.py, hermes_cli/main.py, tools_config.py, environments, computer_use, gateway, scripts, skills helpers, agent/*). Together with the salvaged NousResearch#55339/NousResearch#60741 commits this closes out issue NousResearch#53428's bug class; the salvaged NousResearch#60751 linter rule in check-windows-footguns.py now enforces it repo-wide (verified: 807 files scanned, zero findings).

… call sites, kwarg-snapshot tests - Strip the salvaged commit's inline encoding kwargs where main had since gained its own (process_registry, local env, cua doctor, gateway, commands, gateway_windows — the latter keeps its locale-aware _schtasks_encoding() from NousResearch#38186) - Revert encoding kwargs mistakenly applied to non-subprocess APIs (exa get_contents, tempfile.mkstemp in webhook.py) - Guard the ddgs worker Popen (new on main since NousResearch#55339) - Update two kwarg-snapshot test assertions for the new kwargs

tonydwb reviewed Jun 30, 2026

View reviewed changes

Stoltemberg force-pushed the fix/subprocess-gbk-encoding branch from 5f97c1e to 3e75bd9 Compare June 30, 2026 02:44

Stoltemberg mentioned this pull request Jun 30, 2026

fix: TUI gateway crash on Windows - UnicodeDecodeError in subprocess _readerthread #53137

Closed

This was referenced Jul 8, 2026

fix: add explicit UTF-8 encoding to remaining subprocess text=True calls (#53428) #60741

Closed

feat(linter): detect subprocess text=True without explicit encoding= #60751

Closed

Sahil-SS9 mentioned this pull request Jul 10, 2026

fix(install): normalize accented-username 8.3 short paths (GH #52842) #61833

Closed

qdivan mentioned this pull request Jul 15, 2026

fix(cli): tolerate undecodable alias lookup output #48838

Closed

13 tasks

teknium1 mentioned this pull request Jul 15, 2026

fix: add explicit encoding="utf-8" to subprocess.run calls with text=True #52249

Closed

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

Navigation Menu

Uh oh!

fix: add explicit UTF-8 encoding to all subprocess text=True calls (#53428) - #55339

fix: add explicit UTF-8 encoding to all subprocess text=True calls (#53428)#55339
Stoltemberg wants to merge 1 commit into
NousResearch:mainfrom
Stoltemberg:fix/subprocess-gbk-encoding

Stoltemberg commented Jun 30, 2026

Uh oh!

alt-glitch commented Jun 30, 2026

Uh oh!

alt-glitch commented Jun 30, 2026

Uh oh!

tonydwb left a comment

Uh oh!

Stoltemberg commented Jun 30, 2026

Uh oh!

tonydwb commented Jun 30, 2026

Uh oh!

teknium1 commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Stoltemberg commented Jun 30, 2026

What does this PR do?

Related Issue

Type of Change

Changes Made

How to Test

Platforms Tested

Checklist

Uh oh!

alt-glitch commented Jun 30, 2026

Uh oh!

alt-glitch commented Jun 30, 2026

Uh oh!

tonydwb left a comment

Choose a reason for hiding this comment

Uh oh!

Stoltemberg commented Jun 30, 2026

Changes Made

Verification

Uh oh!

tonydwb commented Jun 30, 2026

Code Review Summary

⚠️ High Surface Area

Uh oh!

teknium1 commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants