From 570e0187afa44dddb8410368f36a7c9adc4ae4d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E7=A7=80=E5=A5=87?= Date: Wed, 21 Oct 2026 01:19:29 +0800 Subject: [PATCH] fix(step): resolve search rows structurally instead of re-parsing rendered text search_files could return the wrong source text when a matched line itself contains a :: shape. The native grep tool resolves path, line and text as structured values, then the tool boundary flattens them into "path:line: text" and the Step adapter parses that string back into a row. Its greedy path group re-read the :: shape inside the matched text as the line number and reloaded that line, or reported "(no matches)" when the false line number was past EOF. Tuning that regex only moves the ambiguity to paths, since both fields may contain the delimiter. So the re-parse is removed instead: - grep reports the rows it already resolved in details.matchRows (details are extension metadata and are not sent to the model). - search_files reads those rows and only falls back to parsing rendered text for custom/remote search operations that emit plain text. - the git and POSIX grep backends request NUL-separated output (-z / -Z) so their own parse carries the field boundary. git grep -z escapes ambiguous path bytes into the private use area (0xF000 + byte), decoded back here; grep -Z is GNU-only, so it is gated and the colon form stays supported. Both directions are now correct: matched text containing ::, and a path containing it. --- packages/coding-agent/src/core/tools/grep.ts | 98 +++++++++++++++++-- .../coding-agent/src/step/tool-profile.ts | 40 +++++++- .../test/step-tool-profile.test.ts | 75 ++++++++++++++ 3 files changed, 202 insertions(+), 11 deletions(-) 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) =>