diff --git a/.changeset/agent-sdk-max-items-row-loss.md b/.changeset/agent-sdk-max-items-row-loss.md new file mode 100644 index 00000000..3146bc77 --- /dev/null +++ b/.changeset/agent-sdk-max-items-row-loss.md @@ -0,0 +1,8 @@ +--- +"@rozenite/agent-sdk": patch +"@rozenite/agent-shared": minor +--- + +Fix `autoPaginate.maxItems` silently dropping rows and reporting a `nextCursor` that skipped past them. The auto-pagination helper now requests exactly the rows still needed on every call (including the first), so a well-behaved tool's own cursor stays correct by construction instead of pointing past rows the caller never received. + +For tools that clamp or ignore the requested `limit` and still return more rows than asked, the extra rows are trimmed and the page's `nextCursor` is dropped rather than propagated (it can no longer be trusted to resume correctly). This is now signalled explicitly via a new `truncated: true` field on `PageEnvelope` (`@rozenite/agent-shared`), paired with `hasMore: true`, meaning "rows were dropped; resume position is unknown - restart pagination or narrow the query." diff --git a/packages/agent-sdk/src/__tests__/pagination.test.ts b/packages/agent-sdk/src/__tests__/pagination.test.ts index 971e299a..a08d9eb2 100644 --- a/packages/agent-sdk/src/__tests__/pagination.test.ts +++ b/packages/agent-sdk/src/__tests__/pagination.test.ts @@ -1,6 +1,50 @@ import { describe, expect, it, vi } from 'vitest'; import { callToolWithOptionalPagination } from '../pagination.js'; +// Mirrors the real cursor encoding used by production paginated tools +// (e.g. packages/middleware/src/agent/local-domains.ts `paginateRows`): +// base64url JSON of { v: 1, scope, index }. +const encodeCursor = (scope: string, index: number): string => { + return Buffer.from( + JSON.stringify({ v: 1, scope, index }), + 'utf8', + ).toString('base64url'); +}; + +const decodeCursorIndex = (cursor: string): number => { + const decoded = JSON.parse( + Buffer.from(cursor, 'base64url').toString('utf8'), + ) as { index: number }; + return decoded.index; +}; + +// A well-behaved paginated tool: honors whatever `limit` it's asked for on +// each call, and encodes `nextCursor` as the exact index to resume from. +// Used to prove the row-loss is caused by the SDK's own request/merge +// logic, not by a misbehaving producer. +const createWellBehavedPaginatedTool = ( + scope: string, + totalRows: number, +) => { + const rows = Array.from({ length: totalRows }, (_, i) => ({ id: i })); + return vi.fn().mockImplementation((_name: string, args: unknown) => { + const record = (args ?? {}) as { limit?: number; cursor?: string }; + const limit = record.limit ?? totalRows; + const startIndex = record.cursor ? decodeCursorIndex(record.cursor) : 0; + const endIndex = Math.min(startIndex + limit, rows.length); + const items = rows.slice(startIndex, endIndex); + const hasMore = endIndex < rows.length; + return Promise.resolve({ + items, + page: { + limit, + hasMore, + nextCursor: hasMore ? encodeCursor(scope, endIndex) : undefined, + }, + }); + }); +}; + describe('agent tool pagination helper', () => { it('returns raw result when auto-pagination is not requested', async () => { const client = { @@ -78,6 +122,11 @@ describe('agent tool pagination helper', () => { }); it('caps an oversized first page without fetching another page', async () => { + // This producer ignores the requested `limit` (it always returns 3 + // items no matter what's asked) - the guaranteed-correct fallback path. + // Since it over-returns relative to what was requested (2, driven by + // maxItems), its `nextCursor: 'c1'` cannot be trusted to resume after + // the item we discard, so it must be dropped and `truncated` reported. const client = { callTool: vi.fn().mockResolvedValue({ items: [{ id: 1 }, { id: 2 }, { id: 3 }], @@ -92,16 +141,27 @@ describe('agent tool pagination helper', () => { { pagesLimit: 2, maxItems: 2 }, )) as { items: Array<{ id: number }>; - page: { hasMore: boolean; nextCursor?: string; limit: number }; + page: { + hasMore: boolean; + nextCursor?: string; + limit: number; + truncated?: boolean; + }; }; expect(result.items.map((item) => item.id)).toEqual([1, 2]); expect(result.page).toEqual({ limit: 3, hasMore: true, - nextCursor: 'c1', + nextCursor: undefined, + truncated: true, }); expect(client.callTool).toHaveBeenCalledTimes(1); + // The SDK asked for exactly the remaining budget (min(callerLimit=3, + // maxItems=2) = 2), not the caller's full page size of 3. + expect(client.callTool).toHaveBeenCalledWith('listRequests', { + limit: 2, + }); }); it('stops merging if a subsequent page is not paged', async () => { @@ -202,6 +262,93 @@ describe('agent tool pagination helper', () => { expect(client.callTool).toHaveBeenCalledTimes(2); }); + it('BUG REPRO: drops rows on the initial page because the SDK requests a full page instead of maxItems', async () => { + // A well-behaved 20-row producer. The caller asks for pages of 20 with + // maxItems: 5. The un-fixed SDK forwards `limit: 20` unmodified on the + // very first call, receives the ENTIRE 20-row dataset in one page + // (hasMore: false - there is nothing left upstream), then trims the + // response down to 5 items while keeping that page's `hasMore: false`. + // Rows 5-19 are real, were served by the producer, and are now + // unreachable: there's no cursor (hasMore is false) to go get them. + const tool = createWellBehavedPaginatedTool('initial-repro', 20); + const client = { callTool: tool }; + + const result = (await callToolWithOptionalPagination( + client, + 'listRequests', + { limit: 20 }, + { pagesLimit: 1, maxItems: 5 }, + )) as { + items: Array<{ id: number }>; + page: { hasMore: boolean; nextCursor?: string; limit: number }; + }; + + const receivedIds = result.items.map((item) => item.id); + const reachableViaCursor: number[] = []; + if (result.page.nextCursor) { + for ( + let i = decodeCursorIndex(result.page.nextCursor); + i < 20; + i++ + ) { + reachableViaCursor.push(i); + } + } else if (result.page.hasMore) { + // hasMore is true but there's no cursor to act on - still unreachable. + } + const union = new Set([...receivedIds, ...reachableViaCursor]); + + const lostRows = Array.from({ length: 20 }, (_, i) => i).filter( + (id) => !union.has(id), + ); + // Rows 5-19 were served by the producer but are absent from both what + // the caller received AND what's reachable via the reported cursor. + expect(lostRows).toEqual([]); + // The reported state must not falsely claim the dataset is exhausted. + expect(result.page.hasMore).toBe(true); + }); + + it('BUG REPRO: drops rows mid-loop because subsequent requests reuse the caller limit instead of the remaining budget', async () => { + // A well-behaved 20-row producer. Caller asks for pages of 5 with + // maxItems: 7 over up to 2 pages. Page 1 delivers rows 0-4 (5 items, + // remaining budget now 2). The un-fixed SDK's second call still asks + // for `limit: 5` (the caller's original page size) instead of the + // remaining budget of 2, so the producer legitimately returns rows + // 5-9 with a valid nextCursor pointing at row 10. The SDK keeps only + // 2 of those 5 rows (5, 6) to respect maxItems, discards rows 7-9, but + // still reports the producer's nextCursor - which skips straight over + // rows 7-9 to row 10. They are gone even though hasMore correctly + // (but misleadingly, given the gap) says there's more. + const tool = createWellBehavedPaginatedTool('mid-loop-repro', 20); + const client = { callTool: tool }; + + const result = (await callToolWithOptionalPagination( + client, + 'listRequests', + { limit: 5 }, + { pagesLimit: 2, maxItems: 7 }, + )) as { + items: Array<{ id: number }>; + page: { hasMore: boolean; nextCursor?: string; limit: number }; + }; + + const receivedIds = result.items.map((item) => item.id); + const reachableFromCursor = result.page.nextCursor + ? decodeCursorIndex(result.page.nextCursor) + : undefined; + + // Rows 7, 8, 9 were served by the producer, are not in the caller's + // result, and are skipped over by the reported nextCursor (which jumps + // straight to row 10): silent data loss with a misleading resume point. + const droppedButSkippedOver = [7, 8, 9].filter( + (id) => + !receivedIds.includes(id) && + reachableFromCursor !== undefined && + id < reachableFromCursor, + ); + expect(droppedButSkippedOver).toEqual([]); + }); + it('requires pagesLimit when maxItems is used', async () => { const client = { callTool: vi.fn(), diff --git a/packages/agent-sdk/src/pagination.ts b/packages/agent-sdk/src/pagination.ts index 11dd39a5..e3069938 100644 --- a/packages/agent-sdk/src/pagination.ts +++ b/packages/agent-sdk/src/pagination.ts @@ -1,3 +1,4 @@ +import type { PageEnvelope } from '@rozenite/agent-shared'; import type { AgentCallToolAutoPaginationOptions } from './types.js'; type ToolCaller = { @@ -6,12 +7,7 @@ type ToolCaller = { type PagedResponse = { items: unknown[]; - page: { - limit: number; - hasMore: boolean; - nextCursor?: string; - reset?: boolean; - }; + page: PageEnvelope; [key: string]: unknown; }; @@ -74,6 +70,73 @@ const getTrimmedItems = ( return maxItems === undefined ? [...items] : items.slice(0, maxItems); }; +// `limit` is a universal convention across every paginated agent tool (see +// packages/middleware/src/agent/local-domains.ts and the plugin +// packages/*/src/shared/agent-tools.ts contracts), so it's safe for the SDK +// to read and rewrite it when driving auto-pagination. +const getCallerLimit = (args: Record): number | undefined => { + return isPositiveInteger(args.limit) ? args.limit : undefined; +}; + +// The request limit for a call that must not fetch more than `remaining` +// rows. Respects a caller-supplied `limit` as a ceiling when present; +// otherwise requests exactly what's still needed. +const getRequestLimit = ( + callerLimit: number | undefined, + remaining: number, +): number => { + return callerLimit === undefined ? remaining : Math.min(callerLimit, remaining); +}; + +/** + * Guaranteed-correct fallback for producers that clamp or ignore the + * requested `limit` (e.g. a tool that declares "Page size. Default 50, max + * 200" and returns its own default regardless of what's asked). If a + * response comes back with MORE rows than were requested, the rows beyond + * `requestedLimit` must be trimmed - but the producer's own `nextCursor` + * was computed assuming the caller received the whole page it sent, so it + * cannot be trusted to resume after the point we're truncating at. + * + * We only need to intervene when trimming would otherwise silently discard + * a resumable position: if the untouched response has no cursor to lose + * (hasMore is false / nextCursor is absent), trimming to the remaining + * budget is the same "reached maxItems, stop here" behavior this helper + * already performs elsewhere, and is left alone. + */ +const applyRequestBudget = ( + response: PagedResponse, + requestedLimit: number | undefined, +): PagedResponse => { + if ( + requestedLimit === undefined || + response.items.length <= requestedLimit + ) { + return response; + } + + const trimmedItems = response.items.slice(0, requestedLimit); + const hadResumablePosition = + response.page.hasMore || response.page.nextCursor !== undefined; + + if (!hadResumablePosition) { + return { ...response, items: trimmedItems }; + } + + return { + ...response, + items: trimmedItems, + page: { + ...response.page, + // We positively know unconsumed rows exist: the ones we just + // trimmed off. `hasMore` must reflect that even though we can no + // longer say where to resume. + hasMore: true, + nextCursor: undefined, + truncated: true, + }, + }; +}; + export const callToolWithOptionalPagination = async ( client: ToolCaller, toolName: string, @@ -82,22 +145,40 @@ export const callToolWithOptionalPagination = async ( ): Promise => { validateAutoPagination(config); - const initial = await client.callTool(toolName, args); const shouldAutoPaginate = config.pagesLimit !== undefined || config.maxItems !== undefined; + const pagesLimit = config.pagesLimit ?? 1; + const maxItems = config.maxItems; + + const baseArgs = normalizeArgs(args); + const callerLimit = getCallerLimit(baseArgs); + + // Never over-fetch: request exactly the rows still needed so the page + // boundary coincides with the maxItems boundary, and the producer's own + // nextCursor stays correct by construction. This applies to the initial + // call too - not just subsequent ones. + const initialRequestLimit = + shouldAutoPaginate && maxItems !== undefined + ? getRequestLimit(callerLimit, maxItems) + : undefined; + const initialArgs = + initialRequestLimit === undefined + ? args + : { ...baseArgs, limit: initialRequestLimit }; + + const initial = await client.callTool(toolName, initialArgs); if (!shouldAutoPaginate || !isPagedResponse(initial)) { return initial; } - const pagesLimit = config.pagesLimit ?? 1; - const maxItems = config.maxItems; + const initialAdjusted = applyRequestBudget(initial, initialRequestLimit); let pageCount = 1; - let cursor = initial.page.nextCursor; + let cursor = initialAdjusted.page.nextCursor; const merged: PagedResponse = { - ...initial, - items: getTrimmedItems(initial.items, maxItems), - page: { ...initial.page }, + ...initialAdjusted, + items: getTrimmedItems(initialAdjusted.items, maxItems), + page: { ...initialAdjusted.page }, }; while ( @@ -106,20 +187,30 @@ export const callToolWithOptionalPagination = async ( pageCount < pagesLimit && (maxItems === undefined || merged.items.length < maxItems) ) { + const remainingBudget = + maxItems === undefined + ? undefined + : Math.max(0, maxItems - merged.items.length); + const requestLimit = + remainingBudget === undefined + ? undefined + : getRequestLimit(callerLimit, remainingBudget); + const nextArgs = { - ...normalizeArgs(args), + ...baseArgs, cursor, + ...(requestLimit === undefined ? {} : { limit: requestLimit }), }; - const next = await client.callTool(toolName, nextArgs); - if (!isPagedResponse(next)) { + const rawNext = await client.callTool(toolName, nextArgs); + if (!isPagedResponse(rawNext)) { break; } - if (next.page.reset) { - merged.items = getTrimmedItems(next.items, maxItems); - merged.page = { ...next.page }; - cursor = next.page.nextCursor; + if (rawNext.page.reset) { + merged.items = getTrimmedItems(rawNext.items, maxItems); + merged.page = { ...rawNext.page }; + cursor = rawNext.page.nextCursor; pageCount += 1; if (maxItems !== undefined && merged.items.length >= maxItems) { @@ -129,6 +220,8 @@ export const callToolWithOptionalPagination = async ( continue; } + const next = applyRequestBudget(rawNext, requestLimit); + const remaining = maxItems === undefined ? next.items.length @@ -144,6 +237,8 @@ export const callToolWithOptionalPagination = async ( } } + // The reported `limit` reflects the producer's page size for the very + // first raw response, never our internally-reduced per-request limit. merged.page.limit = initial.page.limit; return merged; }; diff --git a/packages/agent-shared/src/index.ts b/packages/agent-shared/src/index.ts index d985c977..70e78fc3 100644 --- a/packages/agent-shared/src/index.ts +++ b/packages/agent-shared/src/index.ts @@ -162,6 +162,27 @@ export type AgentErrorInfo = { message: string; }; +/** + * Envelope describing pagination state for a cursor-paginated agent tool + * result. `limit` and `hasMore` are always required; `nextCursor` is an + * opaque, producer-owned token (its encoding is not part of this contract + * and must never be constructed by a consumer). `reset` signals that the + * underlying dataset was reshuffled and pagination should restart from + * these items. `truncated` signals that a consumer (e.g. an auto-pagination + * helper) discarded trailing rows from this page because the producer + * returned more rows than were requested; when `truncated` is `true`, + * `nextCursor` MUST be absent because the discarded rows make the + * producer's own cursor unsafe to resume from - callers should restart or + * narrow their query instead of trusting a stale cursor. + */ +export type PageEnvelope = { + limit: number; + hasMore: boolean; + nextCursor?: string; + reset?: boolean; + truncated?: boolean; +}; + export type AgentSuccessEnvelope = { ok: true; result: TResult;