Skip to content

fix(shell_hooks): use explicit UTF-8 encoding with errors='replace' - #64374

Closed
seasonmsg wants to merge 1 commit into
NousResearch:mainfrom
seasonmsg:pr/shell-hooks-encoding
Closed

fix(shell_hooks): use explicit UTF-8 encoding with errors='replace'#64374
seasonmsg wants to merge 1 commit into
NousResearch:mainfrom
seasonmsg:pr/shell-hooks-encoding

Conversation

@seasonmsg

Copy link
Copy Markdown

Problem

On Windows, text=True in subprocess.run() defaults to the system's ANSI codepage (e.g., GBK/CP936 on Chinese Windows). When shell hook scripts output characters outside this codepage, subprocess.run raises UnicodeDecodeError, causing the hook to silently fail.

Fix

Replace text=True with explicit encoding='utf-8', errors='replace' in _spawn() (shell_hooks.py:467).

  • encoding='utf-8': consistent cross-platform behavior
  • errors='replace': graceful degradation instead of crash on invalid bytes

Before/After

# Before
subprocess.run(argv, input=stdin_json, capture_output=True,
               timeout=spec.timeout, text=True, shell=False, **_popen_kwargs)

# After
subprocess.run(argv, input=stdin_json, capture_output=True,
               timeout=spec.timeout, encoding='utf-8', errors='replace',
               shell=False, **_popen_kwargs)

Testing

  • Verified that _spawn() no longer crashes on non-ASCII hook output on Windows (Chinese locale)
  • Backward compatible: encoding='utf-8' + errors='replace' is a superset of text=True behavior on UTF-8 systems

Replace text=True (which uses locale encoding on Windows, e.g. GBK)
with explicit encoding='utf-8', errors='replace' to prevent
UnicodeDecodeError when shell hooks output non-ASCII characters.

text=True on Windows defaults to the system's ANSI codepage (e.g.
GBK/CP936 on Chinese Windows), causing silent failures when hook
output contains characters outside that codepage.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for targeting the shared shell-hook subprocess bridge. The reported call remains on current main at agent/shell_hooks.py:462-470, and this is a small, focused fix.

Problems

  • The PR adds no regression test for the new encoding contract. The existing Windows hook test at tests/test_windows_subprocess_no_window_flags.py:264-283 verifies only creationflags.
  • The public hook protocol documents JSON stdin/stdout at website/docs/user-guide/features/hooks.md:1325-1361, but does not state an encoding. Replacing locale decoding with UTF-8 changes behavior for hooks that emit locale-encoded JSON, so the backward-compatibility claim needs an explicit protocol decision.

Suggested changes

  • Add a focused captured-kwargs test asserting encoding='utf-8', errors='replace', and no text=True.
  • Document UTF-8 as the hook wire encoding, or preserve locale-encoded-hook compatibility if that remains supported.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 16, 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

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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