From db3c3035df76c2724f5b5b3f60d2ac4ea913e692 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Thu, 3 Sep 2026 15:07:27 -0500 Subject: [PATCH 1/3] fix(server): anticipated tool errors keep their text under mcp >= 2.1 (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mcp 2.1.x hands the client a bare "Error executing tool " for any exception that is not a deliberate ToolError. omind's tools let their domain failures escape as NoteError / NoteConflictError / ValueError, so every anticipated message — a missing note, an unsafe name, a bad graph argument, and the stale-version conflict that tells an agent to re-read before writing — vanished, and five test_server assertions went red on every PR. Re-raise those failures as ToolError at the tool boundary (every registration routes through one wrapper); a real crash stays masked as the SDK intends. Move uv.lock to mcp 2.1.1 so a local run sees what CI sees. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011T5zwA8x2zqEghR2aq86hc --- CHANGELOG.md | 10 ++++++- src/omind/server.py | 65 ++++++++++++++++++++++++++++++++++++++++---- tests/test_server.py | 31 +++++++++++++++++++++ uv.lock | 12 ++++---- 4 files changed, 106 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1feadfc..4949bf8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,15 @@ 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 ` 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. ## [9.0.0] - 2026-09-07 diff --git a/src/omind/server.py b/src/omind/server.py index c7cb4de..e89e2e6 100644 --- a/src/omind/server.py +++ b/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 `` +#: (#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: diff --git a/tests/test_server.py b/tests/test_server.py index 343478f..7982df5 100644 --- a/tests/test_server.py +++ b/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 ``. 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() diff --git a/uv.lock b/uv.lock index c54ead4..d690788 100644 --- a/uv.lock +++ b/uv.lock @@ -1852,7 +1852,7 @@ wheels = [ [[package]] name = "mcp" -version = "2.0.0" +version = "2.1.1" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "anyio" }, @@ -1870,22 +1870,22 @@ dependencies = [ { name = "typing-inspection" }, { name = "uvicorn", marker = "sys_platform != 'emscripten'" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/74/33/32d4dff2c95bb5d897c3ef4c83649a08996b17b58f0a326d2495d4c81179/mcp-2.0.0.tar.gz", hash = "sha256:0f440e735c13ece8bb19bc62cf0b86f4313448432fbb77d35e14034f4e050728", size = 1662284, upload-time = "2026-07-28T13:45:32.346Z" } +sdist = { url = "https://files.pythonhosted.org/packages/d4/6e/21fb8e5d579dbe21d96ea4d5034200d46d8bdf2261053b5bd041f3c2f612/mcp-2.1.1.tar.gz", hash = "sha256:50b7ba1ebbe117008ea7bdd288234043e69c20b403d6851d19661e6d431a75ef", size = 3984589, upload-time = "2026-08-25T16:14:02.376Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/67/72/7d7897418912c1d12e87556630dfb7bf0eac71160e9bef8b447960804ee3/mcp-2.0.0-py3-none-any.whl", hash = "sha256:1cb4c75d2d2c7b8c1d756355e5d82a39f2822cc7f13e22a2051d7ca3592349d6", size = 349980, upload-time = "2026-07-28T13:45:28.853Z" }, + { url = "https://files.pythonhosted.org/packages/50/af/8644cc5fa26a59afd2df2e98eeb19e72926887fa4b7441aba4ff661140db/mcp-2.1.1-py3-none-any.whl", hash = "sha256:1c6c31c5d6471c58db76af3af8af67f46d11d01f0a59077d0a308cbdb3d3e915", size = 357912, upload-time = "2026-08-25T16:13:59.024Z" }, ] [[package]] name = "mcp-types" -version = "2.0.0" +version = "2.1.1" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "pydantic" }, { name = "typing-extensions" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/bb/56/9b8e1c152f61f6c6b07c4b5896c88c7d0ae90bac6ee6306f852fcc5c1eb0/mcp_types-2.0.0.tar.gz", hash = "sha256:d7d939b9285c9961ae8866ba75ef85da34d12bafe276efbf4eb6a131786d8379", size = 66632, upload-time = "2026-07-28T13:45:33.804Z" } +sdist = { url = "https://files.pythonhosted.org/packages/6a/dd/1c4417dc0b722c23a1669032d5f044e41170fe5d4773b488a50fcce98c32/mcp_types-2.1.1.tar.gz", hash = "sha256:77dcbe48fba73cca71a673f2646a5f037a017b7a0a07ac89cec1113028890eda", size = 66674, upload-time = "2026-08-25T16:14:03.861Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/f5/4c/c78d78c3d52b0ac594ad7cc8ef5972adfe070e3597a8a4c6ce0cd39196ea/mcp_types-2.0.0-py3-none-any.whl", hash = "sha256:6b2de797ca2797f568b79529e1b25948e34de511bcc0bd82fef1039a6d1b8eb0", size = 69649, upload-time = "2026-07-28T13:45:30.713Z" }, + { url = "https://files.pythonhosted.org/packages/71/d0/242e63c510f4a17381f55b1549a3f94f5687a0595984febd2b6f87a687a0/mcp_types-2.1.1-py3-none-any.whl", hash = "sha256:26f9f7f03f2a5730717a5b98e2ab7eb640ac352d05a00cdc725c311864778295", size = 69656, upload-time = "2026-08-25T16:14:00.667Z" }, ] [[package]] From 8d0132fc89d99420f116087d050d7941091714a3 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark (CryptoJones)" Date: Mon, 7 Sep 2026 20:08:08 -0500 Subject: [PATCH 2/3] fix: the Windows failures #294's redness was hiding (#306) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the mcp 2.1 masking fixed, the test matrix went green everywhere except both Windows jobs — which had been failing on their own the whole time, invisible because #294 reddened all eight. rollup_journals took an exclusive lock on each daily and then re-opened it by path to tally it. POSIX flock is advisory so the second open succeeds; msvcrt.locking is MANDATORY, so Windows answered PermissionError: [Errno 13] on a file this process itself had locked. That arrived with the 2026-08-27 locking hardening — correct on POSIX, untested on Windows. It now reads through the descriptor it already holds. That is not just a Windows workaround: it closes a correctness gap everywhere, because the tally now counts exactly the bytes the lock protects instead of whatever a second open happens to see. The fd takes filelock.BINARY so the CRT's text mode can't rewrite bytes underneath it. test_resolve_finds_a_cli_outside_path moved home with HOME alone, but expanduser() reads USERPROFILE on Windows, so home never moved and the fake CLI was never found. A test bug; conftest._isolate_home already sets both vars and documents exactly this. 1,024 tests, ruff and mypy green, now under mcp 2.1.1 locally so a local run finally sees what CI sees. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwxtwuoRxP75PdR6NAmMS3 --- BACKLOG.md | 8 ++++++++ CHANGELOG.md | 12 ++++++++++++ src/omind/filelock.py | 4 ++++ src/omind/journal.py | 26 ++++++++++++++++++++++++-- tests/test_ai_usage.py | 4 ++++ 5 files changed, 52 insertions(+), 2 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index fc1bf76..b4390b9 100644 --- a/BACKLOG.md +++ b/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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 4949bf8..a506c53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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. ## [9.0.0] - 2026-09-07 diff --git a/src/omind/filelock.py b/src/omind/filelock.py index 34d6b9a..2db9eb4 100644 --- a/src/omind/filelock.py +++ b/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]: diff --git a/src/omind/journal.py b/src/omind/journal.py index 2cb9ecb..d7aa7ea 100644 --- a/src/omind/journal.py +++ b/src/omind/journal.py @@ -229,6 +229,26 @@ def render_rollup(week: str, days: list[str], stats: JournalStats) -> str: return render_fields(fields) +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,12 +307,14 @@ def rollup_journals( # 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)) diff --git a/tests/test_ai_usage.py b/tests/test_ai_usage.py index 98c2a46..5f81997 100644 --- a/tests/test_ai_usage.py +++ b/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) From 0e6b1b30a0e995f1edb4f1dc7efdf7129eeef52c Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark (CryptoJones)" Date: Mon, 7 Sep 2026 20:13:45 -0500 Subject: [PATCH 3/3] fix(journal): drop the locks before archiving on Windows (#306) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading through the held descriptor fixed the first Windows failure and exposed the next one: Windows refuses to rename or unlink a file anyone still holds open, so the archive step died with [WinError 32] on the very descriptors the rollup was deliberately holding across tally -> write -> rename. Releasing them before the archive is not a hole in what that hold was protecting. On Windows an appending hook's own open handle is itself what blocks the rename, so a daily cannot be moved out from under a live append. POSIX has no such property — an open fd doesn't obstruct a rename there, only the lock closes the window — so POSIX keeps holding across the rename exactly as before. Both release paths now go through one _release() helper that clears the list, so the finally can't double-close a descriptor number the OS has already handed back out. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwxtwuoRxP75PdR6NAmMS3 --- CHANGELOG.md | 4 +++- src/omind/journal.py | 28 +++++++++++++++++++++++----- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a506c53..1c64a34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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. + `_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 diff --git a/src/omind/journal.py b/src/omind/journal.py index d7aa7ea..a50c80e 100644 --- a/src/omind/journal.py +++ b/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,17 @@ def render_rollup(week: str, days: list[str], stats: JournalStats) -> str: 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. @@ -318,6 +330,16 @@ def rollup_journals( 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: @@ -329,11 +351,7 @@ def rollup_journals( 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,