test(review): close the two test-coverage gaps from PR #185 auto-review

Approve-with-comments auto-review (8 axes); no blockers. Closes the two flagged
test gaps; the two forward-looking dedup suggestions (reconcileHasChildren helper;
unifying reconcileChildren/mergeRootTrees) are non-blocking architecture notes and
left for a follow-up (as with #186's forward-looking point).

1. Ambiguous-id refusal end-to-end (#159): the patch_node/delete_node guard
   `if (replaced/deleted !== 1) return null` was only covered in pieces — the
   replaceNodeById/deleteNodeById counts and assertUnambiguousMatch in isolation —
   so loosening the guard would not have failed a test. New mock test stands up a
   REAL Hocuspocus collab server seeded (via buildYDoc, same docmost extensions)
   with a two-blocks-one-id document and drives the real client methods: both must
   reject with /ambiguous/ AND never write to collab. Tracked via Hocuspocus
   onChange (fires synchronously per update, unlike the debounced onStoreDocument)
   so a clobbering write is actually observed — verified the test FAILS when the
   guard is loosened to `< 1`.

2. scrollToReference zero-match bail: the branch "non-empty id but querySelectorAll
   returns 0 -> matches[index] ?? matches[0] is undefined -> return false" (the real
   desync: definition present, inline ref removed from the DOM) was uncovered. Added
   a footnote.test.ts case: a definition for 'ghost' with no rendered ref -> false,
   no scroll.

Verified: 313 mcp tests + 24 editor-ext footnote tests; prettier clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

packages/editor-ext/src/lib/footnote/footnote.test.ts

@@ -267,6 +267,55 @@ describe('scrollToReference command (occurrence selection + fallback)', () => {
       (Element.prototype as any).scrollIntoView = original;
     }
   });
+
+  // #185 auto-review pt 2: a NON-empty id that renders ZERO references — the real
+  // desync where the definition still exists but its inline ref was removed from
+  // the DOM. querySelectorAll returns 0 matches, so `matches[index] ?? matches[0]`
+  // is undefined and the command must bail with `false` (not throw, not scroll).
+  it('returns false for a non-empty id with no rendered references', () => {
+    const scrolled: Element[] = [];
+    const original = (Element.prototype as any).scrollIntoView;
+    (Element.prototype as any).scrollIntoView = function () {
+      scrolled.push(this as Element);
+    };
+    try {
+      // A lone definition for id 'ghost' and a reference for a DIFFERENT id, so
+      // there is a footnotes structure but no `sup[data-id="ghost"]` in the DOM.
+      const editor = makeEditor({
+        type: 'doc',
+        content: [
+          {
+            type: 'paragraph',
+            content: [
+              { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'other' } },
+            ],
+          },
+          {
+            type: FOOTNOTES_LIST_NAME,
+            content: [
+              {
+                type: FOOTNOTE_DEFINITION_NAME,
+                attrs: { id: 'ghost' },
+                content: [{ type: 'paragraph' }],
+              },
+            ],
+          },
+        ],
+      });
+      expect(
+        editor.view.dom.querySelectorAll(
+          'sup[data-footnote-ref][data-id="ghost"]',
+        ).length,
+      ).toBe(0);
+
+      expect(editor.commands.scrollToReference('ghost')).toBe(false);
+      expect(scrolled.length).toBe(0);
+
+      editor.destroy();
+    } finally {
+      (Element.prototype as any).scrollIntoView = original;
+    }
+  });
 });
 
 describe('setFootnote command', () => {

packages/mcp/test/mock/ambiguous-node-id.test.mjs

@@ -0,0 +1,165 @@
+// Mock collab regression for the AMBIGUOUS-id refusal in patch_node / delete_node
+// (#159, PR #185 review pt 1). When a page has TWO blocks sharing one attrs.id
+// (Docmost duplicates block ids on copy/paste), the transform's
+// `if (replaced !== 1) return null` / `if (deleted !== 1) return null` guard must
+// SKIP the collab write, and the call must then reject with an "ambiguous" error.
+//
+// The replaceNodeById/deleteNodeById counts and assertUnambiguousMatch are unit-
+// tested in isolation (test/unit/node-ops.test.mjs); this exercises the END-TO-END
+// wiring through the real client method + a live Hocuspocus collab doc, so a
+// regression that loosened the guard (e.g. back to `=== 0`) would be caught here
+// where the isolated unit tests would not.
+//
+// Unlike the other mock tests (which deliberately avoid the collab WebSocket), this
+// one DOES stand up a real Hocuspocus server seeded with a duplicate-id document,
+// so the transform actually runs against a live two-match doc.
+import { test, after } from "node:test";
+import assert from "node:assert/strict";
+import http from "node:http";
+import { WebSocketServer } from "ws";
+import { Hocuspocus } from "@hocuspocus/server";
+import { DocmostClient } from "../../build/client.js";
+import { buildYDoc } from "../../build/lib/collaboration.js";
+
+// A document with TWO paragraphs sharing the SAME attrs.id — the duplicate-id
+// shape replaceNodeById/deleteNodeById report as `count === 2` (ambiguous).
+const DUP_ID = "dup-block-id";
+function seedDoc() {
+  return {
+    type: "doc",
+    content: [
+      {
+        type: "paragraph",
+        attrs: { id: DUP_ID },
+        content: [{ type: "text", text: "first copy" }],
+      },
+      {
+        type: "paragraph",
+        attrs: { id: DUP_ID },
+        content: [{ type: "text", text: "second copy" }],
+      },
+    ],
+  };
+}
+
+// Stand up an HTTP server that authenticates + hands out a collab token AND
+// upgrades /collab to a Hocuspocus instance seeded with the duplicate-id doc.
+// `state.changed` flips true the instant Hocuspocus applies ANY client document
+// update — it must stay false, proving the ambiguous write was never sent. (We
+// track onChange, which fires synchronously per update, NOT onStoreDocument,
+// which is debounced and would not fire before the test tears the server down —
+// making a real clobbering write look clean.)
+async function spawnCollabStack() {
+  const state = { changed: false };
+
+  const hocuspocus = new Hocuspocus({
+    quiet: true,
+    // Seed every requested document with a fresh duplicate-id Y.Doc, encoded with
+    // the SAME docmost extensions the client reads with (so attrs.id round-trips).
+    async onLoadDocument() {
+      return buildYDoc(seedDoc());
+    },
+    // Fires immediately on any client-driven document update. A real (clobbering)
+    // write would trip this; the ambiguous guard must keep it from firing.
+    async onChange() {
+      state.changed = true;
+    },
+  });
+
+  const wss = new WebSocketServer({ noServer: true });
+
+  const server = http.createServer((req, res) => {
+    let raw = "";
+    req.on("data", (c) => (raw += c));
+    req.on("end", () => {
+      if (req.url === "/api/auth/login") {
+        res.writeHead(200, {
+          "Content-Type": "application/json",
+          "Set-Cookie": "authToken=t; Path=/; HttpOnly",
+        });
+        res.end(JSON.stringify({ success: true }));
+        return;
+      }
+      if (req.url === "/api/auth/collab-token") {
+        res.writeHead(200, { "Content-Type": "application/json" });
+        res.end(JSON.stringify({ data: { token: "collab-jwt" } }));
+        return;
+      }
+      res.writeHead(404, { "Content-Type": "application/json" });
+      res.end(JSON.stringify({ message: "not found" }));
+    });
+  });
+
+  // buildCollabWsUrl maps http://host:port/api -> ws://host:port/collab.
+  server.on("upgrade", (request, socket, head) => {
+    if (!request.url || !request.url.startsWith("/collab")) {
+      socket.destroy();
+      return;
+    }
+    wss.handleUpgrade(request, socket, head, (ws) => {
+      hocuspocus.handleConnection(ws, request);
+    });
+  });
+
+  const baseURL = await new Promise((resolve) => {
+    server.listen(0, "127.0.0.1", () => {
+      const { port } = server.address();
+      resolve(`http://127.0.0.1:${port}/api`);
+    });
+  });
+
+  openStacks.push({ server, hocuspocus });
+  return { state, baseURL };
+}
+
+const openStacks = [];
+after(async () => {
+  await Promise.all(
+    openStacks.map(
+      ({ server, hocuspocus }) =>
+        new Promise((resolve) => {
+          server.close(() => {
+            Promise.resolve(hocuspocus.destroy?.()).finally(resolve);
+          });
+        }),
+    ),
+  );
+});
+
+test("patch_node REFUSES an ambiguous (duplicate) id without writing to collab", async () => {
+  const { state, baseURL } = await spawnCollabStack();
+  const client = new DocmostClient(baseURL, "user@example.com", "pw");
+
+  await assert.rejects(
+    () =>
+      client.patchNode("page-1", DUP_ID, {
+        type: "paragraph",
+        content: [{ type: "text", text: "replacement" }],
+      }),
+    /ambiguous/i,
+    "patch_node must reject a duplicate-id target with an 'ambiguous' error",
+  );
+
+  assert.equal(
+    state.changed,
+    false,
+    "the collab document must NEVER be written when the id is ambiguous",
+  );
+});
+
+test("delete_node REFUSES an ambiguous (duplicate) id without writing to collab", async () => {
+  const { state, baseURL } = await spawnCollabStack();
+  const client = new DocmostClient(baseURL, "user@example.com", "pw");
+
+  await assert.rejects(
+    () => client.deleteNode("page-2", DUP_ID),
+    /ambiguous/i,
+    "delete_node must reject a duplicate-id target with an 'ambiguous' error",
+  );
+
+  assert.equal(
+    state.changed,
+    false,
+    "the collab document must NEVER be written when the id is ambiguous",
+  );
+});