fix(mcp): drawio — авто-стрип XML-комментариев вместо lint-ошибки (#505)

Модель органически вставляет `<!-- ... -->` в mxGraph-XML вопреки запрету
в описании тула (природа LLM, промптингом не лечится). Жёсткая ошибка
линтера [no-comments] заставляла её ПОЛНОСТЬЮ перегенерировать диаграмму —
впустую потраченный tool-call на почти каждой первой генерации.

Теперь в общем prepare-пути (`prepareModel`, через который идут
drawioCreate/drawioUpdate/drawioEditCells) комментарии срезаются ДО линта:
`xml.replace(/<!--[\s\S]*?-->/g, "")`. Комментарии не несут семантики, так
что стрип безопасен. Число срезанных уходит в `warnings[]` как
`stripped N XML comment(s)` (только при N > 0) — модель это видит, но НЕ
ретраит.

Стрип ГЕЙТИТСЯ двумя условиями, чтобы регэксп бил только по настоящим
comment-нодам:
  1. well-formedness — сначала парсим модель; на malformed-входе (сырой
     неэкранированный `<!--` внутри значения атрибута на пути create/update,
     где normalizeInput отдаёт строку без парсинга) стрип НЕ выполняется,
     иначе он молча вырезал бы текст автора. Вход отклоняют правила
     value-escaping / well-formed-xml, автор видит реальную ошибку.
  2. отсутствие CDATA — CDATA-секция well-formed, но её текст может содержать
     литеральный `<!-- ... -->`, который НЕ является comment-нодой; стрип
     молча удалил бы контент автора. Настоящий drawio держит подписи в
     атрибутах `value=` (CDATA невозможен), так что исключение CDATA бесплатно
     на легитимных моделях; CDATA-модель с реальным комментарием падает на
     сохранённый backstop no-comments (явная ошибка), а не тихо портится.

Правило `no-comments` в линтере оставлено как defense-in-depth backstop.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/server/src/core/page/page.controller.ts

@@ -818,11 +818,6 @@ export class PageController {
       throw new NotFoundException('Page not found');
     }
 
-    // Target-only validateCanView is intentional: getPageBreadCrumbs returns
-    // the full ancestor chain WITHOUT per-ancestor permission filtering. Safe
-    // because page restrictions inherit down the tree, so any ancestor the
-    // caller could not view would already hide the target here — see the
-    // getPageBreadCrumbs docstring / #471.
     await this.pageAccessService.validateCanView(page, user);
 
     return this.pageService.getPageBreadCrumbs(page.id);

apps/server/src/core/page/services/page.service.ts

@@ -1071,29 +1071,6 @@ export class PageService {
     });
   }
 
