[zhuowp]

Summary

Fix Windows Python Chinese output becoming garbled during ordinary shell execution. Every such execution calls with_env, even when no hook/task environment is supplied. It previously discarded the constructor's PYTHONIOENCODING=utf-8, so Python fell back to its locale encoding while PowerShell expected UTF-8.

Merge overlays instead of replacing defaults, and handle explicit Windows variable names case-insensitively. This revision leaves the reviewed production fix unchanged and hardens the Windows test against inherited UTF-8 workarounds, enforces its timeout, and explains how to select PowerShell for opt-in Python tests.

Verification

• cargo test -p codewhale-tui --lib --locked sandbox::tests -- --test-threads=1: 23 passed, 0 failed. Cargo's substring filter selects 19 sandbox::tests::* plus 4 repl::sandbox::tests::* in this maintenance fork.
• Strengthened default regression: the test locks the process environment, sets PYTHONIOENCODING=gbk, and bounds shell execution to 30 seconds with child cleanup. Reintroducing the old replacing with_env under an outer PYTHONIOENCODING=utf-8 made it fail (0 passed, 1 failed), proving an inherited UTF-8 workaround cannot hide the defect. The final source was restored byte-for-byte and all positive checks rerun.
• Actual lowercase bash tool and background ShellManager: 2 passed, 0 failed with system Python 3.14 and 2 passed, 0 failed with bundled Python 3.13.14. These opt-in tests cover Python stdout/stderr under a GBK parent environment and 64 KiB of preceding output. In the earlier verification, both tests failed for each interpreter when only the original environment builders were restored, with corrupted Python Chinese and intact PowerShell Chinese.
• Separate Git Bash child-environment regression: 1 passed, 0 failed. The normal PowerShell run also exercises encoded PowerShell commands.
• Rust formatting and whitespace checks passed.

Opt-in command: cargo test -p codewhale-tui --lib --locked forkguard_windows_python_stdio -- --ignored --test-threads=1. Use PowerShell or cmd with SHELL unset; set CODEWHALE_TEST_PYTHON to select an interpreter.

Coverage and integration boundaries

Windows encoding evidence above comes from local author executions. The maintenance fork's pinvou3-clean CI is Ubuntu-only; it does not execute the Windows-gated regressions. Upstream codewhale-hq#7041 is the path for eventual upstream Windows CI coverage, not evidence that a native Windows CI lane has already run.

The common shell builder covers foreground, background/PTY, interactive, and encoded PowerShell retry paths. The hardened read-only Python lane and codew sandbox run use direct CommandSpec::program construction, which already lacked this shell default before the change; those paths are outside this fix's claim. Python file/default-text encoding, explicit environment overrides, and the child-environment sanitizer remain unchanged.

No PowerShell 7 verification, customer-device replay, installer build, or release was performed.

Review candidate: af50096ff843617f3f68f47bc6fc45bbffe3c937. Target pinvou3-clean. Parent Pinvou/pinvou-agent#750 must replace its review-candidate gitlink with the actual public maintenance commit after this PR merges, recount the history, and pass the unchanged public submodule gate. The immutable baseline tag stays unchanged.

No-Issue: mirrored upstream in codewhale-hq#7041; parent integration tracked in Pinvou/pinvou-agent#750.

[JensenChen28]

Standards

No findings. The overlay reuses the single-variable builder; Windows casing handling is platform-scoped, and execution still uses the existing child-environment sanitizer.

Spec

No findings. Empty and non-empty overlays preserve the shell UTF-8 stdio default, explicit overrides remain possible, and foreground/background/PTY/interactive/encoded retry paths share the corrected builder. Python file/default-text encoding is unchanged. Tests cover the reported failure modes and disclose native opt-in requirements.

Validation: reviewed both changed files and production consumers, checked repository/TUI guidance, whitespace, and a read-only integration simulation against the current maintenance branch. Current-head fork CI is green. I did not run Cargo or native Python integration tests locally; the reported 23 sandbox and interpreter regressions are author-provided evidence, not this reviewer's executions.

