fix(ai-chat): write-class-ретрай только для доверенного внутреннего Docmost-сервера (#489 ревью)

MEDIUM (реальная брешь): SHARED_TOOL_WRITE_CLASS — имена ТУЛОВ Docmost, но
mergeNamespaced применял мапу к тулам ЛЮБОГО стороннего MCP-сервера по совпадению
rawName. Сторонний WRITE-тул, названный как Docmost-read (getPage/listPages/…),
наследовал readOnly → авто-ретрай при транспортной ошибке → double-apply (класс
#435). Гарантия «unknown third-party → write → не ретраится» была ЛОЖНОЙ при
коллизии имён.

Фикс: write-class мапа применяется ТОЛЬКО к серверу, про который известно, что
это внутренний Docmost-MCP (isInternalDocmostServer). Сейчас это ВСЕГДА false —
в этом пути нет встроенного/доверенного Docmost-сервера: все ai_mcp_servers суть
сторонние admin-конфиги, а собственные тулы Docmost идут отдельным in-app путём
(docmostTools), не через mcp-clients. Значит НИ ОДИН сторонний тул не получает
readOnly по коллизии и не авто-ретраится (undefined → трактуется как write).
Мапа грузится лениво только если есть доверенный сервер (иначе ESM-импорт
пропускается). isInternalDocmostServer — метод-сим, флипается при появлении
доверенного сервера (kind/isBuiltin-колонка или сконфигурённый self-MCP URL).

LOW: reconnect не наблюдает composed abort во время 5-сек handshake — задокумен-
тировано (окно поздней отмены ≤5s, сокет закрывается на turn-end); проброс
composed в общий CAS-дедуплицированный reconnect намеренно НЕ сделан (отменил бы
реконнект, нужный конкурентному живому вызову).

Тест: сторонний WRITE-тул с именем getPage при транспортной ошибке НЕ ретраится;
mutation-verify — форсирование trusted делает тест красным (чужой getPage ретраится).

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

apps/server/src/core/ai-chat/external-mcp/mcp-clients.recovery.spec.ts

@@ -55,7 +55,7 @@ const server = (over: Partial<FakeServer> = {}): FakeServer => ({
   ...over,
 });
 
-function buildService(servers: FakeServer[]) {
+function buildService(servers: FakeServer[], trusted = false) {
   const repo = { listEnabled: jest.fn().mockResolvedValue(servers) };
   const service = new McpClientsService(repo as never, {} as never);
   // Seed a DETERMINISTIC write-class map so the retry gate is controlled here
@@ -67,6 +67,21 @@ function buildService(servers: FakeServer[]) {
     getPage: 'readOnly',
     patchNode: 'write',
   });
+  // The service only APPLIES that map to a TRUSTED internal Docmost server
+  // (isInternalDocmostServer, really false for every third-party row). A retry
+  // test needs a trusted server to exercise the readOnly-retry path at all, so it
+  // passes trusted=true to model a Docmost-origin server; the third-party
+  // double-apply test leaves it at the real value (false).
+  if (trusted) {
+    jest
+      .spyOn(
+        service as unknown as {
+          isInternalDocmostServer: (s: FakeServer) => boolean;
+        },
+        'isInternalDocmostServer',
+      )
+      .mockReturnValue(true);
+  }
   return { service, repo };
 }
 
@@ -121,7 +136,7 @@ describe('McpClientsService in-run transport recovery (#489)', () => {
 
   it('a readOnly tool whose transport breaks reconnects and retries WITHIN the same run', async () => {
     const realErr = await realSocketResetError();
-    const { service } = buildService([server()]);
+    const { service } = buildService([server()], true);
     const first = jest.fn().mockRejectedValue(realErr);
     const second = jest.fn().mockResolvedValue({ ok: true });
     const connectSpy = stubConnect(service, 'getPage', [first, second]);
@@ -146,7 +161,7 @@ describe('McpClientsService in-run transport recovery (#489)', () => {
 
   it('a WRITE tool does NOT auto-retry on a transport error (indeterminate)', async () => {
     const realErr = await realSocketResetError();
-    const { service } = buildService([server()]);
+    const { service } = buildService([server()], true);
     const exec = jest.fn().mockRejectedValue(realErr);
     const connectSpy = stubConnect(service, 'patchNode', [exec]);
 
@@ -168,7 +183,7 @@ describe('McpClientsService in-run transport recovery (#489)', () => {
 
   it('does NOT retry (or reconnect) after the run is aborted (Stop)', async () => {
     const realErr = await realSocketResetError();
-    const { service } = buildService([server()]);
+    const { service } = buildService([server()], true);
     const controller = new AbortController();
     // The transport error arrives, but the run was Stopped in the same tick.
     const first = jest.fn().mockImplementation(async () => {
@@ -194,7 +209,7 @@ describe('McpClientsService in-run transport recovery (#489)', () => {
   });
 
   it('an app-level (non-transport) tool error is surfaced verbatim, never retried', async () => {
-    const { service } = buildService([server()]);
+    const { service } = buildService([server()], true);
     const appErr = new Error('tool says: bad input');
     const exec = jest.fn().mockRejectedValue(appErr);
     const connectSpy = stubConnect(service, 'getPage', [exec]);
@@ -211,4 +226,36 @@ describe('McpClientsService in-run transport recovery (#489)', () => {
     expect(connectSpy).toHaveBeenCalledTimes(1); // no reconnect for an app error
     await Promise.all(toolset.clients.map((c) => c.close()));
   });
+
+  // #489 (review, MEDIUM) — the Docmost write-class map keys by DOCMOST tool
+  // names; a THIRD-PARTY server may name a WRITE tool `getPage` (a Docmost read
+  // name). It must NOT inherit readOnly and must NOT auto-retry on a transport
+  // error — a blind retry of that write is a double-apply (the #435 class). Here
+  // the server is UNTRUSTED (buildService default, isInternalDocmostServer=false),
+  // so the map is not applied and `getPage` classifies as a write.
+  //
+  // MUTATION-VERIFY: forcing the server "trusted" (buildService(..., true)) makes
+  // `getPage` inherit readOnly -> it WOULD reconnect+retry (connect twice) and the
+  // assertions below fail — i.e. removing the trust scope re-opens the bug.
+  it('a THIRD-PARTY WRITE tool named like a Docmost read does NOT auto-retry (no double-apply)', async () => {
+    const realErr = await realSocketResetError();
+    // Untrusted: default trusted=false — a real third-party server.
+    const { service } = buildService([server()]);
+    const exec = jest.fn().mockRejectedValue(realErr);
+    const connectSpy = stubConnect(service, 'getPage', [exec, exec]);
+
+    const toolset = await service.toolsFor('ws-5');
+    const tool = toolset.tools['srv_getPage'];
+    await expect(
+      (tool.execute as (a: unknown, o: unknown) => Promise<unknown>)(
+        { pageId: 'p' },
+        opts(),
+      ),
+    ).rejects.toThrow(/MAY have already applied/);
+
+    // Exactly one call, NO reconnect — the name collision granted no readOnly-retry.
+    expect(exec).toHaveBeenCalledTimes(1);
+    expect(connectSpy).toHaveBeenCalledTimes(1);
+    await Promise.all(toolset.clients.map((c) => c.close()));
+  });
 });

apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts

@@ -217,6 +217,23 @@ export class McpClientsService {
     private readonly secretBox: SecretBoxService,
   ) {}
 
+  /**
+   * Whether an external MCP server is the TRUSTED internal Docmost MCP server —
+   * the only server whose tools may be classified by the Docmost write-class map
+   * (#489 review). Today this is ALWAYS false: every `ai_mcp_servers` row is an
+   * admin-configured THIRD-PARTY endpoint (there is no builtin/self flag, sentinel
+   * URL, or synthetic server in this path — Docmost's OWN tools are exposed via the
+   * separate in-app tools path, never through this external-MCP client). So no
+   * third-party tool can inherit `readOnly` by a name collision with a Docmost read
+   * tool, and none is ever auto-retried on a transport error (which would risk a
+   * double-apply — the #435 class). Flip this (an explicit `kind`/`isBuiltin`
+   * column, or a configured self-MCP URL) if a trusted internal server is ever
+   * introduced. A method (not a free function) so it is a single, mockable seam.
+   */
+  private isInternalDocmostServer(_server: AiMcpServer): boolean {
+    return false;
+  }
+
   /** Lazily load + memoize the shared write-class map (see the field doc). */
   private getWriteClassMap(): Promise<Record<string, ToolWriteClass>> {
     if (!this.writeClassMapPromise) {
@@ -388,9 +405,14 @@ export class McpClientsService {
     const instructions: McpServerInstruction[] = [];
     // merged-key -> provenance for the per-run recovery wrapper (#489).
     const toolMeta: Record<string, ToolProvenance> = {};
-    // Shared write-class map (#489): classifies each merged tool by its raw name
-    // so the recovery wrapper knows which tools are retry-safe reads.
-    const writeClassMap = await this.getWriteClassMap();
+    // Shared Docmost write-class map (#489) — classifies a tool by its raw name.
+    // Loaded ONLY when at least one server is a TRUSTED internal Docmost server
+    // (see isInternalDocmostServer): for third-party servers the map is never
+    // applied (a name collision must not grant readOnly-retry), so we skip the
+    // dynamic ESM load entirely in that (currently universal) case.
+    const writeClassMap = servers.some((s) => this.isInternalDocmostServer(s))
+      ? await this.getWriteClassMap()
+      : null;
 
     // Per-server connect+tools result, still tagged with its server so the merge
     // below can be applied in the SAME order as `servers` (see the parallel note).
@@ -464,6 +486,15 @@ export class McpClientsService {
       // against names already merged from earlier servers, so no external
       // tool is silently overwritten on collision. The returned count drives
       // whether this server's prompt guidance is included (≥1 tool merged).
+      // #489 (review): the Docmost write-class map keys by DOCMOST tool names and
+      // may ONLY be trusted for a server KNOWN to be the internal Docmost MCP
+      // server. Every row here is an admin-configured THIRD-PARTY endpoint, so a
+      // third-party WRITE tool that happens to be named like a Docmost read
+      // (getPage, listPages, ...) must NOT inherit readOnly — that would auto-retry
+      // a mutation on a transport error (double-apply, the #435 class). Gate the
+      // map on the trust check; untrusted servers get writeClass=undefined -> the
+      // recovery wrapper treats them as writes and never auto-retries.
+      const trustWriteClass = this.isInternalDocmostServer(server);
       const merged = this.mergeNamespaced(
         tools,
         result.guarded,
@@ -471,7 +502,7 @@ export class McpClientsService {
         server.id,
         toolMeta,
         i,
-        writeClassMap,
+        trustWriteClass ? writeClassMap : null,
       );
       outcomes.push({ name: server.name, ok: true });
       // Include this server's guidance ONLY when it actually contributed at
@@ -523,7 +554,9 @@ export class McpClientsService {
     serverId: string,
     toolMeta: Record<string, ToolProvenance>,
     serverIndex: number,
-    writeClassMap: Record<string, ToolWriteClass>,
+    // The Docmost write-class map, or `null` for an UNTRUSTED (third-party)
+    // server whose tools must all default to write (never auto-retried).
+    writeClassMap: Record<string, ToolWriteClass> | null,
   ): { count: number; prefix: string } {
     let count = 0;
     for (const { full, raw, tool } of namespace(picked, serverName)) {
@@ -537,13 +570,14 @@ export class McpClientsService {
       }
       target[key] = tool;
       // Record provenance so the per-run recovery wrapper (#489) can reconnect
-      // this tool's server and re-resolve it by its raw name, and gate the retry
-      // on its write-class (unknown third-party tool -> undefined -> treated as a
-      // write, i.e. never auto-retried).
+      // this tool's server and re-resolve it by its raw name. writeClass is set
+      // ONLY from a TRUSTED (internal-Docmost) map; for a third-party server the
+      // map is null -> writeClass stays undefined -> the wrapper treats the tool
+      // as a write and never auto-retries it (no double-apply on name collision).
       toolMeta[key] = {
         serverIndex,
         rawName: raw,
-        writeClass: writeClassMap[raw],
+        writeClass: writeClassMap ? writeClassMap[raw] : undefined,
       };
       count += 1;
     }
@@ -751,7 +785,15 @@ export class McpClientsService {
                 `retrying. (${shortError(err)})`,
             );
           }
-          // Abort check BEFORE minting a fresh connection.
+          // Abort check BEFORE minting a fresh connection (no socket for a
+          // stopped run). LIMITATION (#489, LOW): the reconnect's own connect is
+          // bounded by CONNECT_TIMEOUT_MS but does NOT itself observe `composed`,
+          // so a Stop that lands DURING the handshake is only honored at the next
+          // `stopped()` gate (before the retry) — a bounded ≤5s late-abort window;
+          // the throwaway client is closed at turn-end regardless. Threading
+          // `composed` into the SHARED (CAS-deduped) reconnect is deliberately
+          // avoided: it would let the first caller's abort tear down a reconnect a
+          // concurrent still-live caller depends on.
           if (stopped()) throw err;
           // CAS-swap by IDENTITY: mint+swap only if nobody swapped since this
           // call's snapshot; a losing concurrent call awaits the same reconnect