feat(mcp): advertise tool annotations on tools/list#2268
Merged
senamakel merged 5 commits intoMay 20, 2026
Conversation
Set the MCP `ToolAnnotations` block (2025-03-26+ spec) on every tool the stdio MCP server exposes. Clients use these hints to decide which confirmation gates to draw — without them, Claude Desktop / Cursor / Inspector treat all 10 tools as the spec defaults (`readOnlyHint: false`, `openWorldHint: true`), so even pure memory-tree reads can trigger destructive-action warnings. Annotation shape: - The 9 read-only tools (`core.list_tools`, `core.tool_instructions`, `agent.list_subagents`, `memory.search`, `memory.recall`, `tree.read_chunk`, `tree.browse`, `tree.top_entities`, `tree.list_sources`) advertise `readOnlyHint: true` and `openWorldHint: false` (local memory tree / agent registry only). Per spec, `destructiveHint` / `idempotentHint` are meaningful only when `readOnlyHint == false`, so they are deliberately omitted. - `agent.run_subagent` is the one Act-policy surface on this server. Sub-agents can call further tools (composio, web search, …), so it advertises `readOnlyHint: false`, `destructiveHint: true`, `idempotentHint: false`, `openWorldHint: true`. Also drop the stale "read-only" claim from the `mcp_server` module doc — `agent.run_subagent` made that comment inaccurate when it landed. Tests: - `list_tools_emits_annotations_for_every_tool` — every entry in the `tools/list` payload has a serialized `annotations` object. - `read_only_tools_are_marked_read_only_and_closed_world` — the 9 read-only tools advertise the right two hints AND do not leak the destructive/idempotent hints that the spec says are meaningful only on non-read-only tools. - `run_subagent_annotations_signal_act_semantics` — `agent.run_subagent` advertises the full Act shape. Local validation was blocked: no Rust toolchain on this machine (`rust-toolchain.toml` pins 1.93.0). CI gate is the enforcement source.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds an ChangesMCP Tool Annotations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/mcp_server/tools.rs (1)
976-989: ⚡ Quick winStrengthen future-proof coverage of read-only annotation semantics.
Line [976]-Line [989] only validates tools present in
read_only_names; a newly added read-only tool could bypass semantic checks if the list isn’t updated. Prefer asserting all tools are read-only by default except an explicit act-capable exception list.Proposed test refactor
- let read_only_names = [ - "core.list_tools", - "core.tool_instructions", - "agent.list_subagents", - "memory.search", - "memory.recall", - "tree.read_chunk", - "tree.browse", - "tree.top_entities", - "tree.list_sources", - ]; + let act_tool_names = ["agent.run_subagent"]; for spec in tool_specs() { - if !read_only_names.contains(&spec.name) { + if act_tool_names.contains(&spec.name) { continue; } let annotations = &spec.annotations; assert_eq!( annotations.get("readOnlyHint").and_then(Value::as_bool),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/mcp_server/tools.rs` around lines 976 - 989, The current check iterates read_only_names and skips any tool not listed, which can miss newly added read-only tools; change the logic to treat tools as read-only by default and only exempt an explicit act-capable list: replace the read_only_names array with an act_capable_names (or similar) whitelist, iterate tool_specs() and for each spec.name assert it's read-only unless spec.name is in act_capable_names, using the same spec.name symbol and the existing assertion code paths to locate where to validate semantics (e.g., in the loop that previously referenced read_only_names and tool_specs()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 976-989: The current check iterates read_only_names and skips any
tool not listed, which can miss newly added read-only tools; change the logic to
treat tools as read-only by default and only exempt an explicit act-capable
list: replace the read_only_names array with an act_capable_names (or similar)
whitelist, iterate tool_specs() and for each spec.name assert it's read-only
unless spec.name is in act_capable_names, using the same spec.name symbol and
the existing assertion code paths to locate where to validate semantics (e.g.,
in the loop that previously referenced read_only_names and tool_specs()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 918c41d1-61bb-4ff7-84a6-05ca0fab7314
📒 Files selected for processing (2)
src/openhuman/mcp_server/mod.rssrc/openhuman/mcp_server/tools.rs
justinhsu1477
force-pushed
the
feat/mcp-tool-annotations
branch
from
3225012 to
df44210
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/mcp_server/tools.rs (1)
970-1017: ⚡ Quick winMake the read-only annotation test exhaustive instead of allowlist-based.
This test currently skips any tool not in
read_only_names, so a newly added read-only tool can accidentally miss validation without failing tests. Prefer asserting by default for all non-agent.run_subagenttools (or asserting full set coverage).Suggested diff
#[test] fn read_only_tools_are_marked_read_only_and_closed_world() { - // Every tool except `agent.run_subagent` reads local OpenHuman state - // (memory tree / agent registry). Per MCP spec defaults these would be - // `readOnlyHint: false` and `openWorldHint: true`, so we MUST set both - // explicitly to communicate accurate safety affordances to clients. - let read_only_names = [ - "core.list_tools", - "core.tool_instructions", - "agent.list_subagents", - "memory.search", - "memory.recall", - "tree.read_chunk", - "tree.browse", - "tree.top_entities", - "tree.list_sources", - ]; for spec in tool_specs() { - if !read_only_names.contains(&spec.name) { + if spec.name == "agent.run_subagent" { continue; } let annotations = &spec.annotations; assert_eq!( annotations.get("readOnlyHint").and_then(Value::as_bool),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/mcp_server/tools.rs` around lines 970 - 1017, Replace the allowlist-based check in read_only_tools_are_marked_read_only_and_closed_world with an exhaustive assertion over tool_specs(): iterate every spec and if spec.name != "agent.run_subagent" assert annotations.get("readOnlyHint").and_then(Value::as_bool) == Some(true) and annotations.get("openWorldHint").and_then(Value::as_bool) == Some(false), and assert destructiveHint/idempotentHint are None for those read-only tools; for "agent.run_subagent" assert the inverse expectations (or explicitly skip it) so newly added tools can't slip past validation—locate this logic in the read_only_tools_are_marked_read_only_and_closed_world test and replace the current read_only_names allowlist usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 970-1017: Replace the allowlist-based check in
read_only_tools_are_marked_read_only_and_closed_world with an exhaustive
assertion over tool_specs(): iterate every spec and if spec.name !=
"agent.run_subagent" assert
annotations.get("readOnlyHint").and_then(Value::as_bool) == Some(true) and
annotations.get("openWorldHint").and_then(Value::as_bool) == Some(false), and
assert destructiveHint/idempotentHint are None for those read-only tools; for
"agent.run_subagent" assert the inverse expectations (or explicitly skip it) so
newly added tools can't slip past validation—locate this logic in the
read_only_tools_are_marked_read_only_and_closed_world test and replace the
current read_only_names allowlist usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 336fa482-f246-489a-bfb3-bc770c55c9bd
📒 Files selected for processing (2)
src/openhuman/mcp_server/mod.rssrc/openhuman/mcp_server/tools.rs
✅ Files skipped from review due to trivial changes (1)
- src/openhuman/mcp_server/mod.rs
5 tasks
graycyrus
reviewed
May 20, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Nice work — clean implementation of MCP ToolAnnotations (spec 2025-03-26+).
What I checked:
McpToolSpec.annotationsfield addition + all tool specs updated ✓read_only_local_annotations()correctly omitsdestructiveHint/idempotentHintper spec (only meaningful whenreadOnlyHint == false) ✓agent.run_subagentannotations accurately reflect its Act-policy semantics (destructiveHint: true,openWorldHint: true) ✓list_tools_result()serializes annotations into the wire format ✓- Three new tests cover: every tool has annotations, read-only split correctness, and run_subagent act semantics ✓
- Docstring in
mod.rsupdated to no longer claim purely read-only surface ✓ - Backward compatible — older clients ignore the field ✓
No issues found. LGTM.
# Conflicts: # src/openhuman/mcp_server/tools.rs
Replace the allowlist with an act-capable exclusion list so any newly added tool that forgets `readOnlyHint` fails the test instead of being silently skipped. `searxng_search` is read-only but openWorld, so it's called out separately on the openWorld axis only. Addresses CodeRabbit nits on PR tinyhumansai#2268.
senamakel
approved these changes
May 20, 2026
Member
|
huge thanks @justinhsu1477, this one's awesome 🙌 wiring up tool annotations so clients get proper safety hints (like that destructive-action gate) is such a nice quality-of-life win. always love seeing you back in the repo 💚 |
12 tasks
senamakel
added a commit
to justinhsu1477/openhuman
that referenced
this pull request
May 22, 2026
- Add `annotations` to the new memory.store / memory.note / tree.tag McpToolSpec entries (writeable, idempotent upsert, closed-world). This unblocks the `Rust Quality (clippy)` CI failure introduced by the `annotations` field added in tinyhumansai#2268 — the new specs were forked from pre-tinyhumansai#2268 tinyhumansai#2306 scaffolding and were missing the required field on rebase against current main. - Extend the read-only-by-default test exemption (`act_tool_names`) to cover the three new write tools, matching their `readOnlyHint: false`. - Fix CodeRabbit nit (tools.rs:803): the oversize-tag error said "characters" but used `String::len()` (byte length). Reword to "bytes" so Unicode tag input gets a self-consistent message. Defers @graycyrus's three inline threads on lines 686/705/1072 (slug_from collisions, memory.note upsert-vs-append, dispatch_write_tool hardcoded rpc_method) to tinyhumansai#2306 per the author's review-thread reply — that's where the underlying functions land and where the fix should originate so this branch can simply inherit on rebase.
5 tasks
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
3 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.
Summary
Adds MCP
ToolAnnotations(spec 2025-03-26+) to every tool advertised byopenhuman-core mcp. Clients usereadOnlyHint/destructiveHint/idempotentHint/openWorldHintto surface accurate safety affordances — e.g. Claude Desktop's "this tool can take destructive actions" confirmation gate.Problem
tools/listtoday returns the curated tool surface but says nothing about each tool's behavior class. The MCP spec definesToolAnnotationsprecisely for this — without them:agent.run_subagent— the one Act-policy surface on the MCP server today — looks identical tomemory.searchto clients, even though it can call further tools (including potentially destructive ones) through any sub-agent it spawns.Solution
annotations: ValuetoMcpToolSpec(always present, serialized into each tool'sannotationsfield in thetools/listresponse).read_only_local_annotations()helper (readOnlyHint: true,openWorldHint: false).agent.run_subagentgets 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.mcp_server/mod.rsdocstring to no longer claim the surface is purely read-only — it's curated, withagent.run_subagentas the one Act-policy surface.destructiveHint/idempotentHintare only meaningful whenreadOnlyHint == false, so the read-only helper omits them.Submission Checklist
list_tools_emits_annotations_for_every_toolasserts every entry serializes a non-nullannotationsobject; a second test pins the read-only / destructive split to each tool's actual behavior.cargo test --lib mcp_server::runs 38/38 locally.N/A: extends row 11.1.4 (MCP stdio server) rather than adding a new feature.## RelatedN/A: extends existing MCP surface, not a release-cut path.Closes #NNN—N/A: capability extension, no issue tracking this.Impact
openhuman-core mcponly. No HTTP RPC, web, or mobile impact.annotationsignore the field; newer clients pick up the safety affordances.Valueper tool, serialized on eachtools/listcall (low frequency).agent.run_subagentstill goes throughenforce_act_policy; the annotation is an advertised hint, not an enforcement point.Related
11.1.4(MCP stdio server)Summary by CodeRabbit
New Features
Tests