[Bram910]

What

Use UTF-8 consistently for the patch advisor's standard streams and captured relinker output. The relinker prints UTF-8, but subprocess.run(text=True) previously decoded it using the host locale. On a legacy Windows code page or an ASCII locale, non-ASCII diagnostics can be corrupted or fail decoding. Separately, ASCII-configured advisor output streams cannot print a valid recommendation whose path contains non-ASCII characters.

Configure standard streams as UTF-8 with backslashreplace, following the other tools. Decode relinker stdout/stderr as UTF-8 with replacement for malformed diagnostic bytes. Hash validation, verification summaries, mismatch handling and exit-status policy are unchanged.

Add regressions using a real Python child that emits UTF-8 and malformed bytes under a forced ASCII subprocess codec, plus actual ASCII TextIOWrapper streams. This is separate from #10667's path-quoting assertion fix; both merge orders are clean and the combined unit suite passes.

Review correction: the real-child wrapper now uses the documented encoding argument with options.setdefault("encoding", "ascii") to provide a deterministic legacy-codec fallback only when the advisor omits its explicit encoding. No private subprocess helper or locale internals are patched. The child's actual UTF-8/malformed output is still decoded by the real subprocess implementation.

Tested

• Output-coverage follow-up head e49a927811fefea42be1df2c41e3387f2da340a5 changes tests only. The ASCII-stream case now prints an actual Unicode catalog path to stdout and a Unicode missing-file diagnostic to stderr, checks the UTF-8 bytes, and checks both success/error statuses. Real relinker-child tests exercise both exit 0 and exit 2 for each diagnostic payload and assert the decoded stdout/stderr reach the user on exit 2.

• Follow-up validation: 26/26 unit tests pass; both Windows advisor CTests pass five times each, and the full Linux Python 3.11 unit suite and native integration each pass five times. A mutant that reconfigures streams only after output passes the old stream test but fails the revised test; suppressing decoded diagnostics fails both exit-2 payload subcases. No timeout or production-code changes.

• Branch base: upstream main at e7e99adcc4580e7dc1337258b3099161105847b1; original published head 8d8b2496ed163c5611001ebe1e01b440d2b706cd. Rebased after initial validation; intervening upstream changes affect only shader code, not either changed file. The 26-test unit suite on both Windows and Linux, both registered Windows patch-advisor CTests, and conventions checks pass again after that rebase.

• Review-correction head a3e2626eea63c29887a4055ad60aea0287183307: production code is unchanged. 26/26 unit tests pass on Windows Python 3.13.5 and Linux Python 3.11.0rc1; both registered Windows advisor CTests pass five runs each, and the Linux unit suite and native relinker integration each pass five runs. Revised regressions still fail against unchanged main for the encoding bugs, without private-helper setup errors. Conventions and whitespace checks pass.

• Python-version scope: core/relinker/CMakeLists.txt already registers these two advisor tests only for Python 3.11+, and the existing advisor imports tomllib. The full advisor suite was therefore not run on 3.9/3.10. Separate real-child decoder probes pass on actual Windows CPython 3.9.13 and Linux CPython 3.10.12: the documented ASCII fallback rejects both UTF-8 and malformed payloads, while explicit UTF-8 with replacement decodes both stdout and stderr correctly. No TOML shim, new dependency or lowered version gate was added.

• Hosted CI on original published head 8d8b2496ed163c5611001ebe1e01b440d2b706cd: Linux, Windows and both macOS relinker variants, ASan, conventions, eligibility, intake and labels succeed. Full-product Linux/Windows, register-context and write-watch were skipped. These completed runs precede the test-only review correction.

• Full Windows Ninja Release relinker-only build and CTest: 244/244 passed in 195.43 seconds on pre-rebase head d629d7c866de857a4b8968a3b82450e87ecd5449 (base d01070d6c3). Both changed files are identical after the rebase.

• Red/green: both new regression methods fail against unchanged main on Windows Python 3.13.5 and Linux Python 3.11.0rc1, then pass with this fix. The child writes directly to both file descriptors, so its UTF-8 bytes do not depend on its own standard-stream encoding.

• python -m unittest tools.tests.test_suggest_patches: 26/26 passed on Windows and Linux.

• Real relinker CLI red/green on Windows and native Linux: use a Unicode input filename and directory (café-にほん) with PYTHONIOENCODING=ascii, and test both matching and mismatching patch bytes. Main fails both cases (Windows exits 1; Linux exits 2). The fix recommends the valid patch with exit 0, rejects mismatching bytes with exit 1, preserves the Unicode paths in output, and leaves the input ELF unchanged.

• Windows: both registered patch_advisor_unit and patch_advisor_integration CTests pass five consecutive runs each, with no timeout changes.

