diff --git a/.alfonso/release-notes/next/pi-hashline-edit-row.md b/.alfonso/release-notes/next/pi-hashline-edit-row.md new file mode 100644 index 0000000000..a2b9e7fbd3 --- /dev/null +++ b/.alfonso/release-notes/next/pi-hashline-edit-row.md @@ -0,0 +1 @@ +- Pi: with `edit_mode: "hashline"`, the `edit` row now names the file(s) the patch changes (with the destination for a move) and shows the diff for every file, instead of an empty "…" row (#1410). diff --git a/benchmarks/aft-search/slice-descriptors/land-pi-hashline-edit-row.json b/benchmarks/aft-search/slice-descriptors/land-pi-hashline-edit-row.json new file mode 100644 index 0000000000..ebd574711e --- /dev/null +++ b/benchmarks/aft-search/slice-descriptors/land-pi-hashline-edit-row.json @@ -0,0 +1,8 @@ +{ + "targeted_mechanism": "none", + "kind": "harness", + "slice_class": "non_ranking", + "fixtures": [ + "harness-goldens" + ] +} diff --git a/packages/pi-plugin/src/__tests__/hashline-edit-render.test.ts b/packages/pi-plugin/src/__tests__/hashline-edit-render.test.ts new file mode 100644 index 0000000000..013a0fd0b3 --- /dev/null +++ b/packages/pi-plugin/src/__tests__/hashline-edit-render.test.ts @@ -0,0 +1,267 @@ +/** + * The hashline `edit` row in Pi: the call names the files its patch addresses + * and the result shows the diff Rust returns in `metadata.files`. + */ + +/// + +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdir, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { AgentToolResult } from "@earendil-works/pi-coding-agent"; +import type { Component } from "@earendil-works/pi-tui"; +import { buildHashlineMutationResult, registerHoistedTools } from "../tools/hoisted.js"; +import { makeContext, mockTheme, renderToString } from "./render-test-helpers.js"; +import { + executeTool, + type MockToolDef, + makeExtContext, + makeMockApi, + makeMockBridge, + makePluginContext, +} from "./tool-test-utils.js"; + +interface DisplayFile { + path: string; + type: "update" | "add" | "delete" | "move"; + hunks: string; + additions: number; + deletions: number; + sourcePath?: string; +} + +/** `crates/aft/src/edit.rs::build_unified_diff`: an `Index:` banner, file headers, then hunks. */ +function unifiedPatch(path: string, hunks: string): string { + return `Index: ${path}\n${"=".repeat(67)}\n--- ${path}\n+++ ${path}\n${hunks}`; +} + +/** + * A successful hashline `edit` reply as the Pi transport sends it + * (`crates/aft/src/hashline/integration/transport.rs::render_mutation_response`), + * plus the `text` the tool_call layer copies from `output`. The absolute-path + * fields of the real reply are left out: the Pi adapter does not read them. + */ +function hashlineReply(files: DisplayFile[]): Record { + const metaFiles = files.map((file) => { + const patch = unifiedPatch(file.path, file.hunks); + return { + relativePath: file.path, + type: file.type, + patch, + additions: file.additions, + deletions: file.deletions, + ...(file.sourcePath ? { movePath: file.sourcePath, sourcePath: file.sourcePath } : {}), + }; + }); + const output = [ + `${files.length} of ${files.length} files applied`, + ...files.map((file) => `primary ${file.path}: applied (applied)\n[${file.path}#1A2B]`), + ].join("\n"); + return { + output, + text: output, + title: "Hashline edit applied", + success: true, + complete: true, + partial: false, + all_failed: false, + preview: false, + metadata: { + diff: metaFiles.map((file) => file.patch).join("\n"), + files: metaFiles, + }, + op_id: "op-1", + hashline: true, + registers_committed: false, + transport: "pi", + }; +} + +const EDIT_A: DisplayFile = { + path: "src/a.ts", + type: "update", + hunks: "@@ -1,3 +1,3 @@\n one\n-two\n+TWO\n three\n", + additions: 1, + deletions: 1, +}; + +const EDIT_B: DisplayFile = { + path: "src/b.ts", + type: "update", + hunks: "@@ -1 +1,2 @@\n x\n+y\n", + additions: 1, + deletions: 0, +}; + +const MOVE: DisplayFile = { + path: "new.ts", + type: "move", + hunks: "@@ -0,0 +1 @@\n+keep\n", + additions: 1, + deletions: 0, + sourcePath: "old.ts", +}; + +const roots: string[] = []; + +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))); +}); + +/** Register the hashline `edit` tool against a bridge whose `edit` reply is `reply`. */ +async function hashlineEditTool(reply: Record = {}) { + const root = join(tmpdir(), `aft-pi-hashline-render-${process.pid}-${roots.length}`); + roots.push(root); + await mkdir(root, { recursive: true }); + const { api, tools } = makeMockApi(); + const { bridge } = makeMockBridge((command) => + command === "hashline_preflight" + ? { success: true, affected_paths: [], mv_destinations: [] } + : reply, + ); + registerHoistedTools( + api, + makePluginContext(bridge, { config: { edit_mode: "hashline" }, hashlineEffective: true }), + { + hoistRead: false, + hoistWrite: false, + hoistEdit: true, + hoistGrep: false, + restrictToProjectRoot: true, + }, + ); + const tool = tools.get("edit") as MockToolDef; + return { tool, extCtx: makeExtContext(root, "hashline-render") }; +} + +function renderCall(tool: MockToolDef, patch: unknown): string { + expect(tool.renderCall).toBeDefined(); + const args = { patch }; + return renderToString(tool.renderCall?.(args, mockTheme, makeContext(args)) as Component); +} + +function renderResult(tool: MockToolDef, result: unknown, expanded: boolean): string { + expect(tool.renderResult).toBeDefined(); + return renderToString( + tool.renderResult?.( + result, + { expanded, isPartial: false }, + mockTheme, + makeContext({ patch: "unused" }), + ) as Component, + ); +} + +describe("hashline edit call row", () => { + test("names the file of a single-section patch", async () => { + const { tool } = await hashlineEditTool(); + expect(renderCall(tool, "[src/a.ts#1A2B]\nPUT 2:\n+TWO\n")).toBe("edit src/a.ts"); + }); + + test("names the first file and counts the others in a multi-file patch", async () => { + const { tool } = await hashlineEditTool(); + const patch = [ + "*** Begin Patch", + "[src/a.ts#1A2B]", + "PUT 2:", + "+TWO", + "[src/b.ts#3C4D]", + "PUT >1:", + "+y", + "[src/a.ts#1A2B]", + "CUT 3", + "[src/c.ts#5E6F]", + "REM", + "*** End Patch", + ].join("\n"); + // src/a.ts appears in two sections but is one file. + expect(renderCall(tool, patch)).toBe("edit src/a.ts and 2 more"); + }); + + test("names the source and destination of a move", async () => { + const { tool } = await hashlineEditTool(); + expect(renderCall(tool, "[old.ts#1A2B]\nMV new.ts\n")).toBe("edit old.ts → new.ts"); + }); + + test("keeps the file while the model is still writing the patch", async () => { + const { tool } = await hashlineEditTool(); + // The last line is cut mid-directive, as Pi shows it while arguments stream. + expect(renderCall(tool, "[src/a.ts#1A2B]\nPU")).toBe("edit src/a.ts"); + expect(renderCall(tool, "[src/a.ts#1A")).toBe("edit ..."); + expect(renderCall(tool, undefined)).toBe("edit ..."); + }); +}); + +describe("hashline edit result", () => { + test("maps a single-file reply into the diff details the edit row renders", () => { + const reply = hashlineReply([EDIT_A]); + const result = buildHashlineMutationResult(reply); + + expect(result.content).toEqual([{ type: "text", text: reply.text as string }]); + expect(result.details?.diff).toBe(" 1 one\n-2 two\n+2 TWO\n 3 three"); + expect(result.details?.additions).toBe(1); + expect(result.details?.deletions).toBe(1); + expect(result.details?.firstChangedLine).toBe(2); + expect(result.details?.files).toEqual([ + { + path: "src/a.ts", + sourcePath: undefined, + diff: result.details?.diff, + additions: 1, + deletions: 1, + }, + ]); + }); + + test("keeps every file of a multi-file reply in the diff and the rendered result", async () => { + const reply = hashlineReply([EDIT_A, EDIT_B]); + const { tool, extCtx } = await hashlineEditTool(reply); + const result = (await executeTool( + tool, + { patch: "[src/a.ts#1A2B]\nPUT 2:\n+TWO\n[src/b.ts#3C4D]\nPUT >1:\n+y\n" }, + extCtx, + )) as AgentToolResult<{ + diff?: string; + additions: number; + deletions: number; + firstChangedLine?: number; + }>; + + expect(result.content).toEqual([{ type: "text", text: reply.text as string }]); + expect(result.details.diff).toBe( + [ + "src/a.ts (+1/-1)", + " 1 one", + "-2 two", + "+2 TWO", + " 3 three", + "src/b.ts (+1/-0)", + " 1 x", + "+2 y", + ].join("\n"), + ); + expect(result.details.additions).toBe(2); + expect(result.details.deletions).toBe(1); + expect(result.details.firstChangedLine).toBe(2); + + const expanded = renderResult(tool, result, true); + for (const fragment of ["src/a.ts (+1/-1)", "+2 TWO", "src/b.ts (+1/-0)", "+2 y"]) { + expect(expanded).toContain(fragment); + } + expect(renderResult(tool, result, false)).toBe("edited src/a.ts and 1 more (+2/-1)"); + }); + + test("shows a moved file's diff and names its source", async () => { + const reply = hashlineReply([MOVE]); + const { tool } = await hashlineEditTool(reply); + const result = buildHashlineMutationResult(reply); + + expect(result.details?.diff).toBe("+1 keep"); + expect(result.details?.additions).toBe(1); + expect(result.details?.deletions).toBe(0); + expect(result.details?.firstChangedLine).toBe(1); + expect(result.details?.files?.[0]?.sourcePath).toBe("old.ts"); + expect(renderResult(tool, result, true)).toContain("+1 keep"); + }); +}); diff --git a/packages/pi-plugin/src/omp-approval.ts b/packages/pi-plugin/src/omp-approval.ts index 4748e34690..5de345ebc9 100644 --- a/packages/pi-plugin/src/omp-approval.ts +++ b/packages/pi-plugin/src/omp-approval.ts @@ -171,19 +171,24 @@ function unquote(value: string): string { : value; } +/** One `[path#TAG]` section of a hashline patch and the `MV` destinations it names. */ +export interface HashlinePatchSection { + path: string; + moves: string[]; +} + /** - * Every file a hashline patch names: each `[path#TAG]` section header and each - * `MV` destination (grammar: docs/hashline.md). Lines are classified in the - * engine parser's order (envelope, header, directive, `+` body row), so every - * path the engine would act on is listed. It extracts targets and does not - * validate the rest of the grammar, so it may list paths for a patch the - * engine later rejects. Undefined when a line fits none of those forms or a - * header or `MV` target is malformed: the caller must then fail toward asking. + * The sections of a hashline patch, in patch order: each `[path#TAG]` section + * header with the `MV` destinations that follow it (grammar: docs/hashline.md). + * Lines are classified in the engine parser's order (envelope, header, + * directive, `+` body row), so every path the engine would act on is listed. It + * extracts targets and does not validate the rest of the grammar, so it may + * list paths for a patch the engine later rejects. Undefined when a line fits + * none of those forms or a header or `MV` target is malformed. */ -export function hashlinePatchPaths(patch: string): string[] | undefined { +export function hashlinePatchSections(patch: string): HashlinePatchSection[] | undefined { if (patch.trim().length === 0) return undefined; - const paths: string[] = []; - let sections = 0; + const sections: HashlinePatchSection[] = []; const rows = patch.split("\n"); for (let index = 0; index < rows.length; index++) { const raw = rows[index] as string; @@ -196,24 +201,34 @@ export function hashlinePatchPaths(patch: string): string[] | undefined { const inner = trimmed.slice(1, -1); const hash = inner.indexOf("#"); if (hash <= 0) return undefined; - paths.push(inner.slice(0, hash)); - sections++; + sections.push({ path: inner.slice(0, hash), moves: [] }); continue; } const directive = /^(PUT|CUT|REM|MV)(?:\s|$)/.exec(line); if (directive) { - if (sections === 0) return undefined; + const current = sections[sections.length - 1]; + if (current === undefined) return undefined; if (directive[1] === "MV") { const destination = line.slice(2).trim(); if (destination.length === 0 || /\s/.test(destination)) return undefined; - paths.push(unquote(destination)); + current.moves.push(unquote(destination)); } continue; } - if (trimmed.length === 0 && sections === 0) continue; - if (sections === 0 || !line.startsWith("+")) return undefined; + if (trimmed.length === 0 && sections.length === 0) continue; + if (sections.length === 0 || !line.startsWith("+")) return undefined; } - return sections > 0 ? paths : undefined; + return sections.length > 0 ? sections : undefined; +} + +/** + * Every file a hashline patch names: each `[path#TAG]` section header and each + * `MV` destination, in patch order (see `hashlinePatchSections`). Undefined + * when the patch cannot be read: the approval tier must then treat the call as + * one that names an outside path, so OMP asks before running it. + */ +export function hashlinePatchPaths(patch: string): string[] | undefined { + return hashlinePatchSections(patch)?.flatMap((section) => [section.path, ...section.moves]); } const APPLY_PATCH_TARGETS = [ diff --git a/packages/pi-plugin/src/tools/hoisted.ts b/packages/pi-plugin/src/tools/hoisted.ts index 37869e975b..4fffc93522 100644 --- a/packages/pi-plugin/src/tools/hoisted.ts +++ b/packages/pi-plugin/src/tools/hoisted.ts @@ -44,6 +44,7 @@ import { type Component, Container, Spacer, Text } from "@earendil-works/pi-tui" import { type Static, type TObject, Type } from "typebox"; import { resolveBashConfig, resolveGithubConfig, toolEnabled } from "../config.js"; import { hashlineTagSourceSentence } from "../hashline-tag-sources.js"; +import { hashlinePatchSections } from "../omp-approval.js"; import type { PluginContext } from "../types.js"; import { bridgeFor, @@ -59,9 +60,12 @@ import { } from "./_shared.js"; import { formatDiffForPi } from "./diff-format.js"; import { + asNumber, asRecord, + asRecords, asString, collapsibleResult, + formatUnifiedDiffForPi, normalizeTerminalText, type RenderResultOptionsLike, } from "./render-helpers.js"; @@ -555,6 +559,25 @@ interface FileMutationDetails { * See GitHub #45. */ noOp?: boolean; + /** + * One row per file a hashline patch changed, in patch order. Set only for + * hashline `edit` results, whose call arguments carry no `path`, so the + * renderer names the files from here and shows each file's diff under its + * own header. + */ + files?: MutationFileDetails[]; +} + +/** One file of a hashline `edit` result (see `buildHashlineMutationResult`). */ +interface MutationFileDetails { + /** The path as the patch named it (project-relative when inside the project). */ + path: string; + /** For a move, the file the content moved from; `path` is the destination. */ + sourcePath?: string; + /** Pi line-numbered diff of this file; absent when the file content did not change. */ + diff?: string; + additions: number; + deletions: number; } function readPathArg(args: unknown): string | undefined { @@ -853,7 +876,15 @@ export function registerHoistedTools( } const response = await callToolCallForSession(bridge, "edit", rawArgs, sessionId, extCtx); if (response.success === false) throw toolErrorFromResponse("edit", response); - return buildMutationResult(response); + return buildHashlineMutationResult(response); + }, + // Without these Pi renders this tool with its built-in `edit` renderer, + // which reads `path` and so shows nothing for a `{patch}` call. + renderCall(args, theme, context) { + return renderHashlineEditCall(args, theme, context); + }, + renderResult(result, options = { expanded: false, isPartial: false }, theme, context) { + return renderMutationResult(result, theme, context, options); }, }); } @@ -1106,6 +1137,105 @@ export function buildMutationResult( }; } +/** + * Shape a hashline `edit` reply into an `AgentToolResult`. A hashline reply + * carries its diff as one unified patch per file in `metadata.files` rather + * than the top-level `diff: {before, after}` object `buildMutationResult` + * reads, so this maps those rows into the same `details` fields. The text + * returned to the model is exactly what `buildMutationResult` produces. + */ +export function buildHashlineMutationResult( + response: Record, +): AgentToolResult { + const result = buildMutationResult(response); + const rows = asRecords(asRecord(response.metadata)?.files); + const files = rows.map(hashlineFileDetails); + if (files.length === 0) return result; + + const changed = files.filter((file) => file.diff !== undefined); + // With several files each diff restarts its line numbers, so the combined + // diff names every file above its part. + const diff = + changed.length === 0 + ? undefined + : files.length === 1 + ? files[0]?.diff + : files + .map((file) => + file.diff === undefined + ? mutationFileHeader(file) + : `${mutationFileHeader(file)}\n${file.diff}`, + ) + .join("\n"); + const firstChangedLine = rows + .map((row) => firstChangedLineOfPatch(asString(row.patch) ?? "")) + .find((line) => line !== undefined); + return { + ...result, + details: { + ...result.details, + diff, + firstChangedLine, + additions: files.reduce((sum, file) => sum + file.additions, 0), + deletions: files.reduce((sum, file) => sum + file.deletions, 0), + files, + }, + }; +} + +/** One `metadata.files` row of a hashline reply, in the renderer's shape. */ +function hashlineFileDetails(row: Record): MutationFileDetails { + const patch = asString(row.patch) ?? ""; + const diff = formatUnifiedDiffForPi(patch); + return { + path: asString(row.relativePath) ?? "", + sourcePath: asString(row.sourcePath) ?? asString(row.movePath), + diff: diff.length > 0 ? diff : undefined, + additions: asNumber(row.additions) ?? 0, + deletions: asNumber(row.deletions) ?? 0, + }; +} + +/** + * The new-file line number of the first changed line in a unified patch, the + * same line `formatDiffForPi` reports. Undefined when the patch has no change. + */ +function firstChangedLineOfPatch(patch: string): number | undefined { + let newLine: number | undefined; + for (const line of patch.split("\n")) { + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + newLine = Number(hunk[1]); + continue; + } + // File headers (`---`/`+++`) come before the first hunk header. + if (newLine === undefined) continue; + if (line.startsWith("+") || line.startsWith("-")) return Math.max(1, newLine); + if (line.startsWith(" ")) newLine++; + } + return undefined; +} + +/** The file's display path, or `source → destination` for a move. */ +function mutationFileName(file: MutationFileDetails): string { + return file.sourcePath + ? `${shortenPath(file.sourcePath)} → ${shortenPath(file.path)}` + : shortenPath(file.path); +} + +/** `path (+a/-d)`: the line above each file's part of a multi-file diff. */ +function mutationFileHeader(file: MutationFileDetails): string { + return `${mutationFileName(file)} (+${file.additions}/-${file.deletions})`; +} + +/** The first file's name plus how many more files there are, for one-line summaries. */ +function mutationFilesLabel(files: MutationFileDetails[] | undefined): string | undefined { + const first = files?.[0]; + if (!first || !files) return undefined; + const name = mutationFileName(first); + return files.length > 1 ? `${name} and ${files.length - 1} more` : name; +} + function formatGlobSkipReasonsNote(reasons: unknown): string | undefined { if (!Array.isArray(reasons)) return undefined; const actionable = reasons @@ -1189,6 +1319,46 @@ export function renderMutationCall( return text; } +/** + * The call row of a hashline `edit`: its arguments are only `{patch}`, so the + * file comes from the patch's first `[path#TAG]` section header (with its `MV` + * destination when it moves), followed by how many other files it names. + */ +export function renderHashlineEditCall( + args: unknown, + theme: Theme, + context: RenderContextLike, +): Text { + const text = reuseText(context.lastComponent); + const target = hashlinePatchTarget(asString(asRecord(args)?.patch)); + const targetDisplay = target + ? `${theme.fg("accent", target.name)}${target.more > 0 ? theme.fg("muted", ` and ${target.more} more`) : ""}` + : theme.fg("toolOutput", "..."); + text.setText(`${theme.fg("toolTitle", theme.bold("edit"))} ${targetDisplay}`); + return text; +} + +function hashlinePatchTarget( + patch: string | undefined, +): { name: string; more: number } | undefined { + if (!patch) return undefined; + // Pi re-renders the call row while the model is still writing the patch, so + // its last line can be incomplete; read the patch without it in that case. + const sections = + hashlinePatchSections(patch) ?? + hashlinePatchSections(patch.slice(0, patch.lastIndexOf("\n") + 1)); + const first = sections?.[0]; + if (!sections || !first) return undefined; + const destination = first.moves[first.moves.length - 1]; + const name = destination + ? `${shortenPath(first.path)} → ${shortenPath(destination)}` + : shortenPath(first.path); + const others = new Set(sections.flatMap((section) => [section.path, ...section.moves])); + others.delete(first.path); + if (destination) others.delete(destination); + return { name, more: others.size }; +} + export function renderMutationResult( result: AgentToolResult, theme: Theme, @@ -1248,23 +1418,29 @@ export function renderMutationResult( const container = reuseContainer(context.lastComponent); container.clear(); container.addChild(new Spacer(1)); - let renderedDiff: string; - try { - renderedDiff = renderDiff(diff); - } catch { - // Renderer tests and non-interactive hosts may not have initialized Pi's theme proxy. - renderedDiff = diff; + const files = details?.files ?? []; + if (files.length > 1) { + // A multi-file hashline patch: each file's diff restarts its line + // numbers, so name every file above its diff. + for (const file of files) { + const counts = theme.fg("muted", ` (+${file.additions}/-${file.deletions})`); + container.addChild(new Text(`${theme.fg("accent", mutationFileName(file))}${counts}`, 1, 0)); + if (file.diff !== undefined) { + container.addChild(new Text(renderTerminalDiff(file.diff), 1, 0)); + } + } + } else { + container.addChild(new Text(renderTerminalDiff(diff), 1, 0)); } - // Normalize before Text wraps the lines, then collapsibleResult applies the - // same safeguard to every renderer's final terminal-bound output. - container.addChild(new Text(normalizeTerminalText(renderedDiff), 1, 0)); const rawPath = context.args && typeof context.args === "object" && typeof (context.args as { path?: unknown }).path === "string" ? (context.args as { path: string }).path : undefined; - const summaryPath = rawPath ? ` ${shortenPath(rawPath)}` : ""; + // Hashline calls carry no `path` argument; their result names the files. + const summaryLabel = rawPath ? shortenPath(rawPath) : mutationFilesLabel(details?.files); + const summaryPath = summaryLabel ? ` ${summaryLabel}` : ""; const summary = `edited${summaryPath} (+${details?.additions ?? 0}/-${details?.deletions ?? 0})`; return collapsibleResult({ summary, @@ -1274,6 +1450,20 @@ export function renderMutationResult( }); } +/** A Pi line-numbered diff colored by Pi's diff renderer, safe for the terminal. */ +function renderTerminalDiff(diff: string): string { + let renderedDiff: string; + try { + renderedDiff = renderDiff(diff); + } catch { + // Renderer tests and non-interactive hosts may not have initialized Pi's theme proxy. + renderedDiff = diff; + } + // Normalize before Text wraps the lines, then collapsibleResult applies the + // same safeguard to every renderer's final terminal-bound output. + return normalizeTerminalText(renderedDiff); +} + /** Resolve a path argument to an absolute path if it exists, decoding file: * URLs and expanding `~`. */ export async function resolvePathArg(cwd: string, path: string): Promise {