From 5e903cbb79abf75a0883c2dd054455e9f1c71a6f Mon Sep 17 00:00:00 2001 From: Kigbnajd Date: Thu, 13 Aug 2026 22:09:50 +0200 Subject: [PATCH] fix(edit): address seen-line retry review --- .../coding-agent/src/edit/hashline/execute.ts | 65 ++++++++++++------- .../coding-agent/src/tools/output-meta.ts | 5 ++ .../test/edit/seen-line-guard.test.ts | 29 ++++++++- 3 files changed, 76 insertions(+), 23 deletions(-) diff --git a/packages/coding-agent/src/edit/hashline/execute.ts b/packages/coding-agent/src/edit/hashline/execute.ts index 88e124e8f..948be4b5b 100644 --- a/packages/coding-agent/src/edit/hashline/execute.ts +++ b/packages/coding-agent/src/edit/hashline/execute.ts @@ -30,7 +30,7 @@ import { import type { AgentToolResult } from "@oh-my-pi/pi-agent-core"; import type { FileDiagnosticsResult, WritethroughCallback, WritethroughDeferredHandle } from "../../lsp"; import type { ToolSession } from "../../tools"; -import { outputMeta } from "../../tools/output-meta"; +import { outputMeta, resolveArtifactSpillThresholdBytes } from "../../tools/output-meta"; import { ToolError } from "../../tools/tool-errors"; import { generateDiffString } from "../diff"; import { getEditClipboard } from "../edit-clipboard"; @@ -66,7 +66,7 @@ function resolveHashlineInput(session: ToolSession, input: string): string { return input; } const pending = pendingSeenLineRetries.get(session); - if (!pending || pending.token !== match[1]) { + if (!pending || pending.token.toLowerCase() !== match[1].toLowerCase()) { throw new ToolError( "Unknown or expired edit retry token. Use the token from the latest seen-line rejection, or submit a full patch.", ); @@ -149,9 +149,33 @@ function narrowBatchRequest(outer: LspBatchRequest | undefined, isLast: boolean) return { id: outer.id, flush: isLast && outer.flush }; } +interface SeenLineProvenance { + absolutePath: string; + tag: string; + body: string; +} + interface RenderedSection { toolResult: AgentToolResult; perFileResult: EditToolPerFileResult; + seenLineProvenance?: SeenLineProvenance; +} + +function recordRenderedSeenLines( + session: ToolSession, + result: AgentToolResult, + rendered: readonly RenderedSection[], +): void { + const fullText = result.content + .filter(part => part.type === "text" && part.text) + .map(part => (part.type === "text" ? part.text : "")) + .join("\n"); + if (Buffer.byteLength(fullText, "utf8") > resolveArtifactSpillThresholdBytes(session.settings)) return; + for (const section of rendered) { + const provenance = section.seenLineProvenance; + if (!provenance) continue; + recordSeenLinesFromBody(session, provenance.absolutePath, provenance.tag, provenance.body); + } } const BLOCK_OP_LABELS: Record = { @@ -179,7 +203,6 @@ function renderSection( result: PatchSectionResult, diagnostics: FileDiagnosticsResult | undefined, sourcePath: string, - session: ToolSession, ): RenderedSection { if (result.op === "delete") { const toolResult: AgentToolResult = { @@ -229,10 +252,16 @@ function renderSection( const moveBlock = result.moveDest ? `\nMoved to ${result.moveDest}` : ""; const firstChangedLine = result.firstChangedLine ?? diff.firstChangedLine; const text = `${result.header}${blockBlock}${moveBlock}${previewBlock}${warningsBlock}`; - if (normalizeToLF(stripBom(result.written).text) === result.after) { - recordSeenLinesFromBody(session, canonicalSnapshotKey(result.canonicalPath), result.fileHash, text); - } + const seenLineProvenance = + normalizeToLF(stripBom(result.written).text) === result.after + ? { + absolutePath: canonicalSnapshotKey(result.canonicalPath), + tag: result.fileHash, + body: text, + } + : undefined; return { + seenLineProvenance, toolResult: { content: [ { @@ -305,15 +334,12 @@ export async function executeHashlineSingle( if (escalate) { throw new ToolError(noChangeLoopDiagnostic(sectionResult.path, count)); } - return renderSection(sectionResult, undefined, prepared.section.path, options.session).toolResult; + return renderSection(sectionResult, undefined, prepared.section.path).toolResult; } resetNoopEdit(options.session, sectionResult.canonicalPath); - return renderSection( - sectionResult, - fs.consumeDiagnostics(sectionResult.path), - prepared.section.path, - options.session, - ).toolResult; + const rendered = renderSection(sectionResult, fs.consumeDiagnostics(sectionResult.path), prepared.section.path); + recordRenderedSeenLines(options.session, rendered.toolResult, [rendered]); + return rendered.toolResult; } // Multi-section: prepare every section up front so we fail fast before @@ -354,16 +380,9 @@ export async function executeHashlineSingle( : new ToolError(noChangeDiagnostic(sectionResult.path)); } resetNoopEdit(options.session, sectionResult.canonicalPath); - rendered.push( - renderSection( - sectionResult, - fs.consumeDiagnostics(sectionResult.path), - prepared[i].section.path, - options.session, - ), - ); + rendered.push(renderSection(sectionResult, fs.consumeDiagnostics(sectionResult.path), prepared[i].section.path)); } - return { + const result: AgentToolResult = { content: [ { type: "text", @@ -377,6 +396,8 @@ export async function executeHashlineSingle( perFileResults: rendered.map(r => r.perFileResult), }), }; + recordRenderedSeenLines(options.session, result, rendered); + return result; } export { HashlineMismatchError, type HashlineParams, hashlineEditParamsSchema }; diff --git a/packages/coding-agent/src/tools/output-meta.ts b/packages/coding-agent/src/tools/output-meta.ts index e2f8e301e..e403983b5 100644 --- a/packages/coding-agent/src/tools/output-meta.ts +++ b/packages/coding-agent/src/tools/output-meta.ts @@ -625,6 +625,11 @@ function getSpillConfig(s: Settings | undefined) { }; } +/** Resolve the byte threshold above which ordinary tool output spills to an artifact. */ +export function resolveArtifactSpillThresholdBytes(s: Settings | undefined): number { + return getSpillConfig(s).threshold; +} + /** * Resolve the OutputSink `headBytes` budget from session settings. * Exposed so streaming executors (bash/python/ssh/eval) can opt into diff --git a/packages/coding-agent/test/edit/seen-line-guard.test.ts b/packages/coding-agent/test/edit/seen-line-guard.test.ts index f0d2b5bcd..1ce314be2 100644 --- a/packages/coding-agent/test/edit/seen-line-guard.test.ts +++ b/packages/coding-agent/test/edit/seen-line-guard.test.ts @@ -321,7 +321,7 @@ describe("read → edit seen-line guard", () => { expect(retry).toBeDefined(); expect(await Bun.file(file).text()).toBe(CONTENT); - await executeHashlineSingle(execOptions(retry as string, session)); + await executeHashlineSingle(execOptions((retry as string).toUpperCase(), session)); const after = await Bun.file(file).text(); expect(after).toContain("X10\nX11\nX12"); expect(after).not.toContain("line 10"); @@ -374,6 +374,33 @@ describe("read → edit seen-line guard", () => { ).rejects.toThrow(/never displayed \(it showed/); }); + it("does not record edit-output lines when the result will spill", async () => { + const file = path.join(tmpDir, "notes.txt"); + await Bun.write(file, CONTENT); + const session = { + ...createSession(tmpDir), + settings: Settings.isolated({ + "edit.enforceSeenLines": true, + "tools.artifactSpillThreshold": 0.1, + "tools.artifactHeadBytes": 0.03, + "tools.artifactTailBytes": 0.03, + }), + } as ToolSession; + const store = getFileSnapshotStore(session); + + const read = await new ReadTool(session).execute("r1", { path: `${file}:1-3` }); + const tag = tagFromOutput(resultText(read)); + const edit = await executeHashlineSingle( + execOptions(`[notes.txt#${tag}]\nPUT 2.=2:\n+${"EDITED ".repeat(80)}`, session), + ); + const text = resultText(edit); + const nextTag = tagFromOutput(text); + expect(text).toContain("2:"); + + const seen = store.byHash(canonicalSnapshotKey(file), nextTag)?.seenLines; + expect(seen?.has(2) ?? false).toBe(false); + }); + it("does not trust requested diff lines after an ACP client transforms the write", async () => { const file = path.join(tmpDir, "notes.txt"); const content = "ONE\nTWO\nTHREE\nFOUR\nFIVE\n";