feat(pi-utils): added occurrence previews for ambiguous patch matches
- Enhanced patch application to handle duplicate content with occurrence previews. - Added AsyncQueue class for improved async stream management in ptree. - Improved error messages with line previews for ambiguous text matches. - Added occurrence tracking properties to MatchOutcome interface.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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<string, Array<{ patternIdx: number; actualIdx: number }>>();
|
||||
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<string, string[]>();
|
||||
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<number>();
|
||||
// Track which actual lines we've used to handle duplicate content correctly
|
||||
const usedActualLines = new Map<string, number>(); // 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.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.`,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -390,8 +390,11 @@ export class EditTool implements AgentTool<TInput> {
|
||||
});
|
||||
|
||||
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.`,
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
+129
-15
@@ -16,6 +16,59 @@ const isWindows = process.platform === "win32";
|
||||
// Set of live children for managed termination/cleanup on shutdown.
|
||||
const managedChildren = new Set<PipedSubprocess>();
|
||||
|
||||
class AsyncQueue<T> {
|
||||
#items: T[] = [];
|
||||
#resolvers: Array<(result: IteratorResult<T>) => 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<IteratorResult<T>> {
|
||||
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<IteratorResult<T>>((resolve) => {
|
||||
this.#resolvers.push(resolve);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
function createProcessStream(queue: AsyncQueue<Uint8Array>): ReadableStream<Uint8Array> {
|
||||
const stream = new ReadableStream<Uint8Array>({
|
||||
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<Uint8Array<ArrayBuffer>>;
|
||||
#stderrBuffer = "";
|
||||
#stdoutQueue = new AsyncQueue<Uint8Array>();
|
||||
#stderrQueue = new AsyncQueue<Uint8Array>();
|
||||
#stdoutStream?: ReadableStream<Uint8Array>;
|
||||
#stderrStream?: ReadableStream<Uint8Array>;
|
||||
#exitReason?: Exception;
|
||||
#exitReasonPending?: Exception;
|
||||
#exited: Promise<void>;
|
||||
@@ -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<Exception | undefined>();
|
||||
|
||||
@@ -152,11 +260,17 @@ export class ChildProcess {
|
||||
get stdin(): FileSink | undefined {
|
||||
return this.#proc.stdin;
|
||||
}
|
||||
get stdout(): ReadableStream<Uint8Array<ArrayBuffer>> {
|
||||
return this.#proc.stdout;
|
||||
get stdout(): ReadableStream<Uint8Array> {
|
||||
if (!this.#stdoutStream) {
|
||||
this.#stdoutStream = createProcessStream(this.#stdoutQueue);
|
||||
}
|
||||
return this.#stdoutStream;
|
||||
}
|
||||
get stderr(): ReadableStream<Uint8Array<ArrayBuffer>> {
|
||||
return this.#stderrTee;
|
||||
get stderr(): ReadableStream<Uint8Array> {
|
||||
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<Blob>();
|
||||
|
||||
const blob = this.#proc.stdout.blob();
|
||||
const blob = this.stdout.blob();
|
||||
if (!this.#nothrow) {
|
||||
this.#exited.catch((ex: Exception) => {
|
||||
reject(ex);
|
||||
|
||||
Reference in New Issue
Block a user