fix(shell): decode Windows output reliably

[zhuowp]

Background

Windows-native tools may emit stdout or stderr using the active ANSI code page even when the surrounding shell is configured for UTF-8. CodeWhale previously decoded those bytes as UTF-8 lossily, and background paths decoded individual polls independently. Once that happened, Pinvou Agent could no longer recover the original bytes, so both the model and UI could receive mojibake or replacement characters.

The reusable fix was first contributed upstream in Hmbown/CodeWhale #5602. This PR publishes the reviewed behavior on Pinvou's current maintenance baseline. It is based on public r10 (feb8761aeda31749f3d54c6e1f8ef460540567a1, from Pinvou/CodeWhale #19) and contains one topic commit.

Changes

• Preserve strict UTF-8 decoder state across shell reads.
• Fall back to the current Windows ACP only after genuine invalid UTF-8.
• Preserve the prefix already certified as UTF-8 and send only the invalid suffix to the legacy decoder.
• Share decoding semantics across synchronous output, bounded snapshots, raw deltas, tails, and detached readers.
• Seal detached streams without discarding bytes captured before the cutoff.
• Include the upstream non-Windows -D dead_code follow-up requested in review, with an English explanation and cfg_attr on the Windows-only mapping helper.
• Add decoder, mixed UTF-8/ACP, shell-lifecycle, and forkguard_* regression coverage.

Review follow-up

Candidate head:

654490026edf2a3105858ff644c7087a23dd5f6c

This candidate resolves both findings in the parent-side review:

1. ShellStreamDecoder::decode now uses Utf8Error::valid_up_to() to emit the valid UTF-8 prefix before legacy-decoding only the invalid suffix. The regression covers ?? followed by CP1252 0x92 and expects ??'.
2. The incomplete-final-byte regression uses the injected no-legacy path for its platform-independent U+FFFD expectation and separately asserts CP1252 output, so Windows CI no longer inherits the host ACP accidentally.

The earlier requested upstream follow-up 0a85b13ba remains incorporated without an automated co-author trailer, which this repository's contribution policy rejects. The rebuilt candidate remains one DCO-compliant topic commit on public r10.

Verification on the exact candidate

• cargo metadata --locked --no-deps --format-version 1 passed.
• cargo fmt --all -- --check passed.
• git diff --check passed.
• Shell output module: 10/10 passed on Windows.
• Newly added shell lifecycle regressions: 6/6 passed.
• All forkguard_* tests: 42/42 passed.
• Commit author/co-author policy check passed for the topic commit.
• A broader concurrent Windows tools::shell run passed 100/104. Three timing-sensitive failures passed when rerun individually with one test thread; the remaining read-only-policy test could not locate canonical pwd in this local environment. These are disclosed as local environment/scheduling limitations, not claimed as a full-suite pass.

The exact candidate is 66 files, +6389/-794 (net +5595), and 15 commits over upstream v0.9.5, including the already-published r10 maintenance history.

Release request

• Keep existing immutable tags through pinvou-v0.9.5-r10 unchanged.
• Merge this PR into pinvou3-clean after its required checks and advisory disposition are complete.
• Create immutable tag pinvou-v0.9.5-r11 at the resulting final maintenance head.
• If the merge method produces a SHA other than the reviewed candidate, report the final SHA and rerun the candidate-specific checks before the parent repository is marked Ready.

Risk

ACP detection is deliberately a fallback after invalid UTF-8 and cannot infer OEM or arbitrary child-selected encodings. The decoder switches one-way to its resolved legacy encoding after a genuine invalid UTF-8 suffix. The change is confined to the shell byte-decoding lifecycle and does not modify sandbox, PowerShell, Python, or command-generation behavior.

No-Issue: publishes the downstream-reviewed Windows shell output decoding fix on the Pinvou maintenance branch.

[github-actions]

Thanks @zhuowp for taking the time to contribute.

This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered.

Please read CONTRIBUTING.md for the expected contribution shape. A maintainer can grant recurring PR access by commenting /lgtm on a pull request.

[asto18089]

