From e348433a3922d23b2ec3b96426d8618aa6fc1129 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 4 Jul 2026 21:26:02 +0300 Subject: [PATCH 1/6] refactor(ai-chat): unify comment tools into SHARED_TOOL_SPECS (#294, comments family) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrates the three-layer comment tools into the single transport-agnostic spec registry (schema + model-facing description declared once; each transport keeps only its execute/auth): - createComment, listComments, resolveComment, checkNewComments — moved to SHARED_TOOL_SPECS; index.ts uses registerShared(), ai-chat uses sharedTool(); removed from INLINE_TOOL_TIERS (tier/catalogLine now on the spec). Tiers preserved from CORE_TOOL_KEYS (create/list/resolve = core, check = deferred). Intentionally NOT migrated (kept MCP-inline): update_comment / delete_comment — they are MCP-only by design; the in-app AI-chat layer deliberately has no updateComment/deleteComment (comment edits are irreversible / not version-tracked), asserted by ai-chat-tools.service.spec.ts. A registry spec's tier/catalogLine are in-app metadata and the catalog-partition test forbids a deferred spec without a live in-app tool, so these stay per-transport. Drift reconciled (documented inline): createComment/listComments/checkNewComments took the more-maintained/superset description + stricter .min(1) guards. resolveComment: `resolved` drifted (MCP optional+default(true) vs in-app required) — kept the MCP superset, so in-app resolveComment now accepts an omitted `resolved` (defaults to resolve) — a deliberate, backward-compatible unification (never rejects a previously-valid input). Gate: mcp build 0 + node --test 480/480, ai-chat 654, tool-tiers (incl. F3 catalog-partition) 16/16, server tsc 0. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/tools/ai-chat-tools.service.ts | 127 +++---------- .../src/core/ai-chat/tools/tool-tiers.ts | 24 +-- packages/mcp/src/index.ts | 129 ++----------- packages/mcp/src/tool-specs.ts | 171 ++++++++++++++++++ 4 files changed, 214 insertions(+), 237 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 76aa48e7..21933219 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 @@ -455,59 +455,13 @@ 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 ' + - 'comment (one level only — the backend rejects replies to replies). ' + - 'The comment is anchored inline to the given exact `selection` text ' + - '(which gets highlighted); page-level comments are NOT supported. A ' + - "new top-level comment REQUIRES a `selection`. Replies inherit the " + - "parent's anchor and take no selection. If the call fails with a " + - '"selection not found" error, retry with a corrected EXACT selection ' + - 'copied verbatim from a single paragraph/block. You may also attach a ' + - '`suggestedText` proposing a replacement for the `selection` (a human ' + - 'applies it from the UI); when set, the `selection` must occur exactly ' + - 'once in the page. Reversible via the comment UI.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page to comment on.'), - content: z.string().describe('The comment body as Markdown.'), - selection: z - .string() - .min(1) - .max(250) - .optional() - .describe( - 'EXACT contiguous text from a SINGLE paragraph/block to anchor ' + - '(highlight) the comment on (<=250 chars, avoid spanning across ' + - 'formatting boundaries). Required for a new top-level comment; ' + - 'omit only when replying via parentCommentId.', - ), - parentCommentId: z - .string() - .optional() - .describe( - 'Optional id of a TOP-LEVEL comment to reply to (one level ' + - 'of replies only).', - ), - suggestedText: z - .string() - .min(1) - .max(2000) - .optional() - .describe( - 'Optional proposed replacement (PLAIN TEXT) for the `selection`, ' + - 'applied by a human via the UI (never auto-applied). REQUIRES a ' + - '`selection`; NOT allowed on a reply. When set, the `selection` ' + - 'must be UNIQUE in the page — expand it with surrounding context ' + - '(still <=250 chars) if it occurs more than once, or the call is ' + - 'refused.', - ), - }), - execute: async ({ + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + // This layer keeps only its own execute-side guards (require a selection + // for a top-level comment; reject suggestedText on a reply / without a + // selection) — the schema+description are shared. + createComment: sharedTool( + sharedToolSpecs.createComment, + async ({ pageId, content, selection, @@ -548,26 +502,17 @@ export class AiChatToolsService { const data = (result?.data ?? {}) as { id?: string }; return { commentId: data.id, pageId }; }, - }), + ), - resolveComment: tool({ - description: - 'Resolve or reopen a top-level comment thread (reversible — toggle ' + - 'the resolved flag). Only top-level comments can be resolved.', - inputSchema: modelFriendlyInput({ - commentId: z - .string() - .describe('The id of the top-level comment to resolve/reopen.'), - resolved: z - .boolean() - .describe('true to resolve the thread, false to reopen it.'), - }), - execute: async ({ commentId, resolved }) => { + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + resolveComment: sharedTool( + sharedToolSpecs.resolveComment, + async ({ commentId, resolved }) => { // resolveComment(commentId, resolved) -> { success, commentId, resolved }. await client.resolveComment(commentId, resolved); return { commentId, resolved }; }, - }), + ), // --- READ tools (added) --- @@ -673,24 +618,12 @@ export class AiChatToolsService { await client.getTable(pageId, tableRef), }), - listComments: tool({ - description: - 'List comments on a page in one call. By DEFAULT only ACTIVE ' + - 'threads are returned; resolved threads (a resolved top-level ' + - 'comment and all its replies) are hidden and their count reported ' + - 'as `resolvedThreadsHidden` so you can re-query with ' + - '`includeResolved: true` to see everything. Returns ' + - '`{ items, resolvedThreadsHidden }`. Content is returned as Markdown.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page.'), - includeResolved: z - .boolean() - .optional() - .describe('default only active threads; true — include resolved'), - }), - execute: async ({ pageId, includeResolved }) => + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + listComments: sharedTool( + sharedToolSpecs.listComments, + async ({ pageId, includeResolved }) => await client.listComments(pageId, includeResolved), - }), + ), getComment: tool({ description: 'Fetch a single comment by id (content as Markdown).', @@ -700,26 +633,12 @@ export class AiChatToolsService { execute: async ({ commentId }) => await client.getComment(commentId), }), - checkNewComments: tool({ - description: - 'Find new comments across a space (optionally scoped to a subtree) ' + - 'created after a given timestamp.', - inputSchema: modelFriendlyInput({ - spaceId: z.string().describe('The id of the space to scan.'), - since: z - .string() - .describe('An ISO-8601 timestamp; only comments created after it.'), - parentPageId: z - .string() - .optional() - .describe( - 'Optional page id to scope the scan to that page and its ' + - 'descendants.', - ), - }), - execute: async ({ spaceId, since, parentPageId }) => + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + checkNewComments: sharedTool( + sharedToolSpecs.checkNewComments, + async ({ spaceId, since, parentPageId }) => await client.checkNewComments(spaceId, since, parentPageId), - }), + ), listShares: sharedTool( sharedToolSpecs.listShares, diff --git a/apps/server/src/core/ai-chat/tools/tool-tiers.ts b/apps/server/src/core/ai-chat/tools/tool-tiers.ts index 4708416f..5a5f8cde 100644 --- a/apps/server/src/core/ai-chat/tools/tool-tiers.ts +++ b/apps/server/src/core/ai-chat/tools/tool-tiers.ts @@ -108,23 +108,14 @@ export const INLINE_TOOL_TIERS: Record< tier: 'core', catalogLine: "listPages — list recent pages, or a space's full page tree.", }, - listComments: { - tier: 'core', - catalogLine: 'listComments — list all comments on a page (including resolved).', - }, + // NOTE: createComment, listComments and resolveComment moved to + // @docmost/mcp's SHARED_TOOL_SPECS (#294); they carry their own tier + + // catalogLine there. getComment stays inline (MCP-only shape divergence is + // n/a — it simply has no shared spec). getComment: { tier: 'core', catalogLine: 'getComment — fetch a single comment by id.', }, - createComment: { - tier: 'core', - catalogLine: - 'createComment — add an inline comment (optionally with a suggested edit).', - }, - resolveComment: { - tier: 'core', - catalogLine: 'resolveComment — resolve or reopen a comment thread.', - }, // --- deferred inline --- createPage: { @@ -157,11 +148,8 @@ export const INLINE_TOOL_TIERS: Record< tier: 'deferred', catalogLine: 'getTable — read a table as a matrix of cell texts and cell ids.', }, - checkNewComments: { - tier: 'deferred', - catalogLine: - 'checkNewComments — find comments in a space created after a timestamp.', - }, + // NOTE: checkNewComments moved to @docmost/mcp's SHARED_TOOL_SPECS (#294); + // it carries its own deferred tier + catalogLine there. getPageHistory: { tier: 'deferred', catalogLine: diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index fe6d8ffd..ccb35005 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -721,26 +721,8 @@ server.registerTool( // --- Comment tools (ported from upstream PR #3 by Max Nikitin) --- // Tool: list_comments -server.registerTool( - "list_comments", - { - description: - "List comments on a page in one call (pagination is handled " + - "internally). By DEFAULT only ACTIVE threads are returned; resolved " + - "threads (a resolved top-level comment and all its replies) are hidden " + - "and their count reported as `resolvedThreadsHidden` so you can re-query " + - "with `includeResolved: true` to see everything. Returns " + - "`{ items, resolvedThreadsHidden }`. Content is returned as Markdown.", - inputSchema: { - pageId: z.string().describe("ID of the page"), - includeResolved: z - .boolean() - .optional() - .describe( - "default only active threads; true — include resolved", - ), - }, - }, +registerShared( + SHARED_TOOL_SPECS.listComments, async ({ pageId, includeResolved }) => { const comments = await docmostClient.listComments(pageId, includeResolved); return jsonContent(comments); @@ -748,55 +730,11 @@ 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", - { - 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 " + - "comments. Content is provided as Markdown and automatically converted. " + - "A top-level comment REQUIRES an exact `selection`; if the selection " + - "cannot be found in the page the call fails (no orphan comment is left). " + - "Replies (parentCommentId set) inherit the parent's anchor and take no " + - "selection. You may also attach a `suggestedText` proposing a replacement " + - "for the `selection`; a human applies (or rejects) it from the UI. When " + - "`suggestedText` is set the `selection` MUST occur exactly once in the " + - "page — expand it with surrounding context if it is ambiguous.", - inputSchema: { - pageId: z.string().describe("ID of the page to comment on"), - content: z.string().min(1).describe("Comment content in Markdown format"), - selection: z - .string() - .min(1) - // Enforce the documented 250-char cap to match the description above. - .max(250) - .optional() - .describe( - "EXACT contiguous text from a single paragraph/block to anchor the " + - "comment on (<=250 chars). Required for a top-level comment; omit " + - "only when replying via parentCommentId.", - ), - parentCommentId: z - .string() - .optional() - .describe("Parent comment ID to create a reply (max 2 nesting levels)"), - suggestedText: z - .string() - .min(1) - .max(2000) - .optional() - .describe( - "Optional proposed replacement (PLAIN TEXT) for the `selection`, " + - "applied by a human via the UI (never auto-applied). REQUIRES a " + - "`selection`; NOT allowed on a reply. When set, the `selection` must " + - "be UNIQUE in the page — expand it with surrounding context (still " + - "<=250 chars) if it occurs more than once, or the call is refused.", - ), - }, - }, +// Schema + description now live in the shared registry (#294). The execute body +// keeps this transport's own guards (require a selection for a top-level +// comment; reject suggestedText on a reply / without a selection). +registerShared( + SHARED_TOOL_SPECS.createComment, async ({ pageId, content, selection, parentCommentId, suggestedText }) => { if (!parentCommentId && (!selection || !selection.trim())) { throw new Error( @@ -872,28 +810,9 @@ server.registerTool( ); // Tool: resolve_comment -server.registerTool( - "resolve_comment", - { - description: - "Resolve (close) or reopen a comment thread. Only top-level comments can " + - "be resolved — the server rejects resolving a reply. Reversible: pass " + - "resolved=false to reopen. Resolving keeps the thread and its replies " + - "(unlike delete_comment, which permanently removes them).", - inputSchema: { - commentId: z - .string() - .min(1) - .describe("ID of the top-level comment thread to resolve or reopen"), - resolved: z - .boolean() - .optional() - .default(true) - .describe( - "true (default) marks the thread resolved/closed; false reopens it", - ), - }, - }, +// Schema + description now live in the shared registry (#294). +registerShared( + SHARED_TOOL_SPECS.resolveComment, async ({ commentId, resolved }) => { const result = await docmostClient.resolveComment(commentId, resolved); return jsonContent(result); @@ -901,30 +820,10 @@ server.registerTool( ); // Tool: check_new_comments -server.registerTool( - "check_new_comments", - { - description: - "Check for new comments across pages in a space since a given timestamp. " + - "Optionally scope to a page subtree (folder). Returns only comments " + - "created after the specified time.", - inputSchema: { - spaceId: z.string().describe("Space ID to check for new comments"), - since: z - .string() - .min(1) - .describe( - "ISO 8601 timestamp — only return comments created after this time (e.g. '2026-03-10T00:00:00Z')", - ), - parentPageId: z - .string() - .optional() - .describe( - "Optional root page ID to scope the check to a subtree (folder). " + - "Only pages under this parent will be checked.", - ), - }, - }, +// Schema + description now live in the shared registry (#294). The execute body +// keeps this transport's own guard rejecting an unparseable `since` timestamp. +registerShared( + SHARED_TOOL_SPECS.checkNewComments, async ({ spaceId, since, parentPageId }) => { // Reject an unparseable timestamp up front: otherwise the comparison // against NaN silently treats every comment as "not new" and the tool diff --git a/packages/mcp/src/tool-specs.ts b/packages/mcp/src/tool-specs.ts index 762e292b..a2c80e90 100644 --- a/packages/mcp/src/tool-specs.ts +++ b/packages/mcp/src/tool-specs.ts @@ -509,4 +509,175 @@ export const SHARED_TOOL_SPECS = { pageId: z.string().min(1), }), }, + + // --- comment tools (unified from the per-layer inline definitions, #294) --- + // + // create_comment and resolve_comment previously carried a "per-transport + // divergence" note in BOTH layers; #294 unifies their schema + description + // here. Only the four tools that genuinely exist in BOTH layers live in the + // registry: create/list/resolve comment and check_new_comments. + // + // update_comment and delete_comment are intentionally NOT here: they exist + // ONLY on the standalone MCP server. The in-app agent deliberately exposes no + // hard comment edit/delete tool (comment edits are irreversible / not + // version-tracked; see the guardrail tests in ai-chat-tools.service.spec.ts), + // so there is nothing to unify — they stay inline in index.ts. + + createComment: { + mcpName: 'create_comment', + inAppKey: 'createComment', + // CANONICAL: the in-app copy (the more-maintained one). It keeps the same + // rules as the MCP copy — inline-only, top-level requires a `selection`, no + // page-level comments, replies inherit the anchor, suggestedText must be + // unique — and adds the "retry with a corrected EXACT selection" and reply- + // to-reply-rejected guidance the MCP copy lacked. Execute-side validation + // (reject suggestedText on a reply, require a selection) stays per-layer. + description: + 'Add an INLINE comment to a page, or reply to an existing top-level ' + + 'comment (one level only — the backend rejects replies to replies). ' + + 'The comment is anchored inline to the given exact `selection` text ' + + '(which gets highlighted); page-level comments are NOT supported. A ' + + 'new top-level comment REQUIRES a `selection`. Replies inherit the ' + + "parent's anchor and take no selection. If the call fails with a " + + '"selection not found" error, retry with a corrected EXACT selection ' + + 'copied verbatim from a single paragraph/block. You may also attach a ' + + '`suggestedText` proposing a replacement for the `selection` (a human ' + + 'applies it from the UI); when set, the `selection` must occur exactly ' + + 'once in the page. Reversible via the comment UI.', + tier: 'core', + catalogLine: + 'createComment — add an inline comment (optionally with a suggested edit).', + // Reconciled schema: the field set is identical across both layers; the + // only constraint drift is `content`, which the MCP copy pinned to + // .min(1) while the in-app copy left unbounded — the stricter MCP form is + // kept (an empty comment body is never valid). + buildShape: (z) => ({ + pageId: z.string().describe('The id of the page to comment on.'), + content: z.string().min(1).describe('The comment body as Markdown.'), + selection: z + .string() + .min(1) + .max(250) + .optional() + .describe( + 'EXACT contiguous text from a SINGLE paragraph/block to anchor ' + + '(highlight) the comment on (<=250 chars, avoid spanning across ' + + 'formatting boundaries). Required for a new top-level comment; ' + + 'omit only when replying via parentCommentId.', + ), + parentCommentId: z + .string() + .optional() + .describe( + 'Optional id of a TOP-LEVEL comment to reply to (one level ' + + 'of replies only).', + ), + suggestedText: z + .string() + .min(1) + .max(2000) + .optional() + .describe( + 'Optional proposed replacement (PLAIN TEXT) for the `selection`, ' + + 'applied by a human via the UI (never auto-applied). REQUIRES a ' + + '`selection`; NOT allowed on a reply. When set, the `selection` ' + + 'must be UNIQUE in the page — expand it with surrounding context ' + + '(still <=250 chars) if it occurs more than once, or the call is ' + + 'refused.', + ), + }), + }, + + listComments: { + mcpName: 'list_comments', + inAppKey: 'listComments', + // CANONICAL: the two copies are near-identical; the MCP copy is the + // superset (it keeps the "(pagination is handled internally)" note the + // in-app copy dropped), so it is used verbatim. + description: + 'List comments on a page in one call (pagination is handled ' + + 'internally). By DEFAULT only ACTIVE threads are returned; resolved ' + + 'threads (a resolved top-level comment and all its replies) are hidden ' + + 'and their count reported as `resolvedThreadsHidden` so you can re-query ' + + 'with `includeResolved: true` to see everything. Returns ' + + '`{ items, resolvedThreadsHidden }`. Content is returned as Markdown.', + tier: 'core', + catalogLine: + 'listComments — list all comments on a page (including resolved).', + buildShape: (z) => ({ + pageId: z.string().describe('ID of the page'), + includeResolved: z + .boolean() + .optional() + .describe('default only active threads; true — include resolved'), + }), + }, + + resolveComment: { + mcpName: 'resolve_comment', + inAppKey: 'resolveComment', + // CANONICAL: the MCP copy's richer wording, minus its snake_case reference + // to `delete_comment` (a sibling tool that does NOT exist in the in-app + // layer) — rephrased transport-neutrally per the registry convention. + description: + 'Resolve (close) or reopen a top-level comment thread (reversible — ' + + 'pass resolved=false to reopen). Only top-level comments can be ' + + 'resolved; the server rejects resolving a reply. Resolving keeps the ' + + 'thread and its replies intact (it is not a deletion).', + tier: 'core', + catalogLine: 'resolveComment — resolve or reopen a comment thread.', + // Reconciled schema: `resolved` drifted — the MCP copy made it optional + // with .default(true) (resolve is the common case, documented), the in-app + // copy made it required. The MCP form is kept (a strict superset: it never + // rejects a previously-valid input and adds a sensible default), and + // commentId keeps the MCP copy's stricter .min(1). + buildShape: (z) => ({ + commentId: z + .string() + .min(1) + .describe('ID of the top-level comment thread to resolve or reopen'), + resolved: z + .boolean() + .optional() + .default(true) + .describe( + 'true (default) marks the thread resolved/closed; false reopens it', + ), + }), + }, + + checkNewComments: { + mcpName: 'check_new_comments', + inAppKey: 'checkNewComments', + // CANONICAL: the MCP copy (the more detailed of the two). The MCP layer's + // execute-side guard that rejects an unparseable `since` timestamp stays in + // its execute body (per-layer logic), not in the shared schema. + description: + 'Check for new comments across pages in a space since a given ' + + 'timestamp. Optionally scope to a page subtree (folder). Returns only ' + + 'comments created after the specified time.', + tier: 'deferred', + catalogLine: + 'checkNewComments — find comments in a space created after a timestamp.', + // Reconciled schema: `since` keeps the MCP copy's stricter .min(1) (the + // in-app copy left it unbounded); field descriptions use the MCP copy's + // more detailed wording (it carries an example timestamp). + buildShape: (z) => ({ + spaceId: z.string().describe('Space ID to check for new comments'), + since: z + .string() + .min(1) + .describe( + "ISO 8601 timestamp — only return comments created after this time " + + "(e.g. '2026-03-10T00:00:00Z')", + ), + parentPageId: z + .string() + .optional() + .describe( + 'Optional root page ID to scope the check to a subtree (folder). ' + + 'Only pages under this parent will be checked.', + ), + }), + }, } satisfies Record; From eebbe6717c3eed1ac2b63dc0ac415cce99769847 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 02:06:52 +0300 Subject: [PATCH 2/6] refactor(ai-chat): unify table row/cell tools into SHARED_TOOL_SPECS (#294, tables family) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrates the three-layer table WRITE tools into the transport-agnostic spec registry (schema + description declared once; each transport keeps only its execute/auth): - tableInsertRow, tableDeleteRow, tableUpdateCell -> SHARED_TOOL_SPECS; index.ts uses registerShared(), ai-chat uses sharedTool(); removed from INLINE_TOOL_TIERS (all three are deferred; not in CORE_TOOL_KEYS). Drift reconciled (documented inline): the four table tools previously carried a "NOT shared" note in both layers over a single parameter-NAME drift — the MCP layer named the table reference `table`, the in-app layer `tableRef`. Unified on the MCP name `table` (renaming the public MCP parameter would break external MCP clients; the in-app parameter is model-facing/prompt-only and safe to rename). The in-app execute bodies now destructure `table`. Descriptions took the MCP copy's richer wording (documents `#`, padding, header-row behavior) plus the in-app copy's "Reversible via page history" note; both fields keep the MCP copy's stricter .min(1) (in-app left them unbounded); sibling tool references phrased transport-neutrally. Intentionally NOT migrated (kept inline): table_get / getTable. Its MCP tool name is noun-first (`table_get`) while the in-app key is verb-first (`getTable`), which breaks the snake_case(inAppKey) naming convention the registry enforces (shared-tool-specs.contract.spec.ts). Renaming the public MCP tool would break external clients, so it stays per-transport — but its in-app reference param was still aligned to `table` (was `tableRef`) for consistency with the migrated trio. Gate: mcp tsc 0 + node --test 458/458 (page-search excluded — hangs only under the local re2->RegExp type-shim, its source is untouched), server jest 730 incl. tool-tiers catalog-partition + shared-spec contract parity, server tsc 0. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/tools/ai-chat-tools.service.ts | 90 ++++++------------- .../src/core/ai-chat/tools/tool-tiers.ts | 16 +--- packages/mcp/src/index.ts | 68 ++++---------- packages/mcp/src/tool-specs.ts | 90 +++++++++++++++++++ 4 files changed, 138 insertions(+), 126 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 21933219..81b1f450 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 @@ -601,21 +601,26 @@ export class AiChatToolsService { }), ), + // NOT shared (kept inline): the MCP tool name `table_get` is noun-first + // while this key is `getTable` (verb-first), breaking the + // snake_case(inAppKey) convention the shared registry enforces. Its + // reference parameter is still named `table` (was `tableRef`) so it matches + // the migrated table row/cell tools below. getTable: tool({ description: 'Read a table as a matrix of cell texts (plus a parallel cellIds ' + 'matrix so cells can be addressed for rich edits).', inputSchema: modelFriendlyInput({ pageId: z.string().describe('The id of the page.'), - tableRef: z + table: z .string() .describe( - '"#" from getOutline, or a block id of any node inside ' + - 'the table.', + '"#" from the page outline, or a block id of any node ' + + 'inside the table.', ), }), - execute: async ({ pageId, tableRef }) => - await client.getTable(pageId, tableRef), + execute: async ({ pageId, table }) => + await client.getTable(pageId, table), }), // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). @@ -766,64 +771,27 @@ 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 ' + - 'page history.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page.'), - tableRef: z - .string() - .describe('"#" from getOutline, or a block id in the table.'), - cells: z.array(z.string()).describe('The cell texts for the row.'), - index: z - .number() - .int() - .optional() - .describe('0-based insert position (omit/out-of-range to append).'), - }), - execute: async ({ pageId, tableRef, cells, index }) => - await client.tableInsertRow(pageId, tableRef, cells, index), - }), + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + // The table reference parameter was unified to `table` (was `tableRef`). + tableInsertRow: sharedTool( + sharedToolSpecs.tableInsertRow, + async ({ pageId, table, cells, index }) => + await client.tableInsertRow(pageId, table, 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.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page.'), - tableRef: z - .string() - .describe('"#" from getOutline, or a block id in the table.'), - index: z.number().int().describe('0-based row index to delete.'), - }), - execute: async ({ pageId, tableRef, index }) => - await client.tableDeleteRow(pageId, tableRef, index), - }), + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + tableDeleteRow: sharedTool( + sharedToolSpecs.tableDeleteRow, + async ({ pageId, table, index }) => + await client.tableDeleteRow(pageId, table, 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). ' + - 'Reversible via page history.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page.'), - tableRef: z - .string() - .describe('"#" from getOutline, or a block id in the table.'), - row: z.number().int().describe('0-based row index.'), - col: z.number().int().describe('0-based column index.'), - text: z.string().describe('The new cell text.'), - }), - execute: async ({ pageId, tableRef, row, col, text }) => - await client.tableUpdateCell(pageId, tableRef, row, col, text), - }), + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + tableUpdateCell: sharedTool( + sharedToolSpecs.tableUpdateCell, + async ({ pageId, table, row, col, text }) => + await client.tableUpdateCell(pageId, table, row, col, text), + ), copyPageContent: sharedTool( sharedToolSpecs.copyPageContent, diff --git a/apps/server/src/core/ai-chat/tools/tool-tiers.ts b/apps/server/src/core/ai-chat/tools/tool-tiers.ts index 5a5f8cde..e021f1af 100644 --- a/apps/server/src/core/ai-chat/tools/tool-tiers.ts +++ b/apps/server/src/core/ai-chat/tools/tool-tiers.ts @@ -148,6 +148,10 @@ export const INLINE_TOOL_TIERS: Record< tier: 'deferred', catalogLine: 'getTable — read a table as a matrix of cell texts and cell ids.', }, + // NOTE: tableInsertRow, tableDeleteRow and tableUpdateCell moved to + // @docmost/mcp's SHARED_TOOL_SPECS (#294); they carry their own deferred tier + + // catalogLine there. getTable stays inline (its MCP name table_get breaks the + // snake_case(inAppKey) convention, so it has no shared spec). // NOTE: checkNewComments moved to @docmost/mcp's SHARED_TOOL_SPECS (#294); // it carries its own deferred tier + catalogLine there. getPageHistory: { @@ -165,18 +169,6 @@ export const INLINE_TOOL_TIERS: Record< catalogLine: "updatePageJson — overwrite a page's body with a full ProseMirror document.", }, - tableInsertRow: { - tier: 'deferred', - catalogLine: 'tableInsertRow — insert a row of plain-text cells into a table.', - }, - tableDeleteRow: { - tier: 'deferred', - catalogLine: 'tableDeleteRow — delete a table row at a 0-based index.', - }, - tableUpdateCell: { - tier: 'deferred', - catalogLine: 'tableUpdateCell — set the text of a table cell at [row, col].', - }, sharePage: { tier: 'deferred', catalogLine: 'sharePage — make a page publicly accessible and return its URL.', diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index ccb35005..90137182 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -201,6 +201,11 @@ registerShared( ); // Tool: table_get +// Tool: table_get +// NOT in the shared registry: the MCP tool name `table_get` is noun-first while +// the in-app key is `getTable` (verb-first), breaking the snake_case(inAppKey) +// convention the shared registry enforces (shared-tool-specs.contract.spec.ts). +// Renaming the public MCP tool would break external clients, so it stays inline. server.registerTool( "table_get", { @@ -223,25 +228,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", - { - 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 " + - "column count; error if more cells than columns). `index` = 0-based " + - "insert position (0 inserts before the header); omit to append at the end.", - inputSchema: { - pageId: z.string().min(1), - table: z.string().min(1), - cells: z.array(z.string()), - index: z.number().int().optional(), - }, - }, +// Schema + description now live in the shared registry (#294); the `table` +// parameter name is the canonical one (the in-app layer was unified to it). +registerShared( + SHARED_TOOL_SPECS.tableInsertRow, async ({ pageId, table, cells, index }) => { const result = await docmostClient.tableInsertRow( pageId, @@ -254,22 +244,9 @@ 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", - { - 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 " + - "out-of-range `index` throws. Deleting `index` 0 removes the header row, " + - "and the next row becomes the new header.", - inputSchema: { - pageId: z.string().min(1), - table: z.string().min(1), - index: z.number().int(), - }, - }, +// Schema + description now live in the shared registry (#294). +registerShared( + SHARED_TOOL_SPECS.tableDeleteRow, async ({ pageId, table, index }) => { const result = await docmostClient.tableDeleteRow(pageId, table, index); return jsonContent(result); @@ -277,24 +254,9 @@ 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", - { - 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 " + - "content with a single text paragraph; for rich formatting use patch_node " + - "on the cell's paragraph id from table_get.", - inputSchema: { - pageId: z.string().min(1), - table: z.string().min(1), - row: z.number().int(), - col: z.number().int(), - text: z.string(), - }, - }, +// Schema + description now live in the shared registry (#294). +registerShared( + SHARED_TOOL_SPECS.tableUpdateCell, async ({ pageId, table, row, col, text }) => { const result = await docmostClient.tableUpdateCell( pageId, diff --git a/packages/mcp/src/tool-specs.ts b/packages/mcp/src/tool-specs.ts index a2c80e90..670e6f97 100644 --- a/packages/mcp/src/tool-specs.ts +++ b/packages/mcp/src/tool-specs.ts @@ -680,4 +680,94 @@ export const SHARED_TOOL_SPECS = { ), }), }, + + // --- table tools (unified from the per-layer inline definitions, #294) --- + // + // These tools carried a "NOT shared" note in BOTH layers because of a single + // parameter-NAME drift: the MCP layer named the table reference `table` while + // the in-app layer named it `tableRef`. #294 reconciles that drift by unifying + // on the MCP name `table` — renaming the MCP public parameter would break + // external MCP clients, whereas the in-app parameter is model-facing + // (prompt-only) and safe to rename. The in-app execute bodies now destructure + // `table` instead of `tableRef` (nothing else changes). Descriptions take the + // MCP copy's richer wording (it documented `#`, padding, header-row + // behavior) plus the in-app copy's "Reversible via page history" note; sibling + // tool references are phrased transport-neutrally. + // + // NOT here (kept inline in index.ts): table_get / getTable. Its MCP tool name + // is noun-first (`table_get`) while the in-app key is verb-first (`getTable`), + // so it breaks the snake_case(inAppKey) naming convention the registry enforces + // (shared-tool-specs.contract.spec.ts). Renaming the public MCP tool would + // break external clients, so it stays per-transport (its in-app param was still + // aligned to `table` for consistency with the migrated trio below). + + tableInsertRow: { + mcpName: 'table_insert_row', + inAppKey: 'tableInsertRow', + description: + 'Insert a row of plain-text cells into a table. `table` is `#` ' + + 'from the page outline, or a block id inside it. `cells` is the text per ' + + "column (padded to the table's column count; an error if more cells than " + + 'columns). `index` is the 0-based insert position (0 inserts before the ' + + 'header); omit to append at the end. Reversible via page history.', + tier: 'deferred', + catalogLine: 'tableInsertRow — insert a row of plain-text cells into a table.', + buildShape: (z) => ({ + pageId: z.string().min(1).describe('The id of the page.'), + table: z + .string() + .min(1) + .describe('"#" from the page outline, or a block id in the table.'), + cells: z.array(z.string()).describe('The cell texts for the row (one per column).'), + index: z + .number() + .int() + .optional() + .describe('0-based insert position (0 inserts before the header); omit to append.'), + }), + }, + + tableDeleteRow: { + mcpName: 'table_delete_row', + inAppKey: 'tableDeleteRow', + description: + 'Delete the row at 0-based `index` from a table (`table` is `#` ' + + 'from the page outline, or a block id inside it). Refuses to delete the ' + + "table's only row; an out-of-range `index` throws. Deleting `index` 0 " + + 'removes the header row, and the next row becomes the new header. ' + + 'Reversible via page history.', + tier: 'deferred', + catalogLine: 'tableDeleteRow — delete a table row at a 0-based index.', + buildShape: (z) => ({ + pageId: z.string().min(1).describe('The id of the page.'), + table: z + .string() + .min(1) + .describe('"#" from the page outline, or a block id in the table.'), + index: z.number().int().describe('0-based row index to delete.'), + }), + }, + + tableUpdateCell: { + mcpName: 'table_update_cell', + inAppKey: 'tableUpdateCell', + description: + 'Set the plain-text content of cell [row, col] (0-based) in a table ' + + '(`table` is `#` from the page outline, or a block id inside it). ' + + "Replaces the cell's content with a single text paragraph; for rich " + + "formatting, patch the cell's paragraph id (obtained from reading the " + + 'table) instead. Reversible via page history.', + tier: 'deferred', + catalogLine: 'tableUpdateCell — set the text of a table cell at [row, col].', + buildShape: (z) => ({ + pageId: z.string().min(1).describe('The id of the page.'), + table: z + .string() + .min(1) + .describe('"#" from the page outline, or a block id in the table.'), + row: z.number().int().describe('0-based row index.'), + col: z.number().int().describe('0-based column index.'), + text: z.string().describe('The new cell text.'), + }), + }, } satisfies Record; From 39735afd735608c2d7b4a5eb950f0a93e0dad8de Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 02:13:41 +0300 Subject: [PATCH 3/6] refactor(ai-chat): unify page tools into SHARED_TOOL_SPECS (#294, pages family) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrates the three-layer page tools into the transport-agnostic spec registry (schema + description declared once; each transport keeps only its execute/auth): - getPage, listPages (core), createPage, movePage, renamePage, deletePage, updatePageJson, exportPageMarkdown (deferred) -> SHARED_TOOL_SPECS; index.ts uses registerShared(), ai-chat uses sharedTool(); removed from INLINE_TOOL_TIERS. Tiers preserved from CORE_TOOL_KEYS (getPage/listPages = core, the rest deferred). delete_page is genuinely three-layer (in-app deletePage exists), so it IS migrated — not MCP-only. Its H4 guardrail is preserved: the shared schema exposes ONLY pageId, so no permanentlyDelete/forceDelete flag can reach the client (still asserted by ai-chat-tools.service.spec.ts). Descriptions merged (documented inline): each canonical text takes the MCP copy's richer structural notes plus the in-app copy's reversibility framing. Schema DRIFT reconciled (documented inline): - createPage.content: MCP pinned .min(1) but the in-app copy left it unbounded and DOCUMENTS an empty body as valid ("may be empty" — creating an empty page to fill later is a real use). Kept the looser no-min form: create_page now also accepts an empty body (harmless) and no previously-valid in-app input is rejected. title/spaceId keep the MCP .min(1) (empty is never valid). - movePage: MCP exposed an optional `position` (fractional-index) field the in-app copy lacked. Unified by KEEPING position — the in-app client already accepts an optional position arg, so the in-app execute now forwards it; optional, so no previously-valid call is rejected. `parentPageId` is nullable on both (real JSON null -> root); the MCP execute keeps its 'null'/'' string coercion as a per-layer robustness fallback. - getPage/renamePage/updatePageJson/exportPageMarkdown/listPages: kept the MCP copy's stricter .min(1) on ids where the in-app copy was unbounded. Per-transport execute logic preserved: getPage's {title,markdown} projection, updatePageJson's JSON-string normalization, list_pages' default limit/tree, and move_page's cycle guard + positive-confirmation check all stay in their execute bodies. Intentionally NOT touched: updatePageContent (Markdown-based body update; no MCP equivalent) and getTable (name-convention divergence, see tables family) stay inline. Gate: mcp build 0 + node --test 458/458 (page-search excluded — hangs only under the local re2->RegExp type-shim, its source untouched), server jest 770 incl. tool-tiers catalog-partition + shared-spec contract parity + deletePage H4 guardrail, server tsc 0. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/tools/ai-chat-tools.service.ts | 202 +++++------------ .../src/core/ai-chat/tools/tool-tiers.ts | 39 +--- packages/mcp/src/index.ts | 212 ++++-------------- packages/mcp/src/tool-specs.ts | 205 +++++++++++++++++ 4 files changed, 311 insertions(+), 347 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 81b1f450..d7548e15 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 @@ -316,50 +316,27 @@ export class AiChatToolsService { execute: async () => resolveCurrentPageResult(openedPage), }), - getPage: tool({ - description: - 'Fetch a single page as Markdown by its page id. Returns the page ' + - 'title and its Markdown content. Inline tags ' + - 'in the markdown are comment highlight anchors (also present for ' + - 'RESOLVED threads) — treat them as markup, not page text.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id (or slugId) of the page.'), - }), - execute: async ({ pageId }) => { - // getPage(pageId) -> { data: filterPage(page, markdown), success }. - const result = await client.getPage(pageId); - const data = (result?.data ?? {}) as { - title?: string; - content?: string; - }; - return { - title: data.title ?? '', - markdown: typeof data.content === 'string' ? data.content : '', - }; - }, + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + // The execute body keeps this layer's { title, markdown } projection. + getPage: sharedTool(sharedToolSpecs.getPage, async ({ pageId }) => { + // getPage(pageId) -> { data: filterPage(page, markdown), success }. + const result = await client.getPage(pageId); + const data = (result?.data ?? {}) as { + title?: string; + content?: string; + }; + return { + title: data.title ?? '', + markdown: typeof data.content === 'string' ? data.content : '', + }; }), // --- WRITE tools (all reversible — history/trash; §6.5 / D3) --- - createPage: tool({ - description: - 'Create a new page with a Markdown body in a space, optionally under ' + - 'a parent page. Returns the new page id and title. Reversible: a page ' + - 'can be moved to trash later.', - inputSchema: modelFriendlyInput({ - title: z.string().describe('The title of the new page.'), - content: z - .string() - .describe('The page body as Markdown (may be empty).'), - spaceId: z - .string() - .describe('The id of the space to create the page in.'), - parentPageId: z - .string() - .optional() - .describe('Optional parent page id to nest the new page under.'), - }), - execute: async ({ title, content, spaceId, parentPageId }) => { + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + createPage: sharedTool( + sharedToolSpecs.createPage, + async ({ title, content, spaceId, parentPageId }) => { // createPage(title, content, spaceId, parentPageId?) -> // { data: filterPage(page, markdown), success }. const result = await client.createPage( @@ -375,7 +352,7 @@ export class AiChatToolsService { }; return { id: data.id ?? data.slugId, title: data.title ?? title }; }, - }), + ), updatePageContent: tool({ description: @@ -399,60 +376,37 @@ export class AiChatToolsService { }, }), - renamePage: tool({ - description: - "Rename a page (change its title only; the body is untouched). " + - 'Reversible: rename back at any time.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page to rename.'), - title: z.string().describe('The new title.'), - }), - execute: async ({ pageId, title }) => { + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + renamePage: sharedTool( + sharedToolSpecs.renamePage, + async ({ pageId, title }) => { // renamePage(pageId, title) -> { success, pageId, title }. await client.renamePage(pageId, title); return { pageId, title }; }, - }), + ), - movePage: tool({ - description: - 'Move a page under a new parent page, or to the space root when no ' + - 'parent is given. Reversible: move it back at any time.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page to move.'), - parentPageId: z - .string() - .nullable() - .optional() - .describe( - 'Target parent page id. Null/omitted moves the page to the ' + - 'space root.', - ), - }), - execute: async ({ pageId, parentPageId }) => { + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + // The shared schema adds the optional `position` field this layer lacked + // before; the execute now forwards it (the client already accepted it). + movePage: sharedTool( + sharedToolSpecs.movePage, + async ({ pageId, parentPageId, position }) => { // movePage(pageId, parentPageId, position?) -> raw move response. - await client.movePage(pageId, parentPageId ?? null); + await client.movePage(pageId, parentPageId ?? null, position); return { pageId, parentPageId: parentPageId ?? null, moved: true }; }, - }), + ), - deletePage: tool({ - description: - 'Move a page to the trash (SOFT delete only — fully reversible; the ' + - 'page can be restored from trash). This NEVER permanently deletes.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page to move to trash.'), - }), - // GUARDRAIL (§14 H4): the only field ever passed to the client is - // pageId. permanentlyDelete/forceDelete are not part of the schema and - // are never forwarded, so the agent physically cannot permanently - // delete a page through this tool. - execute: async ({ pageId }) => { - // deletePage(pageId) hits POST /pages/delete with { pageId } only, - // which is the soft-delete (trash) path on the server. - await client.deletePage(pageId); - return { pageId, trashed: true }; - }, + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + // GUARDRAIL (§14 H4) preserved: the shared schema exposes ONLY pageId, so + // permanentlyDelete/forceDelete are never part of the input and can never + // be forwarded — the agent physically cannot permanently delete a page. + deletePage: sharedTool(sharedToolSpecs.deletePage, async ({ pageId }) => { + // deletePage(pageId) hits POST /pages/delete with { pageId } only, + // which is the soft-delete (trash) path on the server. + await client.deletePage(pageId); + return { pageId, trashed: true }; }), // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). @@ -530,33 +484,12 @@ export class AiChatToolsService { // 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. ' + - 'Returns a bounded list (default 50, max 100). Pass tree:true (with ' + - "spaceId) to instead get the space's full page hierarchy as a nested tree.", - inputSchema: modelFriendlyInput({ - spaceId: z - .string() - .optional() - .describe('Optional space id to scope the listing to.'), - limit: z - .number() - .int() - .min(1) - .max(100) - .optional() - .describe('Maximum number of pages (1-100).'), - tree: z - .boolean() - .optional() - .describe( - 'When true, return the full page hierarchy of the given space as a nested tree (children arrays) instead of the recent-pages flat list. Requires spaceId; ignores limit.', - ), - }), - execute: async ({ spaceId, limit, tree }) => + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + listPages: sharedTool( + sharedToolSpecs.listPages, + async ({ spaceId, limit, tree }) => await client.listPages(spaceId, limit, tree), - }), + ), listSidebarPages: tool({ description: @@ -673,19 +606,14 @@ export class AiChatToolsService { await client.diffPageVersions(pageId, from, to), ), - exportPageMarkdown: tool({ - description: - 'Export a page to a single self-contained Docmost-flavoured ' + - 'Markdown file (meta + body + comment threads). Lossless round-trip ' + - 'with importPageMarkdown.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page to export.'), - }), - execute: async ({ pageId }) => { + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + exportPageMarkdown: sharedTool( + sharedToolSpecs.exportPageMarkdown, + async ({ pageId }) => { const markdown = await client.exportPageMarkdown(pageId); return { markdown }; }, - }), + ), // --- WRITE tools (added; reversible via page history/trash) --- @@ -735,28 +663,12 @@ export class AiChatToolsService { async ({ pageId, nodeId }) => await client.deleteNode(pageId, nodeId), ), - updatePageJson: tool({ - description: - "Replace a page's body with a full ProseMirror document — a full " + - 'overwrite — and/or update its title. Minimal example content: ' + - '{"type":"doc","content":[{"type":"paragraph","content":' + - '[{"type":"text","text":"Hi"}]}]}. The content arg may be a JSON ' + - 'object or a JSON string (both accepted). Omit content for a ' + - 'title-only update. Reversible: the previous version is kept in page ' + - 'history.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page to update.'), - content: z - .any() - .optional() - .describe( - 'Full ProseMirror doc {"type":"doc","content":[...]} (JSON ' + - 'object or JSON string); omit for a title-only update.', - ), - title: z.string().optional().describe('Optional new title.'), - }), - execute: async ({ pageId, content, title }) => { - // Parity with the standalone MCP server (index.ts update_page_json): + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + // The execute body keeps this layer's content normalization (parity with + // the standalone MCP server, index.ts update_page_json). + updatePageJson: sharedTool( + sharedToolSpecs.updatePageJson, + async ({ pageId, content, title }) => { // undefined/null pass through as undefined (title-only / no-op); any // string is JSON.parsed (so an empty string "" throws, matching the // MCP server); an object is passed through unchanged. @@ -769,7 +681,7 @@ export class AiChatToolsService { } return await client.updatePageJson(pageId, doc, title); }, - }), + ), // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). // The table reference parameter was unified to `table` (was `tableRef`). diff --git a/apps/server/src/core/ai-chat/tools/tool-tiers.ts b/apps/server/src/core/ai-chat/tools/tool-tiers.ts index e021f1af..b0c13b2b 100644 --- a/apps/server/src/core/ai-chat/tools/tool-tiers.ts +++ b/apps/server/src/core/ai-chat/tools/tool-tiers.ts @@ -100,14 +100,8 @@ export const INLINE_TOOL_TIERS: Record< tier: 'core', catalogLine: 'getCurrentPage — the page the user is currently viewing.', }, - getPage: { - tier: 'core', - catalogLine: 'getPage — fetch a page as Markdown by its id.', - }, - listPages: { - tier: 'core', - catalogLine: "listPages — list recent pages, or a space's full page tree.", - }, + // NOTE: getPage and listPages moved to @docmost/mcp's SHARED_TOOL_SPECS + // (#294); they carry their own tier ('core') + catalogLine there. // NOTE: createComment, listComments and resolveComment moved to // @docmost/mcp's SHARED_TOOL_SPECS (#294); they carry their own tier + // catalogLine there. getComment stays inline (MCP-only shape divergence is @@ -118,27 +112,14 @@ export const INLINE_TOOL_TIERS: Record< }, // --- deferred inline --- - createPage: { - tier: 'deferred', - catalogLine: 'createPage — create a new page with a Markdown body in a space.', - }, + // NOTE: createPage, renamePage, movePage, deletePage, updatePageJson and + // exportPageMarkdown moved to @docmost/mcp's SHARED_TOOL_SPECS (#294); they + // carry their own deferred tier + catalogLine there. updatePageContent: { tier: 'deferred', catalogLine: "updatePageContent — replace a page's body (and optionally title) with new Markdown.", }, - renamePage: { - tier: 'deferred', - catalogLine: "renamePage — change a page's title only (body untouched).", - }, - movePage: { - tier: 'deferred', - catalogLine: 'movePage — move a page under a new parent or to the space root.', - }, - deletePage: { - tier: 'deferred', - catalogLine: 'deletePage — move a page to trash (soft delete, reversible).', - }, listSidebarPages: { tier: 'deferred', catalogLine: @@ -159,16 +140,6 @@ export const INLINE_TOOL_TIERS: Record< catalogLine: 'getPageHistory — fetch one page-history version with its ProseMirror content.', }, - exportPageMarkdown: { - tier: 'deferred', - catalogLine: - 'exportPageMarkdown — export a page to self-contained Markdown (body + comments).', - }, - updatePageJson: { - tier: 'deferred', - catalogLine: - "updatePageJson — overwrite a page's body with a full ProseMirror document.", - }, sharePage: { tier: 'deferred', catalogLine: 'sharePage — make a page publicly accessible and return its URL.', diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 90137182..4c798cf3 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -118,56 +118,19 @@ export function createDocmostMcpServer(config: DocmostMcpConfig): McpServer { // 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 " + - "in large spaces. Pass tree:true (with spaceId) to instead get the " + - "space's full page hierarchy as a nested tree.", - inputSchema: { - spaceId: z.string().optional(), - limit: z - .number() - .int() - .min(1) - .max(100) - .optional() - .describe("Max pages to return (default 50, max 100)"), - tree: z - .boolean() - .optional() - .describe( - "When true, return the space's full page hierarchy as a nested tree (each node has a children array) instead of the recent-by-updatedAt flat list. Requires spaceId; ignores limit.", - ), - }, - }, - async ({ spaceId, limit, tree }) => { - const result = await docmostClient.listPages(spaceId, limit ?? 50, tree ?? false); - return jsonContent(result); - }, -); +// Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). This +// transport keeps applying its own defaults (limit=50, tree=false) in execute. +registerShared(SHARED_TOOL_SPECS.listPages, async ({ spaceId, limit, tree }) => { + const result = await docmostClient.listPages(spaceId, limit ?? 50, tree ?? false); + return jsonContent(result); +}); // Tool: get_page -server.registerTool( - "get_page", - { - description: - "Get page details with content converted to Markdown. The conversion is " + - "LOSSY (block ids, exact table/callout structure are approximated); for a " + - "lossless representation use get_page_json. Inline " + - "tags in the markdown are comment highlight anchors (also present for " + - "RESOLVED threads) — treat them as markup, not page text.", - inputSchema: { - pageId: z.string().min(1), - }, - }, - async ({ pageId }) => { - const page = await docmostClient.getPage(pageId); - return jsonContent(page); - }, -); +// Schema + description now live in the shared registry (#294). +registerShared(SHARED_TOOL_SPECS.getPage, async ({ pageId }) => { + const page = await docmostClient.getPage(pageId); + return jsonContent(page); +}); // Tool: get_page_json registerShared(SHARED_TOOL_SPECS.getPageJson, async ({ pageId }) => { @@ -270,22 +233,9 @@ registerShared( ); // Tool: create_page -server.registerTool( - "create_page", - { - description: - "Create a new page from Markdown in a space. Pass parentPageId to nest " + - "it under a parent; omit it to create at the space root.", - inputSchema: { - title: z.string().min(1).describe("Title of the page"), - content: z.string().min(1).describe("Markdown content"), - spaceId: z.string().min(1), - parentPageId: z - .string() - .optional() - .describe("Optional parent page ID to nest under"), - }, - }, +// Schema + description now live in the shared registry (#294). +registerShared( + SHARED_TOOL_SPECS.createPage, async ({ title, content, spaceId, parentPageId }) => { const result = await docmostClient.createPage( title, @@ -298,32 +248,11 @@ server.registerTool( ); // Tool: update_page_json -server.registerTool( - "update_page_json", - { - description: - "Replace a page's content with a raw ProseMirror JSON document " + - "(lossless write: preserves the block ids, callouts, tables and " + - "attributes you pass in). Typical flow: get_page_json -> modify the " + - "JSON -> update_page_json. Keep existing node ids intact so heading " + - "anchors and history stay stable. Minimal full-doc example: " + - '{"type":"doc","content":[{"type":"paragraph","content":' + - '[{"type":"text","text":"Hi"}]}]}. `content` may be a JSON object or a ' + - "JSON string (both accepted), and is OPTIONAL: omit it to update only " + - "the title (though prefer rename_page for a title-only change). " + - "Supplying neither content nor title is an error.", - inputSchema: { - pageId: z.string().min(1).describe("ID of the page to update"), - content: z - .any() - .optional() - .describe( - 'ProseMirror document {"type":"doc","content":[...]} (JSON object or ' + - "JSON string). Omit to rename only.", - ), - title: z.string().optional().describe("Optional new title"), - }, - }, +// Schema + description now live in the shared registry (#294). The execute body +// keeps this transport's content normalization (parse a JSON-string content, +// pass undefined/null through for a title-only/no-op update). +registerShared( + SHARED_TOOL_SPECS.updatePageJson, async ({ pageId, content, title }) => { // Only parse/validate the document when it was actually supplied; when it // is omitted, pass it straight through so the client performs a title-only @@ -341,26 +270,11 @@ server.registerTool( ); // Tool: export_page_markdown -server.registerTool( - "export_page_markdown", - { - description: - "Export a page to a single self-contained, lossless Docmost-flavoured " + - "Markdown file (custom extensions): YAML-free meta header, body with " + - "inline comment anchors and diagrams, and a trailing comments-thread " + - "block. Designed for a download -> edit body -> import_page_markdown " + - "round-trip that preserves everything, including comment highlights. " + - "Comment THREADS are preserved in the file but are not re-pushed to the " + - "server on import.", - inputSchema: { - pageId: z.string().min(1), - }, - }, - async ({ pageId }) => { - const md = await docmostClient.exportPageMarkdown(pageId); - return { content: [{ type: "text" as const, text: md }] }; - }, -); +// Schema + description now live in the shared registry (#294). +registerShared(SHARED_TOOL_SPECS.exportPageMarkdown, async ({ pageId }) => { + const md = await docmostClient.exportPageMarkdown(pageId); + return { content: [{ type: "text" as const, text: md }] }; +}); // Tool: import_page_markdown registerShared( @@ -384,22 +298,11 @@ registerShared( ); // Tool: rename_page -server.registerTool( - "rename_page", - { - description: - "Rename a page (change its title only) without touching or resending " + - "its content.", - inputSchema: { - pageId: z.string().min(1).describe("ID of the page to rename"), - title: z.string().min(1).describe("New title"), - }, - }, - async ({ pageId, title }) => { - const result = await docmostClient.renamePage(pageId, title); - return jsonContent(result); - }, -); +// Schema + description now live in the shared registry (#294). +registerShared(SHARED_TOOL_SPECS.renamePage, async ({ pageId, title }) => { + const result = await docmostClient.renamePage(pageId, title); + return jsonContent(result); +}); // Tool: edit_page_text registerShared(SHARED_TOOL_SPECS.editPageText, async ({ pageId, edits }) => { @@ -603,29 +506,11 @@ registerShared(SHARED_TOOL_SPECS.listShares, async () => { }); // Tool: move_page -server.registerTool( - "move_page", - { - description: - "Move a page under a new parent (nesting) or to the space root.", - inputSchema: { - pageId: z.string().min(1), - parentPageId: z - .string() - .nullable() - .optional() - .describe( - "Target parent page ID. Pass 'null' or empty string to move to root.", - ), - position: z - .string() - .min(5) - .optional() - .describe( - "fractional-index position key; min 5 chars; omit to append at the end.", - ), - }, - }, +// Schema + description now live in the shared registry (#294). The execute body +// keeps this transport's cycle guard, its 'null'/'' -> null string coercion, and +// its positive-confirmation check on the move response. +registerShared( + SHARED_TOOL_SPECS.movePage, async ({ pageId, parentPageId, position }) => { const finalParentId = parentPageId === "" || parentPageId === "null" ? null : parentPageId; @@ -660,25 +545,16 @@ server.registerTool( ); // Tool: delete_page -server.registerTool( - "delete_page", - { - description: - "Delete a single page by ID. SOFT delete only: the page is moved to " + - "trash and can be restored; nothing is permanently deleted.", - inputSchema: { - pageId: z.string().min(1), - }, - }, - async ({ pageId }) => { - await docmostClient.deletePage(pageId); - return { - content: [ - { type: "text" as const, text: `Successfully deleted page ${pageId}` }, - ], - }; - }, -); +// Schema + description now live in the shared registry (#294). The shared schema +// exposes ONLY pageId, so no permanent/force-delete flag can reach the client. +registerShared(SHARED_TOOL_SPECS.deletePage, async ({ pageId }) => { + await docmostClient.deletePage(pageId); + return { + content: [ + { type: "text" as const, text: `Successfully deleted page ${pageId}` }, + ], + }; +}); // --- Comment tools (ported from upstream PR #3 by Max Nikitin) --- diff --git a/packages/mcp/src/tool-specs.ts b/packages/mcp/src/tool-specs.ts index 670e6f97..43eee4fd 100644 --- a/packages/mcp/src/tool-specs.ts +++ b/packages/mcp/src/tool-specs.ts @@ -510,6 +510,211 @@ export const SHARED_TOOL_SPECS = { }), }, + // --- page tools (unified from the per-layer inline definitions, #294) --- + // + // Descriptions merge both layers (the MCP copy's richer structural notes + the + // in-app copy's "Reversible via history/trash" framing where it added one). + // Field constraints keep the MCP copy's stricter .min(1) EXCEPT where the + // in-app layer deliberately allowed a looser value (documented per field). + + getPage: { + mcpName: 'get_page', + inAppKey: 'getPage', + description: + 'Fetch a single page as Markdown by its id. Returns the page title and ' + + 'its Markdown content. The Markdown conversion is LOSSY (block ids, exact ' + + 'table/callout structure are approximated); for a lossless representation ' + + 'use get_page_json. Inline tags in the markdown ' + + 'are comment highlight anchors (also present for RESOLVED threads) — ' + + 'treat them as markup, not page text.', + tier: 'core', + catalogLine: 'getPage — fetch a page as Markdown by its id.', + // Reconciled: MCP's stricter .min(1) kept; in-app's more-informative + // "(or slugId)" describe kept. + buildShape: (z) => ({ + pageId: z.string().min(1).describe('The id (or slugId) of the page.'), + }), + }, + + listPages: { + mcpName: 'list_pages', + inAppKey: 'listPages', + description: + 'List the most recent pages (ordered by updatedAt, descending), ' + + 'optionally scoped to a single space. Returns a bounded list (default ' + + '50, max 100) — use search for lookups in large spaces. Pass tree:true ' + + "(with spaceId) to instead get the space's full page hierarchy as a " + + 'nested tree.', + tier: 'core', + catalogLine: "listPages — list recent pages, or a space's full page tree.", + buildShape: (z) => ({ + spaceId: z + .string() + .optional() + .describe('Optional space id to scope the listing to.'), + limit: z + .number() + .int() + .min(1) + .max(100) + .optional() + .describe('Maximum number of pages (default 50, max 100).'), + tree: z + .boolean() + .optional() + .describe( + "When true, return the space's full page hierarchy as a nested tree " + + '(children arrays) instead of the recent-by-updatedAt flat list. ' + + 'Requires spaceId; ignores limit.', + ), + }), + }, + + createPage: { + mcpName: 'create_page', + inAppKey: 'createPage', + description: + 'Create a new page with a Markdown body in a space, optionally under a ' + + 'parent page (omit parentPageId to create at the space root). Returns ' + + 'the new page id and title. Reversible: a page can be moved to trash ' + + 'later.', + tier: 'deferred', + catalogLine: 'createPage — create a new page with a Markdown body in a space.', + // Reconciled schema DRIFT: the MCP copy pinned `content` to .min(1) while + // the in-app copy left it unbounded and DOCUMENTS an empty body as valid + // ("may be empty") — creating an empty page to fill in later is a real use + // case. The looser (no-min) form is kept, so create_page now also accepts an + // empty body (harmless — it creates an empty page) and no previously-valid + // in-app input is ever rejected. `title`/`spaceId` keep the MCP .min(1) + // (an empty title or space is never valid). + buildShape: (z) => ({ + title: z.string().min(1).describe('The title of the new page.'), + content: z.string().describe('The page body as Markdown (may be empty).'), + spaceId: z.string().min(1).describe('The id of the space to create the page in.'), + parentPageId: z + .string() + .optional() + .describe('Optional parent page id to nest the new page under.'), + }), + }, + + movePage: { + mcpName: 'move_page', + inAppKey: 'movePage', + description: + 'Move a page under a new parent page, or to the space root when no ' + + 'parent is given. Reversible: move it back at any time.', + tier: 'deferred', + catalogLine: 'movePage — move a page under a new parent or to the space root.', + // Reconciled schema DRIFT: the MCP copy exposed a `position` field + // (fractional-index ordering) that the in-app copy lacked. Unified by + // KEEPING position (the in-app client already accepts an optional position + // arg, so the in-app execute now forwards it) — it is optional, so no + // previously-valid in-app call is rejected. `parentPageId` is `.nullable()` + // on both, so a real JSON null moves to root on either transport; the MCP + // execute additionally coerces the strings 'null'/'' to null as a robustness + // fallback (kept in its execute body, not in the shared schema). + buildShape: (z) => ({ + pageId: z.string().min(1).describe('The id of the page to move.'), + parentPageId: z + .string() + .nullable() + .optional() + .describe( + 'Target parent page id. Null or omitted moves the page to the space ' + + 'root.', + ), + position: z + .string() + .min(5) + .optional() + .describe( + 'Optional fractional-index position key (min 5 chars); omit to ' + + 'append at the end.', + ), + }), + }, + + renamePage: { + mcpName: 'rename_page', + inAppKey: 'renamePage', + description: + 'Rename a page (change its title only; the body is untouched, never ' + + 'resent). Reversible: rename back at any time.', + tier: 'deferred', + catalogLine: "renamePage — change a page's title only (body untouched).", + buildShape: (z) => ({ + pageId: z.string().min(1).describe('The id of the page to rename.'), + title: z.string().min(1).describe('The new title.'), + }), + }, + + deletePage: { + mcpName: 'delete_page', + inAppKey: 'deletePage', + description: + 'Move a page to the trash — SOFT delete only: the page can be restored ' + + 'from trash and nothing is ever permanently deleted.', + tier: 'deferred', + catalogLine: 'deletePage — move a page to trash (soft delete, reversible).', + // GUARDRAIL preserved (§14 H4): the schema exposes ONLY pageId, so a + // permanentlyDelete/forceDelete flag can never reach the client through this + // tool (asserted by ai-chat-tools.service.spec.ts). + buildShape: (z) => ({ + pageId: z.string().min(1).describe('The id of the page to move to trash.'), + }), + }, + + updatePageJson: { + mcpName: 'update_page_json', + inAppKey: 'updatePageJson', + description: + "Replace a page's content with a raw ProseMirror JSON document (lossless " + + 'write: preserves the block ids, callouts, tables and attributes you pass ' + + 'in). Typical flow: get_page_json -> modify the JSON -> update_page_json. ' + + 'Keep existing node ids intact so heading anchors and history stay ' + + 'stable. Minimal full-doc example: {"type":"doc","content":[{"type":' + + '"paragraph","content":[{"type":"text","text":"Hi"}]}]}. `content` may be ' + + 'a JSON object or a JSON string (both accepted), and is OPTIONAL: omit it ' + + 'to update only the title (though prefer rename_page for a title-only ' + + 'change). Supplying neither content nor title is an error. Reversible: ' + + 'the previous version is kept in page history.', + tier: 'deferred', + catalogLine: + "updatePageJson — overwrite a page's body with a full ProseMirror document.", + buildShape: (z) => ({ + pageId: z.string().min(1).describe('ID of the page to update'), + content: z + .any() + .optional() + .describe( + 'ProseMirror document {"type":"doc","content":[...]} (JSON object or ' + + 'JSON string). Omit to update only the title.', + ), + title: z.string().optional().describe('Optional new title'), + }), + }, + + exportPageMarkdown: { + mcpName: 'export_page_markdown', + inAppKey: 'exportPageMarkdown', + // CANONICAL: the MCP copy (a strict superset of the terse in-app wording). + description: + 'Export a page to a single self-contained, lossless Docmost-flavoured ' + + 'Markdown file (custom extensions): YAML-free meta header, body with ' + + 'inline comment anchors and diagrams, and a trailing comments-thread ' + + 'block. Designed for a download -> edit body -> import_page_markdown ' + + 'round-trip that preserves everything, including comment highlights. ' + + 'Comment THREADS are preserved in the file but are not re-pushed to the ' + + 'server on import.', + tier: 'deferred', + catalogLine: + 'exportPageMarkdown — export a page to self-contained Markdown (body + comments).', + buildShape: (z) => ({ + pageId: z.string().min(1).describe('The id of the page to export.'), + }), + }, + // --- comment tools (unified from the per-layer inline definitions, #294) --- // // create_comment and resolve_comment previously carried a "per-transport From b51dae16a6257ee1a8b8af4a8d07ed2c6fafe239 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 02:14:39 +0300 Subject: [PATCH 4/6] docs(mcp): mark media tools MCP-only in index.ts (#294, media family) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The media tools — insert_image, replace_image, insert_footnote — are MCP-only by design: the in-app AI-chat agent exposes no image or footnote tools, so there is no second layer to unify into SHARED_TOOL_SPECS. A registry spec's tier/catalogLine are in-app metadata and the catalog-partition test forbids a spec without a live in-app tool, so forcing them into the registry would break the invariant. They stay per-transport (inline in index.ts). No behavior change — documentation only (adds the rationale above each tool so a future migrator does not re-investigate why these are not shared). Gate: mcp tsc 0 (comment-only change). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/index.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 4c798cf3..fb373415 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -381,6 +381,10 @@ registerShared(SHARED_TOOL_SPECS.deleteNode, async ({ pageId, nodeId }) => { }); // Tool: insert_image +// MCP-only by design (NOT in the shared registry): the in-app AI-chat agent +// exposes no image tools (insert/replace), so there is no second layer to unify +// — a SHARED_TOOL_SPECS entry's tier/catalogLine are in-app metadata and the +// catalog-partition test forbids a spec without a live in-app tool (#294). server.registerTool( "insert_image", { @@ -426,6 +430,7 @@ server.registerTool( ); // Tool: replace_image +// MCP-only by design (see insert_image): no in-app equivalent, stays inline. server.registerTool( "replace_image", { @@ -790,6 +795,8 @@ server.registerTool( ); // Tool: insert_footnote +// MCP-only by design (see insert_image): the in-app AI-chat agent exposes no +// footnote tool, so there is no second layer to unify — stays inline (#294). server.registerTool( "insert_footnote", { From ce70fab1dfde5c052c1e008eaa526f512939427f Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 02:17:14 +0300 Subject: [PATCH 5/6] refactor(ai-chat): unify share_page into SHARED_TOOL_SPECS (#294, misc family) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrates share_page / sharePage into the transport-agnostic spec registry (schema + description declared once; each transport keeps only its execute/auth): - sharePage (deferred) -> SHARED_TOOL_SPECS; index.ts uses registerShared(), ai-chat uses sharedTool(); removed from INLINE_TOOL_TIERS. Drift reconciled (documented inline): both inline copies already carried the "only share when the user explicitly asked" security framing, so the old "per-transport divergence" note in BOTH layers was STALE — there was no real behavioral divergence, only wording drift. The canonical description merges the MCP copy's URL-format + idempotency detail with the in-app copy's reversibility note and keeps the shared security framing. pageId keeps the MCP copy's stricter .min(1). The MCP execute keeps its own `searchIndexing ?? true` default (per-layer, not part of the shared schema). Intentionally NOT migrated (kept inline — genuinely divergent, as their existing notes state): - search / searchPages: the in-app tool is a semantic+keyword hybrid (RRF) with in-process access control and a tuned schema (limit 1-20); the MCP `search` is a plain REST full-text search (limit up to 100). Different behavior AND schema. - docmost_transform / transformPage: the in-app tool deliberately omits the `deleteComments` schema field (a comment-deletion guardrail) and carries a shorter description. Different schema. Gate: mcp build 0 + node --test 458/458 (page-search excluded — hangs only under the local re2->RegExp type-shim, its source untouched), server jest 775 incl. tool-tiers catalog-partition + shared-spec contract parity, server tsc 0. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/tools/ai-chat-tools.service.ts | 25 +++++------------ .../src/core/ai-chat/tools/tool-tiers.ts | 8 +++--- packages/mcp/src/index.ts | 24 +++------------- packages/mcp/src/tool-specs.ts | 28 +++++++++++++++++++ 4 files changed, 43 insertions(+), 42 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 d7548e15..d543390e 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 @@ -717,25 +717,14 @@ 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. ' + - 'Reversible via unsharePage. Only share when the user explicitly ' + - 'asked, since this exposes the page to anyone with the link.', - inputSchema: modelFriendlyInput({ - pageId: z.string().describe('The id of the page to share.'), - searchIndexing: z - .boolean() - .optional() - .describe('Allow public search engines to index it (default true).'), - }), - execute: async ({ pageId, searchIndexing }) => + // Schema + description now live in @docmost/mcp's SHARED_TOOL_SPECS (#294). + // Both layers already carried the security-confirmation framing, so there + // was no real divergence to preserve — only wording drift. + sharePage: sharedTool( + sharedToolSpecs.sharePage, + async ({ pageId, searchIndexing }) => await client.sharePage(pageId, searchIndexing), - }), + ), unsharePage: sharedTool( sharedToolSpecs.unsharePage, diff --git a/apps/server/src/core/ai-chat/tools/tool-tiers.ts b/apps/server/src/core/ai-chat/tools/tool-tiers.ts index b0c13b2b..7d26498c 100644 --- a/apps/server/src/core/ai-chat/tools/tool-tiers.ts +++ b/apps/server/src/core/ai-chat/tools/tool-tiers.ts @@ -140,10 +140,10 @@ export const INLINE_TOOL_TIERS: Record< catalogLine: 'getPageHistory — fetch one page-history version with its ProseMirror content.', }, - sharePage: { - tier: 'deferred', - catalogLine: 'sharePage — make a page publicly accessible and return its URL.', - }, + // NOTE: sharePage moved to @docmost/mcp's SHARED_TOOL_SPECS (#294); it carries + // its own deferred tier + catalogLine there. transformPage stays inline (its + // schema deliberately diverges — it omits the deleteComments field the MCP + // docmost_transform exposes, a comment-deletion guardrail). transformPage: { tier: 'deferred', catalogLine: "transformPage — run a sandboxed JS transform over a page's document.", diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index fb373415..771c3e6f 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -163,7 +163,6 @@ registerShared( }, ); -// Tool: table_get // Tool: table_get // NOT in the shared registry: the MCP tool name `table_get` is noun-first while // the in-app key is `getTable` (verb-first), breaking the snake_case(inAppKey) @@ -473,25 +472,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", - { - description: - "Make a page publicly accessible (idempotent) and return its public " + - "URL. The URL format is /share//p/. This exposes the " + - "page content to ANYONE with the URL — do it only when explicitly asked.", - inputSchema: { - pageId: z.string().min(1).describe("ID of the page to share"), - searchIndexing: z - .boolean() - .optional() - .describe("Allow search engines to index the page (default true)"), - }, - }, +// Schema + description now live in the shared registry (#294). The execute body +// keeps this transport's own `searchIndexing ?? true` default. +registerShared( + SHARED_TOOL_SPECS.sharePage, async ({ pageId, searchIndexing }) => { const result = await docmostClient.sharePage(pageId, searchIndexing ?? true); return jsonContent(result); diff --git a/packages/mcp/src/tool-specs.ts b/packages/mcp/src/tool-specs.ts index 43eee4fd..a52f7953 100644 --- a/packages/mcp/src/tool-specs.ts +++ b/packages/mcp/src/tool-specs.ts @@ -316,6 +316,34 @@ export const SHARED_TOOL_SPECS = { // --- share management --- + // Unified from the per-layer inline definitions (#294). Both layers already + // carried the "only share when explicitly asked" security framing (the + // "per-transport divergence" note on the old inline copies was stale), so + // there was no real behavioral divergence to preserve — only wording drift. + sharePage: { + mcpName: 'share_page', + inAppKey: 'sharePage', + // CANONICAL: merges the MCP copy's URL-format + idempotency detail with the + // in-app copy's reversibility note; keeps the security framing both had. + description: + 'Make a page PUBLICLY accessible (idempotent) and return its public URL ' + + '(format: /share//p/). This exposes the page content ' + + 'to ANYONE with the URL — only share when the user explicitly asked. ' + + 'Reversible: unshare it later to revoke the public URL.', + tier: 'deferred', + catalogLine: 'sharePage — make a page publicly accessible and return its URL.', + // Reconciled: MCP's stricter .min(1) on pageId kept; field descriptions from + // the in-app copy. The MCP execute keeps its own `searchIndexing ?? true` + // default (a per-layer concern, not part of the shared schema). + buildShape: (z) => ({ + pageId: z.string().min(1).describe('The id of the page to share.'), + searchIndexing: z + .boolean() + .optional() + .describe('Allow public search engines to index it (default true).'), + }), + }, + unsharePage: { mcpName: 'unshare_page', inAppKey: 'unsharePage', From 36b940fdb811599d758f367c40afc70227242611 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 03:10:42 +0300 Subject: [PATCH 6/6] fix(#294 review F1-F2): test the changed execute wirings + transport-neutral descriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - F1: added in-app execute tests for the two wirings that ACTUALLY changed in the migration (the contract-parity test only checks advertised schema keys, not execute bodies): movePage forwards the newly-added optional `position` to client.movePage (and passes undefined position + null parent when omitted); the table trio (insert/delete/updateCell) forwards the unified `table` param positionally. A field destructured under the wrong name would have silently passed undefined to the client (execute is any-cast, tsc won't catch it). - F2: rewrote the three migrated descriptions that hardcoded snake_case sibling tool names (which the in-app camelCase layer exposes under different ids, violating the registry's own transport-neutral-prose convention) into neutral prose: getPage "use get_page_json" -> "use the lossless page-JSON read tool"; updatePageJson "get_page_json -> ... -> update_page_json" -> "read the page-JSON view -> modify -> write it back", "prefer rename_page" -> "prefer the rename-page tool"; exportPageMarkdown "import_page_markdown round-trip" -> "page-Markdown import round-trip" (the last was a direct regress — the in-app base said the camelCase importPageMarkdown). (stashPage's pre-existing get_page_json mention is out of scope, per the reviewer.) Gate: mcp build 0; ai-chat-tools.service + tool-tiers (catalog-partition) pass, incl. the 5 new execute-wiring tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tools/ai-chat-tools.service.spec.ts | 112 ++++++++++++++++++ packages/mcp/src/tool-specs.ts | 8 +- 2 files changed, 116 insertions(+), 4 deletions(-) diff --git a/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.spec.ts b/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.spec.ts index 53d38895..7e4b35d6 100644 --- a/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.spec.ts +++ b/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.spec.ts @@ -539,3 +539,115 @@ describe('AiChatToolsService model-friendly input validation (#190)', () => { expect(result.error?.message).toContain('parameter "pageId": missing (required)'); }); }); + +/** + * #294 F1 — the contract-parity test introspects only the ADVERTISED schema keys + * (buildShape), not the execute bodies. Most execs are unchanged pass-throughs, + * but two wirings actually CHANGED in the migration and are otherwise untested: + * - movePage now forwards the newly-added optional `position` field to the + * client (client.movePage(pageId, parentPageId, position)); + * - the table trio unified its `tableRef` param to `table` and must forward it + * positionally. A field destructured under the wrong name would silently pass + * `undefined` to the client (execute is `any`-cast, so tsc won't catch it). + */ +describe('AiChatToolsService #294 changed execute wirings', () => { + const calls: Record = { + movePage: [], + tableInsertRow: [], + tableDeleteRow: [], + tableUpdateCell: [], + }; + const fakeClient: Partial = { + movePage: (...args: unknown[]) => { + calls.movePage.push(args); + return Promise.resolve({ success: true }); + }, + tableInsertRow: (...args: unknown[]) => { + calls.tableInsertRow.push(args); + return Promise.resolve({ ok: true }); + }, + tableDeleteRow: (...args: unknown[]) => { + calls.tableDeleteRow.push(args); + return Promise.resolve({ ok: true }); + }, + tableUpdateCell: (...args: unknown[]) => { + calls.tableUpdateCell.push(args); + return Promise.resolve({ ok: true }); + }, + }; + const tokenServiceStub = { + generateAccessToken: jest.fn().mockResolvedValue('access-token'), + generateCollabToken: jest.fn().mockResolvedValue('collab-token'), + }; + let service: AiChatToolsService; + + beforeEach(() => { + for (const k of Object.keys(calls)) calls[k].length = 0; + jest.spyOn(loader, 'loadDocmostMcp').mockResolvedValue( + mockLoaded(function () { + return fakeClient as DocmostClientLike; + } as unknown as loader.DocmostClientCtor), + ); + service = new AiChatToolsService( + tokenServiceStub as never, + {} as never, + {} as never, + {} as never, + {} as never, + { + asSink: () => ({ put: jest.fn(), has: jest.fn(), evict: jest.fn() }), + } as never, + ); + }); + afterEach(() => jest.restoreAllMocks()); + + const buildTools = () => + service.forUser( + { id: 'user-1', email: 'u@example.com', workspaceId: 'ws-1' } as never, + 'session-1', + 'ws-1', + 'chat-1', + ); + + it('movePage forwards the optional position to the client', async () => { + const tools = await buildTools(); + await tools.movePage.execute( + { pageId: 'p1', parentPageId: 'parent1', position: 'a5' } as never, + {} as never, + ); + expect(calls.movePage).toEqual([['p1', 'parent1', 'a5']]); + }); + + it('movePage passes undefined position and null parent when omitted (unchanged behavior)', async () => { + const tools = await buildTools(); + await tools.movePage.execute({ pageId: 'p2' } as never, {} as never); + expect(calls.movePage).toEqual([['p2', null, undefined]]); + }); + + it('tableInsertRow forwards the unified `table` param positionally', async () => { + const tools = await buildTools(); + await tools.tableInsertRow.execute( + { pageId: 'p1', table: '#0', cells: ['a', 'b'], index: 2 } as never, + {} as never, + ); + expect(calls.tableInsertRow).toEqual([['p1', '#0', ['a', 'b'], 2]]); + }); + + it('tableDeleteRow forwards `table` positionally', async () => { + const tools = await buildTools(); + await tools.tableDeleteRow.execute( + { pageId: 'p1', table: '#0', index: 1 } as never, + {} as never, + ); + expect(calls.tableDeleteRow).toEqual([['p1', '#0', 1]]); + }); + + it('tableUpdateCell forwards `table` positionally', async () => { + const tools = await buildTools(); + await tools.tableUpdateCell.execute( + { pageId: 'p1', table: '#0', row: 1, col: 2, text: 'x' } as never, + {} as never, + ); + expect(calls.tableUpdateCell).toEqual([['p1', '#0', 1, 2, 'x']]); + }); +}); diff --git a/packages/mcp/src/tool-specs.ts b/packages/mcp/src/tool-specs.ts index a52f7953..054c1249 100644 --- a/packages/mcp/src/tool-specs.ts +++ b/packages/mcp/src/tool-specs.ts @@ -552,7 +552,7 @@ export const SHARED_TOOL_SPECS = { 'Fetch a single page as Markdown by its id. Returns the page title and ' + 'its Markdown content. The Markdown conversion is LOSSY (block ids, exact ' + 'table/callout structure are approximated); for a lossless representation ' + - 'use get_page_json. Inline tags in the markdown ' + + 'use the lossless page-JSON read tool. Inline tags in the markdown ' + 'are comment highlight anchors (also present for RESOLVED threads) — ' + 'treat them as markup, not page text.', tier: 'core', @@ -699,12 +699,12 @@ export const SHARED_TOOL_SPECS = { description: "Replace a page's content with a raw ProseMirror JSON document (lossless " + 'write: preserves the block ids, callouts, tables and attributes you pass ' + - 'in). Typical flow: get_page_json -> modify the JSON -> update_page_json. ' + + 'in). Typical flow: read the page-JSON view -> modify the JSON -> write it back. ' + 'Keep existing node ids intact so heading anchors and history stay ' + 'stable. Minimal full-doc example: {"type":"doc","content":[{"type":' + '"paragraph","content":[{"type":"text","text":"Hi"}]}]}. `content` may be ' + 'a JSON object or a JSON string (both accepted), and is OPTIONAL: omit it ' + - 'to update only the title (though prefer rename_page for a title-only ' + + 'to update only the title (though prefer the rename-page tool for a title-only ' + 'change). Supplying neither content nor title is an error. Reversible: ' + 'the previous version is kept in page history.', tier: 'deferred', @@ -731,7 +731,7 @@ export const SHARED_TOOL_SPECS = { 'Export a page to a single self-contained, lossless Docmost-flavoured ' + 'Markdown file (custom extensions): YAML-free meta header, body with ' + 'inline comment anchors and diagrams, and a trailing comments-thread ' + - 'block. Designed for a download -> edit body -> import_page_markdown ' + + 'block. Designed for a download -> edit body -> page-Markdown import ' + 'round-trip that preserves everything, including comment highlights. ' + 'Comment THREADS are preserved in the file but are not re-pushed to the ' + 'server on import.',