feat(mcp): add tool safety annotations (#1127)

* feat(mcp): add tool safety annotations

* test(mcp): address PR #1127 review follow-ups

- Replace private `_requestHandlers` SDK access in server.test.ts with
  `Client` + `InMemoryTransport.createLinkedPair()` for the tools/list
  annotation propagation test. The new path uses supported public APIs
  and surfaces SDK changes loudly instead of silently degrading.
- Extract `OPEN_WORLD_READ_ONLY_TOOLS` set in tools.test.ts so future
  read-only open-world tools can be added without rewriting the
  invariant; preserves the current "only `query` is open-world" guard.
- Add inline rationale on `group_sync` annotations explaining the
  conservative `idempotentHint: false` (writes contracts.json on every
  call even when output is deterministic).

No runtime behavior change. Annotations themselves and tools/list shape
are unchanged.

---------

Co-authored-by: Gergo Magyar <gergomagyar@icloud.com>

gitnexus/src/mcp/server.ts

@@ -158,6 +158,7 @@ export function createMCPServer(backend: LocalBackend): Server {
       name: tool.name,
       description: tool.description,
       inputSchema: tool.inputSchema,
+      annotations: tool.annotations,
     })),
   }));
 

gitnexus/src/mcp/tools.ts

@@ -5,9 +5,12 @@
  * All tools support an optional `repo` parameter for multi-repo setups.
  */
 