Independent review from the parent PR (Pinvou/pinvou-agent#348): the decoder implementation is sound and matches what upstream accepted into the 0.9.12 integration branch. One gap before publication:

Missing upstream follow-up 0a85b13ba. Upstream added #[cfg_attr(not(windows), allow(dead_code))] on legacy_encoding_for_code_page in crates/tui/src/tools/shell/output.rs, because in production the function is only called from the #[cfg(windows)] system_legacy_encoding, so a plain non-Windows lib build with -D dead_code fails. The candidate c973c0d5 does not include this annotation. Please cherry-pick 0a85b13ba (or apply the equivalent one-line annotation) into this branch before merge, so the published r9 tag does not carry a latent non-Windows strict-build failure that upstream already fixed.

Also noting for the record: this PR currently has no approvals, and cargo-audit / cargo-deny (advisories) are red — they look like repo-level pre-existing advisories unrelated to this patch (which only touches crates/tui shell files and adds one windows crate feature), but please confirm that explicitly before merging.

[zhuowp]

Independent self-review result for exact head 22ec868291922aa133197dd9736a0da31ae18329: APPROVED; no actionable code, policy, or metadata issue remains.

The explicit review request is resolved: the upstream 0a85b13ba non-Windows -D dead_code follow-up is present above legacy_encoding_for_code_page, including the English production/test explanation and #[cfg_attr(not(windows), allow(dead_code))]. The rebuilt candidate is one DCO-compliant topic commit on public r10 (feb8761a), with no automated co-author trailer.

Exact-head evidence: locked metadata, formatting, diff check, 9/9 output-module tests, 6/6 new shell-lifecycle regressions, 42/42 forkguard_* tests, and author/co-author policy validation passed. Current Ubuntu check, DCO, Gitleaks, licenses/bans/sources, and link checks pass.

Publication remains blocked separately by the repository-level h2/lru advisory jobs. Those findings require explicit disposition before merge and r11 publication; this code approval does not waive them.

[asto18089]

Review findings from the parent-side integration review (Pinvou/pinvou-agent#348), re-verified directly against candidate 22ec86829. The decoder genuinely fixes both stated root causes — split UTF-8 sequences across pipe polls and Windows ACP output — with correct layering and no wheel reinvention. Two defects should be fixed before this merges:

1. MAJOR — the hard-invalid branch re-decodes the valid UTF-8 prefix as ACP (reverse mojibake)

crates/tui/src/tools/shell/output.rs, the Err(_) arm of ShellStreamDecoder::decode (~line 64-73): when from_utf8 fails, the entire pending_utf8 — including the prefix that error.valid_up_to() already certified as valid UTF-8 — is handed to decode_legacy_chunk. Example on CP1252: pending = 中文 (valid UTF-8) + one stray 0x92 → the whole buffer is re-decoded as CP1252 → 中文. The valid Chinese is destroyed instead of only the stray byte.

Impact is bounded to one pending buffer in the delta path (pending is drained to ≤3 bytes between polls), but the synchronous paths (shell.rs execute calling decode_shell_bytes(&stdout, true) once over the full output) are strictly worse than the old from_utf8_lossy there: for mostly-valid-UTF-8 output containing one stray byte, the old code preserved the valid text and the new code mojibakes all of it. For DBCS ACPs (GBK/Big5/Shift-JIS) the legacy decoder can additionally pair the stray lead byte with a following ASCII byte and shift alignment inside the buffer.

Suggested fix: in the Err(_) arm, emit pending_utf8[..valid_up_to] as UTF-8 first, then start the legacy decoder from valid_up_to. The pure-ACP-stream concern (a byte sequence that happens to be valid UTF-8 would be emitted as UTF-8 instead of ACP) is not a regression in practice: real ACP text almost always breaks UTF-8 validation within the first few characters, so the stream converges to legacy decoding almost immediately either way.

2. MAJOR — running_full_decoder_keeps_incomplete_utf8_private_until_final will fail on the Windows CI leg

crates/tui/src/tools/shell/output.rs (~line 296-299):

assert_eq!(decode_shell_bytes(b"ready \xE4", true), "ready \u{FFFD}");

decode_shell_bytes resolves system_legacy_encoding(), which on Windows returns the mapped ACP (GitHub windows-latest runners default to CP1252). With last=true, the dangling \xE4 falls through the incomplete-only guard into Err(_) and legacy-decodes to ä, so the assertion expecting U+FFFD goes red. The CI Test job runs cargo test --workspace --all-features on windows-latest for heavy changes, and this change is heavy.

Suggested fix: test the stateless path via the injectable helper (like the other tests do) so the non-Windows semantic is pinned cross-platform:

decode_shell_bytes_with_legacy(b"ready \xE4", None, true)

(The underlying semantic — dangling final byte decodes via ACP on Windows — is defensible; it just needs a dedicated Windows expectation rather than being asserted as U+FFFD unconditionally.)

Minor items, non-blocking: the one-way legacy switch is claimed semantics but combined with finding 1 one stray byte poisons the rest of the stream (fixed by the same change); stable_utf8_tail_start's None fallback can re-introduce leading U+FFFD in tails on non-Windows where the old forward-snap loop avoided it; no test covers the mixed valid-UTF-8+ACP buffer of finding 1 — worth adding one alongside the fix.

Since the parent PR pins the exact candidate SHA, any fix here changes 22ec86829, and the parent gitlink/fingerprints/registers must be realigned per the documented procedure before pinvou-v0.9.5-r11 is tagged.

[zhuowp]

Addressed both findings from the latest parent-side review in exact head 654490026edf2a3105858ff644c7087a23dd5f6c.

• The decoder now emits the valid_up_to() UTF-8 prefix before passing only the invalid suffix into the legacy decoder. A mixed-buffer CP1252 regression verifies that ?? plus 0x92 becomes ??', without reverse mojibake.
• The final incomplete-byte test now injects None for the cross-platform U+FFFD contract and separately verifies the CP1252 result, so it is independent of the Windows runner's active code page.

Exact-head verification: output decoder 10/10, new shell lifecycle regressions 6/6, forkguard_* 42/42, locked metadata, formatting, diff check, and topic-commit author/co-author policy all pass. The candidate remains one DCO-compliant topic commit on public r10, with 15 commits and 66 files (+6389/-794) over upstream v0.9.5.

For transparency, a broader concurrent Windows tools::shell run was 100/104: three timing-sensitive cases passed on individual single-thread reruns, while the read-only-policy case was blocked because this environment lacks canonical pwd. The PR body records this without claiming a full-suite pass.

Parent Hmbown#348 will now be realigned to this exact SHA and revalidated. Repository-level advisory disposition remains a separate publication blocker.

[h3c-hexin]

Follow-up work is available in #28. The original head repository is no longer available for additional commits or workflow dispatch, so #28 continues from a Pinvou-owned branch while preserving @zhuowp as the author of the signed topic commit. It adds the behavior-neutral Clippy fix, deterministic cross-platform test coverage, and a native Windows shell regression job. No merge or closure action has been taken on this PR.

[h3c-hexin]

Superseded by #29, which has now been merged into pinvou3-clean. #29 preserves @zhuowp’s original implementation, authorship, and Signed-off-by trailer, and adds the review follow-ups and native Windows regression coverage.