fix(step): resolve search rows structurally instead of re-parsing rendered text

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
+	 * `:<digits>:` 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 ":<digits>:" 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 `:<digits>:` 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 ":<digits>:" 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 `<cmd> ... -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 ":<digits>:" 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(

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 ":<digits>:"
+		// 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 ":<digits>:" 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,

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 :<digits>: shape", async () => {
+		// The mirror case: a file literally named with a ":<digits>:" 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 :<digits>: 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) =>