From 6cdffc7b8f461d80057c045542592d6d9b57f127 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 18:40:10 +0300 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20drawioCreate=20=E2=80=94=20=D1=83?= =?UTF-8?q?=D1=81=D0=BF=D0=B5=D1=85=20=D1=81=20warning=20=D0=BD=D0=B0=20?= =?UTF-8?q?=D0=B2=D0=BB=D0=BE=D0=B6=D0=B5=D0=BD=D0=BD=D0=BE=D0=B9=20=D0=B2?= =?UTF-8?q?=D1=81=D1=82=D0=B0=D0=B2=D0=BA=D0=B5,=20=D0=B0=20=D0=BD=D0=B5?= =?UTF-8?q?=20throw=20(#494)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Коммит 3. При вставке диаграммы во вложенный контейнер (анкор внутри callout/ячейки таблицы) узел УЖЕ записан и закоммичен мутацией, но тул кидал ошибку «no addressable # handle». Retry-склонный агент воспринимал это как провал записи и повторял drawioCreate → ДУБЛИКАТ диаграммы (тот же класс double-apply, что в инциденте #435). Правка: ветка insertedIndex<0 возвращает success:true с nodeId:null и warning'ом «written NESTED, saved — do NOT re-create; re-read via getOutline/getPageJson (attachmentId …)» вместо throw. Запись подтверждается, агент знает, что хендла нет и как перечитать — и не ретраит приземлившуюся запись. nodeId стал string|null в drawioCreate и наследующих drawioFromGraph/ drawioFromMermaid (там тот же путь) + в интерфейсе IDrawioMixin. Мок-тест: вложенная вставка не кидает, отдаёт success/nodeId:null/warning и пишет диаграмму РОВНО один раз вложенной (краснеет, если вернуть throw). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/client/drawio.ts | 45 +++++++++++---- packages/mcp/test/mock/drawio-tools.test.mjs | 59 ++++++++++++++++++++ 2 files changed, 93 insertions(+), 11 deletions(-) diff --git a/packages/mcp/src/client/drawio.ts b/packages/mcp/src/client/drawio.ts index c3c4530f..b0e5ad9d 100644 --- a/packages/mcp/src/client/drawio.ts +++ b/packages/mcp/src/client/drawio.ts @@ -57,11 +57,11 @@ import { // Derived from the class below; `implements IDrawioMixin` fails to compile on drift. export interface IDrawioMixin { drawioGet(pageId: string, node: string, format?: "xml" | "svg"): Promise<{ pageId: string; nodeId: string; format: "xml" | "svg"; content: string; meta: { attachmentId: string | null; title: string | null; width: number | null; height: number | null; cellCount: number; hash: string; }; }>; - drawioCreate(pageId: string, where: { position: "before" | "after" | "append"; anchorNodeId?: string; anchorText?: string; }, xml: string, title?: string, layout?: "elk"): Promise<{ success: boolean; nodeId: string; attachmentId: string; warnings: string[]; verify?: any; }>; + drawioCreate(pageId: string, where: { position: "before" | "after" | "append"; anchorNodeId?: string; anchorText?: string; }, xml: string, title?: string, layout?: "elk"): Promise<{ success: boolean; nodeId: string | null; attachmentId: string; warnings: string[]; verify?: any; }>; drawioUpdate(pageId: string, node: string, xml: string, baseHash: string, layout?: "elk"): Promise<{ success: boolean; nodeId: string; attachmentId: string; warnings: string[]; verify?: any; }>; drawioEditCells(pageId: string, node: string, operations: CellOp[], baseHash: string): Promise<{ success: boolean; nodeId: string; attachmentId: string; warnings: string[]; verify?: any; }>; - drawioFromGraph(pageId: string, where: { position: "before" | "after" | "append"; anchorNodeId?: string; anchorText?: string; }, graph: Graph, direction?: "LR" | "RL" | "TB" | "BT", preset?: string, layout?: GraphLayoutMode, node?: string): Promise<{ success: boolean; nodeId: string; attachmentId: string; warnings: string[]; iconsResolved: number; iconsMissing: string[]; verify?: any; }>; - drawioFromMermaid(pageId: string, where: { position: "before" | "after" | "append"; anchorNodeId?: string; anchorText?: string; }, mermaid: string, preset?: string): Promise<{ success: boolean; nodeId: string; attachmentId: string; warnings: string[]; iconsResolved: number; iconsMissing: string[]; verify?: any; }>; + drawioFromGraph(pageId: string, where: { position: "before" | "after" | "append"; anchorNodeId?: string; anchorText?: string; }, graph: Graph, direction?: "LR" | "RL" | "TB" | "BT", preset?: string, layout?: GraphLayoutMode, node?: string): Promise<{ success: boolean; nodeId: string | null; attachmentId: string; warnings: string[]; iconsResolved: number; iconsMissing: string[]; verify?: any; }>; + drawioFromMermaid(pageId: string, where: { position: "before" | "after" | "append"; anchorNodeId?: string; anchorText?: string; }, mermaid: string, preset?: string): Promise<{ success: boolean; nodeId: string | null; attachmentId: string; warnings: string[]; iconsResolved: number; iconsMissing: string[]; verify?: any; }>; } export function DrawioMixin>(Base: TBase): GConstructor & TBase { @@ -164,7 +164,9 @@ export function DrawioMixin>(Ba layout?: "elk", ): Promise<{ success: boolean; - nodeId: string; + // `null` when the diagram was written but nested (no addressable "#" + // handle) — see the nested-insert branch below (#494). + nodeId: string | null; attachmentId: string; warnings: string[]; verify?: any; @@ -276,11 +278,28 @@ export function DrawioMixin>(Ba // anchor), where "#" — which addresses only top-level blocks — // cannot reference it. drawio nodes carry no persisted id, so there is no // stable handle for a nested diagram. - throw new Error( - `drawioCreate: the diagram was inserted on page ${pageId} but not as a ` + - `top-level block, so it has no addressable "#" handle. Anchor ` + - `on a top-level block (or append) so the diagram can be re-read.`, - ); + // + // CRITICAL (#494): the diagram is ALREADY WRITTEN and committed at this + // point (the mutation above succeeded). Throwing here reported a FAILURE for + // a write that in fact landed, so a retry-prone agent re-ran drawioCreate + // and inserted a DUPLICATE diagram (the #435 double-apply class). Return + // SUCCESS with nodeId:null and a warning instead: the write is + // acknowledged, and the agent is told there is no addressable handle and how + // to re-read the diagram — so it never blind-retries a landed write. + return { + success: true, + nodeId: null, + attachmentId: att.id, + warnings: [ + ...prepared.warnings, + `The diagram was written on page ${pageId} but as a NESTED block (not ` + + `top-level), so it has no addressable "#" handle. It is saved ` + + `— do NOT re-create it. To read or edit it, locate it via getOutline ` + + `/ getPageJson (attachmentId ${att.id}). To get a stable "#" ` + + `handle, anchor on a top-level block (or append).`, + ], + verify: mutation.verify, + }; } // The returned handle is POSITIONAL ("#"): valid for the immediate @@ -597,7 +616,9 @@ export function DrawioMixin>(Ba node?: string, ): Promise<{ success: boolean; - nodeId: string; + // `null` when written nested (no addressable handle) — inherited from + // drawioCreate (#494). + nodeId: string | null; attachmentId: string; warnings: string[]; iconsResolved: number; @@ -682,7 +703,9 @@ export function DrawioMixin>(Ba preset?: string, ): Promise<{ success: boolean; - nodeId: string; + // `null` when written nested (no addressable handle) — inherited from + // drawioFromGraph/drawioCreate (#494). + nodeId: string | null; attachmentId: string; warnings: string[]; iconsResolved: number; diff --git a/packages/mcp/test/mock/drawio-tools.test.mjs b/packages/mcp/test/mock/drawio-tools.test.mjs index 464dbfd9..4be3865e 100644 --- a/packages/mcp/test/mock/drawio-tools.test.mjs +++ b/packages/mcp/test/mock/drawio-tools.test.mjs @@ -170,6 +170,65 @@ test("drawioCreate: before/after requires exactly one anchor", async () => { ); }); +// #494 — a NESTED insert (anchored inside a callout/table cell) lands a write +// that "#" cannot address. The tool used to THROW here even though the +// diagram was already committed, so a retry-prone agent re-created a DUPLICATE. +// It must now report SUCCESS (nodeId:null + a warning) so the agent never +// blind-retries a landed write. This REDDENS if the throw is restored (the +// assert.doesNotReject + success asserts would fail). +test("drawioCreate: a NESTED insert succeeds with nodeId:null + a warning (no throw, no duplicate)", async () => { + const pageDoc = { + type: "doc", + content: [ + { + type: "callout", + attrs: { id: "co1" }, + content: [ + { + type: "paragraph", + attrs: { id: "inner" }, + content: [{ type: "text", text: "hello inner" }], + }, + ], + }, + ], + }; + const { client, calls } = makeClient({ pageDoc }); + + let res; + await assert.doesNotReject(async () => { + res = await client.drawioCreate( + "page1", + { position: "after", anchorNodeId: "inner" }, + MODEL, + ); + }); + + // The write is acknowledged as a SUCCESS... + assert.equal(res.success, true); + // ...but with NO addressable "#" handle (it is nested). + assert.equal(res.nodeId, null); + assert.equal(res.attachmentId, "att-1"); + // A warning tells the agent it is saved (do not re-create) and how to re-read. + assert.ok( + res.warnings.some((w) => /NESTED|do NOT re-create/i.test(w)), + "missing the nested-write warning", + ); + + // The diagram was written EXACTLY ONCE, nested inside the callout (not a + // top-level block) — proving it really landed (so a retry would duplicate). + assert.equal(calls.uploads.length, 1); + assert.equal(calls.mutations.length, 1); + const topLevel = calls.mutations[0].doc.content; + assert.ok( + !topLevel.some((b) => b && b.type === "drawio"), + "the diagram must be nested, not a top-level block", + ); + const nested = findDrawio(calls.mutations[0].doc); + assert.equal(nested.length, 1, "exactly one diagram written"); + assert.equal(nested[0].attrs.attachmentId, "att-1"); +}); + // --- drawioGet ------------------------------------------------------------ test("drawioGet: decodes the model and returns meta with a hash", async () => {