From 38a09c5ca1989c59aef89992c7f011a274ff2d33 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 07:24:03 +0300 Subject: [PATCH] =?UTF-8?q?fix(ai-chat):=20write-class-=D1=80=D0=B5=D1=82?= =?UTF-8?q?=D1=80=D0=B0=D0=B9=20=D1=82=D0=BE=D0=BB=D1=8C=D0=BA=D0=BE=20?= =?UTF-8?q?=D0=B4=D0=BB=D1=8F=20=D0=B4=D0=BE=D0=B2=D0=B5=D1=80=D0=B5=D0=BD?= =?UTF-8?q?=D0=BD=D0=BE=D0=B3=D0=BE=20=D0=B2=D0=BD=D1=83=D1=82=D1=80=D0=B5?= =?UTF-8?q?=D0=BD=D0=BD=D0=B5=D0=B3=D0=BE=20Docmost-=D1=81=D0=B5=D1=80?= =?UTF-8?q?=D0=B2=D0=B5=D1=80=D0=B0=20(#489=20=D1=80=D0=B5=D0=B2=D1=8C?= =?UTF-8?q?=D1=8E)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../external-mcp/mcp-clients.recovery.spec.ts | 57 +++++++++++++++-- .../external-mcp/mcp-clients.service.ts | 62 ++++++++++++++++--- 2 files changed, 104 insertions(+), 15 deletions(-) diff --git a/apps/server/src/core/ai-chat/external-mcp/mcp-clients.recovery.spec.ts b/apps/server/src/core/ai-chat/external-mcp/mcp-clients.recovery.spec.ts index 5b129a58..5cf83721 100644 --- a/apps/server/src/core/ai-chat/external-mcp/mcp-clients.recovery.spec.ts +++ b/apps/server/src/core/ai-chat/external-mcp/mcp-clients.recovery.spec.ts @@ -55,7 +55,7 @@ const server = (over: Partial = {}): 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)( + { 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())); + }); }); diff --git a/apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts b/apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts index 4c874ccf..20397acc 100644 --- a/apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts +++ b/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> { 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 = {}; - // 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, serverIndex: number, - writeClassMap: Record, + // 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 | 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