diff --git a/packages/coding-agent/src/core/tools/grep.ts b/packages/coding-agent/src/core/tools/grep.ts index 4aa149d5..e644806b 100644 --- a/packages/coding-agent/src/core/tools/grep.ts +++ b/packages/coding-agent/src/core/tools/grep.ts @@ -49,6 +49,22 @@ export interface GrepToolDetails { matchLimitReached?: number; linesTruncated?: boolean; backend?: SearchBackendName; + /** + * Structured match rows, for consumers that need the exact file/line rather + * than the rendered text. The backends already know these values; without + * this they were flattened into `path:line: text` and had to be parsed back + * out, which silently mis-reads any row whose path or text itself contains a + * `::` shape. Never sent to the LLM (details are local metadata). + */ + matchRows?: GrepMatchRow[]; +} + +/** One matched row in the shape consumers can act on without re-parsing text. */ +export interface GrepMatchRow { + /** Path relative to the search root, using forward slashes. */ + path: string; + line: number; + text: string; } /** Match tuple produced by every search backend; the caller formats it. */ @@ -293,22 +309,60 @@ function createRipgrepBackend(rgPath: string): SearchBackend { } /** - * Parse a `path:line:content` (or `path\0line\0content`) triple emitted by - * git grep / POSIX grep -n. Returns null for lines that do not fit the shape. - * NUL-separated form is preferred; grep here does not enable it, so parsing - * uses `path:line:rest` with the first "::" delimiter. Filenames - * containing that literal sequence are the only case that mis-parses; skipping - * mis-parses beats propagating them as false matches. + * Parse one record emitted by git grep / POSIX grep. + * + * Both backends can separate the path from the rest with a NUL byte, which is + * the only form that stays unambiguous when a path or a matched line itself + * contains a `::` shape. We ask for NUL output from both: + * + * - `git grep -z` emits `path\0line\0text` and escapes special bytes in the + * path into the Unicode private use area (`0xF000 + byte`), so `a:b.ts` + * arrives as `a\uF03Ab.ts`; that is decoded back here. + * - `grep -Z` emits `path\0line:text` with the path verbatim. + * + * The legacy colon form is still accepted for custom backends that emit it. + * The NUL forms are tried first: they carry the field boundary, so no guessing + * is involved. */ function parseGrepLine(line: string): GrepMatch | null { + const nulParts = line.split("\0"); + if (nulParts.length >= 3) { + // git grep -z: path\0line\0text + const [rawPath, lineNumStr, ...rest] = nulParts; + const match = finishGrepMatch(decodeGitPath(rawPath ?? ""), lineNumStr ?? "", rest.join("\0")); + if (match) return match; + } + if (nulParts.length === 2) { + // grep -Z: path\0line:text + const [rawPath, tail] = nulParts; + const separator = (tail ?? "").indexOf(":"); + if (separator > 0) { + return finishGrepMatch(rawPath ?? "", (tail ?? "").slice(0, separator), (tail ?? "").slice(separator + 1)); + } + } + // Legacy colon form: `path:line:text`, first "::" delimiter wins. const match = line.match(/^([^\0]+?):(\d+):(.*)$/); if (!match) return null; const [, filePath, lineNumStr, lineText] = match; + return finishGrepMatch(filePath ?? "", lineNumStr ?? "", lineText ?? ""); +} + +function finishGrepMatch(filePath: string, lineNumStr: string, lineText: string): GrepMatch | null { + if (!filePath) return null; const lineNumber = Number.parseInt(lineNumStr, 10); if (!Number.isInteger(lineNumber) || lineNumber < 1) return null; return { filePath, lineNumber, lineText }; } +/** + * Undo the path escaping `git grep -z` applies: bytes that would be ambiguous in + * its output are shifted into the private use area as `0xF000 + byte`. + */ +function decodeGitPath(raw: string): string { + if (!/[\uF000-\uF0FF]/u.test(raw)) return raw; + return raw.replace(/[\uF000-\uF0FF]/gu, (char) => String.fromCharCode(char.charCodeAt(0) - 0xf000)); +} + /** Common driver for a ` ... -n` fallback backend that streams `path:line:content`. */ function runLinePrefixedSearch( backendName: SearchBackendName, @@ -416,7 +470,7 @@ function createGitGrepBackend(): SearchBackend { return { matches: [], matchLimitReached: false }; } - const args: string[] = ["grep", "-n", "-I", "--no-color", "--untracked"]; + const args: string[] = ["grep", "-n", "-I", "--no-color", "--untracked", "-z"]; if (params.ignoreCase) args.push("-i"); if (params.literal) args.push("-F"); else args.push("-E"); @@ -443,11 +497,26 @@ function globMatch(input: string, pattern: string): boolean { return minimatch(input, pattern, { dot: true }); } +/** + * GNU grep understands `-Z` (filename followed by NUL); BSD grep does not. + * The distinction matters only for ambiguity in the path, so we gate on the + * implementations that are actually GNU-compatible rather than probing. + */ +function posixGrepSupportsNulPath(): boolean { + return process.platform !== "darwin" && process.platform !== "freebsd"; +} + function createPosixGrepBackend(): SearchBackend { return { name: "grep", async run(params) { const args = ["-r", "-n", "-H", "-I"]; + // GNU grep: -Z writes the filename verbatim, NUL-terminated, so a path + // containing "::" can never be mistaken for the line separator. + // BSD grep (macOS) lacks -Z; there the legacy "path:line:text" form is + // parsed instead (see parseGrepLine), so the flag is best-effort. + const supportsNulPath = posixGrepSupportsNulPath(); + if (supportsNulPath) args.push("-Z"); if (params.ignoreCase) args.push("-i"); if (params.literal) args.push("-F"); else args.push("-E"); @@ -589,23 +658,34 @@ export function createGrepToolDefinition( return { content: [{ type: "text", text: "No matches found" }], details: undefined }; } + // Rows in the exact shape the render path uses, so a consumer reading + // these gets the same numbers the rendered text shows — without having + // to parse that text back out. + const matchRows: GrepMatchRow[] = []; + for (const match of matches) { + const relativePath = formatPath(match.filePath); if (contextValue === 0 && match.lineText !== undefined) { - const relativePath = formatPath(match.filePath); const sanitized = match.lineText.replace(/\r\n/g, "\n").replace(/\r/g, "").replace(/\n$/, ""); const { text: truncatedText, wasTruncated } = truncateLine(sanitized); if (wasTruncated) linesTruncated = true; outputLines.push(`${relativePath}:${match.lineNumber}: ${truncatedText}`); + matchRows.push({ path: relativePath, line: match.lineNumber, text: truncatedText }); } else { const block = await formatBlock(match.filePath, match.lineNumber); outputLines.push(...block); + matchRows.push({ + path: relativePath, + line: match.lineNumber, + text: match.lineText !== undefined ? match.lineText.replace(/\r/g, "").replace(/\n$/, "") : "", + }); } } const rawOutput = outputLines.join("\n"); const truncation = truncateHead(rawOutput, { maxLines: Number.MAX_SAFE_INTEGER }); let output = truncation.content; - const details: GrepToolDetails = { backend: backend.name }; + const details: GrepToolDetails = { backend: backend.name, matchRows }; const notices: string[] = []; if (matchLimitReached) { notices.push( diff --git a/packages/coding-agent/src/step/tool-profile.ts b/packages/coding-agent/src/step/tool-profile.ts index fe04f4be..5d07e4ca 100644 --- a/packages/coding-agent/src/step/tool-profile.ts +++ b/packages/coding-agent/src/step/tool-profile.ts @@ -705,12 +705,41 @@ async function executeFindFiles( type SearchRow = { display: string; line: number; text: string }; +/** + * Read the structured match rows the grep tool reports in `details.matchRows`. + * Returns undefined when the backend did not provide them (custom/remote search + * operations), so the caller can fall back to parsing the rendered text. + */ +function searchRowsFromDetails(result: AnyResult): SearchRow[] | undefined { + const details = result.details; + if (!details || typeof details !== "object") return undefined; + const matches = (details as { matchRows?: unknown }).matchRows; + if (!Array.isArray(matches) || matches.length === 0) return undefined; + const rows: SearchRow[] = []; + for (const entry of matches) { + if (!entry || typeof entry !== "object") return undefined; + const { path: display, line, text } = entry as { path?: unknown; line?: unknown; text?: unknown }; + if (typeof display !== "string" || typeof line !== "number" || !Number.isFinite(line)) return undefined; + rows.push({ display, line, text: typeof text === "string" ? text : "" }); + } + return rows; +} + +/** + * Fallback for backends that do not report structured rows (custom/remote + * search operations). The native grep tool supplies `details.matches`, so this + * only runs when those are absent; the lazy path group is the best that can be + * done from rendered text, which is exactly why the structured form is primary. + */ function parseNativeSearchRows(text: string): SearchRow[] { const rows: SearchRow[] = []; for (const rawLine of text.split("\n")) { const line = rawLine.trimEnd(); // Match the row shape, not a prefix: "[" and "WARNING:" can start valid paths. - const match = /^(.*):(\d+): (.*)$/u.exec(line); + // The path group must stay lazy: matched text can itself contain "::" + // shapes (compiler errors, timestamps), and a greedy path would swallow up to + // the last one, mis-reading the line number and reloading the wrong line. + const match = /^(.*?):(\d+): (.*)$/u.exec(line); if (match) { rows.push({ display: match[1] ?? "", line: Number(match[2]), text: match[3] ?? "" }); } @@ -817,7 +846,10 @@ async function executeSearchFiles( } const result = execution.result; - const rows = parseNativeSearchRows(textFromResult(result)); + // Prefer the structured rows the grep tool already resolved. Falling back to + // re-parsing the rendered text is what silently mis-reads rows whose path or + // matched text itself contains a "::" shape. + const rows = searchRowsFromDetails(result) ?? parseNativeSearchRows(textFromResult(result)); const searchRoot = resolveToCwd(args.path ?? ".", effectiveCwd); let searchRootIsFile = false; try { @@ -847,7 +879,11 @@ async function executeSearchFiles( content: [{ type: "text", text: limited.text }], details: { ...details, + // `matches` stays the rendered match count (pre-existing contract); + // `matchRows` carries the structured rows the grep tool resolved, in the + // same shape so consumers have one format to read. matches: rendered.matches, + matchRows: rows.map((row) => ({ path: row.display, line: row.line, text: row.text })), filesMatched: rendered.filesMatched, truncated: nativeCapped || limited.truncated, timedOut: false, diff --git a/packages/coding-agent/test/step-tool-profile.test.ts b/packages/coding-agent/test/step-tool-profile.test.ts index 8ed3581c..8447b024 100644 --- a/packages/coding-agent/test/step-tool-profile.test.ts +++ b/packages/coding-agent/test/step-tool-profile.test.ts @@ -1,3 +1,4 @@ +import { execFileSync } from "node:child_process"; import { mkdir, mkdtemp, readFile, rm, utimes, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -313,6 +314,80 @@ describe("Step tool profile", () => { } }); + it("keeps the real line number when the *path* contains a :: shape", async () => { + // The mirror case: a file literally named with a "::" sequence. + // Windows forbids ":" in filenames, so this only runs where it can exist. + if (process.platform === "win32") return; + const root = await mkdtemp(join(tmpdir(), "step-tool-profile-")); + try { + // Make this a work tree so the git backend stays available as the + // fallback when ripgrep is not installed: its NUL output carries the + // path boundary, while a platform grep without -Z would not. + execFileSync("git", ["init", "-q"], { cwd: root, stdio: "ignore" }); + const lines = Array.from({ length: 10 }, (_, index) => `line ${index + 1}`); + lines[2] = "MISMATCH_TARGET"; + await writeFile(join(root, "report:7:notes.txt"), `${lines.join("\n")}\n`); + const search = createStepToolProfile(root).find((tool) => tool.name === "search_files")!; + const result = await search.execute( + "search-colon-path", + { pattern: "MISMATCH_TARGET", path: "." }, + undefined, + undefined, + undefined as never, + ); + // Line 3 is the match. Reading 7 as the line number would surface line 7. + expect(text(result)).toContain("report:7:notes.txt:3: MISMATCH_TARGET"); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + it("reports structured match rows so consumers never re-parse rendered text", async () => { + const root = await mkdtemp(join(tmpdir(), "step-tool-profile-")); + try { + const lines = Array.from({ length: 45 }, (_, index) => `filler line ${index + 1}`); + lines[0] = "src/example.ts:42: build failed"; + await writeFile(join(root, "many.log"), `${lines.join("\n")}\n`); + const search = createStepToolProfile(root).find((tool) => tool.name === "search_files")!; + const result = await search.execute( + "search-structured", + { pattern: "build failed", path: "many.log" }, + undefined, + undefined, + undefined as never, + ); + const rows = (result.details as { matchRows?: Array<{ path: string; line: number; text: string }> }).matchRows; + expect(rows).toEqual([{ path: "many.log", line: 1, text: "src/example.ts:42: build failed" }]); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + it("keeps the real line number when matched text contains a :: shape", async () => { + const root = await mkdtemp(join(tmpdir(), "step-tool-profile-")); + try { + // Line 1 carries a ":42:" shape (a compiler error echoed into a log) and + // line 42 is unrelated filler. A greedy row parse reads 42 as the line + // number and reloads line 42 instead of line 1. + const lines = Array.from({ length: 45 }, (_, index) => `filler line ${index + 1}`); + lines[0] = "src/example.ts:42: build failed"; + lines[41] = "UNRELATED_LINE_42"; + await writeFile(join(root, "many.log"), `${lines.join("\n")}\n`); + const search = createStepToolProfile(root).find((tool) => tool.name === "search_files")!; + const result = await search.execute( + "search-colon-shape", + { pattern: "build failed", path: "many.log" }, + undefined, + undefined, + undefined as never, + ); + expect(text(result)).toContain("many.log:1: src/example.ts:42: build failed"); + expect(text(result)).not.toContain("UNRELATED_LINE_42"); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + it.each( ["[slug]", "[...slug]", "[[...slug]]"].flatMap((route) => [undefined, "routes"].flatMap((searchPath) =>