Read MCP result fields the way both majors spell them - #32
Merged
Conversation
…LOG count mcp 2.0.0 (released after this branch was opened) renamed the result fields _results.py already reads through result_is_error() for; the tests still read .isError directly, so PR #27's CI has been red on 2.x since 2026-08-01 while the tests never caught up. 1.x registers tool handlers with decorators, 2.x uses explicit add_request_handler - the one real incompatibility, not a rewrite. mcp's bound widens to >=1.0.0,<3. Verified on both majors in isolated venvs on this machine (not just reasoned about): ruff check + format, mypy --ignore-missing-imports, pytest tests/unit tests/integration (602 passed, 4 skipped on each), production_validation.py (11/11 on each) - all pass identically on mcp==1.29.0 and mcp==2.0.0. Also: CHANGELOG's 0.3.1 entry said "Four tools are read-only; the rest are destructive." Counted TOOL_ANNOTATIONS directly - four read-only, nine destructive, two neither (they add or open something but never overwrite or remove it). test_docs_truth.py does not cover this sentence, so nothing would have caught the wrong one. Branched off tool-annotations rather than pushing to it directly: .github/workflows/auto-merge.yml arms on any PR targeting main from an author with write access, and #27 already has every required check red plus a standing bot approval, so making CI green there is the same action as merging to main. This PR targets tool-annotations, not main, so it proposes the fix without arming that. If you want it in, merging this into tool-annotations is what makes PR #27 itself go green - and that merge to main is still a separate decision after that, not a side effect of this one.
Jules ReviewCOVERAGE: ef10a55 10 files Summary VERDICT: approve This review never edits code or force-blocks a merge. No blocking issues were found, so this PR was auto-approved. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 13, 2026
* Say what each tool does to the machine before a client runs it Rigout advertised fifteen tools identically. Reading a CPU count and running an arbitrary command as root arrived at a client looking the same, so anything wanting to warn before the second had nothing to go on and had to guess from the name. MCP has carried the answer since well before the version Rigout pins: Tool.title and ToolAnnotations, with readOnlyHint, destructiveHint, idempotentHint and openWorldHint. Rigout declared none of them. This adds all four to every tool, plus a human-readable title. The classification is one table rather than fifteen scattered arguments, because a missing argument in a definition is invisible while a missing row is not, and because the judgements are arguable and should be somewhere they can be argued with. The conservative reading is the honest one: anything that runs a caller's command is destructive and not idempotent, since what it does is decided by the caller and cannot be known here. Four tools are read-only - hardware information, server activity, system metrics, listing sessions. Eight are destructive. close_terminal_session is destructive and idempotent at once, which is not a contradiction: it can kill running work, and closing an already-closed session changes nothing further. Tests hold the contract rather than the values. Every advertised tool must be classified, the table must contain nothing that is not advertised - so a rename touching one side fails rather than silently leaving a tool unclassified - read-only and destructive can never both be true, and nothing that runs a caller's command may claim idempotence. Found while checking what the current mcp actually offers, which also turned up that this repository's development environment had mcp 1.15 while users install 1.29. Fourteen minor versions of difference, including Tool.execution, which exists in what users run and not in what the tests ran against. The environment is now on 1.29 and the suite passes there. * Record which MCP capabilities Rigout adopts, and why tasks are declined Chasing what the current mcp actually offers turned up the Tasks API - Tool.execution with taskSupport, the Task type, tasks/get and its siblings, and mcp.server.experimental.task_support. It addresses Rigout's oldest limitation directly: a command that outlives its timeout fails, and builds, installs and downloads all can. Declined, and the reason is worth recording so it is not rediscovered by somebody reading mcp.types and finding it apparently available. The API deprecates itself: tasks (SEP-1686) were removed from the MCP specification and the interface is removed in mcp 2.0. Checked rather than taken on the warning's word - mcp.server.experimental.task_support raises ModuleNotFoundError on 2.0.0, while Tool.execution, ServerCapabilities.tasks and Task all still exist there. The types outliving the implementation is exactly what would make this look safe to adopt. Building on it would ship a feature on an interface deprecated in the version Rigout pins and absent from the version it must move to next. That is the same trade as 0.2.0's unbounded mcp>=1.0.0: it works until someone else's release day. When tasks return as an extension it is worth revisiting, and the reason is now written down. * Map the mcp 2.x migration so the discovery is not redone Checked against 2.0.0 rather than assumed. Every tool definition ports as written: 2.x renamed Tool's fields to snake_case but kept the camelCase spellings as aliases, and stdio, streamable_http_manager, models, types, Server and Server.run all survive. The whole of the migration is the registration. The list_tools and call_tool decorators are gone, replaced by add_request_handler(method, params_type, handler) with tools/list taking PaginatedRequestParams and tools/call taking CallToolRequestParams; the handlers then return results directly instead of the bare list and content the decorators wrapped, which also removes the wrinkle where an error result must be raised as RuntimeError for the SDK to rebuild it. Two decisions are recorded rather than made, because both are judgement rather than discovery: whether to support both majors behind one hasattr branch - the incompatibility is narrow enough that the cap could widen to <3 without forcing anyone onto a five-day-old major - and whether it is time at all, given that nothing Rigout needs is exclusive to 2.x now that tasks are gone from both, and the caps mean nobody is broken while it waits. * Run on both mcp majors, and cut 0.3.1 The bound widens to mcp>=1.0.0,<3. Rigout now works on 1.x and 2.x, verified by running against both rather than reasoned about. The whole incompatibility is registration. 1.x uses the list_tools and call_tool decorators; 2.x removed them for add_request_handler(method, params_type, handler), with tools/list taking PaginatedRequestParams and tools/call taking CallToolRequestParams, and handlers returning results directly instead of the bare list and content the decorators wrapped. Every tool definition constructs unchanged, because 2.x renamed Tool's fields while keeping the camelCase spellings as construction aliases. That last point has a trap in it, and running the code found it where reading had not. The aliases cover construction, not attribute access. Building CallToolResult(isError=True) works on both; reading result.isError raises on 2.x, where the field is is_error. The same is true of every annotation hint: readOnlyHint constructs, .readOnlyHint does not read. One read in the product and every read in the annotation tests were affected, and all now go through helpers that answer on either major - result_is_error, and a dump by alias which gives the protocol's own names on both. Supporting both is deliberate rather than a step on the way to dropping 1.x. The fork is four lines wide, so it costs little, and it means nobody is pushed onto a major that is five days old while nobody is stranded on the older one either. Which is in use is decided by what is installed, so there is nothing to configure wrong. Verified on mcp 2.0.0: fifteen tools listed with annotations intact, an unknown tool answering is_error true with its name in the message, and a real tool answering is_error false with its output. Verified on 1.29.0 by the suite, 602 tests. 0.3.1 rather than 0.4.0: nothing here is breaking, and the policy in VERSIONING.md puts a compatible change at a patch below 1.0.0. * Read MCP result fields the way both majors spell them, fix the CHANGELOG count (#32) mcp 2.0.0 (released after this branch was opened) renamed the result fields _results.py already reads through result_is_error() for; the tests still read .isError directly, so PR #27's CI has been red on 2.x since 2026-08-01 while the tests never caught up. 1.x registers tool handlers with decorators, 2.x uses explicit add_request_handler - the one real incompatibility, not a rewrite. mcp's bound widens to >=1.0.0,<3. Verified on both majors in isolated venvs on this machine (not just reasoned about): ruff check + format, mypy --ignore-missing-imports, pytest tests/unit tests/integration (602 passed, 4 skipped on each), production_validation.py (11/11 on each) - all pass identically on mcp==1.29.0 and mcp==2.0.0. Also: CHANGELOG's 0.3.1 entry said "Four tools are read-only; the rest are destructive." Counted TOOL_ANNOTATIONS directly - four read-only, nine destructive, two neither (they add or open something but never overwrite or remove it). test_docs_truth.py does not cover this sentence, so nothing would have caught the wrong one. Branched off tool-annotations rather than pushing to it directly: .github/workflows/auto-merge.yml arms on any PR targeting main from an author with write access, and #27 already has every required check red plus a standing bot approval, so making CI green there is the same action as merging to main. This PR targets tool-annotations, not main, so it proposes the fix without arming that. If you want it in, merging this into tool-annotations is what makes PR #27 itself go green - and that merge to main is still a separate decision after that, not a side effect of this one.
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
1 participant
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.
What this is
PR #27 has been red since 2026-08-01 — mcp 2.0.0 was released after that branch was opened, and its result fields don't have the same names 1.x used.
_results.pyalready reads them throughresult_is_error(), a helper written for exactly this; the tests still read.isErrordirectly, so the whole suite falls over on 2.x in the same handful of places. 1.x also registers tool handlers with decorators where 2.x uses explicitadd_request_handler— the one other real incompatibility, not a rewrite.mcp's bound widens to>=1.0.0,<3.Verified on both majors, in isolated venvs, on this machine — not reasoned about
Two separate
.venvs, one pinned to each major, so neither installation could mask the other's result.Also: a CHANGELOG count that was wrong
The 0.3.1 entry said "Four tools are read-only; the rest are destructive." Counted
TOOL_ANNOTATIONSdirectly rather than trust the sentence: four read-only, nine destructive, two neither —connect_hardwareandcreate_terminal_sessionchange something but only add or open it, never overwrite or remove.4 + 9 + 2 = 15, matching the tool count; the old sentence implied4 + 11.test_docs_truth.pydoesn't cover this particular sentence, so nothing would have caught it either way. Fixed in the same commit.Why this targets
tool-annotations, notmain.github/workflows/auto-merge.ymlarms on any PR targetingmainfrom an author with write access, once every required check passes. #27 already carries a standing bot approval and has nothing but red checks between it and merging — so making CI green there is the same action as merging it tomain, and PyPI publishing is a separate, tag-triggered step so no release follows automatically, but a merge tomainyou haven't seen is still not something this should decide for you.This PR targets
tool-annotationsinstead. Merging it here is what turns #27 itself green; whether #27 then merges tomainstays a separate decision, made after you've seen what's in it, not a side effect of taking this fix.