History

Paginate tools.search and tools.executor.sources.list (#492)

PR #265 reported a real symptom — `tools.executor.sources.list()` silently returns the first 12 sources, so a model that calls it past alphabetical position 12 mistakes the page for the full inventory. The proposed fix returned `{ sources, totalSources }` from `sources.list` only, which: - Tells the model truncation happened, but provides no way to act on it — no offset/cursor parameter exists, so the model can only retry with `limit: 999` (defeating pagination). - Leaves `tools.search` untouched — same truncation hazard, same model. - Introduces an inconsistent shape: `search` returns a bare array, `sources.list` returns an object. This change paginates both with a single shared shape: { items, total, hasMore, nextOffset } Both helpers accept `{ limit?, offset? }`. `nextOffset` is concrete (the exact number to pass next) so the model doesn't have to compute `offset + items.length`. Engine arg validation rejects negative offsets. The `tools.search` description now teaches the paginated shape and shows the second-page call. Default `limit` for `sources.list` drops from 200 to 50; the engine layer still applies 12 by default for both. The off-by-one ergonomics matter more than the headroom — most users have well under 50 sources. Tests cover: - single-page and empty-query results carry the new shape - pagination across pages with limit + offset, including offset past end - source listing pagination via the engine - negative offsets rejected with a clear message Closes #265.

Rhys Sullivan committed 6 months ago

Commit c627c481 parent 99df4b24 files · +277 −50Browse files

Unified · Split

4 files
  1. packages/core/execution/src/description.ts +4 −3
  2. packages/core/execution/src/engine.ts +31 −2
  3. packages/core/execution/src/tool-invoker.test.ts +162 −18
  4. packages/core/execution/src/tool-invoker.ts +80 −27
packages/core/execution/src/description.ts+4−3
@@ -34,7 +34,7 @@ const formatDescription = (sources: readonly Source[]): string => { · 33 unmodified lines
3434 "",
3535 "## Workflow",
3636 "",
37− '1. `const matches = await tools.search({ query: "<intent + key nouns>", limit: 12 });`',
37+ '1. `const { items: matches } = await tools.search({ query: "<intent + key nouns>", limit: 12 });`',
3838 '2. `const path = matches[0]?.path; if (!path) return "No matching tools found.";`',
3939 "3. `const details = await tools.describe.tool({ path });`",
4040 "4. Use `details.inputTypeScript` / `details.outputTypeScript` and `details.typeScriptDefinitions` for compact shapes.",
@@ -43,9 +43,10 @@ const formatDescription = (sources: readonly Source[]): string => { · 2 unmodified lines
4343 "",
4444 "## Rules",
4545 "",
46− "- `tools.search()` returns ranked matches, best-first. Use short intent phrases like `github issues`, `repo details`, or `create calendar event`.",
46+ "- `tools.search()` returns paginated, ranked matches: `{ items, total, hasMore, nextOffset }`. Best-first. Use short intent phrases like `github issues`, `repo details`, or `create calendar event`.",
4747 '- When you already know the namespace, narrow with `tools.search({ namespace: "github", query: "issues" })`.',
48− "- Use `tools.executor.sources.list()` to inspect configured sources and their tool counts. Returns `[{ id, toolCount, ... }]`.",
48+ "- `tools.executor.sources.list()` returns the same paged shape: `{ items: [{ id, toolCount, ... }], total, hasMore, nextOffset }`.",
49+ "- If `hasMore` is true and you didn't find what you need, fetch the next page: `tools.search({ query, offset: nextOffset, limit })`. Same `offset` parameter on `tools.executor.sources.list({ offset, limit })`.",
4950 "- Always use the namespace prefix when calling tools: `tools.<namespace>.<tool>(args)`. Example: `tools.home_assistant_rest_api.states.getState(...)` — not `tools.states.getState(...)`.",
5051 "- The `tools` object is a lazy proxy — `Object.keys(tools)` won't work. Use `tools.search()` or `tools.executor.sources.list()` instead.",
5152 '- Pass an object to system tools, e.g. `tools.search({ query: "..." })`, `tools.executor.sources.list()`, and `tools.describe.tool({ path })`.',
packages/core/execution/src/engine.ts+31−2
@@ -157,6 +157,20 @@ const readOptionalLimit = (value: unknown, toolName: string): number | Execution · 156 unmodified lines
157157 return Math.floor(value);
158158 };
159159
160+const readOptionalOffset = (value: unknown, toolName: string): number | ExecutionToolError => {
161+ if (value === undefined) {
162+ return 0;
163+ }
164+
165+ if (typeof value !== "number" || !Number.isFinite(value) || value < 0) {
166+ return new ExecutionToolError({
167+ message: `${toolName} offset must be a non-negative number when provided`,
168+ });
169+ }
170+
171+ return Math.floor(value);
172+};
173+
160174 const makeFullInvoker = (executor: Executor, invokeOptions: InvokeOptions): SandboxToolInvoker => {
161175 const base = makeExecutorToolInvoker(executor, { invokeOptions });
162176 return {
@@ -166,7 +180,7 @@ const makeFullInvoker = (executor: Executor, invokeOptions: InvokeOptions): Sand · 3 unmodified lines
166180 return Effect.fail(
167181 new ExecutionToolError({
168182 message:
169− "tools.search expects an object: { query?: string; namespace?: string; limit?: number }",
183+ "tools.search expects an object: { query?: string; namespace?: string; limit?: number; offset?: number }",
170184 }),
171185 );
172186 }
@@ -192,8 +206,14 @@ const makeFullInvoker = (executor: Executor, invokeOptions: InvokeOptions): Sand · 19 unmodified lines
192206 return Effect.fail(limit);
193207 }
194208
209+ const offset = readOptionalOffset(args.offset, "tools.search");
210+ if (offset instanceof ExecutionToolError) {
211+ return Effect.fail(offset);
212+ }
213+
195214 return searchTools(executor, args.query ?? "", limit, {
196215 namespace: args.namespace,
216+ offset,
197217 }).pipe(
198218 Effect.withSpan("mcp.tool.dispatch", {
199219 attributes: { "mcp.tool.name": path, "executor.tool.builtin": true },
@@ -205,7 +225,7 @@ const makeFullInvoker = (executor: Executor, invokeOptions: InvokeOptions): Sand · 5 unmodified lines
205225 return Effect.fail(
206226 new ExecutionToolError({
207227 message:
208− "tools.executor.sources.list expects an object: { query?: string; limit?: number }",
228+ "tools.executor.sources.list expects an object: { query?: string; limit?: number; offset?: number }",
209229 }),
210230 );
211231 }
@@ -226,9 +246,18 @@ const makeFullInvoker = (executor: Executor, invokeOptions: InvokeOptions): Sand · 14 unmodified lines
226246 return Effect.fail(limit);
227247 }
228248
249+ const offset = readOptionalOffset(
250+ isRecord(args) ? args.offset : undefined,
251+ "tools.executor.sources.list",
252+ );
253+ if (offset instanceof ExecutionToolError) {
254+ return Effect.fail(offset);
255+ }
256+
229257 return listExecutorSources(executor, {
230258 query: isRecord(args) && typeof args.query === "string" ? args.query : undefined,
231259 limit,
260+ offset,
232261 }).pipe(
233262 Effect.withSpan("mcp.tool.dispatch", {
234263 attributes: { "mcp.tool.name": path, "executor.tool.builtin": true },
packages/core/execution/src/tool-invoker.test.ts+162−18
@@ -143,15 +143,21 @@ describe("tool discovery", () => { · 142 unmodified lines
143143 const executor = yield* makeSearchExecutor();
144144
145145 const githubMatches = yield* searchTools(executor, "github issues", 5);
146− expect(githubMatches.map((match) => match.path)).toEqual(["github.listRepositoryIssues"]);
147− expect(githubMatches[0]?.score ?? 0).toBeGreaterThan(0);
146+ expect(githubMatches.items.map((match) => match.path)).toEqual([
147+ "github.listRepositoryIssues",
148+ ]);
149+ expect(githubMatches.items[0]?.score ?? 0).toBeGreaterThan(0);
150+ expect(githubMatches.hasMore).toBe(false);
151+ expect(githubMatches.nextOffset).toBeNull();
148152
149153 const repoMatches = yield* searchTools(executor, "repo details", 5);
150− expect(repoMatches[0]?.path).toBe("github.getRepositoryDetails");
154+ expect(repoMatches.items[0]?.path).toBe("github.getRepositoryDetails");
151155
152156 const crmMatches = yield* searchTools(executor, "crm create contact", 5);
153− expect(crmMatches[0]?.path).toBe("crm.createContact");
154− expect(crmMatches[0]?.score ?? 0).toBeGreaterThan(crmMatches[1]?.score ?? 0);
157+ expect(crmMatches.items[0]?.path).toBe("crm.createContact");
158+ expect(crmMatches.items[0]?.score ?? 0).toBeGreaterThan(
159+ crmMatches.items[1]?.score ?? 0,
160+ );
155161 }),
156162 );
157163
@@ -159,7 +165,49 @@ describe("tool discovery", () => { · 1 unmodified line
159165 Effect.gen(function* () {
160166 const executor = yield* makeSearchExecutor();
161167 const matches = yield* searchTools(executor, "", 5);
162− expect(matches).toEqual([]);
168+ expect(matches.items).toEqual([]);
169+ expect(matches.total).toBe(0);
170+ expect(matches.hasMore).toBe(false);
171+ expect(matches.nextOffset).toBeNull();
172+ }),
173+ );
174+
175+ it.effect("paginates ranked matches via limit + offset with hasMore + nextOffset", () =>
176+ Effect.gen(function* () {
177+ const executor = yield* makeSearchExecutor();
178+
179+ // "list" matches `listRepositoryIssues`, `searchDocs` (description has
180+ // "documentation" which tokenises adjacent), `listContacts`, etc.
181+ // The exact match set isn't important — the pagination invariants are.
182+ const all = yield* searchTools(executor, "list", 100);
183+ expect(all.items.length).toBeGreaterThan(1);
184+ expect(all.total).toBe(all.items.length);
185+ expect(all.hasMore).toBe(false);
186+ expect(all.nextOffset).toBeNull();
187+
188+ // First page (limit 1) — matches truncate, hasMore + nextOffset surface.
189+ const firstPage = yield* searchTools(executor, "list", 1);
190+ expect(firstPage.items).toEqual([all.items[0]]);
191+ expect(firstPage.total).toBe(all.total);
192+ expect(firstPage.hasMore).toBe(true);
193+ expect(firstPage.nextOffset).toBe(1);
194+
195+ // Second page using nextOffset — order matches the un-paginated rank.
196+ const secondPage = yield* searchTools(executor, "list", 1, {
197+ offset: firstPage.nextOffset!,
198+ });
199+ expect(secondPage.items).toEqual([all.items[1]]);
200+ expect(secondPage.total).toBe(all.total);
201+ // Whether hasMore is true depends on total; at minimum it's consistent.
202+ expect(secondPage.hasMore).toBe(all.total > 2);
203+ expect(secondPage.nextOffset).toBe(secondPage.hasMore ? 2 : null);
204+
205+ // Offset past the end — empty page, no more.
206+ const past = yield* searchTools(executor, "list", 5, { offset: all.total + 10 });
207+ expect(past.items).toEqual([]);
208+ expect(past.total).toBe(all.total);
209+ expect(past.hasMore).toBe(false);
210+ expect(past.nextOffset).toBeNull();
163211 }),
164212 );
165213
@@ -170,21 +218,28 @@ describe("tool discovery", () => { · 4 unmodified lines
170218 const githubOnly = yield* searchTools(executor, "list", 5, {
171219 namespace: "github",
172220 });
173− expect(githubOnly.map((match) => match.path)).toEqual(["github.listRepositoryIssues"]);
221+ expect(githubOnly.items.map((match) => match.path)).toEqual([
222+ "github.listRepositoryIssues",
223+ ]);
174224
175225 const crmOnly = yield* searchTools(executor, "list", 5, {
176226 namespace: "crm",
177227 });
178− expect(crmOnly.map((match) => match.path)).toEqual(["crm.listContacts"]);
228+ expect(crmOnly.items.map((match) => match.path)).toEqual(["crm.listContacts"]);
179229
180230 const sandboxResult = yield* createExecutionEngine({ executor, codeExecutor }).execute(
181231 'return await tools.search({ namespace: "crm", query: "create contact", limit: 5 });',
182232 { onElicitation: acceptAll },
183233 );
184234 expect(sandboxResult.error).toBeUndefined();
185− expect(sandboxResult.result).toEqual([
186− expect.objectContaining({ path: "crm.createContact" }),
187− ]);
235+ expect(sandboxResult.result).toEqual(
236+ expect.objectContaining({
237+ items: [expect.objectContaining({ path: "crm.createContact" })],
238+ total: 1,
239+ hasMore: false,
240+ nextOffset: null,
241+ }),
242+ );
188243 }),
189244 );
190245
@@ -198,10 +253,15 @@ describe("tool discovery", () => { · 7 unmodified lines
198253 );
199254 expect(listed.error).toBeUndefined();
200255 expect(listed.result).toEqual(
201− expect.arrayContaining([
202− expect.objectContaining({ id: "github", toolCount: 3 }),
203− expect.objectContaining({ id: "crm", toolCount: 2 }),
204− ]),
256+ expect.objectContaining({
257+ items: expect.arrayContaining([
258+ expect.objectContaining({ id: "github", toolCount: 3 }),
259+ expect.objectContaining({ id: "crm", toolCount: 2 }),
260+ ]),
261+ total: 2,
262+ hasMore: false,
263+ nextOffset: null,
264+ }),
205265 );
206266
207267 const searched = yield* createExecutionEngine({ executor, codeExecutor }).execute(
@@ -209,7 +269,86 @@ describe("tool discovery", () => { · 1 unmodified line
209269 { onElicitation: acceptAll },
210270 );
211271 expect(searched.error).toBeUndefined();
212− expect(searched.result).toEqual([expect.objectContaining({ path: "crm.listContacts" })]);
272+ expect(searched.result).toEqual(
273+ expect.objectContaining({
274+ items: [expect.objectContaining({ path: "crm.listContacts" })],
275+ }),
276+ );
277+ }),
278+ );
279+
280+ it.effect("paginates source listings via limit + offset", () =>
281+ Effect.gen(function* () {
282+ const executor = yield* makeSearchExecutor();
283+ const engine = createExecutionEngine({ executor, codeExecutor });
284+
285+ // total = 2 (github, crm), sorted by name ("CRM" < "GitHub")
286+ const firstPage = yield* engine.execute(
287+ "return await tools.executor.sources.list({ limit: 1 });",
288+ { onElicitation: acceptAll },
289+ );
290+ expect(firstPage.error).toBeUndefined();
291+ expect(firstPage.result).toEqual(
292+ expect.objectContaining({
293+ items: [expect.objectContaining({ id: "crm" })],
294+ total: 2,
295+ hasMore: true,
296+ nextOffset: 1,
297+ }),
298+ );
299+
300+ const secondPage = yield* engine.execute(
301+ "return await tools.executor.sources.list({ limit: 1, offset: 1 });",
302+ { onElicitation: acceptAll },
303+ );
304+ expect(secondPage.error).toBeUndefined();
305+ expect(secondPage.result).toEqual(
306+ expect.objectContaining({
307+ items: [expect.objectContaining({ id: "github" })],
308+ total: 2,
309+ hasMore: false,
310+ nextOffset: null,
311+ }),
312+ );
313+ }),
314+ );
315+
316+ it.effect("rejects negative offsets via the engine validator", () =>
317+ Effect.gen(function* () {
318+ const executor = yield* makeSearchExecutor();
319+ const engine = createExecutionEngine({ executor, codeExecutor });
320+
321+ const badSearch = yield* engine.execute(
322+ [
323+ "try {",
324+ ' await tools.search({ query: "list", offset: -1 });',
325+ ' return "unexpected";',
326+ "} catch (error) {",
327+ " return error instanceof Error ? error.message : String(error);",
328+ "}",
329+ ].join("\n"),
330+ { onElicitation: acceptAll },
331+ );
332+ expect(badSearch.error).toBeUndefined();
333+ expect(String(badSearch.result)).toContain(
334+ "tools.search offset must be a non-negative number when provided",
335+ );
336+
337+ const badList = yield* engine.execute(
338+ [
339+ "try {",
340+ " await tools.executor.sources.list({ offset: -5 });",
341+ ' return "unexpected";',
342+ "} catch (error) {",
343+ " return error instanceof Error ? error.message : String(error);",
344+ "}",
345+ ].join("\n"),
346+ { onElicitation: acceptAll },
347+ );
348+ expect(badList.error).toBeUndefined();
349+ expect(String(badList.result)).toContain(
350+ "tools.executor.sources.list offset must be a non-negative number when provided",
351+ );
213352 }),
214353 );
215354
@@ -245,7 +384,7 @@ describe("tool discovery", () => { · 29 unmodified lines
245384 );
246385 expect(invalid.error).toBeUndefined();
247386 expect(String(invalid.result)).toContain(
248− "tools.search expects an object: { query?: string; namespace?: string; limit?: number }",
387+ "tools.search expects an object: { query?: string; namespace?: string; limit?: number; offset?: number }",
249388 );
250389
251390 const emptyQuery = yield* engine.execute(
@@ -253,7 +392,12 @@ describe("tool discovery", () => { · 1 unmodified line
253392 { onElicitation: acceptAll },
254393 );
255394 expect(emptyQuery.error).toBeUndefined();
256− expect(emptyQuery.result).toEqual([]);
395+ expect(emptyQuery.result).toEqual({
396+ items: [],
397+ total: 0,
398+ hasMore: false,
399+ nextOffset: null,
400+ });
257401
258402 const invalidDescribe = yield* engine.execute(
259403 [
packages/core/execution/src/tool-invoker.ts+80−27
@@ -143,6 +143,40 @@ export type ExecutorSourceListItem = { · 142 unmodified lines
143143 readonly toolCount: number;
144144 };
145145
146+/**
147+ * Page of results from a list-style discovery tool. Shared by
148+ * `tools.search` and `tools.executor.sources.list` so the model sees one
149+ * consistent shape:
150+ *
151+ * - `items` — the page (slice).
152+ * - `total` — count after filtering, before pagination. The model
153+ * can use this to detect truncation.
154+ * - `hasMore` — convenience flag for `(offset + items.length) < total`.
155+ * - `nextOffset` — concrete offset for the next page when `hasMore`,
156+ * `null` otherwise. Pre-computing it removes a class of
157+ * off-by-one mistakes when the model paginates.
158+ */
159+export type PagedResult<T> = {
160+ readonly items: readonly T[];
161+ readonly total: number;
162+ readonly hasMore: boolean;
163+ readonly nextOffset: number | null;
164+};
165+
166+const paginate = <T>(all: readonly T[], offset: number, limit: number): PagedResult<T> => {
167+ const total = all.length;
168+ const start = Math.min(Math.max(offset, 0), total);
169+ const items = all.slice(start, start + limit);
170+ const consumed = start + items.length;
171+ const hasMore = consumed < total;
172+ return {
173+ items,
174+ total,
175+ hasMore,
176+ nextOffset: hasMore ? consumed : null,
177+ };
178+};
179+
146180 type SearchableTool = Pick<Tool, "id" | "sourceId" | "name" | "description">;
147181
148182 type PreparedField = {
@@ -328,40 +362,57 @@ export const searchTools = Effect.fn("executor.tools.search")(function* ( · 179 unmodified lines
328362 executor: Executor,
329363 query: string,
330364 limit = 12,
331− options?: { readonly namespace?: string },
365+ options?: { readonly namespace?: string; readonly offset?: number },
332366 ) {
367+ const offset = options?.offset ?? 0;
333368 yield* Effect.annotateCurrentSpan({
334369 "executor.search.query_length": query.length,
335370 "executor.search.limit": limit,
371+ "executor.search.offset": offset,
336372 ...(options?.namespace ? { "executor.search.namespace": options.namespace } : {}),
337373 });
338374
375+ const empty: PagedResult<ToolDiscoveryResult> = {
376+ items: [],
377+ total: 0,
378+ hasMore: false,
379+ nextOffset: null,
380+ };
381+
339382 if (normalizeSearchText(query).length === 0) {
340− return [] as ReadonlyArray<ToolDiscoveryResult>;
383+ return empty;
341384 }
342385
343386 const all = yield* executor.tools.list({ includeAnnotations: false }).pipe(Effect.orDie);
344− const results = all
387+ const ranked = all
345388 .filter((tool: Tool) => matchesNamespace(tool, options?.namespace))
346389 .map((tool: Tool) => scoreToolMatch(tool, query))
347390 .filter((tool): tool is ToolDiscoveryResult => tool !== null)
348− .sort((left, right) => right.score - left.score || left.path.localeCompare(right.path))
349− .slice(0, limit);
391+ .sort((left, right) => right.score - left.score || left.path.localeCompare(right.path));
392+
393+ const page = paginate(ranked, offset, limit);
350394
351395 yield* Effect.annotateCurrentSpan({
352396 "executor.search.candidate_count": all.length,
353− "executor.search.result_count": results.length,
397+ "executor.search.match_count": ranked.length,
398+ "executor.search.result_count": page.items.length,
399+ "executor.search.has_more": page.hasMore,
354400 });
355− return results;
401+ return page;
356402 });
357403
358404 /** What `tools.executor.sources.list()` calls inside the sandbox. */
359405 export const listExecutorSources = Effect.fn("executor.sources.list")(function* (
360406 executor: Executor,
361− options?: { readonly query?: string; readonly limit?: number },
407+ options?: {
408+ readonly query?: string;
409+ readonly limit?: number;
410+ readonly offset?: number;
411+ },
362412 ) {
363413 const normalizedQuery = normalizeSearchText(options?.query ?? "");
364− const limit = options?.limit ?? 200;
414+ const limit = options?.limit ?? 50;
415+ const offset = options?.offset ?? 0;
365416 const sources = yield* executor.sources.list().pipe(Effect.orDie);
366417
367418 const filtered =
@@ -379,28 +430,30 @@ export const listExecutorSources = Effect.fn("executor.sources.list")(function* · 11 unmodified lines
379430 toolCountBySource.set(tool.sourceId, (toolCountBySource.get(tool.sourceId) ?? 0) + 1);
380431 }
381432
382− const withCounts = filtered.map(
383− (source: Source) =>
384− ({
385− id: source.id,
386− name: source.name,
387− kind: source.kind,
388− runtime: source.runtime,
389− canRemove: source.canRemove,
390− canRefresh: source.canRefresh,
391− toolCount: toolCountBySource.get(source.id) ?? 0,
392− }) satisfies ExecutorSourceListItem,
393− );
394−
395− const results = withCounts
396− .sort((left, right) => left.name.localeCompare(right.name) || left.id.localeCompare(right.id))
397− .slice(0, limit);
433+ const sortedWithCounts = filtered
434+ .map(
435+ (source: Source) =>
436+ ({
437+ id: source.id,
438+ name: source.name,
439+ kind: source.kind,
440+ runtime: source.runtime,
441+ canRemove: source.canRemove,
442+ canRefresh: source.canRefresh,
443+ toolCount: toolCountBySource.get(source.id) ?? 0,
444+ }) satisfies ExecutorSourceListItem,
445+ )
446+ .sort((left, right) => left.name.localeCompare(right.name) || left.id.localeCompare(right.id));
447+
448+ const page = paginate(sortedWithCounts, offset, limit);
398449
399450 yield* Effect.annotateCurrentSpan({
400451 "executor.sources.candidate_count": sources.length,
401− "executor.sources.result_count": results.length,
452+ "executor.sources.match_count": sortedWithCounts.length,
453+ "executor.sources.result_count": page.items.length,
454+ "executor.sources.has_more": page.hasMore,
402455 });
403− return results;
456+ return page;
404457 });
405458
406459 /** What `tools.describe.tool()` calls inside the sandbox. */