Say what each tool does to the machine before a client runs it - #27
Open
melbinjp wants to merge 4 commits into
Open
Say what each tool does to the machine before a client runs it#27melbinjp wants to merge 4 commits into
melbinjp wants to merge 4 commits into
Conversation
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.
Jules ReviewCOVERAGE: 17a0963 6 files SummaryThe PR adds tool annotations indicating whether each tool is read-only, destructive, idempotent, or communicates with the outside world, following the MCP Findings[NIT]
VerdictVERDICT: approve This review never edits code or force-blocks a merge. No blocking issues were found, so this PR was auto-approved. |
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.
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.
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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
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 but the name.
MCP has carried the answer since well before the version Rigout pins:
Tool.titleandToolAnnotationswithreadOnlyHint,destructiveHint,idempotentHintandopenWorldHint. Rigout declared none of them.What this adds
All four hints and a human-readable title on every tool, as one table rather than fifteen scattered arguments — a missing argument in a definition is invisible, a missing row is not, and these judgements are arguable so they should sit somewhere they can be argued with.
get_hardware_info,get_server_activity,system_monitoring,list_terminal_sessionsexecute_command,execute_in_terminal,install_software,file_operations,docker_operations,bulk_file_transfer,environment_setup,manage_tunnels,close_terminal_sessionThe conservative reading is the honest one: anything that runs a caller's command is destructive and not idempotent, because what it does is decided by the caller and cannot be known here.
close_terminal_sessionis destructive and idempotent, which is not a contradiction — it can kill running work, and closing an already-closed session changes nothing further.Tests hold the contract, not the values
602 tests pass; ruff, format and mypy clean. Verified on the wire:
execute_commandserialises withreadOnlyHint: false, destructiveHint: true.One thing found on the way
This repository's development environment had mcp 1.15 while users install 1.29 — fourteen minor versions apart, 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.