fix(environments): pass encoding='utf-8' errors='replace' to all text-mode Popen calls

[liuhao1024]

What does this PR do?

Adds explicit encoding="utf-8" and errors="replace" to all subprocess.Popen calls that use text=True but were missing these parameters. On Windows, text=True without explicit encoding uses the system ANSI code page (e.g. GBK on Chinese Windows), which causes Python's internal _readerthread to crash with UnicodeDecodeError when the child process outputs non-UTF-8 bytes.

Related Issue

Fixes #47939

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• tools/environments/base.py: Added kwargs.setdefault("encoding", "utf-8") and kwargs.setdefault("errors", "replace") to _popen_bash() — the shared Popen helper used by Docker, SSH, and Singularity backends. Uses setdefault so callers can still override if needed.
• tui_gateway/server.py: Added encoding="utf-8", errors="replace" to _SlashWorker.__init__() Popen call.
• agent/copilot_acp_client.py: Added encoding="utf-8", errors="replace" to CopilotACPClient._run_prompt() Popen call.
• tests/tools/test_base_environment.py: Added TestPopenBashEncodingDefaults with 2 tests verifying the defaults and caller override.

How to Test

1. python -m pytest tests/tools/test_base_environment.py::TestPopenBashEncodingDefaults -v — verifies _popen_bash passes correct encoding params
2. python -m pytest tests/tools/test_base_environment.py -v — full base environment test suite (20 tests)
3. On Windows: run a terminal command that produces GBK-encoded output (e.g. Chinese text) — should no longer crash with UnicodeDecodeError

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/tools/test_base_environment.py -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS

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
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

• Analyzed: _popen_bash (callers: Docker, SSH, Singularity environments), _SlashWorker.__init__, CopilotACPClient._run_prompt
• Blast radius: LOW — only affects text-mode Popen encoding defaults; no control flow changes
• Related patterns: LocalEnvironment._run_bash already had encoding="utf-8", errors="replace" (line 599-600); this PR applies the same safeguard to the remaining code paths

[alt-glitch]

Related: this fix overlaps with open PR #45099, which already adds encoding="utf-8", errors="replace" to _popen_bash in tools/environments/base.py plus agent/copilot_acp_client.py and tui_gateway/server.py (the same three files this PR touches) and additionally covers code_execution_tool.py, tts_tool.py, and transcription_tools.py. This PR is a focused subset of that broader change (and uses kwargs.setdefault(...) so callers can override). Both target #47939 / the Windows non-UTF-8 (GBK) subprocess crash. Sibling subprocess-encoding PRs for other code paths: #25200 (gateway/CLI), #37265 (SSH), #43494 (gateway_windows). Reviewers may want to consolidate on one.

[liuhao1024]

This PR overlaps with #45099, which was created 5 days earlier and covers more files (code_execution_tool.py, run_tests_parallel.py, transcription_tools.py, photon adapter).

One key difference: this PR uses kwargs.setdefault("encoding", "utf-8") in _popen_bash, which allows callers to override the encoding via kwargs. #45099 adds encoding="utf-8" directly in the Popen() call — if any caller passes encoding= via **kwargs, Python raises TypeError for duplicate keyword arguments.

Recommend closing this PR in favor of #45099 for broader coverage, but adopting the kwargs.setdefault() approach in _popen_bash as a follow-up refinement.

[teknium1]

Thanks for isolating the Windows text-pipe failure mode. The direct changes to _SlashWorker and CopilotACPClient address real current-main call sites: both construct text=True pipes without an explicit decode policy (tui_gateway/server.py:314-325, agent/copilot_acp_client.py:506-515), and #52649 records a CP936 failure in _SlashWorker._drain_stderr.

Problems

• The stated all-text-mode-Popen scope remains incomplete: scripts/run_tests_parallel.py:257-270 still has text=True without encoding/errors. The related open PR #45099 includes that site.
• The _popen_bash change is not an active output-decoding fix on current main. BaseEnvironment reads the pipe with os.read() and decodes through UTF-8 errors="replace" itself (tools/environments/base.py:644-655; introduced by f336ae3d7).
• The added tests only inspect _popen_bash; they do not cover either direct reader-thread call site changed here.

Suggested changes

• Add focused regression coverage for the TUI and ACP Popen decode policy, and either cover the remaining runner site or narrow the PR's scope.

Automated hermes-sweeper review.

[teknium1]

_popen_bash output is not consumed through this TextIOWrapper on current main: BaseEnvironment._wait_for_process() reads the raw fd and incrementally decodes UTF-8 with errors="replace" (tools/environments/base.py:644-655). This change should not be treated as coverage for the active environment-output path.

[teknium1]

These tests only cover the base-helper kwargs, while the direct vulnerable readers changed by this PR are _SlashWorker and CopilotACPClient. Please add focused coverage for those Popen calls (or their undecodable-stderr behavior).

[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.