Skip to content

Fix/windows subprocess encoding - #52859

Closed
Ulaganathan-K-B wants to merge 4 commits into
NousResearch:mainfrom
Ulaganathan-K-B:fix/windows-subprocess-encoding
Closed

Fix/windows subprocess encoding#52859
Ulaganathan-K-B wants to merge 4 commits into
NousResearch:mainfrom
Ulaganathan-K-B:fix/windows-subprocess-encoding

Conversation

@Ulaganathan-K-B

Copy link
Copy Markdown

On Windows with non-UTF-8 locales (e.g. Chinese GBK/CP936), subprocess.Popen/subprocess.run(text=True) defaults to the system encoding. When a subprocess outputs UTF-8 characters (e.g. Chinese text), the internal _readerthread crashes, causing output pipe stalls, event loop blocks, WebSocket write timeouts, and gateway restarts. Error logs showed 1,830 identical crash entries.

This PR adds explicit encoding="utf-8", errors="replace" to all text-mode subprocess calls in tui_gateway/ so the output is decoded as UTF-8 regardless of the system locale. Invalid bytes are replaced with the Unicode replacement character instead of crashing.

Related Issue

Fixes #52846

Type of Change

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

Changes Made

  • tui_gateway/server.py: Added encoding="utf-8", errors="replace" to 4 subprocess calls:
    • _SlashWorker (line 272) — Popen for slash command subprocess
    • PDF rendering (line 8996) — pdftoppm subprocess
    • Quick command runner (line 11020) — CLI execution subprocess
    • Quick command exec (line 11085) — Shell command subprocess
    • shell.exec handler (line 13407) — Arbitrary shell command subprocess
  • tui_gateway/git_probe.py: Added encoding="utf-8", errors="replace" to 1 subprocess call:
    • run_git() (line 52) — Git probe subprocess

How to Test

  1. On a Windows machine with a non-UTF-8 locale (e.g. Chinese zh-CN, Russian ru-RU), set the system locale to the corresponding code page
  2. Trigger any of the affected subprocess paths:
    • Run a slash command in the TUI
    • Use a quick command that produces UTF-8 output
    • Execute a shell command via shell.exec that produces UTF-8 output
    • Trigger git probe functionality
  3. Verify the gateway does not crash and output is displayed correctly (non-ASCII characters appear properly or as ` replacement character, but never crash)

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 10 zh-CN

Documentation & Housekeeping

  • I've updated relevant documentation — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — encoding and errors` parameters are supported on all platforms; on Unix with UTF-8 locales this is a no-op
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Ulaganathan-K-B and others added 4 commits June 24, 2026 19:38
lowercase 'any' is Python's builtin function, not a type.
Adds Any to the typing import and corrects the annotation.
Fixes NousResearch#2139
On Windows with non-UTF-8 locales (e.g. Chinese GBK/CP936), subprocess
calls using text=True default to the system encoding. When a subprocess
outputs UTF-8 characters, the internal _readerthread crashes, causing
output pipe stalls, event loop blocks, and gateway restarts.

Add explicit encoding="utf-8", errors="replace" to all text-mode
subprocess calls in tui_gateway to prevent this.

Fixes NousResearch#52846

Co-Authored-By: Ulaganathan K.B <<EMAIL>>
@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) platform/windows Native Windows-specific behavior or breakage P2 Medium — degraded but workaround exists sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows duplicate This issue or pull request already exists labels Jun 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.
Duplicate of #52846 — both add encoding="utf-8", errors="replace" to the same tui_gateway/server.py and tui_gateway/git_probe.py subprocess calls to fix the Windows GBK/CP936 _readerthread crash. #52846 is the earlier open twin. This PR additionally bundles a trivial unrelated toolset_distributions.py type-hint cleanup. Maintainer should pick one; flagging as duplicate of the earlier twin.

@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 identifying the Windows text-decoding failure class. The five text=True subprocess paths in current tui_gateway/server.py still omit explicit decoding: _SlashWorker at tui_gateway/server.py:314-325, PDF attach at :9723-9726, cli.exec at :11796-11806, quick-command exec at :11865-11873, and shell.exec at :14416-14419.

Problems

  • tui_gateway/git_probe.py:52-61 already has encoding="utf-8" and errors="replace" on current main, so that PR hunk is stale.
  • toolset_distributions.py:214 is an unrelated type-hint cleanup and should not be bundled with this Windows fix.
  • The PR contains no regression tests. tests/tui_gateway/test_slash_worker_profile_home.py:16-35 already mocks subprocess.Popen and is a suitable place to assert the decoding kwargs; analogous RPC-path assertions should cover the subprocess.run call sites.

Suggested changes

  • During salvage, retain only the five still-needed tui_gateway/server.py edits, omit the already-landed git_probe.py hunk, and split out the annotation cleanup.
  • Add focused mock assertions for encoding="utf-8" and errors="replace".

Automated hermes-sweeper review.

Comment thread toolset_distributions.py
@@ -214,7 +214,7 @@
}

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 annotation correction is unrelated to the Windows subprocess-encoding fix. Please keep it in a separate focused change so this PR can be salvaged narrowly.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) duplicate This issue or pull request already exists 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants