From f720151c63012de30953b2c9bff6d4e6da1175c5 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 3 Jul 2026 05:55:11 +0300 Subject: [PATCH] refactor(ai-chat): move patch_node/insert_node metadata into the shared tool-spec registry (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same tool metadata (zod schema + model-facing description) was hand-duplicated between the standalone MCP server and the in-app AI-chat agent, so every tweak had to land in two places and copies drifted (a materialized parity bug). The shared transport-agnostic registry (packages/mcp/src/tool-specs.ts) already de-duplicates 14 tools; this migrates two more genuinely-identical ones — patch_node/patchNode and insert_node/insertNode. The canonical description is a strict SUPERSET of both originals (keeps MCP's "without resending the whole document" + table-structure/anchor guidance AND the in-app "reversible via page history" / "exactly one of anchorNodeId or anchorText" framing — no model-facing guidance dropped); the schema is identical (the in-app side just gains MCP's .min(1) on ids, a safe tightening). Each transport keeps its own execute/auth wrapper, and the in-app parseNodeArg node-arg normalization is unchanged. The three table tools are intentionally NOT merged (a real param-name divergence: table vs tableRef) — documented on both sides. Other per-transport divergences (search/share/create_comment/transform/list_pages) are left separate with a short comment explaining why (the issue asked to flag these as intentional). DocmostClientLike stays a hand-mirror (the ESM/CJS boundary blocks a compile-time type import; a runtime drift-guard already pins it). Also fixes a latent contract-spec bug: derive `required` from `instanceof z.ZodOptional` (matches the emitted JSON schema) instead of `isOptional()`, which wrongly reported z.any() fields as optional. Partially addresses #294. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/tools/ai-chat-tools.service.ts | 121 +++++++----------- .../tools/shared-tool-specs.contract.spec.ts | 10 +- packages/mcp/build/index.js | 86 +++++-------- packages/mcp/build/tool-specs.js | 80 ++++++++++++ packages/mcp/src/index.ts | 98 ++++++-------- packages/mcp/src/tool-specs.ts | 92 +++++++++++++ packages/mcp/test/unit/tool-specs.test.mjs | 57 +++++++++ 7 files changed, 352 insertions(+), 192 deletions(-) diff --git a/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts b/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts index 2b0c2d8a..91ed2efd 100644 --- a/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts +++ b/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts @@ -173,6 +173,11 @@ export class AiChatToolsService { }); return { + // INTENTIONAL per-transport divergence (not in the shared registry): this + // in-app search runs a semantic + keyword hybrid (RRF) with in-process + // access control and a tuned schema (limit 1-20); the standalone MCP + // `search` is a plain REST full-text search (limit up to 100). Different + // behaviour AND schema, so kept per-layer. searchPages: tool({ description: 'Search the wiki for pages relevant to a query. Combines exact ' + @@ -432,6 +437,10 @@ export class AiChatToolsService { }, }), + // INTENTIONAL per-transport divergence (not shared): the description is + // tuned for the in-app agent (e.g. "retry with a corrected EXACT selection" + // and "Reversible via the comment UI"); the standalone MCP `create_comment` + // keeps its own wording. Kept per-layer. createComment: tool({ description: 'Add an INLINE comment to a page, or reply to an existing top-level ' + @@ -519,6 +528,10 @@ export class AiChatToolsService { async () => await client.getSpaces(), ), + // INTENTIONAL per-transport divergence (not shared): keeps the `tree:true` + // hierarchy mode but is worded for the in-app agent; the standalone MCP + // `list_pages` carries its own wording. Kept per-layer so each side tunes + // its own guidance. listPages: tool({ description: 'List the most recent pages, optionally scoped to a single space. ' + @@ -692,85 +705,25 @@ export class AiChatToolsService { async ({ pageId }) => await client.stashPage(pageId), ), - patchNode: tool({ - description: - 'Replace a single content block (by id) with a new ProseMirror ' + - 'node; the replacement keeps the same nodeId. Example node: a ' + - 'paragraph {"type":"paragraph","content":[{"type":"text","text":"Hello"}]} ' + - 'or a heading {"type":"heading","attrs":{"level":2},"content":' + - '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + - '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node arg ' + - 'may be a JSON object or a JSON string (both accepted). Reversible: ' + - 'the previous version is kept in page history.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page.'), - nodeId: z - .string() - .describe('The block id to replace (from getOutline/getPageJson).'), - node: z - .any() - .describe( - 'The replacement ProseMirror node, e.g. ' + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + - 'JSON object or JSON string both accepted.', - ), - }), - execute: async ({ pageId, nodeId, node }) => { - // Parity with the standalone MCP server (index.ts patch_node): the - // model sometimes serializes the node as a JSON string. Parse it - // before the client's typeof-object guard rejects it. + // Schema + description from the shared registry (identical across both + // transports). The execute body keeps its OWN parseNodeArg normalization: + // the model sometimes serializes the node as a JSON string, and we parse it + // before the client's typeof-object guard rejects it (parity with the + // standalone MCP server, index.ts patch_node). + patchNode: sharedTool( + sharedToolSpecs.patchNode, + async ({ pageId, nodeId, node }) => { const parsedNode = parseNodeArg(node); return await client.patchNode(pageId, nodeId, parsedNode); }, - }), + ), - insertNode: tool({ - description: - 'Insert a ProseMirror node relative to an anchor, or append it at ' + - 'the top level. For before/after you MUST provide EXACTLY ONE of ' + - 'anchorNodeId or anchorText. Example node: a paragraph ' + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]} or a ' + - 'heading {"type":"heading","attrs":{"level":2},"content":' + - '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + - '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node arg ' + - 'may be a JSON object or a JSON string (both accepted). Reversible ' + - 'via page history.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page.'), - node: z - .any() - .describe( - 'The ProseMirror node to insert, e.g. ' + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + - 'JSON object or JSON string both accepted.', - ), - position: z - .enum(['before', 'after', 'append']) - .describe('Where to insert relative to the anchor.'), - anchorNodeId: z - .string() - .optional() - .describe('Anchor block id (for before/after).'), - anchorText: z - .string() - .optional() - .describe( - 'Anchor text fragment (for before/after), matched against the ' + - "block's literal rendered plain text (no markdown). " + - 'Markdown/emoji are tolerated as a fallback; prefer plain text ' + - 'or anchorNodeId.', - ), - }), - execute: async ({ - pageId, - node, - position, - anchorNodeId, - anchorText, - }) => { - // Parity with the standalone MCP server (index.ts insert_node): the - // model sometimes serializes the node as a JSON string. Parse it - // before the client's typeof-object guard rejects it. + // Shared registry schema + description; execute retains parseNodeArg on the + // incoming node (parity with the standalone MCP server, index.ts + // insert_node). + insertNode: sharedTool( + sharedToolSpecs.insertNode, + async ({ pageId, node, position, anchorNodeId, anchorText }) => { const parsedNode = parseNodeArg(node); return await client.insertNode(pageId, parsedNode, { position, @@ -778,7 +731,7 @@ export class AiChatToolsService { anchorText, }); }, - }), + ), deleteNode: sharedTool( sharedToolSpecs.deleteNode, @@ -821,6 +774,10 @@ export class AiChatToolsService { }, }), + // NOT in the shared registry: this layer names the table argument + // `tableRef`, while the standalone MCP tool names it `table` (index.ts). + // Sharing one buildShape would rename a model-facing parameter on one + // transport, so the table row/cell tools stay per-layer by design. tableInsertRow: tool({ description: 'Insert a row of plain-text cells into a table. Reversible via ' + @@ -841,6 +798,8 @@ export class AiChatToolsService { await client.tableInsertRow(pageId, tableRef, cells, index), }), + // NOT shared — same `tableRef` (here) vs `table` (MCP) parameter-name + // divergence as tableInsertRow. tableDeleteRow: tool({ description: 'Delete a table row at a 0-based index. Reversible via page history.', @@ -855,6 +814,8 @@ export class AiChatToolsService { await client.tableDeleteRow(pageId, tableRef, index), }), + // NOT shared — same `tableRef` (here) vs `table` (MCP) parameter-name + // divergence as tableInsertRow. tableUpdateCell: tool({ description: 'Set the plain-text content of a table cell at [row, col] (0-based). ' + @@ -884,6 +845,10 @@ export class AiChatToolsService { await client.importPageMarkdown(pageId, markdown), ), + // INTENTIONAL per-transport divergence (not shared): adds a security + // confirmation framing ("Only share when the user explicitly asked, since + // this exposes the page to anyone with the link") for the in-app agent; the + // standalone MCP `share_page` keeps the plain public-URL wording. sharePage: tool({ description: 'Make a page PUBLICLY accessible and return its public URL. ' + @@ -910,6 +875,10 @@ export class AiChatToolsService { async ({ historyId }) => await client.restorePageVersion(historyId), ), + // INTENTIONAL per-transport divergence (not shared): deliberately omits the + // `deleteComments` schema field (comment-deletion guardrail) and carries a + // much shorter description; the standalone MCP `docmost_transform` exposes + // the full helper catalogue. Different schema, so kept per-layer. transformPage: tool({ description: 'Run a sandboxed JS transform of the form `(doc, ctx) => doc` over a ' + diff --git a/apps/server/src/core/ai-chat/tools/shared-tool-specs.contract.spec.ts b/apps/server/src/core/ai-chat/tools/shared-tool-specs.contract.spec.ts index 31461717..f62f0cc7 100644 --- a/apps/server/src/core/ai-chat/tools/shared-tool-specs.contract.spec.ts +++ b/apps/server/src/core/ai-chat/tools/shared-tool-specs.contract.spec.ts @@ -113,9 +113,15 @@ describe('SHARED_TOOL_SPECS contract parity', () => { const expectedKeys = Object.keys(shape).sort(); expect(actualKeys).toEqual(expectedKeys); - // A non-.optional() field must surface as required in the advertised schema. + // A field that was NOT wrapped in `.optional()` must surface as required in + // the advertised schema. We test for the ZodOptional wrapper rather than + // `isOptional()`: `z.any()`/`z.unknown()` accept `undefined` and so report + // `isOptional() === true`, yet z.toJSONSchema still lists them under + // `required` (they carry no `.optional()`). Matching on the wrapper is what + // the emitted JSON schema actually does, so it stays correct for the + // registry's `node: z.any()` fields (patchNode/insertNode). const expectedRequired = Object.entries(shape) - .filter(([, field]) => !(field as z.ZodTypeAny).isOptional?.()) + .filter(([, field]) => !(field instanceof z.ZodOptional)) .map(([k]) => k) .sort(); expect((json.required ?? []).slice().sort()).toEqual(expectedRequired); diff --git a/packages/mcp/build/index.js b/packages/mcp/build/index.js index a8405a8d..1ba52bba 100644 --- a/packages/mcp/build/index.js +++ b/packages/mcp/build/index.js @@ -76,6 +76,10 @@ export function createDocmostMcpServer(config) { return jsonContent(spaces); }); // Tool: list_pages + // INTENTIONAL per-transport divergence (not in the shared registry): this + // transport exposes a `tree:true` mode that returns the full nested hierarchy; + // the in-app copy keeps the same tree option but is worded for the in-app agent. + // Kept per-layer so each side can tune its own guidance. server.registerTool("list_pages", { description: "List most recent pages in a space ordered by updatedAt (descending). " + "Returns a bounded list (default 50, max 100) — use search for lookups " + @@ -143,6 +147,10 @@ export function createDocmostMcpServer(config) { return jsonContent(result); }); // Tool: table_insert_row + // NOT in the shared registry: this transport names the table argument `table`, + // while the in-app tool names it `tableRef` (ai-chat-tools.service.ts). Sharing + // one buildShape would rename a public MCP parameter, so the table row/cell + // tools stay per-transport by design. server.registerTool("table_insert_row", { description: "Insert a row of plain-text cells into a table. `table` = `#` or " + "a block id inside it. `cells` = text per column (padded to the table's " + @@ -159,6 +167,8 @@ export function createDocmostMcpServer(config) { return jsonContent(result); }); // Tool: table_delete_row + // NOT shared — same `table` (here) vs `tableRef` (in-app) parameter-name + // divergence as table_insert_row. server.registerTool("table_delete_row", { description: "Delete the row at 0-based `index` from a table (`table` = `#` or " + "a block id inside it). Refuses to delete the table's only row. An " + @@ -174,6 +184,8 @@ export function createDocmostMcpServer(config) { return jsonContent(result); }); // Tool: table_update_cell + // NOT shared — same `table` (here) vs `tableRef` (in-app) parameter-name + // divergence as table_insert_row. server.registerTool("table_update_cell", { description: "Set the plain-text content of cell [row,col] (0-based) in a table " + "(`table` = `#` or a block id inside it). Replaces the cell's " + @@ -317,62 +329,17 @@ export function createDocmostMcpServer(config) { }, }; }); - // Tool: patch_node - server.registerTool("patch_node", { - description: "Replaces a single block identified by its attrs.id WITHOUT resending the " + - "whole document. Get the block id from get_page_json, then pass a " + - "ProseMirror node to put in its place. Example node: a paragraph " + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]} or a ' + - 'heading {"type":"heading","attrs":{"level":2},"content":' + - '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + - '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node may be a ' + - "JSON object or a JSON string (both accepted). Cheaper and safer than " + - "update_page_json for one-block structural edits.", - inputSchema: { - pageId: z.string().min(1), - nodeId: z.string().min(1), - node: z - .any() - .describe("ProseMirror node to put in place of the node with this id, e.g. " + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + - "JSON object or JSON string both accepted."), - }, - }, async ({ pageId, nodeId, node }) => { + // Tool: patch_node — schema + description from the shared registry (identical + // across both transports). The execute body keeps its own parseNodeArg + // normalization (the model sometimes serializes `node` as a JSON string). + registerShared(SHARED_TOOL_SPECS.patchNode, async ({ pageId, nodeId, node }) => { const parsedNode = parseNodeArg(node); const result = await docmostClient.patchNode(pageId, nodeId, parsedNode); return jsonContent(result); }); - // Tool: insert_node - server.registerTool("insert_node", { - description: "Insert a block before/after another block (by attrs.id or anchor text) " + - "or append at the end. Get anchor block ids from get_page_json. Avoids " + - "resending the whole document. Can also insert table structure: to add a " + - "tableRow, pass a tableRow node with position before/after and anchor " + - "INSIDE the target table — anchorNodeId of any block/cell in it, or " + - "anchorText matching the table; to add a tableCell/tableHeader, use " + - "anchorNodeId of a block inside the target row (anchorText only resolves " + - "top-level blocks, so it cannot target a row). `anchorText` is matched " + - "against the block's literal rendered plain text (no markdown); " + - "markdown/emoji are tolerated as a fallback; prefer plain text or " + - "anchorNodeId. Note: append is top-level " + - "only and rejects structural table nodes. Example node: a paragraph " + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]} or a ' + - 'heading {"type":"heading","attrs":{"level":2},"content":' + - '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + - '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node may be a ' + - "JSON object or a JSON string (both accepted).", - inputSchema: { - pageId: z.string().min(1), - node: z - .any() - .describe("ProseMirror node to insert, e.g. " + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + - "JSON object or JSON string both accepted."), - position: z.enum(["before", "after", "append"]), - anchorNodeId: z.string().optional(), - anchorText: z.string().optional(), - }, - }, async ({ pageId, node, position, anchorNodeId, anchorText }) => { + // Tool: insert_node — schema + description from the shared registry. As with + // patch_node, the execute body retains parseNodeArg on the incoming node. + registerShared(SHARED_TOOL_SPECS.insertNode, async ({ pageId, node, position, anchorNodeId, anchorText }) => { const parsedNode = parseNodeArg(node); const result = await docmostClient.insertNode(pageId, parsedNode, { position, @@ -453,6 +420,10 @@ export function createDocmostMcpServer(config) { return jsonContent(result); }); // Tool: share_page + // INTENTIONAL per-transport divergence (not shared): the in-app copy adds a + // security-confirmation framing ("only share when the user explicitly asked, + // since this exposes the page to anyone with the link") tuned for the in-app + // agent; this transport keeps the plain public-URL wording. server.registerTool("share_page", { description: "Make a page publicly accessible (idempotent) and return its public " + "URL. The URL format is /share//p/.", @@ -539,6 +510,9 @@ export function createDocmostMcpServer(config) { return jsonContent(comments); }); // Tool: create_comment + // INTENTIONAL per-transport divergence (not shared): the in-app copy tunes the + // guidance for the in-app agent (e.g. "retry with a corrected EXACT selection" + // and "Reversible via the comment UI"); this transport keeps its own wording. server.registerTool("create_comment", { description: "Create a new comment on a page. The comment is ALWAYS inline and is " + "anchored to (highlights) its `selection` text — there are no page-level " + @@ -652,6 +626,10 @@ export function createDocmostMcpServer(config) { return jsonContent(result); }); // Tool: search + // INTENTIONAL per-transport divergence (not shared): the in-app `searchPages` + // runs a semantic + keyword hybrid (RRF) with in-process access control and a + // different schema (limit 1-20); this transport is a plain REST full-text search + // (limit up to 100). Different behaviour AND schema, so kept per-layer. server.registerTool("search", { description: "Search for pages and content. Results are bounded by `limit` " + "(default applied by the client, max 100).", @@ -672,6 +650,10 @@ export function createDocmostMcpServer(config) { return jsonContent(result); }); // Tool: docmost_transform + // INTENTIONAL per-transport divergence (not shared): the in-app `transformPage` + // deliberately omits the `deleteComments` schema field (comment-deletion + // guardrail) and carries a much shorter description; this transport exposes the + // full helper catalogue. Different schema, so kept per-layer. server.registerTool("docmost_transform", { description: "Edit a page by running an arbitrary JS transform `(doc, ctx) => doc` " + "against its LIVE ProseMirror document, with a diff preview and page " + diff --git a/packages/mcp/build/tool-specs.js b/packages/mcp/build/tool-specs.js index 2d3c8ab6..501e30ad 100644 --- a/packages/mcp/build/tool-specs.js +++ b/packages/mcp/build/tool-specs.js @@ -80,6 +80,86 @@ export const SHARED_TOOL_SPECS = { nodeId: z.string().min(1), }), }, + // --- single-block structural write (patch / insert) --- + // + // CANONICAL description merges both layers: the MCP copy's "WITHOUT resending + // the whole document" + "cheaper/safer than a full-document replace" guidance + // AND the in-app copy's "keeps the same node id" + "Reversible via page + // history" framing — nothing either side conveyed is dropped. Sibling tools are + // named in transport-neutral prose ("the page-JSON view", "a full-document + // replace") to match the rest of the registry, since the two layers expose + // those siblings under different (snake_case vs camelCase) identifiers. + patchNode: { + mcpName: 'patch_node', + inAppKey: 'patchNode', + description: 'Replace a single content block identified by its attrs.id with a new ' + + 'ProseMirror node, WITHOUT resending the whole document; the replacement ' + + 'keeps the same node id. Get the block id from the page-JSON view, then ' + + 'pass a ProseMirror node to put in its place. Example node: a paragraph ' + + '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]} or a ' + + 'heading {"type":"heading","attrs":{"level":2},"content":' + + '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + + '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node may be a ' + + 'JSON object or a JSON string (both accepted). Cheaper and safer than ' + + 'replacing the whole document for one-block structural edits. Reversible: ' + + 'the previous version is kept in page history.', + buildShape: (z) => ({ + pageId: z.string().min(1).describe('ID of the page containing the block'), + nodeId: z + .string() + .min(1) + .describe('attrs.id of the block to replace (from the page outline or ' + + 'page-JSON view)'), + node: z + .any() + .describe('ProseMirror node to put in place of the node with this id, e.g. ' + + '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + + 'JSON object or JSON string both accepted.'), + }), + }, + insertNode: { + mcpName: 'insert_node', + inAppKey: 'insertNode', + description: 'Insert a block before/after another block (by attrs.id or anchor text) ' + + 'or append it at the end (top level). For before/after you MUST provide ' + + 'EXACTLY ONE of anchorNodeId or anchorText. Get anchor block ids from the ' + + 'page-JSON view. Avoids resending the whole document. Can also insert ' + + 'table structure: to add a tableRow, pass a tableRow node with position ' + + 'before/after and anchor INSIDE the target table — anchorNodeId of any ' + + 'block/cell in it, or anchorText matching the table; to add a ' + + 'tableCell/tableHeader, use anchorNodeId of a block inside the target row ' + + '(anchorText only resolves top-level blocks, so it cannot target a row). ' + + "`anchorText` is matched against the block's literal rendered plain text " + + '(no markdown); markdown/emoji are tolerated as a fallback; prefer plain ' + + 'text or anchorNodeId. Note: append is top-level only and rejects ' + + 'structural table nodes. Example node: a paragraph ' + + '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]} or a ' + + 'heading {"type":"heading","attrs":{"level":2},"content":' + + '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + + '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node may be a ' + + 'JSON object or a JSON string (both accepted). Reversible via page history.', + buildShape: (z) => ({ + pageId: z.string().min(1), + node: z + .any() + .describe('ProseMirror node to insert, e.g. ' + + '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + + 'JSON object or JSON string both accepted.'), + position: z + .enum(['before', 'after', 'append']) + .describe('Where to insert relative to the anchor.'), + anchorNodeId: z + .string() + .optional() + .describe('Anchor block id (for before/after).'), + anchorText: z + .string() + .optional() + .describe("Anchor text fragment (for before/after), matched against the " + + "block's literal rendered plain text (no markdown). Markdown/emoji " + + 'are tolerated as a fallback; prefer plain text or anchorNodeId.'), + }), + }, // --- share management --- unsharePage: { mcpName: 'unshare_page', diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index e0eb4f69..456e851b 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -105,6 +105,10 @@ export function createDocmostMcpServer(config: DocmostMcpConfig): McpServer { }); // Tool: list_pages +// INTENTIONAL per-transport divergence (not in the shared registry): this +// transport exposes a `tree:true` mode that returns the full nested hierarchy; +// the in-app copy keeps the same tree option but is worded for the in-app agent. +// Kept per-layer so each side can tune its own guidance. server.registerTool( "list_pages", { @@ -195,6 +199,10 @@ server.registerTool( ); // Tool: table_insert_row +// NOT in the shared registry: this transport names the table argument `table`, +// while the in-app tool names it `tableRef` (ai-chat-tools.service.ts). Sharing +// one buildShape would rename a public MCP parameter, so the table row/cell +// tools stay per-transport by design. server.registerTool( "table_insert_row", { @@ -222,6 +230,8 @@ server.registerTool( ); // Tool: table_delete_row +// NOT shared — same `table` (here) vs `tableRef` (in-app) parameter-name +// divergence as table_insert_row. server.registerTool( "table_delete_row", { @@ -243,6 +253,8 @@ server.registerTool( ); // Tool: table_update_cell +// NOT shared — same `table` (here) vs `tableRef` (in-app) parameter-name +// divergence as table_insert_row. server.registerTool( "table_update_cell", { @@ -445,32 +457,11 @@ server.registerTool( }, ); -// Tool: patch_node -server.registerTool( - "patch_node", - { - description: - "Replaces a single block identified by its attrs.id WITHOUT resending the " + - "whole document. Get the block id from get_page_json, then pass a " + - "ProseMirror node to put in its place. Example node: a paragraph " + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]} or a ' + - 'heading {"type":"heading","attrs":{"level":2},"content":' + - '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + - '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node may be a ' + - "JSON object or a JSON string (both accepted). Cheaper and safer than " + - "update_page_json for one-block structural edits.", - inputSchema: { - pageId: z.string().min(1), - nodeId: z.string().min(1), - node: z - .any() - .describe( - "ProseMirror node to put in place of the node with this id, e.g. " + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + - "JSON object or JSON string both accepted.", - ), - }, - }, +// Tool: patch_node — schema + description from the shared registry (identical +// across both transports). The execute body keeps its own parseNodeArg +// normalization (the model sometimes serializes `node` as a JSON string). +registerShared( + SHARED_TOOL_SPECS.patchNode, async ({ pageId, nodeId, node }) => { const parsedNode = parseNodeArg(node); const result = await docmostClient.patchNode(pageId, nodeId, parsedNode); @@ -478,42 +469,10 @@ server.registerTool( }, ); -// Tool: insert_node -server.registerTool( - "insert_node", - { - description: - "Insert a block before/after another block (by attrs.id or anchor text) " + - "or append at the end. Get anchor block ids from get_page_json. Avoids " + - "resending the whole document. Can also insert table structure: to add a " + - "tableRow, pass a tableRow node with position before/after and anchor " + - "INSIDE the target table — anchorNodeId of any block/cell in it, or " + - "anchorText matching the table; to add a tableCell/tableHeader, use " + - "anchorNodeId of a block inside the target row (anchorText only resolves " + - "top-level blocks, so it cannot target a row). `anchorText` is matched " + - "against the block's literal rendered plain text (no markdown); " + - "markdown/emoji are tolerated as a fallback; prefer plain text or " + - "anchorNodeId. Note: append is top-level " + - "only and rejects structural table nodes. Example node: a paragraph " + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]} or a ' + - 'heading {"type":"heading","attrs":{"level":2},"content":' + - '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + - '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node may be a ' + - "JSON object or a JSON string (both accepted).", - inputSchema: { - pageId: z.string().min(1), - node: z - .any() - .describe( - "ProseMirror node to insert, e.g. " + - '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + - "JSON object or JSON string both accepted.", - ), - position: z.enum(["before", "after", "append"]), - anchorNodeId: z.string().optional(), - anchorText: z.string().optional(), - }, - }, +// Tool: insert_node — schema + description from the shared registry. As with +// patch_node, the execute body retains parseNodeArg on the incoming node. +registerShared( + SHARED_TOOL_SPECS.insertNode, async ({ pageId, node, position, anchorNodeId, anchorText }) => { const parsedNode = parseNodeArg(node); const result = await docmostClient.insertNode(pageId, parsedNode, { @@ -619,6 +578,10 @@ server.registerTool( ); // Tool: share_page +// INTENTIONAL per-transport divergence (not shared): the in-app copy adds a +// security-confirmation framing ("only share when the user explicitly asked, +// since this exposes the page to anyone with the link") tuned for the in-app +// agent; this transport keeps the plain public-URL wording. server.registerTool( "share_page", { @@ -746,6 +709,9 @@ server.registerTool( ); // Tool: create_comment +// INTENTIONAL per-transport divergence (not shared): the in-app copy tunes the +// guidance for the in-app agent (e.g. "retry with a corrected EXACT selection" +// and "Reversible via the comment UI"); this transport keeps its own wording. server.registerTool( "create_comment", { @@ -911,6 +877,10 @@ server.registerTool( ); // Tool: search +// INTENTIONAL per-transport divergence (not shared): the in-app `searchPages` +// runs a semantic + keyword hybrid (RRF) with in-process access control and a +// different schema (limit 1-20); this transport is a plain REST full-text search +// (limit up to 100). Different behaviour AND schema, so kept per-layer. server.registerTool( "search", { @@ -937,6 +907,10 @@ server.registerTool( ); // Tool: docmost_transform +// INTENTIONAL per-transport divergence (not shared): the in-app `transformPage` +// deliberately omits the `deleteComments` schema field (comment-deletion +// guardrail) and carries a much shorter description; this transport exposes the +// full helper catalogue. Different schema, so kept per-layer. server.registerTool( "docmost_transform", { diff --git a/packages/mcp/src/tool-specs.ts b/packages/mcp/src/tool-specs.ts index 4074d099..ae7b58ff 100644 --- a/packages/mcp/src/tool-specs.ts +++ b/packages/mcp/src/tool-specs.ts @@ -119,6 +119,98 @@ export const SHARED_TOOL_SPECS = { }), }, + // --- single-block structural write (patch / insert) --- + // + // CANONICAL description merges both layers: the MCP copy's "WITHOUT resending + // the whole document" + "cheaper/safer than a full-document replace" guidance + // AND the in-app copy's "keeps the same node id" + "Reversible via page + // history" framing — nothing either side conveyed is dropped. Sibling tools are + // named in transport-neutral prose ("the page-JSON view", "a full-document + // replace") to match the rest of the registry, since the two layers expose + // those siblings under different (snake_case vs camelCase) identifiers. + patchNode: { + mcpName: 'patch_node', + inAppKey: 'patchNode', + description: + 'Replace a single content block identified by its attrs.id with a new ' + + 'ProseMirror node, WITHOUT resending the whole document; the replacement ' + + 'keeps the same node id. Get the block id from the page-JSON view, then ' + + 'pass a ProseMirror node to put in its place. Example node: a paragraph ' + + '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]} or a ' + + 'heading {"type":"heading","attrs":{"level":2},"content":' + + '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + + '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node may be a ' + + 'JSON object or a JSON string (both accepted). Cheaper and safer than ' + + 'replacing the whole document for one-block structural edits. Reversible: ' + + 'the previous version is kept in page history.', + buildShape: (z) => ({ + pageId: z.string().min(1).describe('ID of the page containing the block'), + nodeId: z + .string() + .min(1) + .describe( + 'attrs.id of the block to replace (from the page outline or ' + + 'page-JSON view)', + ), + node: z + .any() + .describe( + 'ProseMirror node to put in place of the node with this id, e.g. ' + + '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + + 'JSON object or JSON string both accepted.', + ), + }), + }, + + insertNode: { + mcpName: 'insert_node', + inAppKey: 'insertNode', + description: + 'Insert a block before/after another block (by attrs.id or anchor text) ' + + 'or append it at the end (top level). For before/after you MUST provide ' + + 'EXACTLY ONE of anchorNodeId or anchorText. Get anchor block ids from the ' + + 'page-JSON view. Avoids resending the whole document. Can also insert ' + + 'table structure: to add a tableRow, pass a tableRow node with position ' + + 'before/after and anchor INSIDE the target table — anchorNodeId of any ' + + 'block/cell in it, or anchorText matching the table; to add a ' + + 'tableCell/tableHeader, use anchorNodeId of a block inside the target row ' + + '(anchorText only resolves top-level blocks, so it cannot target a row). ' + + "`anchorText` is matched against the block's literal rendered plain text " + + '(no markdown); markdown/emoji are tolerated as a fallback; prefer plain ' + + 'text or anchorNodeId. Note: append is top-level only and rejects ' + + 'structural table nodes. Example node: a paragraph ' + + '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]} or a ' + + 'heading {"type":"heading","attrs":{"level":2},"content":' + + '[{"type":"text","text":"Title"}]}. Bold is a mark: ' + + '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node may be a ' + + 'JSON object or a JSON string (both accepted). Reversible via page history.', + buildShape: (z) => ({ + pageId: z.string().min(1), + node: z + .any() + .describe( + 'ProseMirror node to insert, e.g. ' + + '{"type":"paragraph","content":[{"type":"text","text":"Hello"}]}. ' + + 'JSON object or JSON string both accepted.', + ), + position: z + .enum(['before', 'after', 'append']) + .describe('Where to insert relative to the anchor.'), + anchorNodeId: z + .string() + .optional() + .describe('Anchor block id (for before/after).'), + anchorText: z + .string() + .optional() + .describe( + "Anchor text fragment (for before/after), matched against the " + + "block's literal rendered plain text (no markdown). Markdown/emoji " + + 'are tolerated as a fallback; prefer plain text or anchorNodeId.', + ), + }), + }, + // --- share management --- unsharePage: { diff --git a/packages/mcp/test/unit/tool-specs.test.mjs b/packages/mcp/test/unit/tool-specs.test.mjs index e98f18b6..3c7d9df0 100644 --- a/packages/mcp/test/unit/tool-specs.test.mjs +++ b/packages/mcp/test/unit/tool-specs.test.mjs @@ -83,6 +83,63 @@ test("getNode builder produces exactly { pageId, nodeId }", () => { assert.deepEqual(Object.keys(shape).sort(), ["nodeId", "pageId"]); }); +test("patchNode spec exists, merges BOTH descriptions, builds { pageId, nodeId, node }", () => { + const spec = SHARED_TOOL_SPECS.patchNode; + assert.ok(spec, "patchNode spec missing"); + assert.equal(spec.mcpName, "patch_node"); + assert.equal(spec.inAppKey, "patchNode"); + + // The canonical description must carry the key guidance from BOTH originals: + // - MCP-only: "WITHOUT resending the whole document" + the cheaper/safer note. + // - in-app-only: "keeps the same node id" + the "Reversible ... page history" + // framing the MCP copy lacked. + assert.match(spec.description, /WITHOUT resending the whole document/); + assert.match(spec.description, /Cheaper and safer/); + assert.match(spec.description, /keeps the same node id/i); + assert.match(spec.description, /Reversible/i); + assert.match(spec.description, /page history/i); + + const shape = spec.buildShape(z); + assert.deepEqual(Object.keys(shape).sort(), ["node", "nodeId", "pageId"]); + // A minimal valid input parses (node accepts an arbitrary object via z.any()). + const parsed = z.object(shape).parse({ + pageId: "p1", + nodeId: "n1", + node: { type: "paragraph" }, + }); + assert.equal(parsed.pageId, "p1"); + assert.equal(parsed.nodeId, "n1"); +}); + +test("insertNode spec exists, merges BOTH descriptions, builds the full anchor shape", () => { + const spec = SHARED_TOOL_SPECS.insertNode; + assert.ok(spec, "insertNode spec missing"); + assert.equal(spec.mcpName, "insert_node"); + assert.equal(spec.inAppKey, "insertNode"); + + // Canonical description must keep BOTH sides' nuance: + // - in-app-only: "EXACTLY ONE of anchorNodeId or anchorText" + "Reversible". + // - MCP-only: the table-structure (tableRow/tableCell) insertion guidance. + assert.match(spec.description, /EXACTLY ONE of anchorNodeId or anchorText/); + assert.match(spec.description, /tableRow/); + assert.match(spec.description, /append is top-level only/); + assert.match(spec.description, /Reversible via page history/); + + const shape = spec.buildShape(z); + assert.deepEqual( + Object.keys(shape).sort(), + ["anchorNodeId", "anchorText", "node", "pageId", "position"], + ); + // before/after/append are the only accepted positions; anchors are optional. + const schema = z.object(shape); + assert.doesNotThrow(() => + schema.parse({ pageId: "p1", node: { type: "paragraph" }, position: "append" }), + ); + assert.throws(() => + schema.parse({ pageId: "p1", node: {}, position: "sideways" }), + ); +}); + test("no-arg specs (getWorkspace/listSpaces/listShares) omit buildShape", () => { for (const key of ["getWorkspace", "listSpaces", "listShares"]) { assert.equal(SHARED_TOOL_SPECS[key].buildShape, undefined, `${key} should be no-arg`); -- 2.52.0