Skip to content

Commit 0d0b2b6

Browse files
authored
fix(cli,mcp): UTF-8 stderr, a stdout guard, and OSW_CRED_FILEPATH paths (#196)
* fix(config): expand a leading ~ in OSW_CRED_FILEPATH - expand in _resolve_cred_file as well: load() checks the path before Settings is built, so the field validator alone runs too late - a relative path stays accepted, unlike state_dir - name the variable when no home directory can be determined - closes #194 * fix(mcp): encode the osw-mcp startup report as UTF-8 - move the mechanism to osw.service.streams.force_utf8, shared by both adapters without osw.mcp importing osw.cli - reconfigure stderr only: the SDK re-wraps stdout as UTF-8 itself - add a test that a log record during a tool call leaves stdout empty - closes #193, closes #192 * fix(mcp): resolve a relative OSW_CRED_FILEPATH at startup - the MCP client chooses the working directory, so store the full path - load() checks existence on the resolved path and rejects a missing one - the rejection names the relative value it was resolved from - the CLI keeps a relative value - closes #197
1 parent 289e2b0 commit 0d0b2b6

7 files changed

Lines changed: 371 additions & 28 deletions

File tree

‎docs/tools/configuration.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ set wins:
103103
| `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 |
104104
| `OSW_USERNAME` | `OSL_USERNAME` | Login user |
105105
| `OSW_PASSWORD` | `OSL_PASSWORD` | Login password |
106-
| `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) |
106+
| `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 |
107107
| `OSW_ENV_FILE` | `OSW_MCP_ENV_FILE` | `.env` file to load |
108108
| `OSW_READ_ONLY` | `OSW_MCP_READ_ONLY` | `true` refuses every write |
109109
| `OSW_SPARQL_ENDPOINT` | | Endpoint for `sparql` queries |

‎src/osw/cli/main.py‎

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
from osw.service.errors import OpError
3131
from osw.service.params import json_value
3232
from osw.service.registry import Operation, bind, iter_operations
33+
from osw.service.streams import force_utf8
3334
from osw.wtsite import SLOTS
3435

3536
from .render import render
@@ -40,14 +41,8 @@
4041
def _force_utf8_output() -> None:
4142
"""Encode stdout and stderr as UTF-8, whatever the locale asks for.
4243
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
44+
The mechanism lives in :func:`osw.service.streams.force_utf8`, which the
45+
osw-mcp server uses as well. stderr is covered as well as stdout, because
5146
``Context.guard`` sends captured stdout to stderr under ``--json``.
5247
5348
Called from the app callback, so it covers every command. Click prints
@@ -60,16 +55,7 @@ def _force_utf8_output() -> None:
6055
after the command is fine, because click resolves the command, runs this
6156
callback, and only then parses the command's own arguments.
6257
"""
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)
58+
force_utf8(sys.stdout, sys.stderr)
7359

7460

7561
@app.callback()

‎src/osw/mcp/server.py‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
from osw.service.config import Settings
2323
from osw.service.context import Context, Policy
2424
from osw.service.registry import Operation, bind, iter_operations
25+
from osw.service.streams import force_utf8
2526

2627
INSTRUCTIONS = """\
2728
This server is pinned to exactly one OpenSemanticLab (OSL) instance for its
@@ -156,6 +157,18 @@ def create_server() -> MCPServer:
156157

157158
def main() -> None:
158159
"""Console-script entry point: build the server and serve over stdio."""
160+
# Before any write below. An MCP client starts this server with stderr on
161+
# a pipe, so Python encodes it with the locale encoding, cp1252 on a
162+
# German Windows system. The report holds the credential file path and the
163+
# env file path, so a directory named "Muller" with an umlaut is enough to
164+
# reach the client's log mangled. Reconfiguring in place also covers osw's
165+
# own log handler, which holds this same stream object.
166+
#
167+
# stdout is deliberately left alone. The SDK's stdio_server re-wraps the
168+
# binary buffer as UTF-8 itself, and claims file descriptor 1 while doing
169+
# it, so the JSON-RPC channel does not depend on this and changing it here
170+
# would only add a way to interfere.
171+
force_utf8(sys.stderr)
159172
# See _build_server for why this is set here too.
160173
config.set_log_prefix("osw-mcp")
161174
report = io.StringIO()

‎src/osw/service/config.py‎

Lines changed: 67 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,21 @@ def _validate_cred_filepath(cls, value: Optional[str]) -> Optional[str]:
176176
return value
177177
if not value.strip():
178178
raise ValueError("must not be empty or whitespace-only")
179+
# Same rewrite as _validate_state_dir, for the same reason: load()
180+
# checks Path(cred_filepath).is_file() and _cred_file_iris() opens the
181+
# path, and neither expands a leading ~, so "~/accounts.pwd.yaml" was
182+
# reported as missing while the file was there. A relative path is
183+
# still accepted, unlike for state_dir: the CLI resolves
184+
# "accounts.pwd.yaml" against the working directory on purpose. For the
185+
# MCP server, _resolve_cred_file has already made it absolute.
186+
if value.startswith("~"):
187+
try:
188+
value = str(Path(value).expanduser())
189+
except RuntimeError as exc:
190+
raise ValueError(
191+
f"starts with '~' but the home directory cannot be "
192+
f"determined ({exc})"
193+
) from exc
179194
return value
180195

181196
def redacted(self) -> dict:
@@ -288,6 +303,10 @@ def _verify_cred_file_has_domain(cred_filepath: str, domain: str) -> None:
288303
_cred_file_path: Optional[str] = None
289304
_cred_file_origin: str = "not searched"
290305
_cred_file_var: Optional[str] = None
306+
# The relative value _resolve_cred_file() made absolute, or None. Only for
307+
# load()'s "does not exist" message, which otherwise shows a directory the
308+
# user never typed.
309+
_cred_file_relative: Optional[str] = None
291310

292311
# The adapter name every "[name] ..." message this module (and the rest of
293312
# osw.service) prints. "osw" is the default, covering a process that embeds
@@ -328,7 +347,9 @@ def set_env_file_discovery(enabled: bool) -> None:
328347
depend on the working directory the process happens to run in, so both
329348
are gated by the same flag: the CLI's working directory is the one the
330349
user typed the command in, while the MCP server's is chosen by the MCP
331-
client, which it does not control.
350+
client, which it does not control. For the same reason, a relative
351+
``OSW_CRED_FILEPATH`` is made absolute while discovery is disabled, see
352+
:func:`_resolve_cred_file`.
332353
333354
Must be called before settings are first loaded, since the file is read
334355
exactly once per process; a call that would *change* the setting after
@@ -406,8 +427,10 @@ def _resolve_cred_file() -> Optional[str]:
406427
1. An explicitly configured ``OSW_CRED_FILEPATH`` (or its
407428
``OSW_MCP_CRED_FILEPATH`` / ``OSL_CRED_FILEPATH`` aliases) always wins,
408429
whether it came from the real environment or from a ``.env`` file.
409-
Existence is not checked here; ``load()`` already reports a missing
410-
configured file with a specific error message.
430+
A leading ``~`` is expanded. While implicit discovery is disabled (the
431+
MCP server), a relative path is made absolute against the working
432+
directory. Existence is not checked here; ``load()`` checks the
433+
returned path and reports a missing file with a specific error message.
411434
2. Otherwise, if implicit discovery is disabled (the MCP server; see
412435
:func:`set_env_file_discovery`), nothing is resolved: origin
413436
``"not searched"``.
@@ -435,7 +458,8 @@ def _resolve_cred_file() -> Optional[str]:
435458
username/password already configured. Safe to call more than once, like
436459
:func:`_load_env_file`, which this assumes has already run.
437460
"""
438-
global _cred_file_path, _cred_file_origin, _cred_file_var
461+
global _cred_file_path, _cred_file_origin, _cred_file_var, _cred_file_relative
462+
_cred_file_relative = None
439463
path = _first_env(ENV_CRED_FILEPATH)
440464
if path:
441465
_cred_file_var = next(
@@ -444,6 +468,29 @@ def _resolve_cred_file() -> Optional[str]:
444468
_cred_file_origin = (
445469
"env file" if _cred_file_var in _env_file_supplied else "environment"
446470
)
471+
# Expanded here, not only in Settings._validate_cred_filepath, because
472+
# load() calls Path(cred_filepath).is_file() on this return value long
473+
# before it constructs Settings. Determined after _cred_file_var above,
474+
# which matches the raw value against os.getenv.
475+
if path.startswith("~"):
476+
try:
477+
path = str(Path(path).expanduser())
478+
except RuntimeError as exc:
479+
raise RuntimeError(
480+
f"{_cred_file_var} starts with '~' but the home directory "
481+
f"cannot be determined ({exc}). Set it to a full path."
482+
) from exc
483+
# With discovery disabled, the working directory is the one the MCP
484+
# client chose, so a relative path names a file the user cannot predict.
485+
# It is resolved once, here, so that load()'s existence check, the
486+
# source report and the stored Settings all name the same full path,
487+
# and no later read depends on the working directory. The CLI keeps
488+
# the relative value: its working directory is the one the user typed
489+
# the command in. os.path.abspath rather than Path.resolve(), which
490+
# would also replace a symlink with its target.
491+
if not _discover_env_file and not Path(path).is_absolute():
492+
_cred_file_relative = path
493+
path = os.path.abspath(path)
447494
_cred_file_path = path
448495
return path
449496
if not _discover_env_file:
@@ -618,9 +665,11 @@ def load(strict: bool = True) -> Settings:
618665
If domain is missing and no usable credential file is configured, if
619666
neither a usable credential file nor username/password are
620667
configured (only when ``strict`` is ``True``), if a configured
621-
credential file does not exist, or if a configured credential file has
622-
no entry matching a configured domain. This keeps the osw interactive
623-
credential prompt from ever being reached.
668+
credential file does not exist, if a configured credential file has
669+
no entry matching a configured domain, or if a configured credential
670+
file path starts with ``~`` and no home directory can be determined.
671+
This keeps the osw interactive credential prompt from ever being
672+
reached.
624673
"""
625674
_load_env_file()
626675

@@ -632,9 +681,17 @@ def load(strict: bool = True) -> Settings:
632681
cred_file_usable = False
633682
if cred_filepath:
634683
if not Path(cred_filepath).is_file():
684+
relative_hint = (
685+
f"{_cred_file_var} is the relative path '{_cred_file_relative}', "
686+
"resolved against the working directory, which the MCP client "
687+
"chooses. "
688+
if _cred_file_relative
689+
else ""
690+
)
635691
raise RuntimeError(
636692
f"Configured credential file '{cred_filepath}' does not exist. "
637-
"Set OSW_CRED_FILEPATH (or its OSW_MCP_CRED_FILEPATH / "
693+
+ relative_hint
694+
+ "Set OSW_CRED_FILEPATH (or its OSW_MCP_CRED_FILEPATH / "
638695
"OSL_CRED_FILEPATH aliases) to a valid path, or remove it and "
639696
"configure OSW_USERNAME/OSW_PASSWORD instead."
640697
+ _escape_hint(cred_filepath)
@@ -744,7 +801,7 @@ def reset() -> None:
744801
"""Drop cached settings and the active-instance selection (used by tests)."""
745802
global _settings, _active_iri, _active_resolved
746803
global _discover_env_file, _env_file_path, _env_file_origin, _env_file_supplied
747-
global _cred_file_path, _cred_file_origin, _cred_file_var
804+
global _cred_file_path, _cred_file_origin, _cred_file_var, _cred_file_relative
748805
_settings = None
749806
_active_iri = None
750807
_active_resolved = False
@@ -755,6 +812,7 @@ def reset() -> None:
755812
_cred_file_path = None
756813
_cred_file_origin = "not searched"
757814
_cred_file_var = None
815+
_cred_file_relative = None
758816

759817

760818
# -- active-instance state ---------------------------------------------------

‎src/osw/service/streams.py‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
"""Output stream setup shared by the osw CLI and the osw-mcp server.
2+
3+
Neither adapter may import the other, and both write text that the locale
4+
encoding may not be able to represent. The single helper here is what they
5+
share; which streams to apply it to is the caller's decision, because the two
6+
adapters differ there. See :func:`force_utf8`.
7+
"""
8+
9+
from __future__ import annotations
10+
11+
from typing import TextIO
12+
13+
14+
def force_utf8(*streams: TextIO) -> None:
15+
"""Encode each given stream as UTF-8, whatever the locale asks for.
16+
17+
Python encodes a redirected stream with the locale encoding, which on a
18+
German Windows system is cp1252. A non-ASCII label then reaches the
19+
consumer as bytes no JSON parser can read, and a character cp1252 has no
20+
code point for -- Japanese, Greek, Cyrillic -- raises UnicodeEncodeError
21+
and ends the command. A Windows console stream is UTF-8 already, so on
22+
Windows only redirected output changes. Elsewhere a terminal uses the
23+
locale encoding, so this overrides a deliberate non-UTF-8 LANG or
24+
PYTHONIOENCODING too.
25+
26+
Each stream is reconfigured in place. A ``logging.StreamHandler`` built
27+
earlier holds the stream object itself, not a name, so it writes UTF-8
28+
from here on as well.
29+
"""
30+
for stream in streams:
31+
reconfigure = getattr(stream, "reconfigure", None)
32+
errors = getattr(stream, "errors", None)
33+
# A stream a test harness or host application substituted may have
34+
# neither, and then decides its own encoding. Both are required:
35+
# errors= must be passed, because reconfigure() silently resets the
36+
# handler to strict otherwise, which would let stderr raise while
37+
# reporting a failure. Passing errors=None does exactly that too.
38+
if reconfigure is not None and errors is not None:
39+
reconfigure(encoding="utf-8", errors=errors)

‎tests/test_mcp_server.py‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,17 @@
1111

1212
import asyncio
1313
import io
14+
import logging
15+
import sys
16+
from contextlib import contextmanager
1417

1518
import pytest
1619
import yaml
1720

21+
import osw
1822
from osw.mcp import server
1923
from osw.service import config
24+
from osw.service.context import Context
2025
from osw.service.registry import iter_operations
2126

2227
_ALL_VARS = [
@@ -237,3 +242,97 @@ def test_main_prints_the_report_when_startup_fails(monkeypatch, tmp_path, capsys
237242
err = capsys.readouterr().err
238243
assert "[osw-mcp] " in err
239244
assert "failed to start" in err
245+
246+
247+
def test_main_forces_utf8_on_stderr_and_leaves_stdout_alone(monkeypatch):
248+
"""An MCP client puts stderr on a pipe, so Python picks the locale encoding.
249+
250+
The startup report carries the credential file path and the env file path
251+
(src/osw/service/config.py), so a directory or account name outside ASCII
252+
reaches the client's log mangled.
253+
254+
stdout is left alone on purpose. The SDK's ``stdio_server`` re-wraps the
255+
binary buffer as UTF-8 itself and claims file descriptor 1 while doing it,
256+
so the JSON-RPC channel does not depend on this.
257+
"""
258+
_configure(monkeypatch)
259+
_serve_without_blocking(monkeypatch)
260+
out = io.TextIOWrapper(io.BytesIO(), encoding="cp1252", errors="strict")
261+
err = io.TextIOWrapper(io.BytesIO(), encoding="cp1252", errors="backslashreplace")
262+
monkeypatch.setattr(sys, "stdout", out)
263+
monkeypatch.setattr(sys, "stderr", err)
264+
265+
server.main()
266+
267+
assert err.encoding == "utf-8"
268+
# reconfigure() resets errors to strict unless it is passed as well, and a
269+
# strict stderr would raise while reporting a failure.
270+
assert err.errors == "backslashreplace"
271+
assert out.encoding == "cp1252"
272+
273+
274+
@contextmanager
275+
def _osw_logging_on_the_captured_stream():
276+
"""osw's own handler, writing to the stream pytest has in place right now.
277+
278+
Two resets are needed. ``enable_logging`` resolves ``sys.stderr`` once,
279+
when it builds the handler (src/osw/__init__.py:149), so the handler
280+
attached when conftest imported osw still holds the stderr from before
281+
capsys replaced it. And that handler steps aside as soon as an ancestor
282+
logger has a handler of its own (src/osw/__init__.py:84-88), which
283+
pytest's log capture puts on the root logger.
284+
285+
A context manager rather than a fixture, because pytest attaches those
286+
root handlers after the fixtures have run. Mirrors ``osw_logger`` and
287+
``plain_logging`` in tests/test_logging_setup.py.
288+
"""
289+
root, osw_logger = logging.getLogger(), logging.getLogger("osw")
290+
saved_root = root.handlers[:]
291+
saved = (osw_logger.handlers[:], osw_logger.level, osw._level_is_ours)
292+
root.handlers = []
293+
try:
294+
osw.enable_logging()
295+
yield
296+
finally:
297+
root.handlers = saved_root
298+
osw_logger.handlers, osw._level_is_ours = saved[0], saved[2]
299+
osw_logger.setLevel(saved[1])
300+
301+
302+
def _no_connection(self, iri):
303+
raise RuntimeError("offline test: no connection is made")
304+
305+
306+
def test_a_log_record_during_a_tool_call_never_reaches_stdout(
307+
monkeypatch, tmp_path, capsys
308+
):
309+
"""stdout is the JSON-RPC channel, so one log line there breaks the client.
310+
311+
Two mechanisms keep it clean and only one of them is osw's own code:
312+
``enable_logging`` defaults its handler to ``sys.stderr``, and the MCP SDK
313+
claims file descriptor 1 for the wire. A single edit to that default would
314+
undo the first, which is what this holds.
315+
316+
The status operation is used because it logs a warning from inside
317+
``ctx.guard()`` when the connection check fails
318+
(src/osw/service/ops/status.py:63). ``guard()`` rebinds ``sys.stdout`` to
319+
``sys.stderr`` for the call's duration, and a handler built earlier does
320+
not follow that rebinding, so the record goes to the handler's own stream.
321+
That is the stream under test here.
322+
"""
323+
_configure(monkeypatch)
324+
monkeypatch.setenv("OSW_STATE_DIR", str(tmp_path / "state"))
325+
# Makes the connection check fail without a network, which is what gets
326+
# status to log while the tool call is running.
327+
monkeypatch.setattr(Context, "osw_for", _no_connection)
328+
config.reset()
329+
mcp = server.create_server()
330+
331+
with _osw_logging_on_the_captured_stream():
332+
asyncio.run(mcp.call_tool("status", {}))
333+
334+
captured = capsys.readouterr()
335+
# First, so a run that emits no record at all fails here rather than
336+
# passing the stdout assertion without having observed anything.
337+
assert "status connection check failed" in captured.err
338+
assert captured.out == ""

0 commit comments

Comments
 (0)