From 292a0836f4433cbbb405628bdeeb2b1bb485e69c Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 4 Jul 2026 21:26:02 +0300 Subject: [PATCH] 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;