Skip to content

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

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

fix: add explicit encoding="utf-8" to subprocess.run calls with text=True#52249
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/subprocess-text-encoding

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Problem

subprocess.run(..., text=True) without encoding= defaults to the system's locale encoding. On non-UTF-8 locales (Windows with CP1252, Linux with C locale), this can produce garbled output or UnicodeDecodeError.

# Current — uses locale encoding (may not be UTF-8)
result = subprocess.run(cmd, capture_output=True, text=True, timeout=120)

# Fixed — explicitly uses UTF-8
result = subprocess.run(cmd, capture_output=True, text=True, encoding="utf-8", timeout=120)

Files changed (11 fixes in 8 files)

File Context
skills/creative/comfyui/scripts/auto_fix_deps.py dependency install
tools/tts_tool.py TTS engine
tools/transcription_tools.py transcription (3 calls)
tools/environments/singularity.py Singularity container
tools/voice_mode.py voice mode (2 calls)
hermes_cli/setup.py SSH setup
hermes_cli/main.py CLI command runner
tui_gateway/server.py TUI gateway

All changes verified with py_compile.

…True

subprocess.run(..., text=True) without encoding= defaults to the
system's locale encoding. On non-UTF-8 locales (e.g. Windows with
CP1252, Linux with C locale), this can produce garbled output or
UnicodeDecodeError.

Add explicit encoding="utf-8" to all subprocess.run calls that use
text=True without specifying encoding.

Fixes 11 occurrences across 8 files.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard comp/tui Terminal UI (ui-tui/ + tui_gateway/) tool/tts Text-to-speech and transcription backend/singularity Singularity container execution P2 Medium — degraded but workaround exists labels Jun 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to the ongoing Windows/non-UTF-8 subprocess-encoding rollout (#25200 helper, #43494 gateway, #45144 Copilot ACP, #49564 doctor) — this PR covers additional unguarded subprocess.run(text=True) sites, so it's complementary rather than a duplicate. Note it adds encoding="utf-8" but not errors="replace" that some sibling PRs use.

@christian-byrne Tagging you on this ComfyUI item (it touches skills/creative/comfyui/scripts/auto_fix_deps.py).

@AlexFucuson9

Copy link
Copy Markdown
Contributor Author

This PR adds encoding='utf-8' to subprocess.run() calls with text=True that are missing it. This is part of the broader Windows encoding fix effort. The change is minimal and targeted — only adding the missing encoding parameter to prevent UnicodeDecodeError on non-UTF-8 Windows locales.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the targeted Windows-locale fix. The premise remains valid on current main: hermes_cli/main.py:1184, hermes_cli/setup.py:1430, tools/transcription_tools.py:1191, tools/tts_tool.py:1884, and tui_gateway/server.py:9723-9725 still use locale-decoded text=True subprocess output without an explicit encoding.

Problems

  • GitHub reports this PR as conflicting. The current PDF-rendering call is at tui_gateway/server.py:9723-9725 and now includes creationflags=windows_hide_flags(); preserve that while applying the encoding change.
  • The patch is incomplete for its stated bug class. The touched Singularity backend has additional unguarded calls at tools/environments/singularity.py:47-50, 137-141, and 252-256; the TUI gateway also has runtime paths at tui_gateway/server.py:11796-11806, 11865-11873, and 14416-14419.
  • The diff adds no regression coverage; py_compile only validates syntax.

Suggested changes

Automated hermes-sweeper review.

@alt-glitch alt-glitch added platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 15, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as resolved by PR #70875 (merged, commit 0f732cb), which closed out this bug class codebase-wide: every text=True subprocess call now passes encoding="utf-8", errors="replace", and a CI linter rule (scripts/check-windows-footguns.py) rejects any future unguarded site. The sites this PR targeted are all guarded on current main — verified per-file. Credit for the class fix goes to @Stoltemberg (#55339, the original sweep) and @jinglun010-cpu (#60741 + the #60751 linter); thanks for your fix as well — the volume of independent PRs on this bug is what escalated it to a class-wide close-out.

@teknium1 teknium1 closed this Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/singularity Singularity container execution comp/cli CLI entry point, hermes_cli/, setup wizard comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage 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 tool/tts Text-to-speech and transcription type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants