fix(mcp): refuse ambiguous patch_node/delete_node on duplicated ids (#159)

Docmost duplicates block ids on copy/paste, and copyPageContent writes the
source document verbatim with the same ids. `patchNode`/`deleteNode` address a
block by `attrs.id` via replaceNodeById/deleteNodeById, which act on EVERY node
sharing the id — so a single patch_node/delete_node could silently
replace/remove multiple unrelated blocks with no signal to the model
(red-team finding #6).

Guard both write paths: when more than one node matches the id, skip the write
entirely (the transform returns null -> no mutation) and throw a clear
"ambiguous id — N nodes share it" error so the model re-targets with a more
specific anchor. Only an unambiguous single match is written; the 0-match and
1-match behavior is unchanged.

The duplicate-count basis is covered by node-ops.test.mjs (replaceNodeById /
deleteNodeById report count===2 for a 2-duplicate doc). The end-to-end guard
is not unit-tested because patchNode/deleteNode require a live collab provider
and the test suite has no provider mock.

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

packages/mcp/build/client.js

@@ -1292,13 +1292,22 @@ export class DocmostClient {
             replaced = 0;
             const { doc: nd, replaced: r } = replaceNodeById(liveDoc, nodeId, target);
             replaced = r;
-            if (replaced === 0)
+            // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS: Docmost
-                return null; // no match -> skip the write entirely
+            // duplicates block ids on copy/paste (and copyPageContent writes them
+            // verbatim), so replacing "the node with id X" would silently clobber
+            // EVERY duplicate (#159). Refuse: skip the write and throw below so the
+            // model re-targets with a more specific anchor instead of corrupting the
+            // page. Only an unambiguous single match is written.
+            if (replaced !== 1)
+                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.`);
+        }
         return { success: true, replaced, nodeId, verify: mutation.verify };
     }
     /**
@@ -1381,13 +1390,21 @@ export class DocmostClient {
             deleted = 0;
             const { doc: nd, deleted: d } = deleteNodeById(liveDoc, nodeId);
             deleted = d;
-            if (deleted === 0)
+            // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS (block
-                return null; // no match -> skip the write entirely
+            // ids are duplicated on copy/paste, #159): deleting "the node with id X"
+            // would silently remove EVERY duplicate. Refuse: skip the write and throw
+            // below so the model re-targets. Only an unambiguous single match is
+            // deleted.
+            if (deleted !== 1)
+                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.`);
+        }
         return { success: true, deleted, nodeId, verify: mutation.verify };
     }
     /** Build the public share URL for a page. */

packages/mcp/src/client.ts

@@ -1625,7 +1625,13 @@ export class DocmostClient {
         replaced = 0;
         const { doc: nd, replaced: r } = replaceNodeById(liveDoc, nodeId, target);
         replaced = r;
-        if (replaced === 0) return null; // no match -> skip the write entirely
+        // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS: Docmost
+        // duplicates block ids on copy/paste (and copyPageContent writes them
+        // verbatim), so replacing "the node with id X" would silently clobber
+        // EVERY duplicate (#159). Refuse: skip the write and throw below so the
+        // model re-targets with a more specific anchor instead of corrupting the
+        // page. Only an unambiguous single match is written.
+        if (replaced !== 1) return null;
         return nd;
       },
     );
@@ -1635,6 +1641,11 @@ export class DocmostClient {
         `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.`,
+      );
+    }
 
     return { success: true, replaced, nodeId, verify: mutation.verify };
   }
@@ -1755,7 +1766,12 @@ export class DocmostClient {
         deleted = 0;
         const { doc: nd, deleted: d } = deleteNodeById(liveDoc, nodeId);
         deleted = d;
-        if (deleted === 0) return null; // no match -> skip the write entirely
+        // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS (block
+        // ids are duplicated on copy/paste, #159): deleting "the node with id X"
+        // would silently remove EVERY duplicate. Refuse: skip the write and throw
+        // below so the model re-targets. Only an unambiguous single match is
+        // deleted.
+        if (deleted !== 1) return null;
         return nd;
       },
     );
@@ -1765,6 +1781,11 @@ export class DocmostClient {
         `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.`,
+      );
+    }
 
     return { success: true, deleted, nodeId, verify: mutation.verify };
   }