From d6827b9210938bd9921f8bc63e5d970f26221837 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 10 Jul 2026 05:35:52 +0300 Subject: [PATCH 1/4] =?UTF-8?q?feat(mcp):=20actionable=20tool=20errors=20?= =?UTF-8?q?=E2=80=94=20central=20axios=20diagnostics=20+=20fail-fast=20com?= =?UTF-8?q?ment-id=20guard=20(#437)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase A: add ONE response interceptor on DocmostClient's axios instance, registered AFTER the re-login interceptor, that reformats a failed request's error.message IN PLACE (never a custom Error subclass, so the live axios.isAxiosError / error.response?.status / config._retry checks keep working) into ` failed ( ): `, or the no-response variant. serverMessage is built ONLY from the whitelisted message/error fields or statusText — raw string/HTML bodies, headers and config never appear; an arraybuffer body is size-capped JSON.parsed; the full body goes to stderr only under DEBUG (parity with downloadImage). A _docmostFormatted flag guards against double-processing. Phase B (#436): assertFullUuid throws an actionable error BEFORE any network call at all five commentId sites (resolve/update/delete/get_comment and create_comment's parentCommentId when provided), so a truncated id can no longer loop as an opaque 400/404. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/client.ts | 181 +++++++ .../mcp/test/mock/create-comment.test.mjs | 10 +- .../mcp/test/unit/error-diagnostics.test.mjs | 446 ++++++++++++++++++ 3 files changed, 634 insertions(+), 3 deletions(-) create mode 100644 packages/mcp/test/unit/error-diagnostics.test.mjs diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index cba14720..2e59023f 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -197,6 +197,157 @@ function readCollabTokenTtlMs(): number { return Number.isFinite(raw) ? Math.max(0, raw) : 5 * 60 * 1000; } +// --- Issue #437: central error diagnostics ------------------------------- +// The agent only ever sees the thrown exception's `error.message`, so a failed +// tool must return an ACTIONABLE message (method, path, status, and the +// server's own validation text) instead of the opaque "Request failed with +// status code 400". These helpers + the response interceptor in the +// constructor are the single authoritative place that text is composed. + +// Overall cap on the composed diagnostic message so the model context stays +// compact and a (whitelisted) server string can never blow up the text. +const ERROR_MESSAGE_CAP = 300; +// Only attempt to JSON.parse an arraybuffer body under this size: a larger +// binary body is never a JSON error envelope, so parsing it just wastes memory +// (fetchInternalFile uses responseType:"arraybuffer", so a failed file fetch +// carries the JSON error envelope as raw bytes here). +const ERROR_BUFFER_PARSE_CAP = 4096; + +// Canonical 36-char UUID (8-4-4-4-12 hex). Deliberately version/variant- +// AGNOSTIC: the ids are UUIDv7 (e.g. 019f499a-9f8c-7d68-...), so only the +// canonical shape/length is enforced, not the version/variant nibble. +const FULL_UUID_RE = + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +/** + * Throw an actionable error BEFORE any network call when `value` is not a full + * canonical UUID. Absorbs #436: a truncated/short comment id used to reach the + * server and bounce back as an opaque 400/404 the agent could not self-correct; + * failing fast here names the exact fix. + */ +export function assertFullUuid( + tool: string, + param: string, + value: string, +): void { + if (typeof value !== "string" || !FULL_UUID_RE.test(value)) { + throw new Error( + `${tool}: '${param}' must be the FULL comment UUID (36 chars, e.g. ` + + `019f499a-9f8c-7d68-b7be-ce100d7c6c56), got '${value}'. Copy the id ` + + `verbatim from list_comments / create_comment output.`, + ); + } +} + +// Keep ONLY the pathname of a request (no host, no query string, no fragment) +// so the message never leaks a host or query params. Resolves a relative +// config.url against config.baseURL, then discards everything but the path. +function requestPath(config: any): string { + const rawUrl = typeof config?.url === "string" ? config.url : ""; + const base = + typeof config?.baseURL === "string" ? config.baseURL : undefined; + try { + // A dummy base makes an absolute config.url parse too; its host is dropped. + return new URL(rawUrl, base ?? "http://localhost").pathname; + } catch { + // Malformed url: still strip any query/fragment manually. + return rawUrl.split(/[?#]/)[0] || rawUrl; + } +} + +/** + * Compose the server-facing message from `error.response.data`, using ONLY the + * whitelisted `message`/`error` fields or the HTTP statusText. SECURITY: the + * raw response body, headers (Authorization!) and config are NEVER read here — + * a string/HTML body (e.g. a proxy's 502 page) is deliberately dropped in + * favour of the statusText. + */ +function extractServerMessage(data: any, statusText: string): string { + // class-validator envelope: { message: string | string[], error?: string }. + if ( + data && + typeof data === "object" && + !Buffer.isBuffer(data) && + !(data instanceof ArrayBuffer) + ) { + const msg = (data as any).message; + if (Array.isArray(msg)) { + const joined = msg.filter((m) => typeof m === "string").join("; "); + if (joined) return joined; + } else if (typeof msg === "string" && msg) { + return msg; + } + const err = (data as any).error; + if (typeof err === "string" && err) return err; + return statusText; + } + + // Buffer / ArrayBuffer body: attempt a size-capped, guarded JSON.parse so a + // failed arraybuffer fetch still surfaces the server's validation text. + if (Buffer.isBuffer(data) || data instanceof ArrayBuffer) { + const buf = Buffer.isBuffer(data) ? data : Buffer.from(data); + if (buf.length > 0 && buf.length <= ERROR_BUFFER_PARSE_CAP) { + try { + return extractServerMessage(JSON.parse(buf.toString("utf8")), statusText); + } catch { + return statusText; + } + } + return statusText; + } + + // A raw string / HTML body is never surfaced (may echo server internals). + return statusText; +} + +/** + * Reformat an AxiosError's `.message` IN PLACE into an actionable diagnostic: + * ` failed ( ): ` + * or, when the request never got a response: + * ` failed: (no response from server)`. + * + * Mutates the SAME error object (never a custom subclass) so the live + * axios.isAxiosError / error.response?.status / config._retry checks around the + * client keep working, and sets `_docmostFormatted` as a double-processing + * guard. A no-op on a non-axios or already-formatted error. + */ +export function formatDocmostAxiosError(error: any): void { + if (!error || error._docmostFormatted) return; + if (!axios.isAxiosError(error)) return; + + const config: any = error.config ?? {}; + const method = + typeof config.method === "string" ? config.method.toUpperCase() : ""; + const methodPath = `${method} ${requestPath(config)}`.trim(); + const response = error.response; + + let message: string; + if (response) { + const statusText = + typeof response.statusText === "string" ? response.statusText : ""; + const serverMessage = extractServerMessage(response.data, statusText); + message = `${methodPath} failed (${response.status} ${statusText}): ${serverMessage}`; + // Full body only to stderr under DEBUG (parity with downloadImage). + if (process.env.DEBUG) { + console.error( + "Docmost request failed; response body:", + JSON.stringify(response.data), + ); + } + } else { + // No response at all (ECONNREFUSED / ETIMEDOUT / ECONNRESET / DNS / timeout). + const reason = error.code ?? error.message ?? "network error"; + message = `${methodPath} failed: ${reason} (no response from server)`; + } + + if (message.length > ERROR_MESSAGE_CAP) { + message = message.slice(0, ERROR_MESSAGE_CAP - 1) + "…"; + } + + error.message = message; + (error as any)._docmostFormatted = true; +} + export class DocmostClient { private client: AxiosInstance; private token: string | null = null; @@ -337,6 +488,22 @@ export class DocmostClient { return Promise.reject(error); }, ); + + // Diagnostics interceptor (issue #437). Registered AFTER the re-login + // interceptor so a successful re-login retry (which resolves to a real + // response) is never seen here as an error; only a genuine failure reaches + // this rejection handler. It reformats error.message IN PLACE (see + // formatDocmostAxiosError — kept as a mutation, not a custom Error class, so + // the surrounding axios.isAxiosError / error.response?.status / config._retry + // checks keep working) and re-rejects the SAME error. The _docmostFormatted + // flag makes a re-processed retry-failure a no-op. + this.client.interceptors.response.use( + (response) => response, + (error) => { + formatDocmostAxiosError(error); + return Promise.reject(error); + }, + ); } /** Application base URL (API URL without the /api suffix). */ @@ -2411,6 +2578,8 @@ export class DocmostClient { } async getComment(commentId: string) { + // Fail fast (#436): reject a truncated id before any network call. + assertFullUuid("get_comment", "commentId", commentId); await this.ensureAuthenticated(); const response = await this.client.post("/comments/info", { commentId }); const comment = response.data.data || response.data; @@ -2500,6 +2669,12 @@ export class DocmostClient { parentCommentId?: string, suggestedText?: string, ) { + // Fail fast (#436): a provided parent id must be a full UUID before any + // network call. Validate only when truthy — a falsy parentCommentId means + // "top-level comment" (mirrors the isReply computation below), not a reply. + if (parentCommentId) { + assertFullUuid("create_comment", "parentCommentId", parentCommentId); + } await this.ensureAuthenticated(); const isReply = !!parentCommentId; @@ -2782,6 +2957,8 @@ export class DocmostClient { } async updateComment(commentId: string, content: string) { + // Fail fast (#436): reject a truncated id before any network call. + assertFullUuid("update_comment", "commentId", commentId); await this.ensureAuthenticated(); // NON-canonicalizing on purpose (comment body — see createComment). const jsonContent = await markdownToProseMirror(content); @@ -2797,6 +2974,8 @@ export class DocmostClient { } async deleteComment(commentId: string) { + // Fail fast (#436): reject a truncated id before any network call. + assertFullUuid("delete_comment", "commentId", commentId); await this.ensureAuthenticated(); return this.client .post("/comments/delete", { commentId }) @@ -2809,6 +2988,8 @@ export class DocmostClient { * rejects resolving a reply. Hits POST /comments/resolve. */ async resolveComment(commentId: string, resolved: boolean) { + // Fail fast (#436): reject a truncated id before any network call. + assertFullUuid("resolve_comment", "commentId", commentId); await this.ensureAuthenticated(); const response = await this.client.post("/comments/resolve", { commentId, diff --git a/packages/mcp/test/mock/create-comment.test.mjs b/packages/mcp/test/mock/create-comment.test.mjs index e9d903aa..4936ccb4 100644 --- a/packages/mcp/test/mock/create-comment.test.mjs +++ b/packages/mcp/test/mock/create-comment.test.mjs @@ -203,14 +203,16 @@ test("a reply creates without selection or anchoring and is stored as type 'page "reply body", "inline", undefined, - "parent-123", + // #437: a parentCommentId must be a full canonical UUID. + "019f499a-9f8c-7d68-b7be-ce100d7c6c56", ); assert.equal(result.success, true, "a reply must resolve successfully"); assert.ok(createPayload, "/comments/create must have been called"); assert.equal( createPayload.parentCommentId, - "parent-123", + // #437: a parentCommentId must be a full canonical UUID. + "019f499a-9f8c-7d68-b7be-ce100d7c6c56", "the reply payload must carry the parentCommentId", ); assert.equal( @@ -321,7 +323,9 @@ test("suggestedText on a reply is rejected", async () => { "body", "inline", undefined, - "parent-1", + // #437: use a valid full UUID so the reply+suggestion rejection fires + // (not the id-shape guard). + "019f499a-9f8c-7d68-b7be-ce100d7c6c56", "replacement", ), /reply/i, diff --git a/packages/mcp/test/unit/error-diagnostics.test.mjs b/packages/mcp/test/unit/error-diagnostics.test.mjs new file mode 100644 index 00000000..be01f294 --- /dev/null +++ b/packages/mcp/test/unit/error-diagnostics.test.mjs @@ -0,0 +1,446 @@ +// Issue #437: central error diagnostics. +// +// Two surfaces are covered here: +// 1. formatDocmostAxiosError — the pure response-interceptor body that +// rewrites an AxiosError's `.message` into an actionable diagnostic. +// 2. assertFullUuid — the fail-fast comment-id guard (absorbs #436) that must +// throw BEFORE any network call. +// Plus an end-to-end pass over a real (offline) http server to prove the +// interceptor is wired, that a re-login retry leaves a success untouched, and +// that a persistent failure gets formatted — and that an invalid comment id +// short-circuits every comment tool with ZERO network traffic. +import { test, after } from "node:test"; +import assert from "node:assert/strict"; +import http from "node:http"; +import axios, { AxiosError } from "axios"; +import { + DocmostClient, + formatDocmostAxiosError, + assertFullUuid, +} from "../../build/client.js"; + +// Build an AxiosError-shaped object the way the interceptor's rejection handler +// receives it. Using the real AxiosError ctor makes axios.isAxiosError() true. +function makeAxiosError({ + method = "post", + url = "/comments/resolve", + baseURL = "http://host.example/api", + status, + statusText, + data, + code, + message = "Request failed", +}) { + const config = { method, url, baseURL }; + const response = + status === undefined + ? undefined + : { status, statusText, data, headers: {}, config }; + return new AxiosError(message, code, config, {}, response); +} + +// --------------------------------------------------------------------------- +// formatDocmostAxiosError: message-body extraction rules. +// --------------------------------------------------------------------------- +test("class-validator message array is joined with '; '", () => { + const err = makeAxiosError({ + status: 400, + statusText: "Bad Request", + data: { message: ["commentId must be a UUID", "resolved must be a boolean"] }, + }); + formatDocmostAxiosError(err); + assert.equal( + err.message, + "POST /comments/resolve failed (400 Bad Request): commentId must be a UUID; resolved must be a boolean", + ); +}); + +test("a string message is used as-is", () => { + const err = makeAxiosError({ + method: "post", + url: "/comments/resolve", + status: 400, + statusText: "Bad Request", + data: { message: "commentId must be a UUID" }, + }); + formatDocmostAxiosError(err); + assert.equal( + err.message, + "POST /comments/resolve failed (400 Bad Request): commentId must be a UUID", + ); +}); + +test("falls back to data.error when message is absent", () => { + const err = makeAxiosError({ + method: "get", + url: "/pages/info", + status: 403, + statusText: "Forbidden", + data: { error: "Forbidden" }, + }); + formatDocmostAxiosError(err); + assert.equal(err.message, "GET /pages/info failed (403 Forbidden): Forbidden"); +}); + +test("empty object body falls back to statusText", () => { + const err = makeAxiosError({ + status: 404, + statusText: "Not Found", + url: "/comments/info", + data: {}, + }); + formatDocmostAxiosError(err); + assert.equal(err.message, "POST /comments/info failed (404 Not Found): Not Found"); +}); + +test("HTML/string body is NEVER surfaced — only the statusText", () => { + const html = "502 Bad Gateway — nginx internals here"; + const err = makeAxiosError({ + status: 502, + statusText: "Bad Gateway", + url: "/comments/create", + data: html, + }); + formatDocmostAxiosError(err); + assert.equal( + err.message, + "POST /comments/create failed (502 Bad Gateway): Bad Gateway", + ); + assert.ok(!err.message.includes("nginx"), "raw HTML body must not leak"); + assert.ok(!err.message.includes(""), "raw HTML body must not leak"); +}); + +test("Buffer body carrying JSON is parsed for its message", () => { + const buf = Buffer.from(JSON.stringify({ message: "file too large" }), "utf8"); + const err = makeAxiosError({ + method: "get", + url: "/files/abc/x.png", + status: 413, + statusText: "Payload Too Large", + data: buf, + }); + formatDocmostAxiosError(err); + assert.equal( + err.message, + "GET /files/abc/x.png failed (413 Payload Too Large): file too large", + ); +}); + +test("Buffer body with non-JSON garbage falls back to statusText", () => { + const buf = Buffer.from("<<< not json at all >>>", "utf8"); + const err = makeAxiosError({ + method: "get", + url: "/files/abc/x.png", + status: 500, + statusText: "Internal Server Error", + data: buf, + }); + formatDocmostAxiosError(err); + assert.equal( + err.message, + "GET /files/abc/x.png failed (500 Internal Server Error): Internal Server Error", + ); + assert.ok(!err.message.includes("not json"), "raw buffer body must not leak"); +}); + +test("an oversized Buffer body is not parsed (size cap) — statusText only", () => { + // A >4KB JSON buffer: even though it IS valid JSON with a message, the size + // cap means we do not attempt to parse it, so only the statusText survives. + const big = { message: "x".repeat(5000) }; + const buf = Buffer.from(JSON.stringify(big), "utf8"); + const err = makeAxiosError({ + method: "get", + url: "/files/abc/x.png", + status: 500, + statusText: "Internal Server Error", + data: buf, + }); + formatDocmostAxiosError(err); + assert.equal( + err.message, + "GET /files/abc/x.png failed (500 Internal Server Error): Internal Server Error", + ); +}); + +// --------------------------------------------------------------------------- +// formatDocmostAxiosError: no-response and path/method handling. +// --------------------------------------------------------------------------- +test("no response uses error.code + path + 'no response from server'", () => { + const err = makeAxiosError({ + method: "post", + url: "/comments/create", + status: undefined, + code: "ECONNREFUSED", + message: "connect ECONNREFUSED 127.0.0.1:3000", + }); + formatDocmostAxiosError(err); + assert.equal( + err.message, + "POST /comments/create failed: ECONNREFUSED (no response from server)", + ); +}); + +test("no response falls back to error.message when code is absent", () => { + const err = makeAxiosError({ + method: "post", + url: "/comments/create", + status: undefined, + message: "timeout of 30000ms exceeded", + }); + formatDocmostAxiosError(err); + assert.equal( + err.message, + "POST /comments/create failed: timeout of 30000ms exceeded (no response from server)", + ); +}); + +test("path drops the host and the query string", () => { + const err = makeAxiosError({ + method: "post", + url: "/comments/resolve?token=secret&x=1", + baseURL: "https://docs.example.com/api", + status: 400, + statusText: "Bad Request", + data: { message: "bad" }, + }); + formatDocmostAxiosError(err); + assert.equal( + err.message, + "POST /comments/resolve failed (400 Bad Request): bad", + ); + assert.ok(!err.message.includes("secret"), "query string must not leak"); + assert.ok(!err.message.includes("docs.example.com"), "host must not leak"); +}); + +// --------------------------------------------------------------------------- +// formatDocmostAxiosError: length cap + guard flag + pass-through. +// --------------------------------------------------------------------------- +test("the overall message is capped at ~300 chars", () => { + const err = makeAxiosError({ + status: 400, + statusText: "Bad Request", + data: { message: "y".repeat(1000) }, + }); + formatDocmostAxiosError(err); + assert.ok(err.message.length <= 300, `expected <=300, got ${err.message.length}`); + assert.ok(err.message.endsWith("…"), "a truncated message ends with an ellipsis"); +}); + +test("a formatted error is not re-processed (guard flag)", () => { + const err = makeAxiosError({ + status: 400, + statusText: "Bad Request", + data: { message: "first" }, + }); + formatDocmostAxiosError(err); + const once = err.message; + assert.equal(err._docmostFormatted, true); + // Mutate the body and re-run: the guard makes it a no-op. + err.response.data = { message: "second" }; + formatDocmostAxiosError(err); + assert.equal(err.message, once, "the guard flag prevents double-processing"); +}); + +test("a non-axios error is passed through untouched", () => { + const plain = new Error("boom"); + formatDocmostAxiosError(plain); + assert.equal(plain.message, "boom"); + assert.equal(plain._docmostFormatted, undefined); +}); + +// --------------------------------------------------------------------------- +// assertFullUuid. +// --------------------------------------------------------------------------- +const GOOD_UUID = "019f499a-9f8c-7d68-b7be-ce100d7c6c56"; + +test("assertFullUuid accepts a full canonical UUID (any version nibble)", () => { + assert.doesNotThrow(() => assertFullUuid("resolve_comment", "commentId", GOOD_UUID)); + // A v4 id also passes (version/variant-agnostic). + assert.doesNotThrow(() => + assertFullUuid("get_comment", "commentId", "3d5b7c1e-2f4a-4b6c-8d9e-0f1a2b3c4d5e"), + ); +}); + +test("assertFullUuid rejects a truncated prefix", () => { + assert.throws( + () => assertFullUuid("resolve_comment", "commentId", "019f499a"), + (e) => + e.message.startsWith( + "resolve_comment: 'commentId' must be the FULL comment UUID", + ) && + e.message.includes("got '019f499a'") && + e.message.includes("Copy the id verbatim"), + ); +}); + +test("assertFullUuid rejects garbage and empty string", () => { + assert.throws( + () => assertFullUuid("delete_comment", "commentId", "not-a-uuid"), + /must be the FULL comment UUID.*got 'not-a-uuid'/s, + ); + assert.throws( + () => assertFullUuid("update_comment", "commentId", ""), + /must be the FULL comment UUID.*got ''/s, + ); +}); + +// --------------------------------------------------------------------------- +// End-to-end over an offline http server: interceptor wiring + re-login. +// --------------------------------------------------------------------------- +function readBody(req) { + return new Promise((resolve) => { + let raw = ""; + req.on("data", (c) => (raw += c)); + req.on("end", () => resolve(raw)); + }); +} +function startServer(handler) { + return new Promise((resolve) => { + const server = http.createServer(handler); + server.listen(0, "127.0.0.1", () => { + const { port } = server.address(); + resolve({ server, baseURL: `http://127.0.0.1:${port}/api` }); + }); + }); +} +function sendJson(res, status, obj, extra = {}) { + res.writeHead(status, { "Content-Type": "application/json", ...extra }); + res.end(JSON.stringify(obj)); +} +const openServers = []; +async function spawn(handler) { + const { server, baseURL } = await startServer(handler); + openServers.push(server); + return { baseURL }; +} +after(async () => { + await Promise.all(openServers.map((s) => new Promise((r) => s.close(r)))); +}); + +test("a 400 on a JSON endpoint is reformatted by the wired interceptor", async () => { + const { baseURL } = await spawn(async (req, res) => { + await readBody(req); + if (req.url === "/api/auth/login") { + sendJson(res, 200, { success: true }, { + "Set-Cookie": "authToken=t; Path=/; HttpOnly", + }); + return; + } + if (req.url === "/api/pages/info") { + sendJson(res, 400, { message: "pageId should not be empty" }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "u@example.com", "pw"); + await assert.rejects( + () => client.getPageRaw("x"), + (e) => { + assert.ok(axios.isAxiosError(e), "still an AxiosError (mutation, not a subclass)"); + assert.equal(e.response?.status, 400, "error.response?.status still readable"); + assert.equal( + e.message, + "POST /pages/info failed (400 Bad Request): pageId should not be empty", + ); + return true; + }, + ); +}); + +test("401 -> re-login -> successful retry: the SUCCESS message is untouched", async () => { + let infoCalls = 0; + const { baseURL } = await spawn(async (req, res) => { + await readBody(req); + if (req.url === "/api/auth/login") { + sendJson(res, 200, { success: true }, { + "Set-Cookie": "authToken=fresh; Path=/; HttpOnly", + }); + return; + } + if (req.url === "/api/workspace/info") { + infoCalls++; + if (infoCalls === 1) sendJson(res, 401, { message: "Unauthorized" }); + else sendJson(res, 200, { success: true, data: { id: "ws" } }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "u@example.com", "pw"); + client.token = "stale"; + client.client.defaults.headers.common["Authorization"] = "Bearer stale"; + + const result = await client.getWorkspace(); + assert.equal(result.success, true, "the retried request resolved successfully"); + assert.equal(infoCalls, 2, "401 then a successful replay"); +}); + +test("401 -> re-login -> persistent failure: formatted AND retry guard intact", async () => { + let infoCalls = 0; + let loginCalls = 0; + const { baseURL } = await spawn(async (req, res) => { + await readBody(req); + if (req.url === "/api/auth/login") { + loginCalls++; + sendJson(res, 200, { success: true }, { + "Set-Cookie": "authToken=fresh; Path=/; HttpOnly", + }); + return; + } + if (req.url === "/api/workspace/info") { + infoCalls++; + // Always 401, even after a fresh login: the _retry guard must stop here. + sendJson(res, 401, { message: "token still invalid" }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "u@example.com", "pw"); + client.token = "stale"; + client.client.defaults.headers.common["Authorization"] = "Bearer stale"; + + await assert.rejects( + () => client.getWorkspace(), + (e) => { + assert.equal( + e.message, + "POST /workspace/info failed (401 Unauthorized): token still invalid", + ); + return true; + }, + ); + // The _retry guard is intact: exactly one replay (2 hits), one re-login. + assert.equal(infoCalls, 2, "endpoint hit at most twice (one retry only)"); + assert.equal(loginCalls, 1, "re-login attempted exactly once"); +}); + +// --------------------------------------------------------------------------- +// assertFullUuid application points: NO network call when the id is invalid. +// A server that counts EVERY request proves the guard short-circuits before +// even the login round-trip. +// --------------------------------------------------------------------------- +test("all 5 comment-id call sites reject a bad id with ZERO network traffic", async () => { + let requests = 0; + const { baseURL } = await spawn(async (req, res) => { + requests++; + await readBody(req); + sendJson(res, 200, { success: true }); + }); + + const client = new DocmostClient(baseURL, "u@example.com", "pw"); + const bad = "019f499a"; // truncated + + await assert.rejects(() => client.resolveComment(bad, true), /resolve_comment: 'commentId'/); + await assert.rejects(() => client.updateComment(bad, "hi"), /update_comment: 'commentId'/); + await assert.rejects(() => client.deleteComment(bad), /delete_comment: 'commentId'/); + await assert.rejects(() => client.getComment(bad), /get_comment: 'commentId'/); + // createComment validates parentCommentId only when provided. + await assert.rejects( + () => client.createComment("page-1", "body", "inline", "sel", bad), + /create_comment: 'parentCommentId'/, + ); + + assert.equal(requests, 0, "no request (not even /auth/login) may be issued for a bad id"); +}); From 9a435201b8061d8ba87ba50c1839d5abacdd2418 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 10 Jul 2026 05:35:57 +0300 Subject: [PATCH 2/4] feat(mcp): enrich collab connect/persist/closed error texts with pageId + retry hint (#437) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Append `(pageId ; transient — retry once; persistent failures mean the collab server is unreachable/overloaded)` to the connect-timeout, persist-timeout and connection-closed error texts in CollabSession, so the agent can self-correct instead of blind-looping. The Yjs-encode error is left untouched (it already names the offending attribute). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/lib/collab-session.ts | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/packages/mcp/src/lib/collab-session.ts b/packages/mcp/src/lib/collab-session.ts index 1c1c8d81..9a4b7a6d 100644 --- a/packages/mcp/src/lib/collab-session.ts +++ b/packages/mcp/src/lib/collab-session.ts @@ -202,6 +202,21 @@ export class CollabSession { this.ydoc = new Y.Doc(); } + /** + * Shared diagnostic suffix (issue #437) appended to the connect-timeout, + * persist-timeout and connection-closed error texts: names the offending + * pageId and tells the agent this class of failure is transient (retry once) + * vs. a persistent collab-server outage, so it can self-correct instead of + * blind-looping. The Yjs-encode error is deliberately NOT touched — it + * already names the offending attribute. + */ + private hint(): string { + return ( + `(pageId ${this.pageId}; transient — retry once; persistent failures ` + + `mean the collab server is unreachable/overloaded)` + ); + } + /** * A cached session may be reused only when it is fully ready, still synced, * has not lost its connection, and has not exceeded its max age (invariant 5 @@ -232,7 +247,9 @@ export class CollabSession { // The 25s connect timeout: the collab connection never became ready. this.opts?.onConnectTimeout?.(); this.teardown( - new Error("Connection timeout to collaboration server"), + new Error( + `Connection timeout to collaboration server ${this.hint()}`, + ), false, ); }, CONNECT_TIMEOUT_MS); @@ -259,7 +276,7 @@ export class CollabSession { if (process.env.DEBUG) console.error("WS Disconnect"); this.teardown( new Error( - "Collaboration connection closed before the update was persisted/synced", + `Collaboration connection closed before the update was persisted/synced ${this.hint()}`, ), true, ); @@ -268,7 +285,7 @@ export class CollabSession { if (process.env.DEBUG) console.error("WS Close"); this.teardown( new Error( - "Collaboration connection closed before the update was persisted/synced", + `Collaboration connection closed before the update was persisted/synced ${this.hint()}`, ), true, ); @@ -403,7 +420,7 @@ export class CollabSession { persistTimer = setTimeout(() => { localFinish( new Error( - "Timeout waiting for collaboration server to persist the update", + `Timeout waiting for collaboration server to persist the update ${this.hint()}`, ), ); }, PERSIST_TIMEOUT_MS); From d3d32d637b35bb0b76c9b2347ed80d7cb881fb2e Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 10 Jul 2026 05:44:21 +0300 Subject: [PATCH 3/4] =?UTF-8?q?fix(mcp):=20no-response=20=D0=B4=D0=B8?= =?UTF-8?q?=D0=B0=D0=B3=D0=BD=D0=BE=D1=81=D1=82=D0=B8=D0=BA=D0=B0=20?= =?UTF-8?q?=E2=80=94=20=D0=BD=D0=B5=20=D0=BE=D1=82=D0=B4=D0=B0=D0=B2=D0=B0?= =?UTF-8?q?=D1=82=D1=8C=20=D1=81=D1=8B=D1=80=D0=BE=D0=B9=20error.message?= =?UTF-8?q?=20(=D1=83=D1=82=D0=B5=D1=87=D0=BA=D0=B0=20host=20=D0=B2=20?= =?UTF-8?q?=D0=BC=D0=BE=D0=B4=D0=B5=D0=BB=D1=8C)=20(=D0=B2=D0=BD=D1=83?= =?UTF-8?q?=D1=82=D1=80.=20=D1=80=D0=B5=D0=B2=D1=8C=D1=8E=20#437)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no-response ветка формата использовала error.code ?? error.message: при отсутствии code axios-сообщения сетевых ошибок содержат host:port ('connect ECONNREFUSED 127.0.0.1:3000', 'getaddrinfo ENOTFOUND host'), что нарушает инвариант #437 «host никогда не попадает в видимое модели сообщение». Теперь только error.code (?? 'network error'); полный нативный текст уходит в stderr под DEBUG. code проставлен фактически для всех реальных no-response ошибок. Тест обновлён: сырое host-содержащее сообщение -> нейтральный reason, плюс ассерт что host в сообщении отсутствует. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/client.ts | 11 ++++++++++- packages/mcp/test/unit/error-diagnostics.test.mjs | 10 +++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 2e59023f..e03b93b4 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -336,8 +336,17 @@ export function formatDocmostAxiosError(error: any): void { } } else { // No response at all (ECONNREFUSED / ETIMEDOUT / ECONNRESET / DNS / timeout). - const reason = error.code ?? error.message ?? "network error"; + // Use ONLY error.code, never the raw error.message: axios network messages + // embed host:port ("connect ECONNREFUSED 127.0.0.1:3000", "getaddrinfo + // ENOTFOUND host") and #437's invariant is that the host never reaches the + // model-visible message. code is set for essentially every real no-response + // error (ECONNREFUSED/ETIMEDOUT/ECONNRESET/ENOTFOUND/ECONNABORTED); the full + // native message still goes to stderr under DEBUG. + const reason = error.code ?? "network error"; message = `${methodPath} failed: ${reason} (no response from server)`; + if (process.env.DEBUG) { + console.error("Docmost request failed; no response:", error.message); + } } if (message.length > ERROR_MESSAGE_CAP) { diff --git a/packages/mcp/test/unit/error-diagnostics.test.mjs b/packages/mcp/test/unit/error-diagnostics.test.mjs index be01f294..ab1bdd56 100644 --- a/packages/mcp/test/unit/error-diagnostics.test.mjs +++ b/packages/mcp/test/unit/error-diagnostics.test.mjs @@ -180,18 +180,22 @@ test("no response uses error.code + path + 'no response from server'", () => { ); }); -test("no response falls back to error.message when code is absent", () => { +test("no response with no code falls back to a neutral reason (raw message not leaked — it may embed host:port)", () => { const err = makeAxiosError({ method: "post", url: "/comments/create", status: undefined, - message: "timeout of 30000ms exceeded", + // A raw axios network message like "connect ECONNREFUSED 127.0.0.1:3000" + // embeds the host; #437's invariant is that it never reaches the message. + message: "connect ECONNREFUSED 10.0.0.5:3000", }); formatDocmostAxiosError(err); assert.equal( err.message, - "POST /comments/create failed: timeout of 30000ms exceeded (no response from server)", + "POST /comments/create failed: network error (no response from server)", ); + // And the host must NOT appear anywhere in the model-visible message. + assert.ok(!err.message.includes("10.0.0.5")); }); test("path drops the host and the query string", () => { From 4809348457b35c5e02e423050afc965f2520e483 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 10 Jul 2026 07:14:39 +0300 Subject: [PATCH 4/4] =?UTF-8?q?test(mcp):=20=D0=B0=D1=81=D1=81=D0=B5=D1=80?= =?UTF-8?q?=D1=82=D0=B8=D1=82=D1=8C=20collab-hint=20(pageId=20+=20transien?= =?UTF-8?q?t/retry)=20=D0=B2=20reject-=D1=82=D0=B5=D0=BA=D1=81=D1=82=D0=B0?= =?UTF-8?q?=D1=85=20(=D1=80=D0=B5=D0=B2=D1=8C=D1=8E=20#441)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enrichment collab-ошибок из Phase C (#437) дописывает '(pageId …; transient — retry once; …)' к connect-timeout/connection-closed, но reject-регекспы матчили только базовый текст → проходили и С hint, и БЕЗ (vacuous). Ужесточил два ассерта (connection-closed, connect-timeout) до '<база> (pageId page-1; transient' — теперь рефактор, убравший hint(), их роняет. Мутационно: hint()->'' → ровно эти 2 теста краснеют (18->16). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/test/unit/collab-session.test.mjs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/mcp/test/unit/collab-session.test.mjs b/packages/mcp/test/unit/collab-session.test.mjs index f5874721..c71ddb9b 100644 --- a/packages/mcp/test/unit/collab-session.test.mjs +++ b/packages/mcp/test/unit/collab-session.test.mjs @@ -168,7 +168,9 @@ test("an in-flight mutate rejects with the connection-closed text on disconnect" FakeProvider.last()._disconnect(); await assert.rejects( p, - /Collaboration connection closed before the update was persisted\/synced/, + // Assert the #437 diagnostic hint tail too (pageId + transient/retry cue), + // so a refactor that drops hint() can't pass this vacuously. + /Collaboration connection closed before the update was persisted\/synced \(pageId page-1; transient/, ); }); @@ -248,7 +250,11 @@ test("connect timeout rejects with the connect-timeout text and fires the metric }, }); mock.timers.tick(25000); - await assert.rejects(p, /Connection timeout to collaboration server/); + await assert.rejects( + p, + // Assert the #437 diagnostic hint tail too (pageId + transient/retry cue). + /Connection timeout to collaboration server \(pageId page-1; transient/, + ); assert.equal(metricFired, 1); assert.equal(__sessionCountForTests(), 0); });