From 30be60bc2ddaeaef4c2f085ff00205a69a3eb53f Mon Sep 17 00:00:00 2001 From: Altay Date: Wed, 26 Aug 2026 18:03:38 +0300 Subject: [PATCH] fix(coding-agents): annotate MCP tool safety --- .../coding-agents/src/core/knowledge-tools.ts | 61 +++++++++++++------ .../coding-agents/src/dsh.test.ts | 6 ++ .../coding-agents/src/mcp-server.test.ts | 55 +++++++++-------- .../coding-agents/src/mcp-server.ts | 7 ++- .../coding-agents/src/prime-agent.test.ts | 6 ++ 5 files changed, 89 insertions(+), 46 deletions(-) diff --git a/hindsight-integrations/coding-agents/src/core/knowledge-tools.ts b/hindsight-integrations/coding-agents/src/core/knowledge-tools.ts index 1e73949f6e..c547486e10 100644 --- a/hindsight-integrations/coding-agents/src/core/knowledge-tools.ts +++ b/hindsight-integrations/coding-agents/src/core/knowledge-tools.ts @@ -1,10 +1,12 @@ /** - * Knowledge-page MCP tool specs — SDK-free so this stays unit-testable without a real MCP host. + * Knowledge-page MCP tool specs — runtime SDK-free so this stays unit-testable without a real MCP + * host. * - * `src/mcp-server.ts` is the only file that imports the MCP SDK; it wires the specs returned here - * into an `McpServer`. Each tool wraps one `HindsightClient` knowledge-page/recall method: it never - * throws — a thrown client error is caught and turned into an `isError:true` text result so the - * calling LLM sees the failure instead of the process crashing. + * `src/mcp-server.ts` is the only file with a runtime MCP SDK import; this module uses only its + * `ToolAnnotations` type. The server wires the specs returned here into an `McpServer`. Each tool + * wraps one `HindsightClient` knowledge-page/recall method: it never throws — a thrown client error + * is caught and turned into an `isError:true` text result so the calling LLM sees the failure + * instead of the process crashing. * * The agent-facing surface is intentionally curated: grounding + capture only. Raw page CRUD * (create/update/delete) is deliberately NOT exposed — agents never author page structure; they @@ -16,6 +18,7 @@ import { z } from "zod"; import { existsSync } from "node:fs"; import { homedir } from "node:os"; import { join } from "node:path"; +import type { ToolAnnotations } from "@modelcontextprotocol/sdk/types.js"; import type { ZodRawShape } from "zod"; import type { HindsightClient } from "./hindsight"; import { syncStatus } from "./status"; @@ -33,21 +36,39 @@ export interface ToolResult { isError?: boolean; } +type ToolSafetyAnnotations = Required< + Pick +>; + +const READ_ONLY_ANNOTATIONS: ToolSafetyAnnotations = { + readOnlyHint: true, + destructiveHint: false, + idempotentHint: true, + openWorldHint: false, +}; + +const NON_DESTRUCTIVE_WRITE_ANNOTATIONS: ToolSafetyAnnotations = { + readOnlyHint: false, + destructiveHint: false, + idempotentHint: false, + openWorldHint: false, +}; + export interface ToolSpec { name: string; description: string; inputSchema: ZodRawShape; /** - * True when the tool only READS Hindsight — no document, page or initiative is written. + * Safety metadata published verbatim as the tool's MCP annotations (src/mcp-server.ts). * - * Surfaced to clients as MCP's standard `readOnlyHint` annotation (src/mcp-server.ts). It is not - * cosmetic: Dcode gates every MCP tool lacking a coherent read-only annotation behind an approval - * prompt, so in its headless (`dcode -n`) runtime an unannotated tool is REJECTED outright — - * "This MCP action requires approval, but the current headless runtime has no approval UI." - * Without this, recall and the knowledge-page tools were unusable in every non-interactive Dcode - * session. Writes stay unannotated on purpose: gating them there is the correct behaviour. + * Required, not optional: it is not cosmetic. Dcode gates every MCP tool lacking a coherent + * read-only annotation behind an approval prompt, so in its headless (`dcode -n`) runtime an + * unannotated tool is REJECTED outright — "This MCP action requires approval, but the current + * headless runtime has no approval UI." Codex Auto-review likewise treats an unannotated call as + * unverified external access. Reads get READ_ONLY_ANNOTATIONS; the two writes get + * NON_DESTRUCTIVE_WRITE_ANNOTATIONS so clients still gate them, but for the right reason. */ - readOnly?: boolean; + annotations: ToolSafetyAnnotations; handler: (args: any) => Promise; } @@ -92,7 +113,6 @@ export function buildKnowledgeTools( return [ { name: "hindsight_sync_status", - readOnly: true, description: "Report whether this repo's memory bank is in sync: gitlog seed present, how much recent " + "history has been deepened with full diffs, conversations ingested, knowledge pages " + @@ -100,6 +120,7 @@ export function buildKnowledgeTools( "queryable. Ingestion is automatic and background — if not synced, it is in progress; " + "nothing to run.", inputSchema: {}, + annotations: READ_ONLY_ANNOTATIONS, handler: async () => { try { return ok(await syncStatus(client, bankId, opts.repoDir ?? process.cwd())); @@ -110,13 +131,13 @@ export function buildKnowledgeTools( }, { name: "hindsight_diagnose", - readOnly: true, description: "Report safe Hindsight runtime diagnostics for this coding-agent session: resolved bank, " + "workspace, harness, config location, API endpoint, and non-secret environment overrides. " + "Use this when memory, hooks, MCP tools, or configuration appear not to work. Tokens and " + "other secret values are never returned.", inputSchema: {}, + annotations: READ_ONLY_ANNOTATIONS, handler: async (_args: Record) => { const configPath = process.env.HINDSIGHT_CONFIG || join(homedir(), ".hindsight", "coding-agent.json"); @@ -161,7 +182,6 @@ export function buildKnowledgeTools( }, { name: "hindsight_search_knowledge_pages", - readOnly: true, description: "Search this repository's Hindsight knowledge pages for content relevant to a query — " + "hybrid full-text + semantic search, server-side. Call this when the user's question may " + @@ -171,6 +191,7 @@ export function buildKnowledgeTools( "credit it visibly: start that part with a markdown blockquote header " + '"> 🧠 **From Hindsight memory ()** — ".', inputSchema: { query: z.string().describe("what to look for") }, + annotations: READ_ONLY_ANNOTATIONS, handler: async (args: { query: string }) => { try { const hits = await client.searchKnowledgePages(args.query, 3); @@ -189,7 +210,6 @@ export function buildKnowledgeTools( }, { name: "hindsight_list_knowledge_pages", - readOnly: true, description: "List this repository's Hindsight knowledge pages — curated, continuously-updated " + "summaries of the project's durable knowledge (architecture, components, conventions, key " + @@ -198,11 +218,11 @@ export function buildKnowledgeTools( "periodically in long sessions, to see what the project already knows before you read code " + "or ask the user. The list changes as work is captured, so re-check it occasionally.", inputSchema: {}, + annotations: READ_ONLY_ANNOTATIONS, handler: guarded(async () => client.listPages()), }, { name: "hindsight_read_knowledge_page", - readOnly: true, description: "Read the full content of one knowledge page by its id (from " + "hindsight_list_knowledge_pages). Call this whenever a listed page is relevant to what " + @@ -211,11 +231,11 @@ export function buildKnowledgeTools( "contain [[page:]] links to related pages; follow one by calling this tool again with " + "that id. Prefer reading a page over re-deriving the same understanding from source.", inputSchema: { page_id: z.string() }, + annotations: READ_ONLY_ANNOTATIONS, handler: guarded(async ({ page_id }) => client.getPage(page_id)), }, { name: "hindsight_reflect", - readOnly: true, description: "Deep memory reasoning: an agentic synthesis over this repository's FULL memory (git " + "decisions, past sessions, ingested knowledge) that answers WHY questions — the past " + @@ -224,6 +244,7 @@ export function buildKnowledgeTools( "shallow and you need the root cause or the decided literals. When the answer informs " + 'your reply, credit it visibly with a blockquote header: "> 🧠 **From Hindsight memory** — ".', inputSchema: { query: z.string().describe("the question to reason over memory about") }, + annotations: READ_ONLY_ANNOTATIONS, handler: guarded(async ({ query }: { query: string }) => client.reflect(query, { budget: opts.reflectBudget ?? "high", @@ -260,6 +281,7 @@ export function buildKnowledgeTools( summary: z.string(), relates_to_page_id: z.string().optional(), }, + annotations: NON_DESTRUCTIVE_WRITE_ANNOTATIONS, handler: guarded(async ({ title, summary, relates_to_page_id }) => client.captureInitiative({ title, @@ -281,6 +303,7 @@ export function buildKnowledgeTools( "'Correction: ' stating what memory claimed, what is actually true, and the " + "evidence — the newer fact supersedes the stale one in future retrieval.", inputSchema: { title: z.string(), content: z.string() }, + annotations: NON_DESTRUCTIVE_WRITE_ANNOTATIONS, handler: guarded(async ({ title, content }) => { const docId = title diff --git a/hindsight-integrations/coding-agents/src/dsh.test.ts b/hindsight-integrations/coding-agents/src/dsh.test.ts index 8610258014..47988406cd 100644 --- a/hindsight-integrations/coding-agents/src/dsh.test.ts +++ b/hindsight-integrations/coding-agents/src/dsh.test.ts @@ -153,6 +153,12 @@ describe("toDshParameters", () => { name: "hindsight_capture_initiative", description: "…", inputSchema, + annotations: { + readOnlyHint: false, + destructiveHint: false, + idempotentHint: false, + openWorldHint: false, + }, handler: async () => ({ content: [] }), }); diff --git a/hindsight-integrations/coding-agents/src/mcp-server.test.ts b/hindsight-integrations/coding-agents/src/mcp-server.test.ts index 088a38abdd..c1cabed35a 100644 --- a/hindsight-integrations/coding-agents/src/mcp-server.test.ts +++ b/hindsight-integrations/coding-agents/src/mcp-server.test.ts @@ -164,11 +164,34 @@ describe("buildMcpServer", () => { /** * Dcode computes an "is this tool coherently read-only" verdict from the MCP annotations and, - * in headless mode, REJECTS every call that fails it. So the annotation is a functional - * requirement, not documentation: without it `dcode -n` cannot search or read knowledge pages. - * Assert it over the wire (what a client actually sees), not on the specs. + * in headless mode, REJECTS every call that fails it; Codex Auto-review reads the same metadata + * to tell a safe knowledge read from a write. So the annotations are a functional requirement, + * not documentation. Assert them over the wire (what a client actually sees), not on the specs. */ - it("advertises readOnlyHint on exactly the tools that only read Hindsight", async () => { + it("publishes explicit safety annotations for every enabled tool", async () => { + const readOnly = { + readOnlyHint: true, + destructiveHint: false, + idempotentHint: true, + openWorldHint: false, + }; + // The writes are additive, never idempotent: clients should still gate them. + const nonDestructiveWrite = { + readOnlyHint: false, + destructiveHint: false, + idempotentHint: false, + openWorldHint: false, + }; + const expected = { + hindsight_sync_status: readOnly, + hindsight_diagnose: readOnly, + hindsight_search_knowledge_pages: readOnly, + hindsight_list_knowledge_pages: readOnly, + hindsight_read_knowledge_page: readOnly, + hindsight_reflect: readOnly, + hindsight_capture_initiative: nonDestructiveWrite, + hindsight_ingest_document: nonDestructiveWrite, + }; const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); const server = buildMcpServer(selectTools(resolveConfig({}), stubClient, "b")); const client = new Client({ name: "test-client", version: "0.1.0" }); @@ -176,26 +199,10 @@ describe("buildMcpServer", () => { await server.connect(serverTransport); await client.connect(clientTransport); try { - const { tools } = await client.listTools(); - const readOnly = tools - .filter((t) => t.annotations?.readOnlyHint === true) - .map((t) => t.name) - .sort(); - expect(readOnly).toEqual([ - "hindsight_diagnose", - "hindsight_list_knowledge_pages", - "hindsight_read_knowledge_page", - "hindsight_reflect", - "hindsight_search_knowledge_pages", - "hindsight_sync_status", - ]); - // The writes must stay unannotated: gating them behind approval is correct. - const writes = tools.filter((t) => t.annotations?.readOnlyHint !== true).map((t) => t.name); - expect(writes.sort()).toEqual(["hindsight_capture_initiative", "hindsight_ingest_document"]); - // A read-only hint paired with a destructive one is INCOHERENT and gets gated anyway. - for (const tool of tools) { - if (tool.annotations?.readOnlyHint) expect(tool.annotations.destructiveHint).toBe(false); - } + const listed = await client.listTools(); + expect(Object.fromEntries(listed.tools.map((tool) => [tool.name, tool.annotations]))).toEqual( + expected + ); } finally { await client.close(); await server.close(); diff --git a/hindsight-integrations/coding-agents/src/mcp-server.ts b/hindsight-integrations/coding-agents/src/mcp-server.ts index 6847aabb92..1917bafb2a 100644 --- a/hindsight-integrations/coding-agents/src/mcp-server.ts +++ b/hindsight-integrations/coding-agents/src/mcp-server.ts @@ -85,14 +85,15 @@ export function buildMcpServer(tools: ToolSpec[]): McpServer { } for (const tool of tools) { - // registerTool (not the deprecated `tool()`) so the read-only annotation reaches the client: - // Dcode rejects unannotated MCP calls outright in headless mode. See ToolSpec.readOnly. + // registerTool (not the deprecated `tool()`) so the safety annotations reach the client: + // Dcode rejects unannotated MCP calls outright in headless mode, and Codex Auto-review treats + // them as unverified external access. See ToolSpec.annotations. server.registerTool( tool.name, { description: tool.description, inputSchema: tool.inputSchema, - ...(tool.readOnly ? { annotations: { readOnlyHint: true, destructiveHint: false } } : {}), + annotations: tool.annotations, }, tool.handler ); diff --git a/hindsight-integrations/coding-agents/src/prime-agent.test.ts b/hindsight-integrations/coding-agents/src/prime-agent.test.ts index 08788273ed..9be71545b6 100644 --- a/hindsight-integrations/coding-agents/src/prime-agent.test.ts +++ b/hindsight-integrations/coding-agents/src/prime-agent.test.ts @@ -108,6 +108,12 @@ describe("Prime Agent extension adapter", () => { name: "hindsight_search_knowledge_pages", description: "Search the knowledge pages", inputSchema: { query: z.string(), limit: z.number().optional() }, + annotations: { + readOnlyHint: true, + destructiveHint: false, + idempotentHint: true, + openWorldHint: false, + }, handler: async () => ({ content: [{ type: "text", text: "page A\npage B" }] }), };