diff --git a/docs/tools/configuration.md b/docs/tools/configuration.md index 989b6c7b..fad7ca7b 100644 --- a/docs/tools/configuration.md +++ b/docs/tools/configuration.md @@ -103,7 +103,7 @@ set wins: | `OSW_DOMAIN` | `OSL_DOMAIN` | Instance to connect to. A bare host (`wiki.example.org`) or a full URL (`https://wiki.example.org/w/`); the host is taken from either, and a value no host can be read from (`https://`, `/w/index.php`) is rejected at startup | | `OSW_USERNAME` | `OSL_USERNAME` | Login user | | `OSW_PASSWORD` | `OSL_PASSWORD` | Login password | -| `OSW_CRED_FILEPATH` | `OSW_MCP_CRED_FILEPATH`, `OSL_CRED_FILEPATH` | YAML credential file, keyed by iri (falls back to `accounts.pwd.yaml` in the working directory, CLI only) | +| `OSW_CRED_FILEPATH` | `OSW_MCP_CRED_FILEPATH`, `OSL_CRED_FILEPATH` | YAML credential file, keyed by iri (falls back to `accounts.pwd.yaml` in the working directory, CLI only). A leading `~` is expanded. A relative path is accepted; the MCP server resolves it at startup against the working directory its client chose, and the source report shows the full path | | `OSW_ENV_FILE` | `OSW_MCP_ENV_FILE` | `.env` file to load | | `OSW_READ_ONLY` | `OSW_MCP_READ_ONLY` | `true` refuses every write | | `OSW_SPARQL_ENDPOINT` | | Endpoint for `sparql` queries | diff --git a/src/osw/cli/main.py b/src/osw/cli/main.py index 997683b2..1195c6fb 100644 --- a/src/osw/cli/main.py +++ b/src/osw/cli/main.py @@ -30,6 +30,7 @@ from osw.service.errors import OpError from osw.service.params import json_value from osw.service.registry import Operation, bind, iter_operations +from osw.service.streams import force_utf8 from osw.wtsite import SLOTS from .render import render @@ -40,14 +41,8 @@ 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 + The mechanism lives in :func:`osw.service.streams.force_utf8`, which the + osw-mcp server uses as well. 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 @@ -60,16 +55,7 @@ def _force_utf8_output() -> None: 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) + force_utf8(sys.stdout, sys.stderr) @app.callback() diff --git a/src/osw/mcp/server.py b/src/osw/mcp/server.py index b2b45e7e..9f863c61 100644 --- a/src/osw/mcp/server.py +++ b/src/osw/mcp/server.py @@ -22,6 +22,7 @@ from osw.service.config import Settings from osw.service.context import Context, Policy from osw.service.registry import Operation, bind, iter_operations +from osw.service.streams import force_utf8 INSTRUCTIONS = """\ This server is pinned to exactly one OpenSemanticLab (OSL) instance for its @@ -156,6 +157,18 @@ def create_server() -> MCPServer: def main() -> None: """Console-script entry point: build the server and serve over stdio.""" + # Before any write below. An MCP client starts this server with stderr on + # a pipe, so Python encodes it with the locale encoding, cp1252 on a + # German Windows system. The report holds the credential file path and the + # env file path, so a directory named "Muller" with an umlaut is enough to + # reach the client's log mangled. Reconfiguring in place also covers osw's + # own log handler, which holds this same stream object. + # + # stdout is deliberately left alone. The SDK's stdio_server re-wraps the + # binary buffer as UTF-8 itself, and claims file descriptor 1 while doing + # it, so the JSON-RPC channel does not depend on this and changing it here + # would only add a way to interfere. + force_utf8(sys.stderr) # See _build_server for why this is set here too. config.set_log_prefix("osw-mcp") report = io.StringIO() diff --git a/src/osw/service/config.py b/src/osw/service/config.py index 52397d42..c68e6649 100644 --- a/src/osw/service/config.py +++ b/src/osw/service/config.py @@ -176,6 +176,21 @@ def _validate_cred_filepath(cls, value: Optional[str]) -> Optional[str]: return value if not value.strip(): raise ValueError("must not be empty or whitespace-only") + # Same rewrite as _validate_state_dir, for the same reason: load() + # checks Path(cred_filepath).is_file() and _cred_file_iris() opens the + # path, and neither expands a leading ~, so "~/accounts.pwd.yaml" was + # reported as missing while the file was there. A relative path is + # still accepted, unlike for state_dir: the CLI resolves + # "accounts.pwd.yaml" against the working directory on purpose. For the + # MCP server, _resolve_cred_file has already made it absolute. + if value.startswith("~"): + try: + value = str(Path(value).expanduser()) + except RuntimeError as exc: + raise ValueError( + f"starts with '~' but the home directory cannot be " + f"determined ({exc})" + ) from exc return value def redacted(self) -> dict: @@ -288,6 +303,10 @@ def _verify_cred_file_has_domain(cred_filepath: str, domain: str) -> None: _cred_file_path: Optional[str] = None _cred_file_origin: str = "not searched" _cred_file_var: Optional[str] = None +# The relative value _resolve_cred_file() made absolute, or None. Only for +# load()'s "does not exist" message, which otherwise shows a directory the +# user never typed. +_cred_file_relative: Optional[str] = None # The adapter name every "[name] ..." message this module (and the rest of # osw.service) prints. "osw" is the default, covering a process that embeds @@ -328,7 +347,9 @@ def set_env_file_discovery(enabled: bool) -> None: depend on the working directory the process happens to run in, so both are gated by the same flag: the CLI's working directory is the one the user typed the command in, while the MCP server's is chosen by the MCP - client, which it does not control. + client, which it does not control. For the same reason, a relative + ``OSW_CRED_FILEPATH`` is made absolute while discovery is disabled, see + :func:`_resolve_cred_file`. Must be called before settings are first loaded, since the file is read exactly once per process; a call that would *change* the setting after @@ -406,8 +427,10 @@ def _resolve_cred_file() -> Optional[str]: 1. An explicitly configured ``OSW_CRED_FILEPATH`` (or its ``OSW_MCP_CRED_FILEPATH`` / ``OSL_CRED_FILEPATH`` aliases) always wins, whether it came from the real environment or from a ``.env`` file. - Existence is not checked here; ``load()`` already reports a missing - configured file with a specific error message. + A leading ``~`` is expanded. While implicit discovery is disabled (the + MCP server), a relative path is made absolute against the working + directory. Existence is not checked here; ``load()`` checks the + returned path and reports a missing file with a specific error message. 2. Otherwise, if implicit discovery is disabled (the MCP server; see :func:`set_env_file_discovery`), nothing is resolved: origin ``"not searched"``. @@ -435,7 +458,8 @@ def _resolve_cred_file() -> Optional[str]: username/password already configured. Safe to call more than once, like :func:`_load_env_file`, which this assumes has already run. """ - global _cred_file_path, _cred_file_origin, _cred_file_var + global _cred_file_path, _cred_file_origin, _cred_file_var, _cred_file_relative + _cred_file_relative = None path = _first_env(ENV_CRED_FILEPATH) if path: _cred_file_var = next( @@ -444,6 +468,29 @@ def _resolve_cred_file() -> Optional[str]: _cred_file_origin = ( "env file" if _cred_file_var in _env_file_supplied else "environment" ) + # Expanded here, not only in Settings._validate_cred_filepath, because + # load() calls Path(cred_filepath).is_file() on this return value long + # before it constructs Settings. Determined after _cred_file_var above, + # which matches the raw value against os.getenv. + if path.startswith("~"): + try: + path = str(Path(path).expanduser()) + except RuntimeError as exc: + raise RuntimeError( + f"{_cred_file_var} starts with '~' but the home directory " + f"cannot be determined ({exc}). Set it to a full path." + ) from exc + # With discovery disabled, the working directory is the one the MCP + # client chose, so a relative path names a file the user cannot predict. + # It is resolved once, here, so that load()'s existence check, the + # source report and the stored Settings all name the same full path, + # and no later read depends on the working directory. The CLI keeps + # the relative value: its working directory is the one the user typed + # the command in. os.path.abspath rather than Path.resolve(), which + # would also replace a symlink with its target. + if not _discover_env_file and not Path(path).is_absolute(): + _cred_file_relative = path + path = os.path.abspath(path) _cred_file_path = path return path if not _discover_env_file: @@ -618,9 +665,11 @@ def load(strict: bool = True) -> Settings: If domain is missing and no usable credential file is configured, if neither a usable credential file nor username/password are configured (only when ``strict`` is ``True``), if a configured - credential file does not exist, or if a configured credential file has - no entry matching a configured domain. This keeps the osw interactive - credential prompt from ever being reached. + credential file does not exist, if a configured credential file has + no entry matching a configured domain, or if a configured credential + file path starts with ``~`` and no home directory can be determined. + This keeps the osw interactive credential prompt from ever being + reached. """ _load_env_file() @@ -632,9 +681,17 @@ def load(strict: bool = True) -> Settings: cred_file_usable = False if cred_filepath: if not Path(cred_filepath).is_file(): + relative_hint = ( + f"{_cred_file_var} is the relative path '{_cred_file_relative}', " + "resolved against the working directory, which the MCP client " + "chooses. " + if _cred_file_relative + else "" + ) raise RuntimeError( f"Configured credential file '{cred_filepath}' does not exist. " - "Set OSW_CRED_FILEPATH (or its OSW_MCP_CRED_FILEPATH / " + + relative_hint + + "Set OSW_CRED_FILEPATH (or its OSW_MCP_CRED_FILEPATH / " "OSL_CRED_FILEPATH aliases) to a valid path, or remove it and " "configure OSW_USERNAME/OSW_PASSWORD instead." + _escape_hint(cred_filepath) @@ -744,7 +801,7 @@ def reset() -> None: """Drop cached settings and the active-instance selection (used by tests).""" global _settings, _active_iri, _active_resolved global _discover_env_file, _env_file_path, _env_file_origin, _env_file_supplied - global _cred_file_path, _cred_file_origin, _cred_file_var + global _cred_file_path, _cred_file_origin, _cred_file_var, _cred_file_relative _settings = None _active_iri = None _active_resolved = False @@ -755,6 +812,7 @@ def reset() -> None: _cred_file_path = None _cred_file_origin = "not searched" _cred_file_var = None + _cred_file_relative = None # -- active-instance state --------------------------------------------------- diff --git a/src/osw/service/streams.py b/src/osw/service/streams.py new file mode 100644 index 00000000..c27cb4ec --- /dev/null +++ b/src/osw/service/streams.py @@ -0,0 +1,39 @@ +"""Output stream setup shared by the osw CLI and the osw-mcp server. + +Neither adapter may import the other, and both write text that the locale +encoding may not be able to represent. The single helper here is what they +share; which streams to apply it to is the caller's decision, because the two +adapters differ there. See :func:`force_utf8`. +""" + +from __future__ import annotations + +from typing import TextIO + + +def force_utf8(*streams: TextIO) -> None: + """Encode each given stream 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. + + Each stream is reconfigured in place. A ``logging.StreamHandler`` built + earlier holds the stream object itself, not a name, so it writes UTF-8 + from here on as well. + """ + for stream in streams: + 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) diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 670000ce..aaf97f7f 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -11,12 +11,17 @@ import asyncio import io +import logging +import sys +from contextlib import contextmanager import pytest import yaml +import osw from osw.mcp import server from osw.service import config +from osw.service.context import Context from osw.service.registry import iter_operations _ALL_VARS = [ @@ -237,3 +242,97 @@ def test_main_prints_the_report_when_startup_fails(monkeypatch, tmp_path, capsys err = capsys.readouterr().err assert "[osw-mcp] " in err assert "failed to start" in err + + +def test_main_forces_utf8_on_stderr_and_leaves_stdout_alone(monkeypatch): + """An MCP client puts stderr on a pipe, so Python picks the locale encoding. + + The startup report carries the credential file path and the env file path + (src/osw/service/config.py), so a directory or account name outside ASCII + reaches the client's log mangled. + + stdout is left alone on purpose. The SDK's ``stdio_server`` re-wraps the + binary buffer as UTF-8 itself and claims file descriptor 1 while doing it, + so the JSON-RPC channel does not depend on this. + """ + _configure(monkeypatch) + _serve_without_blocking(monkeypatch) + out = io.TextIOWrapper(io.BytesIO(), encoding="cp1252", errors="strict") + err = io.TextIOWrapper(io.BytesIO(), encoding="cp1252", errors="backslashreplace") + monkeypatch.setattr(sys, "stdout", out) + monkeypatch.setattr(sys, "stderr", err) + + server.main() + + assert err.encoding == "utf-8" + # reconfigure() resets errors to strict unless it is passed as well, and a + # strict stderr would raise while reporting a failure. + assert err.errors == "backslashreplace" + assert out.encoding == "cp1252" + + +@contextmanager +def _osw_logging_on_the_captured_stream(): + """osw's own handler, writing to the stream pytest has in place right now. + + Two resets are needed. ``enable_logging`` resolves ``sys.stderr`` once, + when it builds the handler (src/osw/__init__.py:149), so the handler + attached when conftest imported osw still holds the stderr from before + capsys replaced it. And that handler steps aside as soon as an ancestor + logger has a handler of its own (src/osw/__init__.py:84-88), which + pytest's log capture puts on the root logger. + + A context manager rather than a fixture, because pytest attaches those + root handlers after the fixtures have run. Mirrors ``osw_logger`` and + ``plain_logging`` in tests/test_logging_setup.py. + """ + root, osw_logger = logging.getLogger(), logging.getLogger("osw") + saved_root = root.handlers[:] + saved = (osw_logger.handlers[:], osw_logger.level, osw._level_is_ours) + root.handlers = [] + try: + osw.enable_logging() + yield + finally: + root.handlers = saved_root + osw_logger.handlers, osw._level_is_ours = saved[0], saved[2] + osw_logger.setLevel(saved[1]) + + +def _no_connection(self, iri): + raise RuntimeError("offline test: no connection is made") + + +def test_a_log_record_during_a_tool_call_never_reaches_stdout( + monkeypatch, tmp_path, capsys +): + """stdout is the JSON-RPC channel, so one log line there breaks the client. + + Two mechanisms keep it clean and only one of them is osw's own code: + ``enable_logging`` defaults its handler to ``sys.stderr``, and the MCP SDK + claims file descriptor 1 for the wire. A single edit to that default would + undo the first, which is what this holds. + + The status operation is used because it logs a warning from inside + ``ctx.guard()`` when the connection check fails + (src/osw/service/ops/status.py:63). ``guard()`` rebinds ``sys.stdout`` to + ``sys.stderr`` for the call's duration, and a handler built earlier does + not follow that rebinding, so the record goes to the handler's own stream. + That is the stream under test here. + """ + _configure(monkeypatch) + monkeypatch.setenv("OSW_STATE_DIR", str(tmp_path / "state")) + # Makes the connection check fail without a network, which is what gets + # status to log while the tool call is running. + monkeypatch.setattr(Context, "osw_for", _no_connection) + config.reset() + mcp = server.create_server() + + with _osw_logging_on_the_captured_stream(): + asyncio.run(mcp.call_tool("status", {})) + + captured = capsys.readouterr() + # First, so a run that emits no record at all fails here rather than + # passing the stdout assertion without having observed anything. + assert "status connection check failed" in captured.err + assert captured.out == "" diff --git a/tests/test_service_config.py b/tests/test_service_config.py index e3cbb3b7..9fda72e4 100644 --- a/tests/test_service_config.py +++ b/tests/test_service_config.py @@ -428,6 +428,117 @@ def test_canonical_cred_filepath(monkeypatch, tmp_path): assert settings.cred_filepath == str(cred_file) +def test_cred_filepath_tilde_expanded_before_the_existence_check(monkeypatch, tmp_path): + """Settings._validate_cred_filepath runs too late to fix this on its own. + + _resolve_cred_file returns the value load() passes to Path(...).is_file(), + and Settings is constructed only afterwards. An unexpanded '~' therefore + reported a file that exists as missing. + """ + _write_cred_file( + tmp_path / "accounts.pwd.yaml", + {"wiki.example.org": {"username": "alice", "password": "secret"}}, + ) + # expanduser reads USERPROFILE on Windows and HOME on POSIX. Both are set + # so the test needs no platform marker. + monkeypatch.setenv("USERPROFILE", str(tmp_path)) + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("OSW_DOMAIN", "wiki.example.org") + monkeypatch.setenv("OSW_CRED_FILEPATH", "~/accounts.pwd.yaml") + + settings = config.load() + + assert settings.cred_filepath == str(tmp_path / "accounts.pwd.yaml") + + +def test_cred_filepath_tilde_with_no_home_names_the_variable(monkeypatch): + """Uncaught, expanduser's RuntimeError says nothing about the setting. + + See test_state_dir_reports_an_undeterminable_home for why the raise is + patched in rather than reproduced. + """ + + def _no_home(self): + raise RuntimeError("Could not determine home directory.") + + monkeypatch.setattr(Path, "expanduser", _no_home) + monkeypatch.setenv("OSW_DOMAIN", "wiki.example.org") + monkeypatch.setenv("OSW_CRED_FILEPATH", "~/accounts.pwd.yaml") + + with pytest.raises(RuntimeError) as exc: + config.load() + + assert "OSW_CRED_FILEPATH" in str(exc.value) + assert "home directory" in str(exc.value) + + +def test_cred_filepath_relative_is_stored_resolved_for_the_mcp_server( + monkeypatch, tmp_path +): + """The MCP client chooses the server's working directory, not the user. + + Implicit discovery is off, which is the MCP server's setting. A relative + path is resolved against the working directory once, at load time, and the + full path is stored, so no later read depends on the working directory. + """ + _write_cred_file( + tmp_path / "accounts.pwd.yaml", + {"wiki.example.org": {"username": "alice", "password": "secret"}}, + ) + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("OSW_DOMAIN", "wiki.example.org") + monkeypatch.setenv("OSW_CRED_FILEPATH", "accounts.pwd.yaml") + + settings = config.load() + + assert settings.cred_filepath == str(Path.cwd() / "accounts.pwd.yaml") + + +def test_cred_filepath_relative_and_missing_names_the_resolved_path( + monkeypatch, tmp_path, capsys +): + """The rejection and the source report both show where the server looked. + + Called in the order the MCP server calls them: the report first, then + load(). The server prints the report when it fails to start, so a bare + 'accounts.pwd.yaml' there would not say which directory was searched. The + full path alone would not say why that directory either: the user never + typed it, so the message names the relative value it came from. + """ + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("OSW_DOMAIN", "wiki.example.org") + monkeypatch.setenv("OSW_CRED_FILEPATH", "accounts.pwd.yaml") + resolved = str(Path.cwd() / "accounts.pwd.yaml") + + config.log_config_sources() + with pytest.raises(RuntimeError) as exc: + config.load() + + assert f"Configured credential file '{resolved}' does not exist" in str(exc.value) + assert "OSW_CRED_FILEPATH is the relative path 'accounts.pwd.yaml'" in ( + str(exc.value) + ) + assert f"credential file: {resolved} (from the OSW_CRED_FILEPATH" in ( + capsys.readouterr().err + ) + + +def test_cred_filepath_relative_stays_relative_for_the_cli(monkeypatch, tmp_path): + """The CLI's working directory is the one the user typed the command in.""" + _write_cred_file( + tmp_path / "accounts.pwd.yaml", + {"wiki.example.org": {"username": "alice", "password": "secret"}}, + ) + monkeypatch.chdir(tmp_path) + config.set_env_file_discovery(True) + monkeypatch.setenv("OSW_DOMAIN", "wiki.example.org") + monkeypatch.setenv("OSW_CRED_FILEPATH", "accounts.pwd.yaml") + + settings = config.load() + + assert settings.cred_filepath == "accounts.pwd.yaml" + + def test_canonical_read_only(monkeypatch): monkeypatch.setenv("OSW_DOMAIN", "wiki.example.org") monkeypatch.setenv("OSW_USERNAME", "alice") @@ -1128,6 +1239,43 @@ def test_state_dir_absolute_is_left_alone(tmp_path): assert settings.state_dir == str(tmp_path / "state") +def test_cred_filepath_expands_a_leading_tilde(): + """load() reports '~/accounts.pwd.yaml' as missing while the file is there. + + Path(cred_filepath).is_file() in load() and open(cred_filepath) in + _cred_file_iris() both read the value literally, so a leading '~' names a + directory called '~'. The failure is loud but points at the wrong cause. + """ + settings = Settings(domain="wiki.example.org", cred_filepath="~/accounts.pwd.yaml") + assert settings.cred_filepath == str(Path.home() / "accounts.pwd.yaml") + + +def test_cred_filepath_relative_is_accepted(): + """Unlike state_dir, a relative credential file stays valid. + + The CLI resolves 'accounts.pwd.yaml' against the working directory the + user typed the command in, which is what they mean. + """ + settings = Settings(domain="wiki.example.org", cred_filepath="accounts.pwd.yaml") + assert settings.cred_filepath == "accounts.pwd.yaml" + + +def test_cred_filepath_reports_an_undeterminable_home(monkeypatch): + """Same failure mode as state_dir: the error has to name the setting. + + See test_state_dir_reports_an_undeterminable_home for why the raise is + patched in rather than reproduced. + """ + + def _no_home(self): + raise RuntimeError("Could not determine home directory.") + + monkeypatch.setattr(Path, "expanduser", _no_home) + with pytest.raises(ValidationError) as exc: + Settings(domain="wiki.example.org", cred_filepath="~/accounts.pwd.yaml") + assert "home directory" in str(exc.value) + + def test_settings_is_frozen(): settings = Settings(domain="wiki.example.org") with pytest.raises(ValidationError):