From f9757fda12dc5bdb752c35ecab7f39f0afe0c2f1 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sun, 21 Jun 2026 06:51:09 +0300 Subject: [PATCH 1/2] refactor(ai-chat): dedupe node-arg JSON normalization into a shared helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First, safe step of docs/backlog/ai-chat-tool-definitions-duplicated.md: the "node may be a JSON object OR a JSON string" quirk was hand-copied at 6 tool sites. Extract it into a single parseNodeArg() helper per package and call it at every site. Behavior-preserving — each site's throw message is byte-identical (patch/insert: 'node was a string but not valid JSON'; update_page_json: 'content was a string but not valid JSON'); no tool name/description/schema changed. Two helper copies (packages/mcp/src/lib/parse-node-arg.ts and apps/server/src/core/ai-chat/tools/parse-node-arg.ts) are intentional: the ESM-only @docmost/mcp cannot be imported by the CommonJS server (it is loaded at runtime via the Function('import()') trick), so runtime code cannot cross that boundary by a normal import. Each copy is now the single source within its package (6 inline copies -> 2 helpers). packages/mcp/build rebuilt in sync. Tests: parse-node-arg.spec.ts (server, Jest) + parse-node-arg.test.mjs (mcp, node:test) — object passthrough, valid-string parse, invalid-string throw with the right message. Server tsc clean; mcp suite 254 pass; agent structural-edit path verified live in-browser (agent inserted a node, persisted to the doc). Deferred (documented for the record, since the backlog doc is removed with this commit): the FULL transport-agnostic tool-spec registry (one name+schema+ description per tool shared by both transports) and deriving DocmostClientLike from the real client type. Both are blocked by the current architecture, not by effort: (1) @docmost/mcp ships no type declarations and is ESM-only, so a type-only derivation needs declaration emission + tsconfig path wiring, and the real client's precise return types break the in-app tool test stubs (attempted, reverted to keep tsc green); (2) the two transports intentionally DIVERGE in tool NAMES (snake_case x38 vs camelCase x41), membership (in-app adds getCurrentPage/ listSidebarPages, omits delete_comment/image tools) and model-facing DESCRIPTIONS, so a unified registry would change behavior on BOTH the agent and external MCP clients and needs its own verification pass. This is forward-looking debt (the code is correct today), to be done incrementally. Co-Authored-By: Claude Opus 4.8 --- .../ai-chat/tools/ai-chat-tools.service.ts | 28 +---- .../core/ai-chat/tools/parse-node-arg.spec.ts | 37 ++++++ .../src/core/ai-chat/tools/parse-node-arg.ts | 24 ++++ .../ai-chat-tool-definitions-duplicated.md | 108 ------------------ packages/mcp/build/index.js | 38 +----- packages/mcp/build/lib/parse-node-arg.js | 15 +++ packages/mcp/src/index.ts | 32 +----- packages/mcp/src/lib/parse-node-arg.ts | 17 +++ .../mcp/test/unit/parse-node-arg.test.mjs | 32 ++++++ 9 files changed, 140 insertions(+), 191 deletions(-) create mode 100644 apps/server/src/core/ai-chat/tools/parse-node-arg.spec.ts create mode 100644 apps/server/src/core/ai-chat/tools/parse-node-arg.ts delete mode 100644 docs/backlog/ai-chat-tool-definitions-duplicated.md create mode 100644 packages/mcp/build/lib/parse-node-arg.js create mode 100644 packages/mcp/src/lib/parse-node-arg.ts create mode 100644 packages/mcp/test/unit/parse-node-arg.test.mjs 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 038e2544..f94e9d1e 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 @@ -12,6 +12,7 @@ import { loadDocmostMcp, type DocmostClientLike, } from './docmost-client.loader'; +import { parseNodeArg } from './parse-node-arg'; /** * Per-user, per-request adapter that exposes Docmost READ operations to the @@ -711,14 +712,7 @@ export class AiChatToolsService { // Parity with the standalone MCP server (index.ts patch_node): the // model sometimes serializes the node as a JSON string. Parse it // before the client's typeof-object guard rejects it. - let parsedNode = node; - if (typeof node === 'string') { - try { - parsedNode = JSON.parse(node); - } catch { - throw new Error('node was a string but not valid JSON'); - } - } + const parsedNode = parseNodeArg(node); return await client.patchNode(pageId, nodeId, parsedNode); }, }), @@ -770,14 +764,7 @@ export class AiChatToolsService { // Parity with the standalone MCP server (index.ts insert_node): the // model sometimes serializes the node as a JSON string. Parse it // before the client's typeof-object guard rejects it. - let parsedNode = node; - if (typeof node === 'string') { - try { - parsedNode = JSON.parse(node); - } catch { - throw new Error('node was a string but not valid JSON'); - } - } + const parsedNode = parseNodeArg(node); return await client.insertNode(pageId, parsedNode, { position, anchorNodeId, @@ -826,14 +813,9 @@ export class AiChatToolsService { let doc; if (content === undefined || content === null) { doc = undefined; - } else if (typeof content === 'string') { - try { - doc = JSON.parse(content); - } catch { - throw new Error('content was a string but not valid JSON'); - } } else { - doc = content; + // String -> JSON.parse (throwing on invalid); object passes through. + doc = parseNodeArg(content, 'content was a string but not valid JSON'); } return await client.updatePageJson(pageId, doc, title); }, diff --git a/apps/server/src/core/ai-chat/tools/parse-node-arg.spec.ts b/apps/server/src/core/ai-chat/tools/parse-node-arg.spec.ts new file mode 100644 index 00000000..729c065d --- /dev/null +++ b/apps/server/src/core/ai-chat/tools/parse-node-arg.spec.ts @@ -0,0 +1,37 @@ +import { parseNodeArg } from './parse-node-arg'; + +/** + * Unit tests for the in-app `parseNodeArg` helper. It mirrors the standalone + * MCP helper (packages/mcp/src/lib/parse-node-arg.ts) and is used by the + * patchNode / insertNode / updatePageJson tool adapters. Behavior must be + * byte-identical: object passthrough, valid-string parse, invalid-string throw. + */ +describe('parseNodeArg', () => { + it('passes an object through unchanged', () => { + const obj = { type: 'paragraph', content: [] }; + expect(parseNodeArg(obj)).toBe(obj); + }); + + it('passes undefined/null through unchanged', () => { + expect(parseNodeArg(undefined)).toBeUndefined(); + expect(parseNodeArg(null)).toBeNull(); + }); + + it('parses a valid JSON string into an object', () => { + expect(parseNodeArg('{"type":"paragraph"}')).toEqual({ + type: 'paragraph', + }); + }); + + it('throws the default message on an invalid JSON string', () => { + expect(() => parseNodeArg('{not json')).toThrow( + 'node was a string but not valid JSON', + ); + }); + + it('throws a custom message on an invalid JSON string', () => { + expect(() => + parseNodeArg('{not json', 'content was a string but not valid JSON'), + ).toThrow('content was a string but not valid JSON'); + }); +}); diff --git a/apps/server/src/core/ai-chat/tools/parse-node-arg.ts b/apps/server/src/core/ai-chat/tools/parse-node-arg.ts new file mode 100644 index 00000000..6f43625d --- /dev/null +++ b/apps/server/src/core/ai-chat/tools/parse-node-arg.ts @@ -0,0 +1,24 @@ +// The model sometimes serializes a ProseMirror node arg as a JSON string +// instead of an object. Normalize: parse a string to an object (throwing on +// invalid JSON), pass an object through unchanged. Shared by patchNode / +// insertNode (and the analogous updatePageJson content parsing). +// +// This mirrors `packages/mcp/src/lib/parse-node-arg.ts` byte-for-byte. We +// cannot import that helper here: `@docmost/mcp` is ESM-only and this server +// compiles with module:commonjs, so it is loaded at runtime via the +// `new Function('import()')` trick (see docmost-client.loader.ts). Sharing +// runtime code across that ESM/CJS boundary by a normal import is impossible, +// hence the mirrored copy. +export function parseNodeArg( + node: unknown, + errMsg = 'node was a string but not valid JSON', +): unknown { + if (typeof node === 'string') { + try { + return JSON.parse(node); + } catch { + throw new Error(errMsg); + } + } + return node; +} diff --git a/docs/backlog/ai-chat-tool-definitions-duplicated.md b/docs/backlog/ai-chat-tool-definitions-duplicated.md deleted file mode 100644 index 2b78dd9b..00000000 --- a/docs/backlog/ai-chat-tool-definitions-duplicated.md +++ /dev/null @@ -1,108 +0,0 @@ -# Дублирование определений инструментов: in-app агент vs standalone MCP-пакет - -Статус: **зафиксировано в беклоге, код не менялся.** Это forward-looking -стоимость поддержки, НЕ баг — код корректен сегодня. Фиксируем, чтобы при -росте набора инструментов (см. §16) долг не разъезжался молча. - -## Суть - -Один и тот же набор инструментов поверх одного `DocmostClient` описан -**тремя независимыми рукописными слоями**. Каждое добавление инструмента или -правка его model-facing описания требует синхронной правки в 2–3 местах, а -parity-баги (расхождение копий) приходится чинить/переоткрывать дважды. - -## Где дублируется (три слоя) - -1. **Standalone MCP-сервер** — `packages/mcp/src/index.ts` (~38 `registerTool`). - Для внешних MCP-клиентов (stdio/http). На каждый инструмент: zod-схема + - длинное model-facing описание + тонкий `execute`, вызывающий `DocmostClient`. -2. **Встроенный AI-чат** — `apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts` - (~39 `tool({...})` через `ai`-SDK). Своя zod-схема + своё описание + свой - `execute` поверх ТОГО ЖЕ клиента (`@docmost/mcp` грузится в - `tools/docmost-client.loader.ts:188` через динамический `import()`). -3. **Ручная копия сигнатур** — интерфейс `DocmostClientLike` в - `apps/server/src/core/ai-chat/tools/docmost-client.loader.ts:9` (в комментарии - прямо: «Signatures here mirror that file exactly»), скопирован руками из - `packages/mcp/src/client.ts`. - -## Что именно продублировано (с подтверждением по коду) - -- **zod-схема + описание** каждого инструмента — в слоях 1 и 2 целиком. -- **Квирк «node как объект ИЛИ JSON-строка»** реализован дважды (НЕ в общем - клиенте): - - in-app: `ai-chat-tools.service.ts:686` (patchNode), `:745` (insertNode), - `:800` (updatePageJson); - - standalone: `index.ts:526` (patch_node), `:578` (insert_node), `:350` - (update_page_json). -- **Guardrail/семантика `transformPage` (dryRun)** описана в обоих: - `ai-chat-tools.service.ts:~935` и `index.ts:~1006`. - -## Почему разделение слоёв 1 и 2 само по себе оправдано - -У путей разный транспорт и auth-контекст, и это правильно держать раздельно: -in-app путь чеканит per-user JWT + provenance collab-токен (подписанная -agent-claim, `docmost-client.loader.ts:159` — `getCollabToken`; см. план §6.5), -а standalone обслуживает внешних клиентов по stdio/http. **Но** это оправдывает -два тонких адаптера (`execute` + auth-обвязка), а НЕ две рукописные копии -МЕТАДАННЫХ (схема + описание + квирки). Метаданные можно объявить один раз и -переиспользовать обоими транспортами. - -## Доказательство стоимости (наблюдалось при фиксе edit_page_text) - -При исправлении ложного «успеха» `edit_page_text` (refuse форматных правок + -`verify`-отчёт): -- **Поведение** легло в общий `DocmostClient` → автоматически дошло до обоих - агентов ОДНОЙ правкой. Это «хороший» случай — логика в едином источнике. -- **Описание** инструмента пришлось править ДВАЖДЫ: в `index.ts` (кодером) и - отдельно в `ai-chat-tools.service.ts:617`, где описание продолжало рекламировать - «Markdown wrappers tolerated via strip-and-retry» — ровно ту формулировку, что - ввела исходного агента в заблуждение. Копия молча разъехалась и какое-то время - встроенный агент получал устаревшую подсказку. Это и есть материализованный - parity-баг. - -## Расширение: дублируется не только описания инструментов — ещё и конвертер (PM ↔ Markdown) - -Зафиксировано при планировании встраивания git-синка (`docmost-sync` → gitmost, -нативная in-process интеграция). Та же болезнь «несколько рукописных копий одного -кода» теперь касается слоя конвертации ProseMirror ↔ Markdown и его lib, а не -только метаданных инструментов. - -- **Копия в gitmost** — `packages/mcp/src/lib/`: `markdown-converter.ts` (~885 - строк), `markdown-document.ts` (~136), `node-ops.ts`, `diff.ts`, - `docmost-schema.ts`. Канонизатора (`canonicalize.ts`) здесь НЕТ. -- **Копия в docmost-sync** — `packages/docmost-client/src/lib/`: тот же набор + - `canonicalize.ts` (~11 КБ, держит идемпотентность round-trip, SPEC §11) + - `markdown-document.ts` с режимом «тело + якоря, без тредов комментов» - (`includeCommentThreads:false`, на ~20 строк больше). -- **Третья копия (планируется)** — план git-синка вендорит чистую часть - конвертера в новый `packages/git-sync` (collab-файл не нужен: запись идёт - нативно через `openDirectConnection` + `@docmost/editor-ext`). - -Копии уже молча разъехались (docmost-sync vs `packages/mcp`): `collaboration.ts` -~329 изменённых строк, `node-ops.ts` ~53, `markdown-converter.ts` ~24, -`markdown-document.ts` ~20. Отдельно: `docmost-schema.ts` в lib дублирует -**реальную** схему сервера `@docmost/editor-ext` (её использует collab/persistence) -— расхождение схем = риск битой конвертации нод. - -Вывод: тот же фикс-вектор (единый источник правды), что и для инструментов, стоит -распространить на конвертер — общий пакет конвертации, потребляемый `mcp`, -`git-sync` и (в идеале) сервером. До конвергенции git-sync держит вендоренную -копию валидированного конвертера с гейтом round-trip против схемы `editor-ext` -(осознанный долг «третья копия сейчас, объединяем позже»). - -## Фикс - -Единый реестр спеков (полное устранение дублирования).** Вынести в - `packages/mcp` один источник на инструмент: `name` + zod-схема + model-facing - описание + общий хелпер нормализации node-строки (для patch/insert/update). - И `index.ts`, и `ai-chat-tools.service.ts` импортируют спеки и добавляют только - свой `execute`/auth. `DocmostClientLike` — выводить из типа реального клиента - (type-only import / генерация), а не копировать руками. - - Ограничение: `@docmost/mcp` — ESM-only, сервер грузит его через трюк - `new Function('import(specifier)')` (`docmost-client.loader.ts:174`), потому - что `module:commonjs` даунлевелит `import()` в `require()`. Реестр спеков - (данные + zod) должен пересекать ту же ESM/CJS-границу — выполнимо тем же - динамическим импортом; `ai`-SDK `tool()` и MCP `registerTool()` имеют разную - форму, поэтому реестр экспортирует транспорт-агностичные `{name, schema, - description}`, а каждая сторона оборачивает их сама. `zod` — общая зависимость - обоих пакетов, типы переносятся. diff --git a/packages/mcp/build/index.js b/packages/mcp/build/index.js index 8214c9bd..efc101fc 100644 --- a/packages/mcp/build/index.js +++ b/packages/mcp/build/index.js @@ -4,6 +4,7 @@ import { readFileSync } from "fs"; import { fileURLToPath } from "url"; import { dirname, join } from "path"; import { DocmostClient } from "./client.js"; +import { parseNodeArg } from "./lib/parse-node-arg.js"; // Re-export the client and its config type so embedding hosts (e.g. the gitmost // NestJS server) can `import('@docmost/mcp')` and construct a DocmostClient // directly — for the credentials variant OR the per-user getToken variant. @@ -245,16 +246,9 @@ export function createDocmostMcpServer(config) { if (content === undefined || content === null) { doc = undefined; } - else if (typeof content === "string") { - try { - doc = JSON.parse(content); - } - catch { - throw new Error("content was a string but not valid JSON"); - } - } else { - doc = content; + // String -> JSON.parse (throwing on invalid); object passes through. + doc = parseNodeArg(content, "content was a string but not valid JSON"); } const result = await docmostClient.updatePageJson(pageId, doc, title); return jsonContent(result); @@ -379,18 +373,7 @@ export function createDocmostMcpServer(config) { "JSON object or JSON string both accepted."), }, }, async ({ pageId, nodeId, node }) => { - let parsedNode; - if (typeof node === "string") { - try { - parsedNode = JSON.parse(node); - } - catch { - throw new Error("node was a string but not valid JSON"); - } - } - else { - parsedNode = node; - } + const parsedNode = parseNodeArg(node); const result = await docmostClient.patchNode(pageId, nodeId, parsedNode); return jsonContent(result); }); @@ -425,18 +408,7 @@ export function createDocmostMcpServer(config) { anchorText: z.string().optional(), }, }, async ({ pageId, node, position, anchorNodeId, anchorText }) => { - let parsedNode; - if (typeof node === "string") { - try { - parsedNode = JSON.parse(node); - } - catch { - throw new Error("node was a string but not valid JSON"); - } - } - else { - parsedNode = node; - } + const parsedNode = parseNodeArg(node); const result = await docmostClient.insertNode(pageId, parsedNode, { position, anchorNodeId, diff --git a/packages/mcp/build/lib/parse-node-arg.js b/packages/mcp/build/lib/parse-node-arg.js new file mode 100644 index 00000000..4598b136 --- /dev/null +++ b/packages/mcp/build/lib/parse-node-arg.js @@ -0,0 +1,15 @@ +// The model sometimes serializes a ProseMirror node arg as a JSON string +// instead of an object. Normalize: parse a string to an object (throwing on +// invalid JSON), pass an object through unchanged. Shared by patch_node / +// insert_node (and the analogous update_page_json content parsing). +export function parseNodeArg(node, errMsg = "node was a string but not valid JSON") { + if (typeof node === "string") { + try { + return JSON.parse(node); + } + catch { + throw new Error(errMsg); + } + } + return node; +} diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 12f6b535..09bd2142 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -4,6 +4,7 @@ import { readFileSync } from "fs"; import { fileURLToPath } from "url"; import { dirname, join } from "path"; import { DocmostClient, DocmostMcpConfig } from "./client.js"; +import { parseNodeArg } from "./lib/parse-node-arg.js"; // Re-export the client and its config type so embedding hosts (e.g. the gitmost // NestJS server) can `import('@docmost/mcp')` and construct a DocmostClient @@ -354,14 +355,9 @@ server.registerTool( let doc; if (content === undefined || content === null) { doc = undefined; - } else if (typeof content === "string") { - try { - doc = JSON.parse(content); - } catch { - throw new Error("content was a string but not valid JSON"); - } } else { - doc = content; + // String -> JSON.parse (throwing on invalid); object passes through. + doc = parseNodeArg(content, "content was a string but not valid JSON"); } const result = await docmostClient.updatePageJson(pageId, doc, title); return jsonContent(result); @@ -529,16 +525,7 @@ server.registerTool( }, }, async ({ pageId, nodeId, node }) => { - let parsedNode; - if (typeof node === "string") { - try { - parsedNode = JSON.parse(node); - } catch { - throw new Error("node was a string but not valid JSON"); - } - } else { - parsedNode = node; - } + const parsedNode = parseNodeArg(node); const result = await docmostClient.patchNode(pageId, nodeId, parsedNode); return jsonContent(result); }, @@ -581,16 +568,7 @@ server.registerTool( }, }, async ({ pageId, node, position, anchorNodeId, anchorText }) => { - let parsedNode; - if (typeof node === "string") { - try { - parsedNode = JSON.parse(node); - } catch { - throw new Error("node was a string but not valid JSON"); - } - } else { - parsedNode = node; - } + const parsedNode = parseNodeArg(node); const result = await docmostClient.insertNode(pageId, parsedNode, { position, anchorNodeId, diff --git a/packages/mcp/src/lib/parse-node-arg.ts b/packages/mcp/src/lib/parse-node-arg.ts new file mode 100644 index 00000000..2e97da42 --- /dev/null +++ b/packages/mcp/src/lib/parse-node-arg.ts @@ -0,0 +1,17 @@ +// The model sometimes serializes a ProseMirror node arg as a JSON string +// instead of an object. Normalize: parse a string to an object (throwing on +// invalid JSON), pass an object through unchanged. Shared by patch_node / +// insert_node (and the analogous update_page_json content parsing). +export function parseNodeArg( + node: unknown, + errMsg = "node was a string but not valid JSON", +): unknown { + if (typeof node === "string") { + try { + return JSON.parse(node); + } catch { + throw new Error(errMsg); + } + } + return node; +} diff --git a/packages/mcp/test/unit/parse-node-arg.test.mjs b/packages/mcp/test/unit/parse-node-arg.test.mjs new file mode 100644 index 00000000..c24cdd54 --- /dev/null +++ b/packages/mcp/test/unit/parse-node-arg.test.mjs @@ -0,0 +1,32 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { parseNodeArg } from "../../build/lib/parse-node-arg.js"; + +test("parseNodeArg passes an object through unchanged", () => { + const obj = { type: "paragraph", content: [] }; + assert.strictEqual(parseNodeArg(obj), obj); +}); + +test("parseNodeArg passes undefined/null through unchanged", () => { + assert.strictEqual(parseNodeArg(undefined), undefined); + assert.strictEqual(parseNodeArg(null), null); +}); + +test("parseNodeArg parses a valid JSON string", () => { + const parsed = parseNodeArg('{"type":"paragraph"}'); + assert.deepStrictEqual(parsed, { type: "paragraph" }); +}); + +test("parseNodeArg throws the default message on invalid JSON string", () => { + assert.throws(() => parseNodeArg("{not json"), { + message: "node was a string but not valid JSON", + }); +}); + +test("parseNodeArg throws a custom message on invalid JSON string", () => { + assert.throws( + () => parseNodeArg("{not json", "content was a string but not valid JSON"), + { message: "content was a string but not valid JSON" }, + ); +}); From c7f0b5138947b9587ccba7c09e0a3134b1038433 Mon Sep 17 00:00:00 2001 From: claude_code Date: Sun, 21 Jun 2026 14:30:37 +0300 Subject: [PATCH 2/2] fix(ai-chat): keep tool-duplication backlog doc; fix parseNodeArg comment Pre-merge review follow-up for the parseNodeArg dedupe (PR #114): - Restore docs/backlog/ai-chat-tool-definitions-duplicated.md instead of deleting it: it still tracks open debt (unified spec registry + ProseMirror <-> Markdown converter unification) that this branch defers, and docs/git-sync-plan.md links to its converter section. Mark the node-arg quirk as done and add a Progress section. - Reword the in-app helper header from "byte-for-byte" to "behaviorally identical": the two copies differ in comments/quote style; only the logic, throw messages and branch order match. Co-Authored-By: Claude Opus 4.8 --- .../src/core/ai-chat/tools/parse-node-arg.ts | 6 +- .../ai-chat-tool-definitions-duplicated.md | 127 ++++++++++++++++++ 2 files changed, 131 insertions(+), 2 deletions(-) create mode 100644 docs/backlog/ai-chat-tool-definitions-duplicated.md diff --git a/apps/server/src/core/ai-chat/tools/parse-node-arg.ts b/apps/server/src/core/ai-chat/tools/parse-node-arg.ts index 6f43625d..e4495c45 100644 --- a/apps/server/src/core/ai-chat/tools/parse-node-arg.ts +++ b/apps/server/src/core/ai-chat/tools/parse-node-arg.ts @@ -3,8 +3,10 @@ // invalid JSON), pass an object through unchanged. Shared by patchNode / // insertNode (and the analogous updatePageJson content parsing). // -// This mirrors `packages/mcp/src/lib/parse-node-arg.ts` byte-for-byte. We -// cannot import that helper here: `@docmost/mcp` is ESM-only and this server +// This is behaviorally identical to `packages/mcp/src/lib/parse-node-arg.ts` +// (the function logic, default/explicit throw messages and branch order match; +// only comments and quote style differ). We cannot import that helper here: +// `@docmost/mcp` is ESM-only and this server // compiles with module:commonjs, so it is loaded at runtime via the // `new Function('import()')` trick (see docmost-client.loader.ts). Sharing // runtime code across that ESM/CJS boundary by a normal import is impossible, diff --git a/docs/backlog/ai-chat-tool-definitions-duplicated.md b/docs/backlog/ai-chat-tool-definitions-duplicated.md new file mode 100644 index 00000000..444628e8 --- /dev/null +++ b/docs/backlog/ai-chat-tool-definitions-duplicated.md @@ -0,0 +1,127 @@ +# Дублирование определений инструментов: in-app агент vs standalone MCP-пакет + +Статус: **частично закрыто.** Квирк «node как объект ИЛИ JSON-строка» вынесен +в общий хелпер `parseNodeArg` (см. «Прогресс» ниже); остальной долг (единый +реестр спеков + унификация конвертера) всё ещё открыт. Это forward-looking +стоимость поддержки, НЕ баг — код корректен сегодня. Держим запись открытой, +чтобы при росте набора инструментов долг не разъезжался молча. + +## Прогресс + +- ✅ **Квирк node-arg вынесен в хелпер** (`refactor/ai-chat-tool-spec-registry`, + PR #114). Шесть рукописных копий нормализации «node как объект ИЛИ + JSON-строка» свёрнуты в `parseNodeArg`: по одному источнику на пакет — + `packages/mcp/src/lib/parse-node-arg.ts` (standalone) и + `apps/server/src/core/ai-chat/tools/parse-node-arg.ts` (in-app). Две копии + намеренны (ESM/CJS-граница), поведение тождественно. +- ⏳ **Единый реестр спеков** (схема + описание на инструмент) и **вывод + `DocmostClientLike` из реального типа** — отложены (см. «Фикс»): требуют + пересечения ESM/CJS-границы для данных+zod и ломают тест-стабы in-app + инструментов при точных типах. Делать инкрементально. +- ⏳ **Унификация конвертера ProseMirror ↔ Markdown** — открыта (см. раздел + «Расширение …» ниже); на неё опирается план git-синка + (`docs/git-sync-plan.md`). + +## Суть + +Один и тот же набор инструментов поверх одного `DocmostClient` описан +**тремя независимыми рукописными слоями**. Каждое добавление инструмента или +правка его model-facing описания требует синхронной правки в 2–3 местах, а +parity-баги (расхождение копий) приходится чинить/переоткрывать дважды. + +## Где дублируется (три слоя) + +1. **Standalone MCP-сервер** — `packages/mcp/src/index.ts` (~38 `registerTool`). + Для внешних MCP-клиентов (stdio/http). На каждый инструмент: zod-схема + + длинное model-facing описание + тонкий `execute`, вызывающий `DocmostClient`. +2. **Встроенный AI-чат** — `apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts` + (~39 `tool({...})` через `ai`-SDK). Своя zod-схема + своё описание + свой + `execute` поверх ТОГО ЖЕ клиента (`@docmost/mcp` грузится в + `tools/docmost-client.loader.ts:188` через динамический `import()`). +3. **Ручная копия сигнатур** — интерфейс `DocmostClientLike` в + `apps/server/src/core/ai-chat/tools/docmost-client.loader.ts:9` (в комментарии + прямо: «Signatures here mirror that file exactly»), скопирован руками из + `packages/mcp/src/client.ts`. + +## Что именно продублировано (с подтверждением по коду) + +- **zod-схема + описание** каждого инструмента — в слоях 1 и 2 целиком. +- ~~**Квирк «node как объект ИЛИ JSON-строка»** реализован дважды (НЕ в общем + клиенте)~~ — **закрыто (PR #114):** вынесен в `parseNodeArg` (по хелперу на + пакет), 6 inline-копий устранены: + - in-app: `patchNode`, `insertNode`, `updatePageJson` → + `apps/server/src/core/ai-chat/tools/parse-node-arg.ts`; + - standalone: `patch_node`, `insert_node`, `update_page_json` → + `packages/mcp/src/lib/parse-node-arg.ts`. +- **Guardrail/семантика `transformPage` (dryRun)** описана в обоих: + `ai-chat-tools.service.ts:~935` и `index.ts:~1006`. + +## Почему разделение слоёв 1 и 2 само по себе оправдано + +У путей разный транспорт и auth-контекст, и это правильно держать раздельно: +in-app путь чеканит per-user JWT + provenance collab-токен (подписанная +agent-claim, `docmost-client.loader.ts:159` — `getCollabToken`; см. план §6.5), +а standalone обслуживает внешних клиентов по stdio/http. **Но** это оправдывает +два тонких адаптера (`execute` + auth-обвязка), а НЕ две рукописные копии +МЕТАДАННЫХ (схема + описание + квирки). Метаданные можно объявить один раз и +переиспользовать обоими транспортами. + +## Доказательство стоимости (наблюдалось при фиксе edit_page_text) + +При исправлении ложного «успеха» `edit_page_text` (refuse форматных правок + +`verify`-отчёт): +- **Поведение** легло в общий `DocmostClient` → автоматически дошло до обоих + агентов ОДНОЙ правкой. Это «хороший» случай — логика в едином источнике. +- **Описание** инструмента пришлось править ДВАЖДЫ: в `index.ts` (кодером) и + отдельно в `ai-chat-tools.service.ts:617`, где описание продолжало рекламировать + «Markdown wrappers tolerated via strip-and-retry» — ровно ту формулировку, что + ввела исходного агента в заблуждение. Копия молча разъехалась и какое-то время + встроенный агент получал устаревшую подсказку. Это и есть материализованный + parity-баг. + +## Расширение: дублируется не только описания инструментов — ещё и конвертер (PM ↔ Markdown) + +Зафиксировано при планировании встраивания git-синка (`docmost-sync` → gitmost, +нативная in-process интеграция). Та же болезнь «несколько рукописных копий одного +кода» теперь касается слоя конвертации ProseMirror ↔ Markdown и его lib, а не +только метаданных инструментов. + +- **Копия в gitmost** — `packages/mcp/src/lib/`: `markdown-converter.ts` (~885 + строк), `markdown-document.ts` (~136), `node-ops.ts`, `diff.ts`, + `docmost-schema.ts`. Канонизатора (`canonicalize.ts`) здесь НЕТ. +- **Копия в docmost-sync** — `packages/docmost-client/src/lib/`: тот же набор + + `canonicalize.ts` (~11 КБ, держит идемпотентность round-trip, SPEC §11) + + `markdown-document.ts` с режимом «тело + якоря, без тредов комментов» + (`includeCommentThreads:false`, на ~20 строк больше). +- **Третья копия (планируется)** — план git-синка вендорит чистую часть + конвертера в новый `packages/git-sync` (collab-файл не нужен: запись идёт + нативно через `openDirectConnection` + `@docmost/editor-ext`). + +Копии уже молча разъехались (docmost-sync vs `packages/mcp`): `collaboration.ts` +~329 изменённых строк, `node-ops.ts` ~53, `markdown-converter.ts` ~24, +`markdown-document.ts` ~20. Отдельно: `docmost-schema.ts` в lib дублирует +**реальную** схему сервера `@docmost/editor-ext` (её использует collab/persistence) +— расхождение схем = риск битой конвертации нод. + +Вывод: тот же фикс-вектор (единый источник правды), что и для инструментов, стоит +распространить на конвертер — общий пакет конвертации, потребляемый `mcp`, +`git-sync` и (в идеале) сервером. До конвергенции git-sync держит вендоренную +копию валидированного конвертера с гейтом round-trip против схемы `editor-ext` +(осознанный долг «третья копия сейчас, объединяем позже»). + +## Фикс + +Единый реестр спеков (полное устранение дублирования).** Вынести в + `packages/mcp` один источник на инструмент: `name` + zod-схема + model-facing + описание + общий хелпер нормализации node-строки (для patch/insert/update). + И `index.ts`, и `ai-chat-tools.service.ts` импортируют спеки и добавляют только + свой `execute`/auth. `DocmostClientLike` — выводить из типа реального клиента + (type-only import / генерация), а не копировать руками. + - Ограничение: `@docmost/mcp` — ESM-only, сервер грузит его через трюк + `new Function('import(specifier)')` (`docmost-client.loader.ts:174`), потому + что `module:commonjs` даунлевелит `import()` в `require()`. Реестр спеков + (данные + zod) должен пересекать ту же ESM/CJS-границу — выполнимо тем же + динамическим импортом; `ai`-SDK `tool()` и MCP `registerTool()` имеют разную + форму, поэтому реестр экспортирует транспорт-агностичные `{name, schema, + description}`, а каждая сторона оборачивает их сама. `zod` — общая зависимость + обоих пакетов, типы переносятся.