Skip to content

fix(cli): tolerate undecodable alias lookup output - #48838

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

fix(cli): tolerate undecodable alias lookup output#48838
qdivan wants to merge 1 commit into
NousResearch:mainfrom
qdivan:fix/windows-subprocess-text-encoding

Conversation

@qdivan

@qdivan qdivan commented Jun 19, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes the current Windows/profile-alias decoding failure in #47939 without changing process-global subprocess behavior.

check_alias_collision() invokes where/which with text=True. If that command emits bytes that the platform-selected text codec cannot decode, subprocess.run() can raise UnicodeDecodeError before Hermes can handle the lookup result.

This patch adds errors="replace" only at that call site. It deliberately preserves Python's platform-default codec for localized Windows command output and avoids the previous import-time subprocess.Popen.__init__ monkeypatch.

Related Issue

Fixes #47939

Type of Change

  • 🐛 Bug fix
  • ✅ Tests
  • ✨ New feature
  • 🔒 Security fix
  • 📝 Documentation update
  • ♻️ Refactor

Changes Made

  • hermes_cli/profiles.py: tolerate undecodable bytes from the alias lookup subprocess with a localized errors="replace" policy.
  • tests/hermes_cli/test_profiles.py: exercise the real check_alias_collision() path with a subprocess that emits undecodable stderr.

Validation

  • Baseline RED on current origin/main: the targeted regression failed in check_alias_collision() with UnicodeDecodeError.
  • Updated branch GREEN: tests/hermes_cli/test_profiles.py156 passed.
  • Changed-file Ruff — passed.
  • git diff --check — passed.

Duplicate / overlap check

Checklist

  • Read the contributing and repository guidance.
  • Conventional commit message.
  • Searched current main, open PRs, and recently merged PRs for equivalent work.
  • Contains only the narrow fix and regression test.
  • Added behavior-focused regression coverage.
  • Considered Windows/localized subprocess behavior.
  • No config, dependency, generated-file, or documentation changes are required.

Screenshots / Logs

Not applicable; this is a CLI subprocess-decoding path with no UI changes.

@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 labels Jun 19, 2026

@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 — hardened subprocess text decoding.

  • 2 files changed (+59/-0)
  • Hardens text subprocess decoding for edge cases
  • No security concerns, no debug artifacts

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for targeting a real Windows decoding failure. The current-head reproduction path is still present at hermes_cli/profiles.py:407-410, where where/which is run with text=True and no error policy.

Problems

  • The proposed hermes_cli/__init__.py import-time replacement of subprocess.Popen.__init__ changes every later text subprocess in the process, including callers outside the reported profile-alias path. Current CLI code uses explicit policies at individual call sites, for example hermes_cli/tools_config.py:1069-1072.
  • tests/hermes_cli/test_ensure_utf8_locale.py only verifies injected kwargs through a mocked initializer. It does not cover check_alias_collision() or a real text-mode subprocess pipe.

Suggested changes

  • Apply the narrow decoding policy at hermes_cli/profiles.py:407-410 and add regression coverage for that call path.
  • Keep explicit caller overrides intact, but avoid a permanent process-global Popen monkeypatch.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added 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 sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 14, 2026
@qdivan
qdivan force-pushed the fix/windows-subprocess-text-encoding branch from 07df3ec to 2c3b8f2 Compare July 15, 2026 02:52
@qdivan

qdivan commented Jul 15, 2026

Copy link
Copy Markdown
Author

Updated in response to the latest hermes-sweeper review.

Changes:

  • removed the import-time global subprocess.Popen.__init__ monkeypatch entirely;
  • applied a localized errors="replace" policy only to check_alias_collision()'s where/which subprocess call, preserving the platform-default codec;
  • replaced the kwargs-only shim test with behavioral coverage of the real check_alias_collision() path using a subprocess that emits undecodable stderr.

Validation:

  • baseline RED on current origin/main: targeted regression failed with UnicodeDecodeError;
  • updated branch GREEN: 156 passed in tests/hermes_cli/test_profiles.py;
  • changed-file Ruff passed;
  • git diff --check passed.

I also refreshed duplicate/fix-existence checks. #55339 overlaps this production call as part of a 76-file sweep, but has no test files or check_alias_collision() regression; the other active subprocess PRs checked do not touch this path.

@qdivan
qdivan marked this pull request as ready for review July 15, 2026 02:52
@qdivan qdivan changed the title fix(cli): harden text subprocess decoding fix(cli): tolerate undecodable alias lookup output Jul 15, 2026
@qdivan

qdivan commented Jul 24, 2026

Copy link
Copy Markdown
Author

Closing this because current main now contains the equivalent fix via c89481db5ec2400e936de6ac402c89e7a39cfe19 (fix: add explicit UTF-8 encoding to all subprocess text=True calls (#53428)). The live check_alias_collision() call now uses encoding="utf-8", errors="replace", so this older conflicting branch is superseded. Thanks to the reviewers who helped narrow the original patch.

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 P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit 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.

[Bug]: BUG_WINDOWS_UNICODE_SUBPROCESS

4 participants