• Ubuntu 22.04 under WSL, Python 3.11.0rc1: repeat the entire 26-test unit suite and the existing real-relinker integration script five times each; all pass. The native Linux relinker was rebuilt successfully with GCC 11.4.

• Merge compatibility: git merge-tree --write-tree in both orders with test(tools): compare quoted and resolved paths in the patch advisor test #10667's head 76008e99b9bf02bcbcbec2ed0d3c5380204cde2b produces the same clean tree. Run the combined unit test file in memory against this fix: 26/26 passed, including test(tools): compare quoted and resolved paths in the patch advisor test #10667's space-containing temporary roots.

• The repository's open-PR overlap scanner reports no overlap. Conventions and whitespace checks pass. No title, GPU or full-product testing; this is Python tool I/O handling.

Checklist

• Based on current main; no other open PR implements the same functions
• One topic per PR; follow-ups go in a new PR
• No comments in code except technical debt
• Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
• No notes, investigation .md files or images added to the repository (attach them to this PR)
• New third-party code is a submodule built from source
• Generic behaviour, not specific to one title
• Depends on: none
• Fixes: none
• Unblocks: none
• GPUs tested: n/a
• AI-assisted: yes

[MarcZX10]

The production change looks sound, and the exact-head focused suite passes locally (26/26); diff-check and conventions pass. I found one blocking supported-Python issue in the new regression test: test_relinker_output_is_utf8_independent_of_host_encoding patches subprocess._text_encoding with patch.object without create=True. That private attribute is absent from CPython 3.9 and 3.10 (it appears in the v3.11 source); the test therefore raises AttributeError before exercising the fix on supported interpreters. The repository build guide supports Python 3.9+. I reproduced the test setup failure by removing that attribute before running the new test (2 subtest errors). Please replace the private-attribute patch with a version-independent way to force the parent's default decoder to ASCII (for example, patch the locale encoding source used by those interpreters), then run the focused suite on the oldest supported Python as well as current Python.

Primary source references: CPython 3.9 Lib/subprocess.py https://github.com/python/cpython/blob/v3.9.0/Lib/subprocess.py and CPython 3.10 https://github.com/python/cpython/blob/v3.10.0/Lib/subprocess.py do not define _text_encoding; CPython 3.11 does: https://github.com/python/cpython/blob/v3.11.0/Lib/subprocess.py. The official Python 3.9 subprocess docs state that encoding and errors have been supported since 3.6: https://docs.python.org/3.9/library/subprocess.html.

[Bram910]

Addressed the private-helper dependency in a3e2626eea63c29887a4055ad60aea0287183307. The real-child wrapper now supplies options.setdefault("encoding", "ascii"): if the advisor forgets its explicit encoding, the real subprocess uses ASCII; otherwise the advisor's explicit UTF-8 passes through unchanged. This uses only the documented subprocess API, works independently of UTF-8 mode or locale implementation, and does not patch or require _text_encoding. The actual child still writes UTF-8 and malformed bytes directly to both descriptors. Production code is unchanged.

One version-scope clarification: the generic build guide says Python 3.9+, but core/relinker/CMakeLists.txt already gates these two advisor tests on Python3_VERSION VERSION_GREATER_EQUAL 3.11, and suggest_patches.py imports tomllib unconditionally. I have not expanded the tool's supported-version range or introduced a TOML shim.

Validation:

