From 3b80285d57a67b67793dce4a09ab4222a115ba17 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Tue, 30 Jun 2026 10:04:49 +0300 Subject: [PATCH] fix(#260): open MCP collab docs by canonical UUID (slugId doc-name split) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real root cause of the silent MCP edit loss: the web editor always opens the collaboration document by the page UUID (`page.${page.id}`), but the MCP opened it by the agent-supplied id — usually a slugId — so `page.${pageId}` became `page.`. For one DB page that is TWO independent Yjs documents; both persist to the same `pages` row (findById/updatePage resolve id or slugId), so the human tab's debounced store overwrites the agent edit (last-store-wins) — gone after reload, never shown live. The slugId doc also made the server's transclusion sync + embedding reindex throw Postgres 22P02. Fix: - MCP (primary): resolvePageId(pageId) returns the canonical UUID — a UUID short-circuits with no network call, a slugId resolves once via getPageRaw and is cached both ways. Every collab-write path (mutatePageContent / updatePageContentRealtime / replacePageContent and the mutate/replace/ unlocked seams) now opens by the resolved UUID, so the MCP and the editor share ONE Yjs doc. replaceImage's whole-operation page lock also keys on the UUID so it serializes against the other (now-UUID-keyed) writes. - Server (defense + kills the 22P02 noise): onStoreDocument passes the resolved page.id — not the raw doc-name id — to syncTransclusion, the embedding queue, the mention-notification job, addContributors, and the in-tx history read. Content store and the empty-guard are untouched. Tests: a new MCP test stands up a real Hocuspocus server and asserts a slugId input opens `page.` (never `page.`), with UUID short-circuit and single-resolve caching; the server spec asserts the side-effects receive the UUID for a `page.` doc. closes #260 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../extensions/persistence-store.spec.ts | 47 ++++ .../extensions/persistence.extension.ts | 20 +- packages/mcp/build/client.js | 126 ++++++++-- packages/mcp/src/client.ts | 130 ++++++++-- .../mcp/test/mock/ambiguous-node-id.test.mjs | 4 +- .../mock/insert-footnote-wrapper.test.mjs | 5 + .../resolve-page-id-collab-doc-name.test.mjs | 233 ++++++++++++++++++ packages/mcp/test/mock/write-order.test.mjs | 8 + 8 files changed, 528 insertions(+), 45 deletions(-) create mode 100644 packages/mcp/test/mock/resolve-page-id-collab-doc-name.test.mjs diff --git a/apps/server/src/collaboration/extensions/persistence-store.spec.ts b/apps/server/src/collaboration/extensions/persistence-store.spec.ts index 1f1b5728..e707290f 100644 --- a/apps/server/src/collaboration/extensions/persistence-store.spec.ts +++ b/apps/server/src/collaboration/extensions/persistence-store.spec.ts @@ -422,4 +422,51 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' expect(historyQueue.add).not.toHaveBeenCalled(); expect(aiQueue.add).not.toHaveBeenCalled(); }); + + // #260 — when the collab doc name carries a SLUGID (`page.`) the + // post-store side effects must use the resolved page.id (a UUID), NOT the + // slugId. The transclusion sync + embedding reindex write uuid-typed columns, + // so a slugId there threw Postgres 22P02; the contributors key must also match + // the PAGE_HISTORY job, which is enqueued with page.id. + it('uses the canonical page.id (not the slugId doc name) for post-store side effects (#260)', async () => { + const SLUG = 'slug-1'; // persistedHumanPage.slugId; findById resolves it + const document = ydocFor(doc('NEW AGENT CONTENT')); + pageRepo.findById.mockResolvedValue(persistedHumanPage('NEW AGENT CONTENT')); + pageHistoryRepo.findPageLastHistory.mockResolvedValue(null); + + // A `page.` document name (the bug's smoking gun), agent store over + // a human page so the in-tx history-boundary read is also exercised. + await ext.onStoreDocument({ + documentName: `page.${SLUG}`, + document, + context: { user: { id: USER_ID, name: 'Alice' }, actor: 'agent' }, + } as any); + + // findById was queried with the slugId (it resolves either id or slugId). + expect(pageRepo.findById).toHaveBeenCalledWith(SLUG, expect.anything()); + + // The in-tx history-boundary read uses the canonical UUID, never the slugId. + expect(pageHistoryRepo.findPageLastHistory).toHaveBeenCalledWith( + PAGE_ID, + expect.anything(), + ); + + // Transclusion sync (uuid-typed columns) must receive the UUID. + expect(transclusionService.syncPageTransclusions.mock.calls[0][0]).toBe( + PAGE_ID, + ); + expect(transclusionService.syncPageReferences.mock.calls[0][0]).toBe( + PAGE_ID, + ); + expect( + transclusionService.syncPageTemplateReferences.mock.calls[0][0], + ).toBe(PAGE_ID); + + // Embedding reindex job keyed by the UUID (slugId there threw 22P02). + expect(aiQueue.add).toHaveBeenCalledTimes(1); + expect(aiQueue.add.mock.calls[0][1].pageIds).toEqual([PAGE_ID]); + + // Contributors keyed by the UUID so they match the PAGE_HISTORY job (page.id). + expect(collabHistory.addContributors.mock.calls[0][0]).toBe(PAGE_ID); + }); }); diff --git a/apps/server/src/collaboration/extensions/persistence.extension.ts b/apps/server/src/collaboration/extensions/persistence.extension.ts index 5607dc85..18eb99e6 100644 --- a/apps/server/src/collaboration/extensions/persistence.extension.ts +++ b/apps/server/src/collaboration/extensions/persistence.extension.ts @@ -329,8 +329,10 @@ export class PersistenceExtension implements Extension { lastUpdatedSource === 'agent' && page.lastUpdatedSource !== 'agent' ) { + // pageHistory.pageId is uuid-typed; use page.id (never the doc-name + // slugId) so a `page.` doc cannot throw 22P02 here (#260). const lastHistory = await this.pageHistoryRepo.findPageLastHistory( - pageId, + page.id, { includeContent: true, trx }, ); const humanBaselineMissing = @@ -398,11 +400,16 @@ export class PersistenceExtension implements Extension { }), ); - await this.syncTransclusion(pageId, page.workspaceId, tiptapJson); + // Use the canonical page UUID (page.id), not the doc-name id, which may be + // a slugId for a `page.` doc (#260). The transclusion/reference + // syncs write uuid-typed columns, so a slugId here threw Postgres 22P02. + await this.syncTransclusion(page.id, page.workspaceId, tiptapJson); } if (page) { - await this.collabHistory.addContributors(pageId, editingUserIds); + // Key contributors by the page UUID so they MATCH the PAGE_HISTORY job, + // which is enqueued with page.id and pops contributors by page.id (#260). + await this.collabHistory.addContributors(page.id, editingUserIds); const mentions = extractMentions(tiptapJson); @@ -420,14 +427,17 @@ export class PersistenceExtension implements Extension { creatorId: m.creatorId, })), oldMentionedUserIds, - pageId, + // Canonical UUID, never the doc-name slugId (#260). + pageId: page.id, spaceId: page.spaceId, workspaceId: page.workspaceId, } as IPageMentionNotificationJob); } await this.aiQueue.add(QueueJob.PAGE_CONTENT_UPDATED, { - pageIds: [pageId], + // Canonical UUID: the embedding reindex resolves pages by uuid, so a + // slugId here threw Postgres 22P02 invalid-uuid (#260). + pageIds: [page.id], workspaceId: page.workspaceId, }); diff --git a/packages/mcp/build/client.js b/packages/mcp/build/client.js index db5240bf..4e083c62 100644 --- a/packages/mcp/build/client.js +++ b/packages/mcp/build/client.js @@ -37,6 +37,15 @@ const MIME_TO_EXT = { "image/webp": ".webp", "image/svg+xml": ".svg", }; +// Canonical UUID shape (versions 1–8, matching the `uuid` package's `validate` +// that the server's isValidUUID uses). page.repo.ts treats any non-UUID pageId +// as a slugId, so the MCP detects a UUID locally and skips a /pages/info +// round-trip in resolvePageId. A 10-char nanoid slugId never contains dashes, +// so it can never be misread as a UUID here. +const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; +function isUuid(value) { + return typeof value === "string" && UUID_RE.test(value); +} export class DocmostClient { client; token = null; @@ -64,6 +73,11 @@ export class DocmostClient { // can all call login() at once. Memoizing a single promise collapses that // thundering herd into ONE /auth/login request that everyone awaits. loginPromise = null; + // Canonical-UUID cache for resolvePageId: maps an agent-supplied pageId + // (slugId OR uuid) to the page's canonical UUID, so repeated collab edits on + // the same page do not re-fetch /pages/info. Both slugId->uuid and uuid->uuid + // are cached. See resolvePageId for why every collab doc must open by UUID. + pageIdCache = new Map(); constructor(configOrBaseURL, email, password) { // Normalize the legacy positional form into the object union. const config = typeof configOrBaseURL === "string" @@ -572,6 +586,36 @@ export class DocmostClient { const response = await this.client.post("/pages/info", { pageId }); return response.data?.data ?? response.data; } + /** + * Resolve an agent-supplied pageId to the page's CANONICAL UUID (`page.id`), + * so every collaboration document the MCP opens is named `page.` — the + * SAME name the web editor always uses (`page.${page.id}`). + * + * The agent commonly passes a 10-char public slugId (from URLs/listings) as + * the pageId. The web editor opens the collab doc by UUID, but the MCP used to + * pass that slugId straight into the collab doc name (`page.`). For one + * DB row that produced TWO independent Yjs documents whose debounced stores + * clobbered each other — the agent's edit was silently lost (#260). + * + * A UUID input short-circuits with no network round-trip. A slugId is resolved + * once via getPageRaw and cached (both slugId->uuid and uuid->uuid), so + * repeated edits on the same page add no extra request. + */ + async resolvePageId(pageId) { + if (isUuid(pageId)) + return pageId; + const cached = this.pageIdCache.get(pageId); + if (cached) + return cached; + const data = await this.getPageRaw(pageId); + const uuid = data?.id; + if (typeof uuid !== "string" || !uuid) { + throw new Error(`Could not resolve a canonical page id for "${pageId}"`); + } + this.pageIdCache.set(pageId, uuid); + this.pageIdCache.set(uuid, uuid); + return uuid; + } async getPage(pageId) { await this.ensureAuthenticated(); const resultData = await this.getPageRaw(pageId); @@ -863,10 +907,12 @@ export class DocmostClient { async tableInsertRow(pageId, tableRef, cells, index) { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Track insertion in an outer var, reset per-transform, so a collab retry // recomputes it cleanly (mirrors insertNode's pattern). let inserted = false; - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, (liveDoc) => { + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, (liveDoc) => { inserted = false; const { doc: nd, inserted: ins } = insertTableRow(liveDoc, tableRef, cells, index); inserted = ins; @@ -892,8 +938,10 @@ export class DocmostClient { async tableDeleteRow(pageId, tableRef, index) { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); let deleted = false; - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, (liveDoc) => { + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, (liveDoc) => { deleted = false; const { doc: nd, deleted: del } = deleteTableRow(liveDoc, tableRef, index); deleted = del; @@ -921,8 +969,10 @@ export class DocmostClient { async tableUpdateCell(pageId, tableRef, row, col, text) { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); let updated = false; - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, (liveDoc) => { + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, (liveDoc) => { updated = false; const { doc: nd, updated: upd } = updateTableCell(liveDoc, tableRef, row, col, text); updated = upd; @@ -1034,6 +1084,10 @@ export class DocmostClient { */ async updatePage(pageId, content, title) { await this.ensureAuthenticated(); + // Open the collab doc by the canonical UUID, never the slugId (#260). The + // REST /pages/update title write below keeps the agent-supplied id (the + // server resolves a slugId there). + const pageUuid = await this.resolvePageId(pageId); // Write the BODY first, then the title (#159 split-brain). If the collab // body write fails (e.g. a persist timeout), the title must be left // UNTOUCHED so the page never ends up with a new title over its old body. @@ -1043,7 +1097,7 @@ export class DocmostClient { let mutation; try { collabToken = await this.getCollabTokenWithReauth(); - mutation = await updatePageContentRealtime(pageId, content, collabToken, this.apiUrl); + mutation = await updatePageContentRealtime(pageUuid, content, collabToken, this.apiUrl); } catch (error) { // Verbose diagnostics (incl. anything that could expose a token prefix) @@ -1259,7 +1313,9 @@ export class DocmostClient { // Write the BODY first, then the title (#159 split-brain): a failed body // write (e.g. persist timeout) must not leave a new title over the old body. const collabToken = await this.getCollabTokenWithReauth(); - const mutation = await this.replacePage(pageId, doc, collabToken, this.apiUrl); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); + const mutation = await this.replacePage(pageUuid, doc, collabToken, this.apiUrl); // Body persisted successfully — now it is safe to set the title. if (title) { await this.client.post("/pages/update", { pageId, title }); @@ -1294,8 +1350,10 @@ export class DocmostClient { throw new Error("insert_footnote: text is required"); } const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); let result = null; - const mutation = await this.mutatePage(pageId, collabToken, this.apiUrl, (liveDoc) => { + const mutation = await this.mutatePage(pageUuid, collabToken, this.apiUrl, (liveDoc) => { const r = insertInlineFootnote(liveDoc, { anchorText, text }); if (!r.inserted) { // Abort the page-locked write by throwing: mutatePageContent does not @@ -1383,7 +1441,9 @@ export class DocmostClient { // PAGE import: canonicalize footnotes (see markdownToProseMirrorCanonical). const doc = await markdownToProseMirrorCanonical(body); const collabToken = await this.getCollabTokenWithReauth(); - const mutation = await replacePageContent(pageId, doc, collabToken, this.apiUrl); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); + const mutation = await replacePageContent(pageUuid, doc, collabToken, this.apiUrl); // Collect distinct comment ids that actually became comment marks in the doc. const collectCommentIds = (node, acc) => { if (!node || typeof node !== "object") @@ -1467,7 +1527,9 @@ export class DocmostClient { // to the target (parity with the other full-doc write paths). const canonical = canonicalizeFootnotes(content); const collabToken = await this.getCollabTokenWithReauth(); - const mutation = await this.replacePage(targetPageId, canonical, collabToken, this.apiUrl); + // Open the TARGET collab doc by its canonical UUID, never the slugId (#260). + const targetUuid = await this.resolvePageId(targetPageId); + const mutation = await this.replacePage(targetUuid, canonical, collabToken, this.apiUrl); return { success: true, sourcePageId, @@ -1483,6 +1545,8 @@ export class DocmostClient { async editPageText(pageId, edits) { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Apply the edits against the LIVE synced document, not the debounced REST // snapshot, so concurrent human edits/comments are preserved. applyTextEdits // records per-edit match problems in `failed` instead of throwing, and @@ -1495,7 +1559,7 @@ export class DocmostClient { // we must NOT write (no spurious history version) and must not claim a write // happened. let wrote = false; - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, (liveDoc) => { + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, (liveDoc) => { wrote = false; const r = applyTextEdits(liveDoc, edits); results = r.results; @@ -1580,10 +1644,12 @@ export class DocmostClient { target.attrs.id = nodeId; } const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Track the replacement count in an outer var, reset per-transform, so a // collab retry recomputes it cleanly (mirrors replaceImage's pattern). let replaced = 0; - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, (liveDoc) => { + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, (liveDoc) => { replaced = 0; const { doc: nd, replaced: r } = replaceNodeById(liveDoc, nodeId, target); replaced = r; @@ -1636,10 +1702,12 @@ export class DocmostClient { } } const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Track insertion in an outer var, reset per-transform, so a collab retry // recomputes it cleanly (mirrors replaceImage's pattern). let inserted = false; - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, (liveDoc) => { + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, (liveDoc) => { inserted = false; const { doc: nd, inserted: ins } = insertNodeRelative(liveDoc, node, opts); inserted = ins; @@ -1675,10 +1743,12 @@ export class DocmostClient { async deleteNode(pageId, nodeId) { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Track the deletion count in an outer var, reset per-transform, so a // collab retry recomputes it cleanly (mirrors replaceImage's pattern). let deleted = 0; - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, (liveDoc) => { + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, (liveDoc) => { deleted = 0; const { doc: nd, deleted: d } = deleteNodeById(liveDoc, nodeId); deleted = d; @@ -1921,7 +1991,10 @@ export class DocmostClient { let anchored = false; try { const collabToken = await this.getCollabTokenWithReauth(); - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, (liveDoc) => { + // Open the collab doc by the canonical UUID, never the slugId (#260). The + // /comments/create REST call above keeps the agent-supplied id. + const pageUuid = await this.resolvePageId(pageId); + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, (liveDoc) => { const doc = liveDoc && liveDoc.type === "doc" ? liveDoc : { type: "doc", content: [] }; @@ -2324,6 +2397,9 @@ export class DocmostClient { if (opts.alt) node.attrs.alt = opts.alt; const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). The + // uploadImage /files/upload call above keeps the agent-supplied id. + const pageUuid = await this.resolvePageId(pageId); // Recursively collect the plain text of a top-level block. const blockText = (n) => { let out = ""; @@ -2337,7 +2413,7 @@ export class DocmostClient { // concurrent edits/comments/images are preserved and parallel insert_image // calls (serialized by the per-page lock) each see the previous insertion. let placement; - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, (liveDoc) => { + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, (liveDoc) => { const doc = liveDoc && liveDoc.type === "doc" ? liveDoc : { type: "doc", content: [] }; @@ -2424,6 +2500,13 @@ export class DocmostClient { */ async replaceImage(pageId, oldAttachmentId, url, opts = {}) { const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). The + // page lock must ALSO key on the UUID so this operation serializes against + // other writes to the same page (mutatePageContent now locks by the resolved + // UUID too); locking by the raw slugId here would desync the mutex key and + // reopen the TOCTOU/orphan-attachment window the lock closes. uploadImage + // keeps the agent-supplied id (it hits REST, not the collab doc). + const pageUuid = await this.resolvePageId(pageId); // Hold ONE per-page lock for the WHOLE operation (scan -> upload -> write). // Previously the scan and the write were two separate mutatePageContent // calls, each acquiring + releasing the lock, with the upload happening in @@ -2435,7 +2518,7 @@ export class DocmostClient { // reentrant, so the self-locking mutatePageContent would deadlock here) // closes that TOCTOU window. uploadImage hits /files/upload over plain HTTP // and does not touch the page lock, so it is safe to call while held. - return withPageLock(pageId, async () => { + return withPageLock(pageUuid, async () => { // STEP 1: read-only live check. Scan the live document for any image node // matching oldAttachmentId BEFORE uploading anything, so a wrong/stale id // throws without ever creating an orphan attachment. @@ -2453,7 +2536,7 @@ export class DocmostClient { scan(node.content); } }; - await this.mutateLiveContentUnlocked(pageId, collabToken, (liveDoc) => { + await this.mutateLiveContentUnlocked(pageUuid, collabToken, (liveDoc) => { matchFound = false; // reset per-transform (collab may retry the read). const doc = liveDoc && liveDoc.type === "doc" ? liveDoc @@ -2501,7 +2584,7 @@ export class DocmostClient { walk(node.content); } }; - const mutation = await this.mutateLiveContentUnlocked(pageId, collabToken, (liveDoc) => { + const mutation = await this.mutateLiveContentUnlocked(pageUuid, collabToken, (liveDoc) => { // Reset per-transform so collab retries recompute cleanly (no double-count). replaced = 0; const doc = liveDoc && liveDoc.type === "doc" @@ -2598,7 +2681,10 @@ export class DocmostClient { // JSON write path) before writing it back. this.validateDocUrls(version.content); const collabToken = await this.getCollabTokenWithReauth(); - const mutation = await mutatePageContent(version.pageId, collabToken, this.apiUrl, () => version.content); + // version.pageId is the page entity id (already a UUID); resolvePageId + // short-circuits a UUID with no round-trip, so this is defensive only (#260). + const pageUuid = await this.resolvePageId(version.pageId); + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, () => version.content); return { pageId: version.pageId, restoredFrom: historyId, @@ -2767,7 +2853,9 @@ export class DocmostClient { } // Apply atomically against the live doc. const collabToken = await this.getCollabTokenWithReauth(); - const mutation = await mutatePageContent(pageId, collabToken, this.apiUrl, runTransform); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); + const mutation = await mutatePageContent(pageUuid, collabToken, this.apiUrl, runTransform); // Optionally delete consumed comments (best-effort; a delete failure must // not undo the successful write). const deletedComments = []; diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index f74a9af3..84d05dcb 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -133,6 +133,18 @@ export type DocmostMcpConfig = { apiUrl: string } & ( }; }; +// Canonical UUID shape (versions 1–8, matching the `uuid` package's `validate` +// that the server's isValidUUID uses). page.repo.ts treats any non-UUID pageId +// as a slugId, so the MCP detects a UUID locally and skips a /pages/info +// round-trip in resolvePageId. A 10-char nanoid slugId never contains dashes, +// so it can never be misread as a UUID here. +const UUID_RE = + /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; + +function isUuid(value: string): boolean { + return typeof value === "string" && UUID_RE.test(value); +} + export class DocmostClient { private client: AxiosInstance; private token: string | null = null; @@ -160,6 +172,11 @@ export class DocmostClient { // can all call login() at once. Memoizing a single promise collapses that // thundering herd into ONE /auth/login request that everyone awaits. private loginPromise: Promise | null = null; + // Canonical-UUID cache for resolvePageId: maps an agent-supplied pageId + // (slugId OR uuid) to the page's canonical UUID, so repeated collab edits on + // the same page do not re-fetch /pages/info. Both slugId->uuid and uuid->uuid + // are cached. See resolvePageId for why every collab doc must open by UUID. + private pageIdCache = new Map(); // Two construction forms: // - new DocmostClient(config) // discriminated union (current) @@ -751,6 +768,37 @@ export class DocmostClient { return response.data?.data ?? response.data; } + /** + * Resolve an agent-supplied pageId to the page's CANONICAL UUID (`page.id`), + * so every collaboration document the MCP opens is named `page.` — the + * SAME name the web editor always uses (`page.${page.id}`). + * + * The agent commonly passes a 10-char public slugId (from URLs/listings) as + * the pageId. The web editor opens the collab doc by UUID, but the MCP used to + * pass that slugId straight into the collab doc name (`page.`). For one + * DB row that produced TWO independent Yjs documents whose debounced stores + * clobbered each other — the agent's edit was silently lost (#260). + * + * A UUID input short-circuits with no network round-trip. A slugId is resolved + * once via getPageRaw and cached (both slugId->uuid and uuid->uuid), so + * repeated edits on the same page add no extra request. + */ + private async resolvePageId(pageId: string): Promise { + if (isUuid(pageId)) return pageId; + const cached = this.pageIdCache.get(pageId); + if (cached) return cached; + const data = await this.getPageRaw(pageId); + const uuid = data?.id; + if (typeof uuid !== "string" || !uuid) { + throw new Error( + `Could not resolve a canonical page id for "${pageId}"`, + ); + } + this.pageIdCache.set(pageId, uuid); + this.pageIdCache.set(uuid, uuid); + return uuid; + } + async getPage(pageId: string) { await this.ensureAuthenticated(); const resultData = await this.getPageRaw(pageId); @@ -1083,12 +1131,14 @@ export class DocmostClient { ) { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Track insertion in an outer var, reset per-transform, so a collab retry // recomputes it cleanly (mirrors insertNode's pattern). let inserted = false; const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc) => { @@ -1126,10 +1176,12 @@ export class DocmostClient { async tableDeleteRow(pageId: string, tableRef: string, index: number) { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); let deleted = false; const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc) => { @@ -1174,10 +1226,12 @@ export class DocmostClient { ) { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); let updated = false; const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc) => { @@ -1313,6 +1367,10 @@ export class DocmostClient { */ async updatePage(pageId: string, content: string, title?: string) { await this.ensureAuthenticated(); + // Open the collab doc by the canonical UUID, never the slugId (#260). The + // REST /pages/update title write below keeps the agent-supplied id (the + // server resolves a slugId there). + const pageUuid = await this.resolvePageId(pageId); // Write the BODY first, then the title (#159 split-brain). If the collab // body write fails (e.g. a persist timeout), the title must be left @@ -1324,7 +1382,7 @@ export class DocmostClient { try { collabToken = await this.getCollabTokenWithReauth(); mutation = await updatePageContentRealtime( - pageId, + pageUuid, content, collabToken, this.apiUrl, @@ -1587,8 +1645,10 @@ export class DocmostClient { // Write the BODY first, then the title (#159 split-brain): a failed body // write (e.g. persist timeout) must not leave a new title over the old body. const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); const mutation = await this.replacePage( - pageId, + pageUuid, doc, collabToken, this.apiUrl, @@ -1630,9 +1690,11 @@ export class DocmostClient { throw new Error("insert_footnote: text is required"); } const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); let result: { footnoteId: string; reused: boolean } | null = null; const mutation = await this.mutatePage( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc: any) => { @@ -1740,8 +1802,10 @@ export class DocmostClient { // PAGE import: canonicalize footnotes (see markdownToProseMirrorCanonical). const doc = await markdownToProseMirrorCanonical(body); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); const mutation = await replacePageContent( - pageId, + pageUuid, doc, collabToken, this.apiUrl, @@ -1840,8 +1904,10 @@ export class DocmostClient { const canonical = canonicalizeFootnotes(content); const collabToken = await this.getCollabTokenWithReauth(); + // Open the TARGET collab doc by its canonical UUID, never the slugId (#260). + const targetUuid = await this.resolvePageId(targetPageId); const mutation = await this.replacePage( - targetPageId, + targetUuid, canonical, collabToken, this.apiUrl, @@ -1864,6 +1930,8 @@ export class DocmostClient { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Apply the edits against the LIVE synced document, not the debounced REST // snapshot, so concurrent human edits/comments are preserved. applyTextEdits @@ -1878,7 +1946,7 @@ export class DocmostClient { // happened. let wrote = false; const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc) => { @@ -1978,12 +2046,14 @@ export class DocmostClient { } const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Track the replacement count in an outer var, reset per-transform, so a // collab retry recomputes it cleanly (mirrors replaceImage's pattern). let replaced = 0; const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc) => { @@ -2066,12 +2136,14 @@ export class DocmostClient { } const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Track insertion in an outer var, reset per-transform, so a collab retry // recomputes it cleanly (mirrors replaceImage's pattern). let inserted = false; const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc) => { @@ -2120,12 +2192,14 @@ export class DocmostClient { await this.ensureAuthenticated(); const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); // Track the deletion count in an outer var, reset per-transform, so a // collab retry recomputes it cleanly (mirrors replaceImage's pattern). let deleted = 0; const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc) => { @@ -2414,8 +2488,11 @@ export class DocmostClient { let anchored = false; try { const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). The + // /comments/create REST call above keeps the agent-supplied id. + const pageUuid = await this.resolvePageId(pageId); const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc) => { @@ -2893,6 +2970,9 @@ export class DocmostClient { if (opts.alt) node.attrs.alt = opts.alt; const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). The + // uploadImage /files/upload call above keeps the agent-supplied id. + const pageUuid = await this.resolvePageId(pageId); // Recursively collect the plain text of a top-level block. const blockText = (n: any): string => { @@ -2907,7 +2987,7 @@ export class DocmostClient { // calls (serialized by the per-page lock) each see the previous insertion. let placement: "replaced" | "after" | "appended" | undefined; const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, (liveDoc) => { @@ -3019,6 +3099,13 @@ export class DocmostClient { opts: { align?: "left" | "center" | "right"; alt?: string } = {}, ) { const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). The + // page lock must ALSO key on the UUID so this operation serializes against + // other writes to the same page (mutatePageContent now locks by the resolved + // UUID too); locking by the raw slugId here would desync the mutex key and + // reopen the TOCTOU/orphan-attachment window the lock closes. uploadImage + // keeps the agent-supplied id (it hits REST, not the collab doc). + const pageUuid = await this.resolvePageId(pageId); // Hold ONE per-page lock for the WHOLE operation (scan -> upload -> write). // Previously the scan and the write were two separate mutatePageContent @@ -3031,7 +3118,7 @@ export class DocmostClient { // reentrant, so the self-locking mutatePageContent would deadlock here) // closes that TOCTOU window. uploadImage hits /files/upload over plain HTTP // and does not touch the page lock, so it is safe to call while held. - return withPageLock(pageId, async () => { + return withPageLock(pageUuid, async () => { // STEP 1: read-only live check. Scan the live document for any image node // matching oldAttachmentId BEFORE uploading anything, so a wrong/stale id // throws without ever creating an orphan attachment. @@ -3050,7 +3137,7 @@ export class DocmostClient { } }; - await this.mutateLiveContentUnlocked(pageId, collabToken, (liveDoc) => { + await this.mutateLiveContentUnlocked(pageUuid, collabToken, (liveDoc) => { matchFound = false; // reset per-transform (collab may retry the read). const doc = liveDoc && liveDoc.type === "doc" @@ -3105,7 +3192,7 @@ export class DocmostClient { }; const mutation = await this.mutateLiveContentUnlocked( - pageId, + pageUuid, collabToken, (liveDoc) => { // Reset per-transform so collab retries recompute cleanly (no double-count). @@ -3214,8 +3301,11 @@ export class DocmostClient { // JSON write path) before writing it back. this.validateDocUrls(version.content); const collabToken = await this.getCollabTokenWithReauth(); + // version.pageId is the page entity id (already a UUID); resolvePageId + // short-circuits a UUID with no round-trip, so this is defensive only (#260). + const pageUuid = await this.resolvePageId(version.pageId); const mutation = await mutatePageContent( - version.pageId, + pageUuid, collabToken, this.apiUrl, () => version.content, @@ -3414,8 +3504,10 @@ export class DocmostClient { // Apply atomically against the live doc. const collabToken = await this.getCollabTokenWithReauth(); + // Open the collab doc by the canonical UUID, never the slugId (#260). + const pageUuid = await this.resolvePageId(pageId); const mutation = await mutatePageContent( - pageId, + pageUuid, collabToken, this.apiUrl, runTransform, diff --git a/packages/mcp/test/mock/ambiguous-node-id.test.mjs b/packages/mcp/test/mock/ambiguous-node-id.test.mjs index d29add0a..d8a55201 100644 --- a/packages/mcp/test/mock/ambiguous-node-id.test.mjs +++ b/packages/mcp/test/mock/ambiguous-node-id.test.mjs @@ -132,7 +132,7 @@ test("patch_node REFUSES an ambiguous (duplicate) id without writing to collab", await assert.rejects( () => - client.patchNode("page-1", DUP_ID, { + client.patchNode("11111111-1111-4111-8111-111111111111", DUP_ID, { type: "paragraph", content: [{ type: "text", text: "replacement" }], }), @@ -152,7 +152,7 @@ test("delete_node REFUSES an ambiguous (duplicate) id without writing to collab" const client = new DocmostClient(baseURL, "user@example.com", "pw"); await assert.rejects( - () => client.deleteNode("page-2", DUP_ID), + () => client.deleteNode("22222222-2222-4222-8222-222222222222", DUP_ID), /ambiguous/i, "delete_node must reject a duplicate-id target with an 'ambiguous' error", ); diff --git a/packages/mcp/test/mock/insert-footnote-wrapper.test.mjs b/packages/mcp/test/mock/insert-footnote-wrapper.test.mjs index 887806b7..117461cf 100644 --- a/packages/mcp/test/mock/insert-footnote-wrapper.test.mjs +++ b/packages/mcp/test/mock/insert-footnote-wrapper.test.mjs @@ -37,6 +37,11 @@ function makeClient(liveDoc) { async getCollabTokenWithReauth() { return "collab-token"; } + // Identity resolution: this test isolates the footnote wrapper, so the + // slugId->uuid resolution (#260) is stubbed to a no-op and "p1" stays "p1". + async resolvePageId(pageId) { + return pageId; + } async mutatePage(pageId, token, apiUrl, transform) { calls.pageId = pageId; calls.token = token; diff --git a/packages/mcp/test/mock/resolve-page-id-collab-doc-name.test.mjs b/packages/mcp/test/mock/resolve-page-id-collab-doc-name.test.mjs new file mode 100644 index 00000000..c6baaac8 --- /dev/null +++ b/packages/mcp/test/mock/resolve-page-id-collab-doc-name.test.mjs @@ -0,0 +1,233 @@ +// Mock collab regression for the #260 data-loss bug: the MCP must open every +// collaboration document by the page's CANONICAL UUID (`page.`) — the same +// name the web editor uses — even when the agent supplies a public slugId. +// +// Root cause: the agent commonly passes a 10-char slugId (from URLs/listings) as +// pageId. The web tab opens `page.`, but the MCP used to pass the slugId +// straight into the collab doc name (`page.`), so one DB page ended up +// with TWO independent Yjs documents whose debounced stores clobbered each other +// — the agent's edit was silently lost on reload. +// +// We stand up a real Hocuspocus server (like ambiguous-node-id.test.mjs) and +// capture the EXACT documentName each connection requests via onLoadDocument. +// The /pages/info mock resolves the slugId -> uuid, and counts its own hits so we +// can also prove the UUID short-circuit + cache (no redundant resolve round-trip). +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"; + +const SLUG = "dwzDdgPep2"; // 10-char nanoid public id (no dashes) +const UUID = "11111111-1111-4111-8111-111111111111"; // canonical page.id + +// A simple one-paragraph document; "hello world" gives editPageText a match and +// insertFootnote an anchor. No table node, so tableInsertRow aborts with +// "no table found" — but the collab doc was still OPENED by then, which is what +// we assert (the doc NAME is fixed at connect time, before any transform runs). +function seedDoc() { + return { + type: "doc", + content: [ + { + type: "paragraph", + attrs: { id: "p1" }, + content: [{ type: "text", text: "hello world" }], + }, + ], + }; +} + +function readBody(req) { + return new Promise((resolve) => { + let raw = ""; + req.on("data", (c) => (raw += c)); + req.on("end", () => resolve(raw)); + }); +} + +// Stand up an HTTP server that authenticates, hands out a collab token, serves +// /pages/info (slugId -> uuid resolution), and upgrades /collab to a Hocuspocus +// instance whose onLoadDocument records the requested documentName. +async function spawnCollabStack() { + const state = { docNames: [], pagesInfoCalls: [] }; + + const hocuspocus = new Hocuspocus({ + quiet: true, + async onLoadDocument({ documentName }) { + state.docNames.push(documentName); + return buildYDoc(seedDoc()); + }, + }); + + const wss = new WebSocketServer({ noServer: true }); + + const server = http.createServer(async (req, res) => { + const raw = await readBody(req); + 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; + } + if (req.url === "/api/pages/info") { + let pageId; + try { + pageId = JSON.parse(raw)?.pageId; + } catch { + pageId = undefined; + } + state.pagesInfoCalls.push(pageId); + // Always resolve to the SAME canonical record, mirroring the server's + // findById (which accepts either the uuid or the slugId). + res.writeHead(200, { "Content-Type": "application/json" }); + res.end( + JSON.stringify({ + data: { + id: UUID, + slugId: SLUG, + title: "Doc", + spaceId: "space-1", + content: seedDoc(), + }, + }), + ); + return; + } + // Title writes (/pages/update) and anything else: succeed quietly. + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ data: {} })); + }); + + // 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("editPageText with a slugId opens the collab doc by the resolved UUID (#260)", async () => { + const { state, baseURL } = await spawnCollabStack(); + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + + const res = await client.editPageText(SLUG, [ + { find: "hello", replace: "hi" }, + ]); + assert.equal(res.success, true); + + assert.ok( + state.docNames.includes(`page.${UUID}`), + `collab doc must be opened as page.${UUID}, got ${JSON.stringify(state.docNames)}`, + ); + assert.ok( + !state.docNames.includes(`page.${SLUG}`), + "collab doc must NEVER be opened by the slugId (that is the data-loss bug)", + ); + // The slugId had to be resolved via /pages/info at least once. + assert.ok(state.pagesInfoCalls.length >= 1); +}); + +test("tableInsertRow with a slugId opens the collab doc by the resolved UUID (#260)", async () => { + const { state, baseURL } = await spawnCollabStack(); + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + + // No table in the seed doc, so this aborts with "no table found" — but the + // collab doc has ALREADY been opened (by UUID) before the transform decides. + await assert.rejects( + () => client.tableInsertRow(SLUG, "#0", ["a", "b"]), + /no table/i, + ); + + assert.deepEqual( + state.docNames, + [`page.${UUID}`], + "tableInsertRow must open the collab doc by the resolved UUID", + ); +}); + +test("the generic mutate (insert_footnote) with a slugId opens by the resolved UUID (#260)", async () => { + const { state, baseURL } = await spawnCollabStack(); + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + + const res = await client.insertFootnote(SLUG, "world", "a note"); + assert.equal(res.success, true); + + assert.deepEqual( + state.docNames, + [`page.${UUID}`], + "insert_footnote (via the mutatePage seam) must open the collab doc by UUID", + ); +}); + +test("a UUID input is passed through unchanged and triggers NO /pages/info fetch (short-circuit)", async () => { + const { state, baseURL } = await spawnCollabStack(); + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + + const res = await client.editPageText(UUID, [ + { find: "hello", replace: "hi" }, + ]); + assert.equal(res.success, true); + + assert.deepEqual(state.docNames, [`page.${UUID}`]); + assert.equal( + state.pagesInfoCalls.length, + 0, + "a UUID input must short-circuit resolvePageId with no /pages/info round-trip", + ); +}); + +test("a repeated slugId edit resolves the UUID only once (cache)", async () => { + const { state, baseURL } = await spawnCollabStack(); + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + + // Each mock connection re-seeds a fresh "hello world" doc (the mock does not + // persist across connects), so both edits target "hello". The cache assertion + // only concerns the slugId->uuid resolution, not the document content. + await client.editPageText(SLUG, [{ find: "hello", replace: "hi" }]); + await client.editPageText(SLUG, [{ find: "hello", replace: "hey" }]); + + assert.deepEqual(state.docNames, [`page.${UUID}`, `page.${UUID}`]); + assert.equal( + state.pagesInfoCalls.length, + 1, + "the slugId->uuid resolution must be cached across edits on the same page", + ); +}); diff --git a/packages/mcp/test/mock/write-order.test.mjs b/packages/mcp/test/mock/write-order.test.mjs index c3a013f3..2d838ba7 100644 --- a/packages/mcp/test/mock/write-order.test.mjs +++ b/packages/mcp/test/mock/write-order.test.mjs @@ -66,6 +66,14 @@ function makeServer() { sendJson(res, 200, { data: { token: "collab-jwt" } }); return; } + if (req.url === "/api/pages/info") { + // Resolve the pageId -> canonical UUID (#260) so the test exercises the + // real body-write failure (no WS upgrade) rather than a resolve failure. + sendJson(res, 200, { + data: { id: "11111111-1111-4111-8111-111111111111", slugId: "page-1" }, + }); + return; + } if (req.url === "/api/pages/update") { state.titlePosted = true; sendJson(res, 200, { data: {} });