refactor(review): address PR #185 review (lease leak, tests, changelog, jsonb seam)

8-point multi-aspect review of the batch PR; security/regressions were clean. 1. Lease leak: the #180 reorder moved `toolsFor` (which leases external MCP clients, refCount+1) ahead of buildSystemPrompt + forUser, but the only release (closeExternalClients) was bound to the streamText callbacks. A throw in between leaked the lease (refCount stuck, undici sockets held until restart). Define closeExternalClients right after the lease and wrap buildSystemPrompt+forUser in try/catch that closes-then-rethrows. 2. Cover the patch_node/delete_node dup-id refusal (#159 #6): extract the guard into a pure `assertUnambiguousMatch` (node-ops) and unit-test 0/1/>1. 3. Regress the body-before-title order (#159 #10): mock-HTTP test (collab fails fast against a server with no WS upgrade) asserts /pages/update (title) is NEVER posted when the body write fails — for updatePage AND updatePageJson. 4. CHANGELOG [Unreleased]: #180, #168 (Added); #163 (Fixed). 5. Add the missing en-US i18n keys (Back to references / {{label}}). 6. Drop the duplicate content/empty/blank cases in ai-chat.prompt.spec.ts (they repeat the buildMcpToolingBlock unit tests); keep only sandwich placement + both-safety-copies. 7. CI Postgres pg16 -> pg18 (match docker-compose). 8. jsonb decode seam: shared `parseJsonbValue(value, guard)` in database/utils.ts holds the legacy double-encoding self-heal in one place; parseToolAllowlist / parseModelConfig keep only a type-guard. Verified: server build + 124 unit + 15 integration; mcp 311; prettier clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-25 11:35:19 +03:00
parent 8218c1a8ef
commit f80276d41a
14 changed files with 342 additions and 153 deletions
--- a/packages/mcp/build/client.js
+++ b/packages/mcp/build/client.js
@@ -11,7 +11,7 @@ import { updatePageContentRealtime, replacePageContent, markdownToProseMirror, m
 import { footnoteWarningsField } from "./lib/footnote-analyze.js";
 import { buildPageTree } from "./lib/tree.js";
 import { serializeDocmostMarkdown, parseDocmostMarkdown, } from "./lib/markdown-document.js";
-import { replaceNodeById, deleteNodeById, insertNodeRelative, buildOutline, getNodeByRef, readTable, insertTableRow, deleteTableRow, updateTableCell, } from "./lib/node-ops.js";
+import { replaceNodeById, deleteNodeById, assertUnambiguousMatch, insertNodeRelative, buildOutline, getNodeByRef, readTable, insertTableRow, deleteTableRow, updateTableCell, } from "./lib/node-ops.js";
 import { withPageLock } from "./lib/page-lock.js";
 import { applyTextEdits, } from "./lib/json-edit.js";
 import { getCollabToken, performLogin } from "./lib/auth-utils.js";
@@ -1331,12 +1331,9 @@ export class DocmostClient {
                return null;
            return nd;
        });
-        if (replaced === 0) {
-            throw new Error(`patch_node: no node with id "${nodeId}" found on page ${pageId}`);
-        }
-        if (replaced > 1) {
-            throw new Error(`patch_node: id "${nodeId}" is ambiguous — ${replaced} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to replace all of them; nothing was changed. Re-target with a more specific anchor.`);
-        }
+        // 0 -> "no node"; >1 -> "ambiguous, refused" (the transform already skipped
+        // the write for any count !== 1). Single shared guard (#159, #185 review).
+        assertUnambiguousMatch("patch_node", "replace", replaced, nodeId, pageId);
        return { success: true, replaced, nodeId, verify: mutation.verify };
    }
    /**
@@ -1428,12 +1425,9 @@ export class DocmostClient {
                return null;
            return nd;
        });
-        if (deleted === 0) {
-            throw new Error(`delete_node: no node with id "${nodeId}" found on page ${pageId}`);
-        }
-        if (deleted > 1) {
-            throw new Error(`delete_node: id "${nodeId}" is ambiguous — ${deleted} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to delete all of them; nothing was changed. Re-target with a more specific anchor.`);
-        }
+        // 0 -> "no node"; >1 -> "ambiguous, refused" (the transform already skipped
+        // the write for any count !== 1). Single shared guard (#159, #185 review).
+        assertUnambiguousMatch("delete_node", "delete", deleted, nodeId, pageId);
        return { success: true, deleted, nodeId, verify: mutation.verify };
    }
    /** Build the public share URL for a page. */