+import type { ToolAnnotations } from '@modelcontextprotocol/sdk/types.js';
+
 export interface ToolDefinition {
   name: string;
   description: string;
+  annotations: ToolAnnotations;
   inputSchema: {
     type: 'object';
     properties: Record<
@@ -27,6 +30,27 @@ export interface ToolDefinition {
   };
 }
 
+const READ_ONLY_TOOL_ANNOTATIONS: ToolAnnotations = {
+  readOnlyHint: true,
+  destructiveHint: false,
+  idempotentHint: true,
+  openWorldHint: false,
+};
+
+const QUERY_TOOL_ANNOTATIONS: ToolAnnotations = {
+  readOnlyHint: true,
+  destructiveHint: false,
+  idempotentHint: true,
+  openWorldHint: true,
+};
+
+const DESTRUCTIVE_TOOL_ANNOTATIONS: ToolAnnotations = {
+  readOnlyHint: false,
+  destructiveHint: true,
+  idempotentHint: false,
+  openWorldHint: false,
+};
+
 export const GITNEXUS_TOOLS: ToolDefinition[] = [
   {
     name: 'list_repos',
@@ -39,6 +63,7 @@ AFTER THIS: READ gitnexus://repo/{name}/context for the repo you want to work wi
 
 When multiple repos are indexed, you MUST specify the "repo" parameter
 on other tools (query, context, impact, etc.) to target the correct one.`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {},
@@ -63,6 +88,7 @@ Hybrid ranking: BM25 keyword + semantic vector search, ranked by Reciprocal Rank
 GROUP MODE: set "repo" to "@<groupName>" to search all member repos in that group (merged via RRF), or "@<groupName>/<groupRepoPath>" to run against a single member (same path keys as in group.yaml). If you use "@<groupName>" only, the member repo defaults to the lexicographically first key in group.yaml "repos". Prefer resources for contracts/status (see migration from legacy group_* tools).
 
 SERVICE: optional monorepo path prefix (POSIX-style, case-sensitive segments). When "repo" starts with "@", only processes whose symbols fall under that prefix are included. For a normal indexed repo name (no leading @), this field is currently ignored by the server.`,
+    annotations: QUERY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -156,6 +182,7 @@ TIPS:
 - Community = auto-detected functional area (Leiden algorithm). Properties: heuristicLabel, cohesion, symbolCount, keywords, description, enrichedBy
 - Process = execution flow trace from entry point to terminal. Properties: heuristicLabel, processType, stepCount, communities, entryPointId, terminalId
 - Use heuristicLabel (not label) for human-readable community/process names`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -183,6 +210,7 @@ NOTE: ACCESSES edges (field read/write tracking) are included in context results
 GROUP MODE: set "repo" to "@<groupName>" to run context in each member repo (aggregated list), or "@<groupName>/<groupRepoPath>" for one member. If you use "@<groupName>" only, the member defaults to the lexicographically first key in group.yaml "repos".
 
 SERVICE: optional monorepo path prefix (case-sensitive path segments). When "repo" starts with "@", prefix-matches resolved symbol file paths; when a hit is outside the prefix, that member returns an empty payload for the symbol. Ignored for a normal indexed repo name.`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -226,6 +254,7 @@ WHEN TO USE: Before committing — to understand what your changes affect. Pre-c
 AFTER THIS: Review affected processes. Use context() on high-risk symbols. READ gitnexus://repo/{name}/process/{name} for full traces.
 
 Returns: changed symbols, affected processes, and a risk summary.`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -258,6 +287,7 @@ AFTER THIS: Run detect_changes() to verify no unexpected side effects.
 Each edit is tagged with confidence:
 - "graph": found via knowledge graph relationships (high confidence, safe to accept)
 - "text_search": found via regex text search (lower confidence, review carefully)`,
+    annotations: DESTRUCTIVE_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -311,6 +341,7 @@ Confidence: 1.0 = certain, <0.8 = fuzzy match
 GROUP MODE: set "repo" to "@<groupName>" for cross-repo impact anchored at the default member (lexicographically first key in group.yaml "repos"), or "@<groupName>/<groupRepoPath>" to choose the member (same path keys as in group.yaml). Phase-1 walk runs in that member; cross-boundary fan-out uses the group bridge.
 
 SERVICE: optional monorepo path prefix (case-sensitive path segments). When "repo" starts with "@", scopes the local impact walk and cross-repo symbol paths to files under that prefix; ignored for a normal indexed repo name.`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -404,6 +435,7 @@ WHEN TO USE: Understanding API consumption patterns, finding orphaned routes. Fo
 AFTER THIS: Use impact() on specific route handlers to see full blast radius.
 
 Returns: route nodes with their handlers, middleware wrapper chains (e.g., withAuth, withRateLimit), and consumers.`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -426,6 +458,7 @@ Returns: route nodes with their handlers, middleware wrapper chains (e.g., withA
 WHEN TO USE: Understanding tool APIs, finding tool implementations, impact analysis for tool changes.
 
 Returns: tool nodes with their handler files and descriptions.`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -443,6 +476,7 @@ WHEN TO USE: Detecting mismatches between what an API route returns and what con
 REQUIRES: Route nodes with responseKeys (extracted from .json({...}) calls during indexing).
 
 Returns routes that have both detected response keys AND consumers. Shows top-level keys each endpoint returns (e.g., data, pagination, error) and what keys each consumer accesses. Reports MISMATCH status when a consumer accesses keys not present in the route's response shape.`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -467,6 +501,7 @@ WHEN TO USE: BEFORE modifying any API route handler. Shows what consumers depend
 Risk levels: LOW (0-3 consumers), MEDIUM (4-9 or any mismatches), HIGH (10+ consumers or mismatches with 4+ consumers). Mismatches with confidence "low" indicate the consumer file fetches multiple routes — property attribution is approximate.
 
 Returns: single route object when one match, or { routes: [...], total: N } for multiple matches. Combines route_map, shape_check, and impact data.`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -482,6 +517,7 @@ Returns: single route object when one match, or { routes: [...], total: N } for
     description: `List all configured repository groups, or return details for one group (repos, manifest links).
 
 WHEN TO USE: Discover groups before group_sync. Optional "name" returns a single group's config.`,
+    annotations: READ_ONLY_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {
@@ -495,6 +531,9 @@ WHEN TO USE: Discover groups before group_sync. Optional "name" returns a single
     description: `Rebuild the Contract Registry (contracts.json) for a group: extract HTTP contracts, apply manifest links, exact-match cross-links.
 
 WHEN TO USE: After changing group.yaml or re-indexing member repos.`,
+    // Writes contracts.json on every call; conservatively non-idempotent
+    // even though output is deterministic for identical input.
+    annotations: DESTRUCTIVE_TOOL_ANNOTATIONS,
     inputSchema: {
       type: 'object',
       properties: {

gitnexus/test/unit/server.test.ts

@@ -13,7 +13,10 @@
  * directly through the MCP Server's handler dispatch.
  */
 import { describe, it, expect, vi } from 'vitest';
+import { Client } from '@modelcontextprotocol/sdk/client/index.js';
+import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js';
 import { createMCPServer } from '../../src/mcp/server.js';
+import { GITNEXUS_TOOLS } from '../../src/mcp/tools.js';
 
 // ─── Mock backend ──────────────────────────────────────────────────
 
@@ -52,6 +55,28 @@ describe('createMCPServer', () => {
     // The server has registered handlers — verify it was created without errors
     expect(server).toBeTruthy();
   });
+
+  it('tools/list response includes tool annotations', async () => {
+    const backend = createMockBackend();
+    const server = createMCPServer(backend);
+    const client = new Client({ name: 'test-client', version: '0.0.0' });
+    const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
+
+    try {
+      await Promise.all([server.connect(serverTransport), client.connect(clientTransport)]);
+
+      const response = await client.listTools();
+      expect(response.tools).toHaveLength(GITNEXUS_TOOLS.length);
+
+      for (const tool of response.tools) {
+        const definition = GITNEXUS_TOOLS.find((t) => t.name === tool.name)!;
+        expect(tool.annotations).toEqual(definition.annotations);
+      }
+    } finally {
+      await client.close();
+      await server.close();
+    }
+  });
 });
 
 // ─── getNextStepHint (tested indirectly via server tool handler) ──────

gitnexus/test/unit/tools.test.ts

@@ -11,6 +11,10 @@ import { describe, it, expect } from 'vitest';
 import { GITNEXUS_TOOLS } from '../../src/mcp/tools.js';
 
 const GROUP_TOOLS = new Set(['group_list', 'group_sync']);
+const MUTATING_TOOLS = new Set(['rename', 'group_sync']);
+// Read-only tools that legitimately reach external systems. Add a tool name
+// here when introducing a read-only tool that needs openWorldHint: true.
+const OPEN_WORLD_READ_ONLY_TOOLS = new Set(['query']);
 
 describe('GITNEXUS_TOOLS', () => {
   it('exports all tools (7 base + 3 route/tool/shape + 1 api_impact + 2 group)', () => {
@@ -39,6 +43,7 @@ describe('GITNEXUS_TOOLS', () => {
       expect(typeof tool.name).toBe('string');
       expect(tool.description).toBeTruthy();
       expect(typeof tool.description).toBe('string');
+      expect(tool.annotations).toBeDefined();
       expect(tool.inputSchema).toBeDefined();
       expect(tool.inputSchema.type).toBe('object');
       expect(tool.inputSchema.properties).toBeDefined();
@@ -46,6 +51,48 @@ describe('GITNEXUS_TOOLS', () => {
     }
   });
 
+  it('each tool exposes all MCP safety annotations', () => {
+    for (const tool of GITNEXUS_TOOLS) {
+      expect(typeof tool.annotations.readOnlyHint).toBe('boolean');
+      expect(typeof tool.annotations.destructiveHint).toBe('boolean');
+      expect(typeof tool.annotations.idempotentHint).toBe('boolean');
+      expect(typeof tool.annotations.openWorldHint).toBe('boolean');
+    }
+  });
+
+  it('read-only tools are marked non-destructive and idempotent', () => {
+    for (const tool of GITNEXUS_TOOLS) {
+      if (MUTATING_TOOLS.has(tool.name)) continue;
+
+      expect(tool.annotations.readOnlyHint).toBe(true);
+      expect(tool.annotations.destructiveHint).toBe(false);
+      expect(tool.annotations.idempotentHint).toBe(true);
+      expect(tool.annotations.openWorldHint).toBe(OPEN_WORLD_READ_ONLY_TOOLS.has(tool.name));
+    }
+  });
+
+  it('query is marked open-world because it may use external embeddings', () => {
+    const queryTool = GITNEXUS_TOOLS.find((t) => t.name === 'query')!;
+    expect(queryTool.annotations).toEqual({
+      readOnlyHint: true,
+      destructiveHint: false,
+      idempotentHint: true,
+      openWorldHint: true,
+    });
+  });
+
+  it('rename and group_sync are marked mutating and non-idempotent', () => {
+    for (const name of ['rename', 'group_sync'] as const) {
+      const tool = GITNEXUS_TOOLS.find((t) => t.name === name)!;
+      expect(tool.annotations).toEqual({
+        readOnlyHint: false,
+        destructiveHint: true,
+        idempotentHint: false,
+        openWorldHint: false,
+      });
+    }
+  });
+
   it('query tool requires "query" parameter', () => {
     const queryTool = GITNEXUS_TOOLS.find((t) => t.name === 'query')!;
     expect(queryTool.inputSchema.required).toContain('query');