fix(ssh): tolerate non-UTF-8 subprocess output (errors="replace", encoding="utf-8")

[Chrisyk]

What does this PR do?

The SSH backend crashed with UnicodeDecodeError during _ensure_remote_dirs() (and other init steps) when internal SSH subprocesses emitted non-UTF-8 bytes, which is common on Windows OpenSSH/Git Bash targets. The code used subprocess.run(..., text=True) with strict UTF-8 decoding, so a single bad byte bricked all terminal/file/search tools before any user command ran.

This PR makes all text-mode subprocess decoding in the SSH and base execution backends tolerant and deterministic by passing errors="replace" and encoding="utf-8", so that malformed bytes are replaced with U+FFFD instead of raising an exception.

Related Issue

I originally filed and claimed issue #37130, so I'm opening this PR since I was already working on the fix.

Fixes #37130

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• tools/environments/ssh.py: added errors="replace", encoding="utf-8" to all 7 text-mode subprocess.run(...) calls _establish_connection, _detect_remote_home, _ensure_remote_dirs, _scp_upload (mkdir + scp), _ssh_bulk_upload (parent mkdir), and _ssh_delete. (the encoding="utf-8" pin is adapted from fix: decode SSH subprocess output tolerantly #37265 / honor2030)
• tools/environments/base.py: added errors="replace", encoding="utf-8" to the _popen_bash subprocess.Popen(..., text=True) call. A nearby comment already claimed the TextIOWrapper was built with errors="replace", encoding="utf-8", but the code never set it, leaving the fallback drain path with the same latent crash; the code now matches the documented intent.
• tests/tools/test_ssh_environment.py: added TestNonUTF8SubprocessOutput (5 regression tests). A faithful subprocess.run stand-in that decodes raw bytes exactly as real Python does, reproducing the original UnicodeDecodeError on unfixed code. Covers full __init__, non-UTF-8 on stderr, _detect_remote_home replacement-char output, _ensure_remote_dirs, and a contract test asserting every text-mode run call (including the file-sync sites __init__ doesn't reach) passes errors="replace", encoding="utf-8".
• tests/tools/test_base_environment.py: added TestPopenBashDecoding (1 regression test) that spawns a real subprocess emitting \xff\xfe and asserts the output reads as U+FFFD rather than crashing.

How to Test

1. Check out this branch and run the targeted suites: pytest tests/tools/test_ssh_environment.py tests/tools/test_base_environment.py -q → all pass (live-SSH integration tests skip without TERMINAL_SSH_HOST).
2. Confirm the tests are genuine regressions: temporarily remove the errors="replace", encoding="utf-8" arguments from tools/environments/ssh.py and tools/environments/base.py, re-run the two suites, and observe the 6 new
   tests fail with UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff. Restore the fix, and they pass again.
3. (Optional, real host) Point an SSH backend at a Windows OpenSSH/Git Bash target that emits non-UTF-8 bytes on startup and confirm tools initialize instead of crashing in _ensure_remote_dirs().

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: ran pytest tests/tools -q (the changed area): pass. Full suite skipped: local collection errors are unrelated, missing optional deps (acp, aiohttp) on a Python 3.14/ARM box, not the changed code. CI covers the full run.
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: PiKVM (Arch Linux) -- the 6 new tests pass
• live-SSH integration test -- 10/11 integration tests pass against a real Windows OpenSSH host; the remaining one is a Windows session-persistence quirk that exists on main too. (note: had to switch to bash.exe shell instead of cmd.exe)

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — 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
  Specifically targets Windows OpenSSH/Git Bash byte output
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Regression tests failing on unfixed code (fix temporarily removed):

  FAILED tests/tools/test_ssh_environment.py::TestNonUTF8SubprocessOutput::test_init_survives_non_utf8_output
  FAILED tests/tools/test_ssh_environment.py::TestNonUTF8SubprocessOutput::test_init_survives_non_utf8_on_stderr
  FAILED tests/tools/test_ssh_environment.py::TestNonUTF8SubprocessOutput::test_detect_home_replaces_bad_bytes
  FAILED tests/tools/test_ssh_environment.py::TestNonUTF8SubprocessOutput::test_ensure_remote_dirs_does_not_raise
  FAILED tests/tools/test_ssh_environment.py::TestNonUTF8SubprocessOutput::test_every_text_run_call_uses_errors_replace
  FAILED tests/tools/test_base_environment.py::TestPopenBashDecoding::test_non_utf8_output_is_replaced_not_raised
  E   UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte
  6 failed

Same suites with the fix in place (11 skipped -- Live-SSH tests):

  38 passed, 11 skipped in 2.55s

Live-SSH Integration Test (10 Success, 1 Failure):

[root@pikvm g:hermes-agent:main:/]# python -m pytest tests/tools/test_ssh_environment.py::TestOneShotSSH tests/tools/test_ssh_environment.py::TestPersistentSSH -q -o 'addopts='
..F........                                                                                                                                           [100%]
========================================================================= FAILURES ==========================================================================
________________________________________________________ TestOneShotSSH.test_state_does_not_persist _________________________________________________________

self = <tests.tools.test_ssh_environment.TestOneShotSSH object at 0xffffbb8a0510>

    def test_state_does_not_persist(self):
        _run("export HERMES_ONESHOT_TEST=yes")
        r = _run("echo $HERMES_ONESHOT_TEST")
>       assert r["output"].strip() == ""
E       AssertionError: assert 'yes' == ''
E         
E         + yes

tests/tools/test_ssh_environment.py:351: AssertionError
================================================================== short test summary info ==================================================================
FAILED tests/tools/test_ssh_environment.py::TestOneShotSSH::test_state_does_not_persist - AssertionError: assert 'yes' == ''
1 failed, 10 passed in 19.28s

Live SSH integration tests: 10/11 passed against Windows OpenSSH/Git Bash. The only failure is TestOneShotSSH.test_state_does_not_persist, which also exists unchanged on main and is unrelated to this PR’s non-UTF8 decoding fix; it indicates pre-existing one-shot SSH shell state persistence.

[alt-glitch]

This appears to be a duplicate of #37265 (and #37142) — all three target the same fix for #37130: adding errors="replace" to SSH/base subprocess text decoding. Consolidating onto one PR would help maintainers; flagging so the competing PRs can be reconciled.

[Chrisyk]

Thanks for flagging the overlap. For context: I reported #37130 yesterday and explicitly noted I was working on the fix, so I opened this PR to follow through. Beyond claiming it first, I'd suggest this PR as the consolidation target because it's the most complete of the three:

• Fixes all 7 text-mode subprocess.run sites in ssh.py, plus the latent _popen_bash case in base.py (a nearby comment already claimed errors="replace" but the code never set it).
• Adds regression tests (TestNonUTF8SubprocessOutput + TestPopenBashDecoding) with a negative control (remove the fix, and the 6 tests fail with the exact UnicodeDecodeError from the issue).
• Verified end-to-end against a real Windows OpenSSH host (Japanese/CP932 locale, which is the precise non-UTF-8 condition from the report): the backend now decodes to U+FFFD instead of crashing.

Also worth noting: #37265 and #37142 fix the ssh.py sites, but neither addresses the latent _popen_bash case in base.py (the comment there claims errors="replace" but the code never set it). So the same crash class survives on the drain path in both.

Happy to reconcile with #37265 / #37142 and to fold in anything they cover that I don't, or have them review here. Whatever's least work for maintainers.

[combatsheep]

Strong regression coverage here. Process-wise, the duplicate-PR consolidation concern raised below still looks worth resolving before merge so maintainers only have one place to review the fix.

[teknium1]

Thanks for the focused regression coverage. The strict-decoding failure is still present on current main: tools/environments/ssh.py:149 uses text=True without encoding or errors in the reported _ensure_remote_dirs() path, and the same pattern remains at ssh.py:104, 122, 164, 178, 207, and 325. tools/environments/base.py:146-152 likewise creates a text-mode process without a decoding policy; SSH reaches it through ssh.py:353.

The proposed encoding="utf-8", errors="replace" settings match the existing tolerant decode policy in the shared drain (tools/environments/base.py:590-595) and the already-hardened local backend (tools/environments/local.py:1076-1082). The regression tests exercise both the initialization path and malformed process output.

Current main has added stdin=subprocess.DEVNULL around the SSH setup calls after this PR's base SHA, so salvage requires mechanical conflict resolution that preserves those arguments while adding the decoding policy.

Automated hermes-sweeper review.

[alt-glitch]

This was generated by AI during triage.

Related competing repair: #37265 and #37142 focus on SSH decoding. This PR also protects BaseEnvironment _popen_bash, an independently useful shared execution path, so it is not a duplicate; maintainers can choose whether to take the broader scope.

[teknium1]

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.