-  /**
-   * Walk the ancestor chain of `childPageId` up to the space root, filtered
-   * ONLY by `deletedAt` (+ MAX_PAGE_TREE_DEPTH) — WITHOUT per-ancestor
-   * permission filtering. Callers that expose this to a user (the
-   * `/breadcrumbs` endpoint) validate `validateCanView` on the TARGET page
-   * only, then return the whole chain of ancestor titles (#471).
-   *
-   * This is safe — NOT a title leak — because page restrictions inherit DOWN
-   * the tree: to view a page the caller must hold permission on EVERY
-   * restricted ancestor (`validateCanView` -> `canUserAccessPage` checks the
-   * full ancestor chain — see page-access.service.ts / page-permission.repo.ts
-   * `canUserEditPage`). A restricted ancestor the caller may not see would
-   * therefore already hide the TARGET page itself, so every ancestor reachable
-   * here is one the caller is already entitled to view (content stays gated
-   * regardless — getPage/getNode re-check permissions).
-   *
-   * Note the guarantee is the narrow "may view a descendant => may view its
-   * ancestors", NOT "space membership sees every page" — restricted subtrees do
-   * hide pages from members. Per-ancestor permission filtering here was
-   * considered and declined as redundant given the inheritance invariant above
-   * (#471). The same chain feeds the web-UI breadcrumb bar under identical CASL
-   * scope.
-   */
   async getPageBreadCrumbs(childPageId: string, trx?: KyselyTransaction) {
     const ancestors = await dbOrTx(this.db, trx)
       .withRecursive('page_ancestors', (db) =>

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. `&lt;!--`), so a literal `<!--`
+  //      cannot hide in a value. On MALFORMED input (e.g. a raw unescaped `<!--`
+  //      inside a value on the <mxGraphModel> create/update path, which
+  //      normalizeInput passes through unparsed) we must NOT strip: truncating
+  //      that value would silently corrupt the author's label. We leave it intact
+  //      and let lintModel's value-escaping / well-formed-xml rules reject it so
+  //      the author sees the real error.
+  //   2. No CDATA. A CDATA section is well-formed yet its text may contain 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("<![CDATA[");
+  const commentMatches =
+    parseError === null && !hasCdata
+      ? (rawModel.match(/<!--[\s\S]*?-->/g) ?? [])
+      : [];
+  const commentCount = commentMatches.length;
+  const strippedModel =
+    commentCount > 0 ? rawModel.replace(/<!--[\s\S]*?-->/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),
   };
 }

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 =
+    '<mxGraphModel><root>' +
+    '<!-- header comment -->' +
+    '<mxCell id="0"/><mxCell id="1" parent="0"/>' +
+    '<mxCell id="2" value="Hello" vertex="1" parent="1">' +
+    '<!-- inline note -->' +
+    '<mxGeometry x="0" y="0" width="10" height="10" as="geometry"/></mxCell>' +
+    '</root></mxGraphModel>';
+  // Would fail [no-comments] if not stripped first.
+  const p = prepareModel(m);
+  // Comments are gone from the canonical model.
+  assert.ok(!p.modelXml.includes("<!--"));
+  assert.ok(!p.modelXml.includes("header comment"));
+  assert.ok(!p.modelXml.includes("inline note"));
+  // No [no-comments] error was raised (prepareModel returned instead of throwing).
+  // A single strip warning naming the count (2) is present.
+  assert.ok(p.warnings.includes("stripped 2 XML comment(s)"));
+});
+
+test("prepareModel: zero comments emits no strip warning", () => {
+  const p = prepareModel(VALID_MODEL);
+  assert.ok(!p.warnings.some((w) => w.startsWith("stripped")));
+});
+
+test("prepareModel: an entity-escaped <!-- inside a value is NOT stripped", () => {
+  const m =
+    '<mxGraphModel><root>' +
+    '<mxCell id="0"/><mxCell id="1" parent="0"/>' +
+    '<mxCell id="2" value="a &lt;!-- x --&gt; b" vertex="1" parent="1">' +
+    '<mxGeometry x="0" y="0" width="10" height="10" as="geometry"/></mxCell>' +
+    '</root></mxGraphModel>';
+  const p = prepareModel(m);
+  // Nothing was mistaken for a comment node.
+  assert.ok(!p.warnings.some((w) => w.startsWith("stripped")));
+  // The escaped sequence survives round-trip in the value.
+  const cell = p.cells.find((c) => c.id === "2");
+  assert.equal(cell.value, "a <!-- x --> b");
+});
+
+test("prepareModel: malformed input with a raw <!-- in a value is NOT stripped (no silent truncation)", () => {
+  // A literal, UNescaped `<!--` inside an attribute value makes the XML
+  // malformed (raw `<` is illegal in a value). The comment-strip is gated on
+  // well-formedness, so it must NOT touch this input — otherwise the author's
+  // label ` secret ` would be silently deleted and the corrupt diagram accepted.
+  // Instead lintModel's value-escaping / well-formed-xml rules must reject it.
+  const m =
+    '<mxGraphModel><root>' +
+    '<mxCell id="0"/><mxCell id="1" parent="0"/>' +
+    '<mxCell id="2" value="a <!-- secret --> b" vertex="1" parent="1">' +
+    '<mxGeometry x="0" y="0" width="10" height="10" as="geometry"/></mxCell>' +
+    '</root></mxGraphModel>';
+  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 `<!-- x -->` 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 =
+    '<mxGraphModel><root>' +
+    '<mxCell id="0"/><mxCell id="1" parent="0"/>' +
+    '<mxCell id="2" vertex="1" parent="1"><mxGeometry x="0" y="0" width="10" height="10" as="geometry"/>' +
+    '<foo><![CDATA[keep<!-- x -->keep]]></foo></mxCell>' +
+    '</root></mxGraphModel>';
+  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 =
+    '<mxGraphModel><root>' +
+    '<!-- unstripped --><mxCell id="0"/><mxCell id="1" parent="0"/>' +
+    '</root></mxGraphModel>';
+  const issues = issuesOf(() => lintModel(m));
+  assert.ok(hasRule(issues, "no-comments"));
+});
+
 // --- decode chain: plain -----------------------------------------------------
 
 test("decode chain (plain): buildDrawioSvg -> decodeDrawioSvg round-trips byte-stable", () => {