From 364838d0b2dad504d036b7dec1e584ec7df8160b Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 12:39:18 +0300 Subject: [PATCH] test(review): close the two test-coverage gaps from PR #185 auto-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/lib/footnote/footnote.test.ts | 49 ++++++ .../mcp/test/mock/ambiguous-node-id.test.mjs | 165 ++++++++++++++++++ 2 files changed, 214 insertions(+) create mode 100644 packages/mcp/test/mock/ambiguous-node-id.test.mjs diff --git a/packages/editor-ext/src/lib/footnote/footnote.test.ts b/packages/editor-ext/src/lib/footnote/footnote.test.ts index d539d832..5c510f43 100644 --- a/packages/editor-ext/src/lib/footnote/footnote.test.ts +++ b/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', () => { diff --git a/packages/mcp/test/mock/ambiguous-node-id.test.mjs b/packages/mcp/test/mock/ambiguous-node-id.test.mjs new file mode 100644 index 00000000..d29add0a --- /dev/null +++ b/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", + ); +});