From a670de7638c919ada7fc2426c8c3c215e25d82a6 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 05:14:42 +0300 Subject: [PATCH] fix(mcp): write page body before title to avoid split-brain on failure (#159) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updatePage (markdown) and updatePageJson wrote the title via REST FIRST, then the body via collab. If the body write failed (e.g. a collab persist timeout), the page was left with the NEW title over its OLD body — a split-brain the tool reported as an error but never repaired (red-team finding #10). Reorder both: write the body first, and only set the title after the body has persisted. Now a body-write failure leaves the title untouched (no split-brain). A title write failing after a successful body is rarer (REST is fast) and leaves correct content under a stale title — the strictly lesser inconsistency — which is the same trade-off the issue's "atomic, or roll back the title" intends, without the fragility of a rollback write that could itself fail. No unit test: both paths require a live collab provider and the suite has no provider mock; the change is a pure reordering. All 306 mcp tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/build/client.js | 61 ++++++--- packages/mcp/src/client.ts | 256 ++++++++++++++++++++--------------- 2 files changed, 193 insertions(+), 124 deletions(-) diff --git a/packages/mcp/build/client.js b/packages/mcp/build/client.js index e1d2d82e..8c5fcc9d 100644 --- a/packages/mcp/build/client.js +++ b/packages/mcp/build/client.js @@ -16,7 +16,7 @@ import { withPageLock } from "./lib/page-lock.js"; import { applyTextEdits, } from "./lib/json-edit.js"; import { getCollabToken, performLogin } from "./lib/auth-utils.js"; import { diffDocs, summarizeChange } from "./lib/diff.js"; -import { applyAnchorInDoc, canAnchorInDoc, } from "./lib/comment-anchor.js"; +import { applyAnchorInDoc, canAnchorInDoc } from "./lib/comment-anchor.js"; import { blockText, walk, getList, insertMarkerAfter, setCalloutRange, noteItem, mdToInlineNodes, commentsToFootnotes, } from "./lib/transforms.js"; import vm from "node:vm"; // Supported image types, kept as two lookup tables so both a local file @@ -208,7 +208,9 @@ export class DocmostClient { // getCollabToken wraps the AxiosError in a plain Error but attaches the // HTTP status as `.status`, so detect an auth failure via either the raw // AxiosError shape OR the attached status. - const axiosStatus = axios.isAxiosError(e) ? e.response?.status : undefined; + const axiosStatus = axios.isAxiosError(e) + ? e.response?.status + : undefined; const attachedStatus = e?.status; const isAuthError = axiosStatus === 401 || axiosStatus === 403 || @@ -687,7 +689,12 @@ export class DocmostClient { if (!inserted) { throw new Error(`table_insert_row: no table found for "${tableRef}" on page ${pageId} (use "#" from get_outline, or a block id inside the table)`); } - return { success: true, table: tableRef, inserted: true, verify: mutation.verify }; + return { + success: true, + table: tableRef, + inserted: true, + verify: mutation.verify, + }; } /** * Delete the row at 0-based `index` from a table on the LIVE collab document. @@ -709,7 +716,12 @@ export class DocmostClient { if (!deleted) { throw new Error(`table_delete_row: no table found for "${tableRef}" on page ${pageId} (use "#" from get_outline, or a block id inside the table)`); } - return { success: true, table: tableRef, deleted: true, verify: mutation.verify }; + return { + success: true, + table: tableRef, + deleted: true, + verify: mutation.verify, + }; } /** * Set the plain-text content of cell `[row, col]` (0-based) in a table on the @@ -733,7 +745,13 @@ export class DocmostClient { if (!updated) { throw new Error(`table_update_cell: no table found for "${tableRef}" on page ${pageId} (use "#" from get_outline, or a block id inside the table)`); } - return { success: true, table: tableRef, row, col, verify: mutation.verify }; + return { + success: true, + table: tableRef, + row, + col, + verify: mutation.verify, + }; } /** * Create a new page with title and content. @@ -828,9 +846,11 @@ export class DocmostClient { */ async updatePage(pageId, content, title) { await this.ensureAuthenticated(); - if (title) { - await this.client.post("/pages/update", { pageId, title }); - } + // 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. + // A title write failing AFTER a successful body is rarer (REST is fast) and + // leaves correct content under a stale title — the lesser inconsistency. let collabToken = ""; let mutation; try { @@ -849,6 +869,10 @@ export class DocmostClient { } throw new Error(`Failed to update page content: ${error.message}`); } + // Body persisted successfully — now it is safe to set the title. + if (title) { + await this.client.post("/pages/update", { pageId, title }); + } return { success: true, modified: true, @@ -968,7 +992,9 @@ export class DocmostClient { if (!node || typeof node !== "object" || typeof node.type !== "string") { throw new Error("invalid ProseMirror document: every node must be an object with a string `type`"); } - if ("text" in node && node.type === "text" && typeof node.text !== "string") { + if ("text" in node && + node.type === "text" && + typeof node.text !== "string") { throw new Error("invalid ProseMirror document: a text node must have a string `text`"); } if (node.marks !== undefined) { @@ -976,7 +1002,9 @@ export class DocmostClient { throw new Error("invalid ProseMirror document: `marks` must be an array"); } for (const mark of node.marks) { - if (!mark || typeof mark !== "object" || typeof mark.type !== "string") { + if (!mark || + typeof mark !== "object" || + typeof mark.type !== "string") { throw new Error("invalid ProseMirror document: every mark must be an object with a string `type`"); } } @@ -1035,11 +1063,14 @@ export class DocmostClient { // the markdown link path (which TipTap sanitizes), raw JSON could otherwise // inject javascript:/data: link hrefs or media srcs straight into the doc. this.validateDocUrls(doc); + // 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 replacePageContent(pageId, 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 }); } - const collabToken = await this.getCollabTokenWithReauth(); - const mutation = await replacePageContent(pageId, doc, collabToken, this.apiUrl); return { success: true, modified: true, @@ -1056,9 +1087,7 @@ export class DocmostClient { async exportPageMarkdown(pageId) { await this.ensureAuthenticated(); const page = await this.getPageRaw(pageId); - const body = page.content - ? convertProseMirrorToMarkdown(page.content) - : ""; + const body = page.content ? convertProseMirrorToMarkdown(page.content) : ""; let comments = []; try { comments = await this.listComments(pageId); @@ -1363,7 +1392,7 @@ export class DocmostClient { // markdown/emoji are tolerated only as a strip-and-retry fallback, so a // miss usually means the text differs from what's on the page. const hint = opts.anchorText - ? ' anchorText must be the block\'s literal rendered plain text (no markdown wrappers or emoji); anchorNodeId from get_page_json is more reliable.' + ? " anchorText must be the block's literal rendered plain text (no markdown wrappers or emoji); anchorNodeId from get_page_json is more reliable." : ""; throw new Error(`insert_node: anchor not found (${anchorDesc}) on page ${pageId}.${hint}`); } diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index c6419563..4616f43d 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -49,10 +49,7 @@ import { } from "./lib/json-edit.js"; import { getCollabToken, performLogin } from "./lib/auth-utils.js"; import { diffDocs, summarizeChange } from "./lib/diff.js"; -import { - applyAnchorInDoc, - canAnchorInDoc, -} from "./lib/comment-anchor.js"; +import { applyAnchorInDoc, canAnchorInDoc } from "./lib/comment-anchor.js"; import { blockText, walk, @@ -305,7 +302,9 @@ export class DocmostClient { // getCollabToken wraps the AxiosError in a plain Error but attaches the // HTTP status as `.status`, so detect an auth failure via either the raw // AxiosError shape OR the attached status. - const axiosStatus = axios.isAxiosError(e) ? e.response?.status : undefined; + const axiosStatus = axios.isAxiosError(e) + ? e.response?.status + : undefined; const attachedStatus = (e as any)?.status; const isAuthError = axiosStatus === 401 || @@ -597,11 +596,7 @@ export class DocmostClient { * sidebar requests and is bounded by that method's 10000-node cap (and skips * soft-deleted pages server-side). */ - async listPages( - spaceId?: string, - limit: number = 50, - tree: boolean = false, - ) { + async listPages(spaceId?: string, limit: number = 50, tree: boolean = false) { await this.ensureAuthenticated(); if (tree) { @@ -880,7 +875,12 @@ export class DocmostClient { `table_insert_row: no table found for "${tableRef}" on page ${pageId} (use "#" from get_outline, or a block id inside the table)`, ); } - return { success: true, table: tableRef, inserted: true, verify: mutation.verify }; + return { + success: true, + table: tableRef, + inserted: true, + verify: mutation.verify, + }; } /** @@ -899,7 +899,11 @@ export class DocmostClient { this.apiUrl, (liveDoc) => { deleted = false; - const { doc: nd, deleted: del } = deleteTableRow(liveDoc, tableRef, index); + const { doc: nd, deleted: del } = deleteTableRow( + liveDoc, + tableRef, + index, + ); deleted = del; if (!deleted) return null; // table not found -> skip the write entirely return nd; @@ -911,7 +915,12 @@ export class DocmostClient { `table_delete_row: no table found for "${tableRef}" on page ${pageId} (use "#" from get_outline, or a block id inside the table)`, ); } - return { success: true, table: tableRef, deleted: true, verify: mutation.verify }; + return { + success: true, + table: tableRef, + deleted: true, + verify: mutation.verify, + }; } /** @@ -956,7 +965,13 @@ export class DocmostClient { `table_update_cell: no table found for "${tableRef}" on page ${pageId} (use "#" from get_outline, or a block id inside the table)`, ); } - return { success: true, table: tableRef, row, col, verify: mutation.verify }; + return { + success: true, + table: tableRef, + row, + col, + verify: mutation.verify, + }; } /** @@ -1030,8 +1045,7 @@ export class DocmostClient { response = await axios.post(importUrl, form2, { headers: { ...form2.getHeaders(), - Authorization: - this.client.defaults.headers.common["Authorization"], + Authorization: this.client.defaults.headers.common["Authorization"], }, timeout: 60000, }); @@ -1065,10 +1079,11 @@ export class DocmostClient { async updatePage(pageId: string, content: string, title?: string) { await this.ensureAuthenticated(); - if (title) { - await this.client.post("/pages/update", { pageId, title }); - } - + // 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. + // A title write failing AFTER a successful body is rarer (REST is fast) and + // leaves correct content under a stale title — the lesser inconsistency. let collabToken = ""; let mutation; try { @@ -1095,6 +1110,11 @@ export class DocmostClient { throw new Error(`Failed to update page content: ${error.message}`); } + // Body persisted successfully — now it is safe to set the title. + if (title) { + await this.client.post("/pages/update", { pageId, title }); + } + return { success: true, modified: true, @@ -1169,9 +1189,7 @@ export class DocmostClient { for (const mark of node.marks) { if (mark && mark.type === "link" && mark.attrs) { if (!this.isSafeUrl(mark.attrs.href, "link")) { - throw new Error( - `unsafe link href rejected: "${mark.attrs.href}"`, - ); + throw new Error(`unsafe link href rejected: "${mark.attrs.href}"`); } } } @@ -1230,7 +1248,11 @@ export class DocmostClient { "invalid ProseMirror document: every node must be an object with a string `type`", ); } - if ("text" in node && node.type === "text" && typeof node.text !== "string") { + if ( + "text" in node && + node.type === "text" && + typeof node.text !== "string" + ) { throw new Error( "invalid ProseMirror document: a text node must have a string `text`", ); @@ -1242,7 +1264,11 @@ export class DocmostClient { ); } for (const mark of node.marks) { - if (!mark || typeof mark !== "object" || typeof mark.type !== "string") { + if ( + !mark || + typeof mark !== "object" || + typeof mark.type !== "string" + ) { throw new Error( "invalid ProseMirror document: every mark must be an object with a string `type`", ); @@ -1317,10 +1343,8 @@ export class DocmostClient { // inject javascript:/data: link hrefs or media srcs straight into the doc. this.validateDocUrls(doc); - if (title) { - await this.client.post("/pages/update", { pageId, title }); - } - + // 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 replacePageContent( pageId, @@ -1329,6 +1353,11 @@ export class DocmostClient { this.apiUrl, ); + // Body persisted successfully — now it is safe to set the title. + if (title) { + await this.client.post("/pages/update", { pageId, title }); + } + return { success: true, modified: true, @@ -1346,9 +1375,7 @@ export class DocmostClient { async exportPageMarkdown(pageId: string): Promise { await this.ensureAuthenticated(); const page = await this.getPageRaw(pageId); - const body = page.content - ? convertProseMirrorToMarkdown(page.content) - : ""; + const body = page.content ? convertProseMirrorToMarkdown(page.content) : ""; let comments: any[] = []; try { comments = await this.listComments(pageId); @@ -1562,9 +1589,10 @@ export class DocmostClient { pageId, applied: results, failed, - message: (failed?.length ?? 0) - ? `Applied ${results?.length ?? 0} edit(s); ${failed!.length} failed (see failed[]). Node ids and formatting preserved.` - : "Text edits applied (node ids and formatting preserved).", + message: + (failed?.length ?? 0) + ? `Applied ${results?.length ?? 0} edit(s); ${failed!.length} failed (see failed[]). Node ids and formatting preserved.` + : "Text edits applied (node ids and formatting preserved).", verify: mutation.verify, }; @@ -1623,7 +1651,11 @@ export class DocmostClient { this.apiUrl, (liveDoc) => { replaced = 0; - const { doc: nd, replaced: r } = replaceNodeById(liveDoc, nodeId, target); + const { doc: nd, replaced: r } = replaceNodeById( + liveDoc, + nodeId, + target, + ); replaced = r; // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS: Docmost // duplicates block ids on copy/paste (and copyPageContent writes them @@ -1714,7 +1746,11 @@ export class DocmostClient { this.apiUrl, (liveDoc) => { inserted = false; - const { doc: nd, inserted: ins } = insertNodeRelative(liveDoc, node, opts); + const { doc: nd, inserted: ins } = insertNodeRelative( + liveDoc, + node, + opts, + ); inserted = ins; if (!inserted) return null; // anchor not found -> skip the write entirely return nd; @@ -1729,7 +1765,7 @@ export class DocmostClient { // markdown/emoji are tolerated only as a strip-and-retry fallback, so a // miss usually means the text differs from what's on the page. const hint = opts.anchorText - ? ' anchorText must be the block\'s literal rendered plain text (no markdown wrappers or emoji); anchorNodeId from get_page_json is more reliable.' + ? " anchorText must be the block's literal rendered plain text (no markdown wrappers or emoji); anchorNodeId from get_page_json is more reliable." : ""; throw new Error( `insert_node: anchor not found (${anchorDesc}) on page ${pageId}.${hint}`, @@ -2157,7 +2193,11 @@ export class DocmostClient { * subtree): pages updated after `since` are scanned and their comments * filtered by createdAt > since. */ - async checkNewComments(spaceId: string, since: string, parentPageId?: string) { + async checkNewComments( + spaceId: string, + since: string, + parentPageId?: string, + ) { await this.ensureAuthenticated(); const sinceDate = new Date(since); @@ -2457,8 +2497,7 @@ export class DocmostClient { response = await axios.post(uploadUrl, form2, { headers: { ...form2.getHeaders(), - Authorization: - this.client.defaults.headers.common["Authorization"], + Authorization: this.client.defaults.headers.common["Authorization"], }, timeout: 60000, }); @@ -2545,76 +2584,76 @@ export class DocmostClient { collabToken, this.apiUrl, (liveDoc) => { - const doc = - liveDoc && liveDoc.type === "doc" - ? liveDoc - : { type: "doc", content: [] }; - if (!Array.isArray(doc.content)) doc.content = []; + const doc = + liveDoc && liveDoc.type === "doc" + ? liveDoc + : { type: "doc", content: [] }; + if (!Array.isArray(doc.content)) doc.content = []; - if (opts.replaceText) { - // Ambiguity guard (mirrors editPageText): count matching top-level - // blocks first, so a non-unique fragment cannot silently replace the - // wrong block (e.g. text that also appears inside a callout/table). - const matches = doc.content.filter((b: any) => - blockText(b).includes(opts.replaceText!), - ); - if (matches.length === 0) { - throw new Error(`replaceText not found: "${opts.replaceText}"`); - } - if (matches.length > 1) { - throw new Error( - `replaceText "${opts.replaceText}" matches ${matches.length} blocks; use a longer unique fragment`, + if (opts.replaceText) { + // Ambiguity guard (mirrors editPageText): count matching top-level + // blocks first, so a non-unique fragment cannot silently replace the + // wrong block (e.g. text that also appears inside a callout/table). + const matches = doc.content.filter((b: any) => + blockText(b).includes(opts.replaceText!), ); - } - const idx = doc.content.findIndex((b: any) => - blockText(b).includes(opts.replaceText!), - ); - // Data-loss guard: replaceText swaps the WHOLE top-level block, so if - // the fragment only appears nested inside a container (table, callout, - // list, blockquote) the entire structure would be destroyed. Refuse - // when the matched block is a container rather than a leaf - // paragraph/heading and point the caller at a safer tool. - const CONTAINER_TYPES = new Set([ - "table", - "callout", - "bulletList", - "orderedList", - "taskList", - "blockquote", - ]); - const matchedBlock = doc.content[idx]; - if (matchedBlock && CONTAINER_TYPES.has(matchedBlock.type)) { - throw new Error( - `replaceText matched a ${matchedBlock.type} container block; replacing it would destroy the whole structure. ` + - `Use afterText to insert near it, or update_page_json for surgical edits.`, + if (matches.length === 0) { + throw new Error(`replaceText not found: "${opts.replaceText}"`); + } + if (matches.length > 1) { + throw new Error( + `replaceText "${opts.replaceText}" matches ${matches.length} blocks; use a longer unique fragment`, + ); + } + const idx = doc.content.findIndex((b: any) => + blockText(b).includes(opts.replaceText!), ); - } - doc.content.splice(idx, 1, node); - placement = "replaced"; - } else if (opts.afterText) { - // Ambiguity guard (mirrors editPageText): refuse a non-unique fragment. - const matches = doc.content.filter((b: any) => - blockText(b).includes(opts.afterText!), - ); - if (matches.length === 0) { - throw new Error(`afterText not found: "${opts.afterText}"`); - } - if (matches.length > 1) { - throw new Error( - `afterText "${opts.afterText}" matches ${matches.length} blocks; use a longer unique fragment`, + // Data-loss guard: replaceText swaps the WHOLE top-level block, so if + // the fragment only appears nested inside a container (table, callout, + // list, blockquote) the entire structure would be destroyed. Refuse + // when the matched block is a container rather than a leaf + // paragraph/heading and point the caller at a safer tool. + const CONTAINER_TYPES = new Set([ + "table", + "callout", + "bulletList", + "orderedList", + "taskList", + "blockquote", + ]); + const matchedBlock = doc.content[idx]; + if (matchedBlock && CONTAINER_TYPES.has(matchedBlock.type)) { + throw new Error( + `replaceText matched a ${matchedBlock.type} container block; replacing it would destroy the whole structure. ` + + `Use afterText to insert near it, or update_page_json for surgical edits.`, + ); + } + doc.content.splice(idx, 1, node); + placement = "replaced"; + } else if (opts.afterText) { + // Ambiguity guard (mirrors editPageText): refuse a non-unique fragment. + const matches = doc.content.filter((b: any) => + blockText(b).includes(opts.afterText!), ); + if (matches.length === 0) { + throw new Error(`afterText not found: "${opts.afterText}"`); + } + if (matches.length > 1) { + throw new Error( + `afterText "${opts.afterText}" matches ${matches.length} blocks; use a longer unique fragment`, + ); + } + const idx = doc.content.findIndex((b: any) => + blockText(b).includes(opts.afterText!), + ); + doc.content.splice(idx + 1, 0, node); + placement = "after"; + } else { + doc.content.push(node); + placement = "appended"; } - const idx = doc.content.findIndex((b: any) => - blockText(b).includes(opts.afterText!), - ); - doc.content.splice(idx + 1, 0, node); - placement = "after"; - } else { - doc.content.push(node); - placement = "appended"; - } - return doc; + return doc; }, ); @@ -2871,8 +2910,7 @@ export class DocmostClient { async diffPageVersions(pageId: string, from?: string, to?: string) { await this.ensureAuthenticated(); - const isCurrent = (v?: string) => - v == null || v === "" || v === "current"; + const isCurrent = (v?: string) => v == null || v === "" || v === "current"; const resolveSide = async ( v?: string, @@ -2993,7 +3031,9 @@ export class DocmostClient { throw new Error(`transform did not compile: ${e?.message ?? e}`); } if (typeof fn !== "function") { - throw new Error("transform must evaluate to a function (doc, ctx) => doc"); + throw new Error( + "transform must evaluate to a function (doc, ctx) => doc", + ); } const result = vm.runInNewContext( "f(d, c)",