fix(server): anticipated tool errors keep their text under mcp >= 2.1 (#294)

BACKLOG.md

@@ -212,6 +212,14 @@ Each issue below is written to be executable by any agent without further contex
 
 ### Harness + repo hygiene (2026-09-07)
 
+- [x] **`ToolError` text masked by mcp >= 2.1, reddening CI on every PR**
+  ([#294](https://github.com/CryptoJones/omind/issues/294)) — _fix (server)_ —
+  Anticipated domain failures re-raised as `ToolError` so their message survives.
+- [x] **Windows CI broken underneath #294's redness**
+  ([#306](https://github.com/CryptoJones/omind/issues/306)) — _fix (journal/test)_ —
+  Journal rollup re-opened a file it held a mandatory Windows lock on; and a test
+  moved `HOME` without `USERPROFILE`.
+
 - [x] **Poolside's `pool` CLI wasn't connected to omind at all**
   ([#302](https://github.com/CryptoJones/omind/issues/302)) — _feat (agents)_ —
   `pool mcp list` reported "No MCP servers configured", so the CmdrData/Laguna

CHANGELOG.md

@@ -7,7 +7,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-_Nothing yet._
+### Fixed
+- **Tool error text survives mcp >= 2.1 (#294).** mcp 2.1.x hands the client a
+  bare `Error executing tool <name>` for any exception that is not a deliberate
+  `ToolError`, which hid every anticipated failure — a missing note, an unsafe
+  name, a bad graph argument, and the stale-version conflict whose message is
+  what tells an agent to re-read before writing. The server now re-raises those
+  domain failures (`NoteError`, `NoteConflictError`, `ValueError`) as `ToolError`
+  at the tool boundary; a real crash stays masked as the SDK intends. `uv.lock`
+  moves to mcp 2.1.1 so a local run sees what CI sees.
+- **Windows CI was broken, and #294 was hiding it**
+  ([#306](https://github.com/CryptoJones/omind/issues/306)). With the mcp
+  masking fixed, the matrix went green everywhere except both Windows jobs,
+  which had been failing on their own all along. Two causes: `rollup_journals`
+  locked each daily and then re-opened it by path to tally it — fine under
+  POSIX `flock`, which is advisory, but `msvcrt.locking` is **mandatory**, so
+  Windows answered `PermissionError` on the second open. It now reads through
+  the descriptor it already holds, which also makes the tally count exactly the
+  bytes the lock protects on every platform. And
+  `test_resolve_finds_a_cli_outside_path` moved home with `HOME` alone, while
+  `expanduser()` reads `USERPROFILE` on Windows — a test bug that the
+  `_isolate_home` fixture already documents the fix for. The archive step also had to drop its locks first on Windows,
+  which refuses to rename a file anyone still holds open ([WinError 32]); POSIX
+  keeps them across the rename, where an open fd doesn't obstruct it.
 
 ## [9.0.0] - 2026-09-07
 

src/omind/filelock.py

@@ -55,6 +55,10 @@ def unlock_fd(fd: int) -> None:
 _BINARY = getattr(os, "O_BINARY", 0)
 _NOFOLLOW = getattr(os, "O_NOFOLLOW", 0)
 
+#: Public alias: callers that open their own fd to hold a lock need the same
+#: no-text-mode flag this module uses, and shouldn't re-derive it.
+BINARY = _BINARY
+
 
 @contextlib.contextmanager
 def append_locked(path: Path, *, mode: int = 0o600) -> Iterator[int]:

src/omind/journal.py

@@ -23,6 +23,7 @@
 import contextlib
 import os
 import re
+import sys
 from collections import Counter
 from dataclasses import dataclass, field
 from datetime import date, datetime, timedelta
@@ -229,6 +230,37 @@
     return render_fields(fields)
 
 
+def _release(fds: list[int]) -> None:
+    """Unlock and close every fd, then empty the list so a later pass (and the
+    ``finally``) can't double-close a descriptor number the OS has reused."""
+    for fd in fds:
+        with contextlib.suppress(OSError):
+            filelock.unlock_fd(fd)
+        with contextlib.suppress(OSError):
+            os.close(fd)
+    fds.clear()
+
+
+def _read_held(path: Path, fd: int | None) -> str:
+    """Read a daily, THROUGH the descriptor when we already hold its lock.
+
+    Windows byte-range locks (``msvcrt.locking``) are **mandatory**: re-opening
+    a file this process has locked fails with ``PermissionError``. POSIX
+    ``flock`` is advisory, so the old ``path.read_text()`` worked there and the
+    whole Windows matrix broke silently — hidden behind the mcp 2.1 redness of
+    #294 until that was fixed. Reading the held fd also closes a correctness
+    gap on every platform: we now tally exactly the bytes the lock protects,
+    rather than whatever a second open happens to see.
+    """
+    if fd is None:  # archived dailies aren't locked — nothing is appending to them
+        return path.read_text(encoding="utf-8", errors="replace")
+    os.lseek(fd, 0, os.SEEK_SET)
+    chunks: list[bytes] = []
+    while block := os.read(fd, 1 << 16):
+        chunks.append(block)
+    return b"".join(chunks).decode("utf-8", errors="replace")
+
+
 def rollup_journals(
     omi_dir: Path | str,
     *,
@@ -287,15 +319,27 @@
             # wait across weeks.
             locked_fds: list[int] = []
             try:
+                held: dict[Path, int] = {}
                 for _, path in dated_paths:
-                    fd = os.open(path, os.O_RDWR | os.O_CREAT, 0o600)
+                    fd = os.open(path, os.O_RDWR | os.O_CREAT | filelock.BINARY, 0o600)
                     filelock.lock_fd(fd)
                     locked_fds.append(fd)
+                    held[path] = fd
                 for _, path in [*archived_dated, *dated_paths]:
-                    _tally(path.read_text(encoding="utf-8", errors="replace"), stats)
+                    _tally(_read_held(path, held.get(path)), stats)
                 days = sorted({day.isoformat() for day, _ in [*archived_dated, *dated_paths]})
                 filename = rollup_name(wk)
                 _atomic_write(directory / filename, render_rollup(wk, days, stats))
+                # Windows refuses to rename or unlink a file that anyone still
+                # holds open, so the locks must go before the archive step —
+                # [WinError 32] otherwise. Dropping them here is not the race
+                # the comment above guards against: on Windows an appending
+                # hook's own open handle is itself what blocks the rename, so
+                # the file cannot be moved out from under a live append. POSIX
+                # keeps the locks across the rename, where an open fd does not
+                # obstruct it and only the lock closes the window.
+                if sys.platform == "win32":
+                    _release(locked_fds)
                 archived: list[str] = []
                 deleted: list[str] = []
                 for _, path in dated_paths:
@@ -307,11 +351,7 @@
                         path.replace(archive_dir / path.name)
                         archived.append(path.name)
             finally:
-                for fd in locked_fds:
-                    with contextlib.suppress(OSError):
-                        filelock.unlock_fd(fd)
-                    with contextlib.suppress(OSError):
-                        os.close(fd)
+                _release(locked_fds)
             results.append(
                 WeekRollup(
                     week=wk,

src/omind/server.py

@@ -15,24 +15,36 @@
 from __future__ import annotations
 
 import contextlib
+import functools
 import logging
 import os
 import sys
-from collections.abc import AsyncIterator
+from collections.abc import AsyncIterator, Callable
 from contextlib import asynccontextmanager
 from pathlib import Path
-from typing import Any
+from typing import Any, TypeVar
 
 import anyio
 import mcp.types as mcp_types
 from anyio.streams.memory import MemoryObjectReceiveStream, MemoryObjectSendStream
 from mcp.server.mcpserver import MCPServer
+from mcp.server.mcpserver.exceptions import ToolError
 from mcp.shared.message import SessionMessage
 
 from omind import graph, searchindex
 from omind.help_system import render_help
 from omind.recall import DEFAULT_RECALL_CHARS, compact_recall
-from omind.store import ActionItem, NoteFields, OmiStore, _clean_agent, parse_note
+from omind.store import (
+    ActionItem,
+    NoteConflictError,
+    NoteError,
+    NoteFields,
+    OmiStore,
+    _clean_agent,
+    parse_note,
+)
+
+_T = TypeVar("_T")
 
 SERVER_NAME = "omi"
 
@@ -187,6 +199,36 @@ def _parse_action_items(items: list[str]) -> list[ActionItem]:
     return parsed
 
 
+#: Failures a tool ANTICIPATES — a missing note, an unsafe name, a stale
+#: version token, a bad graph argument. Their text is the whole point: the
+#: version-conflict message is what tells an agent to re-read before writing.
+#: mcp >= 2.1 keeps a deliberate ``ToolError``'s text but treats any other
+#: exception as a crash and hands the client only ``Error executing tool <name>``
+#: (#294), so these are re-raised as ``ToolError`` at the tool boundary. A real
+#: crash (OSError, a bug) stays masked, as the SDK intends.
+_ANTICIPATED_ERRORS: tuple[type[BaseException], ...] = (
+    NoteError,
+    NoteConflictError,
+    ValueError,
+)
+
+
+def _anticipated(fn: Callable[..., _T]) -> Callable[..., _T]:
+    """Re-raise a tool's anticipated domain failures as a deliberate ``ToolError``
+    so the message reaches the caller under every mcp 2.x."""
+
+    @functools.wraps(fn)
+    def wrapper(*args: Any, **kwargs: Any) -> _T:
+        try:
+            return fn(*args, **kwargs)
+        except ToolError:
+            raise
+        except _ANTICIPATED_ERRORS as exc:
+            raise ToolError(str(exc)) from exc
+
+    return wrapper
+
+
 def build_server(omi_dir: Path | str, node_id: str | None = None) -> MCPServer:
     """Build the node MCP server over one OMI folder.
 
@@ -198,7 +240,20 @@ def build_server(omi_dir: Path | str, node_id: str | None = None) -> MCPServer:
     # the mesh daemon, not just this server's tools.
     store = OmiStore(omi_dir, node_id=node_id)
 
-    mcp = MCPServer(SERVER_NAME, instructions=_INSTRUCTIONS)
+    server = MCPServer(SERVER_NAME, instructions=_INSTRUCTIONS)
+
+    class _Registrar:
+        """``mcp.tool(...)`` with every tool body wrapped by :func:`_anticipated`."""
+
+        def tool(self, *args: Any, **kwargs: Any) -> Callable[[Callable[..., _T]], Any]:
+            register = server.tool(*args, **kwargs)
+
+            def decorate(fn: Callable[..., _T]) -> Any:
+                return register(_anticipated(fn))
+
+            return decorate
+
+    mcp = _Registrar()
 
     # The five graph tools each rebuilt the whole [[wikilink]] graph from disk
     # (a full-vault read+parse) on every call. Cache it, invalidated by a cheap
@@ -600,7 +655,7 @@ def graph_tool(
     ) -> dict[str, object]:
         return _graph_query(op, source, target, limit, offset)
 
-    return mcp
+    return server
 
 
 def run_node(omi_dir: Path, node_id: str | None = None) -> int:

tests/test_ai_usage.py

@@ -247,7 +247,11 @@ def test_resolve_finds_a_cli_outside_path(tmp_path, monkeypatch):
     exe.chmod(0o755)
 
     monkeypatch.setattr(ai_usage.shutil, "which", lambda _n: None)  # not on PATH
+    # BOTH vars: expanduser() reads USERPROFILE on Windows and HOME on POSIX, so
+    # setting only HOME left Windows resolving the conftest home and finding
+    # nothing (see the _isolate_home fixture, which sets both for this reason).
     monkeypatch.setenv("HOME", str(fake_home))
+    monkeypatch.setenv("USERPROFILE", str(fake_home))
     monkeypatch.delenv(ai_usage.MODEL_CLI_ENV, raising=False)
     monkeypatch.delenv(ai_usage.MODEL_CMD_ENV, raising=False)
 

tests/test_server.py

@@ -531,3 +531,34 @@ def test_recall_note_warns_about_a_conflicting_memory(server: MCPServer) -> None
     plain = call(server, "recall-note", {"name": "Older.md"})
     assert "conflicts_with" not in plain and "confidence" not in plain
     assert "warning" not in plain
+
+
+def test_anticipated_domain_errors_surface_as_tool_errors_with_their_text() -> None:
+    """#294: mcp >= 2.1 masks any non-``ToolError`` exception as a bare
+    ``Error executing tool <name>``. The tool boundary re-raises the failures a
+    tool anticipates as a deliberate ``ToolError`` so the text — the version
+    conflict's "re-read before writing" in particular — still reaches the caller."""
+    from omind.server import _anticipated
+    from omind.store import NoteConflictError, NoteNotFoundError
+
+    @_anticipated
+    def conflict() -> None:
+        raise NoteConflictError("note changed on disk; re-read before writing")
+
+    @_anticipated
+    def missing() -> None:
+        raise NoteNotFoundError("note not found: 'x.md'")
+
+    @_anticipated
+    def crash() -> None:
+        raise OSError("disk on fire")
+
+    with pytest.raises(ToolError, match="changed on disk") as info:
+        conflict()
+    assert isinstance(info.value.__cause__, NoteConflictError)
+    with pytest.raises(ToolError, match="not found"):
+        missing()
+    # A genuine crash is NOT anticipated: it stays an OSError for the SDK to
+    # mask and log with its traceback, exactly as the SDK intends.
+    with pytest.raises(OSError, match="disk on fire"):
+        crash()