diff --git a/apps/.i18n/native-source.json b/apps/.i18n/native-source.json index dfab86b3c2a8..88d0e9aac2cc 100644 --- a/apps/.i18n/native-source.json +++ b/apps/.i18n/native-source.json @@ -38795,7 +38795,7 @@ }, { "kind": "conditional-branch", - "line": 1377, + "line": 1399, "path": "apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatTranscriptCache.swift", "source": "INSERT INTO cached_transcripts(gateway_id, session_key, agent_id, payload, updated_at)\nSELECT gateway_id, session_key, agent_id, payload, updated_at\nFROM cached_transcripts_pre_v3", "surface": "apple", @@ -38803,7 +38803,7 @@ }, { "kind": "conditional-branch", - "line": 1382, + "line": 1404, "path": "apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatTranscriptCache.swift", "source": "INSERT INTO cached_transcripts(gateway_id, session_key, agent_id, payload, updated_at)\nSELECT gateway_id, session_key, '', payload, updated_at\nFROM cached_transcripts_pre_v3\nWHERE lower(trim(session_key)) GLOB 'agent:*:*'", "surface": "apple", diff --git a/apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatToolDiff.swift b/apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatToolDiff.swift index 495666f788bc..be7495d94cfe 100644 --- a/apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatToolDiff.swift +++ b/apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatToolDiff.swift @@ -184,7 +184,8 @@ enum ChatToolDiff { case "create": return self.resolveWriteDiff( argumentsRecord, - keys: ["file_text", "content"]) + keys: ["file_text", "content"], + details: details) case "insert": return self.resolveInsertionDiff(argumentsRecord, details: details) default: @@ -206,7 +207,10 @@ enum ChatToolDiff { return detailsDiff } guard !isError else { return nil } - return self.resolveWriteDiff(argumentsRecord, keys: ["content", "text", "file_text"]) + return self.resolveWriteDiff( + argumentsRecord, + keys: ["content", "text", "file_text"], + details: details) } if self.patchToolNames.contains(normalizedName) { if let detailsDiff = self.resolveDetailsDiff(details) { @@ -243,8 +247,9 @@ enum ChatToolDiff { private static func resolvePatchDiff( _ arguments: [String: AnyCodable]?) -> (lines: [ChatToolDiffLine], stat: ChatToolDiffStat?)? { - guard let patch = self.string(in: arguments, keys: ["patch", "input", "diff"]), - !patch.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty + // First NON-BLANK alias, matching the transcript cache's selection so a + // cold-cache reload renders the same envelope as the live row. + guard let patch = self.firstNonBlankString(in: arguments, keys: ["input", "patch", "diff"]) else { return nil } let inputClipped = patch.utf16.count > self.maxLocalInputCharacters @@ -261,6 +266,11 @@ enum ChatToolDiff { let structural = current?.operation == .update ? raw.replacingOccurrences(of: #"[ \t]+$"#, with: "", options: .regularExpression) : raw.trimmingCharacters(in: .whitespaces) + if structural == "...(truncated)..." { + // Cached envelopes use this marker to make an unknown patch tail explicit. + clipped = true + continue + } if let header = self.patchFileHeader(structural) { if let current { sections.append(current) } current = PatchSection( @@ -554,8 +564,11 @@ enum ChatToolDiff { private static func resolveWriteDiff( _ arguments: [String: AnyCodable]?, - keys: [String]) -> (lines: [ChatToolDiffLine], stat: ChatToolDiffStat?)? + keys: [String], + details: AnyCodable?) -> (lines: [ChatToolDiffLine], stat: ChatToolDiffStat?)? { + let detailValues = details?.dictionaryValue + guard detailValues?["changed"]?.boolValue != false else { return nil } guard let content = self.string(in: arguments, keys: keys) else { return nil } let allLines = self.splitLines(content) guard !allLines.isEmpty else { return nil } @@ -572,7 +585,11 @@ enum ChatToolDiff { if lines.count < allLines.count { lines.append(ChatToolDiffLine(kind: .skip, text: "")) } - return (lines, ChatToolDiffStat(added: allLines.count, removed: 0)) + // Present details need created=true before zero removals are authoritative. + let stat = detailValues != nil && detailValues?["created"]?.boolValue != true + ? nil + : ChatToolDiffStat(added: allLines.count, removed: 0) + return (lines, stat) } private static func resolveEditDiff( @@ -692,6 +709,21 @@ enum ChatToolDiff { } } + private static func firstNonBlankString( + in record: [String: AnyCodable]?, + keys: [String]) -> String? + { + guard let record else { return nil } + for key in keys { + if let value = record[key]?.stringValue, + !value.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty + { + return value + } + } + return nil + } + private static func string(in record: [String: AnyCodable]?, keys: [String]) -> String? { guard let record else { return nil } return self.firstValue(in: record, keys: keys)?.stringValue diff --git a/apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatTranscriptCache.swift b/apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatTranscriptCache.swift index 209b6b9987fb..03cd09912331 100644 --- a/apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatTranscriptCache.swift +++ b/apps/shared/OpenClawKit/Sources/OpenClawChatUI/ChatTranscriptCache.swift @@ -880,9 +880,9 @@ extension OpenClawChatSQLiteTranscriptCache { extension OpenClawChatSQLiteTranscriptCache { // MARK: - Cached shapes - /// Cache v1 strips attachments and tool arguments; args-derived fallback - /// diffs intentionally do not survive cold paint. Only bounded applied - /// diff details remain so the JSON payload cannot grow without limit. + /// Cache v1 strips attachments and ordinary tool arguments. Patch calls + /// retain only one bounded patch envelope so their diffs survive cold paint; + /// bounded applied diff details remain the other supported diff source. static func cacheableMessages(_ messages: [OpenClawChatMessage]) -> [OpenClawChatMessage] { messages.suffix(self.maxCachedMessagesPerSession).map { message in OpenClawChatMessage( @@ -900,7 +900,8 @@ extension OpenClawChatSQLiteTranscriptCache { content: nil, id: item.id, name: item.name, - arguments: nil, + // Patch envelopes are the sole argument exception because details are not always emitted. + arguments: self.cacheablePatchArguments(item), details: self.cacheableDetails(item.details), isError: item.isError) }, @@ -918,17 +919,38 @@ extension OpenClawChatSQLiteTranscriptCache { private static func cacheableDetails(_ details: AnyCodable?) -> AnyCodable? { guard let diff = details?.dictionaryValue?["diff"]?.stringValue else { return nil } - let limit = 64000 - let truncationMarker = "\n...(truncated)..." - let capped = if diff.utf16.count > limit { - self.utf16Prefix(diff, limit: limit - truncationMarker.utf16.count) + truncationMarker - } else { - diff - } + let capped = self.cacheableText(diff) guard !capped.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return nil } return AnyCodable(["diff": AnyCodable(capped)]) } + private static func cacheablePatchArguments(_ item: OpenClawChatMessageContent) -> AnyCodable? { + guard let type = item.type?.trimmingCharacters(in: .whitespacesAndNewlines).lowercased(), + ["toolcall", "tool_call", "tooluse", "tool_use"].contains(type), + let name = item.name?.trimmingCharacters(in: .whitespacesAndNewlines).lowercased(), + ["apply_patch", "applypatch", "patch"].contains(name), + let arguments = item.arguments?.dictionaryValue + else { return nil } + + for key in ["input", "patch", "diff"] { + guard let value = arguments[key]?.stringValue, + !value.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty + else { continue } + return AnyCodable([key: AnyCodable(self.cacheableText(value))]) + } + return nil + } + + private static func cacheableText(_ value: String) -> String { + let limit = 64000 + let truncationMarker = "\n...(truncated)..." + return if value.utf16.count > limit { + self.utf16Prefix(value, limit: limit - truncationMarker.utf16.count) + truncationMarker + } else { + value + } + } + private static func utf16Prefix(_ value: String, limit: Int) -> String { let units = value.utf16 guard units.count > limit else { return value } diff --git a/apps/shared/OpenClawKit/Tests/OpenClawKitTests/ChatToolDiffTests.swift b/apps/shared/OpenClawKit/Tests/OpenClawKitTests/ChatToolDiffTests.swift index bea3aa586937..b24c1467b31e 100644 --- a/apps/shared/OpenClawKit/Tests/OpenClawKitTests/ChatToolDiffTests.swift +++ b/apps/shared/OpenClawKit/Tests/OpenClawKitTests/ChatToolDiffTests.swift @@ -148,6 +148,32 @@ struct ChatToolDiffTests { #expect(resolved.stat == ChatToolDiffStat(added: 2, removed: 0)) } + @Test func `write created flags control fallback stats`() throws { + let arguments = AnyCodable(["content": "one\ntwo\n"]) + let created = try #require(ChatToolDiff.resolveDiff( + name: "write", + arguments: arguments, + details: AnyCodable(["created": true]))) + let overwritten = try #require(ChatToolDiff.resolveDiff( + name: "write", + arguments: arguments, + details: AnyCodable(["created": false]))) + let unknown = try #require(ChatToolDiff.resolveDiff( + name: "write", + arguments: arguments, + details: AnyCodable(["changed": true]))) + + #expect(created.stat == ChatToolDiffStat(added: 2, removed: 0)) + #expect(overwritten.lines == created.lines) + #expect(overwritten.stat == nil) + #expect(unknown.lines == created.lines) + #expect(unknown.stat == nil) + #expect(ChatToolDiff.resolveDiff( + name: "write", + arguments: arguments, + details: AnyCodable(["changed": false])) == nil) + } + @Test func `renders editor create and insert commands`() throws { let create = try #require(ChatToolDiff.resolveDiff( name: "str_replace_editor", @@ -166,6 +192,16 @@ struct ChatToolDiffTests { #expect(createFallback.lines == [ChatToolDiffLine(kind: .add, lineNo: 1, text: "fallback")]) #expect(createFallback.stat == ChatToolDiffStat(added: 1, removed: 0)) + let overwrittenCreate = try #require(ChatToolDiff.resolveDiff( + name: "str_replace_editor", + arguments: AnyCodable(["command": "create", "file_text": "replacement"]), + details: AnyCodable(["created": false]))) + #expect(overwrittenCreate.stat == nil) + #expect(ChatToolDiff.resolveDiff( + name: "str_replace_editor", + arguments: AnyCodable(["command": "create", "file_text": "unchanged"]), + details: AnyCodable(["changed": false])) == nil) + let insert = try #require(ChatToolDiff.resolveDiff( name: "str_replace_based_edit_tool", arguments: AnyCodable(["command": "insert", "insert_text": "three"]), @@ -243,6 +279,20 @@ struct ChatToolDiffTests { } } + @Test func `patch envelope precedence matches transcript caching`() throws { + let input = "*** Begin Patch\n*** Add File: input.txt\n+input\n*** End Patch" + let patch = "*** Begin Patch\n*** Add File: patch.txt\n+patch\n*** End Patch" + let resolved = try #require(ChatToolDiff.resolveDiff( + name: "apply_patch", + arguments: AnyCodable([ + "input": AnyCodable(input), + "patch": AnyCodable(patch), + ]), + details: nil)) + + #expect(resolved.lines.first == ChatToolDiffLine(kind: .add, lineNo: 1, text: "input")) + } + @Test func `separates add delete and move patch files`() throws { let patch = [ "*** Begin Patch", diff --git a/apps/shared/OpenClawKit/Tests/OpenClawKitTests/ChatTranscriptCacheStoreTests.swift b/apps/shared/OpenClawKit/Tests/OpenClawKitTests/ChatTranscriptCacheStoreTests.swift index 845cec134704..fd293f2bc05d 100644 --- a/apps/shared/OpenClawKit/Tests/OpenClawKitTests/ChatTranscriptCacheStoreTests.swift +++ b/apps/shared/OpenClawKit/Tests/OpenClawKitTests/ChatTranscriptCacheStoreTests.swift @@ -174,6 +174,57 @@ struct ChatTranscriptCacheStoreTests { #expect(ChatToolDiff.resolveDiff(name: "edit", arguments: nil, details: decoded.details)?.stat == nil) } + @Test func `transcript cache keeps bounded patch envelopes and strips other arguments`() async throws { + let url = try makeDatabaseURL() + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + let oversizedInput = "*** Begin Patch\n*** Add File: cached.txt\n" + + String(repeating: "+line\n", count: 12000) + "*** End Patch" + let message = OpenClawChatMessage( + role: "assistant", + content: [ + OpenClawChatMessageContent( + type: "toolCall", + text: nil, + mimeType: nil, + fileName: nil, + content: nil, + name: "apply_patch", + arguments: AnyCodable([ + "input": AnyCodable(oversizedInput), + "patch": AnyCodable("ignored alias"), + "ignored": AnyCodable("drop me"), + ])), + OpenClawChatMessageContent( + type: "toolCall", + text: nil, + mimeType: nil, + fileName: nil, + content: nil, + name: "exec", + arguments: AnyCodable(["command": AnyCodable("echo secret")])), + ], + timestamp: 1) + + let cacheable = try #require(OpenClawChatSQLiteTranscriptCache.cacheableMessages([message]).first) + let patchArguments = try #require(cacheable.content[0].arguments?.dictionaryValue) + #expect(Set(patchArguments.keys) == ["input"]) + #expect(patchArguments["input"]?.stringValue?.utf16.count == 64000) + #expect(patchArguments["input"]?.stringValue?.hasSuffix("\n...(truncated)...") == true) + #expect(cacheable.content[1].arguments == nil) + + let store = OpenClawChatSQLiteTranscriptCache(databaseURL: url, gatewayID: "gw-a") + await store.storeTranscript(sessionKey: "main", messages: [message]) + let decoded = try #require(await store.loadTranscript(sessionKey: "main").first) + #expect(decoded.content[0].arguments == cacheable.content[0].arguments) + #expect(decoded.content[1].arguments == nil) + let resolved = try #require(ChatToolDiff.resolveDiff( + name: "apply_patch", + arguments: decoded.content[0].arguments, + details: nil)) + #expect(resolved.lines.last?.kind == .skip) + #expect(resolved.stat == nil) + } + @Test func `transcript keeps only most recent messages within bound`() async throws { let url = try makeDatabaseURL() defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } diff --git a/src/agents/filesystem-tools-output-contract.test.ts b/src/agents/filesystem-tools-output-contract.test.ts index 51cdab454715..a74ba322ee51 100644 --- a/src/agents/filesystem-tools-output-contract.test.ts +++ b/src/agents/filesystem-tools-output-contract.test.ts @@ -6,7 +6,7 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { createOpenClawReadTool } from "./agent-tools.read.js"; import type { AnyAgentTool } from "./agent-tools.types.js"; import { createApplyPatchTool } from "./apply-patch.js"; -import { createEditTool, createReadTool } from "./sessions/index.js"; +import { createEditTool, createReadTool, createWriteTool } from "./sessions/index.js"; import { DEFAULT_MAX_BYTES } from "./sessions/tools/truncate.js"; import { compactToolOutputHint } from "./tool-schema-hints.js"; @@ -81,6 +81,38 @@ describe("filesystem tool output contracts", () => { ); }); + it("validates write created, overwrite, unknown-state, and no-op results", async () => { + const tool = createWriteTool(tmpDir) as unknown as AnyAgentTool; + const created = await tool.execute("write-created", { path: "write.txt", content: "one\n" }); + const overwritten = await tool.execute("write-overwrite", { + path: "write.txt", + content: "two\n", + }); + const noOp = await tool.execute("write-no-op", { path: "write.txt", content: "two\n" }); + await fs.writeFile(path.join(tmpDir, "large.txt"), "x".repeat(1024 * 1024 + 1), "utf8"); + const unknownOverwrite = await tool.execute("write-unknown-overwrite", { + path: "large.txt", + content: "replacement\n", + }); + const boundedCreate = await tool.execute("write-bounded-create", { + path: "large-created.txt", + content: "x".repeat(1024 * 1024 + 1), + }); + + for (const result of [created, overwritten, unknownOverwrite, boundedCreate, noOp]) { + expectContract(tool, result.details); + } + expectContract(tool, { changed: true }); + expect(created.details).toMatchObject({ changed: true, created: true }); + expect(overwritten.details).toMatchObject({ changed: true, created: false }); + expect(unknownOverwrite.details).toEqual({ changed: true, created: false }); + expect(boundedCreate.details).toEqual({ changed: true, created: true }); + expect(noOp.details).toEqual({ changed: false }); + expect(compactToolOutputHint(tool.outputSchema)).toBe( + "{ changed: false } | { changed: true; created: true; diff: string; patch: string; firstChangedLine?: number } | { changed: true; created: false; diff: string; patch: string; firstChangedLine?: number } | { changed: true; created?: boolean }", + ); + }); + it("validates apply_patch path summaries", async () => { const tool = createApplyPatchTool({ cwd: tmpDir }) as unknown as AnyAgentTool; const result = await tool.execute("patch-add", { diff --git a/src/agents/sessions/tools/index.ts b/src/agents/sessions/tools/index.ts index ea61de5def94..87cffd03263c 100644 --- a/src/agents/sessions/tools/index.ts +++ b/src/agents/sessions/tools/index.ts @@ -26,6 +26,7 @@ export type { ReadToolDetails, ReadToolInput, ReadToolTruncationDetails, + WriteToolDetails, WriteToolInput, } from "./tool-contracts.js"; export { diff --git a/src/agents/sessions/tools/tool-contracts.ts b/src/agents/sessions/tools/tool-contracts.ts index 6e458ab83bda..bd03f3a4bc34 100644 --- a/src/agents/sessions/tools/tool-contracts.ts +++ b/src/agents/sessions/tools/tool-contracts.ts @@ -103,3 +103,21 @@ export interface WriteToolInput { path: string; content: string; } + +export type WriteToolDetails = + | { changed: false } + | { + changed: true; + created: true; + diff: string; + patch: string; + firstChangedLine?: number; + } + | { + changed: true; + created: false; + diff: string; + patch: string; + firstChangedLine?: number; + } + | { changed: true; created?: boolean }; diff --git a/src/agents/sessions/tools/write.test.ts b/src/agents/sessions/tools/write.test.ts index a549df2c7678..2870352dbc7c 100644 --- a/src/agents/sessions/tools/write.test.ts +++ b/src/agents/sessions/tools/write.test.ts @@ -6,6 +6,7 @@ import path from "node:path"; import { pathToFileURL } from "node:url"; import { expectDefined } from "@openclaw/normalization-core"; import { afterEach, describe, expect, it } from "vitest"; +import { generateDiffString, generateUnifiedPatch } from "./edit-diff.js"; import { createWriteTool, type WriteOperations } from "./write.js"; describe("write tool", () => { @@ -139,21 +140,189 @@ describe("write tool", () => { const tc0 = expectDefined(result.content[0], "result.content[0] test invariant"); expect("text" in tc0 ? tc0.text : "").toContain("No changes made"); expect((result as { terminate?: boolean }).terminate).toBe(true); + expect(result.details).toEqual({ changed: false }); await expect(fs.readFile(filePath, "utf-8")).resolves.toBe("hello\n"); }); - it("writes different content successfully (no false positive for no-op)", async () => { + it("reports a created file with its authoritative diff", async () => { + await createTempPath("created.txt"); + const content = "first\nsecond\n"; + const tool = createWriteTool(tmpDir); + + const result = await tool.execute("call-1", { path: "created.txt", content }, undefined); + const diffResult = generateDiffString("", content); + + expect(result.details).toEqual({ + changed: true, + created: true, + diff: diffResult.diff, + patch: generateUnifiedPatch("created.txt", "", content), + firstChangedLine: diffResult.firstChangedLine, + }); + }); + + it("keeps oversized created-file details bounded", async () => { + await createTempPath("large-created.txt"); + const content = "x".repeat(1024 * 1024 + 1); + const tool = createWriteTool(tmpDir); + + const result = await tool.execute("call-1", { path: "large-created.txt", content }, undefined); + + expect(result.details).toEqual({ changed: true, created: true }); + }); + + it("reports an overwrite with the readable old-content diff", async () => { const filePath = await createTempPath("different.txt"); const content = "new 😀\n"; - await fs.writeFile(filePath, "old\n", "utf-8"); + const oldContent = "old\n"; + await fs.writeFile(filePath, oldContent, "utf-8"); const tool = createWriteTool(tmpDir); const result = await tool.execute("call-1", { path: "different.txt", content }, undefined); + const diffResult = generateDiffString(oldContent, content); expect(result.content[0]).toEqual({ type: "text", text: `Successfully wrote ${Buffer.byteLength(content, "utf8")} bytes to different.txt`, }); + expect(result.details).toEqual({ + changed: true, + created: false, + diff: diffResult.diff, + patch: generateUnifiedPatch("different.txt", oldContent, content), + firstChangedLine: diffResult.firstChangedLine, + }); await expect(fs.readFile(filePath, "utf-8")).resolves.toBe(content); }); + + it("omits the diff when the old content is not valid UTF-8 text", async () => { + const filePath = await createTempPath("binary.bin"); + await fs.writeFile(filePath, Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00, 0xff, 0xfe])); + const tool = createWriteTool(tmpDir); + + const result = await tool.execute( + "call-1", + { path: "binary.bin", content: "text now\n" }, + undefined, + ); + + expect(result.details).toEqual({ changed: true, created: false }); + }); + + it("omits the diff when the rewrite's edit distance blows the budget", async () => { + const filePath = await createTempPath("distinct-lines.txt"); + const oldContent = Array.from({ length: 10_000 }, (_, i) => `old-${i}`).join("\n"); + await fs.writeFile(filePath, oldContent, "utf-8"); + const tool = createWriteTool(tmpDir); + + const result = await tool.execute( + "call-1", + { + path: "distinct-lines.txt", + content: Array.from({ length: 10_000 }, (_, i) => `new-${i}`).join("\n"), + }, + undefined, + ); + + expect(result.details).toEqual({ changed: true, created: false }); + }); + + it("omits the diff for created files with excessive line counts", async () => { + await createTempPath("many-lines-created.txt"); + const tool = createWriteTool(tmpDir); + + const result = await tool.execute( + "call-1", + { path: "many-lines-created.txt", content: "a\n".repeat(25_000) }, + undefined, + ); + + expect(result.details).toEqual({ changed: true, created: true }); + }); + + it("omits the diff when combined line counts exceed the diff budget", async () => { + const filePath = await createTempPath("many-lines.txt"); + await fs.writeFile(filePath, "a\n".repeat(15_000), "utf-8"); + const tool = createWriteTool(tmpDir); + + const result = await tool.execute( + "call-1", + { path: "many-lines.txt", content: "b\n".repeat(15_000) }, + undefined, + ); + + expect(result.details).toEqual({ changed: true, created: false }); + }); + + it("omits the diff when combined old and new content exceeds the diff budget", async () => { + const filePath = await createTempPath("combined.txt"); + await fs.writeFile(filePath, "a".repeat(600 * 1024), "utf-8"); + const tool = createWriteTool(tmpDir); + + const result = await tool.execute( + "call-1", + { path: "combined.txt", content: "b".repeat(600 * 1024) }, + undefined, + ); + + expect(result.details).toEqual({ changed: true, created: false }); + }); + + it("reports an overwrite without a fabricated diff when the old file is too large", async () => { + const filePath = await createTempPath("large.txt"); + await fs.writeFile(filePath, "x".repeat(1024 * 1024 + 1), "utf-8"); + let readCalled = false; + const operations = createRecoverableOperations((absolutePath, content) => + fs.writeFile(absolutePath, content, "utf-8"), + ); + operations.readFile = async () => { + readCalled = true; + throw new Error("oversized pre-write read"); + }; + const tool = createWriteTool(tmpDir, { operations }); + + const result = await tool.execute( + "call-1", + { path: "large.txt", content: "replacement\n" }, + undefined, + ); + + expect(readCalled).toBe(false); + expect(result.details).toEqual({ changed: true, created: false }); + }); + + it("keeps oversized overwrite details bounded", async () => { + const filePath = await createTempPath("large-replacement.txt"); + await fs.writeFile(filePath, "old\n", "utf-8"); + const content = "x".repeat(1024 * 1024 + 1); + const tool = createWriteTool(tmpDir); + + const result = await tool.execute( + "call-1", + { path: "large-replacement.txt", content }, + undefined, + ); + + expect(result.details).toEqual({ changed: true, created: false }); + }); + + it("does not guess creation status when the pre-write stat is unavailable", async () => { + await createTempPath("unknown.txt"); + const operations: WriteOperations = { + mkdir: (dir) => fs.mkdir(dir, { recursive: true }).then(() => {}), + writeFile: (absolutePath, content) => fs.writeFile(absolutePath, content, "utf-8"), + statFile: async () => { + throw new Error("remote stat unavailable"); + }, + }; + const tool = createWriteTool(tmpDir, { operations }); + + const result = await tool.execute( + "call-1", + { path: "unknown.txt", content: "new\n" }, + undefined, + ); + + expect(result.details).toEqual({ changed: true }); + }); }); diff --git a/src/agents/sessions/tools/write.ts b/src/agents/sessions/tools/write.ts index 034c5d66e3b1..fdec18f5df21 100644 --- a/src/agents/sessions/tools/write.ts +++ b/src/agents/sessions/tools/write.ts @@ -11,12 +11,14 @@ import { } from "node:fs/promises"; import { dirname } from "node:path"; import { Container, Text } from "@earendil-works/pi-tui"; +import { structuredPatch } from "diff"; import { Type } from "typebox"; import { keyHint } from "../../modes/interactive/components/keybinding-hints.js"; import { getLanguageFromPath, highlightCode } from "../../modes/interactive/theme/theme.js"; import type { AgentTool } from "../../runtime/index.js"; import { textResult } from "../../tools/common.js"; import type { ToolDefinition, ToolRenderResultOptions } from "../extensions/types.js"; +import { generateDiffString, generateUnifiedPatch } from "./edit-diff.js"; import { withFileMutationQueue } from "./file-mutation-queue.js"; import { resolveToCwd } from "./path-utils.js"; import { @@ -26,6 +28,7 @@ import { shortenPath, str, } from "./render-utils.js"; +import type { WriteToolDetails } from "./tool-contracts.js"; import { wrapToolDefinition } from "./tool-definition-wrapper.js"; const writeSchema = Type.Object({ @@ -34,6 +37,34 @@ const writeSchema = Type.Object({ }), content: Type.String({ description: "File content." }), }); + +const WriteToolOutputSchema = Type.Union([ + Type.Object({ changed: Type.Literal(false) }, { additionalProperties: false }), + Type.Object( + { + changed: Type.Literal(true), + created: Type.Literal(true), + diff: Type.String(), + patch: Type.String(), + firstChangedLine: Type.Optional(Type.Integer({ minimum: 1 })), + }, + { additionalProperties: false }, + ), + Type.Object( + { + changed: Type.Literal(true), + created: Type.Literal(false), + diff: Type.String(), + patch: Type.String(), + firstChangedLine: Type.Optional(Type.Integer({ minimum: 1 })), + }, + { additionalProperties: false }, + ), + Type.Object( + { changed: Type.Literal(true), created: Type.Optional(Type.Boolean()) }, + { additionalProperties: false }, + ), +]); /** * Pluggable operations for the write tool. * Override these to delegate file writing to remote systems (for example SSH). @@ -89,9 +120,31 @@ type WriteToolFileStat = { type WriteToolPrecheck = { state: "different" | "same" | "unknown"; beforeStat?: WriteToolFileStat | null; + beforeText?: string; + readAttempted?: boolean; }; const WRITE_PRECHECK_READ_LIMIT_BYTES = 1024 * 1024; +const WRITE_DIFF_MAX_COMBINED_LINES = 20_000; +const WRITE_DIFF_MAX_EDIT_LENGTH = 2_000; + +// Myers cost is quadratic in edit distance, not input size; probe with the +// library's bounded abort before committing to synchronous diff generation. +function withinWriteDiffBudget(oldContent: string, newContent: string): boolean { + const probe = structuredPatch("", "", oldContent, newContent, undefined, undefined, { + context: 0, + maxEditLength: WRITE_DIFF_MAX_EDIT_LENGTH, + }); + return probe !== undefined; +} + +function countNewlines(text: string): number { + let count = 0; + for (let index = text.indexOf("\n"); index !== -1; index = text.indexOf("\n", index + 1)) { + count += 1; + } + return count; +} type WriteHighlightCache = { rawPath: string | null; @@ -288,7 +341,9 @@ async function readOriginalWriteState( try { stat = await ops.statFile(absolutePath); } catch (error) { - return { state: isMissingFileError(error) ? "different" : "unknown" }; + return isMissingFileError(error) + ? { state: "different", beforeStat: null } + : { state: "unknown" }; } if (!stat) { return { state: "different", beforeStat: stat }; @@ -308,15 +363,106 @@ async function readOriginalWriteState( const originalText = Buffer.isBuffer(originalContent) ? originalContent.toString("utf8") : originalContent; + if (Buffer.byteLength(originalText, "utf8") > WRITE_PRECHECK_READ_LIMIT_BYTES) { + return { state: "unknown", beforeStat: stat, readAttempted: true }; + } return { state: originalText === content ? "same" : "different", beforeStat: stat, + beforeText: originalText, + readAttempted: true, }; } catch { - return { state: "unknown", beforeStat: stat }; + return { state: "unknown", beforeStat: stat, readAttempted: true }; } } +async function resolveWriteDetails(params: { + absolutePath: string; + content: string; + ops: WriteOperations; + path: string; + precheck: WriteToolPrecheck; +}): Promise { + if (Buffer.byteLength(params.content, "utf8") > WRITE_PRECHECK_READ_LIMIT_BYTES) { + // Keep diff work bounded; a partial patch would misrepresent the write. + if (params.precheck.beforeStat === null) { + return { changed: true, created: true }; + } + return params.precheck.beforeStat ? { changed: true, created: false } : { changed: true }; + } + if (params.precheck.beforeStat === null) { + // Same line budget as overwrites: a created file's numbered diff is pure + // duplication of the content and must not balloon the result payload. + if (countNewlines(params.content) > WRITE_DIFF_MAX_COMBINED_LINES) { + return { changed: true, created: true }; + } + const diffResult = generateDiffString("", params.content); + return { + changed: true, + created: true, + diff: diffResult.diff, + patch: generateUnifiedPatch(params.path, "", params.content), + ...(diffResult.firstChangedLine === undefined + ? {} + : { firstChangedLine: diffResult.firstChangedLine }), + }; + } + + const beforeStat = params.precheck.beforeStat; + let beforeText = params.precheck.beforeText; + if ( + beforeText === undefined && + !params.precheck.readAttempted && + beforeStat?.type === "file" && + beforeStat.size <= WRITE_PRECHECK_READ_LIMIT_BYTES && + params.ops.readFile + ) { + const originalContent = await params.ops.readFile(params.absolutePath).catch(() => undefined); + const candidate = Buffer.isBuffer(originalContent) + ? originalContent.toString("utf8") + : originalContent; + if ( + candidate !== undefined && + Buffer.byteLength(candidate, "utf8") <= WRITE_PRECHECK_READ_LIMIT_BYTES + ) { + beforeText = candidate; + } + } + // Lossy UTF-8 decoding would publish garbage as authoritative removals. + if (beforeText !== undefined && (beforeText.includes("\uFFFD") || beforeText.includes("\0"))) { + beforeText = undefined; + } + // Bound Myers-diff work: cost scales with line tokens and edit distance, so + // cap combined bytes AND lines before running the synchronous generator. + if ( + beforeText !== undefined && + (Buffer.byteLength(beforeText, "utf8") + Buffer.byteLength(params.content, "utf8") > + WRITE_PRECHECK_READ_LIMIT_BYTES || + countNewlines(beforeText) + countNewlines(params.content) > WRITE_DIFF_MAX_COMBINED_LINES) + ) { + beforeText = undefined; + } + if (beforeText !== undefined && !withinWriteDiffBudget(beforeText, params.content)) { + beforeText = undefined; + } + if (beforeText !== undefined) { + const diffResult = generateDiffString(beforeText, params.content); + return { + changed: true, + created: false, + diff: diffResult.diff, + patch: generateUnifiedPatch(params.path, beforeText, params.content), + ...(diffResult.firstChangedLine === undefined + ? {} + : { firstChangedLine: diffResult.firstChangedLine }), + }; + } + + // Without confirmed existence, neither removals nor overwrite status can be asserted. + return beforeStat ? { changed: true, created: false } : { changed: true }; +} + async function didWriteMetadataChange( absolutePath: string, beforeStat: WriteToolFileStat | null | undefined, @@ -348,10 +494,10 @@ function isWriteRecoveryCandidate(error: unknown, signal: AbortSignal | undefine ); } -function successfulWriteResult(path: string, content: string) { +function successfulWriteResult(path: string, content: string, details: WriteToolDetails) { return textResult( `Successfully wrote ${Buffer.byteLength(content, "utf8")} bytes to ${path}`, - undefined, + details, ); } @@ -362,6 +508,7 @@ async function recoverSuccessfulWrite(params: { ops: WriteOperations; path: string; precheck: WriteToolPrecheck; + details: WriteToolDetails; signal?: AbortSignal; }) { if (!params.ops.readFile || !isWriteRecoveryCandidate(params.error, params.signal)) { @@ -376,13 +523,13 @@ async function recoverSuccessfulWrite(params: { if (currentContent !== params.content || !changed) { return null; } - return successfulWriteResult(params.path, params.content); + return successfulWriteResult(params.path, params.content, params.details); } export function createWriteToolDefinition( cwd: string, options?: WriteToolOptions, -): ToolDefinition { +): ToolDefinition { const ops = options?.operations ?? defaultWriteOperations; return { name: "write", @@ -391,6 +538,7 @@ export function createWriteToolDefinition( promptSnippet: "Create/overwrite files", promptGuidelines: ["Use only new files/complete rewrites."], parameters: writeSchema, + outputSchema: WriteToolOutputSchema, async execute( toolCallId, { path, content }: { path: string; content: string }, @@ -411,13 +559,13 @@ export function createWriteToolDefinition( // Terminal no-op: file already has identical content. if (precheck.state === "same") { return { - ...textResult( - `No changes made to ${path}. The file already has identical content.`, - undefined, - ), + ...textResult(`No changes made to ${path}. The file already has identical content.`, { + changed: false, + } satisfies WriteToolDetails), terminate: true, }; } + const details = await resolveWriteDetails({ absolutePath, content, ops, path, precheck }); try { await ops.mkdir(dir); if (signal?.aborted) { @@ -427,7 +575,7 @@ export function createWriteToolDefinition( if (signal?.aborted) { throw new Error("Operation aborted"); } - return successfulWriteResult(path, content); + return successfulWriteResult(path, content, details); } catch (error: unknown) { const recovered = await recoverSuccessfulWrite({ absolutePath, @@ -436,6 +584,7 @@ export function createWriteToolDefinition( ops, path, precheck, + details, signal, }); if (recovered) { diff --git a/src/gateway/chat-display-projection.test.ts b/src/gateway/chat-display-projection.test.ts new file mode 100644 index 000000000000..661dd0c067d7 --- /dev/null +++ b/src/gateway/chat-display-projection.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from "vitest"; +import { sanitizeChatHistoryMessages } from "./chat-display-projection.js"; + +describe("chat display tool-result detail projection", () => { + it("keeps authoritative write booleans and strips unrelated details", () => { + const [overwrite, created, invalid] = sanitizeChatHistoryMessages([ + { + role: "toolResult", + toolCallId: "write-1", + toolName: "write", + content: [{ type: "text", text: "ok" }], + details: { changed: true, created: false, diff: "-1 old\n+1 new", private: "drop" }, + }, + { + role: "toolResult", + toolCallId: "write-2", + toolName: "write", + content: [{ type: "text", text: "ok" }], + details: { changed: true, created: true }, + }, + { + role: "toolResult", + toolCallId: "write-3", + toolName: "write", + content: [{ type: "text", text: "ok" }], + details: { changed: "true", created: 1 }, + }, + ]) as Array>; + + expect(overwrite?.details).toEqual({ + changed: true, + created: false, + diff: "-1 old\n+1 new", + }); + expect(created?.details).toEqual({ changed: true, created: true }); + expect(invalid).not.toHaveProperty("details"); + }); +}); diff --git a/src/gateway/chat-display-projection.ts b/src/gateway/chat-display-projection.ts index 62bcbe38206e..e300491a02a6 100644 --- a/src/gateway/chat-display-projection.ts +++ b/src/gateway/chat-display-projection.ts @@ -116,6 +116,11 @@ function projectToolResultDetails( return undefined; } const projected: Record = {}; + for (const key of ["changed", "created"] as const) { + if (typeof record[key] === "boolean") { + projected[key] = record[key]; + } + } if (typeof record.diff === "string" && record.diff.trim()) { projected.diff = truncateChatHistoryText(record.diff, maxChars).text; } diff --git a/ui/src/lib/chat/tool-call-view.test.ts b/ui/src/lib/chat/tool-call-view.test.ts index e83fad96866a..d89b987a4824 100644 --- a/ui/src/lib/chat/tool-call-view.test.ts +++ b/ui/src/lib/chat/tool-call-view.test.ts @@ -514,6 +514,55 @@ describe("resolveToolCallView", () => { }); }); + it("uses authoritative write details for diff and created-flag stats", () => { + const args = { path: "/repo/file.ts", content: "line 1\nline 2\n" }; + + expect( + resolveToolCallView({ + name: "write", + args, + details: { created: false, diff: "-4 old\n+4 replacement" }, + }), + ).toMatchObject({ + diff: [ + { kind: "del", lineNo: 4, text: "old" }, + { kind: "add", lineNo: 4, text: "replacement" }, + ], + stat: { added: 1, removed: 1 }, + }); + + const created = resolveToolCallView({ name: "write", args, details: { created: true } }); + expect(created.stat).toEqual({ added: 2, removed: 0 }); + + const overwrite = resolveToolCallView({ name: "write", args, details: { created: false } }); + expect(overwrite.diff).toEqual([ + { kind: "add", lineNo: 1, text: "line 1" }, + { kind: "add", lineNo: 2, text: "line 2" }, + ]); + expect(overwrite.stat).toBeUndefined(); + + const unknown = resolveToolCallView({ name: "write", args, details: { changed: true } }); + expect(unknown.diff).toEqual(overwrite.diff); + expect(unknown.stat).toBeUndefined(); + + expect(resolveToolCallView({ name: "write", args, details: { changed: false } })).toEqual({ + kind: "write", + target: "file.ts", + targetDetail: "/repo", + }); + }); + + it.each(TEXT_EDITOR_TOOL_NAMES)("applies created-flag stats to %s create", (name) => { + const view = resolveToolCallView({ + name, + args: { command: "create", path: "/repo/file.ts", file_text: "replacement\n" }, + details: { created: false }, + }); + + expect(view.diff).toEqual([{ kind: "add", lineNo: 1, text: "replacement" }]); + expect(view.stat).toBeUndefined(); + }); + it("resolves search views from pattern plus path scope", () => { expect(resolveToolCallView({ name: "grep", args: { pattern: "TODO", path: "src" } })).toEqual({ kind: "search", diff --git a/ui/src/lib/chat/tool-call-view.ts b/ui/src/lib/chat/tool-call-view.ts index 0e51b9ec010b..e658ccb52fa3 100644 --- a/ui/src/lib/chat/tool-call-view.ts +++ b/ui/src/lib/chat/tool-call-view.ts @@ -405,6 +405,20 @@ function buildToolCallView( return { kind: "generic" }; } const { base, dir } = splitPathForDisplay(path); + const authoritativeDiff = readDetailsDiff(source.details); + if (authoritativeDiff) { + return { + kind, + target: base, + targetDetail: dir, + diff: authoritativeDiff.lines, + ...(authoritativeDiff.stat ? { stat: authoritativeDiff.stat } : {}), + }; + } + const details = asRecord(source.details); + if (details?.changed === false) { + return { kind, target: base, targetDetail: dir }; + } const content = args ? editorCommand === "create" ? readString(args.file_text) @@ -419,7 +433,10 @@ function buildToolCallView( target: base, targetDetail: dir, diff, - stat: { added: countTextLines(content), removed: 0 }, + // Present details need created=true before zero removals are authoritative. + ...(details && details.created !== true + ? {} + : { stat: { added: countTextLines(content), removed: 0 } }), }; }