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 33b4b86a..74ac66b8 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 @@ -13,6 +13,7 @@ import { type DocmostClientLike, } from './docmost-client.loader'; import { resolveCurrentPageResult } from './current-page.util'; +import { parseNodeArg } from './parse-node-arg'; /** * Per-user, per-request adapter that exposes Docmost READ operations to the @@ -705,14 +706,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); }, }), @@ -764,14 +758,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, @@ -820,14 +807,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..e4495c45 --- /dev/null +++ b/apps/server/src/core/ai-chat/tools/parse-node-arg.ts @@ -0,0 +1,26 @@ +// 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 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, +// 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 index 2b78dd9b..444628e8 100644 --- a/docs/backlog/ai-chat-tool-definitions-duplicated.md +++ b/docs/backlog/ai-chat-tool-definitions-duplicated.md @@ -1,8 +1,26 @@ # Дублирование определений инструментов: in-app агент vs standalone MCP-пакет -Статус: **зафиксировано в беклоге, код не менялся.** Это forward-looking -стоимость поддержки, НЕ баг — код корректен сегодня. Фиксируем, чтобы при -росте набора инструментов (см. §16) долг не разъезжался молча. +Статус: **частично закрыто.** Квирк «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`). ## Суть @@ -28,12 +46,13 @@ parity-баги (расхождение копий) приходится чин ## Что именно продублировано (с подтверждением по коду) - **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). +- ~~**Квирк «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`. 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" }, + ); +});