refactor(ai-chat): dedupe node-arg JSON normalization into a shared helper

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 <noreply@anthropic.com>

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);
         },

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');
+  });
+});

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;
+}

docs/backlog/ai-chat-tool-definitions-duplicated.md

@@ -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` — общая зависимость
-    обоих пакетов, типы переносятся.

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,

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;
+}

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,

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;
+}

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" },
+  );
+});