fix(coding-agent): fixed edit pipeline silent-corruption paths

non-exact patch matches warn and prefix/substring matches must preserve the discarded suffix; multi-entry edits stop at first failure and report applied vs not; ast-edit and file-mention snapshots use canonical realpath keys and re-record post-apply; notebook marker-shaped lines escaped on render; fuzzy matcher pre-normalizes once per seek; streaming preview caches text+tree per tick.
This commit is contained in:
can1357
2026-06-10 01:27:40 +02:00
parent fb9eae19bb
commit f0b6608aff
10 changed files with 243 additions and 38 deletions
@@ -8,7 +8,26 @@
import type { BlockResolver } from "@oh-my-pi/hashline";
import { blockRangeAt } from "@oh-my-pi/pi-natives";
/**
* `blockRangeAt` runs a full synchronous tree-sitter parse of `text` per
* call, and streaming previews re-resolve the same (text, line) every
* streamed chunk. Memoize by content: identical text + line always yields the
* same span. FIFO-bounded; hashing the text is orders of magnitude cheaper
* than re-parsing it.
*/
const resolutionCache = new Map<string, { start: number; end: number } | null>();
const RESOLUTION_CACHE_MAX = 512;
export const nativeBlockResolver: BlockResolver = ({ path, text, line }) => {
const key = `${Bun.hash(text).toString(36)}:${text.length}:${line}:${path}`;
const cached = resolutionCache.get(key);
if (cached !== undefined) return cached;
const range = blockRangeAt({ code: text, path, line });
return range ? { start: range.startLine, end: range.endLine } : null;
const result = range ? { start: range.startLine, end: range.endLine } : null;
if (resolutionCache.size >= RESOLUTION_CACHE_MAX) {
const oldest = resolutionCache.keys().next().value;
if (oldest !== undefined) resolutionCache.delete(oldest);
}
resolutionCache.set(key, result);
return result;
};
@@ -57,6 +57,39 @@ async function readSectionText(absolutePath: string, sectionPath: string): Promi
}
}
/**
* Streaming previews recompute on every streamed chunk; re-reading the target
* file from disk each tick dominates the cost on large files. Cache the raw
* section text keyed by mtime+size so any on-disk change invalidates
* naturally. Used by the streaming path only — the args-complete pass always
* reads fresh.
*/
const streamingTextCache = new Map<string, { mtimeMs: number; size: number; rawContent: string }>();
const STREAMING_TEXT_CACHE_MAX = 8;
async function readSectionTextCached(absolutePath: string, sectionPath: string): Promise<string> {
let stamp: { mtimeMs: number; size: number } | undefined;
try {
const stat = await Bun.file(absolutePath).stat();
stamp = { mtimeMs: stat.mtimeMs, size: stat.size };
} catch {
stamp = undefined;
}
if (stamp) {
const cached = streamingTextCache.get(absolutePath);
if (cached && cached.mtimeMs === stamp.mtimeMs && cached.size === stamp.size) return cached.rawContent;
}
const rawContent = await readSectionText(absolutePath, sectionPath);
if (stamp) {
if (streamingTextCache.size >= STREAMING_TEXT_CACHE_MAX && !streamingTextCache.has(absolutePath)) {
const oldest = streamingTextCache.keys().next().value;
if (oldest !== undefined) streamingTextCache.delete(oldest);
}
streamingTextCache.set(absolutePath, { mtimeMs: stamp.mtimeMs, size: stamp.size, rawContent });
}
return rawContent;
}
function hasAnchorScopedEdit(edits: readonly Edit[]): boolean {
return edits.some(edit => {
if (edit.kind === "delete") return true;
@@ -220,7 +253,9 @@ export async function computeHashlineSectionDiff(
): Promise<{ diff: string; firstChangedLine: number | undefined } | { error: string }> {
try {
const absolutePath = resolveToCwd(section.path, cwd);
const rawContent = await readSectionText(absolutePath, section.path);
const rawContent = options.streaming
? await readSectionTextCached(absolutePath, section.path)
: await readSectionText(absolutePath, section.path);
const { text: content } = stripBom(rawContent);
const normalized = normalizeToLF(content);
// Streaming favors a stable, monotonic preview over an exact unified
@@ -78,11 +78,17 @@ interface RenderedSection {
}
function formatBlockResolution(resolution: BlockResolution): string {
const op = resolution.isDelete ? "delete block" : "replace block";
const op =
resolution.op === "delete"
? "delete block"
: resolution.op === "insert_after"
? "insert after block"
: "replace block";
const lines = resolution.end - resolution.start + 1;
const span =
resolution.start === resolution.end ? `line ${resolution.start}` : `lines ${resolution.start}-${resolution.end}`;
return `${op} ${resolution.anchorLine} → resolved ${span} (${lines} line${lines === 1 ? "" : "s"})`;
const suffix = resolution.op === "insert_after" ? `; body lands after line ${resolution.end}` : "";
return `${op} ${resolution.anchorLine} → resolved ${span} (${lines} line${lines === 1 ? "" : "s"})${suffix}`;
}
function renderSection(result: PatchSectionResult, diagnostics: FileDiagnosticsResult | undefined): RenderedSection {
+16 -1
View File
@@ -238,8 +238,23 @@ async function executeSinglePathEntries(
if (text) contentTexts.push(text);
} catch (err) {
const errorText = err instanceof Error ? err.message : String(err);
contentTexts.push(`Error editing ${path}: ${errorText}`);
contentTexts.push(`Error editing ${path} (entry ${i + 1} of ${runs.length}): ${errorText}`);
if (i > 0) {
contentTexts.push(i === 1 ? `Entry 1 was already applied.` : `Entries 1-${i} were already applied.`);
}
if (i + 1 < runs.length) {
contentTexts.push(
(i + 2 === runs.length
? `Entry ${runs.length} was NOT applied`
: `Entries ${i + 2}-${runs.length} were NOT applied`) +
`; re-read the file and re-issue only the failed and unapplied entries.`,
);
}
errorCount++;
// Stop at the first failure: later entries were authored against
// line numbers/content that assumed this entry succeeded, and
// applying them after a failure compounds the damage.
break;
}
if (!isLast && onUpdate) {
@@ -40,6 +40,7 @@ import {
countLeadingWhitespace,
detectLineEnding,
getLeadingWhitespace,
normalizeForFuzzy,
normalizeToLF,
restoreLineEndings,
stripBom,
@@ -1007,6 +1008,41 @@ async function readExistingPatchFile(fileSystem: FileSystem, absolutePath: strin
}
}
/**
* A prefix/substring strategy matched pattern lines that cover only part of
* the corresponding file lines; replacing whole lines would silently drop the
* uncovered text the model never saw. Allow the replacement only when every
* discarded piece (normalized) survives somewhere in the hunk's new lines.
*/
function assertPartialMatchPreservesDiscardedText(
path: string,
pattern: string[],
matchedLines: string[],
newLines: string[],
matchStartIndex: number,
): void {
let newLinesNorm: string | undefined;
for (let j = 0; j < pattern.length; j++) {
const lineNorm = normalizeForFuzzy(matchedLines[j]);
const patternNorm = normalizeForFuzzy(pattern[j]);
if (lineNorm === patternNorm) continue;
const at = lineNorm.indexOf(patternNorm);
if (at === -1) continue;
const discardedParts = [lineNorm.slice(0, at).trim(), lineNorm.slice(at + patternNorm.length).trim()];
for (const part of discardedParts) {
if (part.length === 0) continue;
newLinesNorm ??= newLines.map(normalizeForFuzzy).join("\n");
if (!newLinesNorm.includes(part)) {
throw new ApplyPatchError(
`Refusing partial-line match in ${path} at line ${matchStartIndex + j + 1}: ` +
`the file line also contains ${JSON.stringify(part)}, which the replacement would silently drop. ` +
`Provide the complete line in the hunk.`,
);
}
}
}
}
/**
* Compute replacements needed to transform originalLines using the diff hunks.
*/
@@ -1253,6 +1289,18 @@ function computeReplacements(
if (searchResult.strategy === "fuzzy-dominant") {
const similarity = Math.round(searchResult.confidence * 100);
warnings.push(`Dominant fuzzy match selected in ${path} near line ${found + 1} (${similarity}% similar).`);
} else if (
searchResult.strategy === "comment-prefix" ||
searchResult.strategy === "prefix" ||
searchResult.strategy === "substring" ||
searchResult.strategy === "fuzzy" ||
searchResult.strategy === "character"
) {
const similarity = Math.round(searchResult.confidence * 100);
warnings.push(
`Inexact match in ${path} near line ${found + 1}: matched via ${searchResult.strategy} strategy ` +
`(${similarity}% similar). Re-read the file if the result is not what you intended.`,
);
}
// Reject if match is ambiguous (prefix/substring matching found multiple matches)
@@ -1305,6 +1353,10 @@ function computeReplacements(
continue;
}
if (searchResult.strategy === "prefix" || searchResult.strategy === "substring") {
assertPartialMatchPreservesDiscardedText(path, pattern, actualMatchedLines, newSlice, found);
}
const adjustedNewLines = adjustLinesIndentation(pattern, actualMatchedLines, newSlice);
replacements.push({ startIndex: found, oldLen: pattern.length, newLines: adjustedNewLines });
lineIndex = found + pattern.length;
+56 -22
View File
@@ -525,29 +525,45 @@ function matchesAt(lines: string[], pattern: string[], i: number, compare: (a: s
return true;
}
/** Compute average similarity score for pattern at position */
function fuzzyScoreAt(lines: string[], pattern: string[], i: number): number {
/**
* Compute average similarity score for pre-normalized pattern lines at
* position `i` of pre-normalized file lines.
*
* `minScore` is a bail threshold: when even perfect similarity on the
* remaining lines cannot lift the average to `minScore`, returns the partial
* average early (always ≤ the true score). The length-difference lower bound
* on Levenshtein distance is used to skip the DP entirely for line pairs the
* bail test already rules out.
*/
function fuzzyScoreAt(linesNorm: string[], patternNorm: string[], i: number, minScore = 0): number {
const count = patternNorm.length;
let totalScore = 0;
for (let j = 0; j < pattern.length; j++) {
const lineNorm = normalizeForFuzzy(lines[i + j]);
const patternNorm = normalizeForFuzzy(pattern[j]);
totalScore += similarity(lineNorm, patternNorm);
for (let j = 0; j < count; j++) {
const lineNorm = linesNorm[i + j];
const patNorm = patternNorm[j];
if (lineNorm === patNorm) {
totalScore += 1;
continue;
}
const remaining = count - j - 1;
const maxLen = Math.max(lineNorm.length, patNorm.length);
// similarity ≤ 1 − |lenA−lenB|/maxLen: test the bound before the DP.
const upperBound = 1 - Math.abs(lineNorm.length - patNorm.length) / maxLen;
if ((totalScore + upperBound + remaining) / count < minScore) return totalScore / count;
if (upperBound > 0) totalScore += similarity(lineNorm, patNorm);
if ((totalScore + remaining) / count < minScore) return totalScore / count;
}
return totalScore / pattern.length;
return totalScore / count;
}
/** Check if line starts with pattern (normalized) */
function lineStartsWithPattern(line: string, pattern: string): boolean {
const lineNorm = normalizeForFuzzy(line);
const patternNorm = normalizeForFuzzy(pattern);
/** Check if pre-normalized line starts with pre-normalized pattern */
function normStartsWith(lineNorm: string, patternNorm: string): boolean {
if (patternNorm.length === 0) return lineNorm.length === 0;
return lineNorm.startsWith(patternNorm);
}
/** Check if line contains pattern as significant substring */
function lineIncludesPattern(line: string, pattern: string): boolean {
const lineNorm = normalizeForFuzzy(line);
const patternNorm = normalizeForFuzzy(pattern);
/** Check if pre-normalized line contains pre-normalized pattern as significant substring */
function normIncludes(lineNorm: string, patternNorm: string): boolean {
if (patternNorm.length === 0) return lineNorm.length === 0;
if (patternNorm.length < PARTIAL_MATCH_MIN_LENGTH) return false;
if (!lineNorm.includes(patternNorm)) return false;
@@ -613,6 +629,13 @@ export function seekSequence(
const searchStart = eof && lines.length >= pattern.length ? lines.length - pattern.length : start;
const maxStart = lines.length - pattern.length;
// Fuzzy and partial passes compare normalizeForFuzzy forms; normalize the
// file and pattern once per call instead of once per candidate position.
let linesNormCache: string[] | undefined;
let patternNormCache: string[] | undefined;
const getLinesNorm = () => (linesNormCache ??= lines.map(normalizeForFuzzy));
const getPatternNorm = () => (patternNormCache ??= pattern.map(normalizeForFuzzy));
const runExactPasses = (from: number, to: number): SequenceSearchResult | undefined => {
const comparisonPasses: Array<{
compare: (a: string, b: string) => boolean;
@@ -646,17 +669,19 @@ export function seekSequence(
return undefined;
}
const linesNorm = getLinesNorm();
const patternNorm = getPatternNorm();
const partialPasses: Array<{
compare: (line: string, patternLine: string) => boolean;
compare: (lineNorm: string, patternLineNorm: string) => boolean;
confidence: number;
strategy: SequenceMatchStrategy;
}> = [
{ compare: lineStartsWithPattern, confidence: 0.965, strategy: "prefix" },
{ compare: lineIncludesPattern, confidence: 0.94, strategy: "substring" },
{ compare: normStartsWith, confidence: 0.965, strategy: "prefix" },
{ compare: normIncludes, confidence: 0.94, strategy: "substring" },
];
for (const pass of partialPasses) {
const matches = collectIndexedMatches(from, to, i => matchesAt(lines, pattern, i, pass.compare));
const matches = collectIndexedMatches(from, to, i => matchesAt(linesNorm, patternNorm, i, pass.compare));
const result = toAmbiguousMatchResult(matches, pass.confidence, pass.strategy);
if (result) {
return result;
@@ -692,9 +717,14 @@ export function seekSequence(
matchIndices: [],
};
const fuzzyLinesNorm = getLinesNorm();
const fuzzyPatternNorm = getPatternNorm();
// Positions scoring below this can neither become a fuzzy match nor affect
// the dominant-fuzzy gap test; let fuzzyScoreAt bail early on them.
const fuzzyBail = SEQUENCE_FUZZY_THRESHOLD - DOMINANT_FUZZY_DELTA;
const scoreFuzzyRange = (from: number, to: number): void => {
for (let i = from; i <= to; i++) {
const score = fuzzyScoreAt(lines, pattern, i);
const score = fuzzyScoreAt(fuzzyLinesNorm, fuzzyPatternNorm, i, fuzzyBail);
if (score >= SEQUENCE_FUZZY_THRESHOLD) {
if (fuzzyMatches.firstMatch === undefined) {
fuzzyMatches.firstMatch = i;
@@ -787,12 +817,16 @@ export function findClosestSequenceMatch(
const eof = options?.eof ?? false;
const maxStart = lines.length - pattern.length;
const searchStart = eof && lines.length >= pattern.length ? maxStart : start;
const linesNorm = lines.map(normalizeForFuzzy);
const patternNorm = pattern.map(normalizeForFuzzy);
let bestIndex: number | undefined;
let bestScore = 0;
// Passing the running best as the bail threshold is exact: a bailed
// position returns a value strictly below it, so it can never win.
for (let i = searchStart; i <= maxStart; i++) {
const score = fuzzyScoreAt(lines, pattern, i);
const score = fuzzyScoreAt(linesNorm, patternNorm, i, bestScore);
if (score > bestScore) {
bestScore = score;
bestIndex = i;
@@ -801,7 +835,7 @@ export function findClosestSequenceMatch(
if (eof && searchStart > start) {
for (let i = start; i < searchStart; i++) {
const score = fuzzyScoreAt(lines, pattern, i);
const score = fuzzyScoreAt(linesNorm, patternNorm, i, bestScore);
if (score > bestScore) {
bestScore = score;
bestIndex = i;
+22 -2
View File
@@ -21,6 +21,26 @@ export interface NotebookDocument {
}
const CELL_MARKER_RE = /^# %% \[(code|markdown|raw)\](?: cell:(\d+))?$/;
/**
* Cell source lines that would themselves parse as (possibly already-escaped)
* cell markers gain one extra `%` on render and lose it on parse, so a
* notebook that *contains* the literal text `# %% [markdown] cell:3` survives
* the editable-text round trip instead of being split into extra cells.
*/
const ESCAPABLE_MARKER_RE = /^# %%+ \[(?:code|markdown|raw)\](?: cell:\d+)?$/;
const ESCAPED_MARKER_RE = /^# %%%+ \[(?:code|markdown|raw)\](?: cell:\d+)?$/;
function escapeMarkerLikeSourceLines(source: string): string {
if (!source.includes("# %%")) return source;
return source
.split("\n")
.map(line => (ESCAPABLE_MARKER_RE.test(line) ? line.replace("# %", "# %%") : line))
.join("\n");
}
function unescapeMarkerLikeLine(line: string): string {
return ESCAPED_MARKER_RE.test(line) ? line.replace("# %%", "# %") : line;
}
export function isNotebookPath(filePath: string): boolean {
return path.extname(filePath).toLowerCase() === ".ipynb";
@@ -100,7 +120,7 @@ export async function readNotebookDocument(absolutePath: string, displayPath: st
export function notebookToEditableText(notebook: NotebookDocument): string {
return notebook.cells
.map((cell, index) => {
const source = sourceToText(cell.source);
const source = escapeMarkerLikeSourceLines(sourceToText(cell.source));
return source.length > 0
? `# %% [${cell.cell_type}] cell:${index}\n${source}`
: `# %% [${cell.cell_type}] cell:${index}`;
@@ -156,7 +176,7 @@ function parseNotebookEditableText(text: string, displayPath: string): ParsedVir
`Invalid notebook editable representation for ${displayPath}: expected first line to be "# %% [code] cell:0", "# %% [markdown] cell:0", or "# %% [raw] cell:0".`,
);
}
current.lines.push(line);
current.lines.push(unescapeMarkerLikeLine(line));
}
flush();
return cells;
+25 -5
View File
@@ -6,7 +6,7 @@ import type { Component } from "@oh-my-pi/pi-tui";
import { replaceTabs, Text } from "@oh-my-pi/pi-tui";
import { $envpos, prompt, untilAborted } from "@oh-my-pi/pi-utils";
import * as z from "zod/v4";
import { getFileSnapshotStore } from "../edit/file-snapshot-store";
import { canonicalSnapshotKey, getFileSnapshotStore } from "../edit/file-snapshot-store";
import { normalizeToLF } from "../edit/normalize";
import type { RenderResultOptions } from "../extensibility/custom-tools/types";
import type { Theme } from "../modes/theme/theme";
@@ -295,7 +295,7 @@ export class AstEditTool implements AgentTool<typeof astEditSchema, AstEditToolD
const absolutePath = path.resolve(this.session.cwd, relativePath);
try {
const fullText = normalizeToLF(await Bun.file(absolutePath).text());
const tag = snapshotStore.record(absolutePath, fullText);
const tag = snapshotStore.record(canonicalSnapshotKey(absolutePath), fullText);
hashContexts.set(relativePath, { tag });
} catch {
// Best-effort: if a file disappears between ast-edit and rendering, emit plain line output.
@@ -402,6 +402,23 @@ export class AstEditTool implements AgentTool<typeof astEditSchema, AstEditToolD
for (const change of applyResult.changes) {
recordAppliedFile(formatPath(change.path));
}
// The preview minted tags from pre-apply content; the rewrite just
// invalidated them. Re-record post-apply snapshots (canonical keys)
// so the model's next hashline edit anchors against fresh tags.
const freshTagLines: string[] = [];
if (useHashLines) {
const snapshotStore = getFileSnapshotStore(this.session);
for (const relativePath of appliedFileList) {
const appliedAbsolutePath = path.resolve(this.session.cwd, relativePath);
try {
const fullText = normalizeToLF(await Bun.file(appliedAbsolutePath).text());
const freshTag = snapshotStore.record(canonicalSnapshotKey(appliedAbsolutePath), fullText);
freshTagLines.push(formatHashlineHeader(relativePath, freshTag));
} catch {
// File disappeared between apply and re-read; skip its tag.
}
}
}
const appliedFileReplacements = appliedFileList.map(filePath => ({
path: filePath,
count: appliedFileReplacementCounts.get(filePath) ?? 0,
@@ -429,17 +446,20 @@ export class AstEditTool implements AgentTool<typeof astEditSchema, AstEditToolD
filePath => fileReplacementCounts.get(filePath) !== appliedFileReplacementCounts.get(filePath),
);
if (stalePreview) {
const text =
const staleText =
applyResult.totalReplacements === 0
? `Preview is stale / no longer matches; no replacements were applied. Preview expected ${result.totalReplacements} replacement${previewReplacementPlural} in ${result.filesTouched} file${previewFilePlural}.`
: applyResult.totalReplacements < result.totalReplacements
? `Preview is stale / no longer matches; only ${applyResult.totalReplacements} of ${result.totalReplacements} replacements were applied in ${applyResult.filesTouched} of ${result.filesTouched} files.`
: `Preview is stale / no longer matches; applied ${applyResult.totalReplacements} replacements but preview expected ${result.totalReplacements}.`;
return { ...toolResult(appliedDetails).text(text).done(), isError: true };
const staleWithTags =
freshTagLines.length > 0 ? `${staleText}\n${freshTagLines.join("\n")}` : staleText;
return { ...toolResult(appliedDetails).text(staleWithTags).done(), isError: true };
}
const appliedReplacementPlural = applyResult.totalReplacements !== 1 ? "s" : "";
const appliedFilePlural = applyResult.filesTouched !== 1 ? "s" : "";
const text = `Applied ${applyResult.totalReplacements} replacement${appliedReplacementPlural} in ${applyResult.filesTouched} file${appliedFilePlural}.`;
const appliedText = `Applied ${applyResult.totalReplacements} replacement${appliedReplacementPlural} in ${applyResult.filesTouched} file${appliedFilePlural}.`;
const text = freshTagLines.length > 0 ? `${appliedText}\n${freshTagLines.join("\n")}` : appliedText;
return toolResult(appliedDetails).text(text).done();
},
});
@@ -11,6 +11,7 @@ import { formatHashlineHeader, formatNumberedLines, type SnapshotStore } from "@
import type { AgentMessage } from "@oh-my-pi/pi-agent-core";
import type { ImageContent } from "@oh-my-pi/pi-ai";
import { formatAge, formatBytes, readImageMetadata } from "@oh-my-pi/pi-utils";
import { canonicalSnapshotKey } from "../edit/file-snapshot-store";
import { normalizeToLF } from "../edit/normalize";
import type { FileMentionMessage } from "../session/messages";
import {
@@ -259,7 +260,7 @@ export async function generateFileMentionMessages(
const normalized = snapshotStore ? normalizeToLF(content) : content;
let { output, lineCount } = buildTextOutput(normalized);
if (snapshotStore) {
const tag = snapshotStore.record(absolutePath, normalized);
const tag = snapshotStore.record(canonicalSnapshotKey(absolutePath), normalized);
output = `${formatHashlineHeader(resolvedPath, tag)}\n${formatNumberedLines(output)}`;
}
files.push({ path: resolvedPath, content: output, lineCount });
@@ -288,11 +288,14 @@ it("multi-entry edit on an auto-generated file surfaces isError + error text ins
// the streaming preview as if it succeeded.
expect(result.isError).toBe(true);
// Both per-entry failures must be preserved in the content text so the
// agent (and the error renderer) see the real cause.
// The orchestrator stops at the first failing entry: the failure must
// carry the real cause and entry position, and the remaining entries
// must be explicitly reported as not applied (never silently skipped).
const text = (result.content?.find(c => c.type === "text") as { text?: string } | undefined)?.text ?? "";
const occurrences = text.match(/Cannot modify auto-generated file/g) ?? [];
expect(occurrences.length).toBe(2);
expect(occurrences.length).toBe(1);
expect(text).toContain("(entry 1 of 2)");
expect(text).toContain("Entry 2 was NOT applied");
// `details.diff` must not contain a fabricated diff that would mislead the
// renderer's preview-fallback branch into showing the proposed change.