--- a/packages/mcp/build/lib/node-ops.js
+++ b/packages/mcp/build/lib/node-ops.js
@@ -77,11 +77,13 @@ export function buildOutline(doc) {
        const entry = {
            index: i,
            type,
-            id: isObject(block) && isObject(block.attrs) ? block.attrs.id ?? null : null,
+            id: isObject(block) && isObject(block.attrs)
+                ? (block.attrs.id ?? null)
+                : null,
            firstText: truncate(blockPlainText(block), 100),
        };
        if (type === "heading") {
-            entry.level = isObject(block.attrs) ? block.attrs.level ?? null : null;
+            entry.level = isObject(block.attrs) ? (block.attrs.level ?? null) : null;
        }
        else if (type === "table") {
            const headerRow = block.content?.[0]?.content ?? [];
@@ -205,6 +207,22 @@ export function deleteNodeById(doc, nodeId) {
    }
    return { doc: out, deleted };
 }
+/**
+ * Throw a clear, model-actionable error when a node-id write op did NOT match
+ * exactly one node (#159). `count === 0` -> "no node found"; `count > 1` ->
+ * "ambiguous, refused" — Docmost duplicates block ids on copy/paste, so a write
+ * by id could clobber/remove EVERY duplicate. The caller skips the write for any
+ * `count !== 1` (the transform returns null), so this only REPORTS; nothing was
+ * changed. No-op for the unambiguous single-match case.
+ */
+export function assertUnambiguousMatch(op, verb, count, nodeId, pageId) {
+    if (count === 0) {
+        throw new Error(`${op}: no node with id "${nodeId}" found on page ${pageId}`);
+    }
+    if (count > 1) {
+        throw new Error(`${op}: id "${nodeId}" is ambiguous — ${count} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to ${verb} all of them; nothing was changed. Re-target with a more specific anchor.`);
+    }
+}
 /**
 * Deep-clone `doc` and strip every node/mark attribute whose value is strictly
 * `undefined`, so the result is safe to hand to Yjs (which throws an opaque
@@ -655,7 +673,7 @@ export function readTable(doc, tableRef) {
                ? cellNode.content[0]
                : undefined;
            const id = isObject(firstPara) && isObject(firstPara.attrs)
-                ? firstPara.attrs.id ?? null
+                ? (firstPara.attrs.id ?? null)
                : null;
            rowIds.push(id);
        }
@@ -683,7 +701,9 @@ export function insertTableRow(doc, tableRef, cells, index) {
        table.content = [];
    const rows = table.content.length;
    const headerRow = table.content[0];
-    const headerCells = Array.isArray(headerRow?.content) ? headerRow.content : [];
+    const headerCells = Array.isArray(headerRow?.content)
+        ? headerRow.content
+        : [];
    // Column count is the WIDEST existing row, so the guard below stays
    // meaningful for ragged tables and the new row matches the table's width.
    // Fall back to the supplied cell count only when the table has no rows.
@@ -699,7 +719,10 @@ export function insertTableRow(doc, tableRef, cells, index) {
    }
    // Resolve the landing index up front so the cell-type decision and the splice
    // below agree: a valid integer in [0, rows] splices there, else we append.
-    const landingIndex = typeof index === "number" && Number.isInteger(index) && index >= 0 && index <= rows
+    const landingIndex = typeof index === "number" &&
+        Number.isInteger(index) &&
+        index >= 0 &&
+        index <= rows
        ? index
        : rows;
    // Seed the id generator with every id already in the doc so the new cell
@@ -717,7 +740,7 @@ export function insertTableRow(doc, tableRef, cells, index) {
        // A row landing at index 0 becomes the new header row, so inherit the
        // current header cell's type per column (Docmost uses "tableHeader" there);
        // every other position is a plain data cell.
-        const cellType = landingIndex === 0 ? headerCells[i]?.type ?? "tableCell" : "tableCell";
+        const cellType = landingIndex === 0 ? (headerCells[i]?.type ?? "tableCell") : "tableCell";
        newCells.push({
            type: cellType,
            attrs,
--- a/packages/mcp/src/client.ts
+++ b/packages/mcp/src/client.ts
@@ -32,6 +32,7 @@ import {
 import {
  replaceNodeById,
  deleteNodeById,
+  assertUnambiguousMatch,
  insertNodeRelative,
  buildOutline,
  getNodeByRef,
@@ -1668,16 +1669,9 @@ export class DocmostClient {
      },
    );

-    if (replaced === 0) {
-      throw new Error(
-        `patch_node: no node with id "${nodeId}" found on page ${pageId}`,
-      );
-    }
-    if (replaced > 1) {
-      throw new Error(
-        `patch_node: id "${nodeId}" is ambiguous — ${replaced} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to replace all of them; nothing was changed. Re-target with a more specific anchor.`,
-      );
-    }
+    // 0 -> "no node"; >1 -> "ambiguous, refused" (the transform already skipped
+    // the write for any count !== 1). Single shared guard (#159, #185 review).
+    assertUnambiguousMatch("patch_node", "replace", replaced, nodeId, pageId);

    return { success: true, replaced, nodeId, verify: mutation.verify };
  }
@@ -1812,16 +1806,9 @@ export class DocmostClient {
      },
    );

-    if (deleted === 0) {
-      throw new Error(
-        `delete_node: no node with id "${nodeId}" found on page ${pageId}`,
-      );
-    }
-    if (deleted > 1) {
-      throw new Error(
-        `delete_node: id "${nodeId}" is ambiguous — ${deleted} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to delete all of them; nothing was changed. Re-target with a more specific anchor.`,
-      );
-    }
+    // 0 -> "no node"; >1 -> "ambiguous, refused" (the transform already skipped
+    // the write for any count !== 1). Single shared guard (#159, #185 review).
+    assertUnambiguousMatch("delete_node", "delete", deleted, nodeId, pageId);

    return { success: true, deleted, nodeId, verify: mutation.verify };
  }
--- a/packages/mcp/src/lib/node-ops.ts
+++ b/packages/mcp/src/lib/node-ops.ts
@@ -99,12 +99,15 @@ export function buildOutline(doc: any): OutlineEntry[] {
    const entry: OutlineEntry = {
      index: i,
      type,
-      id: isObject(block) && isObject(block.attrs) ? block.attrs.id ?? null : null,
+      id:
+        isObject(block) && isObject(block.attrs)
+          ? (block.attrs.id ?? null)
+          : null,
      firstText: truncate(blockPlainText(block), 100),
    };

    if (type === "heading") {
-      entry.level = isObject(block.attrs) ? block.attrs.level ?? null : null;
+      entry.level = isObject(block.attrs) ? (block.attrs.level ?? null) : null;
    } else if (type === "table") {
      const headerRow = block.content?.[0]?.content ?? [];
      entry.rows = block.content?.length ?? 0;
@@ -249,6 +252,33 @@ export function deleteNodeById(
  return { doc: out, deleted };
 }

+/**
+ * Throw a clear, model-actionable error when a node-id write op did NOT match
+ * exactly one node (#159). `count === 0` -> "no node found"; `count > 1` ->
+ * "ambiguous, refused" — Docmost duplicates block ids on copy/paste, so a write
+ * by id could clobber/remove EVERY duplicate. The caller skips the write for any
+ * `count !== 1` (the transform returns null), so this only REPORTS; nothing was
+ * changed. No-op for the unambiguous single-match case.
+ */
+export function assertUnambiguousMatch(
+  op: "patch_node" | "delete_node",
+  verb: "replace" | "delete",
+  count: number,
+  nodeId: string,
+  pageId: string,
+): void {
+  if (count === 0) {
+    throw new Error(
+      `${op}: no node with id "${nodeId}" found on page ${pageId}`,
+    );
+  }
+  if (count > 1) {
+    throw new Error(
+      `${op}: id "${nodeId}" is ambiguous — ${count} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to ${verb} all of them; nothing was changed. Re-target with a more specific anchor.`,
+    );
+  }
+}
+
 /**
 * Deep-clone `doc` and strip every node/mark attribute whose value is strictly
 * `undefined`, so the result is safe to hand to Yjs (which throws an opaque
@@ -644,7 +674,8 @@ function locateTable(
  if (!isObject(rootClone)) return null;

  // "#<n>": index into the top-level content array; must be a table.
-  const indexMatch = typeof tableRef === "string" ? tableRef.match(/^#(\d+)$/) : null;
+  const indexMatch =
+    typeof tableRef === "string" ? tableRef.match(/^#(\d+)$/) : null;
  if (indexMatch) {
    const index = Number(indexMatch[1]);
    const block = Array.isArray(rootClone.content)
@@ -744,7 +775,7 @@ export function readTable(
        : undefined;
      const id =
        isObject(firstPara) && isObject(firstPara.attrs)
-          ? firstPara.attrs.id ?? null
+          ? (firstPara.attrs.id ?? null)
          : null;
      rowIds.push(id);
    }
@@ -778,14 +809,17 @@ export function insertTableRow(
  if (!Array.isArray(table.content)) table.content = [];
  const rows = table.content.length;
  const headerRow = table.content[0];
-  const headerCells = Array.isArray(headerRow?.content) ? headerRow.content : [];
+  const headerCells = Array.isArray(headerRow?.content)
+    ? headerRow.content
+    : [];

  // Column count is the WIDEST existing row, so the guard below stays
  // meaningful for ragged tables and the new row matches the table's width.
  // Fall back to the supplied cell count only when the table has no rows.
  let colCount = 0;
  for (const r of table.content) {
-    if (isObject(r) && Array.isArray(r.content)) colCount = Math.max(colCount, r.content.length);
+    if (isObject(r) && Array.isArray(r.content))
+      colCount = Math.max(colCount, r.content.length);
  }
  if (colCount === 0) colCount = Array.isArray(cells) ? cells.length : 0;

@@ -798,7 +832,10 @@ export function insertTableRow(
  // Resolve the landing index up front so the cell-type decision and the splice
  // below agree: a valid integer in [0, rows] splices there, else we append.
  const landingIndex =
-    typeof index === "number" && Number.isInteger(index) && index >= 0 && index <= rows
+    typeof index === "number" &&
+    Number.isInteger(index) &&
+    index >= 0 &&
+    index <= rows
      ? index
      : rows;

@@ -817,7 +854,8 @@ export function insertTableRow(
    // A row landing at index 0 becomes the new header row, so inherit the
    // current header cell's type per column (Docmost uses "tableHeader" there);
    // every other position is a plain data cell.
-    const cellType = landingIndex === 0 ? headerCells[i]?.type ?? "tableCell" : "tableCell";
+    const cellType =
+      landingIndex === 0 ? (headerCells[i]?.type ?? "tableCell") : "tableCell";
    newCells.push({
      type: cellType,
      attrs,
@@ -889,9 +927,10 @@ export function updateTableCell(
  const rowNodes = Array.isArray(table.content) ? table.content : [];
  const rows = rowNodes.length;
  const rowNode = rowNodes[row];
-  const cols = isObject(rowNode) && Array.isArray(rowNode.content)
-    ? rowNode.content.length
-    : 0;
+  const cols =
+    isObject(rowNode) && Array.isArray(rowNode.content)
+      ? rowNode.content.length
+      : 0;

  if (
    !Number.isInteger(row) ||
--- a/packages/mcp/test/mock/write-order.test.mjs
+++ b/packages/mcp/test/mock/write-order.test.mjs
@@ -0,0 +1,106 @@
+// Mock-HTTP regression for the body-before-title write order (#159 finding #10,
+// PR #185 review pt 3). `updatePage` / `updatePageJson` must write the page BODY
+// (collab) BEFORE the title (REST POST /pages/update), so a failed body write
+// never leaves a NEW title over the OLD body (split-brain). We point the client
+// at a mock server that serves auth + collab-token but has NO WebSocket upgrade
+// handler, so the collab body write fails fast; we then assert the title was
+// never POSTed. With the pre-fix (title-first) order, /pages/update WOULD be hit
+// before the body failed.
+import { test, after } from "node:test";
+import assert from "node:assert/strict";
+import http from "node:http";
+import { DocmostClient } from "../../build/client.js";
+
+function readBody(req) {
+  return new Promise((resolve) => {
+    let raw = "";
+    req.on("data", (c) => (raw += c));
+    req.on("end", () => resolve(raw));
+  });
+}
+function startServer(handler) {
+  return new Promise((resolve) => {
+    const server = http.createServer(handler);
+    server.listen(0, "127.0.0.1", () => {
+      const { port } = server.address();
+      resolve({ server, baseURL: `http://127.0.0.1:${port}/api` });
+    });
+  });
+}
+function sendJson(res, status, obj, extraHeaders = {}) {
+  res.writeHead(status, {
+    "Content-Type": "application/json",
+    ...extraHeaders,
+  });
+  res.end(JSON.stringify(obj));
+}
+
+const openServers = [];
+async function spawn(handler) {
+  const { server, baseURL } = await startServer(handler);
+  openServers.push(server);
+  return { server, baseURL };
+}
+after(async () => {
+  await Promise.all(openServers.map((s) => new Promise((r) => s.close(r))));
+});
+
+// A mock server that authenticates and hands out a collab token, tracks whether
+// the title endpoint was hit, but has NO WS upgrade handler -> collab fails fast.
+function makeServer() {
+  const state = { titlePosted: false };
+  const handler = async (req, res) => {
+    await readBody(req);
+    if (req.url === "/api/auth/login") {
+      sendJson(
+        res,
+        200,
+        { success: true },
+        {
+          "Set-Cookie": "authToken=t; Path=/; HttpOnly",
+        },
+      );
+      return;
+    }
+    if (req.url === "/api/auth/collab-token") {
+      sendJson(res, 200, { data: { token: "collab-jwt" } });
+      return;
+    }
+    if (req.url === "/api/pages/update") {
+      state.titlePosted = true;
+      sendJson(res, 200, { data: {} });
+      return;
+    }
+    sendJson(res, 404, { message: "not found" });
+  };
+  return { state, handler };
+}
+
+test("updatePage does NOT POST the title when the body (collab) write fails (#159)", async () => {
+  const { state, handler } = makeServer();
+  const { baseURL } = await spawn(handler);
+  const client = new DocmostClient(baseURL, "u@e.com", "pw");
+
+  await assert.rejects(() =>
+    client.updatePage("page-1", "# Heading\n\nsome body", "New Title"),
+  );
+  assert.equal(
+    state.titlePosted,
+    false,
+    "title must NOT be posted when the body write failed (body-first order)",
+  );
+});
+
+test("updatePageJson does NOT POST the title when the body (collab) write fails (#159)", async () => {
+  const { state, handler } = makeServer();
+  const { baseURL } = await spawn(handler);
+  const client = new DocmostClient(baseURL, "u@e.com", "pw");
+
+  const doc = { type: "doc", content: [{ type: "paragraph" }] };
+  await assert.rejects(() => client.updatePageJson("page-1", doc, "New Title"));
+  assert.equal(
+    state.titlePosted,
+    false,
+    "title must NOT be posted when the body write failed (body-first order)",
+  );
+});
--- a/packages/mcp/test/unit/node-ops.test.mjs
+++ b/packages/mcp/test/unit/node-ops.test.mjs
@@ -5,6 +5,7 @@ import {
  blockPlainText,
  replaceNodeById,
  deleteNodeById,
+  assertUnambiguousMatch,
  insertNodeRelative,
 } from "../../build/lib/node-ops.js";

@@ -216,10 +217,7 @@ test("deleteNodeById removes EVERY node sharing the id", () => {
 });

 test("deleteNodeById does NOT mutate input (deep-equal snapshot)", () => {
-  const input = doc(
-    para("p-1", textNode("one")),
-    para("p-2", textNode("two")),
-  );
+  const input = doc(para("p-1", textNode("one")), para("p-2", textNode("two")));
  const snap = snapshot(input);
  const { doc: out } = deleteNodeById(input, "p-2");
  assert.deepEqual(input, snap);
@@ -487,3 +485,35 @@ test("insertNodeRelative truly-missing anchor still returns inserted:false", ()
  });
  assert.equal(inserted, false);
 });
+
+// assertUnambiguousMatch (#159, #185 review pt 2): the patch_node/delete_node
+// guard. Docmost duplicates block ids on copy/paste, so a write by id that
+// matches >1 node must be REFUSED (the caller already skipped the write for any
+// count !== 1; this reports the error). The duplicate COUNT itself is covered by
+// the replaceNodeById/deleteNodeById tests above (count===2 for a 2-dup doc).
+test("assertUnambiguousMatch: count 0 throws 'no node found'", () => {
+  assert.throws(
+    () => assertUnambiguousMatch("patch_node", "replace", 0, "n1", "p1"),
+    /patch_node: no node with id "n1" found on page p1/,
+  );
+});
+
+test("assertUnambiguousMatch: count > 1 refuses with an 'ambiguous' error", () => {
+  assert.throws(
+    () => assertUnambiguousMatch("patch_node", "replace", 2, "dup", "p1"),
+    /ambiguous.*Refusing to replace all of them; nothing was changed/,
+  );
+  assert.throws(
+    () => assertUnambiguousMatch("delete_node", "delete", 3, "dup", "p1"),
+    /ambiguous.*Refusing to delete all of them; nothing was changed/,
+  );
+});
+
+test("assertUnambiguousMatch: exactly one match does NOT throw", () => {
+  assert.doesNotThrow(() =>
+    assertUnambiguousMatch("patch_node", "replace", 1, "n1", "p1"),
+  );
+  assert.doesNotThrow(() =>
+    assertUnambiguousMatch("delete_node", "delete", 1, "n1", "p1"),
+  );
+});