From 575125a5dc6741cdc83efddee8edecf4edca5fb3 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 02:20:12 +0300 Subject: [PATCH 1/3] fix(mcp): enrich bad/inaccessible spaceId 404 into an actionable error (#534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A well-formed but non-existent/inaccessible spaceId made the space-permissions check answer with an opaque 404 ("Space permissions not found"), which the agent could not self-correct from. Add an enrich-on-404 wrapper at the client-method level (Variant A — no backend change) that, ONLY on that 404, replaces the server text with a factual message naming the bad spaceId and the spaces the token can actually see, pointing at listSpaces. Mechanism (context.ts): - getAccessibleSpaceIndex(): the token's accessible spaces from the single source of truth (/spaces), with a per-instance short-TTL cache (MCP_SPACES_CACHE_TTL_MS, default 60s; 0 disables) and single-flight dedup. Only a COMPLETE (untruncated) result is cached and only a complete result may drive the rewrite. The in-flight promise is nulled on BOTH resolve and reject so a transient /spaces blip is never memoized. Cache invalidated on every identity change, mirroring collabTokenCache (login() + the 401/403 reauth interceptor). - paginateAllWithMeta(): surfaces the `truncated` flag paginateAll swallows; paginateAll now delegates to it (unchanged contract, getSpaces untouched). - withSpaceAccessDiagnostics(spaceId, mcpName, fn): abort/cap wins FIRST (by the toolAbortSignal flag, NOT e.name — a cap may be TimeoutError/custom); only a 404 is enrichable; FAILS OPEN (rethrows the original server error) on every source of uncertainty (fetch failed / !complete / spaceId present / aborted). Wrapped only the paths where the sole 404 cause is the space membership check: getTree, listPages (tree + recent-with-spaceId), search (with spaceId), checkNewComments. createPage/getPageContext are deliberately NOT wrapped. formatSpaceNotAccessible (errors.ts) composes the message (<=10 spaces + "(+N ещё)" tail; distinct zero-spaces variant). Tests: 11 new unit tests (stub client) covering all 9 acceptance criteria + message shape; full mcp unit suite green (701 tests). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/client.ts | 6 +- packages/mcp/src/client/comments.ts | 13 +- packages/mcp/src/client/context.ts | 216 ++++++++++- packages/mcp/src/client/errors.ts | 34 ++ packages/mcp/src/client/read.ts | 62 ++- .../unit/space-access-diagnostics.test.mjs | 354 ++++++++++++++++++ 6 files changed, 661 insertions(+), 24 deletions(-) create mode 100644 packages/mcp/test/unit/space-access-diagnostics.test.mjs diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 75c62871..e33b8211 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -31,7 +31,11 @@ import { TransformsMixin, type ITransformsMixin } from "./client/transforms.js"; // existing importer (index.ts, http.ts, stdio.ts, the in-app host) keeps working // with ZERO changes. export type { DocmostMcpConfig, SandboxPut } from "./client/context.js"; -export { formatDocmostAxiosError, assertFullUuid } from "./client/errors.js"; +export { + formatDocmostAxiosError, + assertFullUuid, + formatSpaceNotAccessible, +} from "./client/errors.js"; // The full public + shared instance surface of the assembled client. Built by // INTERSECTING each domain mixin's public interface (each DERIVED from its class diff --git a/packages/mcp/src/client/comments.ts b/packages/mcp/src/client/comments.ts index c1fd557d..da12e606 100644 --- a/packages/mcp/src/client/comments.ts +++ b/packages/mcp/src/client/comments.ts @@ -650,10 +650,15 @@ export function CommentsMixin>( // The subtree scope (parentPageId given) already INCLUDES the root node // itself: /pages/tree seeds getPageAndDescendants with id = parentPageId, so // no separate getPageRaw fetch for the parent is needed. - const { pages: pagesInScope, truncated } = await this.enumerateSpacePages( - spaceId, - parentPageId, - ); + // #534: the enumerateSpacePages seed (`/pages/tree`) 404s for a bad or + // inaccessible spaceId; wrap it so that 404 becomes an actionable "spaceId + // not accessible" hint instead of the opaque "Space permissions not found". + // Only the whole enumeration is wrapped (the only 404 source here) — see the + // wrap-allowlist invariant on withSpaceAccessDiagnostics. + const { pages: pagesInScope, truncated } = + await this.withSpaceAccessDiagnostics(spaceId, "checkNewComments", () => + this.enumerateSpacePages(spaceId, parentPageId), + ); // 2. Fetch comments for each page, keep ones created after since const results: any[] = []; diff --git a/packages/mcp/src/client/context.ts b/packages/mcp/src/client/context.ts index 8c784f53..c615ce5e 100644 --- a/packages/mcp/src/client/context.ts +++ b/packages/mcp/src/client/context.ts @@ -25,7 +25,10 @@ import { } from "../lib/collab-session.js"; import { withPageLock, isUuid } from "../lib/page-lock.js"; import { getCollabToken, performLogin } from "../lib/auth-utils.js"; -import { formatDocmostAxiosError } from "./errors.js"; +import { + formatDocmostAxiosError, + formatSpaceNotAccessible, +} from "./errors.js"; import { GetPageConversionCache } from "./getpage-cache.js"; // A generic mixin base constructor (issue #450). Each domain mixin is a factory @@ -116,6 +119,36 @@ function readCollabTokenTtlMs(): number { return Number.isFinite(raw) ? Math.max(0, raw) : 5 * 60 * 1000; } +/** + * Accessible-space index cache TTL in milliseconds (issue #534). Read fresh from + * the environment on every access — mirroring readCollabTokenTtlMs above — so a + * test or a live rollback can change it without reloading the module. + * + * The index (see getAccessibleSpaceIndex) is fetched ONLY on the enrich-on-404 + * slow path to turn an opaque "Space permissions not found" 404 into a factual + * "spaceId X is not among your accessible spaces" hint; a short TTL keeps a burst + * of failing tool calls from re-sweeping /spaces each time while never widening + * the permission-staleness window meaningfully. Default 60s. An EXPLICIT 0 (or + * negative) DISABLES the cache (exact fetch-per-enrichment). Unset/unparseable + * (NaN) falls back to the 60s default with the cache ON. + */ +function readSpacesCacheTtlMs(): number { + const raw = parseInt(process.env.MCP_SPACES_CACHE_TTL_MS ?? "", 10); + return Number.isFinite(raw) ? Math.max(0, raw) : 60000; +} + +/** + * The set of spaces the current token can see, plus a `complete` flag that is + * false when the /spaces listing was truncated at the pagination ceiling. Used + * by the enrich-on-404 diagnostics: an authoritative membership test is only + * possible when `complete` is true (see withSpaceAccessDiagnostics). + */ +export type AccessibleSpaceIndex = { + ids: Set; + spaces: { id: string; name: string }[]; + complete: boolean; +}; + export abstract class DocmostClientContext { protected client: AxiosInstance; protected token: string | null = null; @@ -163,6 +196,27 @@ export abstract class DocmostClientContext { // bypassed on a forced refresh (the 401/403 reauth path). null = no token yet. protected collabTokenCache: { token: string; mintedAt: number } | null = null; + // Accessible-space index cache + single-flight (issue #534). TWO separate + // fields, mirroring loginPromise (in-flight dedup) vs collabTokenCache + // (persistent value): + // - spaceIndexCache: the last SUCCESSFULLY-FETCHED, COMPLETE index plus the + // wall-clock time it was fetched. Written ONLY from a resolved /spaces + // sweep whose result was complete (a truncated list is never cached, since + // it cannot answer "is this spaceId missing?"). Per-instance (a + // DocmostClient is built per user / per chat) so it can never leak across + // identities; invalidated on every identity change exactly like + // collabTokenCache (login() + the 401/403 reauth interceptor). + // - spaceIndexInFlight: dedups concurrent enrich-on-404 fetches into ONE + // /spaces sweep. CRITICAL INVARIANT: this promise is nulled in `.finally` + // on BOTH resolve AND reject — a rejected/settled promise is NEVER + // memoized, so a transient /spaces blip during one failed tool call cannot + // poison the diagnostics for the rest of the session. + protected spaceIndexCache: { + index: AccessibleSpaceIndex; + fetchedAt: number; + } | null = null; + protected spaceIndexInFlight: Promise | null = null; + // Content-addressed conversion cache for getPage (issue #479). Keyed on // (canonical pageId, updatedAt, optionsHash) -> the converted Markdown, so a // re-read of an UNCHANGED page skips the expensive convertProseMirrorToMarkdown @@ -280,6 +334,9 @@ export abstract class DocmostClientContext { // keep serving a collab token minted under the old one. this.token = null; this.collabTokenCache = null; + // #534: a new identity/login must not keep serving a space index + // computed under the old token (same reasoning as collabTokenCache). + this.spaceIndexCache = null; delete this.client.defaults.headers.common["Authorization"]; try { await this.login(); @@ -412,6 +469,8 @@ export abstract class DocmostClientContext { // Identity (re)established: drop any collab token minted under a // previous identity so the #435 cache can never outlive it. this.collabTokenCache = null; + // #534: likewise drop the accessible-space index of the old identity. + this.spaceIndexCache = null; this.client.defaults.headers.common["Authorization"] = `Bearer ${token}`; }) @@ -622,13 +681,34 @@ export abstract class DocmostClientContext { } /** - * Generic pagination handler for Docmost API endpoints + * Generic pagination handler for Docmost API endpoints. Thin wrapper over + * paginateAllWithMeta that discards the `truncated` flag — the historical + * contract every caller (getSpaces, etc.) relies on. Callers that need to KNOW + * whether the result set was complete (e.g. #534's getAccessibleSpaceIndex, + * which must not assert "spaceId missing" against a truncated list) call + * paginateAllWithMeta directly. */ async paginateAll( endpoint: string, basePayload: Record = {}, limit: number = 100, ): Promise { + return (await this.paginateAllWithMeta(endpoint, basePayload, limit)) + .items; + } + + /** + * Generic pagination handler that ALSO surfaces whether the result was + * truncated at the MAX_PAGES ceiling. `paginateAll` swallows this flag (it only + * warns); callers that must distinguish "complete listing" from "gave up at the + * cap" use this overload. `truncated` is true iff the loop stopped at the + * ceiling while the server still reported more pages. + */ + async paginateAllWithMeta( + endpoint: string, + basePayload: Record = {}, + limit: number = 100, + ): Promise<{ items: T[]; truncated: boolean }> { await this.ensureAuthenticated(); const clampedLimit = Math.max(1, Math.min(100, limit)); @@ -689,7 +769,137 @@ export abstract class DocmostClientContext { ); } - return allItems; + return { items: allItems, truncated }; + } + + /** + * The set of spaces the current token can access (issue #534), fetched from the + * single source of truth — the `/spaces` listing — with a per-instance + * short-TTL cache and single-flight dedup. Used ONLY by the enrich-on-404 slow + * path (withSpaceAccessDiagnostics), so the happy path incurs ZERO extra + * requests. + * + * `complete` is `!truncated`: it is false when the /spaces listing was cut at + * the pagination ceiling. A truncated index can never authoritatively answer + * "is this spaceId missing?", so only a complete result is cached AND only a + * complete result is allowed to drive the "not accessible" rewrite. + * + * Cache/single-flight discipline (see the spaceIndexCache / spaceIndexInFlight + * field docs): + * - serve a fresh, complete cached index without any request; + * - otherwise collapse concurrent callers onto ONE in-flight /spaces sweep; + * - write the persistent cache ONLY from a resolved, complete fetch; + * - null the in-flight promise on BOTH resolve and reject (never memoize a + * rejected promise — a transient /spaces failure must be retried fresh). + */ + async getAccessibleSpaceIndex(): Promise { + const ttl = readSpacesCacheTtlMs(); + + // Fast path: a still-fresh, complete cached index needs no request at all. + if ( + ttl > 0 && + this.spaceIndexCache && + Date.now() - this.spaceIndexCache.fetchedAt < ttl + ) { + return this.spaceIndexCache.index; + } + + // Single-flight: a concurrent enrichment joins the in-flight sweep instead of + // issuing its own. (A settled/rejected promise is never left here — see the + // `.finally` below — so this only ever joins a genuinely in-progress fetch.) + if (this.spaceIndexInFlight) return this.spaceIndexInFlight; + + const fetchPromise = (async (): Promise => { + const { items, truncated } = await this.paginateAllWithMeta("/spaces", {}); + const spaces = items.map((s: any) => ({ + id: s?.id, + name: s?.name, + })); + return { + ids: new Set(spaces.map((s) => s.id)), + spaces, + complete: !truncated, + }; + })(); + + this.spaceIndexInFlight = fetchPromise + .then((index) => { + // Cache ONLY a complete result, and only while the cache is enabled. + if (ttl > 0 && index.complete) { + this.spaceIndexCache = { index, fetchedAt: Date.now() }; + } + return index; + }) + .finally(() => { + // CRITICAL (#534): clear the in-flight slot on BOTH resolve and reject. + // Nulling on reject too means a transient /spaces error is retried by the + // NEXT enrichment with a fresh fetch, never re-serving the rejection. + this.spaceIndexInFlight = null; + }); + + return this.spaceIndexInFlight; + } + + /** + * Wrap a client method whose 404 means "the supplied spaceId is not accessible" + * and, ONLY on that 404, replace the opaque server text ("Space permissions not + * found") with a factual, actionable message naming the spaceId and the spaces + * the token can actually see (issue #534). A HINT layered on top of the + * backend, which stays authoritative — so it FAILS OPEN on ANY uncertainty: + * every branch below that is not a confident "this spaceId is genuinely + * missing" rethrows the ORIGINAL server error unchanged. The happy path returns + * fn()'s value with zero extra requests. + * + * WRAP-ALLOWLIST INVARIANT (load-bearing — read before wrapping a new method): + * among the currently wrapped tools a 404 comes ONLY from the spaceId + * membership / space-permissions check — their pageId / rootPageId / + * parentPageId branches resolve to 403 or 200, NEVER 404. If a future change + * adds a `NotFoundException` to `/pages/tree`, `/pages/recent`, + * `/pages/sidebar-pages` or `/search` (e.g. "page not found"), this enrichment + * would MISATTRIBUTE that 404 to the spaceId. Re-audit the wrapped call before + * relying on this, and only wrap paths where the sole 404 cause is the space. + */ + protected async withSpaceAccessDiagnostics( + spaceId: string, + mcpName: string, + fn: () => Promise, + ): Promise { + try { + return await fn(); + } catch (e) { + // Abort/cap wins FIRST and is detected by the SIGNAL FLAG, not e.name: a + // per-call cap may be an AbortSignal.timeout() (reason name "TimeoutError") + // or a custom reason, so `e.name === 'AbortError'` is NOT reliable (#534 + // hole B). A stopped/capped turn must propagate its reason, never trigger a + // /spaces sweep or a rewrite. + if (this.toolAbortSignal?.aborted) throw e; + + // Only a 404 is enrichable; any other status/shape is a different failure. + if (!(axios.isAxiosError(e) && e.response?.status === 404)) throw e; + + let idx: AccessibleSpaceIndex; + try { + idx = await this.getAccessibleSpaceIndex(); + } catch (fetchErr) { + // The /spaces sweep itself failed. If we were aborted mid-sweep, + // propagate the abort reason; otherwise FAIL OPEN with the ORIGINAL + // server error rather than a misleading "not found". + if (this.toolAbortSignal?.aborted) throw fetchErr; + if (process.env.DEBUG) { + console.error("space-diag: /spaces fetch failed:", fetchErr); + } + throw e; + } + + // Fail open when the listing is incomplete (can't assert "missing") or when + // the spaceId IS present (the 404 is about something else, not the space). + if (!idx.complete) throw e; + if (idx.ids.has(spaceId)) throw e; + + // Confident: the spaceId is well-formed but not among the accessible + // spaces. Replace the opaque server text with the actionable fact. + throw new Error(formatSpaceNotAccessible(mcpName, spaceId, idx.spaces)); + } } diff --git a/packages/mcp/src/client/errors.ts b/packages/mcp/src/client/errors.ts index 19c3d4aa..aa9ada95 100644 --- a/packages/mcp/src/client/errors.ts +++ b/packages/mcp/src/client/errors.ts @@ -46,6 +46,40 @@ export function assertFullUuid( } } +// Max number of accessible spaces to enumerate inline in the "space not +// accessible" message (issue #534) before collapsing the rest into a "(+N ещё)" +// tail, so a workspace with many spaces cannot blow up the model context. +const SPACE_LIST_CAP = 10; + +/** + * Compose the model-facing "spaceId is not accessible" message (issue #534). + * This is FACT text about the supplied spaceId that REPLACES the opaque server + * string ("Space permissions not found") on the enrich-on-404 path — it names + * the exact bad id, lists the spaces the token can actually see (id + name, so + * the agent can copy the right id verbatim), and points at `listSpaces`. + * + * Deliberately Russian: like the other agent-facing tool guidance in this repo, + * this is the message the acting agent reads to self-correct. + */ +export function formatSpaceNotAccessible( + mcpName: string, + spaceId: string, + spaces: { id: string; name: string }[], +): string { + // No accessible spaces at all — a distinct diagnosis (token has no space + // access), not "you picked the wrong one from this list". + if (!Array.isArray(spaces) || spaces.length === 0) { + return `${mcpName}: spaceId "${spaceId}" недоступен, и доступных тебе спейсов нет — проверь доступ токена / вызови listSpaces.`; + } + + const shown = spaces.slice(0, SPACE_LIST_CAP); + const listed = shown.map((s) => `${s.id} (${s.name})`).join(", "); + const remaining = spaces.length - shown.length; + const tail = + remaining > 0 ? ` (+${remaining} ещё, см. listSpaces)` : ""; + return `${mcpName}: spaceId "${spaceId}" не найден среди доступных тебе спейсов. Доступные: ${listed}${tail} — скопируй нужный id дословно из listSpaces.`; +} + // 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. diff --git a/packages/mcp/src/client/read.ts b/packages/mcp/src/client/read.ts index 0587ab47..85fb9fa6 100644 --- a/packages/mcp/src/client/read.ts +++ b/packages/mcp/src/client/read.ts @@ -132,17 +132,29 @@ export function ReadMixin>(Base // BFS hit its node cap) had no way to know pages were missing. Return the // tree alongside the flag; the primary /pages/tree path is uncapped so this // is false there. - const { pages, truncated } = await this.enumerateSpacePages(spaceId); - return { tree: buildPageTree(pages), truncated }; + // #534: spaceId is required here; wrap so a bad-spaceId 404 (from the + // /pages/tree seed inside enumerateSpacePages) becomes an actionable hint. + return this.withSpaceAccessDiagnostics(spaceId, "listPages", async () => { + const { pages, truncated } = await this.enumerateSpacePages(spaceId); + return { tree: buildPageTree(pages), truncated }; + }); } const clampedLimit = Math.max(1, Math.min(100, limit)); const payload: Record = { limit: clampedLimit, page: 1 }; if (spaceId) payload.spaceId = spaceId; - const response = await this.client.post("/pages/recent", payload); - const data = response.data; - const items = data.data?.items || data.items || []; - return items.map((page: any) => filterPage(page)); + // #534: only the WITH-spaceId recent path can 404 on space access; wrap it so + // that 404 is rewritten. Without a spaceId there is no space to diagnose, so + // the wrapper is inert (run the request directly). + const runRecent = async () => { + const response = await this.client.post("/pages/recent", payload); + const data = response.data; + const items = data.data?.items || data.items || []; + return items.map((page: any) => filterPage(page)); + }; + return spaceId + ? this.withSpaceAccessDiagnostics(spaceId, "listPages", runRecent) + : runRecent(); } /** @@ -174,8 +186,16 @@ export function ReadMixin>(Base "getTree: spaceId is required (a page tree is scoped to one space).", ); } - const { pages } = await this.enumerateSpacePages(spaceId, rootPageId); - return buildPageTree(pages, { shape: "getTree", maxDepth }); + // #534: the 404 for a bad/inaccessible spaceId surfaces from the + // `/pages/tree` seeding step inside enumerateSpacePages (which is NOT in a + // try/catch of its own for that case) — wrap the whole body so it is caught + // and rewritten into an actionable "spaceId not accessible" message. Only the + // space membership check can 404 here (rootPageId 403/200), see the + // wrap-allowlist invariant on withSpaceAccessDiagnostics. + return this.withSpaceAccessDiagnostics(spaceId, "getTree", async () => { + const { pages } = await this.enumerateSpacePages(spaceId, rootPageId); + return buildPageTree(pages, { shape: "getTree", maxDepth }); + }); } /** @@ -700,17 +720,27 @@ export function ReadMixin>(Base if (limit !== undefined) { payload.limit = Math.max(1, Math.min(50, limit)); } - const response = await this.client.post("/search", payload); - // Normalize both response shapes: bare array and paginated { items: [...] } - const data = response.data?.data; - const items = Array.isArray(data) ? data : data?.items || []; - const filteredItems = items.map((item: any) => filterSearchResult(item)); + const runSearch = async () => { + const response = await this.client.post("/search", payload); - return { - items: filteredItems, - success: response.data?.success || false, + // Normalize both response shapes: bare array and paginated { items: [...] } + const data = response.data?.data; + const items = Array.isArray(data) ? data : data?.items || []; + const filteredItems = items.map((item: any) => filterSearchResult(item)); + + return { + items: filteredItems, + success: response.data?.success || false, + }; }; + + // #534: a search scoped to a spaceId 404s when that space is inaccessible; + // wrap only that case so the 404 becomes an actionable hint. A workspace-wide + // search (no spaceId) has no space to diagnose — run it directly (inert). + return spaceId + ? this.withSpaceAccessDiagnostics(spaceId, "search", runSearch) + : runSearch(); } } diff --git a/packages/mcp/test/unit/space-access-diagnostics.test.mjs b/packages/mcp/test/unit/space-access-diagnostics.test.mjs new file mode 100644 index 00000000..1d91355b --- /dev/null +++ b/packages/mcp/test/unit/space-access-diagnostics.test.mjs @@ -0,0 +1,354 @@ +// Issue #534: enrich-on-404 space-access diagnostics. +// +// When a tool is handed a well-formed but non-existent/inaccessible spaceId, the +// server answers the space-permissions check with an opaque 404 ("Space +// permissions not found"). The client wrapper (withSpaceAccessDiagnostics) turns +// ONLY that 404 into an actionable message naming the bad spaceId and the spaces +// the token can actually see — while FAILING OPEN (rethrowing the original +// server error unchanged) on every source of uncertainty. +// +// These tests drive the assembled DocmostClient with its inner seams +// (enumerateSpacePages / client.post / paginateAllWithMeta) stubbed at runtime, +// mirroring the stub-client style of error-diagnostics.test.mjs. Only criterion +// 9 (createPage is NOT wrapped) uses a real offline http server, because +// createPage's 404 arrives over a bare-axios multipart path. +import { test, after } from "node:test"; +import assert from "node:assert/strict"; +import http from "node:http"; +import axios, { AxiosError } from "axios"; +import { + DocmostClient, + formatSpaceNotAccessible, +} from "../../build/client.js"; + +// Two accessible spaces (id + name is all getAccessibleSpaceIndex maps/uses). +const SPACES = [ + { id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", name: "Engineering" }, + { id: "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb", name: "Design" }, +]; +// A well-formed UUID that is NOT among the accessible spaces. +const BAD = "99999999-9999-4999-8999-999999999999"; + +// Build an AxiosError shaped exactly as the response interceptor would hand it +// on a 404 — status readable, axios.isAxiosError() true. +function make404(url = "/pages/tree") { + const config = { method: "post", url, baseURL: "http://host.example/api" }; + const response = { + status: 404, + statusText: "Not Found", + data: { message: "Space permissions not found" }, + headers: {}, + config, + }; + return new AxiosError( + "Request failed with status code 404", + "ERR_BAD_REQUEST", + config, + {}, + response, + ); +} + +// A client whose token is pre-set (so ensureAuthenticated never hits the +// network) and whose /spaces sweep is a counted stub. Individual tests override +// enumerateSpacePages / client.post to shape the method-under-test's outcome. +function makeClient({ spaces = SPACES, truncated = false } = {}) { + const c = new DocmostClient("http://127.0.0.1:1/api", "u@example.com", "pw"); + c.token = "t"; + c.client.defaults.headers.common["Authorization"] = "Bearer t"; + c._spacesFetches = 0; + c.paginateAllWithMeta = async (endpoint) => { + if (endpoint === "/spaces") { + c._spacesFetches++; + return { items: spaces, truncated }; + } + throw new Error(`unexpected paginate endpoint ${endpoint}`); + }; + return c; +} + +// --- Criterion 1: bad spaceId on getTree -> actionable rewrite -------------- +test("getTree with a non-existent spaceId is rewritten into an actionable hint", async () => { + const c = makeClient(); + c.enumerateSpacePages = async () => { + throw make404(); + }; + + await assert.rejects( + () => c.getTree(BAD), + (e) => { + assert.ok(e.message.includes(BAD), "names the passed spaceId"); + // At least one valid "id (name)" pair. + assert.ok( + e.message.includes(`${SPACES[0].id} (${SPACES[0].name})`), + "lists an accessible id (name)", + ); + assert.ok(e.message.includes("listSpaces"), "points at listSpaces"); + assert.ok( + !e.message.includes("Space permissions not found"), + "the opaque server text is replaced", + ); + return true; + }, + ); + assert.equal(c._spacesFetches, 1, "one /spaces sweep on the enrichment path"); +}); + +// --- Criterion 2: happy path -> no /spaces request -------------------------- +test("getTree with a valid spaceId returns the tree and makes NO /spaces request", async () => { + const c = makeClient(); + // Spy client.post to prove no /spaces POST is issued on the happy path. + const posted = []; + c.client.post = async (url) => { + posted.push(url); + throw new Error(`unexpected post ${url}`); + }; + c.enumerateSpacePages = async () => ({ pages: [], truncated: false }); + + const res = await c.getTree(SPACES[0].id); + assert.ok(Array.isArray(res), "tree returned as before"); + assert.equal(c._spacesFetches, 0, "no /spaces sweep on the happy path"); + assert.ok( + !posted.includes("/spaces"), + "no /spaces POST on the happy path", + ); +}); + +// --- Criterion 3: short-TTL cache + refetch after expiry -------------------- +test("two bad getTree within TTL share ONE /spaces fetch; a refetch happens after TTL", async () => { + const c = makeClient(); + c.enumerateSpacePages = async () => { + throw make404(); + }; + + await assert.rejects(() => c.getTree(BAD), /не найден среди/); + await assert.rejects(() => c.getTree(BAD), /не найден среди/); + assert.equal(c._spacesFetches, 1, "second call served from cache"); + + // Age the cache past the default 60s TTL -> exactly one refetch. + assert.ok(c.spaceIndexCache, "a complete result was cached"); + c.spaceIndexCache.fetchedAt = Date.now() - 10 * 60 * 1000; + await assert.rejects(() => c.getTree(BAD), /не найден среди/); + assert.equal(c._spacesFetches, 2, "exactly one refetch after TTL"); +}); + +// --- Criterion 4: no spaceId -> wrapper inert ------------------------------- +test("listPages WITHOUT spaceId is inert (raw error propagates, no /spaces sweep)", async () => { + const c = makeClient(); + c.client.post = async () => { + throw make404("/pages/recent"); + }; + await assert.rejects( + () => c.listPages(), + (e) => { + assert.equal(e.response?.status, 404, "raw server 404 propagates"); + assert.ok(!/не найден среди/.test(e.message ?? ""), "not rewritten"); + return true; + }, + ); + assert.equal(c._spacesFetches, 0, "no /spaces sweep without a spaceId"); +}); + +test("search WITHOUT spaceId is inert (raw error propagates, no /spaces sweep)", async () => { + const c = makeClient(); + c.client.post = async () => { + throw make404("/search"); + }; + await assert.rejects( + () => c.search("query"), + (e) => { + assert.equal(e.response?.status, 404, "raw server 404 propagates"); + assert.ok(!/не найден среди/.test(e.message ?? ""), "not rewritten"); + return true; + }, + ); + assert.equal(c._spacesFetches, 0, "no /spaces sweep without a spaceId"); +}); + +// --- Criterion 5: incomplete listing -> fail open --------------------------- +test("a truncated /spaces listing (!complete) fails OPEN with the original 404", async () => { + const c = makeClient({ truncated: true }); + c.enumerateSpacePages = async () => { + throw make404(); + }; + await assert.rejects( + () => c.getTree(BAD), + (e) => { + assert.equal(e.response?.status, 404, "original server error preserved"); + assert.ok(!/не найден среди/.test(e.message ?? ""), "no false rewrite"); + return true; + }, + ); + assert.equal(c.spaceIndexCache, null, "a truncated result is never cached"); +}); + +// --- Criterion 6: /spaces fetch fails -> fail open; in-flight nulled on reject +test("a /spaces fetch failure fails OPEN, logs under DEBUG, and never memoizes the rejected in-flight promise", async () => { + const c = makeClient(); + let fetchCount = 0; + c.paginateAllWithMeta = async () => { + fetchCount++; + throw new Error("network boom"); + }; + c.enumerateSpacePages = async () => { + throw make404(); + }; + + const prevDebug = process.env.DEBUG; + process.env.DEBUG = "1"; + const errs = []; + const origErr = console.error; + console.error = (...a) => errs.push(a.map(String).join(" ")); + try { + await assert.rejects( + () => c.getTree(BAD), + (e) => { + assert.equal(e.response?.status, 404, "original 404 rethrown"); + return true; + }, + ); + } finally { + console.error = origErr; + if (prevDebug === undefined) delete process.env.DEBUG; + else process.env.DEBUG = prevDebug; + } + + assert.ok( + errs.some((l) => l.includes("space-diag: /spaces fetch failed")), + "a DEBUG stderr line is emitted", + ); + assert.equal(c.spaceIndexInFlight, null, "in-flight promise nulled on reject"); + + // The rejected in-flight promise must NOT be reused: a second enrichment does + // a genuinely fresh fetch (fetchCount increments to 2). + await assert.rejects( + () => c.getTree(BAD), + (e) => e.response?.status === 404, + ); + assert.equal(fetchCount, 2, "fresh fetch — rejected promise not memoized"); +}); + +// --- Criterion 7: zero accessible spaces ------------------------------------ +test("zero accessible spaces yields the dedicated 'no accessible spaces' message", async () => { + const c = makeClient({ spaces: [] }); + c.enumerateSpacePages = async () => { + throw make404(); + }; + await assert.rejects( + () => c.getTree(BAD), + (e) => { + assert.ok( + e.message.includes("доступных тебе спейсов нет"), + "the zero-spaces branch fired", + ); + assert.ok(e.message.includes(BAD), "still names the bad spaceId"); + return true; + }, + ); +}); + +// --- Criterion 8: abort/cap during enrichment ------------------------------- +test("an aborted signal (custom/TimeoutError reason) propagates its reason, not a rewrite, and skips the sweep", async () => { + const c = makeClient(); + const ac = new AbortController(); + const reason = new Error("per-call cap exceeded"); + reason.name = "TimeoutError"; // NOT 'AbortError' — hole B + ac.abort(reason); + c.setToolAbortSignal(ac.signal); + // Simulate paginateAll's throwIfAborted(): the inner op rejects with the + // signal's reason (not an AxiosError). + c.enumerateSpacePages = async () => { + throw ac.signal.reason; + }; + + await assert.rejects( + () => c.getTree(BAD), + (e) => { + assert.equal(e, reason, "the abort/cap reason itself propagates"); + assert.equal(e.name, "TimeoutError"); + assert.ok(!/не найден среди/.test(e.message ?? ""), "no rewrite"); + return true; + }, + ); + assert.equal(c._spacesFetches, 0, "abort short-circuits before any sweep"); +}); + +// --- Criterion 9: createPage is NOT wrapped (real offline server) ----------- +function readBody(req) { + return new Promise((resolve) => { + let raw = ""; + req.on("data", (c) => (raw += c)); + req.on("end", () => resolve(raw)); + }); +} +function sendJson(res, status, obj, extra = {}) { + res.writeHead(status, { "Content-Type": "application/json", ...extra }); + res.end(JSON.stringify(obj)); +} +const openServers = []; +function spawn(handler) { + return new Promise((resolve) => { + const server = http.createServer(handler); + server.listen(0, "127.0.0.1", () => { + openServers.push(server); + const { port } = server.address(); + resolve({ baseURL: `http://127.0.0.1:${port}/api` }); + }); + }); +} +after(async () => { + await Promise.all(openServers.map((s) => new Promise((r) => s.close(r)))); +}); + +test("createPage with an inaccessible spaceId returns the server error as-is (NOT wrapped)", 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/import") { + // The space-permissions 404 createPage would see for a bad spaceId. + sendJson(res, 404, { message: "Space permissions not found" }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "u@example.com", "pw"); + // Prove the diagnostics path is never entered from createPage. + let idxCalls = 0; + const realIdx = client.getAccessibleSpaceIndex.bind(client); + client.getAccessibleSpaceIndex = async () => { + idxCalls++; + return realIdx(); + }; + + await assert.rejects( + () => client.createPage("Title", "body", BAD), + (e) => { + assert.equal(e.response?.status, 404, "the raw server 404 surfaces"); + assert.ok( + !/не найден среди/.test(e.message ?? ""), + "createPage's 404 is NOT rewritten (multi-cause path)", + ); + return true; + }, + ); + assert.equal(idxCalls, 0, "createPage never invokes the space diagnostics"); +}); + +// --- formatSpaceNotAccessible unit shape ------------------------------------ +test("formatSpaceNotAccessible caps the inline list at 10 and appends a (+N ещё) tail", () => { + const many = Array.from({ length: 13 }, (_, i) => ({ + id: `id-${i}`, + name: `Space ${i}`, + })); + const msg = formatSpaceNotAccessible("getTree", BAD, many); + assert.ok(msg.includes("id-0 (Space 0)")); + assert.ok(msg.includes("id-9 (Space 9)"), "10th entry (index 9) is shown"); + assert.ok(!msg.includes("id-10 (Space 10)"), "the 11th is collapsed"); + assert.ok(msg.includes("(+3 ещё, см. listSpaces)"), "tail counts the remainder"); +}); From e2367822603fd819c6c21f0bfe27c20ae7e167c4 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 05:49:31 +0300 Subject: [PATCH 2/3] fix(mcp): cap #534 space-not-accessible message + cover fail-open guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up (#536). F1 (test coverage): add two tests that pin the wrapper's fail-open guards, which previously survived mutation: - a non-404 error (500) on a wrapped call with a spaceId propagates unchanged and triggers NO /spaces sweep (a real 5xx/403 must never be swallowed/reformatted); - a 404 when the spaceId IS in the accessible index fails open (the 404 is about another resource), so it is not falsely rewritten to "not found among spaces". Both mutation-verified: forcing the non-404 condition to false reddens the first; removing the id-present guard reddens the second. F2 (conventions): formatSpaceNotAccessible now honours ERROR_MESSAGE_CAP (300), the same budget formatDocmostAxiosError enforces. Only the interpolated space list is truncated (with an ellipsis); the fixed prefix (bad spaceId) and suffix (listSpaces pointer) are always kept, so the actionable parts survive. Test: 10 long space names -> message <= 300 and still contains the spaceId + listSpaces. Adjusted the existing list-cap test to short ids/names so it exercises the 10-item cap + "(+N ещё)" tail below the length cap. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/client/errors.ts | 17 +++- .../unit/space-access-diagnostics.test.mjs | 89 ++++++++++++++++--- 2 files changed, 94 insertions(+), 12 deletions(-) diff --git a/packages/mcp/src/client/errors.ts b/packages/mcp/src/client/errors.ts index aa9ada95..787c6b88 100644 --- a/packages/mcp/src/client/errors.ts +++ b/packages/mcp/src/client/errors.ts @@ -73,11 +73,24 @@ export function formatSpaceNotAccessible( } const shown = spaces.slice(0, SPACE_LIST_CAP); - const listed = shown.map((s) => `${s.id} (${s.name})`).join(", "); const remaining = spaces.length - shown.length; const tail = remaining > 0 ? ` (+${remaining} ещё, см. listSpaces)` : ""; - return `${mcpName}: spaceId "${spaceId}" не найден среди доступных тебе спейсов. Доступные: ${listed}${tail} — скопируй нужный id дословно из listSpaces.`; + + // Cap the whole message at ERROR_MESSAGE_CAP (same budget as + // formatDocmostAxiosError) so ~10 long space names cannot blow up the model + // context. Truncate ONLY the interpolated space LIST — the fixed prefix (which + // carries the bad spaceId) and the fixed suffix (the "…из listSpaces" + // instruction) are always kept intact, so the actionable parts survive even + // when the list is trimmed. + const prefix = `${mcpName}: spaceId "${spaceId}" не найден среди доступных тебе спейсов. Доступные: `; + const suffix = ` — скопируй нужный id дословно из listSpaces.`; + let listed = shown.map((s) => `${s.id} (${s.name})`).join(", ") + tail; + const budget = ERROR_MESSAGE_CAP - prefix.length - suffix.length; + if (listed.length > budget) { + listed = listed.slice(0, Math.max(0, budget - 1)) + "…"; + } + return `${prefix}${listed}${suffix}`; } // Keep ONLY the pathname of a request (no host, no query string, no fragment) diff --git a/packages/mcp/test/unit/space-access-diagnostics.test.mjs b/packages/mcp/test/unit/space-access-diagnostics.test.mjs index 1d91355b..8f12c124 100644 --- a/packages/mcp/test/unit/space-access-diagnostics.test.mjs +++ b/packages/mcp/test/unit/space-access-diagnostics.test.mjs @@ -31,23 +31,26 @@ const BAD = "99999999-9999-4999-8999-999999999999"; // Build an AxiosError shaped exactly as the response interceptor would hand it // on a 404 — status readable, axios.isAxiosError() true. -function make404(url = "/pages/tree") { +function makeAxiosErr(status, url = "/pages/tree", message = "boom") { const config = { method: "post", url, baseURL: "http://host.example/api" }; const response = { - status: 404, - statusText: "Not Found", - data: { message: "Space permissions not found" }, + status, + statusText: String(status), + data: { message }, headers: {}, config, }; return new AxiosError( - "Request failed with status code 404", + `Request failed with status code ${status}`, "ERR_BAD_REQUEST", config, {}, response, ); } +function make404(url = "/pages/tree") { + return makeAxiosErr(404, url, "Space permissions not found"); +} // A client whose token is pre-set (so ensureAuthenticated never hits the // network) and whose /spaces sweep is a counted stub. Individual tests override @@ -165,6 +168,51 @@ test("search WITHOUT spaceId is inert (raw error propagates, no /spaces sweep)", assert.equal(c._spacesFetches, 0, "no /spaces sweep without a spaceId"); }); +// --- Fail-open: a NON-404 error is never enriched (regression guard) -------- +test("a non-404 error (500) on a wrapped call propagates unchanged, with NO /spaces sweep", async () => { + const c = makeClient(); + // A real server failure — must surface as-is, never be swallowed by the + // enrichment path or reformatted into a "not found among your spaces" message. + c.enumerateSpacePages = async () => { + throw makeAxiosErr(500, "/pages/tree", "Internal Server Error"); + }; + await assert.rejects( + () => c.getTree(BAD), + (e) => { + assert.equal(e.response?.status, 500, "the original 500 propagates"); + assert.ok( + !/не найден среди/.test(e.message ?? ""), + "a non-404 is NOT rewritten", + ); + return true; + }, + ); + assert.equal(c._spacesFetches, 0, "no /spaces sweep for a non-404"); +}); + +// --- Fail-open: 404 when the spaceId IS accessible -> not about the space ---- +test("a 404 when the spaceId IS in the accessible index fails open (the 404 is about something else)", async () => { + const c = makeClient(); + // The wrapped call 404s, but the spaceId is genuinely accessible — the 404 + // must be about some OTHER resource, so the original error propagates and is + // NOT falsely rewritten to "spaceId not found among your spaces". + c.enumerateSpacePages = async () => { + throw make404(); + }; + await assert.rejects( + () => c.getTree(SPACES[0].id), + (e) => { + assert.equal(e.response?.status, 404, "original 404 preserved"); + assert.ok( + !/не найден среди/.test(e.message ?? ""), + "no false 'not found' when the space is accessible", + ); + return true; + }, + ); + assert.equal(c._spacesFetches, 1, "the index WAS consulted to make this call"); +}); + // --- Criterion 5: incomplete listing -> fail open --------------------------- test("a truncated /spaces listing (!complete) fails OPEN with the original 404", async () => { const c = makeClient({ truncated: true }); @@ -342,13 +390,34 @@ test("createPage with an inaccessible spaceId returns the server error as-is (NO // --- formatSpaceNotAccessible unit shape ------------------------------------ test("formatSpaceNotAccessible caps the inline list at 10 and appends a (+N ещё) tail", () => { + // Short ids/names so the whole message stays under the length cap and the full + // list-cap behaviour (first 10 shown, rest collapsed) is observable intact. const many = Array.from({ length: 13 }, (_, i) => ({ - id: `id-${i}`, - name: `Space ${i}`, + id: `s${i}`, + name: `${i}`, })); const msg = formatSpaceNotAccessible("getTree", BAD, many); - assert.ok(msg.includes("id-0 (Space 0)")); - assert.ok(msg.includes("id-9 (Space 9)"), "10th entry (index 9) is shown"); - assert.ok(!msg.includes("id-10 (Space 10)"), "the 11th is collapsed"); + assert.ok(msg.includes("s0 (0)")); + assert.ok(msg.includes("s9 (9)"), "10th entry (index 9) is shown"); + assert.ok(!msg.includes("s10 (10)"), "the 11th is collapsed"); assert.ok(msg.includes("(+3 ещё, см. listSpaces)"), "tail counts the remainder"); + assert.ok(msg.length <= 300, `short-name message stays under the cap (${msg.length})`); +}); + +test("formatSpaceNotAccessible caps the assembled message at ERROR_MESSAGE_CAP (300)", () => { + // 10 spaces with long names would, uncapped, produce a message several times + // over the 300-char budget. The cap must keep it compact while still carrying + // the bad spaceId and the listSpaces pointer. + const longName = "X".repeat(120); + const spaces = Array.from({ length: 10 }, (_, i) => ({ + id: `id-${i}`, + name: `${longName}-${i}`, + })); + const msg = formatSpaceNotAccessible("getTree", BAD, spaces); + assert.ok( + msg.length <= 300, + `message must be <= 300 chars, got ${msg.length}`, + ); + assert.ok(msg.includes(BAD), "the bad spaceId survives the cap"); + assert.ok(msg.includes("listSpaces"), "the listSpaces pointer survives the cap"); }); From 8b34a428f41b0263e14a0009912c3990a3b2e16e Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 05:58:04 +0300 Subject: [PATCH 3/3] fix(mcp): unconditional final ERROR_MESSAGE_CAP backstop in formatSpaceNotAccessible (#536 review) The list-only cap assumed a well-formed prefix; a pathologically long agent-supplied spaceId (unvalidated for length) lives in the prefix and bypassed the cap. Add the same unconditional final slice formatDocmostAxiosError uses, so the WHOLE message is bounded regardless of spaceId length. No-op for normal inputs (36-char UUID) where the list cap already keeps it <= 300. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/src/client/errors.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/mcp/src/client/errors.ts b/packages/mcp/src/client/errors.ts index 787c6b88..07b4aa08 100644 --- a/packages/mcp/src/client/errors.ts +++ b/packages/mcp/src/client/errors.ts @@ -90,7 +90,14 @@ export function formatSpaceNotAccessible( if (listed.length > budget) { listed = listed.slice(0, Math.max(0, budget - 1)) + "…"; } - return `${prefix}${listed}${suffix}`; + const message = `${prefix}${listed}${suffix}`; + // Unconditional final backstop (mirrors formatDocmostAxiosError): the list + // cap above assumes a well-formed prefix, but a pathologically long + // agent-supplied spaceId lives in the prefix and would otherwise blow past the + // budget. Cap the WHOLE message so nothing bloats the model context. + return message.length > ERROR_MESSAGE_CAP + ? message.slice(0, ERROR_MESSAGE_CAP - 1) + "…" + : message; } // Keep ONLY the pathname of a request (no host, no query string, no fragment)