diff --git a/CHANGELOG.md b/CHANGELOG.md index 0624716dc..addad8e14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed a server-side memory leak where a single shared react-query cache retained state from every server render; the cache is now created per-request. [#1575](https://github.com/sourcebot-dev/sourcebot/pull/1575) - Fixed code host retry warnings to include the HTTP response status. [#1576](https://github.com/sourcebot-dev/sourcebot/pull/1576) - Fixed streamed code search updates silently cancelling in-flight result navigation. [#1577](https://github.com/sourcebot-dev/sourcebot/pull/1577) +- Fixed the `grep` and `glob` agent tools mis-parsing structured search inputs containing spaces, commas, or quotes. [#1573](https://github.com/sourcebot-dev/sourcebot/pull/1573) ## [5.1.6] - 2026-08-10 diff --git a/packages/web/src/features/tools/glob.test.ts b/packages/web/src/features/tools/glob.test.ts new file mode 100644 index 000000000..16ddff134 --- /dev/null +++ b/packages/web/src/features/tools/glob.test.ts @@ -0,0 +1,64 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + search: vi.fn(), + getRepoInfoByName: vi.fn(), +})); + +vi.mock('@/features/search', () => ({ + search: mocks.search, +})); + +vi.mock('@/actions', () => ({ + getRepoInfoByName: mocks.getRepoInfoByName, +})); + +vi.mock('@/lib/utils', () => ({ + isServiceError: () => false, +})); + +vi.mock('./logger', () => ({ + logger: { debug: vi.fn() }, +})); + +import { globDefinition } from './glob'; +import { buildGlobSearchQuery } from './searchQuery'; + +const emptySearchResponse = { + files: [], + repositoryInfo: [], + stats: { actualMatchCount: 0 }, + isSearchExhaustive: true, +}; + +describe('glob', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.search.mockResolvedValue(emptySearchResponse); + }); + + it('executes with QueryIR', async () => { + const result = await globDefinition.execute({ + pattern: 'My Folder/**/*.ts', + ref: 'feature/my feature', + }, { + source: 'test', + selectedRepos: ['Repo One'], + }); + + expect(mocks.search).toHaveBeenCalledWith({ + queryType: 'ir', + query: buildGlobSearchQuery({ + pattern: 'My Folder/**/*.ts', + ref: 'feature/my feature', + selectedRepos: ['Repo One'], + }), + options: { + matches: 100, + contextLines: 0, + }, + source: 'test', + }); + expect(result.metadata).not.toHaveProperty('query'); + }); +}); diff --git a/packages/web/src/features/tools/glob.ts b/packages/web/src/features/tools/glob.ts index 6e855a32c..65b034539 100644 --- a/packages/web/src/features/tools/glob.ts +++ b/packages/web/src/features/tools/glob.ts @@ -1,22 +1,16 @@ import { z } from "zod"; -import globToRegexp from "glob-to-regexp"; import { isServiceError } from "@/lib/utils"; import { search } from "@/features/search"; -import escapeStringRegexp from "escape-string-regexp"; import { Source, ToolDefinition } from "./types"; import { logger } from "./logger"; import description from "./glob.txt"; import { CodeHostType } from "@sourcebot/db"; import { getRepoInfoByName } from "@/actions"; +import { buildGlobSearchQuery } from "./searchQuery"; const DEFAULT_LIMIT = 100; const TRUNCATION_MESSAGE = `(Results truncated. Consider using a more specific pattern, specifying a repo, or increasing the limit.)`; -function globToFileRegexp(glob: string): string { - const re = globToRegexp(glob, { extended: true, globstar: true }); - return re.source.replace(/^\^/, ''); -} - const globShape = { pattern: z .string() @@ -55,7 +49,6 @@ export type GlobRepoInfo = { export type GlobMetadata = { files: GlobFile[]; pattern: string; - query: string; fileCount: number; repoCount: number; repoInfoMap: Record; @@ -81,30 +74,20 @@ export const globDefinition: ToolDefinition<'glob', typeof globShape, GlobMetada logger.debug('glob', { pattern, repo, ref, path, limit }); - let query = `file:${globToFileRegexp(pattern)}`; - - if (path) { - query += ` file:${escapeStringRegexp(path)}`; - } - - if (repo) { - query += ` repo:${escapeStringRegexp(repo)}`; - } else if (context.selectedRepos && context.selectedRepos.length > 0) { - query += ` reposet:${context.selectedRepos.join(',')}`; - } - - if (ref) { - query += ` rev:${ref}`; - } + const query = buildGlobSearchQuery({ + pattern, + path, + repo, + ref, + selectedRepos: context.selectedRepos, + }); const response = await search({ - queryType: 'string', + queryType: 'ir', query, options: { matches: limit, contextLines: 0, - isCaseSensitivityEnabled: true, - isRegexEnabled: true, }, source: context.source, }); @@ -144,7 +127,6 @@ export const globDefinition: ToolDefinition<'glob', typeof globShape, GlobMetada const metadata: GlobMetadata = { files, pattern, - query, fileCount: files.length, repoCount: new Set(files.map((f) => f.repo)).size, repoInfoMap, diff --git a/packages/web/src/features/tools/grep.test.ts b/packages/web/src/features/tools/grep.test.ts new file mode 100644 index 000000000..74bc04fe5 --- /dev/null +++ b/packages/web/src/features/tools/grep.test.ts @@ -0,0 +1,65 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + search: vi.fn(), + getRepoInfoByName: vi.fn(), +})); + +vi.mock('@/features/search', () => ({ + search: mocks.search, +})); + +vi.mock('@/actions', () => ({ + getRepoInfoByName: mocks.getRepoInfoByName, +})); + +vi.mock('@/lib/utils', () => ({ + isServiceError: () => false, +})); + +vi.mock('./logger', () => ({ + logger: { debug: vi.fn() }, +})); + +import { grepDefinition } from './grep'; +import { buildGrepSearchQuery } from './searchQuery'; + +const emptySearchResponse = { + files: [], + repositoryInfo: [], + stats: { actualMatchCount: 0 }, + isSearchExhaustive: true, +}; + +describe('grep', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.search.mockResolvedValue(emptySearchResponse); + }); + + it('executes with QueryIR', async () => { + const result = await grepDefinition.execute({ + pattern: 'needle', + path: 'src/my dir', + limit: 25, + }, { + source: 'test', + selectedRepos: ['Repo One'], + }); + + expect(mocks.search).toHaveBeenCalledWith({ + queryType: 'ir', + query: buildGrepSearchQuery({ + pattern: 'needle', + path: 'src/my dir', + selectedRepos: ['Repo One'], + }), + options: { + matches: 25, + contextLines: 0, + }, + source: 'test', + }); + expect(result.metadata).not.toHaveProperty('query'); + }); +}); diff --git a/packages/web/src/features/tools/grep.ts b/packages/web/src/features/tools/grep.ts index 26b78afde..eda4acab8 100644 --- a/packages/web/src/features/tools/grep.ts +++ b/packages/web/src/features/tools/grep.ts @@ -1,13 +1,12 @@ import { z } from "zod"; -import globToRegexp from "glob-to-regexp"; import { isServiceError } from "@/lib/utils"; import { search } from "@/features/search"; -import escapeStringRegexp from "escape-string-regexp"; import { Source, ToolDefinition } from "./types"; import { logger } from "./logger"; import description from "./grep.txt"; import { CodeHostType } from "@sourcebot/db"; import { getRepoInfoByName } from "@/actions"; +import { buildGrepSearchQuery } from "./searchQuery"; const DEFAULT_LIMIT = 100; const DEFAULT_GROUP_BY_REPO_LIMIT = 10_000; @@ -15,11 +14,6 @@ const MAX_LINE_LENGTH = 2000; const MAX_LINE_SUFFIX = `... (line truncated to ${MAX_LINE_LENGTH} chars)`; const TRUNCATION_MESSAGE = `(Results truncated. Consider using a more specific path or pattern, specifying a repo, or increasing the limit.)`; -function globToFileRegexp(glob: string): string { - const re = globToRegexp(glob, { extended: true, globstar: true }); - return re.source.replace(/^\^/, ''); -} - const grepShape = { pattern: z .string() @@ -66,7 +60,6 @@ export type GrepRepoInfo = { export type GrepMetadata = { files: GrepFile[]; pattern: string; - query: string; matchCount: number; repoCount: number; repoInfoMap: Record; @@ -95,35 +88,21 @@ export const grepDefinition: ToolDefinition<'grep', typeof grepShape, GrepMetada logger.debug('grep', { pattern, path, include, repo, ref, limit, groupByRepo }); - const quotedPattern = `"${pattern.replace(/"/g, '\\"')}"`; - let query = quotedPattern; - - if (path) { - query += ` file:${escapeStringRegexp(path)}`; - } - - if (include) { - query += ` file:${globToFileRegexp(include)}`; - } - - if (repo) { - query += ` repo:${escapeStringRegexp(repo)}`; - } else if (context.selectedRepos && context.selectedRepos.length > 0) { - query += ` reposet:${context.selectedRepos.join(',')}`; - } - - if (ref) { - query += ` rev:${ref}`; - } + const query = buildGrepSearchQuery({ + pattern, + path, + include, + repo, + ref, + selectedRepos: context.selectedRepos, + }); const response = await search({ - queryType: 'string', + queryType: 'ir', query, options: { matches: limit, contextLines: 0, - isCaseSensitivityEnabled: true, - isRegexEnabled: true, }, source: context.source, }); @@ -161,7 +140,6 @@ export const grepDefinition: ToolDefinition<'grep', typeof grepShape, GrepMetada const metadata: GrepMetadata = { files, pattern, - query, matchCount: response.stats.actualMatchCount, repoCount: new Set(files.map((f) => f.repo)).size, repoInfoMap, diff --git a/packages/web/src/features/tools/searchQuery.test.ts b/packages/web/src/features/tools/searchQuery.test.ts new file mode 100644 index 000000000..e60ad03ae --- /dev/null +++ b/packages/web/src/features/tools/searchQuery.test.ts @@ -0,0 +1,148 @@ +import { describe, expect, it } from 'vitest'; +import { buildGlobSearchQuery, buildGrepSearchQuery } from './searchQuery'; + +describe('buildGrepSearchQuery', () => { + it('preserves spaces and quotes in structured search inputs', () => { + const query = buildGrepSearchQuery({ + pattern: 'call("hello world")', + path: 'src/my dir', + include: 'My Tests/**/*.test.ts', + repo: 'Acme, Platform', + ref: 'feature/my feature', + }); + + expect(query).toEqual({ + and: { + children: [ + { + regexp: { + regexp: 'call("hello world")', + case_sensitive: true, + file_name: false, + content: true, + }, + query: 'regexp', + }, + { + regexp: { + regexp: 'src/my dir', + case_sensitive: true, + file_name: true, + content: false, + }, + query: 'regexp', + }, + { + regexp: { + regexp: 'My Tests\\/((?:[^/]*(?:\\/|$))*)([^/]*)\\.test\\.ts$', + case_sensitive: true, + file_name: true, + content: false, + }, + query: 'regexp', + }, + { + repo_set: { + set: { 'Acme, Platform': true }, + }, + query: 'repo_set', + }, + { + branch: { + pattern: 'feature/my feature', + exact: false, + }, + query: 'branch', + }, + ], + }, + query: 'and', + }); + }); + + it('uses exact selected repository names when no explicit repo is provided', () => { + const query = buildGrepSearchQuery({ + pattern: 'needle', + selectedRepos: ['Repo, One', 'Repo Two'], + }); + + expect(query).toMatchObject({ + and: { + children: [ + { regexp: { regexp: 'needle' } }, + { + repo_set: { + set: { + 'Repo, One': true, + 'Repo Two': true, + }, + }, + }, + ], + }, + }); + }); + + it('keeps a bare content search as a single IR node', () => { + expect(buildGrepSearchQuery({ pattern: 'needle' })).toEqual({ + regexp: { + regexp: 'needle', + case_sensitive: true, + file_name: false, + content: true, + }, + query: 'regexp', + }); + }); +}); + +describe('buildGlobSearchQuery', () => { + it('keeps a glob containing spaces in one file predicate', () => { + const query = buildGlobSearchQuery({ + pattern: 'My Folder/**/*.ts', + path: 'packages/my-dir/[legacy] (copy)+', + }); + + expect(query).toEqual({ + and: { + children: [ + { + regexp: { + regexp: 'My Folder\\/((?:[^/]*(?:\\/|$))*)([^/]*)\\.ts$', + case_sensitive: true, + file_name: true, + content: false, + }, + query: 'regexp', + }, + { + regexp: { + regexp: 'packages/my-dir/\\[legacy\\] \\(copy\\)\\+', + case_sensitive: true, + file_name: true, + content: false, + }, + query: 'regexp', + }, + ], + }, + query: 'and', + }); + }); + + it('preserves the rev:* behavior for searching every branch', () => { + const query = buildGlobSearchQuery({ + pattern: '*.ts', + ref: '*', + }); + + expect(query).toMatchObject({ + and: { + children: [ + { regexp: { file_name: true } }, + { branch: { pattern: '', exact: false } }, + ], + }, + }); + }); +}); diff --git a/packages/web/src/features/tools/searchQuery.ts b/packages/web/src/features/tools/searchQuery.ts new file mode 100644 index 000000000..4446c3328 --- /dev/null +++ b/packages/web/src/features/tools/searchQuery.ts @@ -0,0 +1,128 @@ +import globToRegexp from "glob-to-regexp"; +import type { QueryIR } from "@/features/search/ir"; + +type SearchScope = { + repo?: string; + ref?: string; + selectedRepos?: string[]; +}; + +type GrepSearchQuery = SearchScope & { + pattern: string; + path?: string; + include?: string; +}; + +type GlobSearchQuery = SearchScope & { + pattern: string; + path?: string; +}; + +const createRegexpQuery = ({ + regexp, + fileName, + content, +}: { + regexp: string; + fileName: boolean; + content: boolean; +}): QueryIR => ({ + regexp: { + regexp, + case_sensitive: true, + file_name: fileName, + content, + }, + query: "regexp", +}); + +// Keep literal tool inputs compatible with Zoekt's RE2 parser. The generic +// escape-string-regexp package emits hex escapes for some characters. +const escapeRE2Regexp = (value: string): string => value.replace(/[\\.^$|?*+()[\]{}]/g, '\\$&'); + +const globToFileRegexp = (glob: string): string => { + const regexp = globToRegexp(glob, { extended: true, globstar: true }); + return regexp.source.replace(/^\^/, ''); +}; + +const createRepoScopeQuery = ({ repo, selectedRepos }: SearchScope): QueryIR | undefined => { + const repos = repo ? [repo] : selectedRepos; + if (!repos || repos.length === 0) { + return undefined; + } + + return { + repo_set: { + set: Object.fromEntries(repos.map((repoName) => [repoName, true])), + }, + query: "repo_set", + }; +}; + +const createBranchQuery = (ref?: string): QueryIR | undefined => ref ? { + branch: { + pattern: ref === '*' ? '' : ref, + exact: false, + }, + query: "branch", +} : undefined; + +const combineQueries = (queries: Array): QueryIR => { + const children = queries.filter((query): query is QueryIR => query !== undefined); + if (children.length === 1) { + return children[0]; + } + + return { + and: { children }, + query: "and", + }; +}; + +export const buildGrepSearchQuery = ({ + pattern, + path, + include, + repo, + ref, + selectedRepos, +}: GrepSearchQuery): QueryIR => combineQueries([ + createRegexpQuery({ + regexp: pattern, + fileName: false, + content: true, + }), + path ? createRegexpQuery({ + regexp: escapeRE2Regexp(path), + fileName: true, + content: false, + }) : undefined, + include ? createRegexpQuery({ + regexp: globToFileRegexp(include), + fileName: true, + content: false, + }) : undefined, + createRepoScopeQuery({ repo, selectedRepos }), + createBranchQuery(ref), +]); + +export const buildGlobSearchQuery = ({ + pattern, + path, + repo, + ref, + selectedRepos, +}: GlobSearchQuery): QueryIR => combineQueries([ + createRegexpQuery({ + regexp: globToFileRegexp(pattern), + fileName: true, + content: false, + }), + path ? createRegexpQuery({ + regexp: escapeRE2Regexp(path), + fileName: true, + content: false, + }) : undefined, + createRepoScopeQuery({ repo, selectedRepos }), + createBranchQuery(ref), +]);