Summary: Standards 0 findings; Spec 0 findings. Approval covers this candidate only.

[JensenChen28]

CI follow-up: the link job is the PR closes an issue workflow, not a native-link failure. Its latest log fails because the PR body has neither a closing keyword nor an explicit opt-out. Please add a real Closes #... reference, or a truthful No-Issue: <reason> line as permitted by .github/workflows/pr-issue-link.yml, and rerun the check. My source approval is unchanged; I have not changed PR metadata or bypassed this gate.

[asto18089]

Round-1 review (fresh, 5 parallel lanes + local verification)

Rebase state checked first: head 104649398 sits directly on the current pinvou3-clean tip 083cebfd9 (merge-base == base head), so no rebase was needed.

Verified correct

• Root cause TRUE, fix complete for the claimed scope. Old with_env did self.env = env, and the overlay at tools/shell.rs:2184/:2303 is unconditional, so the Windows PYTHONIOENCODING=utf-8 seed (windows_shell_default_env, sandbox/mod.rs:298) was wiped on every exec_shell call, not just hook/task-overlaid ones — the body actually understates the blast radius. All four claimed paths (foreground sync, background/PTY via apply_to_pty_command, interactive, encoded PowerShell retry at :2245-2254) route through the fixed builder and now preserve the seed; no remaining clobber path exists (only .env = left is MCP config; terminal_session struct literal is unix-only).
• Elegant, no reinvention. Production code is byte-identical to upstream codewhale-hq#7041 (fork adds a 2-line comment); the fork's test set is a superset of upstream's. Minimal diff, correct retain semantics (matches Rust std's Windows EnvKey ASCII case folding and the repo's own child_env::normalize_key).
• No smuggling. Exactly 2 files, every hunk on-topic. Commit message conventional, DCO signs off.
• Sanitizer preserved. child_env.rs untouched; hook/task vars were already injected as trusted overrides under the old replace semantics, so the merge can only add back the constant utf-8 default — no weakening path.
• No caller regression. All 3 production with_env call sites audited: two want merge (the fix), one is a map-onto-itself no-op. Off-Windows the change is provably a no-op (seed map is empty ⇒ replace ≡ merge). Perf: worst case ~30×30 ASCII compares per spawn — negligible.
• Linux verification. Full cargo test -p codewhale-tui --lib on this head: sandbox 84/84, shell-tool 125/125, ~12.2k total identical to base (+1 = the new passing test); cargo fmt --check and git diff --check clean. (Two runtime_api stack overflows and a few remote_control flakes reproduce identically on base 083cebfd9 — pre-existing environment issues, not this PR.)
• Required checks (Check Signed-off-by, gate, Gitleaks, check) all green; parent integration PR codewhale-hq#750 registers the topics/fingerprints correctly.

MAJOR

M1 — forkguard_windows_shell_env_overlays_keep_python_stdio_utf8 can pass with the bug reintroduced; its mutation kill is environment-dependent. (sandbox/mod.rs:974-1022) Unlike the two opt-in tests in tools/shell/tests.rs:284-285, this test sets no parent sentinel. It spawns via apply_to_command → sanitized_child_env, which passes the parent process env and the Windows registry Environment keys (both allowlisted for PYTHONIOENCODING). Revert with_env to self.env = env on any machine whose process env or registry carries PYTHONIOENCODING=utf-8 — the standard workaround for exactly this garbling bug, and therefore likely present on machines that run Windows regressions — and the child still prints utf-8, the assert passes, and the buggy code ships green. Please add lock_test_env + EnvVarGuard::set("PYTHONIOENCODING", "gbk") here, mirroring the opt-in tests, so the kill is deterministic. This matters doubly because parent codewhale-hq#750 fingerprints these test functions as fork-behavior pins; a self-satisfying pin is worse than none.

