diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1a07c7d6f..6576d688c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,8 +1,12 @@ # Changelog ## [Unreleased] + ### Fixed +- Improved error messages when multiple text occurrences are found by showing line previews and context +- Enhanced patch application to better handle duplicate content in context lines +- Added occurrence previews to help users disambiguate between multiple matches - Fixed cache invalidation for streaming edits to prevent stale data - Fixed file existence check for prompt templates directory - Fixed bash output streaming to prevent premature stream closure diff --git a/packages/coding-agent/src/core/tools/patch/applicator.ts b/packages/coding-agent/src/core/tools/patch/applicator.ts index acbffaaa9..83e4af569 100644 --- a/packages/coding-agent/src/core/tools/patch/applicator.ts +++ b/packages/coding-agent/src/core/tools/patch/applicator.ts @@ -92,17 +92,17 @@ function adjustLinesIndentation(patternLines: string[], actualLines: string[], n } } - // Build a map from trimmed content to available (pattern index, actual index) pairs - // This lets us find context lines and their corresponding actual content - const contentToIndices = new Map>(); - for (let i = 0; i < Math.min(patternLines.length, actualLines.length); i++) { - const trimmed = patternLines[i].trim(); + // Build a map from trimmed content to actual lines (by content, not position) + // This handles fuzzy matches where pattern and actual may not be positionally aligned + const contentToActualLines = new Map(); + for (const line of actualLines) { + const trimmed = line.trim(); if (trimmed.length === 0) continue; - const arr = contentToIndices.get(trimmed); + const arr = contentToActualLines.get(trimmed); if (arr) { - arr.push({ patternIdx: i, actualIdx: i }); + arr.push(line); } else { - contentToIndices.set(trimmed, [{ patternIdx: i, actualIdx: i }]); + contentToActualLines.set(trimmed, [line]); } } @@ -119,8 +119,8 @@ function adjustLinesIndentation(patternLines: string[], actualLines: string[], n } const avgDelta = deltaCount > 0 ? Math.round(totalDelta / deltaCount) : 0; - // Track which indices we've used to handle duplicate content correctly - const usedIndices = new Set(); + // Track which actual lines we've used to handle duplicate content correctly + const usedActualLines = new Map(); // trimmed content -> count used return newLines.map((newLine) => { if (newLine.trim().length === 0) { @@ -128,16 +128,15 @@ function adjustLinesIndentation(patternLines: string[], actualLines: string[], n } const trimmed = newLine.trim(); - const indices = contentToIndices.get(trimmed); + const matchingActualLines = contentToActualLines.get(trimmed); - // Check if this is a context line (same trimmed content exists in pattern) - if (indices) { - for (const { patternIdx, actualIdx } of indices) { - if (!usedIndices.has(patternIdx)) { - usedIndices.add(patternIdx); - // Use actual file content directly for context lines - return actualLines[actualIdx]; - } + // Check if this is a context line (same trimmed content exists in actual) + if (matchingActualLines && matchingActualLines.length > 0) { + const usedCount = usedActualLines.get(trimmed) ?? 0; + if (usedCount < matchingActualLines.length) { + usedActualLines.set(trimmed, usedCount + 1); + // Use actual file content directly for context lines + return matchingActualLines[usedCount]; } } @@ -599,9 +598,11 @@ function applyCharacterMatch( // Check for multiple exact occurrences if (matchOutcome.occurrences && matchOutcome.occurrences > 1) { + const previews = matchOutcome.occurrencePreviews?.join("\n\n") ?? ""; + const moreMsg = matchOutcome.occurrences > 5 ? ` (showing first 5 of ${matchOutcome.occurrences})` : ""; throw new ApplyPatchError( - `Found ${matchOutcome.occurrences} occurrences of the text in ${path}. ` + - `The text must be unique. Please provide more context to make it unique.`, + `Found ${matchOutcome.occurrences} occurrences in ${path}${moreMsg}:\n\n${previews}\n\n` + + `Add more context lines to disambiguate.`, ); } @@ -857,9 +858,22 @@ function computeReplacements( if (hunk.changeContext === undefined && !hunk.hasContextLines && !hunk.isEndOfFile && lineHint === undefined) { const secondMatch = seekSequence(originalLines, pattern, found + 1, false, { allowFuzzy }); if (secondMatch.index !== undefined) { + // Extract 3-line previews for each match + const formatPreview = (startIdx: number) => { + const lines = originalLines.slice(startIdx, startIdx + 3); + return lines + .map((line, i) => { + const num = startIdx + i + 1; + const truncated = line.length > 60 ? `${line.slice(0, 57)}...` : line; + return ` ${num} | ${truncated}`; + }) + .join("\n"); + }; + const preview1 = formatPreview(found); + const preview2 = formatPreview(secondMatch.index); throw new ApplyPatchError( - `Found 2 occurrences of the text in ${path}. ` + - `The text must be unique. Please provide more context to make it unique.`, + `Found 2 occurrences in ${path}:\n\n${preview1}\n\n${preview2}\n\n` + + `Add more context lines to disambiguate.`, ); } } diff --git a/packages/coding-agent/src/core/tools/patch/diff.ts b/packages/coding-agent/src/core/tools/patch/diff.ts index 7cac34ac0..5636c984a 100644 --- a/packages/coding-agent/src/core/tools/patch/diff.ts +++ b/packages/coding-agent/src/core/tools/patch/diff.ts @@ -228,9 +228,11 @@ export function replaceText(content: string, oldText: string, newText: string, o }); if (matchOutcome.occurrences && matchOutcome.occurrences > 1) { + const previews = matchOutcome.occurrencePreviews?.join("\n\n") ?? ""; + const moreMsg = matchOutcome.occurrences > 5 ? ` (showing first 5 of ${matchOutcome.occurrences})` : ""; throw new Error( - `Found ${matchOutcome.occurrences} occurrences of the text. ` + - `The text must be unique. Please provide more context to make it unique, or use all: true to replace all.`, + `Found ${matchOutcome.occurrences} occurrences${moreMsg}:\n\n${previews}\n\n` + + `Add more context lines to disambiguate.`, ); } @@ -307,8 +309,10 @@ export async function computeEditDiff( }); if (matchOutcome.occurrences && matchOutcome.occurrences > 1) { + const previews = matchOutcome.occurrencePreviews?.join("\n\n") ?? ""; + const moreMsg = matchOutcome.occurrences > 5 ? ` (showing first 5 of ${matchOutcome.occurrences})` : ""; return { - error: `Found ${matchOutcome.occurrences} occurrences of the text in ${path}. The text must be unique. Please provide more context to make it unique, or use all: true to replace all.`, + error: `Found ${matchOutcome.occurrences} occurrences in ${path}${moreMsg}:\n\n${previews}\n\nAdd more context lines to disambiguate.`, }; } diff --git a/packages/coding-agent/src/core/tools/patch/fuzzy.ts b/packages/coding-agent/src/core/tools/patch/fuzzy.ts index d56722cc8..58a0d7280 100644 --- a/packages/coding-agent/src/core/tools/patch/fuzzy.ts +++ b/packages/coding-agent/src/core/tools/patch/fuzzy.ts @@ -215,7 +215,25 @@ export function findMatch( if (exactIndex !== -1) { const occurrences = content.split(target).length - 1; if (occurrences > 1) { - return { occurrences }; + // Find line numbers and previews for each occurrence (up to 5) + const contentLines = content.split("\n"); + const occurrenceLines: number[] = []; + const occurrencePreviews: string[] = []; + let searchStart = 0; + for (let i = 0; i < 5; i++) { + const idx = content.indexOf(target, searchStart); + if (idx === -1) break; + const lineNumber = content.slice(0, idx).split("\n").length; + occurrenceLines.push(lineNumber); + // Extract 3 lines starting from match (0-indexed) + const previewLines = contentLines.slice(lineNumber - 1, lineNumber + 2); + const preview = previewLines + .map((line, i) => ` ${lineNumber + i} | ${line.length > 60 ? `${line.slice(0, 57)}...` : line}`) + .join("\n"); + occurrencePreviews.push(preview); + searchStart = idx + 1; + } + return { occurrences, occurrenceLines, occurrencePreviews }; } const startLine = content.slice(0, exactIndex).split("\n").length; return { diff --git a/packages/coding-agent/src/core/tools/patch/index.ts b/packages/coding-agent/src/core/tools/patch/index.ts index 13769f756..a339ea753 100644 --- a/packages/coding-agent/src/core/tools/patch/index.ts +++ b/packages/coding-agent/src/core/tools/patch/index.ts @@ -390,8 +390,11 @@ export class EditTool implements AgentTool { }); if (matchOutcome.occurrences && matchOutcome.occurrences > 1) { + const previews = matchOutcome.occurrencePreviews?.join("\n\n") ?? ""; + const moreMsg = matchOutcome.occurrences > 5 ? ` (showing first 5 of ${matchOutcome.occurrences})` : ""; throw new Error( - `Found ${matchOutcome.occurrences} occurrences of the text in ${path}. The text must be unique. Please provide more context to make it unique, or use all: true to replace all.`, + `Found ${matchOutcome.occurrences} occurrences in ${path}${moreMsg}:\n\n${previews}\n\n` + + `Add more context lines to disambiguate.`, ); } diff --git a/packages/coding-agent/src/core/tools/patch/types.ts b/packages/coding-agent/src/core/tools/patch/types.ts index be2c615c6..e562aa7e2 100644 --- a/packages/coding-agent/src/core/tools/patch/types.ts +++ b/packages/coding-agent/src/core/tools/patch/types.ts @@ -40,6 +40,10 @@ export interface MatchOutcome { closest?: FuzzyMatch; /** Number of occurrences if multiple exact matches found */ occurrences?: number; + /** Line numbers where occurrences were found (1-indexed) */ + occurrenceLines?: number[]; + /** Preview snippets for each occurrence (up to 5) */ + occurrencePreviews?: string[]; /** Number of fuzzy matches above threshold */ fuzzyMatches?: number; } diff --git a/packages/pi-utils/src/ptree.ts b/packages/pi-utils/src/ptree.ts index b1f74de03..033655030 100644 --- a/packages/pi-utils/src/ptree.ts +++ b/packages/pi-utils/src/ptree.ts @@ -16,6 +16,59 @@ const isWindows = process.platform === "win32"; // Set of live children for managed termination/cleanup on shutdown. const managedChildren = new Set(); +class AsyncQueue { + #items: T[] = []; + #resolvers: Array<(result: IteratorResult) => void> = []; + #closed = false; + + push(item: T): void { + if (this.#closed) return; + const resolver = this.#resolvers.shift(); + if (resolver) { + resolver({ value: item, done: false }); + return; + } + this.#items.push(item); + } + + close(): void { + if (this.#closed) return; + this.#closed = true; + while (this.#resolvers.length > 0) { + const resolver = this.#resolvers.shift(); + if (resolver) { + resolver({ value: undefined, done: true }); + } + } + } + + async next(): Promise> { + if (this.#items.length > 0) { + return { value: this.#items.shift() as T, done: false }; + } + if (this.#closed) { + return { value: undefined, done: true }; + } + return await new Promise>((resolve) => { + this.#resolvers.push(resolve); + }); + } +} + +function createProcessStream(queue: AsyncQueue): ReadableStream { + const stream = new ReadableStream({ + pull: async (controller) => { + const result = await queue.next(); + if (result.done) { + controller.close(); + return; + } + controller.enqueue(result.value); + }, + }); + return stream; +} + /** * Kill a child process and its descendents. * - Windows: uses taskkill for tree and forceful kill (/T /F) @@ -82,8 +135,11 @@ export class ChildProcess { #proc: PipedSubprocess; #detached = false; #nothrow = false; - #stderrTee: ReadableStream>; #stderrBuffer = ""; + #stdoutQueue = new AsyncQueue(); + #stderrQueue = new AsyncQueue(); + #stdoutStream?: ReadableStream; + #stderrStream?: ReadableStream; #exitReason?: Exception; #exitReasonPending?: Exception; #exited: Promise; @@ -92,23 +148,75 @@ export class ChildProcess { constructor(proc: PipedSubprocess) { registerManaged(proc); - const [left, right] = proc.stderr.tee(); - this.#stderrTee = right; + const exitSettled = proc.exited.then( + () => {}, + () => {}, + ); + + // Capture stdout at all times. Close the passthrough when the process exits. + void (async () => { + const reader = proc.stdout.getReader(); + try { + while (true) { + const result = await Promise.race([ + reader.read(), + exitSettled.then(() => ({ done: true, value: undefined as Uint8Array | undefined })), + ]); + if (result.done) break; + if (!result.value) continue; + this.#stdoutQueue.push(result.value); + } + } catch { + // ignore + } finally { + try { + await reader.cancel(); + } catch {} + try { + reader.releaseLock(); + } catch {} + this.#stdoutQueue.close(); + } + })().catch(() => { + this.#stdoutQueue.close(); + }); // Capture stderr at all times, with a capped buffer for errors. const decoder = new TextDecoder(); void (async () => { - for await (const chunk of left) { - this.#stderrBuffer += decoder.decode(chunk, { stream: true }); + const reader = proc.stderr.getReader(); + try { + while (true) { + const result = await Promise.race([ + reader.read(), + exitSettled.then(() => ({ done: true, value: undefined as Uint8Array | undefined })), + ]); + if (result.done) break; + if (!result.value) continue; + this.#stderrQueue.push(result.value); + this.#stderrBuffer += decoder.decode(result.value, { stream: true }); + if (this.#stderrBuffer.length > NonZeroExitError.MAX_TRACE) { + this.#stderrBuffer = this.#stderrBuffer.slice(-NonZeroExitError.MAX_TRACE); + } + } + } catch { + // ignore + } finally { + this.#stderrBuffer += decoder.decode(); if (this.#stderrBuffer.length > NonZeroExitError.MAX_TRACE) { this.#stderrBuffer = this.#stderrBuffer.slice(-NonZeroExitError.MAX_TRACE); } + try { + await reader.cancel(); + } catch {} + try { + reader.releaseLock(); + } catch {} + this.#stderrQueue.close(); } - this.#stderrBuffer += decoder.decode(); - if (this.#stderrBuffer.length > NonZeroExitError.MAX_TRACE) { - this.#stderrBuffer = this.#stderrBuffer.slice(-NonZeroExitError.MAX_TRACE); - } - })().catch(() => {}); + })().catch(() => { + this.#stderrQueue.close(); + }); const { promise, resolve } = Promise.withResolvers(); @@ -152,11 +260,17 @@ export class ChildProcess { get stdin(): FileSink | undefined { return this.#proc.stdin; } - get stdout(): ReadableStream> { - return this.#proc.stdout; + get stdout(): ReadableStream { + if (!this.#stdoutStream) { + this.#stdoutStream = createProcessStream(this.#stdoutQueue); + } + return this.#stdoutStream; } - get stderr(): ReadableStream> { - return this.#stderrTee; + get stderr(): ReadableStream { + if (!this.#stderrStream) { + this.#stderrStream = createProcessStream(this.#stderrQueue); + } + return this.#stderrStream; } /** @@ -227,7 +341,7 @@ export class ChildProcess { async blob() { const { promise, resolve, reject } = Promise.withResolvers(); - const blob = this.#proc.stdout.blob(); + const blob = this.stdout.blob(); if (!this.#nothrow) { this.#exited.catch((ex: Exception) => { reject(ex);