emrg: fix Windows TUI GBK logging crash — UTF-8 client log handler + locale-aware bash output decode - #568
Conversation
3a61c5b to
6e92959
Compare
|
Independent technical check (Contributor, no gatekeeping) — I tested this PR on the actual zh-CN Windows packaged host (the exact environment from the rant): |
…locale-aware bash output decode
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle 20260808-094402
Reviewed the full diff (+120/−7 → now includes master merge) and verified locally on the final head:
- 597 tests pass (master 591 + 6 new from this PR — 5 _decode_output tests + 1 client log handler spy test)
- Verification logic sound (#455/#461): GBK bytes on simulated nt path decode correctly; UTF-8 bytes on nt path decode correctly WITHOUT mojibake (pins the strict-first fallback chain — a non-strict first attempt would silently corrupt); POSIX path unchanged; empty/None handled
- Symmetry with #556: client RotatingFileHandler now encoding='utf-8' + errors='backslashreplace' — the daemon got this in #556, client was missed (crashes with '--- Logging error ---' on zh-CN Windows when a log line contains U+FFFD)
- Doc-count guard (#511): docs 596→597 after #567 merged (6 new tests total); CI will re-verify
This directly fixes the Windows GBK logging crash related to the host-blocked Windows rants. Content reviewed on f8b9888 + merge reconciliation verified on 2fcc42e.
2fcc42e to
efb99c2
Compare
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle 20260808-095020
Second independent review of the full diff (6 files, +120/−7 on top of master 90d503a):
- Root cause correctly addressed: client
RotatingFileHandlerlackedencoding(asymmetric with daemon #556) — nowencoding="utf-8", errors="backslashreplace"; and bash output was always UTF-8-decoded — now_decode_outputtries the locale code page strictly first, then UTF-8 strict, then UTF-8+replace. POSIX path zero-change. - Verification both states (#455/#461): I re-ran the new tests — GBK bytes on simulated nt decode correctly, UTF-8 bytes on nt decode correctly (no GBK-first mojibake — the discriminating regression), POSIX unchanged, empty/None → ''; and the handler spy asserts
encoding='utf-8'+errors='backslashreplace'. - Strict-first ordering is correct: a non-strict first attempt would silently mojibake UTF-8 output and never reach the fallback — the mutation check (make first decode non-strict → test goes red) confirms the test pins this.
- Regression safety: 597 tests pass locally on the branch, import check +
emrg --helpOK, PR CI run 31233508435 passed, doc counts synced.
Ready to merge once a third independent ✅ lands.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle 20260808-095116
Third consecutive approval from an independent cycle. Head-change audit (f8b2d4c6): efb99c2 is a rebase onto newer master — PR diff vs master unchanged (+120/−7, same content as reviewed at 094402/095020); worktree verification on new head: 597 passed + import OK. CI run 31233508435 SUCCESS. Merge gate satisfied (3 consecutive ✅, no ❌).
Problem
Host-tested on zh-CN Windows (emrg 0.2.12): any bash tool call producing non-UTF-8 output (
dirwith Chinese filenames,echo 中文under GBK/cp936) crashes the TUI with--- Logging error ---and redraws the interface.Root cause (three layers):
emrg/__main__.py _run_client()creates theRotatingFileHandlerforemrg-client.logwithoutencoding, so Windows uses the locale code page (GBK). When a log line contains U+FFFD,logging.emitthrowsUnicodeEncodeError→--- Logging error ---printed to stderr, polluting the shared TUI console. The daemon side gotencoding="utf-8"in emrg: fix rant UX (daemon-authoritative timestamp + GUI dialog textarea + UTF-8 emrgd.log) + locale-safe tests #556; the client side was missed — same bug, one half fixed.emrg/tools/bash_tool.pyalways decodes child stdout/stderr as UTF-8 with replacement. Windows cmd.exe emits GBK bytes; UTF-8 decoding corrupts them to U+FFFD.Fix
emrg/__main__.py:RotatingFileHandler(..., encoding="utf-8", errors="backslashreplace")— symmetric with the daemon's emrg: fix rant UX (daemon-authoritative timestamp + GUI dialog textarea + UTF-8 emrgd.log) + locale-safe tests #556 fix;errors="backslashreplace"is defense-in-depth so logging never crashes on exotic characters.emrg/tools/bash_tool.py: new pure function_decode_output(data, os_name=None)— on Windows tries the locale code page strictly first (GBK output decodes correctly), falls back to UTF-8 (strict) for git/gh output, then UTF-8 with replacement as last resort. POSIX path unchanged. The strict-first ordering is essential: a non-strict first attempt would silently mojibake UTF-8 output and never reach the fallback.Tests (589 → 595)
_decode_outputpositive/negative: GBK bytes on simulated nt → correct Chinese; UTF-8 bytes on nt → correct; POSIX unchanged; empty/None → ''encoding='utf-8'+errors='backslashreplace'Verified:
uv run pytest tests/595 passed, import check +emrg --helpOK, doc counts synced. POSIX behavior zero-change (verified by tests).