From 15bdac5be8b951d9b49307c1b2d802e73dd4ebdf Mon Sep 17 00:00:00 2001 From: Justin Date: Thu, 21 May 2026 07:41:11 +0800 Subject: [PATCH] feat(mcp): advertise tool annotations on tools/list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Adds MCP `ToolAnnotations` (spec 2025-03-26+) to every tool advertised by `openhuman-core mcp`. Clients use `readOnlyHint` / `destructiveHint` / `idempotentHint` / `openWorldHint` to surface accurate safety affordances — e.g. Claude Desktop's "this tool can take destructive actions" confirmation gate. ## Problem `tools/list` today returns the curated tool surface but says nothing about each tool's behavior class. The MCP spec defines `ToolAnnotations` precisely for this — without them: - Claude Desktop / Cursor can't render confirmation gates for destructive actions. - Humans inspecting the tool list (e.g. via MCP Inspector) can't see at a glance which tools are safe to retry. - `agent.run_subagent` — the one Act-policy surface on the MCP server today — looks identical to `memory.search` to clients, even though it can call further tools (including potentially destructive ones) through any sub-agent it spawns. ## Solution - Added `annotations: Value` to `McpToolSpec` (always present, serialized into each tool's `annotations` field in the `tools/list` response). - All read-only tools share a `read_only_local_annotations()` helper (`readOnlyHint: true`, `openWorldHint: false`). - `agent.run_subagent` gets explicit annotations: `readOnlyHint: false / destructiveHint: true / idempotentHint: false / openWorldHint: true`. Sub-agent execution can call further tools, isn't a no-op on repeat, and reaches into the broader OpenHuman environment. - Updated `mcp_server/mod.rs` docstring to no longer claim the surface is purely read-only — it's curated, with `agent.run_subagent` as the one Act-policy surface. - Per spec, `destructiveHint` / `idempotentHint` are only meaningful when `readOnlyHint == false`, so the read-only helper omits them. ## Submission Checklist - [x] Tests added — `list_tools_emits_annotations_for_every_tool` asserts every entry serializes a non-null `annotations` object; a second test pins the read-only / destructive split to each tool's actual behavior. `cargo test --lib mcp_server::` runs 38/38 locally. - [x] **Diff coverage ≥ 80%** — new code is mostly literal annotations + a 6-line helper; both the "every tool has annotations" and "read-only vs destructive" axes are covered. - [x] Coverage matrix updated — `N/A: extends row 11.1.4 (MCP stdio server) rather than adding a new feature.` - [x] All affected feature IDs listed under `## Related` - [x] No new external network dependencies introduced - [x] Manual smoke checklist — `N/A: extends existing MCP surface, not a release-cut path.` - [x] Linked issue closed via `Closes #NNN` — `N/A: capability extension, no issue tracking this.` ## Impact - **Runtime**: `openhuman-core mcp` only. No HTTP RPC, web, or mobile impact. - **Compatibility**: Backward compatible — older MCP clients that don't read `annotations` ignore the field; newer clients pick up the safety affordances. - **Performance**: Negligible — one extra static `Value` per tool, serialized on each `tools/list` call (low frequency). - **Security**: No policy change. `agent.run_subagent` still goes through `enforce_act_policy`; the annotation is an advertised hint, not an enforcement point. ## Related - Feature IDs: `11.1.4` (MCP stdio server) - Spec: https://modelcontextprotocol.io/specification/2025-03-26/server/tools/#tool-annotations - Builds on: #1760, #1790, #1974 ## Summary by CodeRabbit * **New Features** * Tools now include detailed security annotations (readOnly, destructive, idempotent, openWorld hints) so MCP clients can better understand safety and behavior. * Most tools are advertised as read-only; the subagent tool is explicitly advertised as act-capable/destructive, and one search tool is advertised as read-only but open-world. * **Tests** * Added tests ensuring each tool includes annotations and that read-only vs. destructive/open-world hints are correctly serialized. [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2268?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) Co-authored-by: justin Co-authored-by: Steven Enamakel --- src/openhuman/mcp_server/mod.rs | 8 +- src/openhuman/mcp_server/tools.rs | 146 ++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/src/openhuman/mcp_server/mod.rs b/src/openhuman/mcp_server/mod.rs index 14b67a0f6..7cebeee6d 100644 --- a/src/openhuman/mcp_server/mod.rs +++ b/src/openhuman/mcp_server/mod.rs @@ -1,7 +1,13 @@ -//! Stdio MCP server for exposing a curated, read-only OpenHuman tool surface. +//! Stdio MCP server for exposing a curated OpenHuman tool surface. //! //! The server is opt-in via `openhuman-core mcp` and writes only JSON-RPC //! protocol messages to stdout. Diagnostics go through stderr logging. +//! +//! Most tools (memory tree reads, core/agent introspection) are read-only and +//! gated through `SecurityPolicy` with `ToolOperation::Read`. The one +//! exception is `agent.run_subagent`, which runs through `ToolOperation::Act` +//! and is advertised to clients via MCP tool annotations +//! (`readOnlyHint: false`, `destructiveHint: true`). mod protocol; mod stdio; diff --git a/src/openhuman/mcp_server/tools.rs b/src/openhuman/mcp_server/tools.rs index 52feb6d4e..558047043 100644 --- a/src/openhuman/mcp_server/tools.rs +++ b/src/openhuman/mcp_server/tools.rs @@ -34,6 +34,13 @@ pub struct McpToolSpec { pub description: &'static str, pub rpc_method: Option<&'static str>, pub input_schema: Value, + /// MCP `ToolAnnotations` per the 2025-03-26+ spec — `readOnlyHint`, + /// `destructiveHint`, `idempotentHint`, `openWorldHint`. Hints, not + /// guarantees; clients use them to surface accurate safety affordances + /// (e.g. Claude Desktop's "this tool can take destructive actions" + /// confirmation gate). Per spec, destructive/idempotent are meaningful + /// only when `readOnlyHint == false`, so read-only tools omit them. + pub annotations: Value, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -87,6 +94,7 @@ fn base_tool_specs() -> Vec { description: "List the live core agent tool catalog that OpenHuman exposes to its orchestrator session.", rpc_method: None, input_schema: no_args_schema(), + annotations: read_only_local_annotations(), }, McpToolSpec { name: "core.tool_instructions", @@ -94,6 +102,7 @@ fn base_tool_specs() -> Vec { description: "Emit the markdown tool-use instructions block that OpenHuman injects into prompt-guided agents.", rpc_method: None, input_schema: no_args_schema(), + annotations: read_only_local_annotations(), }, McpToolSpec { name: "agent.list_subagents", @@ -101,6 +110,7 @@ fn base_tool_specs() -> Vec { description: "List registered sub-agent definitions that the core can dispatch for specialized work.", rpc_method: None, input_schema: no_args_schema(), + annotations: read_only_local_annotations(), }, McpToolSpec { name: "agent.run_subagent", @@ -122,6 +132,17 @@ fn base_tool_specs() -> Vec { "required": ["agent_id", "prompt"], "additionalProperties": false }), + // Sub-agent execution is the one Act-policy surface on the MCP + // server today (see `enforce_act_policy` dispatch in `call_tool`). + // Sub-agents can call further tools, so destructive/openWorld are + // both true; running the same agent twice is not a no-op so + // idempotent is false. + annotations: json!({ + "readOnlyHint": false, + "destructiveHint": true, + "idempotentHint": false, + "openWorldHint": true + }), }, McpToolSpec { name: "memory.search", @@ -129,6 +150,7 @@ fn base_tool_specs() -> Vec { description: "Keyword-search OpenHuman's local memory tree and return matching chunks ordered by recency.", rpc_method: Some("openhuman.memory_tree_search"), input_schema: query_schema("Substring to match against stored memory chunks."), + annotations: read_only_local_annotations(), }, McpToolSpec { name: "memory.recall", @@ -136,6 +158,7 @@ fn base_tool_specs() -> Vec { description: "Semantically recall local memory-tree chunks relevant to a natural-language query.", rpc_method: Some("openhuman.memory_tree_recall"), input_schema: query_schema("Natural-language query to embed and rerank against memory summaries."), + annotations: read_only_local_annotations(), }, McpToolSpec { name: "tree.read_chunk", @@ -153,6 +176,7 @@ fn base_tool_specs() -> Vec { "required": ["chunk_id"], "additionalProperties": false }), + annotations: read_only_local_annotations(), }, McpToolSpec { name: "tree.browse", @@ -165,6 +189,7 @@ fn base_tool_specs() -> Vec { pagination.", rpc_method: Some("openhuman.memory_tree_list_chunks"), input_schema: tree_browse_schema(), + annotations: read_only_local_annotations(), }, McpToolSpec { name: "tree.top_entities", @@ -175,6 +200,7 @@ fn base_tool_specs() -> Vec { or `memory.search`. Returns entities ordered by reference count.", rpc_method: Some("openhuman.memory_tree_top_entities"), input_schema: tree_top_entities_schema(), + annotations: read_only_local_annotations(), }, McpToolSpec { name: "tree.list_sources", @@ -186,10 +212,24 @@ fn base_tool_specs() -> Vec { `tree.browse`.", rpc_method: Some("openhuman.memory_tree_list_sources"), input_schema: tree_list_sources_schema(), + annotations: read_only_local_annotations(), }, ] } +/// Annotation preset for the read-only, closed-world tools that just read +/// OpenHuman's local memory tree or agent registry. The MCP spec defaults are +/// `readOnlyHint: false` / `openWorldHint: true`, so both fields must be set +/// explicitly to communicate the actual shape to clients. Destructive and +/// idempotent hints are deliberately omitted — per the spec they are +/// meaningful only when `readOnlyHint == false`. +fn read_only_local_annotations() -> Value { + json!({ + "readOnlyHint": true, + "openWorldHint": false + }) +} + fn searxng_tool_spec() -> McpToolSpec { McpToolSpec { name: "searxng_search", @@ -197,6 +237,14 @@ fn searxng_tool_spec() -> McpToolSpec { description: "Search the configured self-hosted SearXNG instance and return normalized title, URL, snippet, and source results. Requires searxng.enabled=true in OpenHuman config.", rpc_method: Some("openhuman.tools_searxng_search"), input_schema: searxng_search_schema(), + // SearXNG queries an external (self-hosted but network-reachable) + // search engine: read-only (no state mutation), open-world (results + // come from outside OpenHuman). Per spec, destructive/idempotent + // hints are meaningful only when readOnlyHint=false, so omit them. + annotations: json!({ + "readOnlyHint": true, + "openWorldHint": true + }), } } @@ -350,6 +398,7 @@ fn list_tools_result_from_specs(specs: Vec) -> Value { "title": tool.title, "description": tool.description, "inputSchema": tool.input_schema, + "annotations": tool.annotations, }) }) .collect::>(); @@ -1030,6 +1079,103 @@ mod tests { ); } + #[test] + fn list_tools_emits_annotations_for_every_tool() { + // Exercise the searxng-enabled config so the annotation contract covers + // every shipping tool, not just the base set. + let mut config = crate::openhuman::config::Config::default(); + config.searxng.enabled = true; + let result = list_tools_result_for_config(&config); + let tools = result["tools"].as_array().expect("tools array"); + for tool in tools { + let name = tool["name"].as_str().expect("tool name"); + assert!( + tool.get("annotations") + .map(Value::is_object) + .unwrap_or(false), + "tool `{name}` is missing a serialized `annotations` object", + ); + } + } + + #[test] + fn read_only_tools_are_marked_read_only_and_closed_world() { + // Every tool except the act-capable ones reads local OpenHuman state + // (memory tree / agent registry) or queries an external read-only + // search engine. Per MCP spec defaults these would be + // `readOnlyHint: false` and `openWorldHint: true`, so we MUST set + // `readOnlyHint` explicitly to communicate accurate safety affordances + // to clients. (`searxng_search` is read-only but openWorld, so it + // verifies the read-only axis here and is exempt from the + // openWorld=false check below.) + let act_tool_names = ["agent.run_subagent"]; + let open_world_read_only = ["searxng_search"]; + for spec in tool_specs() { + if act_tool_names.contains(&spec.name) { + continue; + } + let annotations = &spec.annotations; + assert_eq!( + annotations.get("readOnlyHint").and_then(Value::as_bool), + Some(true), + "expected `{}` to advertise readOnlyHint=true", + spec.name + ); + let expected_open_world = open_world_read_only.contains(&spec.name); + assert_eq!( + annotations.get("openWorldHint").and_then(Value::as_bool), + Some(expected_open_world), + "expected `{}` to advertise openWorldHint={}", + spec.name, + expected_open_world + ); + // Per spec these are meaningful only when readOnlyHint == false. + // Emitting them on a read-only tool would be misleading. + assert!( + annotations.get("destructiveHint").is_none(), + "read-only tool `{}` should not emit destructiveHint", + spec.name + ); + assert!( + annotations.get("idempotentHint").is_none(), + "read-only tool `{}` should not emit idempotentHint", + spec.name + ); + } + } + + #[test] + fn run_subagent_annotations_signal_act_semantics() { + let spec = tool_specs() + .into_iter() + .find(|spec| spec.name == "agent.run_subagent") + .expect("agent.run_subagent must be registered"); + assert_eq!( + spec.annotations + .get("readOnlyHint") + .and_then(Value::as_bool), + Some(false) + ); + assert_eq!( + spec.annotations + .get("destructiveHint") + .and_then(Value::as_bool), + Some(true) + ); + assert_eq!( + spec.annotations + .get("idempotentHint") + .and_then(Value::as_bool), + Some(false) + ); + assert_eq!( + spec.annotations + .get("openWorldHint") + .and_then(Value::as_bool), + Some(true) + ); + } + #[test] fn list_tools_includes_searxng_when_enabled() { let mut config = crate::openhuman::config::Config::default();