fix(terminal): decode non-UTF-8 command output in the host codepage

[jackulau]

What does this PR do?

The terminal tool's output drain decoded every byte strictly as UTF-8 with errors="replace". Git Bash is a byte pipe rather than a transcoder, so on a CJK Windows install a powershell.exe error arrives as GBK inside a stream whose MSYS tools write UTF-8 — and replace is applied at decode time, so those bytes are gone. The agent reads a wall of U+FFFD instead of the error message it needs.

This retries a line that strict UTF-8 rejects in the host's locale encoding.

The invariant that makes it safe: output that is valid UTF-8 decodes exactly as it did before. The fallback is only ever consulted for byte sequences UTF-8 rejects, which are already lost to U+FFFD today. So this can recover text and cannot corrupt text that was previously fine — test_a_pure_utf8_stream_matches_the_old_decoder_exactly asserts byte-for-byte agreement with the decoder it replaces.

Related Issue

Refs #89442

One correction to that report, because it points at the wrong lines and the difference is load-bearing.

The issue's root cause is that text=True, encoding="utf-8" on Popen decodes at the pipe level and "the original bytes are discarded". That is not what happens on this path. _wait_for_process resolves proc.stdout.fileno() and os.read()s the descriptor directly, bypassing the TextIOWrapper entirely — the raw bytes arrive intact, and the decision is made by the drain's own codecs.getincrementaldecoder("utf-8")(errors="replace") a few lines later. The suggested remedy ("collect raw bytes, then decode with sniffing") is therefore already half-built in-tree; only the decoder choice was hardcoded.

That makes this a much smaller change than the issue implies: no Popen argument changes, no chcp, no shell wrapping. The three sites named in the issue (local.py:863, :929, and _popen_bash) are the Mandatory-ASLR probe, the bash-startup probe, and the SDK-backend spawn helper — none of them is the tool output path, and all are left alone.

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/base.py

• _Utf8WithFallbackDecoder — same decode(chunk, final=False) shape as the incremental decoder it stands in for. Tries strict UTF-8 first; only on UnicodeDecodeError does it retry in the fallback; anything that fails both ends on the original errors="replace" path.
• _system_ansi_encoding() — locale.getencoding(), canonicalised through codecs.lookup, returning None when it is already UTF-8.
• BaseEnvironment._output_fallback_encoding() — new hook, returns None.
• _wait_for_process — picks the decoder from that hook. None keeps the exact previous object, so the untouched case is untouched by construction rather than by inspection.

tools/environments/local.py

• LocalEnvironment._output_fallback_encoding() — the host codepage, on Windows only.

Three judgment calls, stated rather than buried

Decode per line, not per read. Legacy multi-byte encodings are not self-synchronising: GBK is two bytes per character with no lead/continuation distinction, so a fallback decode over an arbitrary 4096-byte read boundary can split a character and produce mojibake. A line is the unit that has a single origin — one program wrote it. This costs nothing in latency because _wait_for_process renders the collected output only after the drain thread joins; nothing observes the stream mid-command. An unterminated buffer is flushed at 32 KiB, and a bare CR is a boundary too so progress output does not accumulate.

Opt-in per backend, defaulting to off. The hook returns None on BaseEnvironment, so docker / ssh / daytona / modal keep the old decoder. Their bytes come from a container or a remote host whose codepage this process cannot observe, and answering from the local locale would be a fabricated answer. Only LocalEnvironment overrides it, because there the writer really is a child of this process. It is further gated on Windows: on POSIX the shell and its children agree on the locale, so a non-UTF-8 line there is far likelier to be binary than to be locale text.

A unit containing NUL stays on the replacement path. This is the one place the change could plausibly make something worse. Legacy codepages are byte-dense — cp1252 decodes almost anything — so without a guard, cat of a binary file would come back as mojibake instead of the U+FFFD it produces today. No text encoding in real use emits an interior NUL, so the guard is free on the case that matters. It is not a complete binary detector, and I have not tried to make it one: the honest scope is "recover text that was destroyed", not "classify every stream".

How to Test

pytest tests/tools/test_terminal_ansi_codepage_fallback.py -q     # 32 passed

Behavioural throughout — real os.pipe() file descriptors driven through _wait_for_process itself for the wiring, and real bytes/codecs for the decoder. No mocking at the boundary under test.

Mutation proof — every property is independently load-bearing:

Reverted	Tests that fail
the wiring (always use the old strict decoder)	1 — test_a_backend_with_a_fallback_recovers_the_message
try the fallback first instead of only on a UTF-8 error	2 — incl. test_a_pure_utf8_stream_matches_the_old_decoder_exactly
the NUL guard	1 — test_binary_with_a_nul_byte_keeps_the_replacement_behaviour
decode per read instead of per line	13 — the whole TestChunkBoundaries and TestBufferingContract groups
the None-on-UTF-8-host short circuit	3 — incl. test_local_on_a_utf8_windows_host_has_no_fallback
LocalEnvironment's Windows gate	1 — test_local_has_no_fallback_off_windows
the base default returning a fallback	2 — incl. test_a_backend_without_one_is_unchanged

