fix(browser): safely decode subprocess output with regression coverage

[plcunha]

Summary

• Add _read_output_file_lossy() to read agent-browser stdout/stderr as bytes and decode malformed UTF-8 with errors="replace".
• Route _run_browser_command, _read_command_output_files, and the Chrome fallback (_run_tmp) through the helper.
• Preserve compatibility with file-like wrappers and existing tests that return an already-decoded str, even when opened in binary mode.
• Add focused regression tests for valid UTF-8, invalid bytes, missing files, combined stdout/stderr, and text-returning wrappers.

Why

The agent-browser subprocess can emit output in the system code page (for example Cp1252 on Windows with accented characters) rather than UTF-8. The previous strict encoding="utf-8" readers raised:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc6 in position 15: invalid continuation byte

That exception turned a recoverable browser error into a hard browser_navigate failure instead of surfacing the subprocess output.

The first implementation exposed a second compatibility issue in CI: existing Lightpanda tests mock open().read() with a str. Calling .decode() on that value caused two regressions. The helper now handles both production bytes and already-decoded str values.

Relationship to #47516

#47516 contains the earlier minimal behavioral fix (errors="replace") at the same three read sites. This PR is a more defensive, tested variant: it centralizes those reads in one helper, preserves compatibility with text-returning file wrappers/mocks, handles missing temp files best-effort, and adds focused regression coverage. Maintainers can choose the canonical implementation; the tests and compatibility case here are intended to be reusable either way.

Test plan

• pytest tests/tools/test_browser_output_decode_lossy.py -q — 5 passed
• pytest tests/tools/test_browser_lightpanda.py tests/tools/test_browser_output_decode_lossy.py -q — 47 passed
• ruff check tools/browser_tool.py tests/tools/test_browser_output_decode_lossy.py — passed
• python -m py_compile tools/browser_tool.py tests/tools/test_browser_output_decode_lossy.py — passed
• git diff --check — passed

[alt-glitch]

This was generated by AI during triage.

Duplicate of #47516 (earliest-open canonical, Fixes #47456). Both add errors="replace" lossy decoding to the agent-browser subprocess stdout/stderr reads in tools/browser_tool.py (_run_browser_command inline reader + _read_command_output_files), fixing the UnicodeDecodeError hard browser_navigate failure on non-UTF-8 (Cp1252/GBK) system code pages. Same file, same mechanism, same target issue. See also #50182 (open twin) and closed #47463. A maintainer picks the canonical one to merge.

[plcunha]

Fixed the CI regression from the first implementation: existing Lightpanda tests use file-like mocks whose read() returns str, while real subprocess temp files return bytes. The helper now handles both without weakening lossy decoding for malformed byte output. I also tightened the replacement-character assertions and removed unused imports. Local verification: 47 focused browser tests passed, ruff passed, py_compile passed, and git diff --check passed.

[plcunha]

Thanks — #47516 has the earlier minimal fix at the same three read sites. I updated this PR to make the relationship explicit. The additional value here is the shared best-effort reader plus regression coverage, including the str-returning file-wrapper compatibility case that the first version exposed in CI. Happy for maintainers to choose the canonical implementation; these tests/compatibility changes can be reused either way.

[plcunha]

Thanks for flagging the overlap. I compared this branch with #47516 and the current main before deciding how to proceed.

#47516 was closed without merge, and current main still reads these subprocess output files with strict UTF-8 decoding. This PR keeps the same core fix active, while also adding coverage for:

• the shared stdout/stderr helper;
• the Chrome fallback path;
• already-decoded text returned by wrappers/mocks; and
• missing output files.

I have now merged current main into this branch (e5301fa00); the PR remains limited to tools/browser_tool.py and its focused regression test. Local verification: 14 tests passed across the lossy-decoding and browser-timeout suites; ruff and git diff --check are clean.

Given that the earlier implementation is no longer open or present in main, I am keeping this as the active fix. Happy to adjust if maintainers prefer a different consolidation path.

[teknium1]

