mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-28 14:59:47 +00:00
list_pages clamps limit to max 100 (default 50) — deliberate server protection, pinned in test/search-limit.test.ts. But the clamp was SILENT: a caller whose limit was defaulted or clamped got a full-looking array with no signal that rows were dropped, and with the default updated_desc sort the dropped rows are always the OLDEST — precisely what exhaustive consumers (audits, scans, backfills) exist to find. Observed in the field: a source with 212 pages enumerated as 80 visible rows, hiding 26 pages from a compliance scan for days. Fix, with no response-shape change (MCP consumers still get an array) and no engine surface change (handler probes limit+1): - handler probes one row past the effective limit; when the caller's limit was NOT honored (unset -> default, or clamped to cap) and rows were dropped, it warns on stderr for local (CLI) callers — same operator-facing channel as the put_page unknown-type hint, but without the isTTY gate: scripted callers are exactly the consumers that cannot detect truncation any other way, and stderr keeps stdout parseable. An explicit honored limit stays silent (ordinary pagination), as does a clamped-but-complete result. Remote (MCP) ctx never writes to stderr. - LIST_PAGES_DESCRIPTION documents the cap and the exhaustive-listing recipe (sort=updated_asc + updated_after cursor) — the description is the signal channel MCP clients actually read. - regression suite: default-limit truncation warns, honored limit silent, clamped-but-complete silent, remote silent, and the documented cursor recipe enumerates a corpus to completion. Co-authored-by: paul-0320 <paul@ymyd.co.kr> Co-authored-by: YMYD <53603073+OJ-OnJourney@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Garry Tan <garrytan@gmail.com>
This commit is contained in:
co-authored by
paul-0320
YMYD
Claude Fable 5
Garry Tan
parent
32d42454e9
commit
c44cdb52b1
@@ -58,7 +58,11 @@ export const GET_RECENT_TRANSCRIPTS_DESCRIPTION =
|
||||
export const LIST_PAGES_DESCRIPTION =
|
||||
"List pages with optional filters. " +
|
||||
"For 'what's recent / what did I touch this week' questions, use list_pages " +
|
||||
"with sort=updated_desc instead of semantic search.";
|
||||
"with sort=updated_desc instead of semantic search. " +
|
||||
"Default 50 rows; remote callers are capped at 100 (local CLI callers' explicit " +
|
||||
"limits are honored). A result with exactly `limit` rows may be truncated. " +
|
||||
"For exhaustive listing, page with sort=updated_asc + " +
|
||||
"updated_after=<last row's updated_at> until a page returns fewer rows than the limit.";
|
||||
|
||||
export const QUERY_DESCRIPTION =
|
||||
"Hybrid search with vector + keyword + multi-query expansion. " +
|
||||
|
||||
+25
-2
@@ -1542,16 +1542,39 @@ const list_pages: Operation = {
|
||||
requestedOffset !== undefined && Number.isFinite(requestedOffset) && requestedOffset > 0
|
||||
? Math.floor(requestedOffset)
|
||||
: undefined;
|
||||
const pages = await ctx.engine.listPages({
|
||||
// Probe one row past the effective limit so truncation is detectable
|
||||
// without a COUNT query. The bug class sealed here is SILENT truncation
|
||||
// — an exhaustive consumer (audit, scan, backfill) gets a full-looking
|
||||
// list and never learns rows were dropped, and with the default
|
||||
// updated_desc sort the dropped rows are always the OLDEST, i.e. exactly
|
||||
// the pages such consumers exist to find.
|
||||
const rows = await ctx.engine.listPages({
|
||||
type: p.type as any,
|
||||
tag: p.tag as string,
|
||||
limit,
|
||||
limit: limit + 1,
|
||||
offset,
|
||||
includeDeleted: (p.include_deleted as boolean) === true,
|
||||
updated_after: typeof p.updated_after === 'string' ? p.updated_after : undefined,
|
||||
sort,
|
||||
...scope,
|
||||
});
|
||||
const truncated = rows.length > limit;
|
||||
const pages = truncated ? rows.slice(0, limit) : rows;
|
||||
// Warn only when the caller's limit was NOT honored (unset → default 50):
|
||||
// an explicit honored limit that happens to land on more rows is ordinary
|
||||
// pagination, not a trap. Local (CLI) only — same operator-facing stderr
|
||||
// channel as the put_page unknown-type hint above — but with no isTTY
|
||||
// gate: scripted callers are precisely the consumers that cannot detect
|
||||
// truncation any other way, and stderr keeps stdout parseable for them.
|
||||
// (Local explicit limits are honored unbounded since #3322, so the
|
||||
// requestedLimit > limit arm is defense in depth only.)
|
||||
if (truncated && isLocal && (requestedLimit === undefined || requestedLimit > limit)) {
|
||||
console.error(
|
||||
`[list_pages] output truncated at ${limit} rows (default 50). ` +
|
||||
`Pass an explicit limit, page through with sort=updated_asc + ` +
|
||||
`updated_after=<last row's updated_at>, or narrow with type/tag.`,
|
||||
);
|
||||
}
|
||||
return pages.map(pg => ({
|
||||
slug: pg.slug,
|
||||
source_id: pg.source_id,
|
||||
|
||||
@@ -0,0 +1,155 @@
|
||||
/**
|
||||
* list_pages silent-truncation seal.
|
||||
*
|
||||
* The op defaults limit to 50 and clamps remote callers to max 100 (local
|
||||
* explicit limits are honored since #3322 — pinned in
|
||||
* test/list-clamp-local-trust.test.ts). Pre-fix, a caller whose limit was
|
||||
* defaulted (or remotely clamped) received a full-looking array with NO
|
||||
* signal that rows were dropped, and with the default updated_desc sort the
|
||||
* dropped rows are always the OLDEST — precisely what exhaustive consumers
|
||||
* (audits, scans, backfills) exist to find.
|
||||
*
|
||||
* Covers, at the op-handler layer (engine listPages surface unchanged —
|
||||
* the handler only probes limit+1):
|
||||
* - default-limit truncation returns exactly 50 rows and warns on stderr
|
||||
* (local ctx only)
|
||||
* - an explicit, honored limit does NOT warn (ordinary pagination)
|
||||
* - a clamped-but-complete result (requested > cap, rows ≤ cap) does NOT
|
||||
* warn — nothing was dropped
|
||||
* - remote ctx never writes to stderr (MCP server logs stay clean)
|
||||
* - the pagination recipe in LIST_PAGES_DESCRIPTION (sort=updated_asc +
|
||||
* updated_after cursor) actually enumerates every row to completion
|
||||
*
|
||||
* Runs against PGLite in-memory (both engines share the SQL surface; the
|
||||
* handler change touches no engine code).
|
||||
*/
|
||||
|
||||
import { describe, test, expect, beforeAll, afterAll, beforeEach, spyOn } from 'bun:test';
|
||||
import { PGLiteEngine } from '../src/core/pglite-engine.ts';
|
||||
import { resetPgliteState } from './helpers/reset-pglite.ts';
|
||||
import { operations, type OperationContext } from '../src/core/operations.ts';
|
||||
|
||||
const list_pages = operations.find(o => o.name === 'list_pages')!;
|
||||
|
||||
let engine: PGLiteEngine;
|
||||
|
||||
beforeAll(async () => {
|
||||
engine = new PGLiteEngine();
|
||||
await engine.connect({});
|
||||
await engine.initSchema();
|
||||
});
|
||||
afterAll(async () => { await engine.disconnect(); });
|
||||
beforeEach(async () => { await resetPgliteState(engine); });
|
||||
|
||||
function ctxOf(overrides: Partial<OperationContext> = {}): OperationContext {
|
||||
return {
|
||||
engine: engine as any,
|
||||
config: {} as any,
|
||||
logger: console as any,
|
||||
dryRun: false,
|
||||
remote: false,
|
||||
sourceId: 'default',
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
const page = (n: number) => ({
|
||||
type: 'note' as const,
|
||||
title: `Note ${String(n).padStart(3, '0')}`,
|
||||
compiled_truth: `Body of note ${n}.`,
|
||||
timeline: '',
|
||||
frontmatter: {},
|
||||
});
|
||||
|
||||
async function seed(count: number) {
|
||||
for (let i = 1; i <= count; i++) {
|
||||
await engine.putPage(`notes/note-${String(i).padStart(3, '0')}`, page(i), { sourceId: 'default' });
|
||||
}
|
||||
}
|
||||
|
||||
/** Run the handler while capturing stderr writes made through console.error. */
|
||||
async function runCapturing(ctx: OperationContext, params: Record<string, unknown>) {
|
||||
const spy = spyOn(console, 'error').mockImplementation(() => {});
|
||||
try {
|
||||
const result = await list_pages.handler(ctx, params) as any[];
|
||||
const warnings = spy.mock.calls.map(args => args.join(' ')).filter(s => s.includes('[list_pages]'));
|
||||
return { result, warnings };
|
||||
} finally {
|
||||
spy.mockRestore();
|
||||
}
|
||||
}
|
||||
|
||||
describe('list_pages truncation signal', () => {
|
||||
test('default limit: 51 rows → exactly 50 returned + stderr warning', async () => {
|
||||
await seed(51);
|
||||
const { result, warnings } = await runCapturing(ctxOf(), {});
|
||||
expect(result.length).toBe(50);
|
||||
expect(warnings.length).toBe(1);
|
||||
expect(warnings[0]).toContain('truncated at 50 rows');
|
||||
expect(warnings[0]).toContain('sort=updated_asc');
|
||||
}, 30_000);
|
||||
|
||||
test('explicit honored limit: no warning even when more rows exist', async () => {
|
||||
await seed(12);
|
||||
const { result, warnings } = await runCapturing(ctxOf(), { limit: 10 });
|
||||
expect(result.length).toBe(10);
|
||||
expect(warnings.length).toBe(0);
|
||||
}, 30_000);
|
||||
|
||||
test('clamped but complete: requested > cap with rows ≤ cap → all rows, no warning', async () => {
|
||||
await seed(12);
|
||||
const { result, warnings } = await runCapturing(ctxOf(), { limit: 200 });
|
||||
expect(result.length).toBe(12);
|
||||
expect(warnings.length).toBe(0);
|
||||
}, 30_000);
|
||||
|
||||
test('local explicit limit above 100 is honored (#3322) — all rows, no warning', async () => {
|
||||
await seed(101);
|
||||
const { result, warnings } = await runCapturing(ctxOf(), { limit: 200 });
|
||||
expect(result.length).toBe(101);
|
||||
expect(warnings.length).toBe(0);
|
||||
}, 60_000);
|
||||
|
||||
test('remote requested above cap: clamped to 100, truncation stays off stderr', async () => {
|
||||
await seed(101);
|
||||
const { result, warnings } = await runCapturing(ctxOf({ remote: true }), { limit: 200 });
|
||||
expect(result.length).toBe(100);
|
||||
// The #3322 clamp warning goes through ctx.logger.warn; the [list_pages]
|
||||
// truncation hint is local-only, so console.error stays clean here.
|
||||
expect(warnings.length).toBe(0);
|
||||
}, 60_000);
|
||||
|
||||
test('remote ctx: truncation stays silent on stderr (MCP logs clean)', async () => {
|
||||
await seed(51);
|
||||
const { result, warnings } = await runCapturing(ctxOf({ remote: true }), {});
|
||||
expect(result.length).toBe(50);
|
||||
expect(warnings.length).toBe(0);
|
||||
}, 30_000);
|
||||
|
||||
test('documented cursor recipe enumerates all rows to completion', async () => {
|
||||
await seed(23);
|
||||
// Spread updated_at deterministically: back-to-back putPage calls can land
|
||||
// on identical timestamps, and a strict `updated_at > cursor` walk would
|
||||
// then skip the tied rows — that would be a flake in THIS test, not a
|
||||
// property of the recipe (real corpora update over time).
|
||||
await engine.executeRaw(
|
||||
`UPDATE pages SET updated_at = now() - (interval '1 minute' * (100 - id)) WHERE slug LIKE 'notes/note-%'`,
|
||||
);
|
||||
const seen = new Set<string>();
|
||||
let cursor: string | undefined;
|
||||
// sort=updated_asc + updated_after=<last row's updated_at>, stop when a
|
||||
// page returns fewer rows than the limit — verbatim the recipe in
|
||||
// LIST_PAGES_DESCRIPTION.
|
||||
for (let guard = 0; guard < 10; guard++) {
|
||||
const params: Record<string, unknown> = { limit: 10, sort: 'updated_asc' };
|
||||
if (cursor !== undefined) params.updated_after = cursor;
|
||||
const { result } = await runCapturing(ctxOf(), params);
|
||||
for (const row of result) seen.add(row.slug);
|
||||
if (result.length < 10) break;
|
||||
cursor = result[result.length - 1].updated_at instanceof Date
|
||||
? result[result.length - 1].updated_at.toISOString()
|
||||
: String(result[result.length - 1].updated_at);
|
||||
}
|
||||
expect(seen.size).toBe(23);
|
||||
}, 30_000);
|
||||
});
|
||||
Reference in New Issue
Block a user