fix(mcp): внятная ошибка на недоступный spaceId вместо «Space permissions not found» (#534) #536
@@ -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";
|
||||
|
||||
// Branded canonical page-identity type (#435): the internal page UUID is a
|
||||
// distinct nominal type so an unresolved raw/slug string can't be swapped into
|
||||
|
||||
@@ -725,10 +725,15 @@ export function CommentsMixin<TBase extends GConstructor<DocmostClientContext>>(
|
||||
// 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. Runs with
|
||||
// bounded concurrency (#490) instead of one-at-a-time — the per-page reads are
|
||||
|
||||
@@ -26,7 +26,10 @@ import {
|
||||
import { withPageLock, isUuid } from "../lib/page-lock.js";
|
||||
import type { PageId } from "../lib/page-id.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
|
||||
@@ -117,6 +120,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<string>;
|
||||
spaces: { id: string; name: string }[];
|
||||
complete: boolean;
|
||||
};
|
||||
|
||||
export abstract class DocmostClientContext {
|
||||
protected client: AxiosInstance;
|
||||
protected token: string | null = null;
|
||||
@@ -164,6 +197,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<AccessibleSpaceIndex> | 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
|
||||
@@ -281,6 +335,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();
|
||||
@@ -413,6 +470,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}`;
|
||||
})
|
||||
@@ -630,13 +689,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<T = any>(
|
||||
endpoint: string,
|
||||
basePayload: Record<string, any> = {},
|
||||
limit: number = 100,
|
||||
): Promise<T[]> {
|
||||
return (await this.paginateAllWithMeta<T>(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<T = any>(
|
||||
endpoint: string,
|
||||
basePayload: Record<string, any> = {},
|
||||
limit: number = 100,
|
||||
): Promise<{ items: T[]; truncated: boolean }> {
|
||||
await this.ensureAuthenticated();
|
||||
|
||||
const clampedLimit = Math.max(1, Math.min(100, limit));
|
||||
@@ -697,7 +777,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<AccessibleSpaceIndex> {
|
||||
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<AccessibleSpaceIndex> => {
|
||||
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<T>(
|
||||
spaceId: string,
|
||||
mcpName: string,
|
||||
fn: () => Promise<T>,
|
||||
): Promise<T> {
|
||||
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));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
|
||||
@@ -46,6 +46,60 @@ 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 remaining = spaces.length - shown.length;
|
||||
const tail =
|
||||
remaining > 0 ? ` (+${remaining} ещё, см. 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)) + "…";
|
||||
}
|
||||
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)
|
||||
// so the message never leaks a host or query params. Resolves a relative
|
||||
// config.url against config.baseURL, then discards everything but the path.
|
||||
|
||||
@@ -132,17 +132,29 @@ export function ReadMixin<TBase extends GConstructor<DocmostClientContext>>(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<string, any> = { 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<TBase extends GConstructor<DocmostClientContext>>(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<TBase extends GConstructor<DocmostClientContext>>(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();
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@@ -0,0 +1,423 @@
|
||||
// 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 makeAxiosErr(status, url = "/pages/tree", message = "boom") {
|
||||
const config = { method: "post", url, baseURL: "http://host.example/api" };
|
||||
const response = {
|
||||
status,
|
||||
statusText: String(status),
|
||||
data: { message },
|
||||
headers: {},
|
||||
config,
|
||||
};
|
||||
return new AxiosError(
|
||||
`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
|
||||
// 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");
|
||||
});
|
||||
|
||||
// --- 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 });
|
||||
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", () => {
|
||||
// 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: `s${i}`,
|
||||
name: `${i}`,
|
||||
}));
|
||||
const msg = formatSpaceNotAccessible("getTree", BAD, many);
|
||||
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");
|
||||
});
|
||||
Reference in New Issue
Block a user