fix(mcp): route #502 extension flags per write tool; add server import flag (#502 review)

Internal review found the two layered-extension flags landed in the SHARED markdownToProseMirrorCanonical wrapper, which mis-covered two tools. Move the decision to each caller by the tool's semantics. BLOCKER 1 — createPage was NOT covered. createPage POSTs to the server /pages/import endpoint, which imported with DEFAULTS (math + fuzzy autolink ON), so an agent's `$x=1$` still became a formula and `www.host` still autolinked. Add an optional `disableMarkdownExtensions` multipart field to /pages/import (default OFF, so human file uploads keep math ON); import.controller reads it, import.service.importPage/processMarkdown thread it into markdownToProseMirror. MCP createPage sends it true. BLOCKER 2 — import_page_markdown was wrongly disabled. It goes through the same wrapper, so hardcoding parseMath:false degraded an exported `$x^2$` to literal text on re-import, breaking the #328 lossless export→import pair. The wrapper no longer hardcodes the flags: it takes them from the caller and DEFAULTS to the package importer defaults (extensions ON). Callers now set them explicitly: - updatePageMarkdown (updatePageContentRealtime) -> OFF - patch_node/insert_node (importMarkdownFragment) -> OFF (unchanged) - import_page_markdown -> DEFAULTS (math ON) — #328 round-trip restored Tests: rewrite the mcp write test around the corrected per-caller semantics (agent-write OFF, import_page_markdown DEFAULTS incl. the REAL #328 round-trip); add a collab-backed wiring test driving client.updatePage (OFF) and client.importPageMarkdown (DEFAULTS) end-to-end; add a createPage multipart-flag test; add a server import.service.processMarkdown spec (flag OFF -> literal / no autolink, default -> math ON for human uploads); reword the prosemirror-markdown round-trip test to its package-default scope. Mutation-verified both caller wirings (flip updatePageMarkdown -> defaults reddens the OFF wiring test; make the wrapper default OFF reddens the #328 round-trip). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-12 06:58:48 +03:00
parent 44cdb5a4c3
commit 3163f50c98
9 changed files with 474 additions and 50 deletions
@@ -85,6 +85,14 @@ export class ImportController {
      throw new BadRequestException('spaceId is required');
    }