• Full advisor suite: 26/26 pass on Windows CPython 3.13.5 and Linux CPython 3.11.0rc1 (the tool's minimum minor version).
• Both registered Windows advisor CTests pass five runs each; the Linux full unit suite and native relinker integration also each pass five runs, with no timeout changes.
• On Windows and Linux, the revised regressions still fail against unchanged main for the original encoding bugs, not a private-attribute setup failure.
• Separate actual-runtime decoder probes on Windows CPython 3.9.13 and Linux CPython 3.10.12 confirm the documented ASCII fallback rejects both diagnostic payloads and explicit UTF-8 with replacement correctly captures both streams. This is decoder compatibility evidence, not a claim that the tomllib-dependent full advisor suite ran on those interpreters.
• Conventions and whitespace checks pass. The PR description now separates original completed CI from this test-only revision.

[THEMANnt]

Reviewed a3e2626eea63 against 66de8e7a12cb. No actionable findings in this focused review.

Source review

The advisor now configures supported output streams as UTF-8 with backslashreplace and decodes captured relinker output as UTF-8 with replacement for malformed bytes. Hash validation, verification summaries, mismatch handling, timeouts, and exit-status policy are preserved. The regressions exercise real child-process decoding and real ASCII text streams. The earlier private-API test patch is gone; the documented advisor minimum and CMake test gate are Python 3.11+. Pristine and candidate files match, with no local amendments.

Validation

• Controller-recorded Windows/Python 3.11.9 runs of tools/tests/test_suggest_patches.py: baseline 24 passed; pristine PR and candidate each 26 passed; zero skips.
• Candidate tests against baseline produced the intended ascii != utf-8 assertion failure. Two additional decoding subtest errors followed ASCII decoder failures during child-output capture; these were not setup errors. The stream control stops at its first assertion.
• Recorded whitespace and conventions checks passed.
• GitHub-reported effective CI succeeded for the Ubuntu/Windows relinker jobs, both macOS relinker variants, relinker-asan, conventions, eligibility, and labels. Linux/Windows product builds, register-context, and write-watch were skipped. The earlier cancelled conventions run was superseded by a successful matching-workflow execution.

Scope

Local validation covers the focused Python tool suite. The author's additional integration/build/interpreter checks were not independently reproduced locally; full application behavior was not tested.

[roboterspinne]

Reviewed head a3e2626 merged onto upstream/main befe264 (2026-10-24); the merge is clean, also together with my #10667 (same test file, different hunks, either order), check_conventions.py passes and pr_overlap.py reports no overlap with open pull requests.

Environment: Windows 11 Home 10.0.26200, Python 3.13.9 (Anaconda), locale code page 1252; Python only, no build needed.

Results

• python -m unittest tools.tests.test_suggest_patches on the merged tree: 25 of 26 pass. The one failure is test_uppercase_digest_and_conversion_options, which fails on main whenever the temp path contains a space (the tool quotes such paths, the test compares unquoted); #10667 fixes exactly that assertion, and with it merged on top all 26 pass. Red/green: against main's suggest_patches.py both payloads of test_relinker_output_is_utf8_independent_of_host_encoding error and test_output_streams_use_utf8 fails; both pass on the branch.
• main's failure modes reproduced: with text=True and no encoding, subprocess.run decodes with the locale encoding, cp1252 here, which never raises but turns the relinker's UTF-8 into mojibake and maps 0xff to ÿ; under a code page that can fail (ascii, cp932) the decode error is raised in subprocess's reader thread and the advisor then dies at line 146 (VERIFY_SUMMARY.search(result.stdout)) with TypeError: expected string or bytes-like object, got NoneType, a traceback instead of a handled exit. The branch decodes both payloads correctly and returns the expected exit codes.
• UTF-8 is the right decoder: on Windows the relinker converts its wide argv to UTF-8 (core/relinker/main.cpp:491-497, WideCharToMultiByte(CP_UTF8, ...)) and prints paths as u8string (core/relinker/io/src/NativePath.cpp:11), so its diagnostics are UTF-8 bytes whatever the console code page; errors="replace" covers stray bytes such as OS error text.
• The stream reconfigure fixes a second real failure: with a non-ASCII title or path and stdout on an ascii or cp1252 pipe, main's print raises UnicodeEncodeError, which the handler at line 188 turns into exit 2 Patch advisor: ... codec can't encode, so a real verdict including the Recommended: line is lost; the branch writes UTF-8 and exits with the right code. It overrides PYTHONIOENCODING, as check_skips.py:49-50, build_web.py:169-170 and check_compile.py:229 already do.
• The revised wrapper answers the change request: options.setdefault("encoding", "ascii") uses only the documented encoding keyword (3.6+), no private helper, and forces an ASCII decode only when the advisor passes no encoding, so the test is red on main on every platform (an ERROR via the reader thread on Windows, in the main thread on POSIX) and does not depend on the host locale or PYTHONUTF8. Removing any one part of the fix (either reconfigure, the encoding, the errors) makes a test red. The 3.9/3.10 question is moot for this file anyway: suggest_patches.py:9 imports tomllib unconditionally, USAGE.md:103 says the advisor needs 3.11 or newer, and core/relinker/CMakeLists.txt:629-634 registers patch_advisor_unit and patch_advisor_integration only for Python3_VERSION >= 3.11, so CI never ran these tests on older interpreters; still better without the private helper.

Nits

• test_suggest_patches.py:96: test_output_streams_use_utf8 asserts only the streams' encoding and errors after advise(), and the temp root is ASCII, so no non-ASCII text is ever written through them. A variant that reconfigures only after all output still passes, yet it returns 2 with 'ascii' codec can't encode character '\xe9' when the root is created with prefix café-. Building the root with a non-ASCII prefix and asserting rc 0 plus the UTF-8 path in stdout.buffer.getvalue() would pin what the fix is for.
• test_suggest_patches.py:82: the relinker regression runs only the exit-0 path, so it checks that the output is decoded as UTF-8 but never that the decoded diagnostic reaches the user; printing happens only in the rejected and exit-2 branches (suggest_patches.py:151-157). Optional: one subcase where the fake relinker exits 2 and the decoded text is asserted in the captured advisor output.

Notes, nothing to change here: main() now reconfigures the real sys.stdout and sys.stderr, so every in-process test that does not replace both streams leaves the runner's streams at utf-8/backslashreplace (cp1252 before, utf-8 after, here); harmless, the tests already leaked output on main, and the other tools do the same. The body's "26/26 on Windows" holds only when the temp path has no space; the five-run, Linux 3.11.0rc1, 244/244 CTest and 3.9/3.10 probe claims come without logs and I could not check them. Hosted CI is green on a3e2626 (ubuntu, windows, both macOS, ASan).

Not checked: Linux runs, Python 3.11 itself, a real relinker child with non-UTF-8 output (only python -c children with the two payloads).

I found no blocking issue: the three changes fix two failures I could reproduce, the revised test uses only documented API, and the counter-checks hold, so the open change request looks addressed to me. For disclosure: I have open PRs (#10667 on tools/tests/test_suggest_patches.py, and relinker PRs #10679, #10658, #10646, #10607, #10596, #10564, #10403, #10402, #10401, #10391, #10390); #10667 changes one assertion in test_uppercase_digest_and_conversion_options, a different test in this file, and is the fix for the one failure above.

Written with AI assistance (Claude Code). I ran the tests above on my Windows 11 machine.

[roboterspinne]

Nit: this asserts only the streams' attributes after advise(), and the temp root is ASCII, so no non-ASCII text is ever written through them. A variant that reconfigures only after all output still passes, yet it returns 2 with 'ascii' codec can't encode character '\xe9' when the root is created with prefix café-. Building the root with a non-ASCII prefix and asserting rc 0 plus the UTF-8 path in stdout.buffer.getvalue() would pin what the fix is for.

[roboterspinne]

Nit, optional: this runs only the exit-0 path, so it checks that the output is decoded as UTF-8 but never that the decoded diagnostic reaches the user; printing happens only in the rejected and exit-2 branches (suggest_patches.py:151-157). One subcase where the fake relinker exits 2 and the decoded text is asserted in the captured advisor output would cover that.

[Bram910]

Addressed both output-coverage nits in e49a927811fefea42be1df2c41e3387f2da340a5:

• The ASCII-stream case now uses a Unicode catalog path and a Unicode metadata filename. It asserts successful recommendation output contains the actual UTF-8 catalog path, then checks an error diagnostic contains the UTF-8 missing filename on stderr. Both streams are flushed before inspecting their bytes. The unrelated path-quoting test remains untouched (test(tools): compare quoted and resolved paths in the patch advisor test #10667 still owns that fix).
• The real-child decoder case now runs exit 0 and exit 2 for both diagnostic payloads. The exit-2 subcases assert the decoded combined stdout/stderr is present in the advisor's user-visible output and that the advisor returns 2 with its no-recommendation diagnostic.

Negative controls: moving stream reconfiguration after output passes the old stream test but fails the revised Unicode test; hiding the decoded diagnostic fails both exit-2 subcases. Production code is unchanged. Both Windows advisor CTests pass five runs each; the Linux Python 3.11 full 26-test unit suite and native integration also pass five runs each. Conventions and whitespace checks pass, with no timeout changes.

[MarcZX10]

Reviewed exact head e49a927811fefea42be1df2c41e3387f2da340a5. The UTF-8 subprocess decoding and stream reconfiguration are covered by tests for non-ASCII and invalid-byte diagnostics, successful and failing relinker exits, and non-ASCII output paths. The latest test commit also addresses the earlier gaps around asserting user-visible diagnostics and non-ASCII paths. I ran python3 -m unittest tools.tests.test_suggest_patches on this head (24/24 passed), plus git diff --check and the conventions checker (passed). Hosted eligibility, labels and conventions pass; remaining relinker matrix checks are unrelated to these Python-only files.

[roboterspinne]

@Bram910 Thanks, checked e49a927 merged onto upstream/main 2dc8560 (2026-10-24), Windows 11, Python 3.13.9, locale cp1252: tools.tests.test_suggest_patches 26 tests, 25 pass; the one failure is still test_uppercase_digest_and_conversion_options from the space in my temp path, which #10667 fixes (with it merged on top all 26 pass). Against main's suggest_patches.py all four relinker subcases error and the stream test fails. Your negative controls hold here as well: with the two reconfigure calls removed only test_output_streams_use_utf8 fails (the UTF-8 catalog path no longer reaches the ASCII stream), and with the decoded diagnostic blanked (detail = "") exactly the two exit-2 subcases fail. Both nits are done from my side.

Written with AI assistance (Claude Code).