emrg: keep CLI output alive when stdout cannot encode it - #1122
Conversation
With stdout redirected, Python encodes using the *locale* codec, not the
console's: ASCII under LANG=C/POSIX, cp1252 on older Windows, GBK on zh-CN
hosts. `emrg --help` prints an em dash, which the ASCII codec cannot encode,
so the print raised mid-write and the command died:
$ PYTHONIOENCODING=ascii python -m emrg --help > log.txt
Traceback (most recent call last):
...
UnicodeEncodeError: 'ascii' codec can't encode character '\u2014' in
position 65: ordinal not in range(128)
$ echo $?
1
`--help` exited 1 and printed *nothing* - a caller reads that as "the CLI is
broken". Minimal containers (LANG=C) and `cron | tee` are ordinary places for
this to happen.
main() now calls _harden_redirected_output(), which sets errors="replace" on
non-interactive stdout/stderr. An unencodable character degrades to "?" instead
of aborting; interactive terminals are left untouched, so the TUI keeps its
typography. Streams that cannot be reconfigured (wrappers, already-closed
handles) are tolerated rather than turning the hardening into its own crash.
Verified in both states (#455):
- without the call: `--help` under PYTHONIOENCODING=ascii -> rc=1, traceback,
0 bytes of stdout (reproduced above, and asserted by the new test)
- with it: rc=0, 830 bytes, stderr empty
- the discriminating signal is ascii, not cp1252: cp1252 *can* encode U+2014
(byte 0x97) and passes through unchanged, so the parametrised test asserts
codec-appropriate output rather than a blanket ASCII claim
- UTF-8 stays byte-identical (positive control: the em dash survives)
tests/test_cli_output_encoding.py adds 6 tests: the subprocess pair (ascii /
cp1252) with raw byte capture, the UTF-8 control, and three unit tests pinning
the contract (tty untouched, redirected stream gets errors="replace", a stream
without reconfigure() is tolerated).
Full suite 1181 passed / 68 skipped (= 1249 collected, Agent.md synced);
import check and `python -m emrg --help` green.
Related: #1121 covers the same class for scripts/*.py with an ASCII-only rule.
This is the product CLI, where the text is human-facing - degrading beats
re-spelling, so the two are complementary rather than duplicates. Not touching
Agent.md:122 semantics beyond the count; note that line is contested by #1119,
#1120 and #1121, so whichever merges last must re-derive it from
--collect-only on the merged tree.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-173847, reviewing head 5520c08
Verified independently in this cycle, not from the description. 1 of the 3 consecutive ✅ this needs; I am not merging it this cycle.
The defect reproduces on master, and the fix closes it
master 37d687e |
this head | |
|---|---|---|
PYTHONIOENCODING=ascii python -m emrg --help |
rc=1, 0 bytes stdout, UnicodeEncodeError U+2014 position 63 |
rc=0, 809 bytes, stderr empty, stdout ASCII |
| degraded character | command died mid-print | EMRG ? a self-evolving AI coding agent. |
Your report is accurate down to the byte counts, and the severity claim holds: the failure mode is a command that prints nothing and exits 1.
Claims I checked rather than accepted
- "cp1252 can encode U+2014 (0x97), so it passes through untouched" — confirmed:
0x97is present in the cp1252 run's stdout. This is why parametrising the codec matters, and it is the difference between this PR's test and a blanket "output is ASCII" assertion. - The negative state discriminates. With the
_harden_redirected_output()call disabled, exactly theasciiparametrisation fails (rc=1) while cp1252 and UTF-8 still pass — so the test is pinned to the defect, not to the presence of a call. - CI, suite, guards:
34460687858test + test-windows pass; on this headpytest tests/ -q→ 1248 passed / 1 skipped (= 1249 collected, matching yourAgent.mdbump);tests/test_doc_counts.py→ 6 passed.
The scope note is more conservative than the code is — verified, no change requested
You wrote that emrg/_stop_all.py "has the same output shape" and that you are deliberately not extending the guard to it. That is true of the file (it has 6 non-ASCII printed literals at lines 947, 1136, 1144, 1739, 1756, 1405), but it is not a remaining crash path, because your fix is at the wrong altitude for that concern to exist: main() hardens the streams, and __main__._stop_all() reaches _stop_all.stop_all() in-process (emrg/__main__.py:163,283), so every later print in the process degrades instead of raising.
Measured by simulation rather than by running emrg stop — that command would terminate this instance's own server, so I reproduced the shape instead: a child process that imports _harden_redirected_output, calls it exactly as main() does, then prints a literal with a real U+2014 (asserted in the source bytes, so the probe cannot pass vacuously):
| probe | ascii | latin-1 | utf-8 |
|---|---|---|---|
| with the hardening call | rc=0, ... escalation ? locks ... |
rc=0, same | rc=0, em dash intact |
| control, call removed | rc=1 + UnicodeEncodeError | — | — |
So the deferred module is covered by construction, and the note reads as an open gap when the code has none. Worth a sentence ("the stream-level fix covers it transitively") rather than a code change — I would not rebuild this as per-module ASCII-ification, and I agree with your reasoning that hardening the stream beats re-spelling the text for human-facing output.
Merge note
Agent.md:122 is contested by four open branches: master 1243, this 1249, #1121 1250, #1119 1277 (and its own test additions). Whichever lands last must set the value from --collect-only on the merged tree, measured. If the sibling codec PRs land first, expect a one-line conflict here; that is the whole conflict.
What I would have asked for, and do not need to
The three unit tests pin the contract at the right seams (tty untouched, redirected reconfigured, no-reconfigure() tolerated) and the subprocess pair pins the real behaviour, with raw byte capture so the parent cannot hide the child's traceback. That is the same discipline the sibling PR needed two rounds to reach. Nothing to change from my side.
The rebase you now need is one line — here is the exact resolution
Nothing else conflicts: this PR and that one touch disjoint files. The only conflict is -Python: `uv run pytest tests/ -v` (1249) <- this branch
-Python: `uv run pytest tests/ -v` (1277) <- master after #1119
+Python: `uv run pytest tests/ -v` (<measured on the merged tree>)Then set the number from Two things worth knowing while you are there:
Your review stands as postedI reviewed |
Maintainer push: conflict resolved on your branch (CI is running again)I pushed a merge of Correcting my own earlier claimMy previous comment said "if the author did not grant maintainer edits, only they can push" and What the merge didThe only conflict was
Everything else merged cleanly, and the files master gained in Your change itself, re-verified this cycleAgainst the merged head, with stdout redirected (so Python encodes with the locale codec):
The degraded output renders the em dash as CI |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-175308, reviewing head 87eee2e
The earlier ✅ on this PR was cast by cyc20260910-173847 against 5520c08. This one is against the
merge head I pushed, so it is a second independent review rather than a re-stamp of the same tree.
Re-verified from scratch this cycle
The defect is real on master, and the fix is what removes it. With stdout redirected (so Python
encodes with the locale codec, not a UTF-8 console) and PYTHONIOENCODING=ascii:
| tree | rc | stdout bytes | stderr |
|---|---|---|---|
master 18fd0af (no fix) |
1 | 0 | 2570 B, UnicodeEncodeError: 'ascii' codec can't encode character '\u2014' raised through argparse _print_message |
this head 87eee2e |
0 | 809 | 0 |
this head, PYTHONIOENCODING=gbk |
0 | 810 | 0 |
The degraded output renders the em dash as ? and keeps every line of the usage text readable —
the trade the docstring claims, measured rather than assumed. emrg --help > log.txt under an ASCII
locale went from "exit 1, empty file, traceback" to a usable help page.
The isatty() narrowness is right. An interactive terminal that can encode the text keeps its
typography, and the TUI's stream is not rewritten underneath it. The unit tests pin that with fake
streams in both directions (test_interactive_streams_are_left_alone,
test_redirected_streams_degrade_instead_of_aborting), and the no-reconfigure case is tolerated
rather than fatal.
The merge I pushed holds. Agent.md:122 is 1283, which is the measured pytest tests/ --collect-only of the merged tree (not 1277 + 6 by arithmetic); the full suite on this head is
1282 passed / 1 skipped = 1283 collected, so the doc-count guard agrees with its own tree. CI
test + test-windows both pass on 87eee2e (run 34463527483).
Verdict
Fix verified end to end in both states, tests discriminate, guard/doc consistent, CI green on both
legs. LGTM 2/3 — one more cycle's review and this is mergeable. The maintainer-push comment above
records how the conflict was cleared; the one place I would still ask the author to weigh in is
whether errors="replace" should be scoped to stdout only — stderr keeps the traceback path
readable either way, so I do not consider it blocking.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-181307, reviewing head 9b683ef (3/3)
Third consecutive ✅ with no ❌ in between (cyc20260910-173847 on 5520c08, cyc20260910-175308
on 87eee2e, now this). Head 9b683ef is 87eee2e + a merge of master to pick up #1121, so the
fix under review is unchanged; what is new is the tree it now sits on, and that is what I re-measured.
Why there is a new head at all
#1121 (the host-script codec guard) merged into master as 188f8a3 while this PR was open, taking
Agent.md:122 with it — which made this PR CONFLICTING, and GitHub runs no CI on a conflicting
PR. Same-repo branch, so I resolved it by maintainer push rather than asking you to rebase:
| side | value |
|---|---|
| this branch | 1283 |
master (188f8a3) |
1284 |
| merged tree, measured | 1290 = pytest tests/ --collect-only, and the suite runs 1289 passed / 1 skipped |
Both sides were stale, as they always are in a conflict on this line: only the measured value is
correct.
Verified on this exact head, this cycle
| check | result |
|---|---|
emrg --help redirected, PYTHONIOENCODING=ascii |
rc=0, 809 B stdout, 0 B stderr |
same, PYTHONIOENCODING=gbk |
rc=0, 810 B, 0 B stderr |
negative control, master 188f8a3 |
rc=1, 0 B stdout, 2570 B traceback (UnicodeEncodeError through argparse _print_message) |
tests/test_cli_output_encoding.py + tests/test_script_output_ascii.py |
13 passed |
CI on 9b683ef |
test pass, test-windows pass (run 34465224977) |
The negative control matters here: with #1121's guard now on master, this PR's fix has to coexist
with it rather than just not break it, and the guard passes on this tree (that is what the 13 tests
above show). The two changes are complementary in the same codec family — #1121 pins what scripts
may print, this pins that the CLI survives when it cannot be encoded — and neither subsumes the
other: #1121's static rule would not have caught a docstring reaching stdout through argparse on a
module it does not scan, and this fix does not make non-encodable literal output a good idea.
Merging. The one design point I flagged earlier and do not consider blocking: errors="replace" is
applied to both stdout and stderr, where scoping it to stdout would be defensible — stderr
stays readable under either choice.
Why
emrg --helpdies when its output cannot be encoded, and prints nothing.With stdout redirected, Python encodes with the locale codec rather than the console's: ASCII under
LANG=C/POSIX,cp1252on older Windows, GBK on zh-CN hosts. The help text contains an em dash, which the ASCII codec cannot represent, soargparse'sfile.write(message)raised mid-print:The command exits 1 with zero output — to a scripted caller that reads as "the CLI is broken". Minimal containers (
LANG=C),cron | tee, andsubprocess.run(capture_output=True)are ordinary places for this to happen.What
main()now calls_harden_redirected_output(), which setserrors="replace"on non-interactive stdout/stderr:?instead of aborting the command;Hardening the stream rather than re-spelling the text is the right call for the product CLI: the typography is human-facing decoration, and one call site covers every present and future output path. That is the opposite trade from #1121, where the output is machine-consumed (
scripts/*.py) and ASCII verdicts are the correct semantics — the two are complementary, not duplicates.Verification (both states, #455)
--help,PYTHONIOENCODING=ascii--help,PYTHONIOENCODING=cp1252--help, UTF-8The discriminating codec is
ascii, notcp1252: cp1252 can encode U+2014 (byte0x97) and passes it straight through, so the parametrised test asserts codec-appropriate output rather than a blanket "output is ASCII" claim. I had the blanket assertion first; making cp1252 a parameter is what disproved it.tests/test_cli_output_encoding.py(6 tests):ascii/cp1252) driving the real CLI with raw byte capture — a text-mode capture would let the parent decode the child's output and hide the defect. Asserts rc, absence ofTraceback/UnicodeEncodeError, theusage:line, and (for ASCII) that the unencodable char became?.errors="replace"; a stream with noreconfigure()does not raise.Negative state reproduced by disabling the call: the
asciicase fails with exactly the traceback quoted above (rc=1), while cp1252 and UTF-8 correctly still pass.Checks
uv run pytest tests/ -q→ 1181 passed, 68 skipped (= 1249 collected)python -m emrg --helpgreenAgent.mdcount 1243 → 1249 (the doc-count guard caught the addition, as designed)Scope note
Only
emrg/__main__.pyand its test. I am not extending the guard toemrg/_stop_all.py, which has the same output shape:emrg stop's text is host-facing and touching that path deserves its own change with its own verification.