diff --git a/src/osw/cli/main.py b/src/osw/cli/main.py index bf9b86db..997683b2 100644 --- a/src/osw/cli/main.py +++ b/src/osw/cli/main.py @@ -11,6 +11,7 @@ from __future__ import annotations import inspect +import sys from typing import Any, Optional, get_type_hints import click @@ -36,6 +37,41 @@ app = typer.Typer(no_args_is_help=True, add_completion=False) +def _force_utf8_output() -> None: + """Encode stdout and stderr as UTF-8, whatever the locale asks for. + + Python encodes a redirected stream with the locale encoding, which on a + German Windows system is cp1252. A non-ASCII label then reaches the + consumer as bytes no JSON parser can read, and a character cp1252 has no + code point for -- Japanese, Greek, Cyrillic -- raises UnicodeEncodeError + and ends the command. A Windows console stream is UTF-8 already, so on + Windows only redirected output changes. Elsewhere a terminal uses the + locale encoding, so this overrides a deliberate non-UTF-8 LANG or + PYTHONIOENCODING too. stderr is covered as well as stdout, because + ``Context.guard`` sends captured stdout to stderr under ``--json``. + + Called from the app callback, so it covers every command. Click prints + help and rejects an unknown root-level name before any callback runs, so + those paths keep the locale encoding. They carry no wiki content: every + help string in this package is ASCII (held by a test), and rich + substitutes its box-drawing characters once the stream is not UTF-8. What + stays exposed is the name the user typed, echoed back in a usage error -- + an unknown command name or an unknown root option name. A name typed + after the command is fine, because click resolves the command, runs this + callback, and only then parses the command's own arguments. + """ + for stream in (sys.stdout, sys.stderr): + reconfigure = getattr(stream, "reconfigure", None) + errors = getattr(stream, "errors", None) + # A stream a test harness or host application substituted may have + # neither, and then decides its own encoding. Both are required: + # errors= must be passed, because reconfigure() silently resets the + # handler to strict otherwise, which would let stderr raise while + # reporting a failure. Passing errors=None does exactly that too. + if reconfigure is not None and errors is not None: + reconfigure(encoding="utf-8", errors=errors) + + @app.callback() def _callback( ctx: typer.Context, @@ -66,6 +102,8 @@ def _callback( # the prefix is always correct for any message printed on the way out, # including one printed while handling set_env_file_discovery's error. config.set_log_prefix("osw") + # Before any output, including the configuration banner. + _force_utf8_output() # The CLI's working directory is the one the user typed the command in, so # searching it upward for a .env is what they mean. The MCP server leaves # this off: its working directory is chosen by the MCP client. diff --git a/tests/test_cli.py b/tests/test_cli.py index 542cf9e2..77fad5bb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -11,6 +11,7 @@ import json import logging import re +import sys from unittest.mock import MagicMock import click @@ -292,6 +293,178 @@ def test_render_dict_shows_key_value_lines(): assert "exists" in rendered +# -- output encoding ------------------------------------------------------------ +# Redirected stdout on Windows is opened with the locale encoding, not UTF-8, so +# a German label used to reach the consumer as cp1252 bytes. CliRunner's charset +# gives the captured stream that same encoding, which reproduces the platform +# behaviour everywhere, so these run on Linux CI too. +_MISSING = object() # "do not set this attribute at all", distinct from None + + +@pytest.fixture +def cp1252_runner(): + return CliRunner(mix_stderr=False, charset="cp1252") + + +def _fake_osw_labelled(monkeypatch, label: str): + """Patch in an entity whose label slot holds ``label``.""" + fake_osw, page = _fake_osw_with_page() + page.get_slot_content.return_value = {"label": [{"text": label}]} + page.get_url.return_value = "https://wiki.example.org/wiki/Item:OSW1" + monkeypatch.setattr("osw.service.context.OswExpress", lambda **kwargs: fake_osw) + + +def test_json_output_is_utf8_when_stdout_uses_the_locale_encoding( + cp1252_runner, configured_env, monkeypatch +): + _fake_osw_labelled(monkeypatch, "Änderungen") + + result = cp1252_runner.invoke(app, ["--json", "entity", "get", "Item:OSW1"]) + + assert result.exit_code == 0, result.stderr + payload = json.loads(result.stdout_bytes.decode("utf-8")) + assert payload["jsondata"]["label"][0]["text"] == "Änderungen" + + +def test_human_output_is_utf8_when_stdout_uses_the_locale_encoding( + cp1252_runner, configured_env, monkeypatch +): + _fake_osw_labelled(monkeypatch, "Änderungen") + + result = cp1252_runner.invoke(app, ["entity", "get", "Item:OSW1"]) + + assert result.exit_code == 0, result.stderr + assert "Änderungen" in result.stdout_bytes.decode("utf-8") + + +def test_error_message_is_utf8_when_stderr_uses_the_locale_encoding( + cp1252_runner, configured_env, monkeypatch +): + """An error names the page it failed on, so stderr carries labels too.""" + fake_osw, _page = _fake_osw_with_page(exists=False) + fake_osw.load_entity.return_value.entities = [] + monkeypatch.setattr("osw.service.context.OswExpress", lambda **kwargs: fake_osw) + + result = cp1252_runner.invoke(app, ["--json", "entity", "export", "Item:Änderung"]) + + assert result.exit_code == 2 + assert "Item:Änderung" in result.stderr_bytes.decode("utf-8") + + +def test_forcing_utf8_keeps_the_error_handler_each_stream_was_given(monkeypatch): + """``reconfigure`` resets ``errors`` to strict unless it is passed as well. + + Python gives stderr ``backslashreplace`` precisely so that reporting a + failure cannot itself raise. Switching the encoding must not drop that. + """ + err = io.TextIOWrapper(io.BytesIO(), encoding="cp1252", errors="backslashreplace") + monkeypatch.setattr(sys, "stderr", err) + monkeypatch.setattr( + sys, "stdout", io.TextIOWrapper(io.BytesIO(), encoding="cp1252") + ) + + cli_main._force_utf8_output() + + assert err.encoding == "utf-8" + err.write("\udc80") # a lone surrogate, which "strict" refuses to encode + err.flush() + assert err.buffer.getvalue() == rb"\udc80" + + +def test_every_help_string_is_ascii(): + """Guards the one gap ``_force_utf8_output`` cannot close. + + Click prints help and rejects an unknown name before any callback runs, + so those paths keep the locale encoding. That is only harmless while no + help string contains a character the locale encoding may lack. Adding a + German option description would make it a real defect, and this test is + what reports it. + """ + offenders = [] + + def walk(command, path): + texts = {"help": command.help, "short_help": command.short_help} + for param in command.params: + texts[f"--{param.name}"] = getattr(param, "help", None) + for where, text in texts.items(): + if text and not text.isascii(): + offenders.append(f"{' '.join(path) or 'osw'} {where}: {text!r}") + for name, sub in getattr(command, "commands", {}).items(): + walk(sub, [*path, name]) + + walk(typer.main.get_command(app), []) + + assert offenders == [] + + +def test_a_substituted_stream_with_no_usable_errors_value_is_left_alone(monkeypatch): + """Both halves of the guard are needed, not just the ``reconfigure`` half. + + A host application may put an object that is not a ``TextIOWrapper`` on + ``sys.stdout``. Reading ``.errors`` on one that lacks it raises, which + would end the command. A ``.errors`` of ``None`` is no better: passing it + on means ``strict``, the handler this function exists to preserve. + """ + + class Substituted: + def __init__(self, errors): + self.calls = [] + if errors is not _MISSING: + self.errors = errors + + def reconfigure(self, **kwargs): + self.calls.append(kwargs) + + without = Substituted(_MISSING) + none_valued = Substituted(None) + monkeypatch.setattr(sys, "stdout", without) + monkeypatch.setattr(sys, "stderr", none_valued) + + cli_main._force_utf8_output() + + assert without.calls == [] + assert none_valued.calls == [] + + +def test_a_log_handler_holding_stderr_writes_utf8_after_the_switch(monkeypatch): + """osw logs to ``sys.stderr``, and its handler is built at import time. + + ``logging.StreamHandler`` stores the stream object it was given, so the + handler osw attaches in ``enable_logging`` holds ``sys.stderr`` itself. + ``reconfigure`` changes that object in place rather than replacing it, + which is why an already attached handler writes UTF-8 too. Replacing + ``sys.stderr`` with a new object would leave the handler on the old one. + """ + err = io.TextIOWrapper(io.BytesIO(), encoding="cp1252", errors="backslashreplace") + monkeypatch.setattr(sys, "stderr", err) + handler = logging.StreamHandler(sys.stderr) # as osw.enable_logging does + logger = logging.getLogger("test_utf8_handler") + logger.addHandler(handler) + monkeypatch.setattr( + sys, "stdout", io.TextIOWrapper(io.BytesIO(), encoding="cp1252") + ) + + cli_main._force_utf8_output() + logger.warning("Änderungen") + handler.flush() + + assert handler.stream is err + assert "Änderungen" in err.buffer.getvalue().decode("utf-8") + + +def test_label_the_locale_encoding_cannot_represent_is_written_not_raised( + cp1252_runner, configured_env, monkeypatch +): + """cp1252 has no Japanese characters, so encoding used to raise, not corrupt.""" + _fake_osw_labelled(monkeypatch, "文字") + + result = cp1252_runner.invoke(app, ["--json", "entity", "get", "Item:OSW1"]) + + assert result.exit_code == 0, result.exception or result.stderr + payload = json.loads(result.stdout_bytes.decode("utf-8")) + assert payload["jsondata"]["label"][0]["text"] == "文字" + + # -- CLI-only path-taking file commands (osw.cli.ops) --------------------------- # These are the only operations in the codebase allowed to name a path; they # are exercised here rather than in tests/test_service_ops_files.py.