M2 — No CI lane will ever run any of the four Windows-gated tests on pinvou3-clean; say so plainly in the PR body. fork-ci.yml (the only workflow triggered on pinvou3-clean PRs/pushes) is ubuntu-only (all three jobs runs-on: ubuntu-latest); the windows-latest legs in ci.yml trigger only on master/main. So on this branch, the only mutation-killing test CI executes is the cross-platform builder test command_spec_env_overlays_preserve_existing_values — which covers the merge but neither the #[cfg(windows)] retain nor any end-to-end encoding behavior. The Windows verification in the body is honest but dev-machine-only; one sentence in the body making that explicit (and noting upstream codewhale-hq#7041's CI as the eventual net) would close the gap. Adding a Windows lane is the maintainer's call; the disclosure is the ask.

MINOR

• m1 — link check is red. The body has neither a closing keyword nor No-Issue:. Not a required check, but every recently merged fork PR (#107, #108, #111, #112, #113) carries No-Issue: — one line, e.g. No-Issue: mirrored upstream codewhale-hq#7041; parent integration tracked in Pinvou/pinvou-agent#750.
• m2 — "Sandbox tests: 23 passed" is not reproducible from any natural filter. At this head, sandbox::tests counts 19 Windows-compilable tests; the whole sandbox:: module counts 59. If 23 came from a specific filter or command, please state it in the body (the same figure propagates to upstream codewhale-hq#7041 and the parent register).
• m3 — Residual garble path outside this PR's claim (pre-existing, follow-up candidate): the read-only hardened lane builds CommandSpec::program with an empty env (tools/shell.rs:2172-2179), so python script.py under a read-only policy on Windows still runs with locale stdio encoding; same for the codew sandbox run CLI path. Not this PR's scope and not a regression — worth an issue if the codewhale-hq#4202 objective is "all Windows Python stdio is UTF-8".

NIT

• windows_python_stdio_fixture's bare is_powershell() assert (tools/shell/tests.rs:215-219) panics without a message when the opt-in command is run from Git Bash (SHELL is set → dispatcher picks bash). A message pointing at "run from PowerShell/cmd" would save the next person a confusing failure.
• The sandbox e2e test drives cmd.output() directly, so the 30 s spec timeout is never enforced (a wedged shell hangs cargo test). Matches the existing forkguard_powershell_encoded_spec_* convention, so noting only for awareness.

Verdict: REQUEST CHANGES — the code fix itself is correct, complete for its claim, and regression-free; the two MAJORs are about the durability of the regression net this fork's guard model depends on, and both are small fixes (a sentinel + a body sentence). Everything else can ride along in the same push.

[zhuowp]

Round-2 update: af50096ff addresses the review feedback without changing the reviewed production fix.

• M1: the default Windows environment probe now holds lock_test_env and sets a GBK parent sentinel. Mutation verification explicitly started from an outer PYTHONIOENCODING=utf-8, restored the old replacing with_env, and observed 0 passed / 1 failed with the child reporting gbk. The final source was restored byte-for-byte before the passing runs. This closes the inherited-workaround false-pass case.
• M2: the body and parent fork registers explicitly state that pinvou3-clean CI is Ubuntu-only and the Windows results are local executions. Upstream fix(shell): preserve Python stdio env defaults codewhale-hq/Codewhale#7041 is the contribution path for eventual Windows CI coverage; no unexecuted CI coverage is claimed.
• m1/m2: added the truthful No-Issue: entry and the exact Cargo filter. Its 23 passing tests are 19 sandbox + 4 REPL sandbox, because Cargo matches substrings. The upstream body separately records its 17 + 6 split.
• m3: documented the pre-existing direct-program/read-only/CLI boundary in both PRs and the parent registers; this shell-default fix does not claim those paths are repaired.
• Nits: the opt-in fixture explains the PowerShell requirement and SHELL setting. The default probe now enforces its 30-second timeout and kills the child on cancellation.

Final local verification: default filter 23/23, native foreground/background 2/2 with system Python 3.14 and 2/2 with bundled Python 3.13.14, separate Git Bash probe 1/1; workspace formatting and whitespace checks pass. Parent architecture and fast fork guard checks also pass (229 registered behavior names).

Parent codewhale-hq#750 still awaits this PR's actual squash-merged maintenance SHA and the separately reviewed dependency-advisory fix codewhale-hq#747; its public submodule gate remains unchanged. Please re-review this candidate.

[JensenChen28]

Reviewed current candidate af50096ff and the complete two-file increment from 104649398, together with the original environment fix and current review discussion. APPROVED.

Standards

0 new findings. The production builder/sanitizer is unchanged. The default Windows probe now serializes environment changes and installs a GBK parent sentinel before running, so an inherited UTF-8 workaround cannot satisfy the test after reverting the fix. Tokio timeout plus kill_on_drop bounds the direct child. The opt-in fixture now explains the PowerShell/SHELL requirement.

Spec

0 new findings. Both round-one major requests are addressed: the regression is structurally independent of the inherited UTF-8 workaround, and the body explicitly discloses Ubuntu-only maintenance CI, author-local Windows evidence and the pre-existing direct-program/read-only boundary. The exact substring filter and No-Issue entry clarify the smaller comments. No native Windows CI run is implied.

Actual reviewer validation: full diff/call-site inspection, git diff --check passed, production-prefix comparison unchanged, and current maintenance target independently confirmed at 083cebf. I did not run Cargo, native Codewhale/Windows regressions or the author's mutation tests locally; their counts remain author evidence. Current-head checks remain independent of this code approval. Parent candidate-to-actual-maintenance-SHA replacement and unchanged public-submodule gate are still integration requirements.

Summary: Standards 0; Spec 0. No merge, queue or auto-merge action.

[asto18089]

Round-2 re-review: round-1 fixes verified, approving

My round-1 CHANGES_REQUESTED items are all addressed in af50096ff (test-only on top of 104649398) and were re-verified fresh this round by independent review shards plus local runs on the pinned tree:

• M1 (environment sentinel): forkguard_windows_shell_env_overlays_keep_python_stdio_utf8 now takes lock_test_env + EnvVarGuard::set("PYTHONIOENCODING", "gbk") (sandbox/mod.rs:978-979) before probing, so an inherited or registry-provided UTF-8 workaround can no longer mask a reintroduced bug; mechanism re-derived via apply_to_tokio_command's env_clear + allowlisted parent env + upserted overrides.
• M2 (CI disclosure): the body now states plainly that pinvou3-clean CI is Ubuntu-only, the Windows evidence is author-local, and upstream codewhale-hq#7041 is the path to Windows CI coverage.
• m1: No-Issue: trailer present. m2: the body explains the 23 = 19 + 4 arithmetic — correct for Windows (a static cfg census gives 19 windows-runnable sandbox tests + 4 repl; Linux selects 22 + 4 = 26, measured locally at 26 passed / 0 failed; both are right on their own OS). m3: the direct-CommandSpec::program lanes (read-only lane shell.rs:2172, codew sandbox run lib.rs:10216) are disclosed as out of claim and predate this PR. NITs addressed: the fixture's dispatcher assert carries an actionable message, and the e2e probe now wraps cmd.output() in tokio::time::timeout(spec.timeout) with kill_on_drop(true), so a wedged shell can no longer hang the suite.
• Fresh correctness confirmation: production change (merge semantics + #[cfg(windows)] case-insensitive retain) is correct with no caller regression — all three production with_env call sites audited, none relied on replace-as-removal; sanitizer, explicit overrides, and Python's file/default-text encoding policy untouched. Mutation controls: reverting with_env to replace semantics is killed by the cross-platform unit test; the fork-guard fingerprints for all five new tests resolve at this head.

Standing disclosure (unchanged, now documented in the body): Ubuntu CI never compiles the cfg(windows) tests, so the Windows kill-power rests on author-local runs.