From 2b103bf9ad6f4abe99bc47d32bdae5887039c7c1 Mon Sep 17 00:00:00 2001 From: can1357 Date: Mon, 19 Jan 2026 09:43:08 +0100 Subject: [PATCH] feat(coding-agent/core): added nested @@ context anchors and hierarchical matching - Added support for nested @@ context anchors and hierarchical context matching. - Enhanced patch parser to handle @@ line N syntax and multiple context accumulation. - Improved diff rendering and error messages with hierarchical context display. - Added comprehensive regression tests for model-generated patch formats. --- packages/coding-agent/CHANGELOG.md | 13 +- .../src/core/tools/patch/applicator.ts | 168 ++++++++--- .../src/core/tools/patch/parser.ts | 53 +++- .../src/modes/interactive/components/diff.ts | 25 +- .../test/core/apply-patch-regression.test.ts | 260 ++++++++++++++++++ 5 files changed, 472 insertions(+), 47 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ef83c92ff..a655cf21a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,9 +1,13 @@ # Changelog ## [Unreleased] - ### Added +- Added support for `@@ line N` syntax to use line numbers as positioning hints +- Added support for nested `@@` anchors on multiple lines for hierarchical context matching +- Added support for space-separated hierarchical anchors like `@@ class PatchTool constructor` +- Enhanced context matching to handle hierarchical searches with nested anchors +- Added comprehensive regression tests for model-generated patch formats - Added configurable fuzzy matching threshold for edit operations - Added edit fuzzy threshold setting with options from 0.85 (lenient) to 0.98 (strict) - Added binary file reading support for improved BOM detection in patch operations @@ -14,6 +18,10 @@ ### Changed +- Improved diff rendering to handle optional line numbers in diff lines +- Enhanced patch parser to accumulate multiple `@@` context lines for nested matching +- Updated context search to treat hierarchical matching as single unique match +- Improved error messages to display hierarchical context with `>` separator for clarity - Refactored output sanitization logic into centralized streaming-output module - Converted voice recording from factory function to class-based implementation - Converted RPC extension UI context from factory function to class-based implementation @@ -53,6 +61,9 @@ ### Fixed +- Fixed patch parser to correctly interpret `@@ line 125` as line hint instead of literal context +- Fixed hierarchical context matching to properly disambiguate between similar code structures +- Fixed space-separated anchor parsing to handle multi-word contexts correctly - Fixed BOM detection to check binary content when text-based detection fails - Fixed patch preview to use configured fuzzy threshold for consistent behavior - Fixed trailing newline preservation to maintain original file behavior diff --git a/packages/coding-agent/src/core/tools/patch/applicator.ts b/packages/coding-agent/src/core/tools/patch/applicator.ts index 89c059b0d..278640f82 100644 --- a/packages/coding-agent/src/core/tools/patch/applicator.ts +++ b/packages/coding-agent/src/core/tools/patch/applicator.ts @@ -18,7 +18,7 @@ import { stripBom, } from "./normalize"; import { normalizeCreateContent, parseHunks } from "./parser"; -import type { ApplyPatchOptions, ApplyPatchResult, DiffHunk, FileSystem, PatchInput } from "./types"; +import type { ApplyPatchOptions, ApplyPatchResult, ContextLineResult, DiffHunk, FileSystem, PatchInput } from "./types"; import { ApplyPatchError } from "./types"; // ═══════════════════════════════════════════════════════════════════════════ @@ -117,6 +117,123 @@ function getHunkHintIndex(hunk: DiffHunk, currentIndex: number): number | undefi return hintIndex >= currentIndex ? hintIndex : undefined; } +/** + * Find hierarchical context in file lines. + * + * Handles three formats: + * 1. Simple context: "function foo" - find this line + * 2. Hierarchical (newline): "class Foo\nmethod" - find class, then method after it + * 3. Hierarchical (space): "class Foo method" - try as literal first, then split and search + * + * @returns The result from finding the final (innermost) context, or undefined if not found + */ +function findHierarchicalContext( + lines: string[], + context: string, + startFrom: number, + lineHint: number | undefined, +): ContextLineResult { + // Check for newline-separated hierarchical contexts (from nested @@ anchors) + if (context.includes("\n")) { + const parts = context + .split("\n") + .map((p) => p.trim()) + .filter((p) => p.length > 0); + let currentStart = startFrom; + + for (let i = 0; i < parts.length; i++) { + const part = parts[i]; + const isLast = i === parts.length - 1; + + // For intermediate contexts, we just need to find them and continue from there + // For the final context, we return the full result with match count + const result = findContextLine(lines, part, currentStart); + + if (result.index === undefined) { + // Try from beginning if not found from current position + const fromStartResult = findContextLine(lines, part, 0); + if (fromStartResult.index === undefined) { + return { index: undefined, confidence: 0 }; + } + currentStart = fromStartResult.index + 1; + if (isLast) { + // Hierarchical matching narrows scope - treat as single match + return { ...fromStartResult, matchCount: 1 }; + } + } else { + currentStart = result.index + 1; + if (isLast) { + // Hierarchical matching narrows scope - treat as single match + return { ...result, matchCount: 1 }; + } + } + } + return { index: undefined, confidence: 0 }; + } + + // Try literal context first + let result = findContextLine(lines, context, startFrom); + + // If line hint exists and result is ambiguous or missing, try from hint + if ((result.index === undefined || (result.matchCount ?? 0) > 1) && lineHint !== undefined) { + const hintStart = Math.max(0, lineHint - 1); + const hintedResult = findContextLine(lines, context, hintStart); + if (hintedResult.index !== undefined) { + // Line hint successfully found a match - treat as disambiguated + return { ...hintedResult, matchCount: 1 }; + } + } + + // If found uniquely, return it + if (result.index !== undefined && (result.matchCount ?? 0) <= 1) { + return result; + } + + // Try from beginning if not found + if (result.index === undefined && startFrom !== 0) { + result = findContextLine(lines, context, 0); + if (result.index !== undefined && (result.matchCount ?? 0) <= 1) { + return result; + } + } + + // Fallback: try space-separated hierarchical matching + // e.g., "class PatchTool constructor" -> find "class PatchTool", then "constructor" after it + const spaceParts = context.split(/\s+/).filter((p) => p.length > 0); + if (spaceParts.length > 1) { + let currentStart = startFrom; + + for (let i = 0; i < spaceParts.length; i++) { + const part = spaceParts[i]; + const isLast = i === spaceParts.length - 1; + + const partResult = findContextLine(lines, part, currentStart); + + if (partResult.index === undefined) { + // Try from beginning + const fromStartResult = findContextLine(lines, part, 0); + if (fromStartResult.index === undefined) { + // Space-separated fallback failed, return original result + return result; + } + currentStart = fromStartResult.index + 1; + if (isLast) { + // Hierarchical matching narrows scope - treat as single match + return { ...fromStartResult, matchCount: 1 }; + } + } else { + currentStart = partResult.index + 1; + if (isLast) { + // Hierarchical matching narrows scope - treat as single match + return { ...partResult, matchCount: 1 }; + } + } + } + } + + return result; +} + /** Find sequence with optional hint position, returning full search result */ function findSequenceWithHint( lines: string[], @@ -224,49 +341,34 @@ function computeReplacements(originalLines: string[], path: string, hunks: DiffH // If hunk has a changeContext, find it and adjust lineIndex if (hunk.changeContext !== undefined) { - // Use findContextLine for robust matching with substring/fuzzy fallback - let result = findContextLine(originalLines, hunk.changeContext, lineIndex); - let usedLineHint = false; + // Use hierarchical context matching for nested @@ anchors and space-separated contexts + const result = findHierarchicalContext(originalLines, hunk.changeContext, lineIndex, lineHint); - // If ambiguous or missing and a line hint exists, bias using the hint - if ((result.index === undefined || (result.matchCount ?? 0) > 1) && lineHint !== undefined) { - const hintStart = Math.max(0, lineHint - 1); - const hintedResult = findContextLine(originalLines, hunk.changeContext, hintStart); - if (hintedResult.index !== undefined) { - result = hintedResult; - usedLineHint = true; - } - } - - if (!usedLineHint && result.matchCount !== undefined && result.matchCount > 1) { + if (result.matchCount !== undefined && result.matchCount > 1) { + const displayContext = hunk.changeContext.includes("\n") + ? hunk.changeContext.split("\n").pop() + : hunk.changeContext; throw new ApplyPatchError( - `Found ${result.matchCount} matches for context '${hunk.changeContext}' in ${path}. ` + + `Found ${result.matchCount} matches for context '${displayContext}' in ${path}. ` + `Add more surrounding context or additional @@ anchors to make it unique.`, ); } - // If search failed, try fallback strategies - let idx = result.index; - if (idx === undefined && lineIndex !== 0) { - // Last resort: search from beginning (handles out-of-order hunks) - const fromStartResult = findContextLine(originalLines, hunk.changeContext, 0); - if (fromStartResult.matchCount !== undefined && fromStartResult.matchCount > 1) { - throw new ApplyPatchError( - `Found ${fromStartResult.matchCount} matches for context '${hunk.changeContext}' in ${path}. ` + - `Add more surrounding context or additional @@ anchors to make it unique.`, - ); - } - idx = fromStartResult.index; - } - + const idx = result.index; if (idx === undefined) { - throw new ApplyPatchError(`Failed to find context '${hunk.changeContext}' in ${path}`); + const displayContext = hunk.changeContext.includes("\n") + ? hunk.changeContext.split("\n").join(" > ") + : hunk.changeContext; + throw new ApplyPatchError(`Failed to find context '${displayContext}' in ${path}`); } - // If oldLines[0] matches changeContext, start search at idx (not idx+1) + // If oldLines[0] matches the final context, start search at idx (not idx+1) // This handles the common case where @@ scope and first context line are identical const firstOldLine = hunk.oldLines[0]; - if (firstOldLine !== undefined && firstOldLine.trim() === hunk.changeContext.trim()) { + const finalContext = hunk.changeContext.includes("\n") + ? hunk.changeContext.split("\n").pop()?.trim() + : hunk.changeContext.trim(); + if (firstOldLine !== undefined && firstOldLine.trim() === finalContext) { lineIndex = idx; } else { lineIndex = idx + 1; diff --git a/packages/coding-agent/src/core/tools/patch/parser.ts b/packages/coding-agent/src/core/tools/patch/parser.ts index 58e9ea09a..115cd51af 100644 --- a/packages/coding-agent/src/core/tools/patch/parser.ts +++ b/packages/coding-agent/src/core/tools/patch/parser.ts @@ -21,6 +21,9 @@ const EMPTY_CHANGE_CONTEXT_MARKER = "@@"; /** Regex to match unified diff hunk headers: @@ -OLD,COUNT +NEW,COUNT @@ optional-context */ const UNIFIED_HUNK_HEADER_REGEX = /^@@\s*-(\d+)(?:,(\d+))?\s+\+(\d+)(?:,(\d+))?\s*@@(?:\s*(.*))?$/; +/** Regex to match @@ line N pattern (model-generated line hints) */ +const LINE_HINT_REGEX = /^line\s+(\d+)$/i; + /** * Check if a line is a diff content line (context, addition, or removal). * These should never be treated as metadata even if their content looks like it. @@ -193,13 +196,20 @@ interface ParseHunkResult { /** * Parse a single hunk from lines starting at the current position. + * + * Handles several context formats: + * - Empty: `@@` (no context, match from current position) + * - Unified: `@@ -10,3 +10,3 @@` (line numbers as hints) + * - Context: `@@ function foo` (search for context line) + * - Line hint: `@@ line 125` (use line 125 as starting position) + * - Nested: `@@ class Foo\n@@ method` (hierarchical context search) */ function parseOneHunk(lines: string[], lineNumber: number, allowMissingContext: boolean): ParseHunkResult { if (lines.length === 0) { throw new ParseError("Diff does not contain any lines", lineNumber); } - let changeContext: string | undefined; + const changeContexts: string[] = []; let oldStartLine: number | undefined; let newStartLine: number | undefined; let startIndex: number; @@ -209,28 +219,61 @@ function parseOneHunk(lines: string[], lineNumber: number, allowMissingContext: // Check for context marker if (headerLine === EMPTY_CHANGE_CONTEXT_MARKER) { - changeContext = undefined; startIndex = 1; } else if (unifiedHeader) { - changeContext = unifiedHeader.changeContext; + if (unifiedHeader.changeContext) { + changeContexts.push(unifiedHeader.changeContext); + } oldStartLine = unifiedHeader.oldStartLine; newStartLine = unifiedHeader.newStartLine; startIndex = 1; } else if (headerLine.startsWith(CHANGE_CONTEXT_MARKER)) { - changeContext = headerLine.slice(CHANGE_CONTEXT_MARKER.length); + const contextValue = headerLine.slice(CHANGE_CONTEXT_MARKER.length); + + // Check for @@ line N pattern (model-generated line hints) + const lineHintMatch = contextValue.match(LINE_HINT_REGEX); + if (lineHintMatch) { + oldStartLine = Number(lineHintMatch[1]); + newStartLine = oldStartLine; + } else { + changeContexts.push(contextValue); + } startIndex = 1; } else { if (!allowMissingContext) { throw new ParseError(`Expected hunk to start with @@ context marker, got: '${lines[0]}'`, lineNumber); } - changeContext = undefined; startIndex = 0; } + // Check for nested @@ anchors on subsequent lines + // Format: @@ class Foo + // @@ method + while (startIndex < lines.length) { + const nextLine = lines[startIndex]; + const trimmed = nextLine.trim(); + + // Check if it's another @@ line (nested anchor) + if (trimmed.startsWith(CHANGE_CONTEXT_MARKER)) { + const nestedContext = trimmed.slice(CHANGE_CONTEXT_MARKER.length); + changeContexts.push(nestedContext); + startIndex++; + } else if (trimmed === EMPTY_CHANGE_CONTEXT_MARKER) { + // Empty @@ as separator - skip it + startIndex++; + } else { + // Not an @@ line, stop accumulating + break; + } + } + if (startIndex >= lines.length) { throw new ParseError("Hunk does not contain any lines", lineNumber + 1); } + // Combine contexts: if multiple, join with newline for hierarchical matching + const changeContext = changeContexts.length > 0 ? changeContexts.join("\n") : undefined; + const hunk: DiffHunk = { changeContext, oldStartLine, diff --git a/packages/coding-agent/src/modes/interactive/components/diff.ts b/packages/coding-agent/src/modes/interactive/components/diff.ts index a9df307a2..7681da70a 100644 --- a/packages/coding-agent/src/modes/interactive/components/diff.ts +++ b/packages/coding-agent/src/modes/interactive/components/diff.ts @@ -6,9 +6,9 @@ import { theme } from "../theme/theme"; * Format: "+123 content" or "-123 content" or " 123 content" or " ..." */ function parseDiffLine(line: string): { prefix: string; lineNum: string; content: string } | null { - const match = line.match(/^([+-\s])(\s*\d*)\s(.*)$/); + const match = line.match(/^([+-\s])(?:(\s*\d+)\s)?(.*)$/); if (!match) return null; - return { prefix: match[1], lineNum: match[2], content: match[3] }; + return { prefix: match[1], lineNum: match[2] ?? "", content: match[3] }; } /** @@ -80,6 +80,13 @@ export function renderDiff(diffText: string, _options: RenderDiffOptions = {}): const lines = diffText.split("\n"); const result: string[] = []; + const formatLine = (prefix: string, lineNum: string, content: string): string => { + if (lineNum.trim().length === 0) { + return `${prefix}${content}`; + } + return `${prefix}${lineNum} ${content}`; + }; + let i = 0; while (i < lines.length) { const line = lines[i]; @@ -121,24 +128,26 @@ export function renderDiff(diffText: string, _options: RenderDiffOptions = {}): replaceTabs(added.content), ); - result.push(theme.fg("toolDiffRemoved", `-${removed.lineNum} ${removedLine}`)); - result.push(theme.fg("toolDiffAdded", `+${added.lineNum} ${addedLine}`)); + result.push(theme.fg("toolDiffRemoved", formatLine("-", removed.lineNum, removedLine))); + result.push(theme.fg("toolDiffAdded", formatLine("+", added.lineNum, addedLine))); } else { // Show all removed lines first, then all added lines for (const removed of removedLines) { - result.push(theme.fg("toolDiffRemoved", `-${removed.lineNum} ${replaceTabs(removed.content)}`)); + result.push( + theme.fg("toolDiffRemoved", formatLine("-", removed.lineNum, replaceTabs(removed.content))), + ); } for (const added of addedLines) { - result.push(theme.fg("toolDiffAdded", `+${added.lineNum} ${replaceTabs(added.content)}`)); + result.push(theme.fg("toolDiffAdded", formatLine("+", added.lineNum, replaceTabs(added.content)))); } } } else if (parsed.prefix === "+") { // Standalone added line - result.push(theme.fg("toolDiffAdded", `+${parsed.lineNum} ${replaceTabs(parsed.content)}`)); + result.push(theme.fg("toolDiffAdded", formatLine("+", parsed.lineNum, replaceTabs(parsed.content)))); i++; } else { // Context line - result.push(theme.fg("toolDiffContext", ` ${parsed.lineNum} ${replaceTabs(parsed.content)}`)); + result.push(theme.fg("toolDiffContext", formatLine(" ", parsed.lineNum, replaceTabs(parsed.content)))); i++; } } diff --git a/packages/coding-agent/test/core/apply-patch-regression.test.ts b/packages/coding-agent/test/core/apply-patch-regression.test.ts index ac891ef62..63724726c 100644 --- a/packages/coding-agent/test/core/apply-patch-regression.test.ts +++ b/packages/coding-agent/test/core/apply-patch-regression.test.ts @@ -736,3 +736,263 @@ describe("regression: *** End of File marker handling (2A/2G)", () => { expect(result).toBe("item\nmore content\nFINAL ITEM\n"); }); }); + +// ═══════════════════════════════════════════════════════════════════════════ +// Regression: Model edit attempts that failed due to parser limitations +// These tests document real model behaviors that we want to recover from. +// Session: 2026-01-19T08-29-03-476Z_v0FEI1ixUrlssLyHL3TT3.jsonl +// ═══════════════════════════════════════════════════════════════════════════ + +describe("regression: model edit attempt - @@ line N syntax (session 2026-01-19)", () => { + let tempDir: string; + + beforeEach(() => { + tempDir = join(tmpdir(), `model-line-n-${Date.now()}-${Math.random().toString(36).slice(2)}`); + mkdirSync(tempDir, { recursive: true }); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + test("@@ line 125 is parsed as line hint, not literal context search", async () => { + const filePath = join(tempDir, "settings.ts"); + // Create file with enough lines - the target is around line 125 + const lines: string[] = []; + for (let i = 1; i <= 130; i++) { + if (i === 125) { + lines.push("\tfuzzyMatch?: boolean; // default: true"); + } else if (i === 126) { + lines.push("\tfuzzyThreshold?: number; // default: 0.95"); + } else if (i === 127) { + lines.push("\tpatchMode?: boolean; // default: false"); + } else if (i === 128) { + lines.push("}"); + } else { + lines.push(`// line ${i}`); + } + } + await Bun.write(filePath, `${lines.join("\n")}\n`); + + // Model's actual attempt: used @@ line 125 as anchor + await applyPatch( + { + path: "settings.ts", + operation: "update", + diff: `@@ line 125 + fuzzyMatch?: boolean; // default: true + fuzzyThreshold?: number; // default: 0.95 +- patchMode?: boolean; // default: false ++ patchMode?: boolean; // default: true + }`, + }, + { cwd: tempDir }, + ); + + const result = readFileSync(filePath, "utf-8"); + expect(result).toContain("patchMode?: boolean; // default: true"); + expect(result).not.toContain("patchMode?: boolean; // default: false"); + }); + + test("@@ line N works with various formats", async () => { + const filePath = join(tempDir, "code.ts"); + const lines = Array.from({ length: 20 }, (_, i) => `content line ${i + 1}`); + lines[9] = "old value"; // line 10 + await Bun.write(filePath, `${lines.join("\n")}\n`); + + // Variations the model might produce + await applyPatch( + { + path: "code.ts", + operation: "update", + diff: `@@ line 10 +-old value ++new value`, + }, + { cwd: tempDir }, + ); + + expect(readFileSync(filePath, "utf-8")).toContain("new value"); + }); +}); + +describe("regression: model edit attempt - nested @@ anchors (session 2026-01-19)", () => { + let tempDir: string; + + beforeEach(() => { + tempDir = join(tmpdir(), `model-nested-${Date.now()}-${Math.random().toString(36).slice(2)}`); + mkdirSync(tempDir, { recursive: true }); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + test("@@ class X followed by @@ method on next line is parsed as nested anchors", async () => { + const filePath = join(tempDir, "patch.ts"); + await Bun.write( + filePath, + `class OtherTool { + constructor(session: ToolSession) { + this.session = session; + this.mode = false; + } +} + +class PatchTool { + constructor(session: ToolSession) { + this.session = session; + this.patchMode = session.settings?.getEditPatchMode?.() ?? false; + this.allowFuzzy = true; + } +} +`, + ); + + // Model's actual attempt: multi-line @@ anchors + await applyPatch( + { + path: "patch.ts", + operation: "update", + diff: `@@ class PatchTool +@@ constructor + constructor(session: ToolSession) { + this.session = session; +- this.patchMode = session.settings?.getEditPatchMode?.() ?? false; ++ this.patchMode = session.settings?.getEditPatchMode?.() ?? true; + this.allowFuzzy = true;`, + }, + { cwd: tempDir }, + ); + + const result = readFileSync(filePath, "utf-8"); + // Should change PatchTool's constructor, not OtherTool's + expect(result).toContain("this.patchMode = session.settings?.getEditPatchMode?.() ?? true;"); + expect(result).toContain("this.mode = false;"); // OtherTool unchanged + }); + + test("nested @@ anchors disambiguate between multiple matching methods", async () => { + const filePath = join(tempDir, "multi-class.ts"); + await Bun.write( + filePath, + `class Alpha { + process() { + return "alpha"; + } +} + +class Beta { + process() { + return "beta"; + } +} +`, + ); + + await applyPatch( + { + path: "multi-class.ts", + operation: "update", + diff: `@@ class Beta +@@ process + process() { +- return "beta"; ++ return "BETA"; + }`, + }, + { cwd: tempDir }, + ); + + const result = readFileSync(filePath, "utf-8"); + expect(result).toContain('return "alpha"'); // Alpha unchanged + expect(result).toContain('return "BETA"'); // Beta changed + }); +}); + +describe("regression: model edit attempt - space-separated anchors (session 2026-01-19)", () => { + let tempDir: string; + + beforeEach(() => { + tempDir = join(tmpdir(), `model-space-${Date.now()}-${Math.random().toString(36).slice(2)}`); + mkdirSync(tempDir, { recursive: true }); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + test("@@ class PatchTool constructor is parsed as hierarchical anchors", async () => { + const filePath = join(tempDir, "tool.ts"); + await Bun.write( + filePath, + `class OtherTool { + constructor() { + this.value = 1; + } +} + +class PatchTool { + constructor() { + this.value = 2; + } +} +`, + ); + + // Model's actual attempt: space-separated anchors + await applyPatch( + { + path: "tool.ts", + operation: "update", + diff: `@@ class PatchTool constructor + constructor() { +- this.value = 2; ++ this.value = 200; + }`, + }, + { cwd: tempDir }, + ); + + const result = readFileSync(filePath, "utf-8"); + expect(result).toContain("this.value = 1;"); // OtherTool unchanged + expect(result).toContain("this.value = 200;"); // PatchTool changed + }); + + test("space-separated anchors work with function keyword", async () => { + const filePath = join(tempDir, "funcs.ts"); + await Bun.write( + filePath, + `function outer() { + function helper() { + return 1; + } + return helper(); +} + +function process() { + function helper() { + return 2; + } + return helper(); +} +`, + ); + + await applyPatch( + { + path: "funcs.ts", + operation: "update", + diff: `@@ function process helper + function helper() { +- return 2; ++ return 200; + }`, + }, + { cwd: tempDir }, + ); + + const result = readFileSync(filePath, "utf-8"); + expect(result).toContain("return 1;"); // outer's helper unchanged + expect(result).toContain("return 200;"); // process's helper changed + }); +});