From 24264efa2566047a99d785cec2b87538859a6326 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 11:35:19 +0300 Subject: [PATCH] refactor(review): address PR #185 review (lease leak, tests, changelog, jsonb seam) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8-point multi-aspect review of the batch PR; security/regressions were clean. 1. Lease leak: the #180 reorder moved `toolsFor` (which leases external MCP clients, refCount+1) ahead of buildSystemPrompt + forUser, but the only release (closeExternalClients) was bound to the streamText callbacks. A throw in between leaked the lease (refCount stuck, undici sockets held until restart). Define closeExternalClients right after the lease and wrap buildSystemPrompt+forUser in try/catch that closes-then-rethrows. 2. Cover the patch_node/delete_node dup-id refusal (#159 #6): extract the guard into a pure `assertUnambiguousMatch` (node-ops) and unit-test 0/1/>1. 3. Regress the body-before-title order (#159 #10): mock-HTTP test (collab fails fast against a server with no WS upgrade) asserts /pages/update (title) is NEVER posted when the body write fails — for updatePage AND updatePageJson. 4. CHANGELOG [Unreleased]: #180, #168 (Added); #163 (Fixed). 5. Add the missing en-US i18n keys (Back to references / {{label}}). 6. Drop the duplicate content/empty/blank cases in ai-chat.prompt.spec.ts (they repeat the buildMcpToolingBlock unit tests); keep only sandwich placement + both-safety-copies. 7. CI Postgres pg16 -> pg18 (match docker-compose). 8. jsonb decode seam: shared `parseJsonbValue(value, guard)` in database/utils.ts holds the legacy double-encoding self-heal in one place; parseToolAllowlist / parseModelConfig keep only a type-guard. Verified: server build + 124 unit + 15 integration; mcp 311; prettier clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/test.yml | 2 +- CHANGELOG.md | 19 +++- .../public/locales/en-US/translation.json | 2 + .../src/core/ai-chat/ai-chat.prompt.spec.ts | 45 +------- .../src/core/ai-chat/ai-chat.service.ts | 74 +++++++----- .../ai-agent-roles/ai-agent-roles.repo.ts | 20 ++-- .../repos/ai-chat/ai-mcp-server.repo.ts | 20 ++-- apps/server/src/database/utils.ts | 26 +++++ packages/mcp/build/client.js | 20 ++-- packages/mcp/build/lib/node-ops.js | 35 +++++- packages/mcp/src/client.ts | 27 ++--- packages/mcp/src/lib/node-ops.ts | 61 ++++++++-- packages/mcp/test/mock/write-order.test.mjs | 106 ++++++++++++++++++ packages/mcp/test/unit/node-ops.test.mjs | 38 ++++++- 14 files changed, 342 insertions(+), 153 deletions(-) create mode 100644 packages/mcp/test/mock/write-order.test.mjs diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f2330749..3a756656 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -26,7 +26,7 @@ jobs: # TEST_*_URL overrides are needed. services: postgres: - image: pgvector/pgvector:pg16 + image: pgvector/pgvector:pg18 env: POSTGRES_USER: docmost POSTGRES_PASSWORD: docmost_dev_pw diff --git a/CHANGELOG.md b/CHANGELOG.md index 26adb3f9..992b6af6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **AI-agent attribution for MCP writes.** Comments (and pages) created through the MCP endpoint by a dedicated agent account are now badged as "AI", with unspoofable provenance derived from a per-user `is_agent` flag (not from the - request body). **Operator setup:** use a *dedicated* service account for the + request body). **Operator setup:** use a _dedicated_ service account for the MCP fallback and set the flag with SQL — `UPDATE users SET is_agent = true WHERE email = ''`. Never flag a human or shared account, or its normal edits get mis-attributed as AI. See the @@ -32,6 +32,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 OpenRouter, etc.; `openai` uses the official provider (real-OpenAI reasoning-model request shaping). Chosen explicitly rather than inferred from the base URL, since a custom URL can front real OpenAI too. (#175, #177) +- **Per-MCP-server instructions in the agent prompt.** Each external MCP server + now has an admin-authored `instructions` field ("how/when to use this server's + tools") that is injected into the agent's system prompt next to that server's + tool descriptions. Trusted text, rendered inside the prompt safety sandwich; + shown only for a server that actually connected and contributed ≥1 callable + tool. (#180) +- **Footnote multi-backlinks.** A footnote referenced more than once now shows a + back-link per reference (↩ a b c …), each scrolling to its own occurrence, like + Pandoc/Wikipedia; a single-reference footnote keeps the plain ↩. (#168) ### Changed @@ -67,6 +76,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 are nudged after a paste to refresh stale hit-testing geometry. The caret symptom is macOS-specific and was confirmed manually on macOS; the automated guard pins the DOM-order invariant, not the caret behavior itself. (#146, #147) +- **AI chat: the live token counter now ticks between agent steps.** During a + multi-step turn the header token badge (and the "Thinking… · N tokens" line) + no longer froze on the previous step's authoritative usage; the current step's + estimate is combined per-component with `max`, so the count rises smoothly and + never jumps backwards. (#163) ## [0.93.0] - 2026-06-21 @@ -150,8 +164,7 @@ embeds — plus a large batch of security hardening and test coverage. - Page templates: import `ThrottleModule` so collab boots, never strand an in-flight page-embed id, and add defense-in-depth workspace checks. - Pages: `movePage` cycle guard with no phantom `PAGE_MOVED` event. -- Import: surface the real error cause from `/pages/import` instead of a generic - 400. +- Import: surface the real error cause from `/pages/import` instead of a generic 400. ### Security diff --git a/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index cdad5023..b57fffa8 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -1078,6 +1078,8 @@ "Undo": "Undo", "Redo": "Redo", "Backlinks": "Backlinks", + "Back to references": "Back to references", + "Back to reference {{label}}": "Back to reference {{label}}", "Last updated by": "Last updated by", "Last updated": "Last updated", "Stats": "Stats", diff --git a/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts b/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts index 9b3c3398..ca885a85 100644 --- a/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts @@ -174,47 +174,10 @@ describe('buildSystemPrompt mcp tooling guidance', () => { const workspace = { name: 'Acme' } as unknown as Workspace; const SAFETY_MARKER = 'Operating rules (always in effect)'; - it('renders the server name, tool prefix and text when guidance is present', () => { - const prompt = buildSystemPrompt({ - workspace, - mcpInstructions: [ - { - serverName: 'Tavily', - toolPrefix: 'tavily', - instructions: 'Use tavily_search for fresh web facts; cite sources.', - }, - ], - }); - expect(prompt).toContain('_*`. - expect(prompt).toContain('tavily_*'); - expect(prompt).toContain( - 'Use tavily_search for fresh web facts; cite sources.', - ); - }); - - it('renders nothing for an empty list', () => { - const prompt = buildSystemPrompt({ workspace, mcpInstructions: [] }); - expect(prompt).not.toContain(' { - const prompt = buildSystemPrompt({ workspace }); - expect(prompt).not.toContain(' { - const prompt = buildSystemPrompt({ - workspace, - mcpInstructions: [ - { serverName: 'A', toolPrefix: 'a', instructions: ' ' }, - { serverName: 'B', toolPrefix: 'b', instructions: '' }, - ], - }); - expect(prompt).not.toContain(' { const prompt = buildSystemPrompt({ workspace, diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index 7189672f..8a807ba5 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -332,38 +332,14 @@ export class AiChatService { ); } - const system = buildSystemPrompt({ - workspace, - adminPrompt: resolved?.systemPrompt, - // The role (pre-resolved by the controller) REPLACES the persona layer; - // the safety framework is still appended by buildSystemPrompt. - roleInstructions: role?.instructions, - // Server-validated open page (authoritative title), not the client value. - openedPage: openPageContext, - // Guidance only for servers that connected and yielded ≥1 callable tool. - mcpInstructions: external.instructions, - }); - - // Pass the resolved chatId so the write tools can mint provenance tokens - // (access + collab) carrying { actor:'agent', aiChatId: chatId }, making - // agent REST/collab writes attributable and non-spoofable (§6.5/§6.6). - const docmostTools = await this.tools.forUser( - user, - sessionId, - workspace.id, - chatId, - // Same server-validated open page used by the system prompt above; exposed - // to the model via getCurrentPage so page identity (and the AUTHORITATIVE - // title) survives prompt mangling and client title spoofing (#159). - openPageContext, - ); - - const tools = { ...external.tools, ...docmostTools }; - // Close every external client EXACTLY ONCE across the turn's terminal // callbacks (onFinish/onError/onAbort all fire at most once collectively, - // but guard anyway). Close errors are swallowed so they never break the - // response. + // but guard anyway). DEFINED HERE — before the prompt/toolset are built — so + // that if buildSystemPrompt or forUser throws AFTER the external lease was + // taken (toolsFor above), the lease is still released. Otherwise its refCount + // stays >= 1 forever and the external undici sockets leak until restart + // (#180 reorder moved toolsFor ahead of these; #185 review). Close errors are + // swallowed so they never break the response. let clientsClosed = false; const closeExternalClients = async (): Promise => { if (clientsClosed) return; @@ -381,6 +357,44 @@ export class AiChatService { ); }; + // Build the system prompt + Docmost toolset. If either throws after the + // external MCP lease was taken above, release the lease before rethrowing so + // the leased transports are not leaked (#185 review). + let system: string; + let docmostTools: Awaited>; + try { + system = buildSystemPrompt({ + workspace, + adminPrompt: resolved?.systemPrompt, + // The role (pre-resolved by the controller) REPLACES the persona layer; + // the safety framework is still appended by buildSystemPrompt. + roleInstructions: role?.instructions, + // Server-validated open page (authoritative title), not the client value. + openedPage: openPageContext, + // Guidance only for servers that connected and yielded ≥1 callable tool. + mcpInstructions: external.instructions, + }); + + // Pass the resolved chatId so the write tools can mint provenance tokens + // (access + collab) carrying { actor:'agent', aiChatId: chatId }, making + // agent REST/collab writes attributable and non-spoofable (§6.5/§6.6). + docmostTools = await this.tools.forUser( + user, + sessionId, + workspace.id, + chatId, + // Same server-validated open page used by the system prompt above; + // exposed to the model via getCurrentPage so page identity (and the + // AUTHORITATIVE title) survives prompt mangling / client title spoofing. + openPageContext, + ); + } catch (err) { + await closeExternalClients(); + throw err; + } + + const tools = { ...external.tools, ...docmostTools }; + // Persist the assistant message. Used by onFinish (full result) and the // abort/error paths (partial result). Guarded so we persist at most once. let persisted = false; diff --git a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts index 1621b3e5..b46e24c0 100644 --- a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts +++ b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts @@ -1,7 +1,7 @@ import { Injectable } from '@nestjs/common'; import { InjectKysely } from 'nestjs-kysely'; import { KyselyDB, KyselyTransaction } from '../../types/kysely.types'; -import { dbOrTx, jsonbBind } from '../../utils'; +import { dbOrTx, jsonbBind, parseJsonbValue } from '../../utils'; import { AiAgentRole } from '@docmost/db/types/entity.types'; /** The jsonb shape persisted in `model_config` (loosely typed for the column). */ @@ -183,17 +183,13 @@ export class AiAgentRoleRepo { export function parseModelConfig( value: unknown, ): Record | null { - let v: unknown = value; - if (typeof v === 'string') { - try { - v = JSON.parse(v); // legacy double-encoded read - } catch { - return null; - } - } - return v !== null && typeof v === 'object' && !Array.isArray(v) - ? (v as Record) - : null; + // Shape guard only; the legacy double-encoding self-heal lives in + // parseJsonbValue (database/utils.ts). + return parseJsonbValue( + value, + (v): v is Record => + v !== null && typeof v === 'object' && !Array.isArray(v), + ); } /** Normalize a DB row so `modelConfig` is always an object or null. The cast diff --git a/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts index b6243f7c..8bcfc661 100644 --- a/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts +++ b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts @@ -1,7 +1,7 @@ import { Injectable, Logger } from '@nestjs/common'; import { InjectKysely } from 'nestjs-kysely'; import { KyselyDB, KyselyTransaction } from '../../types/kysely.types'; -import { dbOrTx, jsonbBind } from '../../utils'; +import { dbOrTx, jsonbBind, parseJsonbValue } from '../../utils'; import { AiMcpServer } from '@docmost/db/types/entity.types'; const logger = new Logger('AiMcpServerRepo'); @@ -161,17 +161,13 @@ export function blankToNull(value: string | null | undefined): string | null { * array with a non-string element all become null (unrestricted). */ export function parseToolAllowlist(value: unknown): string[] | null { - let v: unknown = value; - if (typeof v === 'string') { - try { - v = JSON.parse(v); // legacy double-encoded read - } catch { - return null; - } - } - return Array.isArray(v) && v.every((x) => typeof x === 'string') - ? (v as string[]) - : null; + // Shape guard only; the legacy double-encoding self-heal lives in + // parseJsonbValue (database/utils.ts). + return parseJsonbValue( + value, + (v): v is string[] => + Array.isArray(v) && v.every((x) => typeof x === 'string'), + ); } /** diff --git a/apps/server/src/database/utils.ts b/apps/server/src/database/utils.ts index c493798c..9ed16cbb 100644 --- a/apps/server/src/database/utils.ts +++ b/apps/server/src/database/utils.ts @@ -64,3 +64,29 @@ export function jsonbBind( } return sql`${JSON.stringify(value)}::text::jsonb`; } + +/** + * READ-side counterpart to {@link jsonbBind}: tolerantly decode a jsonb value + * read back from the DB and validate its shape with `guard`. THE single place + * the legacy double-encoding self-heal lives, so repos keep only a type-guard. + * + * A row written by the old `::jsonb` bind round-trips as a JSON STRING (see the + * quirk in jsonbBind), so the driver hands back e.g. `'["a"]'` / `'{"k":1}'` + * rather than the structure. This parses such a string once, then applies the + * caller's `guard`. Returns `null` for null / an unparseable string / a value + * the guard rejects (so a corrupt or wrong-shaped value degrades to "unset"). + */ +export function parseJsonbValue( + value: unknown, + guard: (v: unknown) => v is T, +): T | null { + let v: unknown = value; + if (typeof v === 'string') { + try { + v = JSON.parse(v); // legacy double-encoded read + } catch { + return null; + } + } + return guard(v) ? v : null; +} diff --git a/packages/mcp/build/client.js b/packages/mcp/build/client.js index 8c5fcc9d..a5219c5c 100644 --- a/packages/mcp/build/client.js +++ b/packages/mcp/build/client.js @@ -11,7 +11,7 @@ import { updatePageContentRealtime, replacePageContent, markdownToProseMirror, m import { footnoteWarningsField } from "./lib/footnote-analyze.js"; import { buildPageTree } from "./lib/tree.js"; import { serializeDocmostMarkdown, parseDocmostMarkdown, } from "./lib/markdown-document.js"; -import { replaceNodeById, deleteNodeById, insertNodeRelative, buildOutline, getNodeByRef, readTable, insertTableRow, deleteTableRow, updateTableCell, } from "./lib/node-ops.js"; +import { replaceNodeById, deleteNodeById, assertUnambiguousMatch, insertNodeRelative, buildOutline, getNodeByRef, readTable, insertTableRow, deleteTableRow, updateTableCell, } from "./lib/node-ops.js"; import { withPageLock } from "./lib/page-lock.js"; import { applyTextEdits, } from "./lib/json-edit.js"; import { getCollabToken, performLogin } from "./lib/auth-utils.js"; @@ -1331,12 +1331,9 @@ export class DocmostClient { 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.`); - } + // 0 -> "no node"; >1 -> "ambiguous, refused" (the transform already skipped + // the write for any count !== 1). Single shared guard (#159, #185 review). + assertUnambiguousMatch("patch_node", "replace", replaced, nodeId, pageId); return { success: true, replaced, nodeId, verify: mutation.verify }; } /** @@ -1428,12 +1425,9 @@ export class DocmostClient { 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.`); - } + // 0 -> "no node"; >1 -> "ambiguous, refused" (the transform already skipped + // the write for any count !== 1). Single shared guard (#159, #185 review). + assertUnambiguousMatch("delete_node", "delete", deleted, nodeId, pageId); return { success: true, deleted, nodeId, verify: mutation.verify }; } /** Build the public share URL for a page. */ diff --git a/packages/mcp/build/lib/node-ops.js b/packages/mcp/build/lib/node-ops.js index 3f8ca1a8..7f8490ca 100644 --- a/packages/mcp/build/lib/node-ops.js +++ b/packages/mcp/build/lib/node-ops.js @@ -77,11 +77,13 @@ export function buildOutline(doc) { const entry = { index: i, type, - id: isObject(block) && isObject(block.attrs) ? block.attrs.id ?? null : null, + id: isObject(block) && isObject(block.attrs) + ? (block.attrs.id ?? null) + : null, firstText: truncate(blockPlainText(block), 100), }; if (type === "heading") { - entry.level = isObject(block.attrs) ? block.attrs.level ?? null : null; + entry.level = isObject(block.attrs) ? (block.attrs.level ?? null) : null; } else if (type === "table") { const headerRow = block.content?.[0]?.content ?? []; @@ -205,6 +207,22 @@ export function deleteNodeById(doc, nodeId) { } return { doc: out, deleted }; } +/** + * Throw a clear, model-actionable error when a node-id write op did NOT match + * exactly one node (#159). `count === 0` -> "no node found"; `count > 1` -> + * "ambiguous, refused" — Docmost duplicates block ids on copy/paste, so a write + * by id could clobber/remove EVERY duplicate. The caller skips the write for any + * `count !== 1` (the transform returns null), so this only REPORTS; nothing was + * changed. No-op for the unambiguous single-match case. + */ +export function assertUnambiguousMatch(op, verb, count, nodeId, pageId) { + if (count === 0) { + throw new Error(`${op}: no node with id "${nodeId}" found on page ${pageId}`); + } + if (count > 1) { + throw new Error(`${op}: id "${nodeId}" is ambiguous — ${count} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to ${verb} all of them; nothing was changed. Re-target with a more specific anchor.`); + } +} /** * Deep-clone `doc` and strip every node/mark attribute whose value is strictly * `undefined`, so the result is safe to hand to Yjs (which throws an opaque @@ -655,7 +673,7 @@ export function readTable(doc, tableRef) { ? cellNode.content[0] : undefined; const id = isObject(firstPara) && isObject(firstPara.attrs) - ? firstPara.attrs.id ?? null + ? (firstPara.attrs.id ?? null) : null; rowIds.push(id); } @@ -683,7 +701,9 @@ export function insertTableRow(doc, tableRef, cells, index) { table.content = []; const rows = table.content.length; const headerRow = table.content[0]; - const headerCells = Array.isArray(headerRow?.content) ? headerRow.content : []; + const headerCells = Array.isArray(headerRow?.content) + ? headerRow.content + : []; // Column count is the WIDEST existing row, so the guard below stays // meaningful for ragged tables and the new row matches the table's width. // Fall back to the supplied cell count only when the table has no rows. @@ -699,7 +719,10 @@ export function insertTableRow(doc, tableRef, cells, index) { } // Resolve the landing index up front so the cell-type decision and the splice // below agree: a valid integer in [0, rows] splices there, else we append. - const landingIndex = typeof index === "number" && Number.isInteger(index) && index >= 0 && index <= rows + const landingIndex = typeof index === "number" && + Number.isInteger(index) && + index >= 0 && + index <= rows ? index : rows; // Seed the id generator with every id already in the doc so the new cell @@ -717,7 +740,7 @@ export function insertTableRow(doc, tableRef, cells, index) { // A row landing at index 0 becomes the new header row, so inherit the // current header cell's type per column (Docmost uses "tableHeader" there); // every other position is a plain data cell. - const cellType = landingIndex === 0 ? headerCells[i]?.type ?? "tableCell" : "tableCell"; + const cellType = landingIndex === 0 ? (headerCells[i]?.type ?? "tableCell") : "tableCell"; newCells.push({ type: cellType, attrs, diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 4616f43d..39ff3146 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -32,6 +32,7 @@ import { import { replaceNodeById, deleteNodeById, + assertUnambiguousMatch, insertNodeRelative, buildOutline, getNodeByRef, @@ -1668,16 +1669,9 @@ export class DocmostClient { }, ); - 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.`, - ); - } + // 0 -> "no node"; >1 -> "ambiguous, refused" (the transform already skipped + // the write for any count !== 1). Single shared guard (#159, #185 review). + assertUnambiguousMatch("patch_node", "replace", replaced, nodeId, pageId); return { success: true, replaced, nodeId, verify: mutation.verify }; } @@ -1812,16 +1806,9 @@ export class DocmostClient { }, ); - 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.`, - ); - } + // 0 -> "no node"; >1 -> "ambiguous, refused" (the transform already skipped + // the write for any count !== 1). Single shared guard (#159, #185 review). + assertUnambiguousMatch("delete_node", "delete", deleted, nodeId, pageId); return { success: true, deleted, nodeId, verify: mutation.verify }; } diff --git a/packages/mcp/src/lib/node-ops.ts b/packages/mcp/src/lib/node-ops.ts index 8a619266..cdb67902 100644 --- a/packages/mcp/src/lib/node-ops.ts +++ b/packages/mcp/src/lib/node-ops.ts @@ -99,12 +99,15 @@ export function buildOutline(doc: any): OutlineEntry[] { const entry: OutlineEntry = { index: i, type, - id: isObject(block) && isObject(block.attrs) ? block.attrs.id ?? null : null, + id: + isObject(block) && isObject(block.attrs) + ? (block.attrs.id ?? null) + : null, firstText: truncate(blockPlainText(block), 100), }; if (type === "heading") { - entry.level = isObject(block.attrs) ? block.attrs.level ?? null : null; + entry.level = isObject(block.attrs) ? (block.attrs.level ?? null) : null; } else if (type === "table") { const headerRow = block.content?.[0]?.content ?? []; entry.rows = block.content?.length ?? 0; @@ -249,6 +252,33 @@ export function deleteNodeById( return { doc: out, deleted }; } +/** + * Throw a clear, model-actionable error when a node-id write op did NOT match + * exactly one node (#159). `count === 0` -> "no node found"; `count > 1` -> + * "ambiguous, refused" — Docmost duplicates block ids on copy/paste, so a write + * by id could clobber/remove EVERY duplicate. The caller skips the write for any + * `count !== 1` (the transform returns null), so this only REPORTS; nothing was + * changed. No-op for the unambiguous single-match case. + */ +export function assertUnambiguousMatch( + op: "patch_node" | "delete_node", + verb: "replace" | "delete", + count: number, + nodeId: string, + pageId: string, +): void { + if (count === 0) { + throw new Error( + `${op}: no node with id "${nodeId}" found on page ${pageId}`, + ); + } + if (count > 1) { + throw new Error( + `${op}: id "${nodeId}" is ambiguous — ${count} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to ${verb} all of them; nothing was changed. Re-target with a more specific anchor.`, + ); + } +} + /** * Deep-clone `doc` and strip every node/mark attribute whose value is strictly * `undefined`, so the result is safe to hand to Yjs (which throws an opaque @@ -644,7 +674,8 @@ function locateTable( if (!isObject(rootClone)) return null; // "#": index into the top-level content array; must be a table. - const indexMatch = typeof tableRef === "string" ? tableRef.match(/^#(\d+)$/) : null; + const indexMatch = + typeof tableRef === "string" ? tableRef.match(/^#(\d+)$/) : null; if (indexMatch) { const index = Number(indexMatch[1]); const block = Array.isArray(rootClone.content) @@ -744,7 +775,7 @@ export function readTable( : undefined; const id = isObject(firstPara) && isObject(firstPara.attrs) - ? firstPara.attrs.id ?? null + ? (firstPara.attrs.id ?? null) : null; rowIds.push(id); } @@ -778,14 +809,17 @@ export function insertTableRow( if (!Array.isArray(table.content)) table.content = []; const rows = table.content.length; const headerRow = table.content[0]; - const headerCells = Array.isArray(headerRow?.content) ? headerRow.content : []; + const headerCells = Array.isArray(headerRow?.content) + ? headerRow.content + : []; // Column count is the WIDEST existing row, so the guard below stays // meaningful for ragged tables and the new row matches the table's width. // Fall back to the supplied cell count only when the table has no rows. let colCount = 0; for (const r of table.content) { - if (isObject(r) && Array.isArray(r.content)) colCount = Math.max(colCount, r.content.length); + if (isObject(r) && Array.isArray(r.content)) + colCount = Math.max(colCount, r.content.length); } if (colCount === 0) colCount = Array.isArray(cells) ? cells.length : 0; @@ -798,7 +832,10 @@ export function insertTableRow( // Resolve the landing index up front so the cell-type decision and the splice // below agree: a valid integer in [0, rows] splices there, else we append. const landingIndex = - typeof index === "number" && Number.isInteger(index) && index >= 0 && index <= rows + typeof index === "number" && + Number.isInteger(index) && + index >= 0 && + index <= rows ? index : rows; @@ -817,7 +854,8 @@ export function insertTableRow( // A row landing at index 0 becomes the new header row, so inherit the // current header cell's type per column (Docmost uses "tableHeader" there); // every other position is a plain data cell. - const cellType = landingIndex === 0 ? headerCells[i]?.type ?? "tableCell" : "tableCell"; + const cellType = + landingIndex === 0 ? (headerCells[i]?.type ?? "tableCell") : "tableCell"; newCells.push({ type: cellType, attrs, @@ -889,9 +927,10 @@ export function updateTableCell( const rowNodes = Array.isArray(table.content) ? table.content : []; const rows = rowNodes.length; const rowNode = rowNodes[row]; - const cols = isObject(rowNode) && Array.isArray(rowNode.content) - ? rowNode.content.length - : 0; + const cols = + isObject(rowNode) && Array.isArray(rowNode.content) + ? rowNode.content.length + : 0; if ( !Number.isInteger(row) || diff --git a/packages/mcp/test/mock/write-order.test.mjs b/packages/mcp/test/mock/write-order.test.mjs new file mode 100644 index 00000000..c3a013f3 --- /dev/null +++ b/packages/mcp/test/mock/write-order.test.mjs @@ -0,0 +1,106 @@ +// Mock-HTTP regression for the body-before-title write order (#159 finding #10, +// PR #185 review pt 3). `updatePage` / `updatePageJson` must write the page BODY +// (collab) BEFORE the title (REST POST /pages/update), so a failed body write +// never leaves a NEW title over the OLD body (split-brain). We point the client +// at a mock server that serves auth + collab-token but has NO WebSocket upgrade +// handler, so the collab body write fails fast; we then assert the title was +// never POSTed. With the pre-fix (title-first) order, /pages/update WOULD be hit +// before the body failed. +import { test, after } from "node:test"; +import assert from "node:assert/strict"; +import http from "node:http"; +import { DocmostClient } from "../../build/client.js"; + +function readBody(req) { + return new Promise((resolve) => { + let raw = ""; + req.on("data", (c) => (raw += c)); + req.on("end", () => resolve(raw)); + }); +} +function startServer(handler) { + return new Promise((resolve) => { + const server = http.createServer(handler); + server.listen(0, "127.0.0.1", () => { + const { port } = server.address(); + resolve({ server, baseURL: `http://127.0.0.1:${port}/api` }); + }); + }); +} +function sendJson(res, status, obj, extraHeaders = {}) { + res.writeHead(status, { + "Content-Type": "application/json", + ...extraHeaders, + }); + res.end(JSON.stringify(obj)); +} + +const openServers = []; +async function spawn(handler) { + const { server, baseURL } = await startServer(handler); + openServers.push(server); + return { server, baseURL }; +} +after(async () => { + await Promise.all(openServers.map((s) => new Promise((r) => s.close(r)))); +}); + +// A mock server that authenticates and hands out a collab token, tracks whether +// the title endpoint was hit, but has NO WS upgrade handler -> collab fails fast. +function makeServer() { + const state = { titlePosted: false }; + const handler = async (req, res) => { + await readBody(req); + if (req.url === "/api/auth/login") { + sendJson( + res, + 200, + { success: true }, + { + "Set-Cookie": "authToken=t; Path=/; HttpOnly", + }, + ); + return; + } + if (req.url === "/api/auth/collab-token") { + sendJson(res, 200, { data: { token: "collab-jwt" } }); + return; + } + if (req.url === "/api/pages/update") { + state.titlePosted = true; + sendJson(res, 200, { data: {} }); + return; + } + sendJson(res, 404, { message: "not found" }); + }; + return { state, handler }; +} + +test("updatePage does NOT POST the title when the body (collab) write fails (#159)", async () => { + const { state, handler } = makeServer(); + const { baseURL } = await spawn(handler); + const client = new DocmostClient(baseURL, "u@e.com", "pw"); + + await assert.rejects(() => + client.updatePage("page-1", "# Heading\n\nsome body", "New Title"), + ); + assert.equal( + state.titlePosted, + false, + "title must NOT be posted when the body write failed (body-first order)", + ); +}); + +test("updatePageJson does NOT POST the title when the body (collab) write fails (#159)", async () => { + const { state, handler } = makeServer(); + const { baseURL } = await spawn(handler); + const client = new DocmostClient(baseURL, "u@e.com", "pw"); + + const doc = { type: "doc", content: [{ type: "paragraph" }] }; + await assert.rejects(() => client.updatePageJson("page-1", doc, "New Title")); + assert.equal( + state.titlePosted, + false, + "title must NOT be posted when the body write failed (body-first order)", + ); +}); diff --git a/packages/mcp/test/unit/node-ops.test.mjs b/packages/mcp/test/unit/node-ops.test.mjs index 155b99a0..694ac93e 100644 --- a/packages/mcp/test/unit/node-ops.test.mjs +++ b/packages/mcp/test/unit/node-ops.test.mjs @@ -5,6 +5,7 @@ import { blockPlainText, replaceNodeById, deleteNodeById, + assertUnambiguousMatch, insertNodeRelative, } from "../../build/lib/node-ops.js"; @@ -216,10 +217,7 @@ test("deleteNodeById removes EVERY node sharing the id", () => { }); test("deleteNodeById does NOT mutate input (deep-equal snapshot)", () => { - const input = doc( - para("p-1", textNode("one")), - para("p-2", textNode("two")), - ); + const input = doc(para("p-1", textNode("one")), para("p-2", textNode("two"))); const snap = snapshot(input); const { doc: out } = deleteNodeById(input, "p-2"); assert.deepEqual(input, snap); @@ -487,3 +485,35 @@ test("insertNodeRelative truly-missing anchor still returns inserted:false", () }); assert.equal(inserted, false); }); + +// assertUnambiguousMatch (#159, #185 review pt 2): the patch_node/delete_node +// guard. Docmost duplicates block ids on copy/paste, so a write by id that +// matches >1 node must be REFUSED (the caller already skipped the write for any +// count !== 1; this reports the error). The duplicate COUNT itself is covered by +// the replaceNodeById/deleteNodeById tests above (count===2 for a 2-dup doc). +test("assertUnambiguousMatch: count 0 throws 'no node found'", () => { + assert.throws( + () => assertUnambiguousMatch("patch_node", "replace", 0, "n1", "p1"), + /patch_node: no node with id "n1" found on page p1/, + ); +}); + +test("assertUnambiguousMatch: count > 1 refuses with an 'ambiguous' error", () => { + assert.throws( + () => assertUnambiguousMatch("patch_node", "replace", 2, "dup", "p1"), + /ambiguous.*Refusing to replace all of them; nothing was changed/, + ); + assert.throws( + () => assertUnambiguousMatch("delete_node", "delete", 3, "dup", "p1"), + /ambiguous.*Refusing to delete all of them; nothing was changed/, + ); +}); + +test("assertUnambiguousMatch: exactly one match does NOT throw", () => { + assert.doesNotThrow(() => + assertUnambiguousMatch("patch_node", "replace", 1, "n1", "p1"), + ); + assert.doesNotThrow(() => + assertUnambiguousMatch("delete_node", "delete", 1, "n1", "p1"), + ); +});