Skip to content

Commit 0b49e89

Browse files
committed
fix(cli): write stdout and stderr as UTF-8, not the locale encoding
- a redirected stream used cp1252 on Windows, so a German label reached the consumer as bytes no JSON parser could read, with exit code 0 - a character cp1252 cannot represent raised UnicodeEncodeError instead - the app callback reconfigures both streams before any output - errors= is passed too, or reconfigure would reset stderr to strict - --help and an unknown command name still bypass it; neither carries wiki content Closes #187
1 parent 996be9c commit 0b49e89

2 files changed

Lines changed: 211 additions & 0 deletions

File tree

‎src/osw/cli/main.py‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
from __future__ import annotations
1212

1313
import inspect
14+
import sys
1415
from typing import Any, Optional, get_type_hints
1516

1617
import click
@@ -36,6 +37,41 @@
3637
app = typer.Typer(no_args_is_help=True, add_completion=False)
3738

3839

40+
def _force_utf8_output() -> None:
41+
"""Encode stdout and stderr as UTF-8, whatever the locale asks for.
42+
43+
Python encodes a redirected stream with the locale encoding, which on a
44+
German Windows system is cp1252. A non-ASCII label then reaches the
45+
consumer as bytes no JSON parser can read, and a character cp1252 has no
46+
code point for -- Japanese, Greek, Cyrillic -- raises UnicodeEncodeError
47+
and ends the command. A Windows console stream is UTF-8 already, so on
48+
Windows only redirected output changes. Elsewhere a terminal uses the
49+
locale encoding, so this overrides a deliberate non-UTF-8 LANG or
50+
PYTHONIOENCODING too. stderr is covered as well as stdout, because
51+
``Context.guard`` sends captured stdout to stderr under ``--json``.
52+
53+
Called from the app callback, so it covers every command. Click prints
54+
help and rejects an unknown root-level name before any callback runs, so
55+
those paths keep the locale encoding. They carry no wiki content: every
56+
help string in this package is ASCII (held by a test), and rich
57+
substitutes its box-drawing characters once the stream is not UTF-8. What
58+
stays exposed is the name the user typed, echoed back in a usage error --
59+
an unknown command name or an unknown root option name. A name typed
60+
after the command is fine, because click resolves the command, runs this
61+
callback, and only then parses the command's own arguments.
62+
"""
63+
for stream in (sys.stdout, sys.stderr):
64+
reconfigure = getattr(stream, "reconfigure", None)
65+
errors = getattr(stream, "errors", None)
66+
# A stream a test harness or host application substituted may have
67+
# neither, and then decides its own encoding. Both are required:
68+
# errors= must be passed, because reconfigure() silently resets the
69+
# handler to strict otherwise, which would let stderr raise while
70+
# reporting a failure. Passing errors=None does exactly that too.
71+
if reconfigure is not None and errors is not None:
72+
reconfigure(encoding="utf-8", errors=errors)
73+
74+
3975
@app.callback()
4076
def _callback(
4177
ctx: typer.Context,
@@ -66,6 +102,8 @@ def _callback(
66102
# the prefix is always correct for any message printed on the way out,
67103
# including one printed while handling set_env_file_discovery's error.
68104
config.set_log_prefix("osw")
105+
# Before any output, including the configuration banner.
106+
_force_utf8_output()
69107
# The CLI's working directory is the one the user typed the command in, so
70108
# searching it upward for a .env is what they mean. The MCP server leaves
71109
# this off: its working directory is chosen by the MCP client.

‎tests/test_cli.py‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import json
1212
import logging
1313
import re
14+
import sys
1415
from unittest.mock import MagicMock
1516

1617
import click
@@ -292,6 +293,178 @@ def test_render_dict_shows_key_value_lines():
292293
assert "exists" in rendered
293294

294295

296+
# -- output encoding ------------------------------------------------------------
297+
# Redirected stdout on Windows is opened with the locale encoding, not UTF-8, so
298+
# a German label used to reach the consumer as cp1252 bytes. CliRunner's charset
299+
# gives the captured stream that same encoding, which reproduces the platform
300+
# behaviour everywhere, so these run on Linux CI too.
301+
_MISSING = object() # "do not set this attribute at all", distinct from None
302+
303+
304+
@pytest.fixture
305+
def cp1252_runner():
306+
return CliRunner(mix_stderr=False, charset="cp1252")
307+
308+
309+
def _fake_osw_labelled(monkeypatch, label: str):
310+
"""Patch in an entity whose label slot holds ``label``."""
311+
fake_osw, page = _fake_osw_with_page()
312+
page.get_slot_content.return_value = {"label": [{"text": label}]}
313+
page.get_url.return_value = "https://wiki.example.org/wiki/Item:OSW1"
314+
monkeypatch.setattr("osw.service.context.OswExpress", lambda **kwargs: fake_osw)
315+
316+
317+
def test_json_output_is_utf8_when_stdout_uses_the_locale_encoding(
318+
cp1252_runner, configured_env, monkeypatch
319+
):
320+
_fake_osw_labelled(monkeypatch, "Änderungen")
321+
322+
result = cp1252_runner.invoke(app, ["--json", "entity", "get", "Item:OSW1"])
323+
324+
assert result.exit_code == 0, result.stderr
325+
payload = json.loads(result.stdout_bytes.decode("utf-8"))
326+
assert payload["jsondata"]["label"][0]["text"] == "Änderungen"
327+
328+
329+
def test_human_output_is_utf8_when_stdout_uses_the_locale_encoding(
330+
cp1252_runner, configured_env, monkeypatch
331+
):
332+
_fake_osw_labelled(monkeypatch, "Änderungen")
333+
334+
result = cp1252_runner.invoke(app, ["entity", "get", "Item:OSW1"])
335+
336+
assert result.exit_code == 0, result.stderr
337+
assert "Änderungen" in result.stdout_bytes.decode("utf-8")
338+
339+
340+
def test_error_message_is_utf8_when_stderr_uses_the_locale_encoding(
341+
cp1252_runner, configured_env, monkeypatch
342+
):
343+
"""An error names the page it failed on, so stderr carries labels too."""
344+
fake_osw, _page = _fake_osw_with_page(exists=False)
345+
fake_osw.load_entity.return_value.entities = []
346+
monkeypatch.setattr("osw.service.context.OswExpress", lambda **kwargs: fake_osw)
347+
348+
result = cp1252_runner.invoke(app, ["--json", "entity", "export", "Item:Änderung"])
349+
350+
assert result.exit_code == 2
351+
assert "Item:Änderung" in result.stderr_bytes.decode("utf-8")
352+
353+
354+
def test_forcing_utf8_keeps_the_error_handler_each_stream_was_given(monkeypatch):
355+
"""``reconfigure`` resets ``errors`` to strict unless it is passed as well.
356+
357+
Python gives stderr ``backslashreplace`` precisely so that reporting a
358+
failure cannot itself raise. Switching the encoding must not drop that.
359+
"""
360+
err = io.TextIOWrapper(io.BytesIO(), encoding="cp1252", errors="backslashreplace")
361+
monkeypatch.setattr(sys, "stderr", err)
362+
monkeypatch.setattr(
363+
sys, "stdout", io.TextIOWrapper(io.BytesIO(), encoding="cp1252")
364+
)
365+
366+
cli_main._force_utf8_output()
367+
368+
assert err.encoding == "utf-8"
369+
err.write("\udc80") # a lone surrogate, which "strict" refuses to encode
370+
err.flush()
371+
assert err.buffer.getvalue() == rb"\udc80"
372+
373+
374+
def test_every_help_string_is_ascii():
375+
"""Guards the one gap ``_force_utf8_output`` cannot close.
376+
377+
Click prints help and rejects an unknown name before any callback runs,
378+
so those paths keep the locale encoding. That is only harmless while no
379+
help string contains a character the locale encoding may lack. Adding a
380+
German option description would make it a real defect, and this test is
381+
what reports it.
382+
"""
383+
offenders = []
384+
385+
def walk(command, path):
386+
texts = {"help": command.help, "short_help": command.short_help}
387+
for param in command.params:
388+
texts[f"--{param.name}"] = getattr(param, "help", None)
389+
for where, text in texts.items():
390+
if text and not text.isascii():
391+
offenders.append(f"{' '.join(path) or 'osw'} {where}: {text!r}")
392+
for name, sub in getattr(command, "commands", {}).items():
393+
walk(sub, [*path, name])
394+
395+
walk(typer.main.get_command(app), [])
396+
397+
assert offenders == []
398+
399+
400+
def test_a_substituted_stream_with_no_usable_errors_value_is_left_alone(monkeypatch):
401+
"""Both halves of the guard are needed, not just the ``reconfigure`` half.
402+
403+
A host application may put an object that is not a ``TextIOWrapper`` on
404+
``sys.stdout``. Reading ``.errors`` on one that lacks it raises, which
405+
would end the command. A ``.errors`` of ``None`` is no better: passing it
406+
on means ``strict``, the handler this function exists to preserve.
407+
"""
408+
409+
class Substituted:
410+
def __init__(self, errors):
411+
self.calls = []
412+
if errors is not _MISSING:
413+
self.errors = errors
414+
415+
def reconfigure(self, **kwargs):
416+
self.calls.append(kwargs)
417+
418+
without = Substituted(_MISSING)
419+
none_valued = Substituted(None)
420+
monkeypatch.setattr(sys, "stdout", without)
421+
monkeypatch.setattr(sys, "stderr", none_valued)
422+
423+
cli_main._force_utf8_output()
424+
425+
assert without.calls == []
426+
assert none_valued.calls == []
427+
428+
429+
def test_a_log_handler_holding_stderr_writes_utf8_after_the_switch(monkeypatch):
430+
"""osw logs to ``sys.stderr``, and its handler is built at import time.
431+
432+
``logging.StreamHandler`` stores the stream object it was given, so the
433+
handler osw attaches in ``enable_logging`` holds ``sys.stderr`` itself.
434+
``reconfigure`` changes that object in place rather than replacing it,
435+
which is why an already attached handler writes UTF-8 too. Replacing
436+
``sys.stderr`` with a new object would leave the handler on the old one.
437+
"""
438+
err = io.TextIOWrapper(io.BytesIO(), encoding="cp1252", errors="backslashreplace")
439+
monkeypatch.setattr(sys, "stderr", err)
440+
handler = logging.StreamHandler(sys.stderr) # as osw.enable_logging does
441+
logger = logging.getLogger("test_utf8_handler")
442+
logger.addHandler(handler)
443+
monkeypatch.setattr(
444+
sys, "stdout", io.TextIOWrapper(io.BytesIO(), encoding="cp1252")
445+
)
446+
447+
cli_main._force_utf8_output()
448+
logger.warning("Änderungen")
449+
handler.flush()
450+
451+
assert handler.stream is err
452+
assert "Änderungen" in err.buffer.getvalue().decode("utf-8")
453+
454+
455+
def test_label_the_locale_encoding_cannot_represent_is_written_not_raised(
456+
cp1252_runner, configured_env, monkeypatch
457+
):
458+
"""cp1252 has no Japanese characters, so encoding used to raise, not corrupt."""
459+
_fake_osw_labelled(monkeypatch, "文字")
460+
461+
result = cp1252_runner.invoke(app, ["--json", "entity", "get", "Item:OSW1"])
462+
463+
assert result.exit_code == 0, result.exception or result.stderr
464+
payload = json.loads(result.stdout_bytes.decode("utf-8"))
465+
assert payload["jsondata"]["label"][0]["text"] == "文字"
466+
467+
295468
# -- CLI-only path-taking file commands (osw.cli.ops) ---------------------------
296469
# These are the only operations in the codebase allowed to name a path; they
297470
# are exercised here rather than in tests/test_service_ops_files.py.

0 commit comments

Comments
 (0)