Thanks for the focused regression coverage. The premise remains valid on current main: tools/browser_tool.py:2477 and :2479 strictly decode normal agent-browser output, while the timeout path at :322 and Chrome fallback at :1120 retain the same failure class. The PR’s shared byte reader covers all three paths, preserves text-returning wrapper compatibility, and its new malformed-byte regression tests exercise the helper and timeout reader.

All required CI checks are passing according to gh pr checks 63512.

Automated hermes-sweeper review.

[alt-glitch]

This was generated by AI during triage.

Related to #47516 and #50182. This remains open as a focused, tested variant: it centralizes lossy decoding, covers the fallback path, and preserves compatibility with text-returning test wrappers; the earlier anchor is closed.

[alt-glitch]

This was generated by AI during triage.

Related to #47516 rather than a duplicate: #47516 is closed unmerged, while this version adds the wrapper-compatibility and fallback regression coverage needed for the current implementation.

[plcunha]

Closing as duplicate of #47516. The earlier PR already contains the same lossy-decoding fix.

[lifeFedorovAlexey]

Request to reopen this PR — a fresh Windows Desktop reproduction shows that the underlying bug is still present on current main.

Fresh reproduction

browser_console failed on Windows with:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0x91 in position 0

Current origin/main still manually reads agent-browser stdout/stderr temp files with strict UTF-8 in tools/browser_tool.py. The normal success path, timeout helper, and Chrome fallback retain variants of the same failure class.

How the fix was lost

There is an unfortunate consolidation chain here:

1. fix(tools): use errors=replace for browser subprocess output reads (#47456) #47516 was the earliest PR for the same strict stdout/stderr reads. It was closed as stale (no merge activity for weeks) and was not merged.
2. fix(browser): prevent UnicodeDecodeError on Windows with non-UTF-8 subprocess output #50182 was then classified as a duplicate of fix(tools): use errors=replace for browser subprocess output reads (#47456) #47516. Its Sweeper review nevertheless confirmed that the strict reads were still present on current main, while noting that fix(browser): prevent UnicodeDecodeError on Windows with non-UTF-8 subprocess output #50182 itself needed narrower line-ending-preserving changes and regression coverage.
3. This PR, fix(browser): safely decode subprocess output with regression coverage #63512, supplied the more complete implementation requested by that review: a shared best-effort reader, coverage for normal/timeout/fallback paths, malformed-byte regression tests, compatibility with wrappers returning str, and clean focused checks. Sweeper marked it keep_open with salvageability=high and reported passing required CI.
4. fix(browser): safely decode subprocess output with regression coverage #63512 was nevertheless closed as a duplicate of fix(tools): use errors=replace for browser subprocess output reads (#47456) #47516 — but fix(tools): use errors=replace for browser subprocess output reads (#47456) #47516 had already been closed without merge, so there was no surviving canonical fix in main.
5. fix(windows): close out the text-mode subprocess decode bug class — salvage #55339/#60741/#60751 + full sweep + linter #70875 was later merged and described as closing the Windows subprocess decode class codebase-wide. In tools/browser_tool.py, however, its actual patch only added encoding='utf-8', errors='replace' to a subprocess.run(..., text=True) call. It did not modify these manual temp-file reads (open(stdout_path/stderr_path, encoding='utf-8')).
6. Issue [Bug]: browser_tool.py UnicodeDecodeError on Windows with non-UTF-8 system encoding (GBK/CJK) #47456 and the remaining PRs were then closed as resolved by fix(windows): close out the text-mode subprocess decode bug class — salvage #55339/#60741/#60751 + full sweep + linter #70875, although this separate manual-file-decoding path remained.

So the current state is:

#47516  closed stale, unmerged
#50182  closed as duplicate/resolved, unmerged
#63512  closed as duplicate of already-closed #47516, unmerged
#70875  merged, but did not touch the manual temp-file reads
current main still crashes on malformed/non-UTF-8 output

This is not a request for a competing fourth implementation. Reopening #63512 appears to be the cleanest path because it preserves the existing contributor work and already contains the broader tested implementation that prior review requested. Could maintainers please reopen/re-evaluate this PR against current main rather than leaving the fix orphaned by the duplicate/stale chain?