Skip to content

fix(cli): default Windows subprocess text decoding - #48839

Closed
rylena wants to merge 1 commit into
NousResearch:mainfrom
rylena:fix/issue-47939-windows-subprocess-decoding
Closed

fix(cli): default Windows subprocess text decoding#48839
rylena wants to merge 1 commit into
NousResearch:mainfrom
rylena:fix/issue-47939-windows-subprocess-decoding

Conversation

@rylena

@rylena rylena commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a Windows-only guard that defaults Hermes CLI text-mode subprocess pipes to UTF-8 with replacement decoding.
  • Preserves explicit caller-provided encoding / errors choices.
  • Covers text=True, universal_newlines=True, positional universal_newlines, and encoding/errors-triggered text mode.

Closes #47939

Verification

  • scripts/run_tests.sh tests/hermes_cli/test_windows_subprocess_text_decoding.py ✅ (6 passed)
  • python scripts/check-windows-footguns.py --all
  • Added-line static scan for secrets, shell injection, eval/exec, pickle, and SQL formatting: no findings
  • Independent delegated code review: passed

Codex was attempted first for repo-safe implementation, but it hit the known hidden-workspace / CA-bundle sandbox failures and could not inspect the checkout, so Hermes handled the fallback implementation and verification directly.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jun 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #48838 — both PRs fix #47939 by monkeypatching subprocess.Popen.__init__ in hermes_cli/__init__.py to default text-mode pipes to encoding="utf-8" / errors="replace" while preserving explicit caller choices. Same file, same issue, same mechanism (filed ~1 minute after #48838). The only difference is this PR gates the shim to sys.platform == "win32"; #48838 applies it cross-platform. Anchoring to the earliest still-open PR.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved — Windows subprocess text decoding fix.

  • 2 files changed (+187/-0)
  • Fixes default Windows subprocess text decoding
  • No security concerns, no debug artifacts

Reviewed by Hermes Agent

@rylena

rylena commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Closing this as a duplicate of #48838 for the same Windows subprocess text-decoding fix, so review effort stays concentrated on the earlier open PR.

@rylena rylena closed this Jun 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: BUG_WINDOWS_UNICODE_SUBPROCESS

3 participants