feat(mcp): add per-tool risk annotations#1098
Conversation
|
@ahjota is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughMCP tool definitions now support ChangesMCP risk annotations
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ToolRegistry
participant getVisibleTools
participant handleToolsList
participant MCPClient
ToolRegistry->>getVisibleTools: provide annotated tool definitions
getVisibleTools->>handleToolsList: return visible annotated tools
handleToolsList->>MCPClient: send tools/list response with annotations
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/mcp-tool-annotations.test.ts`:
- Around line 149-162: Extend the test around getVisibleTools to also set
AGENTMEMORY_TOOLS to "core" and verify every returned tool has defined
annotations. Preserve the existing environment-variable restoration in the
finally block, and assert the expected filtered result as appropriate for the
core visibility path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3937830d-e403-4f91-954f-6b0e276aee67
📒 Files selected for processing (3)
AGENTS.mdsrc/mcp/tools-registry.tstest/mcp-tool-annotations.test.ts
Address CodeRabbit feedback on rohitg00#1098: the getVisibleTools test only exercised AGENTMEMORY_TOOLS=all. Also assert AGENTMEMORY_TOOLS=core returns the 8 essential tools and that every filtered tool retains its annotations, so the core code path is covered. Refs rohitg00#1097 Signed-off-by: AJ Alon <alexander.joel.alon@gmail.com>
Follow-up to CodeRabbit feedback on rohitg00#1098: hardcoding 53 and 8 duplicated contracts owned elsewhere (tool-count-consistency.test.ts owns the 53; ESSENTIAL_TOOLS.size is the source of truth for core). Derive total via getAllTools().length and core via ESSENTIAL_TOOLS.size, keeping the 23/26/4 classification counts and frozen name sets hardcoded since those are the contract this test verifies. Adds a partition sum invariant (23+4+26 === total) that now meaningfully catches a new tool added without classification. Refs rohitg00#1097 Signed-off-by: AJ Alon <alexander.joel.alon@gmail.com>
|
Occurs to me that you'd want the full table for all of the MCP tools.
Totals: 23 read-only + 27 state-changing + 3 destructive = 53.
Mapping to Droid Autonomy Level auto-run behavior:
|
Add MCP-standard annotations (readOnlyHint/destructiveHint) to all 53 tools so Droid classifies read-only tools as low/medium risk and auto-runs them under Auto (Low)/Medium, instead of defaulting every tool to high risk and prompting on each call. Non-destructive writes are marked non-read-only (medium), destructive tools keep the destructive hint (high); Auto (High) is unchanged. Annotations propagate through the existing tools/list response in both stdio and http transports, so no handler or REST changes are needed. Tool count is unchanged. Refs rohitg00#1097 Signed-off-by: AJ Alon <alexander.joel.alon@gmail.com>
Address CodeRabbit feedback on rohitg00#1098: the getVisibleTools test only exercised AGENTMEMORY_TOOLS=all. Also assert AGENTMEMORY_TOOLS=core returns the 8 essential tools and that every filtered tool retains its annotations, so the core code path is covered. Refs rohitg00#1097 Signed-off-by: AJ Alon <alexander.joel.alon@gmail.com>
Follow-up to CodeRabbit feedback on rohitg00#1098: hardcoding 53 and 8 duplicated contracts owned elsewhere (tool-count-consistency.test.ts owns the 53; ESSENTIAL_TOOLS.size is the source of truth for core). Derive total via getAllTools().length and core via ESSENTIAL_TOOLS.size, keeping the 23/26/4 classification counts and frozen name sets hardcoded since those are the contract this test verifies. Adds a partition sum invariant (23+4+26 === total) that now meaningfully catches a new tool added without classification. Refs rohitg00#1097 Signed-off-by: AJ Alon <alexander.joel.alon@gmail.com>
8de106c to
5485a40
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/mcp-tool-annotations.test.ts`:
- Around line 160-162: Update the annotation assertions in the visible-tools
loop of the mode test to compare each tool’s annotations with the corresponding
registry entry, rather than only checking that annotations are defined. Use the
tool’s identifying name or registry lookup already available in the test, and
preserve the existing coverage across each mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5732a68-597b-445b-85c5-26e87b0cebe7
📒 Files selected for processing (3)
AGENTS.mdsrc/mcp/tools-registry.tstest/mcp-tool-annotations.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- AGENTS.md
- src/mcp/tools-registry.ts
What
Adds MCP-standard
annotations(readOnlyHint/destructiveHint) to all 53 tools insrc/mcp/tools-registry.tsso that MCP clients (notably Droid, via its Autonomy Level risk classification) can auto-run read-only agentmemory tools in low and medium autonomy modes, instead of defaulting every tool to high risk and prompting on each call.Why
Droid compares each MCP tool's risk level to the session's Autonomy Level and auto-runs when risk is at or below that level (docs).
Tools missing safety metadata are classified as
high, so they prompt at every level below Auto (High). Today every agentmemory tool — even pure reads likememory_recall,memory_sessions,memory_slot_list— prompts under Auto (Low) and Auto (Medium), making low/medium autonomy unusable with agentmemory.With per-tool annotations, the classification becomes:
Behavioral matrix:
Auto (High) is unchanged. The change is purely additive metadata.
How
src/mcp/tools-registry.ts: extendedMcpToolDefwith an optionalMcpToolAnnotationsfield and addedannotationsto every tool definition. (I admit I let Droid categorize each definition for me.)test/mcp-tool-annotations.test.ts: new behavior-named test asserting the 23/26/4 classification, the mutual-exclusivity of read-only/destructive hints,getVisibleTools()annotation preservation underAGENTMEMORY_TOOLS=all, and thathandleToolsList()(standalonetools/listresponse) carries the same annotations as the registry.AGENTS.md: one-line note on the "adding/removing MCP tools" consistency checklist so future tools carryannotations.The annotations propagate through the existing
tools/listresponse in both transports (proxymcp::tools::listreturnsgetVisibleTools(); standalonehandleToolsListreturnsgetAllTools()/local fallback), so noserver.ts,standalone.ts,transport.ts, orsrc/triggers/api.tschanges are needed.How to verify
npm install(Node >=20),npm run build— compiles clean.npm test— new test passes; the only failing tests in this worktree are pre-existing and unrelated (test/hook-project.test.tsasserts cwd basenameagentmemorybut the worktree dir differs;test/embedding-provider.test.tsis env-dependent).node dist/standalone.mjsin local-fallback mode, issuetools/list, confirmmemory_recallreportsannotations: { readOnlyHint: true, destructiveHint: false }.memory_recallauto-runs whilememory_save/memory_slot_deleteprompt; at Auto (Medium) read-only and non-destructive writes auto-run while destructive still prompts; at Auto (High) everything auto-runs (base case preserved).Before
I have some nice screenvids of the before and after, but they are too big for GitHub. :) With most recent release of agentmemory, and in a medium autonomy Droid session, I'm testing the memory_export tool call:
Note that Droid calls this a high impact tool.
(It does this for all of the agentmemory tools.)
After
After running my build of agentmemory and in a low autonomy Droid session:
Note that Droid now classifies memory_export as
low impact. I change autonomy to medium, which lifts the safety floor:I am not prompted:
Closes #1097.
Summary by CodeRabbit
New Features
Tests
Documentation