diff --git a/packages/mcp/src/lib/drawio-xml.ts b/packages/mcp/src/lib/drawio-xml.ts index 620b12ea..097cc7a6 100644 --- a/packages/mcp/src/lib/drawio-xml.ts +++ b/packages/mcp/src/lib/drawio-xml.ts @@ -1080,8 +1080,43 @@ export interface PreparedModel { */ export function prepareModel(inputXml: string): PreparedModel { const rawModel = normalizeInput(inputXml); - const { cells, warnings } = lintModel(rawModel); - const modelXml = normalizeXml(rawModel); + // Auto-strip XML comments before linting. The model routinely inserts + // `` into the diagram XML despite the prohibition in the tool + // description; a hard [no-comments] lint failure would force it to regenerate + // the whole diagram (a wasted tool call). Comments carry no diagram semantics, + // so stripping them is always safe. The linter's no-comments rule stays as a + // defense-in-depth backstop for any path that reaches it without this strip. + // + // The strip is GATED on two conditions so the regex only ever targets real + // comment nodes: + // 1. Well-formedness. Well-formed XML forbids a bare `<` inside an attribute + // value (it must be entity-escaped, e.g. `<!--`), so a literal `` that is NOT a comment node; stripping it would + // silently delete author content. Real drawio labels live in `value=` + // attributes (CDATA impossible), so excluding CDATA is free on legitimate + // models; a CDATA-bearing model with a genuine comment then falls to the + // retained no-comments lint backstop (an explicit error) rather than being + // silently corrupted. + const { error: parseError } = parseXml(rawModel); + const hasCdata = rawModel.includes("/g) ?? []) + : []; + const commentCount = commentMatches.length; + const strippedModel = + commentCount > 0 ? rawModel.replace(//g, "") : rawModel; + const stripWarnings = + commentCount > 0 ? [`stripped ${commentCount} XML comment(s)`] : []; + const { cells, warnings } = lintModel(strippedModel); + const modelXml = normalizeXml(strippedModel); const bbox = computeBBox(cells); const cellCount = cells.filter((c) => c.id !== "0" && c.id !== "1").length; // Geometry quality warnings (non-blocking) are appended to any structural @@ -1092,7 +1127,7 @@ export function prepareModel(inputXml: string): PreparedModel { cells, bbox, cellCount, - warnings: [...warnings, ...quality], + warnings: [...stripWarnings, ...warnings, ...quality], hash: mxHash(modelXml), }; } diff --git a/packages/mcp/test/unit/drawio-xml.test.mjs b/packages/mcp/test/unit/drawio-xml.test.mjs index 3295b0f8..b00b271c 100644 --- a/packages/mcp/test/unit/drawio-xml.test.mjs +++ b/packages/mcp/test/unit/drawio-xml.test.mjs @@ -240,6 +240,107 @@ test("prepareModel: returns bbox, cellCount, hash and lints", () => { assert.equal(p.hash, mxHash(normalizeXml(VALID_MODEL))); }); +// --- comment auto-strip (issue #505) --------------------------------------- + +// The model organically inserts `` into diagram XML. prepareModel +// (the shared path for drawioCreate/drawioUpdate/drawioEditCells) strips them +// BEFORE the lint runs and reports the count as a non-blocking warning, so the +// model never hits a hard [no-comments] error and never regenerates the diagram. +test("prepareModel: strips XML comments, lint passes, warns with the count", () => { + const m = + '' + + '' + + '' + + '' + + '' + + '' + + ''; + // Would fail [no-comments] if not stripped first. + const p = prepareModel(m); + // Comments are gone from the canonical model. + assert.ok(!p.modelXml.includes(" b"); +}); + +test("prepareModel: malformed input with a raw b" vertex="1" parent="1">' + + '' + + ''; + const issues = issuesOf(() => prepareModel(m)); + // Rejected (threw DrawioLintError) rather than silently accepted. + assert.ok(issues, "expected prepareModel to reject malformed input"); + // By a real content rule, not swallowed by the strip. + assert.ok( + hasRule(issues, "value-escaping", "2") || hasRule(issues, "well-formed-xml"), + `expected value-escaping/well-formed-xml, got ${JSON.stringify(issues)}`, + ); +}); + +test("prepareModel: a inside a CDATA section is NOT stripped (no silent corruption)", () => { + // A CDATA section is well-formed, so the well-formedness gate alone would let + // the regex delete a literal `` living inside CDATA text — silent + // content loss with a false "stripped" success. The CDATA sub-gate must skip + // the strip; the model then hits the retained no-comments backstop (an explicit + // error) instead of being silently accepted with a strip warning. + const m = + '' + + '' + + '' + + 'keep]]>' + + ''; + const issues = issuesOf(() => prepareModel(m)); + // Rejected (threw), not silently accepted with a strip warning. + assert.ok(issues, "expected prepareModel to reject rather than silently strip"); + // The no-comments backstop caught the literal comment sequence. + assert.ok( + hasRule(issues, "no-comments"), + `expected no-comments backstop, got ${JSON.stringify(issues)}`, + ); +}); + +test("no-comments rule still fires when a comment reaches the linter directly", () => { + // Defense-in-depth: prepareModel strips first, but lintModel itself (the + // backstop) must still reject a raw comment-bearing model. + const m = + '' + + '' + + ''; + const issues = issuesOf(() => lintModel(m)); + assert.ok(hasRule(issues, "no-comments")); +}); + // --- decode chain: plain ----------------------------------------------------- test("decode chain (plain): buildDrawioSvg -> decodeDrawioSvg round-trips byte-stable", () => {