diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c8dd8f2a4..10ea9bcf94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixes +- Searches with `path:` or `name:` filters now find matching symbols before unrelated results consume the limit, including typo searches and queries containing only filters. + - In TypeScript, an awaited call with type arguments like `await fetchApi("/me")` or `await api.get(url)` now links to the function it calls. Before, the call was dropped, so typed fetch wrappers looked like they had no callers. (#2586) - In React, a component rendered at module scope, in a top-level constant or inside a test's `it(…)` callback, like `createRoot(el).render()` or `render()`, now counts as rendered there, so an app's entry reaches its root component and a component's tests show among its callers. (#2587) - In Python, `import a.b as X` written inside a function now links its calls like `X.f()`, as the same import at module level already did. Thanks @Egrn for the report. (#2600) diff --git a/__tests__/search-query-parser.test.ts b/__tests__/search-query-parser.test.ts index 8a7767dac3..acba91ae27 100644 --- a/__tests__/search-query-parser.test.ts +++ b/__tests__/search-query-parser.test.ts @@ -4,7 +4,12 @@ * `name:` filtering and the fuzzy typo fallback. */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import CodeGraph from '../src'; +import { ToolHandler } from '../src/mcp/tools'; import { parseQuery, boundedEditDistance } from '../src/search/query-parser'; describe('parseQuery', () => { @@ -140,3 +145,110 @@ describe('boundedEditDistance', () => { expect(boundedEditDistance('aaaaa', 'bbbbb', 2)).toBe(3); }); }); + +describe('search hard filters precede candidate limits', () => { + let root: string; + let cg: CodeGraph; + const names = (query: string, limit = 1) => cg.searchNodes(query, { limit, kinds: ['function'] }).map(r => r.node.name); + + beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-search-filter-')); + const write = (file: string, source: string) => { + fs.mkdirSync(path.dirname(path.join(root, file)), { recursive: true }); + fs.writeFileSync(path.join(root, file), source); + }; + const filler = Array.from({ length: 120 }, (_, i) => [ + `export function authenticate${i}() { return ${i}; }`, + `export function aaaNeedle${i}() { return ${i}; }`, + ].join('\n')); + // Distinct names with the same folded spelling occupy the fuzzy name cap. + const variants = Array.from({ length: 64 }, (_, mask) => 'G' + [...'etUser'].map((c, i) => + mask & (1 << i) ? c.toUpperCase() : c.toLowerCase(), + ).join('')); + write('aaa/noise.ts', filler.join('\n') + '\nexport function authenticate() {}\n' + + variants.map(n => `export function ${n}() {}`).join('\n')); + write('product/handler.ts', [ + 'export function authenticateUserHandler() {}', + 'export function zzNeedleHandler() {}', + 'export function zzzRareHandler() {}', + 'export function getUser() {}', + ].join('\n')); + write('Ünicode/handler.ts', 'export function Öffnen() {}'); + write('percent%/unit_/literal.ts', 'export function percent_name() {}'); + write('percentA/unitA/decoy.ts', 'export function percentAname() {}'); + cg = await CodeGraph.init(root, { index: true }); + }); + + afterAll(() => { + cg?.close(); + if (root) fs.rmSync(root, { recursive: true, force: true }); + }); + + it.each([1, 2, 10])('finds a selective FTS path/name hit with limit %i', limit => { + expect(cg.getBackend()).toBe('node-sqlite'); + expect(names('authenticate path:product/', limit)).toEqual(['authenticateUserHandler']); + expect(names('authenticate name:UserHandler', limit)).toEqual(['authenticateUserHandler']); + }); + + it('finds filter-only hits beyond the alphabetically first candidates', () => { + expect(names('kind:function name:zzzRareHandler')).toEqual(['zzzRareHandler']); + expect(names('kind:function path:product/', 10)).toHaveLength(4); + }); + + it('filters LIKE fallback before its limit', () => { + expect(names('needle path:product/')).toEqual(['zzNeedleHandler']); + expect(names('needle name:zzNeedleHandler')).toEqual(['zzNeedleHandler']); + }); + + it('filters fuzzy definitions before per-name and follow-up caps', () => { + expect(names('getUsar path:product/')).toEqual(['getUser']); + expect(names('getUsar name:getUser')).toHaveLength(1); + }); + + it('preserves Unicode case-insensitive literal substring matching', () => { + expect(names('kind:function path:ünicode/ name:öffnen')).toEqual(['Öffnen']); + }); + + it('treats percent signs and underscores literally', () => { + expect(names('kind:function path:percent%/ name:percent_')).toEqual(['percent_name']); + expect(names('kind:function path:unit_/', 10)).toEqual(['percent_name']); + expect(names('kind:function name:percent_', 10)).toEqual(['percent_name']); + }); + + it('ORs repeated filters within each family and ANDs the two families', () => { + expect(names('kind:function path:product/ path:ünicode/ name:zzzRareHandler name:öffnen', 10).sort()) + .toEqual(['zzzRareHandler', 'Öffnen'].sort()); + expect(names('kind:function path:product/ name:öffnen', 10)).toEqual([]); + }); + + it('keeps exact matches first in unfiltered search and avoids supplement leakage', () => { + expect(names('authenticate')).toEqual(['authenticate']); + expect(names('authenticate path:product/')).toEqual(['authenticateUserHandler']); + expect(names('authenticate path:absent/', 100)).toEqual([]); + expect(names('authenticate name:NoSuchHandler', 100)).toEqual([]); + }); + + it('skips filtered candidates, rather than unrelated rows, for offset', () => { + const all = cg.searchNodes('kind:function path:product/', { limit: 10 }); + const page = cg.searchNodes('kind:function path:product/', { limit: 1, offset: 1 }); + expect(page[0]!.node.id).toBe(all[1]!.node.id); + }); + + it('returns a real MCP match and success-shaped guidance only for an absent filter', async () => { + const prior = process.env.CODEGRAPH_MCP_TOOLS; + process.env.CODEGRAPH_MCP_TOOLS = 'search'; + const handler = new ToolHandler(cg); + try { + const result = await handler.execute('codegraph_search', { query: 'authenticate path:product/', limit: 1 }); + expect(result.isError).not.toBe(true); + expect(JSON.stringify(result.content)).toContain('authenticateUserHandler'); + const absent = await handler.execute('codegraph_search', { query: 'authenticate path:absent/', limit: 1 }); + expect(absent.isError).not.toBe(true); + expect(JSON.stringify(absent.content)).toContain('No results found'); + } finally { + await handler.closeAll(); + if (prior === undefined) delete process.env.CODEGRAPH_MCP_TOOLS; + else process.env.CODEGRAPH_MCP_TOOLS = prior; + } + }); +}); diff --git a/src/db/queries.ts b/src/db/queries.ts index fe2f73d9e3..ebd0516c2e 100644 --- a/src/db/queries.ts +++ b/src/db/queries.ts @@ -247,6 +247,12 @@ function rowToFileRecord(row: FileRow): FileRecord { }; } +interface SearchFieldFilters { + /** Lowercased literal substrings, OR within each family and AND across families. */ + pathFilters?: string[]; + nameFilters?: string[]; +} + /** * Files ranked by edges between two of their own symbols — the input to * `getDominantFile`. CROSS JOIN pins the loop order to edges first, then a @@ -1587,8 +1593,9 @@ export class QueryBuilder { parsed.languages.length > 0 ? Array.from(new Set([...(options.languages ?? []), ...parsed.languages])) : options.languages; - const pathFilters = parsed.pathFilters; - const nameFilters = parsed.nameFilters; + const pathFilters = parsed.pathFilters.map(p => p.toLowerCase()); + const nameFilters = parsed.nameFilters.map(n => n.toLowerCase()); + const fieldFilters = { pathFilters, nameFilters }; // The text portion drives FTS/LIKE; if all the user typed was // filters (`kind:function`), we still need *some* candidate set, // so synthesise an empty-text path that returns everything matching @@ -1599,16 +1606,14 @@ export class QueryBuilder { // First try FTS5 with prefix matching (skip if FTS5 not available, #1532) let results = text - ? (this._fts5Available !== false ? this.searchNodesFTS(text, { kinds, languages, limit, offset }) : []) - // Over-fetch by 5× when running filter-only (no text). The - // post-scoring path: + name: filters can be very selective, so - // a smaller multiplier risks returning fewer than `limit` - // results despite the DB having plenty of matches. - : this.searchAllByFilters({ kinds, languages, limit: limit * 5 }); + ? (this._fts5Available !== false ? this.searchNodesFTS(text, { kinds, languages, limit, offset, ...fieldFilters }) : []) + // Keep the existing ranking pool, counting only rows that satisfy the + // explicit filters so unrelated symbols cannot consume that pool. + : this.searchAllByFilters({ kinds, languages, limit: limit * 5, offset, ...fieldFilters }); // If no FTS results, try LIKE-based substring search if (results.length === 0 && text.length >= 2) { - results = this.searchNodesLike(text, { kinds, languages, limit, offset }); + results = this.searchNodesLike(text, { kinds, languages, limit, offset, ...fieldFilters }); } // Final fuzzy fallback: scan all known names and keep those within @@ -1616,7 +1621,7 @@ export class QueryBuilder { // returned nothing AND there's a text portion long enough to be // worth fuzzing (1-char queries would match too much). if (results.length === 0 && text.length >= 3) { - results = this.searchNodesFuzzy(text, { kinds, languages, limit }); + results = this.searchNodesFuzzy(text, { kinds, languages, limit, ...fieldFilters }); } // Supplement: ensure exact name matches are always candidates. @@ -1653,8 +1658,7 @@ export class QueryBuilder { sql += ` AND language IN (${languages.map(() => '?').join(',')})`; params.push(...languages); } - sql += ' LIMIT 20'; - const rows = this.db.prepare(sql).all(...params) as NodeRow[]; + const rows = this.readSearchRows(sql, params, { limit: 20, ...fieldFilters }); for (const row of rows) { if (!existingIds.has(row.id)) { results.push({ node: rowToNode(row), score: maxFtsScore }); @@ -1692,40 +1696,56 @@ export class QueryBuilder { } } - // Apply path: + name: filters AFTER scoring. Scoring already uses - // path/name as a soft signal; the explicit filters here are a hard - // gate. Done last so the FTS limit fetched plenty of candidates to - // narrow from. - if (pathFilters.length > 0) { - const lowered = pathFilters.map((p) => p.toLowerCase()); - results = results.filter((r) => { - const fp = r.node.filePath.toLowerCase(); - return lowered.some((p) => fp.includes(p)); - }); + return results; + } + + /** + * Apply hard field filters before a candidate consumes the search budget. + * Iteration keeps the selected pool bounded even for a very selective + * filter. JavaScript's case folding and literal substring semantics are + * preserved: SQLite's lower() only folds ASCII, and LIKE treats %/_ as + * wildcards. The unfiltered hot path still uses SQL LIMIT/OFFSET directly. + */ + private readSearchRows( + sql: string, + params: (string | number)[], + options: SearchOptions & SearchFieldFilters, + ): T[] { + const { limit = 100, offset = 0, pathFilters = [], nameFilters = [] } = options; + if (pathFilters.length === 0 && nameFilters.length === 0) { + return this.db.prepare(`${sql} LIMIT ? OFFSET ?`).all(...params, limit, offset) as T[]; } - if (nameFilters.length > 0) { - const lowered = nameFilters.map((n) => n.toLowerCase()); - results = results.filter((r) => { - const nm = r.node.name.toLowerCase(); - return lowered.some((n) => nm.includes(n)); - }); + if (limit === 0) return []; + + const rows: T[] = []; + let skipped = 0; + for (const row of this.db.prepare(sql).iterate(...params) as IterableIterator) { + if (!this.matchesSearchFieldFilters(row, options)) continue; + if (skipped++ < offset) continue; + rows.push(row); + if (limit > 0 && rows.length >= limit) break; } + return rows; + } - return results; + private matchesSearchFieldFilters(row: Pick, filters: SearchFieldFilters): boolean { + const { pathFilters = [], nameFilters = [] } = filters; + return (pathFilters.length === 0 || pathFilters.some(p => row.file_path.toLowerCase().includes(p))) + && (nameFilters.length === 0 || nameFilters.some(n => row.name.toLowerCase().includes(n))); } /** * Match-everything path used when the user supplied only field * filters (`kind:function lang:typescript`) with no text. Returns - * candidates ordered by name; the caller's filter pass narrows to - * what was asked for. + * candidates ordered by name, applying field filters before the budget. */ private searchAllByFilters(options: { kinds?: NodeKind[]; languages?: Language[]; limit: number; - }): SearchResult[] { - const { kinds, languages, limit } = options; + offset?: number; + } & SearchFieldFilters): SearchResult[] { + const { kinds, languages } = options; let sql = 'SELECT * FROM nodes WHERE 1=1'; const params: (string | number)[] = []; if (kinds && kinds.length > 0) { @@ -1736,9 +1756,8 @@ export class QueryBuilder { sql += ` AND language IN (${languages.map(() => '?').join(',')})`; params.push(...languages); } - sql += ' ORDER BY name LIMIT ?'; - params.push(limit); - const rows = this.db.prepare(sql).all(...params) as NodeRow[]; + sql += ' ORDER BY name'; + const rows = this.readSearchRows(sql, params, options); return rows.map((row) => ({ node: rowToNode(row), score: 1 })); } @@ -1752,7 +1771,7 @@ export class QueryBuilder { */ private searchNodesFuzzy( text: string, - options: { kinds?: NodeKind[]; languages?: Language[]; limit: number } + options: { kinds?: NodeKind[]; languages?: Language[]; limit: number } & SearchFieldFilters ): SearchResult[] { const { kinds, languages, limit } = options; const lowered = text.toLowerCase(); @@ -1762,7 +1781,27 @@ export class QueryBuilder { // by getAllNodeNames(); even on a 200k-node project the distinct // name set is typically O(10k) because most names repeat. The // candidate-cap below bounds memory regardless. - const allNames = this.getAllNodeNames(); + let allNames = this.getAllNodeNames(); + if ((options.pathFilters?.length ?? 0) > 0 || (options.nameFilters?.length ?? 0) > 0) { + // Establish scope in one streaming read BEFORE the per-name query cap. + // Otherwise many similar names in unrelated files can consume that cap + // even though none of their definitions satisfies the hard filters. + let sql = 'SELECT DISTINCT name, file_path FROM nodes WHERE 1=1'; + const params: (string | number)[] = []; + if (kinds && kinds.length > 0) { + sql += ` AND kind IN (${kinds.map(() => '?').join(',')})`; + params.push(...kinds); + } + if (languages && languages.length > 0) { + sql += ` AND language IN (${languages.map(() => '?').join(',')})`; + params.push(...languages); + } + const eligibleNames = new Set(); + for (const row of this.db.prepare(sql).iterate(...params) as IterableIterator>) { + if (this.matchesSearchFieldFilters(row, options)) eligibleNames.add(row.name); + } + allNames = allNames.filter(name => eligibleNames.has(name)); + } const candidates: Array<{ name: string; dist: number }> = []; for (const name of allNames) { const dist = boundedEditDistance(name.toLowerCase(), lowered, maxDist); @@ -1792,8 +1831,7 @@ export class QueryBuilder { sql += ` AND language IN (${languages.map(() => '?').join(',')})`; params.push(...languages); } - sql += ' LIMIT 5'; - const rows = this.db.prepare(sql).all(...params) as NodeRow[]; + const rows = this.readSearchRows(sql, params, { ...options, limit: 5 }); for (const row of rows) { if (seen.has(row.id)) continue; seen.add(row.id); @@ -1854,8 +1892,8 @@ export class QueryBuilder { /** * FTS5 search with prefix matching */ - private searchNodesFTS(query: string, options: SearchOptions): SearchResult[] { - const { kinds, languages, limit = 100, offset = 0 } = options; + private searchNodesFTS(query: string, options: SearchOptions & SearchFieldFilters): SearchResult[] { + const { kinds, languages, limit = 100 } = options; // Add prefix wildcard for better matching (e.g., "auth" matches "AuthService", "authenticate") // Escape special FTS5 characters and add prefix wildcard. @@ -1904,11 +1942,10 @@ export class QueryBuilder { params.push(...languages); } - sql += ' ORDER BY score LIMIT ? OFFSET ?'; - params.push(ftsLimit, offset); + sql += ' ORDER BY score'; try { - const rows = this.db.prepare(sql).all(...params) as (NodeRow & { score: number })[]; + const rows = this.readSearchRows(sql, params, { ...options, limit: ftsLimit }); return rows.map((row) => ({ node: rowToNode(row), score: Math.abs(row.score), // bm25 returns negative scores @@ -1923,8 +1960,8 @@ export class QueryBuilder { * LIKE-based substring search for cases where FTS doesn't match * Useful for camelCase matching (e.g., "signIn" finds "signInWithGoogle") */ - private searchNodesLike(query: string, options: SearchOptions): SearchResult[] { - const { kinds, languages, limit = 100, offset = 0 } = options; + private searchNodesLike(query: string, options: SearchOptions & SearchFieldFilters): SearchResult[] { + const { kinds, languages } = options; let sql = ` SELECT nodes.*, @@ -1968,10 +2005,8 @@ export class QueryBuilder { params.push(...languages); } - sql += ' ORDER BY score DESC, length(name) ASC LIMIT ? OFFSET ?'; - params.push(limit, offset); - - const rows = this.db.prepare(sql).all(...params) as (NodeRow & { score: number })[]; + sql += ' ORDER BY score DESC, length(name) ASC'; + const rows = this.readSearchRows(sql, params, options); return rows.map((row) => ({ node: rowToNode(row),