From fe5b6ecd8c45f178ab9bc6821a31643d8aafbd79 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 10 Jul 2026 08:29:46 +0300 Subject: [PATCH] =?UTF-8?q?refactor(mcp):=20=D0=B2=D1=8B=D0=B2=D0=B5=D1=81?= =?UTF-8?q?=D1=82=D0=B8=20DocmostClientLike/SharedToolSpec=20=D0=B8=D0=B7?= =?UTF-8?q?=20=D1=80=D0=B5=D0=B0=D0=BB=D1=8C=D0=BD=D0=BE=D0=B3=D0=BE=20?= =?UTF-8?q?=D1=82=D0=B8=D0=BF=D0=B0=20=D0=BA=D0=BB=D0=B8=D0=B5=D0=BD=D1=82?= =?UTF-8?q?=D0=B0=20=E2=80=94=20=D1=83=D0=B1=D0=B8=D1=82=D1=8C=20=D1=80?= =?UTF-8?q?=D1=83=D1=87=D0=BD=D1=8B=D0=B5=20=D0=B7=D0=B5=D1=80=D0=BA=D0=B0?= =?UTF-8?q?=D0=BB=D0=B0=20(#446)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Восстановленный отложенный долг #294: @docmost/mcp не отдавал .d.ts, поэтому в сервере жили ТРИ дрейфующие ручные копии одних и тех же имён/сигнатур (DocmostClientLike ~230 строк, копия SharedToolSpec, name-only HOST_CONTRACT_ METHODS-тест). In-app execute-тела зовут клиент ПОЗИЦИОННО, так что перестановка параметра в client.ts доезжала до прода рантайм-ошибкой без сигнала на компиляции. - declaration:true (+declarationMap) в packages/mcp/tsconfig.json; types-экспорт в package.json (exports → conditional {types, default} для . и ./http; require.resolve/dynamic-import резолвят default → build/index.js, рантайм не тронут). build/index.d.ts эмитится, реэкспортит DocmostClient + SharedToolSpec. Правок исходников пакета для эмита НЕ потребовалось. - DocmostClientLike → Pick из type-only import (стёрт на компиляции, ESM/CJS-границу не задевает); ручное зеркало удалено. - SharedToolSpec → type-only реэкспорт из пакета; ручная копия удалена. - client-host-contract.test.mjs удалён целиком — имена И сигнатуры теперь проверяет tsc. - Позиционная безопасность: never-called __assertClientCallContract(client: DocmostClientLike) воспроизводит каждый позиционный вызов с типизированными плейсхолдерами (AI-SDK стирает вход execute-замыканий в any, иначе позиционные вызовы не проверялись). Перестановка параметров client.ts → ошибка компиляции сервера ровно тут. Loose as-касты в ai-chat-tools.service не потребовали правок; as any не добавлялся. Внутреннее ревью: APPROVE. Runtime resolution через conditional exports не сломан (разобрано для прод-инсталляции, не только symlink); покрытие __assertClientCallContract полное (48 call-sites == union == assert, сверено программно); Pick полон; демонстрация reorder → TS2345 в assert. Единственная находка (Promise в части возвратов) предсуществующая в client.ts, вне цели PR. Стоит на #447 (закрытие skew build/vs/src) — мержить после него. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tools/ai-chat-tools.service.spec.ts | 23 +- .../ai-chat/tools/ai-chat-tools.service.ts | 94 +++++ .../tools/comment-signal-inapp.spec.ts | 20 +- .../ai-chat/tools/docmost-client.loader.ts | 359 ++++-------------- packages/mcp/package.json | 11 +- .../test/unit/client-host-contract.test.mjs | 224 ----------- packages/mcp/tsconfig.json | 2 + 7 files changed, 217 insertions(+), 516 deletions(-) delete mode 100644 packages/mcp/test/unit/client-host-contract.test.mjs 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 57f90b7d..a2bd5be4 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 @@ -1,6 +1,17 @@ import { AiChatToolsService } from './ai-chat-tools.service'; import * as loader from './docmost-client.loader'; import type { DocmostClientLike } from './docmost-client.loader'; + +// Test-double type for the loopback client. `DocmostClientLike` is now DERIVED +// from the real `DocmostClient` (issue #446), so its method RETURN types are the +// concrete client shapes. These stubs deliberately return minimal recording +// shapes (e.g. `{ ok: true }`), which no longer satisfy those concrete returns — +// so the doubles are typed with the same method NAMES but loose async returns. +// Each is still cast to `DocmostClientLike` at the (return-erased) mock site, so +// the positional-call type-safety on the PRODUCTION client is unaffected. +type FakeDocmostClient = Partial< + Record Promise> +>; // The real zod-agnostic shared tool-spec registry. It has no runtime deps, so // importing the TS source directly keeps these mocks honest: the service builds // the shared tools from exactly the specs the package ships, not a hand-stub. @@ -31,7 +42,7 @@ describe('AiChatToolsService deletePage guardrail (H4)', () => { // Minimal fake DocmostClient: only the write methods the tools touch need to // exist; deletePage records its args. No network, no ESM import. - const fakeClient: Partial = { + const fakeClient: FakeDocmostClient = { deletePage: (...args: unknown[]) => { deletePageCalls.push(args); return Promise.resolve({ success: true }); @@ -160,7 +171,7 @@ describe('AiChatToolsService deletePage guardrail (H4)', () => { describe('AiChatToolsService expanded toolset guardrails', () => { // No client method is invoked here — every assertion is on tool presence / // input schema — so an empty fake client is sufficient. - const fakeClient: Partial = {}; + const fakeClient: FakeDocmostClient = {}; const tokenServiceStub = { generateAccessToken: jest.fn().mockResolvedValue('access-token'), @@ -265,7 +276,7 @@ describe('AiChatToolsService node-arg JSON-string coercion', () => { const insertNodeCalls: unknown[][] = []; const updatePageJsonCalls: unknown[][] = []; - const fakeClient: Partial = { + const fakeClient: FakeDocmostClient = { patchNode: (...args: unknown[]) => { patchNodeCalls.push(args); return Promise.resolve({ ok: true }); @@ -439,7 +450,7 @@ describe('AiChatToolsService node-arg JSON-string coercion', () => { * getOutline) are exercised here end-to-end through forUser(). */ describe('AiChatToolsService model-friendly input validation (#190)', () => { - const fakeClient: Partial = {}; + const fakeClient: FakeDocmostClient = {}; const tokenServiceStub = { generateAccessToken: jest.fn().mockResolvedValue('access-token'), generateCollabToken: jest.fn().mockResolvedValue('collab-token'), @@ -557,7 +568,7 @@ describe('AiChatToolsService #294 changed execute wirings', () => { tableDeleteRow: [], tableUpdateCell: [], }; - const fakeClient: Partial = { + const fakeClient: FakeDocmostClient = { movePage: (...args: unknown[]) => { calls.movePage.push(args); return Promise.resolve({ success: true }); @@ -666,7 +677,7 @@ describe('AiChatToolsService #410 footnote + image tools', () => { insertImage: [], replaceImage: [], }; - const fakeClient: Partial = { + const fakeClient: FakeDocmostClient = { insertFootnote: (...args: unknown[]) => { calls.insertFootnote.push(args); return Promise.resolve({ success: true, footnoteId: 'fn1', reused: false }); 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 c7609a77..a6fa4ed5 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 @@ -26,6 +26,100 @@ import { type ToolCatalogEntry, } from './tool-tiers'; +/** + * Compile-time contract (issue #446): the in-app tool `execute` closures below + * call the loopback `DocmostClient` POSITIONALLY (e.g. + * `client.drawioGet(pageId, node, format ?? 'xml')`). Those closures receive an + * AI-SDK-erased (`any`) input, so a positional call inside them is NOT checked + * against the real signature — a parameter reorder/type-change in + * `packages/mcp/src/client.ts` would otherwise reach production as a runtime + * "wrong argument" tool failure with zero compile signal (the restored #294 + * debt). This never-called function reproduces every positional call with + * correctly-typed placeholder arguments against the DERIVED `DocmostClientLike` + * (a `Pick` of the real `DocmostClient`), so any such reorder/rename becomes a + * SERVER COMPILE ERROR here. It emits nothing (types only) and is never invoked; + * keep each call in lockstep with the matching `execute` body below. + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars +function __assertClientCallContract(client: DocmostClientLike): void { + // Placeholders standing in for the AI-SDK-erased execute inputs. Their types + // are deliberately concrete so the positional calls are checked end-to-end. + const s = '' as string; + const n = 0 as number; + const node: unknown = null; + const edits: Array<{ find: string; replace: string; replaceAll?: boolean }> = + []; + const cells: string[] = []; + const align = undefined as 'left' | 'center' | 'right' | undefined; + + // --- read --- + void client.search(s, undefined, n); + void client.getPage(s); + void client.getPageRaw(s); + void client.getWorkspace(); + void client.getSpaces(); + void client.listPages(s, n, true); + void client.listSidebarPages(s, s); + void client.getOutline(s); + void client.getPageJson(s); + void client.getNode(s, s); + void client.searchInPage(s, s, { + regex: true, + caseSensitive: true, + limit: n, + }); + void client.getTable(s, s); + void client.listComments(s, true); + void client.getComment(s); + void client.checkNewComments(s, s, s); + void client.listShares(); + void client.listPageHistory(s, s); + void client.getPageHistory(s); + void client.diffPageVersions(s, s, s); + void client.exportPageMarkdown(s); + // --- write (page) --- + void client.createPage(s, s, s, s); + void client.updatePage(s, s, s); + void client.renamePage(s, s); + void client.movePage(s, s, s); + void client.deletePage(s); + void client.editPageText(s, edits); + void client.patchNode(s, s, node); + void client.insertNode(s, node, { + position: 'append', + anchorNodeId: s, + anchorText: s, + }); + void client.deleteNode(s, s); + void client.updatePageJson(s, node, s); + void client.tableInsertRow(s, s, cells, n); + void client.tableDeleteRow(s, s, n); + void client.tableUpdateCell(s, s, n, n, s); + void client.copyPageContent(s, s); + void client.importPageMarkdown(s, s); + void client.sharePage(s, true); + void client.unsharePage(s); + void client.restorePageVersion(s); + void client.transformPage(s, s, { dryRun: true }); + void client.stashPage(s); + // --- write (image / footnote), in-app since #410 --- + void client.insertFootnote(s, s, s); + void client.insertImage(s, s, { + align, + alt: s, + replaceText: s, + afterText: s, + }); + void client.replaceImage(s, s, s, { align, alt: s }); + // --- draw.io diagrams (#423) --- + void client.drawioGet(s, s, 'xml'); + void client.drawioCreate(s, { position: 'append', anchorNodeId: s }, s, s); + void client.drawioUpdate(s, s, s, s); + // --- write (comment) --- + void client.createComment(s, s, 'inline', s, s, s); + void client.resolveComment(s, true); +} + /** * Per-user, per-request adapter that exposes Docmost READ operations to the * agent as AI SDK tools (STAGE A = read only). diff --git a/apps/server/src/core/ai-chat/tools/comment-signal-inapp.spec.ts b/apps/server/src/core/ai-chat/tools/comment-signal-inapp.spec.ts index 32d28433..1df0fb85 100644 --- a/apps/server/src/core/ai-chat/tools/comment-signal-inapp.spec.ts +++ b/apps/server/src/core/ai-chat/tools/comment-signal-inapp.spec.ts @@ -7,6 +7,16 @@ import type { DocmostClientLike, CommentSignalTrackerLike, } from './docmost-client.loader'; + +// Test-double type for the loopback client. `DocmostClientLike` is now DERIVED +// from the real `DocmostClient` (issue #446), so its method RETURN types are the +// concrete client shapes. These probe stubs deliberately return minimal shapes +// (e.g. `getPageRaw` yielding only `{ title }`), so the doubles use the same +// method NAMES but loose async returns; each is cast to `DocmostClientLike` at +// the (return-erased) mock site, leaving production positional-call safety intact. +type FakeDocmostClient = Partial< + Record Promise> +>; import { SHARED_TOOL_SPECS } from '../../../../../../packages/mcp/src/tool-specs'; // The REAL shared tracker factory, imported from source (same cross-boundary // approach the tool-specs spec uses) so the in-app wiring is exercised against @@ -268,7 +278,7 @@ describe('AiChatToolsService forUser + comment signal (real tracker)', () => { // seeded at forUser time). const future = new Date(Date.now() + 3_600_000).toISOString(); - function buildService(fakeClient: Partial) { + function buildService(fakeClient: FakeDocmostClient) { jest.spyOn(loader, 'loadDocmostMcp').mockResolvedValue({ DocmostClient: function () { return fakeClient as DocmostClientLike; @@ -317,7 +327,7 @@ describe('AiChatToolsService forUser + comment signal (real tracker)', () => { afterEach(() => jest.restoreAllMocks()); it('emits the signal (model-only) on a non-comment tool when a new comment exists', async () => { - const fakeClient: Partial = { + const fakeClient: FakeDocmostClient = { getPage: async () => ({ data: { title: 'Иранские языки', content: 'body' }, success: true, @@ -342,7 +352,7 @@ describe('AiChatToolsService forUser + comment signal (real tracker)', () => { }); it('does NOT add the signal to the listComments tool itself (tautological)', async () => { - const fakeClient: Partial = { + const fakeClient: FakeDocmostClient = { listComments: async () => ({ items: [{ createdAt: future }], resolvedThreadsHidden: 0, @@ -356,7 +366,7 @@ describe('AiChatToolsService forUser + comment signal (real tracker)', () => { }); it('no new comments => tool output is byte-identical AND the model sees no signal', async () => { - const fakeClient: Partial = { + const fakeClient: FakeDocmostClient = { getPage: async () => ({ data: { title: 'T', content: 'body' }, success: true, @@ -372,7 +382,7 @@ describe('AiChatToolsService forUser + comment signal (real tracker)', () => { }); it('injection-safety: a malicious page title cannot forge a second signal', async () => { - const fakeClient: Partial = { + const fakeClient: FakeDocmostClient = { getPage: async () => ({ data: { title: 'body-title', content: 'body' }, success: true, diff --git a/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts b/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts index 55ce1817..4573199b 100644 --- a/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts +++ b/apps/server/src/core/ai-chat/tools/docmost-client.loader.ts @@ -2,266 +2,92 @@ import { createHash } from 'node:crypto'; import { existsSync, readFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { pathToFileURL } from 'node:url'; +import type { DocmostClient, SharedToolSpec } from '@docmost/mcp'; + +// Re-export SharedToolSpec so downstream server modules keep a single import +// path (they import it from this loader). The shape is DERIVED from the package +// entry, not re-declared here — see the import above (issue #446). +export type { SharedToolSpec } from '@docmost/mcp'; /** - * Minimal structural type for the `DocmostClient` class we consume from the - * ESM-only `@docmost/mcp` package. We only need the constructor + the read/write - * methods used by the per-user tool adapter; the full client surface lives in - * `packages/mcp/src/client.ts`. Signatures here mirror that file exactly. - * - * DRIFT GUARD: the method NAMES below are runtime-checked against the real - * `DocmostClient` by `packages/mcp/test/unit/client-host-contract.test.mjs` - * (which can import the ESM class directly). If you rename/remove a method here - * or in client.ts, that test fails — so a stale mirror cannot silently ship a - * runtime "x is not a function" into an agent tool call. Keep the two in sync. - * - * STAGED PLAN — full derivation `DocmostClientLike = ` - * (issue #193, layer 3) is intentionally NOT done; it stays a hand-mirror for - * now because of two verified blockers across the ESM(mcp)/CJS(server) boundary: - * 1. `@docmost/mcp` emits NO declaration files (its tsconfig has no - * `declaration`, package.json has no `types`/types-export) and the server - * tsconfig has no path mapping for it — the server only loads it via the - * runtime `import()` trick below, so there is no type to import today. - * 2. The real client methods have inferred, CONCRETE return types; the in-app - * tool adapter reads results through loose `Record` returns - * + `as` casts (e.g. `(result?.data ?? {}) as { title?: string }`). - * Deriving the exact type would make those casts non-overlapping ("may be a - * mistake") and break the build, and `Partial` test stubs - * would have to satisfy the full concrete surface. - * To do it safely later (incrementally): (a) turn on `declaration: true` in - * packages/mcp/tsconfig.json + add a `types` export condition and commit the - * emitted `.d.ts`; (b) `import type { DocmostClient } from '@docmost/mcp'` here - * and replace this interface with a `Pick` of the consumed - * methods; (c) audit every `as` cast in ai-chat-tools.service.ts against the now - * concrete return types (double-cast through `unknown` only where genuinely - * needed); (d) keep the runtime guard test as a belt-and-braces check. Until - * then the guard test above is the cheap, behaviour-neutral protection. + * The exact set of `DocmostClient` methods the per-user in-app tool adapter + * consumes. This is the AUTHORITATIVE list of the client surface the server + * depends on; the adapter calls these methods POSITIONALLY, so this set is what + * the derived type below type-checks against the real class (issue #446). */ -export interface DocmostClientLike { +type DocmostClientMethod = // --- read --- - search( - query: string, - spaceId?: string, - limit?: number, - ): Promise<{ items: unknown[]; success: boolean }>; - getPage( - pageId: string, - ): Promise<{ data: Record; success: boolean }>; - // Light raw page info (`/pages/info`): title + slugId + ProseMirror content, - // WITHOUT the Markdown render / subpage expansion getPage does. Used by the - // comment-signal probe to read just the page title on a hit. - getPageRaw(pageId: string): Promise | null>; - getWorkspace(): Promise<{ data: Record; success: boolean }>; - getSpaces(): Promise; - listPages( - spaceId?: string, - limit?: number, - tree?: boolean, - ): Promise; - listSidebarPages(spaceId: string, pageId?: string): Promise; - getOutline(pageId: string): Promise>; - getPageJson(pageId: string): Promise>; - getNode(pageId: string, nodeId: string): Promise>; - searchInPage( - pageId: string, - query: string, - opts?: { regex?: boolean; caseSensitive?: boolean; limit?: number }, - ): Promise>; - getTable(pageId: string, tableRef: string): Promise>; - // Returns `{ items, resolvedThreadsHidden }`. DEFAULT (includeResolved unset/ - // false) hides resolved threads wholesale; pass true for the full feed. - listComments( - pageId: string, - includeResolved?: boolean, - ): Promise<{ items: unknown[]; resolvedThreadsHidden: number }>; - getComment( - commentId: string, - ): Promise<{ data: Record; success: boolean }>; - checkNewComments( - spaceId: string, - since: string, - parentPageId?: string, - ): Promise; - listShares(): Promise; - listPageHistory( - pageId: string, - cursor?: string, - ): Promise<{ items: unknown[]; nextCursor: string | null }>; - getPageHistory(historyId: string): Promise>; - diffPageVersions( - pageId: string, - from?: string, - to?: string, - ): Promise>; - exportPageMarkdown(pageId: string): Promise; + | 'search' + | 'getPage' + | 'getPageRaw' + | 'getWorkspace' + | 'getSpaces' + | 'listPages' + | 'listSidebarPages' + | 'getOutline' + | 'getPageJson' + | 'getNode' + | 'searchInPage' + | 'getTable' + | 'listComments' + | 'getComment' + | 'checkNewComments' + | 'listShares' + | 'listPageHistory' + | 'getPageHistory' + | 'diffPageVersions' + | 'exportPageMarkdown' // --- write (page) --- - createPage( - title: string, - content: string, - spaceId: string, - parentPageId?: string, - ): Promise<{ data: Record; success: boolean }>; - // Markdown content update via the collab path (carries provenance via the - // collab-token provider). Optionally also updates the title. - updatePage( - pageId: string, - content: string, - title?: string, - ): Promise>; - // Title-only rename via REST. - renamePage( - pageId: string, - title: string, - ): Promise>; - // Move via REST. parentPageId null => move to space root. - movePage( - pageId: string, - parentPageId: string | null, - position?: string, - ): Promise; - // SOFT delete only (POST /pages/delete with { pageId }). NEVER permanent. - deletePage(pageId: string): Promise; - editPageText( - pageId: string, - edits: Array<{ find: string; replace: string; replaceAll?: boolean }>, - ): Promise>; - patchNode( - pageId: string, - nodeId: string, - node: unknown, - ): Promise>; - insertNode( - pageId: string, - node: unknown, - opts: { - position: 'before' | 'after' | 'append'; - anchorNodeId?: string; - anchorText?: string; - }, - ): Promise>; - deleteNode( - pageId: string, - nodeId: string, - ): Promise>; - updatePageJson( - pageId: string, - doc?: unknown, - title?: string, - ): Promise>; - // Attach an author-inline footnote after the first occurrence of anchorText; - // numbering + the footnotes list are derived server-side. - insertFootnote( - pageId: string, - anchorText: string, - text: string, - ): Promise>; - // Download a web image and insert it into the page (append, or replace/after a - // text anchor). `url` is the image http(s) URL. - insertImage( - pageId: string, - url: string, - opts?: { - align?: 'left' | 'center' | 'right'; - alt?: string; - replaceText?: string; - afterText?: string; - }, - ): Promise>; - // Swap an existing image (by its attachmentId) for a new one fetched from a web - // URL, repointing every reference in the live document. - replaceImage( - pageId: string, - oldAttachmentId: string, - url: string, - opts?: { align?: 'left' | 'center' | 'right'; alt?: string }, - ): Promise>; + | 'createPage' + | 'updatePage' + | 'renamePage' + | 'movePage' + | 'deletePage' + | 'editPageText' + | 'patchNode' + | 'insertNode' + | 'deleteNode' + | 'updatePageJson' + | 'tableInsertRow' + | 'tableDeleteRow' + | 'tableUpdateCell' + | 'copyPageContent' + | 'importPageMarkdown' + | 'sharePage' + | 'unsharePage' + | 'restorePageVersion' + | 'transformPage' + | 'stashPage' + // --- write (image / footnote), in-app since #410 --- + | 'insertImage' + | 'replaceImage' + | 'insertFootnote' // --- draw.io diagrams (#423, stage 1) --- - // Read a diagram as decoded mxGraph XML (default) or the raw .drawio.svg. - // meta.hash is the optimistic-lock key drawioUpdate expects as baseHash. - drawioGet( - pageId: string, - node: string, - format?: 'xml' | 'svg', - ): Promise>; - // Lint mxGraph XML, build the .drawio.svg attachment and insert a drawio node. - drawioCreate( - pageId: string, - where: { - position: 'before' | 'after' | 'append'; - anchorNodeId?: string; - anchorText?: string; - }, - xml: string, - title?: string, - ): Promise>; - // Optimistic-locked full replacement of a diagram (baseHash from drawioGet). - drawioUpdate( - pageId: string, - node: string, - xml: string, - baseHash: string, - ): Promise>; - tableInsertRow( - pageId: string, - tableRef: string, - cells: string[], - index?: number, - ): Promise>; - tableDeleteRow( - pageId: string, - tableRef: string, - index: number, - ): Promise>; - tableUpdateCell( - pageId: string, - tableRef: string, - row: number, - col: number, - text: string, - ): Promise>; - copyPageContent( - sourcePageId: string, - targetPageId: string, - ): Promise>; - importPageMarkdown( - pageId: string, - fullMarkdown: string, - ): Promise>; - sharePage( - pageId: string, - searchIndexing?: boolean, - ): Promise>; - unsharePage(pageId: string): Promise>; - restorePageVersion(historyId: string): Promise>; - // The opts type declares deleteComments? to match the real client signature, - // but the agent tool NEVER sets it (comment deletion stays unreachable). - transformPage( - pageId: string, - transformJs: string, - opts?: { dryRun?: boolean; deleteComments?: boolean }, - ): Promise>; + | 'drawioGet' + | 'drawioCreate' + | 'drawioUpdate' // --- write (comment) --- - createComment( - pageId: string, - content: string, - type?: 'page' | 'inline', - selection?: string, - parentCommentId?: string, - suggestedText?: string, - ): Promise<{ data: Record; success: boolean }>; - resolveComment( - commentId: string, - resolved: boolean, - ): Promise>; - // Serialize a page + mirror its internal images into the blob sandbox; returns - // ONLY a short anonymous URL (the body never enters the model context). - stashPage(pageId: string): Promise<{ - uri: string; - sha256: string; - size: number; - images: { mirrored: number; failed: number }; - }>; -} + | 'createComment' + | 'resolveComment'; + +/** + * The client surface the per-user tool adapter consumes, DERIVED from the real + * `DocmostClient` type in `@docmost/mcp` (issue #446, restored #294 debt). This + * replaces the former hand-mirror of ~45 method signatures. + * + * `import type` (above) is fully ERASED at compile time, so nothing is actually + * imported from the ESM-only package at runtime — the server still loads the + * class through the dynamic `import()` trick in `loadDocmostMcp` below; this is + * purely a compile-time type. Deriving via `Pick` means a parameter reorder or a + * type change to any of these methods in `client.ts` now becomes a SERVER + * COMPILE ERROR at the positional call sites in ai-chat-tools.service.ts, + * instead of a silent runtime "wrong argument" failure inside an agent tool. + * + * This made the old name-only drift-guard test + * (packages/mcp/test/unit/client-host-contract.test.mjs) redundant — tsc now + * enforces both names AND signatures — so that test was removed. + */ +export type DocmostClientLike = Pick; export type DocmostClientConfig = { apiUrl: string; @@ -283,32 +109,7 @@ export type DocmostClientConfig = { }; export interface DocmostClientCtor { - new (config: DocmostClientConfig): DocmostClientLike; -} - -/** - * Local hand-mirror of the `SharedToolSpec` shape exported from - * `@docmost/mcp` (packages/mcp/src/tool-specs.ts). Same approach as - * `DocmostClientLike`: we do not import the ESM package's types directly across - * the CJS/ESM boundary. The registry itself has no runtime deps, but keeping the - * type local avoids coupling the server build to the package's type surface. - * - * `buildShape` is intentionally zod-agnostic: it returns a plain ZodRawShape - * built with whatever zod namespace the caller passes (the server passes its own - * zod v4; the MCP package passes its zod v3). See the registry module comment. - */ -export interface SharedToolSpec { - mcpName: string; - inAppKey: string; - description: string; - // Deferred-tool metadata (#332). Optional in this mirror so an older/stale - // @docmost/mcp build (pre-#332) still type-checks; the in-app catalog builder - // reads them defensively. The external /mcp server ignores both fields. - tier?: 'core' | 'deferred'; - catalogLine?: string; - // Loose `z` on purpose: the registry is zod-agnostic so the server can pass - // its own zod (v4) and the MCP package its own (v3) into the same builder. - buildShape?: (z: any) => Record; + new (config: DocmostClientConfig): DocmostClient; } /** diff --git a/packages/mcp/package.json b/packages/mcp/package.json index 66c6db75..66386299 100644 --- a/packages/mcp/package.json +++ b/packages/mcp/package.json @@ -5,9 +5,16 @@ "private": true, "type": "module", "main": "./build/index.js", + "types": "./build/index.d.ts", "exports": { - ".": "./build/index.js", - "./http": "./build/http.js" + ".": { + "types": "./build/index.d.ts", + "default": "./build/index.js" + }, + "./http": { + "types": "./build/http.d.ts", + "default": "./build/http.js" + } }, "bin": { "docmost-mcp": "./build/stdio.js" diff --git a/packages/mcp/test/unit/client-host-contract.test.mjs b/packages/mcp/test/unit/client-host-contract.test.mjs deleted file mode 100644 index fa9784e4..00000000 --- a/packages/mcp/test/unit/client-host-contract.test.mjs +++ /dev/null @@ -1,224 +0,0 @@ -import { test } from "node:test"; -import assert from "node:assert/strict"; -import { readFileSync } from "node:fs"; -import { fileURLToPath } from "node:url"; -import { dirname, resolve } from "node:path"; - -import { DocmostClient } from "../../build/index.js"; - -// Drift guard for the THIRD hand-written layer of the AI tool set (issue #193, -// layer 3): the in-app server hand-mirrors the DocmostClient method signatures -// it consumes as the `DocmostClientLike` interface in -// apps/server/src/core/ai-chat/tools/docmost-client.loader.ts ("Signatures here -// mirror that file exactly"). That mirror lives across the ESM(mcp)/CJS(server) -// boundary and the package ships NO .d.ts, so the server typecheck cannot verify -// the names against the real class — a rename/removal in client.ts would surface -// only as a runtime "x is not a function" inside an agent tool call. -// -// SCOPE: this guard checks the method-NAME set only, not signatures. It pins the -// contract from the mcp side (ESM, where the real class is directly importable): -// every method the embedding host depends on MUST exist as a function on a real -// DocmostClient instance. If you rename/remove a client method, this fails here -// AND you must update DocmostClientLike to match. It does NOT verify parameter or -// return-type parity — signature drift between the hand-mirror and client.ts can -// still ship silently; full signature/type parity is the deferred staged-plan -// item below. -// -// Keep the HOST_CONTRACT_METHODS NAME list aligned with the method NAMES declared -// in the server's DocmostClientLike interface (the in-app per-user tool adapter -// only — it is a SUBSET of the DocmostClient surface — covers only what the in-app adapter -// consumes; the standalone MCP transport (packages/mcp/src/index.ts) calls additional -// client methods (deleteComment/updateComment) that this guard does NOT track — the -// MCP transport's own typecheck covers those. insertImage/replaceImage/insertFootnote -// were MCP-only but are now in-app-consumed too (#410), so they ARE tracked below. Full type-derivation -// of DocmostClientLike from this class is deferred (see the staged plan in -// docmost-client.loader.ts): the package emits no declarations and the real -// (inferred, concrete) return types conflict with the host's loose -// `Record` + `as`-cast result handling. -const HOST_CONTRACT_METHODS = [ - // read - "search", - "getPage", - "getPageRaw", - "getWorkspace", - "getSpaces", - "listPages", - "listSidebarPages", - "getOutline", - "getPageJson", - "getNode", - "searchInPage", - "getTable", - "listComments", - "getComment", - "checkNewComments", - "listShares", - "listPageHistory", - "getPageHistory", - "diffPageVersions", - "exportPageMarkdown", - // write (page) - "createPage", - "updatePage", - "renamePage", - "movePage", - "deletePage", - "editPageText", - "patchNode", - "insertNode", - "deleteNode", - "updatePageJson", - "tableInsertRow", - "tableDeleteRow", - "tableUpdateCell", - "copyPageContent", - "importPageMarkdown", - "sharePage", - "unsharePage", - "restorePageVersion", - "transformPage", - "stashPage", - // write (image / footnote) — MCP-only until #410 promoted them to in-app tools - "insertImage", - "replaceImage", - "insertFootnote", - // draw.io diagrams (#423, stage 1) — read + create + optimistic-locked update - "drawioGet", - "drawioCreate", - "drawioUpdate", - // write (comment) - "createComment", - "resolveComment", -]; - -test("DocmostClient implements every method the in-app DocmostClientLike mirror declares", () => { - // The constructor is side-effect-free (no network/login on construction): it - // only stores config and creates an axios instance, so it is safe to build a - // throwaway instance here with a dummy token provider. - const client = new DocmostClient({ - apiUrl: "http://127.0.0.1:1/api", - getToken: async () => "test-token", - }); - - const missing = HOST_CONTRACT_METHODS.filter( - (name) => typeof client[name] !== "function", - ); - - assert.deepEqual( - missing, - [], - `DocmostClient is missing host-contract method(s): ${missing.join(", ")}. ` + - `Update packages/mcp/src/client.ts and/or the server's DocmostClientLike ` + - `interface (apps/server/src/core/ai-chat/tools/docmost-client.loader.ts) ` + - `so the hand-mirrored method NAMES stay aligned (this guards names only, ` + - `not signatures).`, - ); -}); - -test("HOST_CONTRACT_METHODS has no duplicates", () => { - assert.equal( - new Set(HOST_CONTRACT_METHODS).size, - HOST_CONTRACT_METHODS.length, - ); -}); - -// Parse the method names declared in the server's `DocmostClientLike` interface -// body. We read the .ts source as plain text (no TS compiler dep, and the file -// lives in the CJS server tree across the ESM boundary): scan from the -// `export interface DocmostClientLike {` line to its closing brace at column 0, -// matching member-signature lines like ` methodName(`. Nested param-object -// braces (`opts: { ... }`) are indented, so only the interface's own closing -// `}` (column 0) ends the scan. -function parseDocmostClientLikeMethods() { - const here = dirname(fileURLToPath(import.meta.url)); - // packages/mcp/test/unit -> repo root is four levels up. - const loaderPath = resolve( - here, - "../../../../apps/server/src/core/ai-chat/tools/docmost-client.loader.ts", - ); - let source; - try { - source = readFileSync(loaderPath, "utf8"); - } catch (err) { - if (err && err.code === "ENOENT") { - throw new Error( - `Expected monorepo layout; server tree at ${loaderPath} not found. ` + - `This drift-guard reads the server's DocmostClientLike interface via a ` + - `fixed relative path and must run from inside the monorepo checkout.`, - ); - } - throw err; - } - const lines = source.split(/\r?\n/); - - const startIdx = lines.findIndex((l) => - /^export interface DocmostClientLike\s*\{/.test(l), - ); - assert.notEqual( - startIdx, - -1, - `Could not find "export interface DocmostClientLike {" in ${loaderPath}. ` + - `If the interface was renamed/moved, update this drift-guard test.`, - ); - - const methods = []; - let closed = false; - // Track whether we are inside a `/* ... */` block comment. Inner lines of a - // block comment need NOT start with `*`, so a `name(` line inside one would be - // falsely parsed as an interface method without this. (`//` line comments can - // never match the method regex below since they start with `/`.) - let inBlockComment = false; - for (let i = startIdx + 1; i < lines.length; i++) { - const line = lines[i]; - if (inBlockComment) { - // Stay in the block until we see its closing `*/`. - if (line.includes("*/")) inBlockComment = false; - continue; - } - // Enter a block comment only when it opens without closing on the same line; - // a self-contained `/* ... */` on one line cannot precede a method name we - // care about (such lines start with `/`, so the method regex won't match). - if (line.includes("/*") && !line.includes("*/")) { - inBlockComment = true; - continue; - } - if (/^\}/.test(line)) { - closed = true; - break; - } - // Method-name match: a TS identifier (letters/digits/`_`/`$`, not starting - // with a digit) optionally followed by a generic clause (`method(`), then - // the opening paren of the signature. - const m = /^\s*([A-Za-z_$][A-Za-z0-9_$]*)\s*(?:<[^>]*>)?\(/.exec(line); - if (m) methods.push(m[1]); - } - assert.ok( - closed, - `Did not find the closing brace of DocmostClientLike in ${loaderPath}.`, - ); - assert.ok( - methods.length > 0, - `Parsed zero methods from DocmostClientLike in ${loaderPath} — the parser ` + - `is likely out of date with the interface formatting.`, - ); - return methods; -} - -// The point of the guard is to protect the DocmostClientLike mirror <-> client.ts -// link, but HOST_CONTRACT_METHODS is itself a HAND-COPY of that interface kept in -// sync manually. The list<->interface link must be tested too: a method consumed -// by the adapter and added to DocmostClientLike but forgotten here (or removed -// from the interface but left here) would otherwise escape both the server -// typecheck (pkg emits no .d.ts) and the first test above (name not in the list). -// Assert the two agree BOTH ways. -test("HOST_CONTRACT_METHODS exactly mirrors the server's DocmostClientLike interface", () => { - const interfaceMethods = parseDocmostClientLikeMethods(); - assert.deepEqual( - [...HOST_CONTRACT_METHODS].sort(), - [...interfaceMethods].sort(), - `HOST_CONTRACT_METHODS has drifted from the DocmostClientLike interface in ` + - `apps/server/src/core/ai-chat/tools/docmost-client.loader.ts. Add/remove ` + - `method names in HOST_CONTRACT_METHODS so it lists EXACTLY the methods ` + - `declared in that interface (both directions are checked).`, - ); -}); diff --git a/packages/mcp/tsconfig.json b/packages/mcp/tsconfig.json index 8fabd247..b400424c 100644 --- a/packages/mcp/tsconfig.json +++ b/packages/mcp/tsconfig.json @@ -5,6 +5,8 @@ "moduleResolution": "Node16", "outDir": "./build", "rootDir": "./src", + "declaration": true, + "declarationMap": true, "strict": true, "esModuleInterop": true, "skipLibCheck": true,