Skip to content

fix(tui-gateway): decode subprocess output as UTF-8 to stop cp950 crashes - #52700

Closed
devorun wants to merge 1 commit into
NousResearch:mainfrom
devorun:fix/tui-gateway-subprocess-utf8-52649
Closed

fix(tui-gateway): decode subprocess output as UTF-8 to stop cp950 crashes#52700
devorun wants to merge 1 commit into
NousResearch:mainfrom
devorun:fix/tui-gateway-subprocess-utf8-52649

Conversation

@devorun

@devorun devorun commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses #52649. On non-UTF-8 Windows consoles (e.g. cp950 / zh-TW) the text-mode subprocess I/O in tui_gateway/server.py decoded child output with the OS locale codepage, not UTF-8. Since Hermes (and git) emit UTF-8, any non-ASCII byte raised UnicodeDecodeError inside the subprocess reader threads — exactly the threads named in the report:

  • _drain_stdout / _drain_stderr — the slash-worker Popen(..., text=True) (no encoding)
  • _readerthread — the subprocess.run(..., capture_output=True, text=True) calls in _git_branch_for_cwd, which run on the periodic session-status refresh (hence the fixed ~580 s crash cadence)

A crashed reader thread stalls the worker → the watchdog sees a stale log → kills and respawns (with a brief console popup), 10–20×/hour.

Fix

Pass encoding="utf-8", errors="replace" to:

  • the slash-worker subprocess.Popen
  • both subprocess.run calls in _git_branch_for_cwd

errors="replace" means a stray byte can never crash the stream, and UTF-8 is what these children actually emit. No behavior change on UTF-8 consoles.

Scope note: this fixes the subprocess sites in tui_gateway/server.py that match the reported crash threads. Other text-mode subprocess reads elsewhere could warrant the same hardening, but are out of scope here.

Tests

tests/tui_gateway/test_subprocess_encoding.py: the _git_branch_for_cwd git lookups are invoked with encoding="utf-8" and errors="replace". Runs on any platform (no cp950 console required).

…shes

On non-UTF-8 Windows consoles (e.g. cp950 / zh-TW) the text-mode subprocess
I/O in tui_gateway decoded child output with the OS locale codepage. Any
non-ASCII byte (Hermes emits UTF-8) then raised UnicodeDecodeError inside the
reader threads — the `_drain_stdout` / `_drain_stderr` slash-worker threads and
the `_readerthread` threads behind the periodic `git branch` / `git rev-parse`
status lookups — killing them and stalling the gateway on a fixed cadence,
which the watchdog then respawns (NousResearch#52649).

Pass `encoding="utf-8", errors="replace"` to the slash-worker `Popen` and both
`_git_branch_for_cwd` `subprocess.run` calls. A stray byte can no longer crash
the stream; Hermes/git output decodes normally.

Adds a test asserting the git-branch lookups decode as UTF-8 with replacement.
@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 labels Jun 25, 2026

@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 isolating the locale-dependent decoder failure. The slash-worker premise still holds on current main: tui_gateway/server.py:314-325 uses text=True without encoding or errors, so adding the proposed Popen arguments is sound.

Problems

  • The patch leaves same-class current-main captures unprotected at tui_gateway/server.py:9723-9726, 11796-11806, 11865-11873, and 14416-14419. Each uses text=True and captures child output without a decoding policy.
  • The new regression test only covers git lookup arguments. That path is already hardened in tui_gateway/git_probe.py:52-62 and covered by tests/test_windows_subprocess_no_window_flags.py:38-68; it does not verify the slash-worker Popen change.

Suggested changes

  • Carry the explicit UTF-8/replacement policy through the remaining text-mode captures above.
  • Add a Popen-focused test for _SlashWorker, then cover each additionally hardened capture.

This is an automated hermes-sweeper review.

"""

from unittest.mock import MagicMock, patch

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 only exercises the git path, which current main already hardens and tests through tui_gateway.git_probe. Please add a focused assertion for _SlashWorker's subprocess.Popen kwargs, because that is the still-unfixed production path this PR changes.

@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 with credit: this fix shipped in PR #70850 (merged, commit ead23aa). You were the earliest submitter of the _SlashWorker utf-8/replace hardening (Jun 25) — the merged salvage was based on #61978, whose regression tests explicitly adapted your test pattern, and the pre-merge duplicate sweep missed that your PR predated it. Apologies for that; noting first-submitter credit here for the record.

Your second site (_git_branch_for_cwd) has since been replaced on main by git_probe.branchbounded_git_probe, which already decodes utf-8 with errors="replace", so nothing further to salvage there. Thanks for the clean fix and the report analysis on #52649.

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/) 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