Skip to content

Commit 6a1a859

Browse files
committed
fix(copilot): make the search_docs topK clamp type-safe and test it
The clamp guarded magnitude but not type: Math.min/Math.max propagate NaN, so a non-numeric topK reached the query as `.limit(NaN)`. The `?? DEFAULT` only caught undefined. Nothing enforced this but the generated Ajv schema, and searchDocs is also called directly, so it should not depend on that. Extract clampTopK, which falls back to the default for anything non-finite (NaN, Infinity, a string that slipped through) and clamps the rest to [1, 25]. The clamp was completely untested because the db mock's .limit() stub discarded its argument — the mock now records it. Covers default, cap, floor, truncation, and the non-finite fallback. Worth pinning: staging's search_documentation documented "max 10" and enforced nothing, so this bound is new behavior, not just a bigger number.
1 parent e52205d commit 6a1a859

2 files changed

Lines changed: 62 additions & 3 deletions

File tree

apps/sim/lib/copilot/docs/docs-search.test.ts

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@
33
*/
44
import { beforeEach, describe, expect, it, vi } from 'vitest'
55

6-
const { mockGenerateSearchEmbedding, capturedWhere, mockRows } = vi.hoisted(() => ({
6+
const { mockGenerateSearchEmbedding, capturedWhere, capturedLimit, mockRows } = vi.hoisted(() => ({
77
mockGenerateSearchEmbedding: vi.fn(),
88
capturedWhere: { value: undefined as unknown },
9+
capturedLimit: { value: undefined as number | undefined },
910
mockRows: { value: [] as unknown[] },
1011
}))
1112

@@ -38,7 +39,12 @@ vi.mock('@sim/db', () => ({
3839
where: (condition: unknown) => {
3940
capturedWhere.value = condition
4041
return {
41-
orderBy: () => ({ limit: async () => mockRows.value }),
42+
orderBy: () => ({
43+
limit: async (n: number) => {
44+
capturedLimit.value = n
45+
return mockRows.value
46+
},
47+
}),
4248
}
4349
},
4450
}),
@@ -227,3 +233,43 @@ describe('searchDocs shortfall reporting', () => {
227233
expect(outcome.results).toHaveLength(1)
228234
})
229235
})
236+
237+
describe('searchDocs topK clamping', () => {
238+
beforeEach(() => {
239+
capturedLimit.value = undefined
240+
mockRows.value = []
241+
mockGenerateSearchEmbedding.mockResolvedValue({ embedding: [0.1, 0.2] })
242+
})
243+
244+
it('defaults to 10 when unspecified', async () => {
245+
await searchDocs('cron')
246+
expect(capturedLimit.value).toBe(10)
247+
})
248+
249+
it('caps at 25 — the documented max, which the old tool never enforced', async () => {
250+
await searchDocs('cron', { topK: 500 })
251+
expect(capturedLimit.value).toBe(25)
252+
})
253+
254+
it('floors at 1', async () => {
255+
await searchDocs('cron', { topK: 0 })
256+
expect(capturedLimit.value).toBe(1)
257+
await searchDocs('cron', { topK: -8 })
258+
expect(capturedLimit.value).toBe(1)
259+
})
260+
261+
it('truncates a fractional count', async () => {
262+
await searchDocs('cron', { topK: 7.9 })
263+
expect(capturedLimit.value).toBe(7)
264+
})
265+
266+
it('falls back to the default rather than passing NaN to the query', async () => {
267+
// Math.min/Math.max propagate NaN, so a bare clamp would reach `.limit(NaN)`.
268+
await searchDocs('cron', { topK: Number.NaN })
269+
expect(capturedLimit.value).toBe(10)
270+
await searchDocs('cron', { topK: 'twelve' as unknown as number })
271+
expect(capturedLimit.value).toBe(10)
272+
await searchDocs('cron', { topK: Number.POSITIVE_INFINITY })
273+
expect(capturedLimit.value).toBe(10)
274+
})
275+
})

apps/sim/lib/copilot/docs/docs-search.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,19 @@ function escapeLikePattern(value: string): string {
108108
return value.replace(/[\\%_]/g, (char) => `\\${char}`)
109109
}
110110

111+
/**
112+
* Clamp a caller-supplied result count into [1, {@link MAX_TOP_K}].
113+
*
114+
* Guards magnitude AND type: `Math.min`/`Math.max` propagate NaN, so a
115+
* non-numeric value would otherwise reach the query as `.limit(NaN)`. The
116+
* generated tool schema rejects a non-number upstream today, but this function
117+
* is also called directly, so it does not rely on that.
118+
*/
119+
function clampTopK(requested: number | undefined): number {
120+
if (requested === undefined || !Number.isFinite(requested)) return DEFAULT_TOP_K
121+
return Math.min(Math.max(Math.trunc(requested), 1), MAX_TOP_K)
122+
}
123+
111124
/**
112125
* Semantic search over the indexed docs corpus (`docs_embeddings`, rebuilt by
113126
* `scripts/process-docs.ts` on release). Every result carries the `docs/` path
@@ -128,7 +141,7 @@ export async function searchDocs(
128141
): Promise<DocsSearchOutcome> {
129142
if (!query || typeof query !== 'string') throw new Error('query is required')
130143

131-
const topK = Math.min(Math.max(Math.trunc(options?.topK ?? DEFAULT_TOP_K), 1), MAX_TOP_K)
144+
const topK = clampTopK(options?.topK)
132145
const where = scopeCondition(options?.path)
133146

134147
logger.info('Executing docs search', { query, topK, path: options?.path ?? null })

0 commit comments

Comments
 (0)