Baseline — every environment/terminal test file (tests/tools/test_base_environment.py, test_local_*.py, test_terminal_*.py, test_ssh_environment.py, test_docker_environment.py, test_daytona_environment.py, test_managed_modal_environment.py, test_approved_command_clean_slate.py), -q -p no:randomly, run serially, with and without the change:

	Result
without the change (stashed)	34 failed, 443 passed, 13 skipped
with the change	34 failed, 475 passed, 13 skipped

The failing sets are identical — I diffed the two FAILED lists and they match line for line. They are pre-existing on this machine and unrelated: the docker/daytona/ssh suites want daemons and remote hosts that are not present, and several terminal tests assume a POSIX shell. The delta is exactly the 32 new tests.

Overlap with open PRs

Nothing open touches this decoder. The neighbours are worth naming because the topic looks crowded from a title search:

PR	What it is	Relationship
#87162, #87181 (open)	pin the Browser-Use CLI's pipes to UTF-8	Same class of bug, different process. Both fix a subprocess wrapper in tools/browser_use_cli.py; neither touches tools/environments/.
#82494, #86636, #80186 (open)	U+FFFD / CJK heuristics in binary detection for read_file	tools/file_operations, deciding whether a file is binary. This decides how a pipe is decoded. No shared code.
#42775 (open)	"Windows compatibility, path resolution, and encoding issues"	Touches acp_adapter/ and two test files only.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing PRs to make sure this isn't a duplicate — see the Overlap table
• My PR contains only changes related to this fix (one commit, rebased on main)
• I've run the environment/terminal slices with and without the change, serially (see How to Test). I did not run pytest tests/ -q wholesale: on Windows tests/hermes_cli/ can't be collected (test_doctor_journal_modes.py calls os.geteuid), so a full-suite number from here would be meaningless. CI runs it.
• I've added tests for my changes — 32, with the mutation proof above
• I've tested on my platform: Windows 11

Documentation & Housekeeping

• I've updated relevant documentation — docstrings only; each of the three judgment calls above is documented at the line that implements it
• N/A — no config keys added or changed. The fallback is derived from the host locale, so there is nothing to configure and nothing to migrate
• N/A — no architecture or workflow change. _output_fallback_encoding is an internal hook on an existing abstract base, defaulting to the previous behaviour
• I've considered cross-platform impact — this is the cross-platform change. POSIX is excluded at LocalEnvironment, a UTF-8 host gets None from _system_ansi_encoding and therefore the byte-identical old decoder, and remote backends are excluded at the base class. scripts/check-windows-footguns.py passes on all three files
• N/A — no tool description or schema change

[jackulau]

The red check on this PR (Python tests / Run tests slice 6/12) is not from this change. It is a pre-existing timing flake in the gateway suite, and I have opened #89481 to fix it.

The failure, from job 95864357118:

tests/gateway/test_goal_continuation_drain.py::test_runner_goal_hook_enqueues_into_the_key_the_adapter_drains
AssertionError: continuation enqueued under a different key than the adapter drains:
pending keys=[] expected=agent:main:slack:channel:C1:1718600000.000100

Why it is not mine:

• This PR's diff is three files — tools/environments/base.py, tools/environments/local.py, and one new test file under tests/tools/. Nothing under gateway/, and nothing that test imports.
• That test passes locally on this branch (2 passed).
• The failing assertion is the empty-list case. pending keys=[] means nothing had been enqueued yet, not that something was enqueued under the wrong key — the message is written for the second case and reports the first one in the same words, which is what makes it read like a real regression.

The cause is a bare await asyncio.sleep(0.05) used to wait for an enqueue that happens on a spawned task. The sibling test in the same file was already converted to a bounded poll in 563376473; that pass missed this site. #89481 applies the same idiom here.

Happy to rebase this PR once that lands, or a re-run of the slice should also come back green.

[jackulau]

Rebased onto 2163f7f8ca to re-roll CI.

For anyone reading the previous red: the failure was not in this PR's area. Both attempts on the last run failed the same unrelated test:

FAILED tests/gateway/test_goal_continuation_drain.py::test_runner_goal_hook_enqueues_into_the_key_the_adapter_drains
E  AssertionError: continuation enqueued under a different key than the adapter drains: pending keys=[]

That test waits for the goal continuation with a single fixed 50 ms sleep while the enqueue is done by a task the hook spawns, so a loaded runner loses the race and reports pending keys=[] - which reads as "enqueued under the wrong key" when in fact nothing has been enqueued yet. #89481 replaces the sleep with the bounded poll the sibling test in the same file already uses. This PR touches only tools/environments/{base,local}.py and its own new test module, and the same test passes on my other open PRs.

No functional change in this push - same three files, same 485/2 diff, just re-parented.