From 1ed009e2cf49111881458453b7d24110dc124f02 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Thu, 1 Jan 2026 20:34:19 +0100 Subject: [PATCH] Show edit diff before tool execution (fixes #393) - Extract diff computation from edit.ts into shared edit-diff.ts - ToolExecutionComponent computes and caches diff when args are complete - Diff is visible while permission hooks block, before tool executes --- .../coding-agent/src/core/tools/edit-diff.ts | 203 ++++++++++++++++++ packages/coding-agent/src/core/tools/edit.ts | 124 +---------- .../interactive/components/tool-execution.ts | 79 ++++++- .../src/modes/interactive/interactive-mode.ts | 5 + 4 files changed, 277 insertions(+), 134 deletions(-) create mode 100644 packages/coding-agent/src/core/tools/edit-diff.ts diff --git a/packages/coding-agent/src/core/tools/edit-diff.ts b/packages/coding-agent/src/core/tools/edit-diff.ts new file mode 100644 index 00000000..810e011a --- /dev/null +++ b/packages/coding-agent/src/core/tools/edit-diff.ts @@ -0,0 +1,203 @@ +/** + * Shared diff computation utilities for the edit tool. + * Used by both edit.ts (for execution) and tool-execution.ts (for preview rendering). + */ + +import * as Diff from "diff"; +import { constants } from "fs"; +import { access, readFile } from "fs/promises"; +import { resolveToCwd } from "./path-utils.js"; + +export function detectLineEnding(content: string): "\r\n" | "\n" { + const crlfIdx = content.indexOf("\r\n"); + const lfIdx = content.indexOf("\n"); + if (lfIdx === -1) return "\n"; + if (crlfIdx === -1) return "\n"; + return crlfIdx < lfIdx ? "\r\n" : "\n"; +} + +export function normalizeToLF(text: string): string { + return text.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); +} + +export function restoreLineEndings(text: string, ending: "\r\n" | "\n"): string { + return ending === "\r\n" ? text.replace(/\n/g, "\r\n") : text; +} + +/** + * Generate a unified diff string with line numbers and context. + * Returns both the diff string and the first changed line number (in the new file). + */ +export function generateDiffString( + oldContent: string, + newContent: string, + contextLines = 4, +): { diff: string; firstChangedLine: number | undefined } { + const parts = Diff.diffLines(oldContent, newContent); + const output: string[] = []; + + const oldLines = oldContent.split("\n"); + const newLines = newContent.split("\n"); + const maxLineNum = Math.max(oldLines.length, newLines.length); + const lineNumWidth = String(maxLineNum).length; + + let oldLineNum = 1; + let newLineNum = 1; + let lastWasChange = false; + let firstChangedLine: number | undefined; + + for (let i = 0; i < parts.length; i++) { + const part = parts[i]; + const raw = part.value.split("\n"); + if (raw[raw.length - 1] === "") { + raw.pop(); + } + + if (part.added || part.removed) { + // Capture the first changed line (in the new file) + if (firstChangedLine === undefined) { + firstChangedLine = newLineNum; + } + + // Show the change + for (const line of raw) { + if (part.added) { + const lineNum = String(newLineNum).padStart(lineNumWidth, " "); + output.push(`+${lineNum} ${line}`); + newLineNum++; + } else { + // removed + const lineNum = String(oldLineNum).padStart(lineNumWidth, " "); + output.push(`-${lineNum} ${line}`); + oldLineNum++; + } + } + lastWasChange = true; + } else { + // Context lines - only show a few before/after changes + const nextPartIsChange = i < parts.length - 1 && (parts[i + 1].added || parts[i + 1].removed); + + if (lastWasChange || nextPartIsChange) { + // Show context + let linesToShow = raw; + let skipStart = 0; + let skipEnd = 0; + + if (!lastWasChange) { + // Show only last N lines as leading context + skipStart = Math.max(0, raw.length - contextLines); + linesToShow = raw.slice(skipStart); + } + + if (!nextPartIsChange && linesToShow.length > contextLines) { + // Show only first N lines as trailing context + skipEnd = linesToShow.length - contextLines; + linesToShow = linesToShow.slice(0, contextLines); + } + + // Add ellipsis if we skipped lines at start + if (skipStart > 0) { + output.push(` ${"".padStart(lineNumWidth, " ")} ...`); + // Update line numbers for the skipped leading context + oldLineNum += skipStart; + newLineNum += skipStart; + } + + for (const line of linesToShow) { + const lineNum = String(oldLineNum).padStart(lineNumWidth, " "); + output.push(` ${lineNum} ${line}`); + oldLineNum++; + newLineNum++; + } + + // Add ellipsis if we skipped lines at end + if (skipEnd > 0) { + output.push(` ${"".padStart(lineNumWidth, " ")} ...`); + // Update line numbers for the skipped trailing context + oldLineNum += skipEnd; + newLineNum += skipEnd; + } + } else { + // Skip these context lines entirely + oldLineNum += raw.length; + newLineNum += raw.length; + } + + lastWasChange = false; + } + } + + return { diff: output.join("\n"), firstChangedLine }; +} + +export interface EditDiffResult { + diff: string; + firstChangedLine: number | undefined; +} + +export interface EditDiffError { + error: string; +} + +/** + * Compute the diff for an edit operation without applying it. + * Used for preview rendering in the TUI before the tool executes. + */ +export async function computeEditDiff( + path: string, + oldText: string, + newText: string, + cwd: string, +): Promise { + const absolutePath = resolveToCwd(path, cwd); + + try { + // Check if file exists and is readable + try { + await access(absolutePath, constants.R_OK); + } catch { + return { error: `File not found: ${path}` }; + } + + // Read the file + const content = await readFile(absolutePath, "utf-8"); + + const normalizedContent = normalizeToLF(content); + const normalizedOldText = normalizeToLF(oldText); + const normalizedNewText = normalizeToLF(newText); + + // Check if old text exists + if (!normalizedContent.includes(normalizedOldText)) { + return { + error: `Could not find the exact text in ${path}. The old text must match exactly including all whitespace and newlines.`, + }; + } + + // Count occurrences + const occurrences = normalizedContent.split(normalizedOldText).length - 1; + if (occurrences > 1) { + return { + error: `Found ${occurrences} occurrences of the text in ${path}. The text must be unique. Please provide more context to make it unique.`, + }; + } + + // Compute the new content + const index = normalizedContent.indexOf(normalizedOldText); + const normalizedNewContent = + normalizedContent.substring(0, index) + + normalizedNewText + + normalizedContent.substring(index + normalizedOldText.length); + + // Check if it would actually change anything + if (normalizedContent === normalizedNewContent) { + return { + error: `No changes would be made to ${path}. The replacement produces identical content.`, + }; + } + + // Generate the diff + return generateDiffString(normalizedContent, normalizedNewContent); + } catch (err) { + return { error: err instanceof Error ? err.message : String(err) }; + } +} diff --git a/packages/coding-agent/src/core/tools/edit.ts b/packages/coding-agent/src/core/tools/edit.ts index a46a209d..dcfb843e 100644 --- a/packages/coding-agent/src/core/tools/edit.ts +++ b/packages/coding-agent/src/core/tools/edit.ts @@ -1,132 +1,10 @@ import type { AgentTool } from "@mariozechner/pi-agent-core"; import { Type } from "@sinclair/typebox"; -import * as Diff from "diff"; import { constants } from "fs"; import { access, readFile, writeFile } from "fs/promises"; +import { detectLineEnding, generateDiffString, normalizeToLF, restoreLineEndings } from "./edit-diff.js"; import { resolveToCwd } from "./path-utils.js"; -function detectLineEnding(content: string): "\r\n" | "\n" { - const crlfIdx = content.indexOf("\r\n"); - const lfIdx = content.indexOf("\n"); - if (lfIdx === -1) return "\n"; - if (crlfIdx === -1) return "\n"; - return crlfIdx < lfIdx ? "\r\n" : "\n"; -} - -function normalizeToLF(text: string): string { - return text.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); -} - -function restoreLineEndings(text: string, ending: "\r\n" | "\n"): string { - return ending === "\r\n" ? text.replace(/\n/g, "\r\n") : text; -} - -/** - * Generate a unified diff string with line numbers and context - * Returns both the diff string and the first changed line number (in the new file) - */ -function generateDiffString( - oldContent: string, - newContent: string, - contextLines = 4, -): { diff: string; firstChangedLine: number | undefined } { - const parts = Diff.diffLines(oldContent, newContent); - const output: string[] = []; - - const oldLines = oldContent.split("\n"); - const newLines = newContent.split("\n"); - const maxLineNum = Math.max(oldLines.length, newLines.length); - const lineNumWidth = String(maxLineNum).length; - - let oldLineNum = 1; - let newLineNum = 1; - let lastWasChange = false; - let firstChangedLine: number | undefined; - - for (let i = 0; i < parts.length; i++) { - const part = parts[i]; - const raw = part.value.split("\n"); - if (raw[raw.length - 1] === "") { - raw.pop(); - } - - if (part.added || part.removed) { - // Capture the first changed line (in the new file) - if (firstChangedLine === undefined) { - firstChangedLine = newLineNum; - } - - // Show the change - for (const line of raw) { - if (part.added) { - const lineNum = String(newLineNum).padStart(lineNumWidth, " "); - output.push(`+${lineNum} ${line}`); - newLineNum++; - } else { - // removed - const lineNum = String(oldLineNum).padStart(lineNumWidth, " "); - output.push(`-${lineNum} ${line}`); - oldLineNum++; - } - } - lastWasChange = true; - } else { - // Context lines - only show a few before/after changes - const nextPartIsChange = i < parts.length - 1 && (parts[i + 1].added || parts[i + 1].removed); - - if (lastWasChange || nextPartIsChange) { - // Show context - let linesToShow = raw; - let skipStart = 0; - let skipEnd = 0; - - if (!lastWasChange) { - // Show only last N lines as leading context - skipStart = Math.max(0, raw.length - contextLines); - linesToShow = raw.slice(skipStart); - } - - if (!nextPartIsChange && linesToShow.length > contextLines) { - // Show only first N lines as trailing context - skipEnd = linesToShow.length - contextLines; - linesToShow = linesToShow.slice(0, contextLines); - } - - // Add ellipsis if we skipped lines at start - if (skipStart > 0) { - output.push(` ${"".padStart(lineNumWidth, " ")} ...`); - // Update line numbers for the skipped leading context - oldLineNum += skipStart; - newLineNum += skipStart; - } - - for (const line of linesToShow) { - const lineNum = String(oldLineNum).padStart(lineNumWidth, " "); - output.push(` ${lineNum} ${line}`); - oldLineNum++; - newLineNum++; - } - - // Add ellipsis if we skipped lines at end - if (skipEnd > 0) { - output.push(` ${"".padStart(lineNumWidth, " ")} ...`); - // Update line numbers for the skipped trailing context - oldLineNum += skipEnd; - newLineNum += skipEnd; - } - } else { - // Skip these context lines entirely - oldLineNum += raw.length; - newLineNum += raw.length; - } - - lastWasChange = false; - } - } - - return { diff: output.join("\n"), firstChangedLine }; -} - const editSchema = Type.Object({ path: Type.String({ description: "Path to the file to edit (relative or absolute)" }), oldText: Type.String({ description: "Exact text to find and replace (must match exactly)" }), diff --git a/packages/coding-agent/src/modes/interactive/components/tool-execution.ts b/packages/coding-agent/src/modes/interactive/components/tool-execution.ts index 837f94fb..453d56ec 100644 --- a/packages/coding-agent/src/modes/interactive/components/tool-execution.ts +++ b/packages/coding-agent/src/modes/interactive/components/tool-execution.ts @@ -12,6 +12,7 @@ import { } from "@mariozechner/pi-tui"; import stripAnsi from "strip-ansi"; import type { CustomTool } from "../../../core/custom-tools/types.js"; +import { computeEditDiff, type EditDiffError, type EditDiffResult } from "../../../core/tools/edit-diff.js"; import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, formatSize } from "../../../core/tools/truncate.js"; import { sanitizeBinaryOutput } from "../../../utils/shell.js"; import { getLanguageFromPath, highlightCode, theme } from "../theme/theme.js"; @@ -58,11 +59,15 @@ export class ToolExecutionComponent extends Container { private isPartial = true; private customTool?: CustomTool; private ui: TUI; + private cwd: string; private result?: { content: Array<{ type: string; text?: string; data?: string; mimeType?: string }>; isError: boolean; details?: any; }; + // Cached edit diff preview (computed when args arrive, before tool executes) + private editDiffPreview?: EditDiffResult | EditDiffError; + private editDiffArgsKey?: string; // Track which args the preview is for constructor( toolName: string, @@ -70,6 +75,7 @@ export class ToolExecutionComponent extends Container { options: ToolExecutionOptions = {}, customTool: CustomTool | undefined, ui: TUI, + cwd: string = process.cwd(), ) { super(); this.toolName = toolName; @@ -77,6 +83,7 @@ export class ToolExecutionComponent extends Container { this.showImages = options.showImages ?? true; this.customTool = customTool; this.ui = ui; + this.cwd = cwd; this.addChild(new Spacer(1)); @@ -98,6 +105,47 @@ export class ToolExecutionComponent extends Container { this.updateDisplay(); } + /** + * Signal that args are complete (tool is about to execute). + * This triggers diff computation for edit tool. + */ + setArgsComplete(): void { + this.maybeComputeEditDiff(); + } + + /** + * Compute edit diff preview when we have complete args. + * This runs async and updates display when done. + */ + private maybeComputeEditDiff(): void { + if (this.toolName !== "edit") return; + + const path = this.args?.path; + const oldText = this.args?.oldText; + const newText = this.args?.newText; + + // Need all three params to compute diff + if (!path || oldText === undefined || newText === undefined) return; + + // Create a key to track which args this computation is for + const argsKey = JSON.stringify({ path, oldText, newText }); + + // Skip if we already computed for these exact args + if (this.editDiffArgsKey === argsKey) return; + + this.editDiffArgsKey = argsKey; + + // Compute diff async + computeEditDiff(path, oldText, newText, this.cwd).then((result) => { + // Only update if args haven't changed since we started + if (this.editDiffArgsKey === argsKey) { + this.editDiffPreview = result; + this.updateDisplay(); + this.ui.requestRender(); + } + }); + } + updateResult( result: { content: Array<{ type: string; text?: string; data?: string; mimeType?: string }>; @@ -415,22 +463,31 @@ export class ToolExecutionComponent extends Container { const rawPath = this.args?.file_path || this.args?.path || ""; const path = shortenPath(rawPath); - // Build path display, appending :line if we have a successful result with line info + // Build path display, appending :line if we have diff info let pathDisplay = path ? theme.fg("accent", path) : theme.fg("toolOutput", "..."); - if (this.result && !this.result.isError && this.result.details?.firstChangedLine) { - pathDisplay += theme.fg("warning", `:${this.result.details.firstChangedLine}`); + const firstChangedLine = + (this.editDiffPreview && "firstChangedLine" in this.editDiffPreview + ? this.editDiffPreview.firstChangedLine + : undefined) || + (this.result && !this.result.isError ? this.result.details?.firstChangedLine : undefined); + if (firstChangedLine) { + pathDisplay += theme.fg("warning", `:${firstChangedLine}`); } text = `${theme.fg("toolTitle", theme.bold("edit"))} ${pathDisplay}`; - if (this.result) { - if (this.result.isError) { - const errorText = this.getTextOutput(); - if (errorText) { - text += `\n\n${theme.fg("error", errorText)}`; - } - } else if (this.result.details?.diff) { - text += `\n\n${renderDiff(this.result.details.diff, { filePath: rawPath })}`; + if (this.result?.isError) { + // Show error from result + const errorText = this.getTextOutput(); + if (errorText) { + text += `\n\n${theme.fg("error", errorText)}`; + } + } else if (this.editDiffPreview) { + // Use cached diff preview (works both before and after execution) + if ("error" in this.editDiffPreview) { + text += `\n\n${theme.fg("error", this.editDiffPreview.error)}`; + } else if (this.editDiffPreview.diff) { + text += `\n\n${renderDiff(this.editDiffPreview.diff, { filePath: rawPath })}`; } } } else if (this.toolName === "ls") { diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index 1a48fdf2..29a3cd1c 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -902,6 +902,11 @@ export class InteractiveMode { }); } this.pendingTools.clear(); + } else { + // Args are now complete - trigger diff computation for edit tools + for (const [, component] of this.pendingTools.entries()) { + component.setArgsComplete(); + } } this.streamingComponent = undefined; this.streamingMessage = undefined;