+    // #502: optional multipart field. Only the MCP agent `createPage` path sends
+    // `disableMarkdownExtensions=true` (its body is agent-authored plain prose /
+    // config, so a `$…$` span must stay literal and a bare `www.host` must not
+    // autolink). A HUMAN file upload omits the field, so it stays false and math
+    // + autolink remain ON for human imports. Settable ONLY via this API param.
+    const disableMarkdownExtensions =
+      file.fields?.disableMarkdownExtensions?.value === 'true';
+
    const ability = await this.spaceAbility.createForUser(user, spaceId);
    if (ability.cannot(SpaceCaslAction.Edit, SpaceCaslSubject.Page)) {
      throw new ForbiddenException();
@@ -95,6 +103,7 @@ export class ImportController {
      user.id,
      spaceId,
      workspace.id,
+      disableMarkdownExtensions,
    );

    const ext = path.extname(file.filename).toLowerCase();
@@ -0,0 +1,67 @@
+// Importing ImportService transitively loads import-formatter.ts, which imports
+// the ESM-only @sindresorhus/slugify (not in jest's transform allowlist). It is
+// irrelevant to this path, so mock it to keep the module graph loadable (mirrors
+// the sibling import.service specs).
+jest.mock('@sindresorhus/slugify', () => ({
+  __esModule: true,
+  default: (input: string) => String(input),
+}));
+
+import { ImportService } from './import.service';
+
+// #502 BLOCKER 1: the server markdown import path (`/pages/import`) now accepts an
+// optional `disableMarkdownExtensions` flag threaded into `processMarkdown`.
+//   - MCP agent `createPage` sends it TRUE  -> extensions OFF (a `$…$` config span
+//     stays literal, a schemeless `www.host` is not autolinked).
+//   - a HUMAN file upload omits it (default FALSE) -> extensions ON, so a real
+//     `$x^2$` still becomes a formula (human imports unaffected).
+// `processMarkdown` only uses the imported converter (no injected deps on this
+// path), so the service is constructed with null deps for this focused unit test.
+
+function makeService(): ImportService {
+  return new ImportService(null as any, null as any, null as any, null as any);
+}
+
+function findAll(node: any, type: string, acc: any[] = []): any[] {
+  if (!node || typeof node !== 'object') return acc;
+  if (node.type === type) acc.push(node);
+  if (Array.isArray(node.content)) for (const c of node.content) findAll(c, type, acc);
+  return acc;
+}
+function hasLink(node: any): boolean {
+  return findAll(node, 'text').some((t: any) =>
+    t.marks?.some((m: any) => m.type === 'link'),
+  );
+}
+
+describe('ImportService.processMarkdown — #502 disableMarkdownExtensions', () => {
+  it('disableMarkdownExtensions=true (MCP createPage): `$…$` stays literal, no math', async () => {
+    const svc = makeService();
+    const doc = await svc.processMarkdown('export A=$FOO and B=$BAR done', true);
+    expect(findAll(doc, 'mathInline')).toHaveLength(0);
+  });
+
+  it('disableMarkdownExtensions=true: a schemeless www is NOT autolinked', async () => {
+    const svc = makeService();
+    const doc = await svc.processMarkdown('see www.example.com here', true);
+    expect(hasLink(doc)).toBe(false);
+  });
+
+  it('disableMarkdownExtensions=true: an explicit https:// STILL links', async () => {
+    const svc = makeService();
+    const doc = await svc.processMarkdown('see https://example.com here', true);
+    expect(hasLink(doc)).toBe(true);
+  });
+
+  it('DEFAULT (human upload): a real `$x^2$` DOES become a math node', async () => {
+    const svc = makeService();
+    const doc = await svc.processMarkdown('$x^2$');
+    expect(findAll(doc, 'mathInline')).toHaveLength(1);
+  });
+
+  it('DEFAULT (human upload): a schemeless www IS autolinked', async () => {
+    const svc = makeService();
+    const doc = await svc.processMarkdown('see www.example.com here');
+    expect(hasLink(doc)).toBe(true);
+  });
+});
@@ -52,6 +52,12 @@ export class ImportService {
    userId: string,
    spaceId: string,
    workspaceId: string,
+    // #502: when true, the markdown importer runs with the two layered
+    // extensions OFF (a `$…$` span stays literal text; a schemeless `www.host` is
+    // NOT autolinked). ONLY the MCP agent `createPage` path sets this; a HUMAN
+    // file upload never passes it, so it defaults false and math/autolink stay ON
+    // for human imports (their `$x^2$` still becomes a formula).
+    disableMarkdownExtensions = false,
  ) {
    const file = await filePromise;
    const fileBuffer = await file.toBuffer();
@@ -66,7 +72,10 @@ export class ImportService {

    try {
      if (fileExtension.endsWith('.md')) {
-        prosemirrorState = await this.processMarkdown(fileContent);
+        prosemirrorState = await this.processMarkdown(
+          fileContent,
+          disableMarkdownExtensions,
+        );
      } else if (fileExtension.endsWith('.html')) {
        prosemirrorState = await this.processHTML(fileContent);
      }
@@ -138,7 +147,12 @@ export class ImportService {
    return createdPage;
  }

-  async processMarkdown(markdownInput: string): Promise<any> {
+  async processMarkdown(
+    markdownInput: string,
+    // #502: forwarded to the importer. DEFAULT false keeps math + fuzzy autolink
+    // ON (human uploads unaffected); the MCP agent `createPage` path passes true.
+    disableMarkdownExtensions = false,
+  ): Promise<any> {
    // Canonical markdown -> ProseMirror JSON directly via
    // `@docmost/prosemirror-markdown` (issue #345) — no HTML intermediate and no
    // second editor-ext markdown layer. Foreign markdown surfaces the strict
@@ -147,7 +161,12 @@ export class ImportService {
    // The HTML-cleanup pass (`normalizeImportHtml`) is intentionally skipped here:
    // it targets foreign *HTML* (Notion/XWiki), which only ever arrives on the
    // `.html` path (`processHTML`), never as canonical markdown.
-    return markdownToProseMirror(normalizeForeignMarkdown(markdownInput));
+    return markdownToProseMirror(
+      normalizeForeignMarkdown(markdownInput),
+      disableMarkdownExtensions
+        ? { parseMath: false, fuzzyLinkify: false }
+        : undefined,
+    );
  }

  async processHTML(htmlInput: string): Promise<any> {