fix(server): anticipated tool errors keep their text under mcp >= 2.1 (#294) - #295
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…#294) mcp 2.1.x hands the client a bare "Error executing tool <name>" 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011T5zwA8x2zqEghR2aq86hc
CryptoJones
force-pushed
the
fix/294-anticipated-tool-errors
branch
from
September 8, 2026 00:59
301539a to
db3c303
Compare
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BwxtwuoRxP75PdR6NAmMS3
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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) |
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BwxtwuoRxP75PdR6NAmMS3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #294. Unblocks CI for every open PR, including #289.
What was wrong
pyproject.tomlallowsmcp>=2.0.0,<3.0and CI installs withpip install -e .[dev], so it resolves mcp 2.1.1, while the lock (and every local venv) sat on 2.0.0. In 2.1.x the SDK keeps the text of a deliberateToolErrorbut treats any other exception as a crash and hands the client onlyError executing tool <name>. omind's tools let their domain failures escape asNoteError/NoteConflictError/ValueError, so every anticipated message vanished: missing note, unsafe name, bad graph argument, and the stale-version conflict whose text is what tells an agent to re-read before writing. Fivetest_server.pyassertions went red on every OS and Python version.The fix
server.py: every@mcp.toolregistration now routes through one_anticipatedwrapper that re-raisesNoteError,NoteConflictError, andValueErroras a deliberateToolError(cause chained). A genuine crash (anOSError, a bug) stays masked and logged with its traceback, exactly as the SDK intends. This answers the question the issue raised: the detail is still reachable by callers, on purpose.uv.lock: mcp 2.0.0 → 2.1.1 so a local run sees what CI sees. Nothing else in the lock moved.Verified
tests/test_server.py: 26 passed under both mcp 2.0.0 (old lock) and 2.1.1 (overlay + new lock).scripts/test.sh): 1019 passed, 1 skipped.ruff check .clean,mypy srcclean (strict).Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/
🤖 Generated with Claude Code
https://claude.ai/code/session_011T5zwA8x2zqEghR2aq86hc