diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 20040432..509e4837 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -552,16 +552,18 @@ export class DocmostClient { // forever and accumulate duplicates). const MAX_PAGES = 50; - let page = 1; + let cursor: string | undefined; let allItems: T[] = []; - let hasNextPage = true; + let truncated = false; - while (hasNextPage && page <= MAX_PAGES) { - const response = await this.client.post(endpoint, { + for (let page = 0; page < MAX_PAGES; page++) { + const payload: Record = { ...basePayload, limit: clampedLimit, - page, - }); + }; + if (cursor) payload.cursor = cursor; + + const response = await this.client.post(endpoint, payload); const data = response.data; const items = data.data?.items || data.items || []; @@ -569,22 +571,28 @@ export class DocmostClient { allItems = allItems.concat(items); - // Stop if the page is empty or shorter than the requested size: a full - // page worth of items is the only situation where another page can exist, - // so this defends against a stuck hasNextPage flag in addition to it. - if (items.length === 0 || items.length < clampedLimit) { + // Advance strictly via the server-issued cursor. A missing nextCursor (or + // hasNextPage false) means we reached the end. A cursor identical to the + // one we just sent means the server did not understand our pagination + // param — stop instead of re-fetching page one forever and duplicating. + const next = meta?.hasNextPage ? meta?.nextCursor : null; + if (!next || next === cursor) { + // If the server still reports more pages but stopped issuing a usable + // cursor at the ceiling, flag the result as truncated below. + if (page === MAX_PAGES - 1 && meta?.hasNextPage) truncated = true; break; } + cursor = next; - hasNextPage = meta?.hasNextPage || false; - page++; + // Reaching the ceiling with more pages still available means the result + // set is truncated. + if (page === MAX_PAGES - 1) truncated = true; } // If the loop stopped because it hit the MAX_PAGES ceiling while the server - // still reported more results (hasNextPage true and the last page was - // full), the result set is truncated — warn so the caller is not silently - // handed an incomplete list. - if (hasNextPage && page > MAX_PAGES) { + // still reported more results, the result set is truncated — warn so the + // caller is not silently handed an incomplete list. + if (truncated) { console.warn( `paginateAll: results from "${endpoint}" truncated at the ${MAX_PAGES}-page cap; more pages exist on the server`, ); @@ -618,9 +626,10 @@ export class DocmostClient { * Tree (`tree` true): the space's FULL page hierarchy as a nested tree (each * node has a `children` array). This mode REQUIRES `spaceId` (a page tree is * scoped to one space) and IGNORES `limit` — the whole hierarchy is returned. - * It walks the sidebar tree via `enumerateSpacePages`, which performs N - * sidebar requests and is bounded by that method's 10000-node cap (and skips - * soft-deleted pages server-side). + * It fetches the tree via `enumerateSpacePages`, which on the fork server + * resolves to a single `/pages/tree` request returning the whole + * permission-filtered flat page set (soft-deleted pages excluded + * server-side). */ async listPages(spaceId?: string, limit: number = 50, tree: boolean = false) { await this.ensureAuthenticated(); @@ -631,8 +640,8 @@ export class DocmostClient { "list_pages: tree mode requires a spaceId (a page tree is scoped to one space). Pass spaceId, or omit tree to get the recent-pages list.", ); } - const nodes = await this.enumerateSpacePages(spaceId); - return buildPageTree(nodes); + const { pages } = await this.enumerateSpacePages(spaceId); + return buildPageTree(pages); } const clampedLimit = Math.max(1, Math.min(100, limit)); @@ -654,57 +663,123 @@ export class DocmostClient { async listSidebarPages(spaceId: string, pageId?: string) { await this.ensureAuthenticated(); - // Paginate: the endpoint returns server-paged children, so posting only - // { page: 1 } silently dropped every child beyond the first page. Loop on - // meta.hasNextPage (with a MAX_PAGES ceiling like paginateAll, guarding - // against a stuck hasNextPage flag) and accumulate all children. + // Paginate via the server-issued cursor. The server switched from OFFSET + // (`page`) to CURSOR (`cursor`/`nextCursor`) pagination, and the global + // ValidationPipe(whitelist:true) SILENTLY STRIPS the obsolete `page` field + // — so the old offset loop got the SAME first page every time (with + // hasNextPage stuck true) and dropped every child beyond the first page. const MAX_PAGES = 50; - let page = 1; + let cursor: string | undefined; let allItems: any[] = []; - let hasNextPage = true; + let truncated = false; - while (hasNextPage && page <= MAX_PAGES) { + for (let i = 0; i < MAX_PAGES; i++) { + // limit: 100 is the server-side Max; cuts request count 5x vs the default 20. + const payload: Record = { spaceId, limit: 100 }; // Only send pageId when scoping to a page's children; omit it for roots. - const payload: Record = { spaceId, page }; if (pageId) payload.pageId = pageId; + if (cursor) payload.cursor = cursor; - const response = await this.client.post("/pages/sidebar-pages", payload); - const data = response.data?.data ?? response.data; - const items = data?.items || []; - allItems = allItems.concat(items); + const data = (await this.client.post("/pages/sidebar-pages", payload)).data + ?.data; + allItems = allItems.concat(data?.items ?? []); - hasNextPage = data?.meta?.hasNextPage || false; - page++; + // Advance strictly via the server-issued cursor; a missing/repeated cursor + // means the protocol drifted again — stop instead of looping on page one. + const next = data?.meta?.hasNextPage ? data?.meta?.nextCursor : null; + if (!next || next === cursor) break; + cursor = next; + + // Reaching the ceiling with more pages still available means the child + // list is truncated (mirrors paginateAll). + if (i === MAX_PAGES - 1) truncated = true; + } + + // Warn on real truncation (ceiling hit while the server still had pages) so + // the caller is not silently handed an incomplete child list. + if (truncated) { + console.warn( + `listSidebarPages: children of "${pageId ?? spaceId}" truncated at the ${MAX_PAGES}-page cap; more pages exist on the server`, + ); } return allItems; } /** - * Enumerate EVERY page in a space (or in a subtree, when rootPageId is given) - * by walking the sidebar-pages tree. + * Enumerate EVERY page in a space (or in a subtree, when rootPageId is given). * - * Starting set: the children of rootPageId when provided, otherwise the - * space root pages. From there it does an iterative breadth-first walk: each - * node is collected, and when node.hasChildren is true its direct children - * are fetched via listSidebarPages(spaceId, node.id) and enqueued. + * Primary path (fork server): a SINGLE `POST /pages/tree` returns the whole + * space (or a subtree) as a flat, permission-filtered list in one request, in + * the exact node shape buildPageTree consumes. This replaces the old + * per-node BFS, which issued N sidebar requests and — after the server moved + * to cursor pagination — silently lost every child past the first sidebar + * page (the obsolete `page` param was stripped by ValidationPipe). * - * This replaces the old "/pages/recent" enumeration, which is a bounded - * recent-activity feed (~5000 cap) and therefore misses comments on older - * pages that were never recently touched. + * The subtree variant (rootPageId given) INCLUDES the root node itself + * (getPageAndDescendants seeds with id = rootPageId), unlike the old BFS + * which started from the root's children. * - * Safeguards: a `visited` Set of page ids prevents re-processing a node - * (cycles / duplicate references), and a hard node cap bounds pathological - * trees so the walk always terminates. + * Fallback path (stdio mode may target STOCK upstream Docmost, which lacks + * `/pages/tree`): on a 404/405 it falls back to the cursor-based BFS below, + * walking direct children via the fixed cursor listSidebarPages. Safeguards: + * a `visited` Set of page ids prevents re-processing a node (cycles / + * duplicate references), and a hard node cap bounds pathological trees so the + * walk always terminates. + * + * Returns `{ pages, truncated }`. `truncated` is true ONLY when the fallback + * BFS stopped at its MAX_NODES cap — the primary /pages/tree path is uncapped + * and always returns the complete set, so it never reports truncation. */ private async enumerateSpacePages( spaceId: string, rootPageId?: string, - ): Promise { + ): Promise<{ pages: any[]; truncated: boolean }> { + await this.ensureAuthenticated(); + + // Single request replaces the whole BFS: /pages/tree returns the full + // permission-filtered flat page set of a space (or a subtree) at once. This + // path is uncapped, so it is never truncated. + const payload = rootPageId ? { pageId: rootPageId } : { spaceId }; + try { + const response = await this.client.post("/pages/tree", payload); + const pages = (response.data?.data ?? response.data)?.items ?? []; + return { pages, truncated: false }; + } catch (e: any) { + // Only fall back when the endpoint is absent (stock upstream Docmost); + // any other error is a genuine failure and must propagate. + if ( + !axios.isAxiosError(e) || + (e.response?.status !== 404 && e.response?.status !== 405) + ) { + throw e; + } + } + + // Fallback: cursor-based breadth-first walk via listSidebarPages. const MAX_NODES = 10000; const result: any[] = []; const visited = new Set(); + // Seed with the root node itself when scoping to a subtree, so its own + // comments aren't dropped: the primary /pages/tree seeds + // getPageAndDescendants with id = rootPageId (root included), but + // listSidebarPages(spaceId, rootPageId) returns only the root's CHILDREN. + // The `visited` set below prevents a double-add if the root also appears + // among the children. getPageRaw returns a page whose id/title/spaceId are + // exactly what buildPageTree and check_new_comments consume. + if (rootPageId) { + try { + const root = await this.getPageRaw(rootPageId); + if (root?.id) { + result.push(root); + visited.add(root.id); + } + } catch { + // Non-fatal: if the root can't be read, fall through to children-only. + } + } + // Seed the queue with the starting level (subtree children or roots). const queue: any[] = await this.listSidebarPages(spaceId, rootPageId); @@ -729,7 +804,12 @@ export class DocmostClient { } } - return result; + // Truncated only when the cap was hit with the queue still non-empty (real + // truncation, not a natural end at exactly MAX_NODES). + return { + pages: result, + truncated: result.length >= MAX_NODES && queue.length > 0, + }; } /** Raw page info including the ProseMirror JSON content and slugId. */ @@ -2360,7 +2440,13 @@ export class DocmostClient { let allComments: any[] = []; let cursor: string | null = null; - do { + // Hard ceiling + immovable-cursor guard (mirrors paginateAll): if /comments + // ever stops advancing the cursor (the exact #442 drift scenario) this loop + // would otherwise spin forever accumulating duplicates. + const MAX_PAGES = 50; + let truncated = false; + + for (let page = 0; page < MAX_PAGES; page++) { const payload: Record = { pageId, limit: 100 }; if (cursor) payload.cursor = cursor; @@ -2368,8 +2454,23 @@ export class DocmostClient { const data = response.data.data || response.data; const items = data.items || []; allComments = allComments.concat(items); - cursor = data.meta?.nextCursor || null; - } while (cursor); + + // Advance strictly via the server-issued cursor. A missing nextCursor or a + // cursor identical to the one we just sent means the end (or a server that + // ignores our pagination param) — stop instead of re-fetching page one. + const next: string | null = data.meta?.nextCursor || null; + if (!next || next === cursor) break; + cursor = next; + + // Reaching the ceiling with a still-advancing cursor means truncation. + if (page === MAX_PAGES - 1) truncated = true; + } + + if (truncated) { + console.warn( + `listComments: comments for "${pageId}" truncated at the ${MAX_PAGES}-page cap; more pages exist on the server`, + ); + } const mapped = allComments.map((comment: any) => { const markdown = comment.content @@ -2842,36 +2943,27 @@ export class DocmostClient { ); } - // 1. Enumerate the FULL set of pages in scope by walking the sidebar-pages - // tree (a complete page index), NOT the bounded "/pages/recent" feed which - // caps at ~5000 recent items and silently misses comments on older pages. + // 1. Enumerate the FULL set of pages in scope via the page tree (a complete + // page index), NOT the bounded "/pages/recent" feed which caps at ~5000 + // recent items and silently misses comments on older pages. // // Subtree scope: when parentPageId is given, the scope is that page ITSELF - // plus every descendant (enumerateSpacePages walks its children). Otherwise - // the scope is the whole space (all roots and their descendants). + // plus every descendant. Otherwise the scope is the whole space (all roots + // and their descendants). // // NOTE: do NOT pre-filter by page.updatedAt — creating a comment does not // bump it (verified on a live server), so such a filter silently misses // comments on pages that were not otherwise edited. The complete tree walk // already restricts the scope correctly, so no recent-feed allow-list is // needed any more. - let pagesInScope: any[]; - if (parentPageId) { - const subtree = await this.enumerateSpacePages(spaceId, parentPageId); - // Include the parent page node itself alongside its descendants. Fetch it - // so its title/id are available even though it is not returned by its own - // children listing. - let parentNode: any = { id: parentPageId }; - try { - parentNode = await this.getPageRaw(parentPageId); - } catch (e: any) { - // Fall back to a minimal node if the parent can't be fetched; its - // comments are still attempted below (the fetch there is non-fatal). - } - pagesInScope = [parentNode, ...subtree]; - } else { - pagesInScope = await this.enumerateSpacePages(spaceId); - } + // + // 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, + ); // 2. Fetch comments for each page, keep ones created after since const results: any[] = []; @@ -2900,10 +2992,9 @@ export class DocmostClient { 0, ); - // enumerateSpacePages caps traversal at 10000 nodes; flag when that cap was - // hit so the caller knows the scan may be incomplete (some pages skipped). - const truncated = pagesInScope.length >= 10000; - + // `truncated` is reported by enumerateSpacePages: it is true ONLY when the + // stdio fallback BFS hit its node cap. The primary /pages/tree path is + // uncapped, so a space with legitimately many pages is not falsely flagged. return { since, scope: parentPageId ? `subtree of ${parentPageId}` : `space ${spaceId}`, diff --git a/packages/mcp/test/mock/pagination-cursor.test.mjs b/packages/mcp/test/mock/pagination-cursor.test.mjs new file mode 100644 index 00000000..9bb61c71 --- /dev/null +++ b/packages/mcp/test/mock/pagination-cursor.test.mjs @@ -0,0 +1,440 @@ +// Mock-HTTP tests for the cursor-pagination migration in DocmostClient (#442). +// +// The server switched its list endpoints from OFFSET (`page`) to CURSOR +// (`cursor`/`nextCursor`) pagination, and the global ValidationPipe silently +// strips the obsolete `page` field — so the old offset loops re-fetched page +// one forever (hasNextPage stuck true), dropping every item past the first +// page. These tests pin the new cursor behaviour and the immovable-cursor +// guard that prevents a silent spin/duplication if the protocol drifts again. +// +// A local http.createServer stands in for Docmost so everything stays +// deterministic and offline (same harness style as reauth.test.mjs). +import { test, after } from "node:test"; +import assert from "node:assert/strict"; +import http from "node:http"; +import { DocmostClient } from "../../build/client.js"; + +function readBody(req) { + return new Promise((resolve) => { + let raw = ""; + req.on("data", (chunk) => { + raw += chunk; + }); + 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 closeServer(server) { + return new Promise((resolve) => server.close(resolve)); +} + +function sendJson(res, status, obj, extraHeaders = {}) { + res.writeHead(status, { "Content-Type": "application/json", ...extraHeaders }); + res.end(JSON.stringify(obj)); +} + +const openServers = []; +async function spawn(handler) { + const { server, baseURL } = await startServer(handler); + openServers.push(server); + return { server, baseURL }; +} + +after(async () => { + await Promise.all(openServers.map((s) => closeServer(s))); +}); + +// A login handler shared by every server below. +function handleLogin(req, res) { + if (req.url === "/api/auth/login") { + sendJson(res, 200, { success: true }, { + "Set-Cookie": "authToken=t; Path=/; HttpOnly", + }); + return true; + } + return false; +} + +// ----------------------------------------------------------------------------- +// 1) listSidebarPages: collects every cursor page; #requests == #pages. +// ----------------------------------------------------------------------------- +test("listSidebarPages walks all cursor pages and collects every item", async () => { + // Three pages keyed by the cursor the client sends back. + const PAGES = { + "": { items: [{ id: "a" }, { id: "b" }], nextCursor: "c1" }, + c1: { items: [{ id: "c" }, { id: "d" }], nextCursor: "c2" }, + c2: { items: [{ id: "e" }], nextCursor: null }, + }; + let requests = 0; + const sentLimits = []; + + const { baseURL } = await spawn(async (req, res) => { + const raw = await readBody(req); + if (handleLogin(req, res)) return; + if (req.url === "/api/pages/sidebar-pages") { + requests++; + const body = JSON.parse(raw || "{}"); + sentLimits.push(body.limit); + const page = PAGES[body.cursor ?? ""] ?? { items: [], nextCursor: null }; + sendJson(res, 200, { + success: true, + data: { + items: page.items, + meta: { + hasNextPage: page.nextCursor != null, + nextCursor: page.nextCursor, + }, + }, + }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + const all = await client.listSidebarPages("space-1"); + + assert.equal(requests, 3, "one request per cursor page"); + assert.deepEqual( + all.map((p) => p.id), + ["a", "b", "c", "d", "e"], + "all items across all pages collected in order", + ); + assert.ok( + sentLimits.every((l) => l === 100), + "requests limit:100 (server-side max)", + ); +}); + +// ----------------------------------------------------------------------------- +// 2) REGRESSION on the bug class: server IGNORES the cursor param and always +// returns page one with hasNextPage:true -> the immovable-cursor guard must +// terminate the loop with no duplicates, NOT spin to MAX_PAGES. +// ----------------------------------------------------------------------------- +test("listSidebarPages terminates (no dups) when the server ignores the cursor", async () => { + let requests = 0; + + const { baseURL } = await spawn(async (req, res) => { + await readBody(req); + if (handleLogin(req, res)) return; + if (req.url === "/api/pages/sidebar-pages") { + requests++; + // Always the SAME first page with hasNextPage:true and the SAME cursor, + // exactly as a server that no longer understands our pagination param. + sendJson(res, 200, { + success: true, + data: { + items: [{ id: "x1" }, { id: "x2" }], + meta: { hasNextPage: true, nextCursor: "stuck" }, + }, + }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + const all = await client.listSidebarPages("space-1"); + + // Request 1 (no cursor) gets "stuck"; request 2 (cursor "stuck") gets "stuck" + // again -> guard trips. Far below the MAX_PAGES=50 ceiling; no runaway dups. + assert.equal(requests, 2, "stops as soon as the cursor stops moving"); + assert.equal(all.length, 4, "no runaway accumulation / duplication"); +}); + +// ----------------------------------------------------------------------------- +// 3a) enumerateSpacePages happy path: a SINGLE /pages/tree request. +// ----------------------------------------------------------------------------- +test("enumerateSpacePages (via list_pages tree) uses one /pages/tree request", async () => { + let treeRequests = 0; + let sidebarRequests = 0; + let treeBody = null; + + const NODES = [ + { id: "r1", slugId: "r1s", title: "Root 1", parentPageId: null, hasChildren: true, spaceId: "space-1", position: "a", icon: null, canEdit: true }, + { id: "c1", slugId: "c1s", title: "Child 1", parentPageId: "r1", hasChildren: false, spaceId: "space-1", position: "a", icon: null, canEdit: true }, + { id: "r2", slugId: "r2s", title: "Root 2", parentPageId: null, hasChildren: false, spaceId: "space-1", position: "b", icon: null, canEdit: true }, + ]; + + const { baseURL } = await spawn(async (req, res) => { + const raw = await readBody(req); + if (handleLogin(req, res)) return; + if (req.url === "/api/pages/tree") { + treeRequests++; + treeBody = JSON.parse(raw || "{}"); + sendJson(res, 200, { success: true, data: { items: NODES } }); + return; + } + if (req.url === "/api/pages/sidebar-pages") { + sidebarRequests++; + sendJson(res, 200, { success: true, data: { items: [], meta: {} } }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + // list_pages tree:true -> enumerateSpacePages(spaceId) -> buildPageTree. + const tree = await client.listPages("space-1", 50, true); + + assert.equal(treeRequests, 1, "exactly one /pages/tree request for the space"); + assert.equal(sidebarRequests, 0, "no per-node sidebar BFS requests"); + assert.deepEqual(treeBody, { spaceId: "space-1" }, "space scope posts spaceId only"); + // buildPageTree nests c1 under r1; two roots at the top level. + assert.equal(tree.length, 2, "two root nodes"); + const r1 = tree.find((n) => n.id === "r1"); + assert.equal(r1.children.length, 1, "child nested under its root"); + assert.equal(r1.children[0].id, "c1"); +}); + +// ----------------------------------------------------------------------------- +// 3b) enumerateSpacePages fallback: /pages/tree 404 -> cursor BFS via sidebar. +// ----------------------------------------------------------------------------- +test("enumerateSpacePages falls back to the cursor BFS on /pages/tree 404", async () => { + let treeRequests = 0; + const sidebarCalls = []; + + // Root level: one root with children. Child level (pageId=r1): one leaf. + const { baseURL } = await spawn(async (req, res) => { + const raw = await readBody(req); + if (handleLogin(req, res)) return; + if (req.url === "/api/pages/tree") { + treeRequests++; + // Stock upstream Docmost has no /pages/tree. + sendJson(res, 404, { message: "Not Found" }); + return; + } + if (req.url === "/api/pages/sidebar-pages") { + const body = JSON.parse(raw || "{}"); + sidebarCalls.push(body.pageId ?? ""); + if (!body.pageId) { + sendJson(res, 200, { + success: true, + data: { + items: [ + { id: "r1", title: "Root", parentPageId: null, hasChildren: true }, + ], + meta: { hasNextPage: false, nextCursor: null }, + }, + }); + } else if (body.pageId === "r1") { + sendJson(res, 200, { + success: true, + data: { + items: [ + { id: "c1", title: "Leaf", parentPageId: "r1", hasChildren: false }, + ], + meta: { hasNextPage: false, nextCursor: null }, + }, + }); + } else { + sendJson(res, 200, { + success: true, + data: { items: [], meta: { hasNextPage: false, nextCursor: null } }, + }); + } + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + const tree = await client.listPages("space-1", 50, true); + + assert.ok(treeRequests >= 1, "the tree endpoint was attempted first"); + assert.deepEqual( + sidebarCalls, + ["", "r1"], + "fell back to the sidebar BFS: roots then the root's children", + ); + assert.equal(tree.length, 1, "one root in the built tree"); + assert.equal(tree[0].children[0].id, "c1", "leaf nested via the BFS"); +}); + +// ----------------------------------------------------------------------------- +// 3c) enumerateSpacePages fallback SUBTREE: /pages/tree 404 + a rootPageId -> +// the ROOT page itself must be seeded (via getPageRaw) so its own comments +// aren't dropped. listSidebarPages(spaceId, root) returns only the root's +// CHILDREN, so without the seed the root would be absent. (Finding 1.) +// ----------------------------------------------------------------------------- +test("enumerateSpacePages fallback subtree seeds the ROOT page itself", async () => { + const sidebarCalls = []; + let infoRequests = 0; + const commentedPages = []; + + const { baseURL } = await spawn(async (req, res) => { + const raw = await readBody(req); + if (handleLogin(req, res)) return; + if (req.url === "/api/pages/tree") { + // Stock upstream Docmost -> fall back to the BFS. + sendJson(res, 404, { message: "Not Found" }); + return; + } + if (req.url === "/api/pages/info") { + // getPageRaw for the root seed. Shape mirrors a real page-info response. + infoRequests++; + sendJson(res, 200, { + success: true, + data: { id: "root", title: "Root", spaceId: "space-1", hasChildren: true }, + }); + return; + } + if (req.url === "/api/pages/sidebar-pages") { + const body = JSON.parse(raw || "{}"); + sidebarCalls.push(body.pageId ?? ""); + // Children of the root: one leaf. (Root itself is NOT in this list.) + const items = + body.pageId === "root" + ? [{ id: "leaf", title: "Leaf", parentPageId: "root", hasChildren: false }] + : []; + sendJson(res, 200, { + success: true, + data: { items, meta: { hasNextPage: false, nextCursor: null } }, + }); + return; + } + if (req.url === "/api/comments") { + const body = JSON.parse(raw || "{}"); + commentedPages.push(body.pageId); + const items = + body.pageId === "root" + ? [{ id: "cm1", createdAt: "2030-01-01T00:00:00.000Z", content: null }] + : []; + sendJson(res, 200, { + success: true, + data: { items, meta: { nextCursor: null } }, + }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + // checkNewComments(space, since, parentPageId) exercises the subtree fallback. + const result = await client.checkNewComments( + "space-1", + "2020-01-01T00:00:00.000Z", + "root", + ); + + assert.equal(infoRequests, 1, "root was seeded via one getPageRaw"); + assert.equal(sidebarCalls[0], "root", "BFS walked the root's children"); + assert.ok( + commentedPages.includes("root"), + "the ROOT page is in scope (its comments were fetched) — not dropped", + ); + assert.ok(commentedPages.includes("leaf"), "the descendant is in scope too"); + assert.equal(result.checkedPages, 2, "root + one descendant scanned"); + assert.equal(result.totalNewComments, 1, "the root's fresh comment found"); +}); + +// ----------------------------------------------------------------------------- +// 5) listComments immovable-cursor guard: the server IGNORES the cursor and +// keeps returning the same nextCursor -> the loop must terminate (no +// infinite loop, no duplicates), not spin forever. (Finding 4.) +// ----------------------------------------------------------------------------- +test("listComments terminates (no dups) when the server ignores the cursor", async () => { + let requests = 0; + + const { baseURL } = await spawn(async (req, res) => { + await readBody(req); + if (handleLogin(req, res)) return; + if (req.url === "/api/comments") { + requests++; + // Always the SAME page with the SAME nextCursor, as a server that no + // longer advances the cursor would. + sendJson(res, 200, { + success: true, + data: { + items: [{ id: "cm1", createdAt: "2030-01-01T00:00:00.000Z", content: null }], + meta: { nextCursor: "stuck" }, + }, + }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + const { items } = await client.listComments("page-1", true); + + // Request 1 (no cursor) gets "stuck"; request 2 (cursor "stuck") gets "stuck" + // again -> guard trips. Bounded far below MAX_PAGES=50, no runaway dups. + assert.equal(requests, 2, "stops as soon as the cursor stops moving"); + assert.equal(items.length, 2, "no runaway accumulation / duplication"); +}); + +// ----------------------------------------------------------------------------- +// 4) check_new_comments subtree: the root is included in scope WITHOUT a +// separate getPageRaw (/pages/info) request for the parent. +// ----------------------------------------------------------------------------- +test("checkNewComments subtree includes the root without a separate getPageRaw", async () => { + let pageInfoRequests = 0; + let treeBody = null; + const commentedPages = []; + + // /pages/tree (subtree) returns the parent itself plus a descendant, exactly + // as getPageAndDescendants seeds with id = parentPageId. + const NODES = [ + { id: "parent", title: "Parent", parentPageId: null, hasChildren: true }, + { id: "kid", title: "Kid", parentPageId: "parent", hasChildren: false }, + ]; + + const { baseURL } = await spawn(async (req, res) => { + const raw = await readBody(req); + if (handleLogin(req, res)) return; + if (req.url === "/api/pages/tree") { + treeBody = JSON.parse(raw || "{}"); + sendJson(res, 200, { success: true, data: { items: NODES } }); + return; + } + if (req.url === "/api/pages/info") { + // If checkNewComments still fetched the parent separately this would fire. + pageInfoRequests++; + sendJson(res, 200, { success: true, data: { id: "parent" } }); + return; + } + if (req.url === "/api/comments") { + const body = JSON.parse(raw || "{}"); + commentedPages.push(body.pageId); + // One fresh comment on the parent, none elsewhere. + const items = + body.pageId === "parent" + ? [{ id: "cm1", createdAt: "2030-01-01T00:00:00.000Z", content: null }] + : []; + sendJson(res, 200, { + success: true, + data: { items, meta: { nextCursor: null } }, + }); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + const result = await client.checkNewComments( + "space-1", + "2020-01-01T00:00:00.000Z", + "parent", + ); + + assert.equal(pageInfoRequests, 0, "no separate getPageRaw for the root"); + assert.deepEqual(treeBody, { pageId: "parent" }, "subtree scope posts pageId"); + assert.ok( + commentedPages.includes("parent"), + "the root itself is in scope (comments fetched for it)", + ); + assert.ok(commentedPages.includes("kid"), "descendants are in scope too"); + assert.equal(result.checkedPages, 2, "root + one descendant scanned"); + assert.equal(result.totalNewComments, 1, "the root's fresh comment found"); +}); diff --git a/packages/mcp/test/mock/reauth.test.mjs b/packages/mcp/test/mock/reauth.test.mjs index a0863ea4..12865b6f 100644 --- a/packages/mcp/test/mock/reauth.test.mjs +++ b/packages/mcp/test/mock/reauth.test.mjs @@ -297,12 +297,12 @@ test("a response with ONLY authTokenRefresh (no authToken) rejects login", async // ----------------------------------------------------------------------------- // 5) paginateAll loop guards. // ----------------------------------------------------------------------------- -test("paginateAll stops at the MAX_PAGES cap when hasNextPage is always true", async () => { +test("paginateAll stops at the MAX_PAGES cap when the server always issues a fresh cursor", async () => { let pageRequests = 0; const LIMIT = 100; const { baseURL } = await spawn(async (req, res) => { - await readBody(req); + const body = JSON.parse((await readBody(req)) || "{}"); if (req.url === "/api/auth/login") { sendJson(res, 200, { success: true }, { "Set-Cookie": "authToken=t; Path=/; HttpOnly", @@ -311,15 +311,18 @@ test("paginateAll stops at the MAX_PAGES cap when hasNextPage is always true", a } if (req.url === "/api/spaces") { pageRequests++; - // Always return a FULL page (== requested limit) AND hasNextPage:true. - // Both the page-length check and the hasNextPage flag say "keep going", - // so only the MAX_PAGES ceiling can stop the loop. + // Always return a FULL page AND hasNextPage:true with a FRESH nextCursor + // that differs from the one the client just sent, so the immovable-cursor + // guard never trips — only the MAX_PAGES ceiling can stop the loop. const items = Array.from({ length: LIMIT }, (_, i) => ({ id: `s-${pageRequests}-${i}`, })); sendJson(res, 200, { success: true, - data: { items, meta: { hasNextPage: true } }, + data: { + items, + meta: { hasNextPage: true, nextCursor: `cursor-${pageRequests}` }, + }, }); return; } @@ -338,7 +341,7 @@ test("paginateAll stops at the MAX_PAGES cap when hasNextPage is always true", a assert.equal(all.length, 50 * LIMIT, "accumulates one full page per request"); }); -test("paginateAll stops early on a short page even if hasNextPage is true", async () => { +test("paginateAll stops on the immovable-cursor guard when the server ignores the cursor param", async () => { let pageRequests = 0; const LIMIT = 100; @@ -352,15 +355,17 @@ test("paginateAll stops early on a short page even if hasNextPage is true", asyn } if (req.url === "/api/spaces") { pageRequests++; - // First page is full; second page is SHORT (fewer than limit). The short - // page must stop the loop immediately even though hasNextPage stays true. - const count = pageRequests === 1 ? LIMIT : 3; - const items = Array.from({ length: count }, (_, i) => ({ - id: `s-${pageRequests}-${i}`, - })); + // The bug class: the server IGNORES the pagination param and keeps + // returning page one with hasNextPage:true and the SAME nextCursor. The + // immovable-cursor guard must stop the loop instead of spinning to + // MAX_PAGES and duplicating items. + const items = Array.from({ length: LIMIT }, (_, i) => ({ id: `s-${i}` })); sendJson(res, 200, { success: true, - data: { items, meta: { hasNextPage: true } }, + data: { + items, + meta: { hasNextPage: true, nextCursor: "stuck" }, + }, }); return; } @@ -370,8 +375,10 @@ test("paginateAll stops early on a short page even if hasNextPage is true", asyn const client = new DocmostClient(baseURL, "user@example.com", "pw"); const all = await client.paginateAll("/spaces", {}, LIMIT); - assert.equal(pageRequests, 2, "stops right after the first short page"); - assert.equal(all.length, LIMIT + 3, "full page + short page accumulated"); + // Request 1 sends no cursor and receives "stuck"; request 2 sends "stuck" and + // receives "stuck" again -> guard trips after exactly two requests, no dups. + assert.equal(pageRequests, 2, "stops once the cursor stops moving"); + assert.equal(all.length, 2 * LIMIT, "no runaway accumulation past the guard"); }); test("paginateAll handles both {data:{items,meta}} and {items,meta} envelopes", async () => { @@ -387,16 +394,16 @@ test("paginateAll handles both {data:{items,meta}} and {items,meta} envelopes", } if (req.url === "/api/groups") { bareRequests.push(1); - // Page 1: full page, hasNextPage true. Page 2: short page -> stop. + // Page 1: hasNextPage true with a next cursor. Page 2: no next -> stop. if (bareRequests.length === 1) { sendJson(res, 200, { items: Array.from({ length: 100 }, (_, i) => ({ id: `g${i}` })), - meta: { hasNextPage: true }, + meta: { hasNextPage: true, nextCursor: "c2" }, }); } else { sendJson(res, 200, { items: [{ id: "tail" }], - meta: { hasNextPage: false }, + meta: { hasNextPage: false, nextCursor: null }, }); } return;