diff --git a/apps/vscode-e2e/src/visual/__screenshots__/electron-chat-dark-sidebar.png b/apps/vscode-e2e/src/visual/__screenshots__/electron-chat-dark-sidebar.png index e8497070ef..a27af05b17 100644 Binary files a/apps/vscode-e2e/src/visual/__screenshots__/electron-chat-dark-sidebar.png and b/apps/vscode-e2e/src/visual/__screenshots__/electron-chat-dark-sidebar.png differ diff --git a/packages/types/src/__tests__/message.test.ts b/packages/types/src/__tests__/message.test.ts index 1fad7c27ed..86433f6194 100644 --- a/packages/types/src/__tests__/message.test.ts +++ b/packages/types/src/__tests__/message.test.ts @@ -1,6 +1,9 @@ // pnpm --filter @roo-code/types test src/__tests__/message.test.ts import { + changeCardDetailSchema, + changeCardFileSchema, + changeCardSchema, clineAsks, clineMessageSchema, getCompletionCheckpoint, @@ -78,3 +81,52 @@ describe("getCompletionCheckpoint", () => { expect(getCompletionCheckpoint(messages)).toBeUndefined() }) }) + +describe("change-card schemas", () => { + it("accepts a valid full-detail change card", () => { + const card = { + stepId: "step-1", + checkpointIds: ["abc123"], + files: [{ path: "src/a.ts", additions: 3, deletions: 1, diff: "@@ -1 +1 @@" }], + totalFiles: 1, + detail: "full", + } + expect(changeCardSchema.parse(card)).toEqual(card) + }) + + it("accepts a summary card without stepId and without per-file diffs", () => { + const card = { + checkpointIds: [], + files: [{ path: "src/b.ts", additions: 0, deletions: 2 }], + totalFiles: 1, + detail: "summary", + } + expect(changeCardSchema.parse(card)).toEqual(card) + }) + + it("rejects invalid detail values", () => { + expect(() => changeCardDetailSchema.parse("detailed")).toThrow() + expect(() => changeCardDetailSchema.parse(undefined)).toThrow() + expect(() => changeCardSchema.parse({ checkpointIds: [], files: [], totalFiles: 0, detail: "diff" })).toThrow() + }) + + it("rejects incomplete file entries", () => { + // Missing additions/deletions. + expect(() => changeCardFileSchema.parse({ path: "src/a.ts" })).toThrow() + // Missing path. + expect(() => changeCardFileSchema.parse({ additions: 1, deletions: 0 })).toThrow() + // Wrong stat type. + expect(() => changeCardFileSchema.parse({ path: "src/a.ts", additions: "1", deletions: 0 })).toThrow() + }) + + it("rejects incomplete payloads", () => { + // Missing checkpointIds. + expect(() => changeCardSchema.parse({ files: [], totalFiles: 1, detail: "full" })).toThrow() + // Missing files. + expect(() => changeCardSchema.parse({ checkpointIds: [], totalFiles: 1, detail: "full" })).toThrow() + // Missing totalFiles. + expect(() => changeCardSchema.parse({ checkpointIds: [], files: [], detail: "full" })).toThrow() + // Missing detail. + expect(() => changeCardSchema.parse({ checkpointIds: [], files: [], totalFiles: 1 })).toThrow() + }) +}) diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index 8513894b0d..cae47c3166 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -12,6 +12,7 @@ import { } from "./provider-settings.js" import { telemetrySettingsSchema } from "./telemetry.js" import { toolNamesSchema } from "./tool.js" +import { changeCardDetailSchema, type ChangeCardDetail } from "./message.js" import { type Keys } from "./type-fu.js" import { languagesSchema } from "./vscode.js" import { providerIdentifiers } from "./provider-identifiers.js" @@ -111,6 +112,21 @@ export const MAX_CHECKPOINT_TIMEOUT_SECONDS = 60 */ export const DEFAULT_CHECKPOINT_TIMEOUT_SECONDS = 15 +/** + * Whether per-write checkpoints and task-start baseline are enabled by default. + * Master switch for the B cluster of checkpoint features. + * @default true + */ +export const DEFAULT_PER_WRITE_CHECKPOINTS = true + +/** + * Default detail level for per-step change cards (B3a). + * "summary" keeps cards compact (file list with +/− counts; the UI fetches + * diffs lazily); "full" carries the unified diff inline per file. + * @default "summary" + */ +export const DEFAULT_CHANGE_CARD_DETAIL: ChangeCardDetail = "summary" + /** * GlobalSettings */ @@ -220,6 +236,19 @@ export const globalSettingsSchema = z.object({ .min(MIN_CHECKPOINT_TIMEOUT_SECONDS) .max(MAX_CHECKPOINT_TIMEOUT_SECONDS) .optional(), + /** + * Whether to record a shadow-git checkpoint after every successful write_to_file, + * edit_file, and apply_patch (per-write checkpoints), plus a task-start baseline. + * @default true + */ + perWriteCheckpoints: z.boolean().optional(), + /** + * Detail level for per-step change cards: "full" includes the unified diff + * inline for every changed file, "summary" carries only the file list with + * +/− counts (diffs are fetched lazily by the UI). + * @default "summary" + */ + changeCardDetail: changeCardDetailSchema.optional(), ttsEnabled: z.boolean().optional(), ttsSpeed: z.number().optional(), diff --git a/packages/types/src/message.ts b/packages/types/src/message.ts index 47cc1ec796..b8d282cb60 100644 --- a/packages/types/src/message.ts +++ b/packages/types/src/message.ts @@ -134,6 +134,7 @@ export function isNonBlockingAsk(ask: ClineAsk): ask is NonBlockingAsk { * - `mcp_server_response`: Response received from MCP server * - `subtask_result`: Result of a completed subtask * - `checkpoint_saved`: Indicates a checkpoint has been saved + * - `change_card`: Per-step change card summarizing the files a completed tool step wrote (B3a) * - `rooignore_error`: Error related to .rooignore file processing * - `diff_error`: Error occurred while applying a diff/patch * - `condense_context`: Context condensation/summarization has started @@ -163,6 +164,7 @@ export const clineSays = [ "mcp_server_response", "subtask_result", "checkpoint_saved", + "change_card", "rooignore_error", "diff_error", "condense_context", @@ -237,6 +239,49 @@ export const contextTruncationSchema = z.object({ export type ContextTruncation = z.infer +/** + * ChangeCard + * + * Payload of the per-step change card (B3a). The extension host emits one + * `say: "change_card"` message per completed tool write step, keyed by the + * shadow-git checkpoint the step produced. The JSON payload (see + * {@link ChangeCardData}) is carried in the message `text` field, the same + * way tool approval messages carry their serialized ClineSayTool. + * + * `detail: "full"` carries the unified diff inline for every file so the UI + * can render it directly; `detail: "summary"` carries only the file list with + * +/− counts and the UI fetches diffs lazily (B3b). Auto-approved steps are + * always emitted with `detail: "summary"` regardless of the user setting. + */ +export const changeCardDetailSchema = z.enum(["full", "summary"]) + +export type ChangeCardDetail = z.infer + +export const changeCardFileSchema = z.object({ + path: z.string(), + additions: z.number(), + deletions: z.number(), + /** + * Unified diff for this file. Only present when the card was emitted with + * `detail: "full"`; summary cards leave it out to stay compact. + */ + diff: z.string().optional(), +}) + +export type ChangeCardFile = z.infer + +export const changeCardSchema = z.object({ + /** Opaque step identifier, reserved for future tool-step tracking. */ + stepId: z.string().optional(), + /** Checkpoint commit SHAs produced by the step (one per per-write checkpoint). */ + checkpointIds: z.array(z.string()), + files: z.array(changeCardFileSchema), + totalFiles: z.number(), + detail: changeCardDetailSchema, +}) + +export type ChangeCardData = z.infer + /** * ClineMessage * diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index c0e8509105..509e63ac01 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -5,7 +5,7 @@ import type { ProviderSettings, ProviderSettingsEntry } from "./provider-setting import type { HistoryItem } from "./history.js" import type { ModeConfig, PromptComponent } from "./mode.js" import type { Experiments } from "./experiment.js" -import type { ClineMessage, QueuedMessage } from "./message.js" +import type { ChangeCardDetail, ClineMessage, QueuedMessage } from "./message.js" import type { MarketplaceItem, MarketplaceInstalledMetadata, InstallMarketplaceItemOptions } from "./marketplace.js" import type { TodoItem } from "./todo.js" import type { CloudUserInfo, CloudOrganizationMembership, OrganizationAllowList, ShareVisibility } from "./cloud.js" @@ -352,6 +352,8 @@ export type ExtensionState = Pick< enableCheckpoints: boolean checkpointTimeout: number // Timeout for checkpoint initialization in seconds (default: 15) + perWriteCheckpoints: boolean + changeCardDetail: ChangeCardDetail maxOpenTabsContext: number // Maximum number of VSCode open tabs to include in context (0-500) maxWorkspaceFiles: number // Maximum number of files to include in current working directory details (0-500) showRooIgnoredFiles: boolean // Whether to show .rooignore'd files in listings diff --git a/src/core/checkpoints/__tests__/changeCard.spec.ts b/src/core/checkpoints/__tests__/changeCard.spec.ts new file mode 100644 index 0000000000..82190638a1 --- /dev/null +++ b/src/core/checkpoints/__tests__/changeCard.spec.ts @@ -0,0 +1,108 @@ +import { describe, expect, it } from "vitest" + +import type { ChangeCardData } from "@roo-code/types" + +import { + buildChangeCard, + buildChangeCardPayload, + isAutoApprovedStep, + resolveChangeCardDetail, + type ChangeCardWrite, +} from "../changeCard" + +describe("changeCard (B3a)", () => { + function write(overrides: Partial = {}): ChangeCardWrite { + return { + path: "src/a.ts", + diffStats: { additions: 2, deletions: 1 }, + diff: "--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1 +1,2 @@\n-old\n+new-1\n+new-2", + ...overrides, + } + } + + describe("isAutoApprovedStep", () => { + it("returns false for an empty step", () => { + expect(isAutoApprovedStep([])).toBe(false) + }) + + it("returns true only when every write was auto-approved", () => { + expect(isAutoApprovedStep([write({ autoApproved: true }), write({ autoApproved: true })])).toBe(true) + expect(isAutoApprovedStep([write({ autoApproved: true }), write()])).toBe(false) + expect(isAutoApprovedStep([write()])).toBe(false) + }) + }) + + describe("resolveChangeCardDetail", () => { + it("forces summary for auto-approved steps even when the setting is full", () => { + const writes = [write({ autoApproved: true })] + expect(resolveChangeCardDetail(writes, "full")).toBe("summary") + expect(resolveChangeCardDetail(writes, undefined)).toBe("summary") + }) + + it("follows the setting for interactive steps, defaulting to summary when unset", () => { + const writes = [write()] + expect(resolveChangeCardDetail(writes, "full")).toBe("full") + expect(resolveChangeCardDetail(writes, "summary")).toBe("summary") + expect(resolveChangeCardDetail(writes, undefined)).toBe("summary") + }) + }) + + describe("buildChangeCard", () => { + it("carries the inline diff per file for full detail on a multi-file step", () => { + const card = buildChangeCard( + "sha-1", + [write(), write({ path: "src/b.ts", diffStats: { additions: 1, deletions: 0 }, diff: "+b" })], + "full", + ) + + expect(card).toEqual({ + checkpointIds: ["sha-1"], + files: [ + { + path: "src/a.ts", + additions: 2, + deletions: 1, + diff: "--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1 +1,2 @@\n-old\n+new-1\n+new-2", + }, + { path: "src/b.ts", additions: 1, deletions: 0, diff: "+b" }, + ], + totalFiles: 2, + detail: "full", + }) + }) + + it("omits the diff per file for summary detail (lazy fetch is B3b)", () => { + const card = buildChangeCard("sha-1", [write()], "summary") + + expect(card.files).toEqual([{ path: "src/a.ts", additions: 2, deletions: 1 }]) + expect(card.files[0]).not.toHaveProperty("diff") + expect(card.detail).toBe("summary") + expect(card.totalFiles).toBe(1) + }) + + it("defaults missing diffStats to zero counts and keeps full detail without diff for a write without one", () => { + const card = buildChangeCard("sha-1", [write({ diffStats: undefined, diff: undefined })], "full") + + expect(card.files[0]).toEqual({ path: "src/a.ts", additions: 0, deletions: 0 }) + }) + }) + + describe("buildChangeCardPayload", () => { + it("resolves the detail level and builds the payload in one call", () => { + // The expectations are typed against the shared ChangeCardData + // contract in @roo-code/types, so the builder's output is checked + // against the same single source of truth the webview consumes. + // Interactive step with the full setting: diff inline. + const full: ChangeCardData = buildChangeCardPayload("sha-1", [write()], "full") + expect(full.detail).toBe("full") + expect(full.files[0].diff).toBe("--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1 +1,2 @@\n-old\n+new-1\n+new-2") + + // Auto-approved step with the full setting: compact summary, no diff. + const compact: ChangeCardData = buildChangeCardPayload("sha-1", [write({ autoApproved: true })], "full") + expect(compact.detail).toBe("summary") + expect(compact.files[0]).not.toHaveProperty("diff") + expect(compact.checkpointIds).toEqual(["sha-1"]) + expect(compact.totalFiles).toBe(1) + }) + }) +}) diff --git a/src/core/checkpoints/__tests__/changeJournal.spec.ts b/src/core/checkpoints/__tests__/changeJournal.spec.ts new file mode 100644 index 0000000000..68c4166da8 --- /dev/null +++ b/src/core/checkpoints/__tests__/changeJournal.spec.ts @@ -0,0 +1,456 @@ +import fs from "fs/promises" +import os from "os" +import path from "path" + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" + +import { appendChange, appendChanges, journalPath, loadChanges, type ChangeJournalEntry } from "../changeJournal" + +describe("changeJournal", () => { + const taskId = "test-task" + + let tmpRoot: string + + beforeEach(async () => { + tmpRoot = await fs.mkdtemp(path.join(os.tmpdir(), "b2-journal-")) + }) + + afterEach(async () => { + await fs.rm(tmpRoot, { recursive: true, force: true }) + }) + + function entry(overrides: Partial = {}): ChangeJournalEntry { + return { + path: "src/foo.ts", + operation: "create", + checkpointId: "abc12345", + ...overrides, + } + } + + describe("appendChange", () => { + it("writes one JSON line per entry with the documented field shape", async () => { + await appendChange(tmpRoot, taskId, entry({ operation: "create", checkpointId: "aaa" })) + + const raw = await fs.readFile(journalPath(tmpRoot, taskId), "utf8") + const lines = raw.split("\n").filter((line) => line !== "") + expect(lines).toHaveLength(1) + const parsed = JSON.parse(lines[0]) as ChangeJournalEntry + expect(parsed.path).toBe("src/foo.ts") + expect(parsed.operation).toBe("create") + expect(parsed.checkpointId).toBe("aaa") + }) + + it("appends multiple entries sequentially", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "a" })) + await appendChange(tmpRoot, taskId, entry({ checkpointId: "b" })) + + const raw = await fs.readFile(journalPath(tmpRoot, taskId), "utf8") + expect(raw.split("\n").filter((line) => line !== "")).toHaveLength(2) + }) + + it("repairs a torn tail before appending so both entries load", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "good" })) + // Simulate a crash that tore the last append mid-write: the file now + // ends with an unterminated JSON fragment. + const filePath = journalPath(tmpRoot, taskId) + const raw = await fs.readFile(filePath, "utf8") + await fs.writeFile(filePath, raw + '{"path":"src/half.ts","operation":"upd') + + await appendChange(tmpRoot, taskId, entry({ checkpointId: "after" })) + + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["good", "after"]) + + // The lost fragment is truncated away, not preserved (a later + // strict reader must never see it. + const repaired = await fs.readFile(filePath, "utf8") + expect(repaired).not.toContain("half.ts") + expect(repaired).not.toContain("half.ts") + }) + + it("does not insert a blank line when the journal ends cleanly", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "a" })) + await appendChange(tmpRoot, taskId, entry({ checkpointId: "b" })) + + const raw = await fs.readFile(journalPath(tmpRoot, taskId), "utf8") + expect(raw).not.toContain("\n\n") + expect(raw.split("\n").filter((line) => line !== "")).toHaveLength(2) + }) + + it("syncs the parent directory after every journal replacement (POSIX)", async () => { + const platformDesc = Object.getOwnPropertyDescriptor(process, "platform") + Object.defineProperty(process, "platform", { value: "linux", configurable: true }) + const dirPath = path.dirname(journalPath(tmpRoot, taskId)) + const realOpen = fs.open + const synced: string[] = [] + const closed: string[] = [] + // Windows cannot open directories, so the directory handle is doubled + // here: the code under test only opens it on non-win32 platforms. + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (pathish, flags) => { + const openAs = realOpen as (p: string | URL, f?: string) => Promise + const p = typeof pathish === "string" ? pathish : String(pathish) + if (p === dirPath && flags === "r") { + return { + sync: async () => { + synced.push(p) + }, + close: async () => { + closed.push(p) + }, + } as unknown as fs.FileHandle + } + return openAs(p, flags as string | undefined) + }) + try { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "first" })) + expect(synced).toEqual([dirPath]) + expect(closed).toEqual([dirPath]) + // Every replacement renames a new file into place, so the directory + // entry is re-synced on each update. + await appendChange(tmpRoot, taskId, entry({ checkpointId: "second" })) + expect(synced).toHaveLength(2) + } finally { + openSpy.mockRestore() + if (platformDesc) Object.defineProperty(process, "platform", platformDesc) + } + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["first", "second"]) + }) + + it("propagates a non-ENOENT journal-read failure instead of replacing blind", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "good" })) + // The current content cannot be read (I/O error): replacing would + // discard whatever the unreadable journal really held, so appendChange + // must fail loudly. + const readSpy = vi + .spyOn(fs, "readFile") + .mockRejectedValueOnce(Object.assign(new Error("simulated I/O error"), { code: "EIO" })) + try { + await expect(appendChange(tmpRoot, taskId, entry({ checkpointId: "after" }))).rejects.toMatchObject({ + code: "EIO", + }) + } finally { + readSpy.mockRestore() + } + // The failed append wrote nothing: the next append still repairs and loads. + await appendChange(tmpRoot, taskId, entry({ checkpointId: "after" })) + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["good", "after"]) + }) + + it("writes the full record when the underlying write completes partially", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "first" })) + const realOpen = fs.open + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (pathish, flags) => { + const openAs = realOpen as (p: string | URL, f?: string) => Promise + const handle = await openAs(pathish as string | URL, flags as string | undefined) + if (flags === "w") { + const realWrite = handle.write.bind(handle) + let first = true + handle.write = (async (buf: Uint8Array, off?: number, len?: number) => { + const length = len ?? buf.length + if (first && length > 1) { + first = false + await realWrite(buf, off, 1) + return { bytesWritten: 1, buffer: Buffer.from(buf) } + } + return realWrite(buf, off, length) + }) as typeof handle.write + } + return handle + }) + try { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "second" })) + } finally { + openSpy.mockRestore() + } + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["first", "second"]) + }) + + it("preserves a complete final record that lost only its newline", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "good" })) + // Simulate a crash that flushed a complete record but lost its + // terminating newline: the record is entry-shaped and parseable, so + // the repair must keep it and re-terminate instead of truncating. + const filePath = journalPath(tmpRoot, taskId) + const raw = await fs.readFile(filePath, "utf8") + await fs.writeFile(filePath, raw + JSON.stringify(entry({ checkpointId: "complete" }))) + + await appendChange(tmpRoot, taskId, entry({ checkpointId: "after" })) + + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["good", "complete", "after"]) + }) + + it("retries a Windows rename rejected with EPERM", async () => { + const platformDesc = Object.getOwnPropertyDescriptor(process, "platform") + Object.defineProperty(process, "platform", { value: "win32", configurable: true }) + const realRename = fs.rename + let attempts = 0 + const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async (from, to) => { + attempts++ + if (attempts === 1) { + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + await realRename(from as string | URL, to as string | URL) + }) + try { + // An antivirus/indexer transiently holding the destination must + // not fail the append: the rename is retried with a short backoff. + await appendChange(tmpRoot, taskId, entry({ checkpointId: "first" })) + expect(attempts).toBe(2) + } finally { + renameSpy.mockRestore() + if (platformDesc) Object.defineProperty(process, "platform", platformDesc) + } + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["first"]) + }) + + it("does not retry a POSIX rename rejection", async () => { + const platformDesc = Object.getOwnPropertyDescriptor(process, "platform") + Object.defineProperty(process, "platform", { value: "linux", configurable: true }) + let attempts = 0 + const renameSpy = vi.spyOn(fs, "rename").mockImplementation(async () => { + attempts++ + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + }) + try { + await expect(appendChange(tmpRoot, taskId, entry({ checkpointId: "x" }))).rejects.toMatchObject({ + code: "EPERM", + }) + expect(attempts).toBe(1) + } finally { + renameSpy.mockRestore() + if (platformDesc) Object.defineProperty(process, "platform", platformDesc) + } + }) + + it("tolerates an unsupported directory-sync errno after the rename (POSIX)", async () => { + const platformDesc = Object.getOwnPropertyDescriptor(process, "platform") + Object.defineProperty(process, "platform", { value: "linux", configurable: true }) + const dirPath = path.dirname(journalPath(tmpRoot, taskId)) + const realOpen = fs.open + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (pathish, flags) => { + const openAs = realOpen as (p: string | URL, f?: string) => Promise + const p = typeof pathish === "string" ? pathish : String(pathish) + if (p === dirPath && flags === "r") { + return { + sync: async () => { + throw Object.assign(new Error("EINVAL: invalid argument"), { code: "EINVAL" }) + }, + close: async () => {}, + } as unknown as fs.FileHandle + } + return openAs(p, flags as string | undefined) + }) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + try { + // The rename already replaced the journal: an fsync-unsupported + // mount must not fail the update, only reduce crash durability. + await appendChange(tmpRoot, taskId, entry({ checkpointId: "kept" })) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("directory sync unsupported")) + } finally { + openSpy.mockRestore() + warnSpy.mockRestore() + if (platformDesc) Object.defineProperty(process, "platform", platformDesc) + } + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["kept"]) + }) + + it("propagates a genuine directory-sync failure (POSIX)", async () => { + const platformDesc = Object.getOwnPropertyDescriptor(process, "platform") + Object.defineProperty(process, "platform", { value: "linux", configurable: true }) + const dirPath = path.dirname(journalPath(tmpRoot, taskId)) + const realOpen = fs.open + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (pathish, flags) => { + const openAs = realOpen as (p: string | URL, f?: string) => Promise + const p = typeof pathish === "string" ? pathish : String(pathish) + if (p === dirPath && flags === "r") { + return { + sync: async () => { + throw Object.assign(new Error("EIO: i/o error"), { code: "EIO" }) + }, + close: async () => {}, + } as unknown as fs.FileHandle + } + return openAs(p, flags as string | undefined) + }) + try { + // A real I/O failure is not an "unsupported" case: it still + // propagates so callers know durability is in doubt. + await expect(appendChange(tmpRoot, taskId, entry({ checkpointId: "x" }))).rejects.toMatchObject({ + code: "EIO", + }) + } finally { + openSpy.mockRestore() + if (platformDesc) Object.defineProperty(process, "platform", platformDesc) + } + }) + + it("serializes concurrent updates to the same task", async () => { + await Promise.all([ + appendChange(tmpRoot, taskId, entry({ checkpointId: "c1" })), + appendChange(tmpRoot, taskId, entry({ checkpointId: "c2" })), + ]) + + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId).sort()).toEqual(["c1", "c2"]) + }) + + it("leaves the journal untouched when the atomic rename fails mid-update", async () => { + await appendChanges(tmpRoot, taskId, [entry({ checkpointId: "kept" })]) + const renameSpy = vi.spyOn(fs, "rename").mockRejectedValueOnce(new Error("simulated rename failure")) + try { + await expect( + appendChanges(tmpRoot, taskId, [ + entry({ checkpointId: "m1", path: "src/m1.ts" }), + entry({ checkpointId: "m2", path: "src/m2.ts" }), + ]), + ).rejects.toThrow("simulated rename failure") + } finally { + renameSpy.mockRestore() + } + // Neither of the two new entries survived: the update is all-or-nothing. + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["kept"]) + // The failed temp file was cleaned up. + const leftovers = (await fs.readdir(path.dirname(journalPath(tmpRoot, taskId)))).filter((name) => + name.endsWith(".tmp"), + ) + expect(leftovers).toEqual([]) + }) + + it("recovers after a crash left a stale temp file and a torn tail", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "before" })) + const dir = path.dirname(journalPath(tmpRoot, taskId)) + // A crash between the temp write and the rename leaves an orphan temp + // file; a crash mid-write can also leave the tail unterminated. + await fs.writeFile(path.join(dir, "changes.jsonl.4242.orphan.tmp"), "partial") + const raw = await fs.readFile(journalPath(tmpRoot, taskId), "utf8") + await fs.writeFile(journalPath(tmpRoot, taskId), raw + '{"path":"src/half.ts","operation":"upd') + + await appendChange(tmpRoot, taskId, entry({ checkpointId: "after" })) + + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["before", "after"]) + }) + + it("does not create a journal for an empty update", async () => { + await appendChanges(tmpRoot, taskId, []) + await expect(fs.stat(journalPath(tmpRoot, taskId))).rejects.toMatchObject({ code: "ENOENT" }) + }) + }) + + describe("task ID containment", () => { + it("rejects task IDs that could escape the tasks directory", async () => { + for (const bad of ["../../outside", "a/b", "a\\b", "", ".", ".."]) { + expect(() => journalPath(tmpRoot, bad)).toThrow(/Unsafe task ID/) + await expect(appendChange(tmpRoot, bad, entry({ checkpointId: "x" }))).rejects.toThrow(/Unsafe task ID/) + await expect(loadChanges(tmpRoot, bad)).rejects.toThrow(/Unsafe task ID/) + // Nothing was created outside the task directory. + await expect(fs.stat(path.join(tmpRoot, "..", "outside"))).rejects.toMatchObject({ code: "ENOENT" }) + } + }) + + it("keeps a safe task ID under the tasks directory", () => { + expect(journalPath(tmpRoot, "task-42")).toBe( + path.join(tmpRoot, "tasks", "task-42", "checkpoints", "changes.jsonl"), + ) + }) + }) + + describe("loadChanges", () => { + it("returns [] for an absent journal file", async () => { + expect(await loadChanges(tmpRoot, taskId)).toEqual([]) + }) + + it("returns [] for an empty journal file", async () => { + await fs.mkdir(path.dirname(journalPath(tmpRoot, taskId)), { recursive: true }) + await fs.writeFile(journalPath(tmpRoot, taskId), "") + + expect(await loadChanges(tmpRoot, taskId)).toEqual([]) + }) + + it("parses all entries in order with a clean tail", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "x" })) + await appendChange(tmpRoot, taskId, entry({ checkpointId: "y" })) + await appendChange(tmpRoot, taskId, entry({ checkpointId: "z" })) + + const result = await loadChanges(tmpRoot, taskId) + expect(result).toHaveLength(3) + expect(result[0].checkpointId).toBe("x") + expect(result[1].checkpointId).toBe("y") + expect(result[2].checkpointId).toBe("z") + }) + + it("parses a journal whose final line has no trailing newline", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "ok" })) + + // Rewrite the file without the trailing newline of the last line. + const filePath = journalPath(tmpRoot, taskId) + const content = (await fs.readFile(filePath, "utf8")).replace(/\n$/, "") + await fs.writeFile(filePath, content) + + const result = await loadChanges(tmpRoot, taskId) + expect(result).toHaveLength(1) + expect(result[0].checkpointId).toBe("ok") + }) + + it("rejects a torn final line instead of silently dropping it", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "ok" })) + + // Append a second line truncated mid-content, with no trailing + // newline. Without verified truncation there is no evidence this + // is a crash tail rather than a corrupted record, and silently + // dropping it would report an incomplete history as valid. + await fs.appendFile(journalPath(tmpRoot, taskId), '{"path":"src/half.ts","operation":"upd') + + await expect(loadChanges(tmpRoot, taskId)).rejects.toThrow(/Corrupt change journal entry at line 2/) + }) + + it("skips a corrupt middle line and still loads the later valid entries", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "ok" })) + + // Corrupt the first line in place, then append a valid entry after it. + await fs.writeFile( + journalPath(tmpRoot, taskId), + '"{"path":"src/corrupt.ts","operation":"update"\n' + + JSON.stringify(entry({ checkpointId: "after" })) + + "\n", + ) + + const result = await loadChanges(tmpRoot, taskId) + expect(result).toHaveLength(1) + expect(result[0].checkpointId).toBe("after") + }) + + it("rejects a journal whose only record is torn", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "first" })) + + // Truncate to a single character — definitely invalid JSON. The + // append path repairs such a tail on the next write; a read must + // not report the lost record as a complete (empty) history. + await fs.writeFile(journalPath(tmpRoot, taskId), "{") + + await expect(loadChanges(tmpRoot, taskId)).rejects.toThrow(/Corrupt change journal entry at line 1/) + }) + + it("includes diffStats when present", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "s", diffStats: { additions: 5, deletions: 2 } })) + + const result = await loadChanges(tmpRoot, taskId) + expect(result[0].diffStats).toEqual({ additions: 5, deletions: 2 }) + }) + + it("omits diffStats when not provided", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "n" })) + + const result = await loadChanges(tmpRoot, taskId) + expect(result[0].diffStats).toBeUndefined() + }) + }) +}) diff --git a/src/core/checkpoints/__tests__/checkpointJournal.test.ts b/src/core/checkpoints/__tests__/checkpointJournal.test.ts new file mode 100644 index 0000000000..a48d9eea69 --- /dev/null +++ b/src/core/checkpoints/__tests__/checkpointJournal.test.ts @@ -0,0 +1,318 @@ +import fs from "fs/promises" +import os from "os" +import path from "path" + +import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from "vitest" + +import type { Task } from "../../task/Task" +import { loadChanges } from "../changeJournal" +import { checkpointSave, type CheckpointWriteInfo } from "../index" + +// Mock the VS Code API surface (index.ts imports vscode at module level). +vi.mock("vscode", () => ({ + window: { + showErrorMessage: vi.fn(), + showInformationMessage: vi.fn(), + createTextEditorDecorationType: vi.fn(() => ({})), + }, + Uri: { + file: vi.fn((p: string) => ({ fsPath: p })), + parse: vi.fn((_uri: string) => ({ with: vi.fn(() => ({})) })), + }, + commands: { + executeCommand: vi.fn(), + }, +})) + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureCheckpointCreated: vi.fn(), + captureCheckpointRestored: vi.fn(), + captureCheckpointDiffed: vi.fn(), + }, + }, +})) + +vi.mock("../../../utils/path", () => ({ + getWorkspacePath: vi.fn(() => "/test/workspace"), +})) + +vi.mock("../../../utils/git", () => ({ + checkGitInstalled: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../../i18n", () => ({ + t: vi.fn((key: string) => key), +})) + +vi.mock("p-wait-for", () => ({ + default: vi.fn(), +})) + +// The real service would require a git repo; the checkpointSave under test +// only needs the pre-initialized service on the task. +vi.mock("../../../services/checkpoints") + +const TASK_ID = "journal-test-task" +const COMMIT = "test-commit-hash" + +interface ServiceLike { + isInitialized: boolean + saveCheckpoint: (...args: unknown[]) => Promise +} + +interface ProviderLike { + context: { globalStorageUri: { fsPath: string } } + log: (...args: unknown[]) => void + postMessageToWebview: (...args: unknown[]) => void + getState: () => Promise> +} + +interface TaskLike { + taskId: string + enableCheckpoints: boolean + checkpointService: ServiceLike + checkpointServiceInitializing: boolean + providerRef: { deref: () => ProviderLike | undefined } + say: (...args: unknown[]) => Promise +} + +describe("checkpointSave change-journal wiring (B2)", () => { + let tmpStorageDir: string + let saveCheckpointSpy: Mock + let mockProvider: ProviderLike + let mockTask: TaskLike + const write: CheckpointWriteInfo = { + path: "src/foo.ts", + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + } + + beforeEach(async () => { + tmpStorageDir = await fs.mkdtemp(path.join(os.tmpdir(), "b2-journal-wiring-")) + saveCheckpointSpy = vi.fn().mockResolvedValue({ commit: COMMIT }) + mockProvider = { + context: { globalStorageUri: { fsPath: tmpStorageDir } }, + log: vi.fn(), + postMessageToWebview: vi.fn(), + // B3a: the card emission reads the live settings through getState. + getState: vi.fn().mockResolvedValue({}), + } + // Structural test double for Task (the class is not instantiated at + // this unit layer); the cast is safe because the fields checkpointSave + // reads are exactly these. + mockTask = { + taskId: TASK_ID, + enableCheckpoints: true, + checkpointService: { isInitialized: true, saveCheckpoint: saveCheckpointSpy }, + checkpointServiceInitializing: false, + providerRef: { deref: () => mockProvider }, + // B3a: the card emission calls task.say; a resolved double keeps the + // test double complete instead of letting the emission take the + // error path. + say: vi.fn().mockResolvedValue(undefined), + } + }) + + afterEach(async () => { + vi.restoreAllMocks() + await fs.rm(tmpStorageDir, { recursive: true, force: true }) + }) + + it("appends exactly one journal line referencing the B1 checkpoint id for a per-write save", async () => { + await checkpointSave(mockTask as Task, false, true, write) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toHaveLength(1) + expect(entries[0]).toEqual({ + path: "src/foo.ts", + operation: "create", + checkpointId: COMMIT, + diffStats: { additions: 3, deletions: 0 }, + }) + + // The raw file holds exactly one JSON line. + const journalFile = path.join(tmpStorageDir, "tasks", TASK_ID, "checkpoints", "changes.jsonl") + const raw = await fs.readFile(journalFile, "utf8") + expect(raw.split("\n").filter((line) => line !== "")).toHaveLength(1) + }) + + it("omits diffStats in the journal entry when not provided", async () => { + await checkpointSave(mockTask as Task, false, true, { path: "src/bar.ts", operation: "update" }) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toHaveLength(1) + expect(entries[0].path).toBe("src/bar.ts") + expect(entries[0].operation).toBe("update") + expect(entries[0].checkpointId).toBe(COMMIT) + expect(entries[0].diffStats).toBeUndefined() + }) + + it("does not write a journal entry for non-write checkpoint saves (task-start baseline)", async () => { + await checkpointSave(mockTask as Task) + + const journalFile = path.join(tmpStorageDir, "tasks", TASK_ID, "checkpoints", "changes.jsonl") + await expect(fs.stat(journalFile)).rejects.toThrow() + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + }) + + it("appends one entry per file change for a multi-file write (apply-patch shape)", async () => { + await checkpointSave(mockTask as Task, false, true, [ + { path: "src/a.ts", operation: "create" }, + { path: "src/b.ts", operation: "update" }, + { path: "src/c.ts", operation: "delete" }, + ]) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toHaveLength(3) + // Every entry references the single checkpoint of the whole patch. + expect(entries.map((entry) => entry.checkpointId)).toEqual([COMMIT, COMMIT, COMMIT]) + expect(entries.map((entry) => entry.path)).toEqual(["src/a.ts", "src/b.ts", "src/c.ts"]) + expect(entries.map((entry) => entry.operation)).toEqual(["create", "update", "delete"]) + }) + + it("keeps the existing error-swallowing behavior for non-write saves on failure", async () => { + // The legacy swallow (log + disable + resolve undefined) is reserved + // for checkpoint calls without write metadata, e.g. the task-start + // baseline: no file write has happened, so there is no orphaned write + // to report. + saveCheckpointSpy.mockRejectedValueOnce(new Error("git exploded")) + + await expect(checkpointSave(mockTask as Task)).resolves.toBeUndefined() + expect(mockTask.enableCheckpoints).toBe(false) + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + }) + + it("propagates a saveCheckpoint rejection for a write-metadata save", async () => { + // The file write already happened: swallowing the commit failure + // would report a tool success whose rollback metadata does not exist. + saveCheckpointSpy.mockRejectedValueOnce(new Error("git exploded")) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined) + + await expect(checkpointSave(mockTask as Task, false, true, write)).rejects.toThrow(/no rollback metadata/) + + // Checkpoints are still disabled on failure, and neither the journal + // nor the change card is written for a commit that never happened. + expect(mockTask.enableCheckpoints).toBe(false) + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + const cardCalls = vi.mocked(mockTask.say).mock.calls.filter((call) => call[0] === "change_card") + expect(cardCalls).toHaveLength(0) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("caught unexpected error"), + expect.anything(), + ) + consoleErrorSpy.mockRestore() + }) + + it("does not write a journal entry when the checkpoint save is a no-op (empty commit)", async () => { + saveCheckpointSpy.mockResolvedValueOnce(undefined) + + await checkpointSave(mockTask as Task, false, true, write) + + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + expect(mockTask.enableCheckpoints).toBe(true) + }) + + it("fails a write-metadata save before committing when the provider has no globalStorageDir", async () => { + // A commit whose journal update would be silently skipped is worse + // than no commit: rollback metadata would be incomplete with no error + // surfaced, so the save must fail before the Git commit runs. + mockTask.providerRef = { deref: () => undefined } + + await expect(checkpointSave(mockTask as Task, false, true, write)).rejects.toThrow(/stable journal location/) + expect(saveCheckpointSpy).not.toHaveBeenCalled() + expect(mockTask.enableCheckpoints).toBe(true) + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + }) + + it("persists the journal through the captured location when the provider disappears during the commit", async () => { + // The weak provider reference dies while the Git save is awaited: + // the journal location captured before the commit must still be used, + // so a successful commit never silently loses its rollback metadata. + saveCheckpointSpy.mockImplementation(async () => { + mockTask.providerRef = { deref: () => undefined } + return { commit: COMMIT } + }) + + await expect(checkpointSave(mockTask as Task, false, true, write)).resolves.toMatchObject({ commit: COMMIT }) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toHaveLength(1) + expect(entries[0]).toMatchObject({ path: "src/foo.ts", checkpointId: COMMIT }) + expect(mockTask.enableCheckpoints).toBe(true) + }) + + it("still runs non-write checkpoint saves when the provider is gone (task-start baseline)", async () => { + // The pre-commit journal-location guard applies only to saves that + // carry write metadata; the baseline save has none and keeps the + // historical behavior. + mockTask.providerRef = { deref: () => undefined } + + await expect(checkpointSave(mockTask as Task)).resolves.toMatchObject({ commit: COMMIT }) + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + }) + + it("propagates a journal persistence failure after the commit and emits no change card", async () => { + // Block the per-task checkpoint dir so the journal mkdir/read fails. + const taskDir = path.join(tmpStorageDir, "tasks", TASK_ID) + await fs.mkdir(taskDir, { recursive: true }) + await fs.writeFile(path.join(taskDir, "checkpoints"), "blocker") + + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined) + + // The checkpoint committed but the journal update did not persist: the + // save must not report a fully durable checkpoint, and the change card + // (which advertises the journaled files) must not be emitted. + await expect(checkpointSave(mockTask as Task, false, true, write)).rejects.toMatchObject({ + name: "CheckpointPartialPersistenceError", + checkpointId: COMMIT, + writes: [{ path: "src/foo.ts", operation: "create", diffStats: { additions: 3, deletions: 0 } }], + }) + + // Propagation is not a checkpoint error: checkpoints stay enabled. + expect(mockTask.enableCheckpoints).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("failed to persist change journal entries"), + expect.anything(), + ) + const cardCalls = vi.mocked(mockTask.say).mock.calls.filter((call) => call[0] === "change_card") + expect(cardCalls).toHaveLength(0) + }) +}) + +describe("loadChanges entry-shape validation (B2)", () => { + let tmpStorageDir: string + + beforeEach(async () => { + tmpStorageDir = await fs.mkdtemp(path.join(os.tmpdir(), "b2-journal-shape-")) + }) + + afterEach(async () => { + await fs.rm(tmpStorageDir, { recursive: true, force: true }) + }) + + it("skips lines that are valid JSON but not entry-shaped", async () => { + const dir = path.join(tmpStorageDir, "tasks", TASK_ID, "checkpoints") + await fs.mkdir(dir, { recursive: true }) + const valid = JSON.stringify({ path: "src/a.ts", operation: "create", checkpointId: "c1" }) + await fs.writeFile( + path.join(dir, "changes.jsonl"), + [ + // A torn append can leave valid JSON that is not an entry. + "1", + "null", + // Missing checkpointId. + JSON.stringify({ path: "src/b.ts", operation: "update" }), + // Unknown operation. + JSON.stringify({ path: "src/c.ts", operation: "rename", checkpointId: "c2" }), + // Non-string path. + JSON.stringify({ path: 42, operation: "create", checkpointId: "c3" }), + valid, + ].join("\n") + "\n", + ) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toEqual([{ path: "src/a.ts", operation: "create", checkpointId: "c1" }]) + }) +}) diff --git a/src/core/checkpoints/__tests__/checkpointSave.spec.ts b/src/core/checkpoints/__tests__/checkpointSave.spec.ts new file mode 100644 index 0000000000..291bc4cdea --- /dev/null +++ b/src/core/checkpoints/__tests__/checkpointSave.spec.ts @@ -0,0 +1,236 @@ +import fs from "fs/promises" +import os from "os" +import path from "path" + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" + +import type { Task } from "../../task/Task" +import { journalPath } from "../changeJournal" +import { checkpointSave } from "../index" + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureCheckpointCreated: vi.fn(), + captureCheckpointRestored: vi.fn(), + captureCheckpointDiffed: vi.fn(), + }, + }, +})) + +/** + * Minimal Task double for the checkpointSave wiring tests. Only the members + * touched by getCheckpointService + checkpointSave are provided: a pre-set + * checkpointService (so no git installation happens), the provider context + * (journal dir + setting state), and say. Structural cast at the boundary, + * matching the documented test-double style of the tool specs. + */ +function makeTask(options: { saveCheckpoint?: unknown; state?: Record; enableCheckpoints?: boolean }) { + const say = vi.fn().mockResolvedValue(undefined) + // An explicit `saveCheckpoint: undefined` (a checkpoint that produced no + // commit) must be preserved as-is; only an omitted option falls back to + // the default commit result. + const saveCheckpoint = vi + .fn() + .mockResolvedValue("saveCheckpoint" in options ? options.saveCheckpoint : { commit: "sha-card-1" }) + const providerDeref = { + context: { globalStorageUri: { fsPath: globalStorageDir } }, + getState: vi.fn().mockResolvedValue(options.state ?? {}), + } + + const task = { + taskId: "task-card", + cwd: "/workspace", + enableCheckpoints: options.enableCheckpoints ?? true, + checkpointService: { + saveCheckpoint, + isInitialized: true, + }, + providerRef: { deref: vi.fn().mockReturnValue(providerDeref) }, + say, + } as unknown as Task + + return { task, say, saveCheckpoint } +} + +let globalStorageDir: string + +beforeEach(async () => { + globalStorageDir = await fs.mkdtemp(path.join(os.tmpdir(), "b3a-save-")) +}) + +afterEach(async () => { + await fs.rm(globalStorageDir, { recursive: true, force: true }) +}) + +describe("checkpointSave change-card emission (B3a)", () => { + it("emits a full-detail change card after a successful per-write checkpoint and still appends the journal", async () => { + const { task, say, saveCheckpoint } = makeTask({ state: { changeCardDetail: "full" } }) + + await checkpointSave(task, false, true, { + path: "src/a.ts", + operation: "create", + diffStats: { additions: 2, deletions: 1 }, + diff: "+line-a\n+line-b\n-old", + }) + + expect(saveCheckpoint).toHaveBeenCalledWith(expect.stringContaining("task-card"), expect.any(Object)) + + const cardCalls = say.mock.calls.filter(([type]) => type === "change_card") + expect(cardCalls).toHaveLength(1) + const [type, text, images, _partial, _sayOptions, _progress, options] = cardCalls[0] as unknown as [ + string, + string, + undefined, + undefined, + undefined, + undefined, + { isNonInteractive?: boolean }, + ] + expect(type).toBe("change_card") + expect(images).toBeUndefined() + expect(options).toEqual({ isNonInteractive: true }) + const card = JSON.parse(text as string) as { + checkpointIds: string[] + files: Array<{ path: string; additions: number; deletions: number; diff?: string }> + totalFiles: number + detail: string + } + expect(card.checkpointIds).toEqual(["sha-card-1"]) + expect(card.totalFiles).toBe(1) + expect(card.detail).toBe("full") + expect(card.files).toEqual([{ path: "src/a.ts", additions: 2, deletions: 1, diff: "+line-a\n+line-b\n-old" }]) + + // B2 regression: the journal entry is still appended with the commit id. + const journalRaw = await fs.readFile(journalPath(globalStorageDir, "task-card"), "utf8") + const entries = journalRaw + .split("\n") + .filter((line) => line !== "") + .map((line) => JSON.parse(line)) + expect(entries).toHaveLength(1) + expect(entries[0]).toMatchObject({ path: "src/a.ts", operation: "create", checkpointId: "sha-card-1" }) + }) + + it("emits a summary card without diffs for the default (summary) setting", async () => { + const { task, say } = makeTask({}) + + await checkpointSave(task, false, true, { + path: "src/a.ts", + operation: "update", + diffStats: { additions: 1, deletions: 0 }, + diff: "+x", + }) + + const cardCall = say.mock.calls.find(([type]) => type === "change_card") + expect(cardCall).toBeDefined() + const card = JSON.parse((cardCall as unknown as [string, string])[1]) as { + files: Array> + detail: string + } + expect(card.detail).toBe("summary") + expect(card.files[0]).not.toHaveProperty("diff") + }) + + it("emits a compact card for auto-approved steps even when the setting is full", async () => { + const { task, say } = makeTask({ state: { changeCardDetail: "full" } }) + + await checkpointSave(task, false, true, { + path: "src/a.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: "+x", + autoApproved: true, + }) + + const cardCall = say.mock.calls.find(([type]) => type === "change_card") + expect(cardCall).toBeDefined() + const card = JSON.parse((cardCall as unknown as [string, string])[1]) as { + files: Array> + detail: string + } + expect(card.detail).toBe("summary") + expect(card.files[0]).not.toHaveProperty("diff") + }) + + it("emits one card with all writes for a multi-file step", async () => { + const { task, say } = makeTask({ state: { changeCardDetail: "full" } }) + + await checkpointSave(task, false, true, [ + { path: "src/a.ts", operation: "create", diffStats: { additions: 2, deletions: 0 }, diff: "+a1\n+a2" }, + { path: "src/b.ts", operation: "delete", diffStats: { additions: 0, deletions: 3 } }, + ]) + + const cardCalls = say.mock.calls.filter(([type]) => type === "change_card") + expect(cardCalls).toHaveLength(1) + const card = JSON.parse((cardCalls[0] as unknown as [string, string])[1]) as { + files: Array<{ path: string; diff?: string }> + totalFiles: number + } + expect(card.totalFiles).toBe(2) + expect(card.files.map((file) => file.path)).toEqual(["src/a.ts", "src/b.ts"]) + expect(card.files[0].diff).toBe("+a1\n+a2") + expect(card.files[1]).not.toHaveProperty("diff") + }) + + it("emits no change card for baseline checkpoints without write info", async () => { + const { task, say } = makeTask({}) + + await checkpointSave(task) + + // `say` is invoked with seven arguments, so a three-argument + // `toHaveBeenCalledWith` negative assertion can never fail; filter the + // recorded calls by type instead. + const cardCalls = say.mock.calls.filter(([type]) => type === "change_card") + expect(cardCalls).toHaveLength(0) + }) + + it("emits no change card when the checkpoint produced no commit", async () => { + const { task, say } = makeTask({ saveCheckpoint: undefined }) + + await checkpointSave(task, false, true, { path: "src/a.ts", operation: "create" }) + + const cardCalls = say.mock.calls.filter(([type]) => type === "change_card") + expect(cardCalls).toHaveLength(0) + }) + + it("emits no change card for a no-op checkpoint with an empty writes array but still saves", async () => { + const { task, say, saveCheckpoint } = makeTask({}) + + // A no-op patch still reaches checkpointSave with [] write metadata; the + // commit may cover unrelated workspace changes, but a zero-file card must + // not be emitted. + await checkpointSave(task, false, true, []) + + expect(saveCheckpoint).toHaveBeenCalled() + const cardCalls = say.mock.calls.filter(([type]) => type === "change_card") + expect(cardCalls).toHaveLength(0) + }) + + it("emits no change card when checkpoints are disabled for the task", async () => { + const { task, say } = makeTask({ enableCheckpoints: false }) + + await checkpointSave(task, false, true, { path: "src/a.ts", operation: "create", diff: "+x" }) + + expect(say).not.toHaveBeenCalled() + }) + + it("keeps the journal append when a card emission failure occurs", async () => { + const { task, say } = makeTask({}) + say.mockImplementation(async (type: string) => { + if (type === "change_card") { + throw new Error("task aborted") + } + }) + + await checkpointSave(task, false, true, { + path: "src/a.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: "+x", + }) + + // The say failure is contained: the journal is still written. + const journalRaw = await fs.readFile(journalPath(globalStorageDir, "task-card"), "utf8") + expect(journalRaw).toContain("src/a.ts") + }) +}) diff --git a/src/core/checkpoints/changeCard.ts b/src/core/checkpoints/changeCard.ts new file mode 100644 index 0000000000..b13f03320d --- /dev/null +++ b/src/core/checkpoints/changeCard.ts @@ -0,0 +1,91 @@ +/** + * Per-step change card builder (B3a). + * + * The card is emitted from `checkpointSave` (index.ts) once the per-write + * checkpoint commit exists, so the payload can key the card by the real + * checkpoint ID and reuse the approval diff + stats the tool already + * computed (threaded through {@link CheckpointWriteInfo}). The card is + * informational and always emitted for write steps, including auto-approved + * ones — which always get the compact ("summary") form regardless of the + * `changeCardDetail` setting. + */ +import { DEFAULT_CHANGE_CARD_DETAIL, type ChangeCardData, type ChangeCardDetail } from "@roo-code/types" + +/** + * The write data a change card is built from. Structurally compatible with + * `CheckpointWriteInfo` (src/core/checkpoints/index.ts), minus the + * `operation` field the card does not need. + */ +export interface ChangeCardWrite { + /** The file path as the tool knows it (relative to the task cwd). */ + path: string + /** { additions, deletions } from the approval diff, when computable. */ + diffStats?: { additions: number; deletions: number } + /** The unified approval diff for this file (reused, not recomputed). */ + diff?: string + /** Whether the tool step was auto-approved (no human interaction). */ + autoApproved?: boolean +} + +/** + * Whether every write of the step was auto-approved. Empty steps are not + * auto-approved (there is nothing for the user to have skipped). + */ +export function isAutoApprovedStep(writes: readonly ChangeCardWrite[]): boolean { + return writes.length > 0 && writes.every((write) => write.autoApproved === true) +} + +/** + * Resolve the card detail level for a step: + * - auto-approved steps always get the compact "summary" card, regardless of + * the user setting (cards for steps the user never saw approving are + * informational only); + * - otherwise the `changeCardDetail` setting applies, defaulting to + * "summary" when unset. + */ +export function resolveChangeCardDetail( + writes: readonly ChangeCardWrite[], + setting: ChangeCardDetail | undefined, +): ChangeCardDetail { + if (isAutoApprovedStep(writes)) { + return "summary" + } + return setting ?? DEFAULT_CHANGE_CARD_DETAIL +} + +/** + * Build the typed change-card payload for one step (one per-write checkpoint). + * + * With `detail: "full"` each file carries its unified diff inline; with + * `detail: "summary"` the diff is omitted and the UI fetches it lazily + * (B3b). + */ +export function buildChangeCard( + checkpointId: string, + writes: readonly ChangeCardWrite[], + detail: ChangeCardDetail, +): ChangeCardData { + return { + checkpointIds: [checkpointId], + files: writes.map((write) => ({ + path: write.path, + additions: write.diffStats?.additions ?? 0, + deletions: write.diffStats?.deletions ?? 0, + ...(detail === "full" && write.diff ? { diff: write.diff } : {}), + })), + totalFiles: writes.length, + detail, + } +} + +/** + * Convenience wrapper: resolve the detail level from the step + setting, then + * build the payload. This is what `checkpointSave` calls. + */ +export function buildChangeCardPayload( + checkpointId: string, + writes: readonly ChangeCardWrite[], + setting: ChangeCardDetail | undefined, +): ChangeCardData { + return buildChangeCard(checkpointId, writes, resolveChangeCardDetail(writes, setting)) +} diff --git a/src/core/checkpoints/changeJournal.ts b/src/core/checkpoints/changeJournal.ts new file mode 100644 index 0000000000..d1220e4717 --- /dev/null +++ b/src/core/checkpoints/changeJournal.ts @@ -0,0 +1,278 @@ +import fs from "fs/promises" +import * as path from "path" + +/** + * A single entry in the per-task change journal (changes.jsonl). + * + * One line is appended for every successful file write that goes through a + * B1 per-write checkpoint hook. WriteToFileTool and EditFileTool emit one + * entry per write; ApplyPatchTool emits one entry per file change of a fully + * successful patch — those entries all reference the single B1 checkpoint + * that the patch's post-loop hook saves for the whole patch. The task-start + * baseline never produces an entry (it is not a file write). + */ +export interface ChangeJournalEntry { + /** The file path as the tool knows it (relative to task cwd). */ + path: string + /** "create" | "update" | "delete" — derived from what the tool did. */ + operation: "create" | "update" | "delete" + /** The B1 checkpoint commit SHA for this write (from checkpointSave result). */ + checkpointId: string + /** { additions, deletions } from the approval diff; null/omit when not computable. */ + diffStats?: { additions: number; deletions: number } +} + +const JOURNAL_FILENAME = "changes.jsonl" + +/** + * Task IDs arrive from task creation and are used as a single path segment + * under /tasks. Reject anything that could escape that + * directory — separators, traversal, absolute paths, empty or dot-only IDs — + * before any filesystem operation touches the journal path. + */ +function assertSafeTaskId(taskId: string): void { + if (!/^[A-Za-z0-9._-]+$/.test(taskId) || taskId === "." || taskId === "..") { + throw new Error(`Unsafe task ID for checkpoint journal path: ${JSON.stringify(taskId)}`) + } +} + +/** Derive the per-task checkpoint directory from globalStorageDir and taskId. */ +function taskCheckpointDir(globalStorageDir: string, taskId: string): string { + return path.join(globalStorageDir, "tasks", taskId, "checkpoints") +} + +/** Journal file path for a given task. */ +export function journalPath(globalStorageDir: string, taskId: string): string { + assertSafeTaskId(taskId) + return path.join(taskCheckpointDir(globalStorageDir, taskId), JOURNAL_FILENAME) +} + +/** + * Per-task update queue: journal replacements for one task never overlap, + * so concurrent checkpoint saves cannot lose each other's entries through + * the read-modify-write cycle below. + */ +const journalQueues = new Map>() + +function withTaskLock(key: string, run: () => Promise): Promise { + const prev = journalQueues.get(key) ?? Promise.resolve() + // The next update runs after the previous one settles, whatever it did. + const next = prev.then(run, run) + // The queued promise swallows its outcome so a failed update neither + // blocks later updates nor surfaces as an unhandled rejection; callers + // observe the failure through `next`. + journalQueues.set( + key, + next.then( + () => undefined, + () => undefined, + ), + ) + return next +} + +/** + * Append change-journal entries to the per-task changes.jsonl file as one + * serialized, atomic update. + * + * The whole multi-entry update is a crash-safe replacement: the current + * journal is read, a torn tail is terminated, the complete next content is + * written to a temporary file in the same directory and fsynced, then + * atomically renamed onto the journal, and the parent directory is fsynced + * (POSIX; Windows cannot open directories). A crash at any point leaves + * either the old complete journal or the new complete journal - never a + * half-applied multi-file update - and a durable checkpoint implies a + * durable journal entry. Creates parent directories if they don't exist + * yet (e.g. first checkpoint). + * + * Failures propagate: callers must not report a fully durable checkpoint + * when the journal update did not complete. + */ +/** True when a parsed JSON value is a complete journal record. */ +function isEntryShaped(value: unknown): value is ChangeJournalEntry { + return ( + !!value && + typeof value === "object" && + typeof (value as ChangeJournalEntry).path === "string" && + typeof (value as ChangeJournalEntry).checkpointId === "string" && + ["create", "update", "delete"].includes((value as ChangeJournalEntry).operation) + ) +} + +export async function appendChanges( + globalStorageDir: string, + taskId: string, + entries: ChangeJournalEntry[], +): Promise { + if (entries.length === 0) { + return + } + const filePath = journalPath(globalStorageDir, taskId) + return withTaskLock(filePath, async () => { + const dir = path.dirname(filePath) + await fs.mkdir(dir, { recursive: true }) + let content = "" + try { + content = await fs.readFile(filePath, "utf8") + } catch (error) { + // A missing journal is a fresh history. Any other read failure + // means the complete next content cannot be built, so propagate + // instead of replacing the journal with a partial view of it. + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error + } + } + // Torn-tail repair: a crash can leave the final line unterminated. If + // that final line is a complete, entry-shaped JSON record it was fully + // written and only its newline was lost - keep it and re-terminate. + // Otherwise it is a fragment of a lost write and the replacement + // truncates it, so the journal only ever holds complete records and a + // later entry can never mask the lost one. + if (content && !content.endsWith("\n")) { + const cut = content.lastIndexOf("\n") + 1 + let tail: unknown + try { + tail = JSON.parse(content.slice(cut)) + } catch { + tail = undefined + } + content = isEntryShaped(tail) ? content + "\n" : content.slice(0, cut) + } + const payload = entries.map((entry) => JSON.stringify(entry) + "\n").join("") + const tmpPath = path.join(dir, `${JOURNAL_FILENAME}.${process.pid}.${Math.random().toString(36).slice(2)}.tmp`) + const data = Buffer.from(content + payload) + try { + const handle = await fs.open(tmpPath, "w") + try { + // A single write can complete partially; sync only after the + // whole content is on the handle, or a short write would + // rename a torn journal into place. + let written = 0 + while (written < data.length) { + const res = await handle.write(data, written, data.length - written) + written += res.bytesWritten + } + await handle.sync() + } finally { + await handle.close() + } + // Windows can transiently reject a rename onto the existing journal + // (an antivirus or indexer holding the destination): retry with a + // short backoff before failing. The destination is never unlinked + // to make room, so atomicity is preserved on every failure. + const renameAttempts = process.platform === "win32" ? 5 : 1 + for (let attempt = 1; ; attempt++) { + try { + await fs.rename(tmpPath, filePath) + break + } catch (error) { + const renameCode = (error as NodeJS.ErrnoException).code + if ( + process.platform !== "win32" || + attempt >= renameAttempts || + !["EPERM", "EBUSY", "EACCES"].includes(renameCode ?? "") + ) { + throw error + } + await new Promise((resolve) => setTimeout(resolve, 20 * attempt)) + } + } + if (process.platform !== "win32") { + // The rename replaced the directory entry: on POSIX the + // directory itself must be synced for the new entry to + // survive a crash (Windows cannot open directories). The + // journal content is already in place at this point, so a + // sync failure means reduced crash durability, not a lost + // update: errnos that mean "fsync is unsupported on this + // mount" are tolerated and logged; any other failure still + // propagates. + try { + const dirHandle = await fs.open(dir, "r") + try { + await dirHandle.sync() + } finally { + await dirHandle.close() + } + } catch (error) { + const syncCode = (error as NodeJS.ErrnoException).code + if (!["EINVAL", "EISDIR", "EBADF", "ENOTSUP", "EOPNOTSUPP", "EPERM"].includes(syncCode ?? "")) { + throw error + } + console.warn( + `[changeJournal] directory sync unsupported after journal rename (${syncCode}); crash durability reduced`, + ) + } + } + } catch (error) { + // Best-effort cleanup: the rename never completed, so the journal + // still holds the previous complete content; drop the temp file. + await fs.rm(tmpPath, { force: true }).catch(() => {}) + throw error + } + }) +} + +/** Append a single entry; see appendChanges for the durability contract. */ +export async function appendChange(globalStorageDir: string, taskId: string, entry: ChangeJournalEntry): Promise { + await appendChanges(globalStorageDir, taskId, [entry]) +} + +/** + * Load all change-journal entries for a task. + * + * Torn-tail repair: if the final line is truncated (JSON.parse fails), it is + * silently discarded. Lines that parse to valid JSON but are not + * entry-shaped (non-object, missing string path/checkpointId, or an + * unknown operation) are skipped for the same reason. The rest of the file + * is returned in order. An absent + * or empty journal returns []. + */ +export async function loadChanges(globalStorageDir: string, taskId: string): Promise { + const filePath = journalPath(globalStorageDir, taskId) + + let content: string + try { + content = await fs.readFile(filePath, "utf8") + } catch { + // File absent or unreadable → empty journal. + return [] + } + + if (!content.trim()) { + return [] + } + + const lines = content.split("\n") + // Remove trailing empty line from a file that ends with \n. + if (lines[lines.length - 1] === "") { + lines.pop() + } + + const entries: ChangeJournalEntry[] = [] + for (let i = 0; i < lines.length; i++) { + try { + const parsed: unknown = JSON.parse(lines[i]) + // A torn append or manual edit can leave valid JSON that is not an + // entry (a bare number, null, or a differently-shaped object). + // Consumers read path/checkpointId unguarded, so skip any line that + // is not entry-shaped instead of returning it as an entry. + if (isEntryShaped(parsed)) { + entries.push(parsed) + } + } catch { + // A corrupt line before the final line (e.g. a partially flushed + // append) must not hide the valid entries after it. The final + // line is different: dropping an unverified record there can + // report an incomplete rollback history as valid (restore-latest + // would no-op over the lost write), and torn-tail repair is + // verified truncation owned by the append path, so a read + // rejects instead. + if (i === lines.length - 1) { + throw new Error(`Corrupt change journal entry at line ${i + 1} of ${filePath}`) + } + continue + } + } + + return entries +} diff --git a/src/core/checkpoints/index.ts b/src/core/checkpoints/index.ts index de424b03f7..8bd83af672 100644 --- a/src/core/checkpoints/index.ts +++ b/src/core/checkpoints/index.ts @@ -16,6 +16,9 @@ import { DIFF_VIEW_URI_SCHEME } from "../../integrations/editor/DiffViewProvider import { CheckpointServiceOptions, RepoPerTaskCheckpointService } from "../../services/checkpoints" +import { appendChanges } from "./changeJournal" +import { buildChangeCardPayload } from "./changeCard" + const WARNING_THRESHOLD_MS = 5000 function sendCheckpointInitWarn(task: Task, type?: "WAIT_TIMEOUT" | "INIT_TIMEOUT", timeout?: number) { @@ -209,22 +212,174 @@ async function checkGitInstallation( } } -export async function checkpointSave(task: Task, force = false, suppressMessage = false) { +/** + * Write metadata for the per-task change journal (B2). + * + * `path` is the file path as the tool knows it (relative to the task cwd), + * consistent with what the B1 per-write checkpoint hooks see. `diffStats` is + * the { additions, deletions } pair from the approval diff when it was + * computable; omitted otherwise. ApplyPatchTool passes one entry per file + * change of a fully successful patch (an array), all sharing the single + * checkpoint the patch's post-loop hook saves. + */ +export type CheckpointWriteInfo = { + path: string + operation: "create" | "update" | "delete" + diffStats?: { additions: number; deletions: number } + /** + * The unified approval diff for this write, reused verbatim by the B3a + * change card (never recomputed). + */ + diff?: string + /** + * Whether the tool step was auto-approved (no human interaction). Auto- + * approved steps always get the compact ("summary") change card. + */ + autoApproved?: boolean +} +/** + * Thrown when a checkpoint commit succeeded but its journal/card persistence + * did not. Carries the committed checkpoint id and the complete affected write + * set so tool callers can report an explicit partial-persistence state + * instead of a silent full success. + */ +export class CheckpointPartialPersistenceError extends Error { + constructor( + message: string, + readonly checkpointId: string, + readonly writes: CheckpointWriteInfo[], + ) { + super(message) + this.name = "CheckpointPartialPersistenceError" + } +} + +export async function checkpointSave( + task: Task, + force = false, + suppressMessage = false, + write?: CheckpointWriteInfo | CheckpointWriteInfo[], +) { const service = await getCheckpointService(task) if (!service) { return } + // A save that carries write metadata needs a stable journal location for + // the whole commit + journal transaction. Capture it before the commit: + // the weak provider reference can disappear while the Git save is + // awaited, and a commit whose journal update was then silently skipped + // leaves rollback metadata incomplete with no error surfaced. Without a + // location at all, fail before committing so the tool reports an + // explicit failure instead of a checkpoint nobody can roll back. + const journalWrites = write === undefined ? undefined : Array.isArray(write) ? write : [write] + const hasWriteMetadata = !!journalWrites && journalWrites.length > 0 + const journalDir = task.providerRef.deref()?.context.globalStorageUri.fsPath + if (hasWriteMetadata && !journalDir) { + throw new Error( + "Checkpoint save with write metadata requires a stable journal location, but the provider reference or global storage path is unavailable.", + ) + } + TelemetryService.instance.captureCheckpointCreated(task.taskId) - // Start the checkpoint process in the background. - return service - .saveCheckpoint(`Task: ${task.taskId}, Time: ${Date.now()}`, { allowEmpty: force, suppressMessage }) - .catch((err) => { + // Save failures keep the historical behavior for non-write calls: + // logged, checkpoints disabled, and the save resolves undefined. A + // write-metadata save must not swallow a failed commit: the file write + // already happened, so the caller must see an explicit failure instead + // of a success whose rollback metadata does not exist. Journal/card + // work below runs only for a real commit, and its failures propagate to + // the caller instead of flowing through this handler. + const savePromise = service.saveCheckpoint(`Task: ${task.taskId}, Time: ${Date.now()}`, { + allowEmpty: force, + suppressMessage, + }) + // `void` keeps the historical union member of the awaited legacy + // catch path (and of this function's return type) unchanged. + let result: Awaited | void + if (hasWriteMetadata) { + try { + result = await savePromise + } catch (err) { + console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err) + task.enableCheckpoints = false + throw new Error( + `Checkpoint save failed after a file write; the write has no rollback metadata: ${err instanceof Error ? err.message : String(err)}`, + { cause: err }, + ) + } + } else { + result = await savePromise.catch((err) => { console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err) task.enableCheckpoints = false }) + } + // B2: record successful file writes in the per-task change journal. + // Only a real commit produces an entry (an empty or failed save + // resolves to undefined / rejects), and non-write checkpoint calls + // (e.g. the task-start baseline) pass no `write` value at all. + // A no-op checkpoint carries an empty writes array: unrelated workspace + // changes can still produce a commit, and emitting a card for it would + // show a change card with zero files. The checkpoint itself still runs. + if (result?.commit && hasWriteMetadata) { + const writes = journalWrites ?? [] + if (!journalDir) { + // Defensive: the pre-commit guard above normally makes this + // unreachable. If the location is nonetheless gone while the commit + // succeeded, surface the partial state instead of skipping the + // journal silently. + throw new CheckpointPartialPersistenceError( + `Checkpoint ${result.commit} was committed but no stable journal location was available (files: ${writes + .map((w) => w.path) + .join(", ")}); rollback metadata is incomplete.`, + result.commit, + writes, + ) + } + // Persist the whole step's journal update as one serialized, + // atomic replacement (temp file + fsync + rename + directory + // sync), preserving write order. A journal failure is logged and + // propagated: the checkpoint must not be reported fully durable + // while its journal entries were lost. + try { + await appendChanges( + journalDir, + task.taskId, + writes.map((w) => ({ + path: w.path, + operation: w.operation, + checkpointId: result.commit, + ...(w.diffStats ? { diffStats: w.diffStats } : {}), + })), + ) + } catch (err) { + console.error("[Task#checkpointSave] failed to persist change journal entries", err) + throw new CheckpointPartialPersistenceError( + `Checkpoint ${result.commit} was committed but its change journal could not be persisted (files: ${writes + .map((w) => w.path) + .join(", ")}); rollback metadata is incomplete.`, + result.commit, + writes, + ) + } + + // B3a: emit the per-step change card now that the checkpoint commit + // exists and the journal update is durable (a journal failure above + // propagated and skips the card). The card reuses the approval diff/stats the tool already + // computed and is always emitted (auto-approved steps included); + // a card failure is logged and never disables checkpoints. + try { + const state = await task.providerRef.deref()?.getState() + const card = buildChangeCardPayload(result.commit, writes, state?.changeCardDetail) + await task.say("change_card", JSON.stringify(card), undefined, undefined, undefined, undefined, { + isNonInteractive: true, + }) + } catch (err) { + console.error("[Task#checkpointSave] failed to emit change card", err) + } + } + return result } export type CheckpointRestoreOptions = { diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index df38fb4309..161f41904b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -454,6 +454,9 @@ export class Task extends EventEmitter implements TaskLike { public lastMessageTs?: number private autoApprovalTimeoutRef?: NodeJS.Timeout + // B1: task-start baseline, recorded at most once (initiateTaskLoop also runs on resume). + private taskStartBaselineDone = false + // Tool Use consecutiveMistakeCount: number = 0 consecutiveMistakeLimit: number @@ -1681,7 +1684,12 @@ export class Task extends EventEmitter implements TaskLike { this.emit(RooCodeEventName.Message, { action: "created", message }) await this.saveClineMessages() - const shouldCaptureMessage = message.partial !== true && CloudService.isEnabled() + // Change cards embed raw unified diffs of approved writes (full + // detail mode): keep them local to the webview so task sync never + // uploads the content of secret-bearing files (.env, PII) to the + // cloud endpoint. + const shouldCaptureMessage = + message.partial !== true && CloudService.isEnabled() && message.say !== "change_card" if (shouldCaptureMessage) { CloudService.instance.captureEvent({ @@ -1731,8 +1739,12 @@ export class Task extends EventEmitter implements TaskLike { await provider?.postMessageToWebview({ type: "messageUpdated", clineMessage: message }) this.emit(RooCodeEventName.Message, { action: "updated", message }) - // Check if we should sync to cloud and haven't already synced this message - const shouldCaptureMessage = message.partial !== true && CloudService.isEnabled() + // Check if we should sync to cloud and haven't already synced this + // message. Change cards stay out of cloud capture (full diffs embed + // file content; see addToClineMessages) - the guard is repeated here + // because a card message is never tracked as synced. + const shouldCaptureMessage = + message.partial !== true && CloudService.isEnabled() && message.say !== "change_card" const hasNotBeenSynced = !this.cloudSyncedMessageTimestamps.has(message.ts) if (shouldCaptureMessage && hasNotBeenSynced) { @@ -3745,6 +3757,24 @@ export class Task extends EventEmitter implements TaskLike { // arm needed. void getCheckpointService(this) + // B1 task-start baseline: a suppressed pre-task root commit (default-on). + // History resumes skip it: the baseline belongs to the original task start + // (exactly once per task), and the resumed task's shadow repo already carries + // a baseline for the first post-resume write checkpoint to diff against. + // An already-aborted task skips it too: the loop will never run, so the + // save would only add latency to a task that is being disposed. + if (!this.taskStartBaselineDone && !this._isHistoryTask && !this.abort) { + this.taskStartBaselineDone = true + const baselineEnabled = (await this.providerRef.deref()?.getState())?.perWriteCheckpoints + // Cancellation can arrive while the state lookup is pending; recheck + // before the save so a canceled task performs no checkpoint work. + if (baselineEnabled !== false && !this.abort) { + // allowEmpty=true so a clean workspace still produces the baseline + // commit; awaited so the first per-write checkpoint cannot interleave. + await this.checkpointSave(true, true) + } + } + let nextUserContent = userContent let includeFileDetails = true @@ -4178,7 +4208,6 @@ export class Task extends EventEmitter implements TaskLike { // under the canonical name slip past a name comparison. The first call wins. const idAlreadyUsed = this.assistantMessageContent.some( (entry) => - // Stryker disable next-line ConditionalExpression: only tool_use and mcp_tool_use entries ever carry an id — the text block pushed in the text case has no id field and the start events reaching this guard always carry the string id locked in by processRawChunk — so the type check cannot change the outcome of entry.id === event.id for any entry this pipeline produces; the false replacement is additionally killed by the rejection tests. (entry.type === "tool_use" || entry.type === "mcp_tool_use") && (entry as { id?: string }).id === event.id, ) @@ -4231,7 +4260,6 @@ export class Task extends EventEmitter implements TaskLike { // keep the deprecated single-entry-per-ID lookup. let streamingKey: string | undefined - // Stryker disable next-line ConditionalExpression: event.name is always a string on this path — the only delta events reaching this handler are emitted by processRawChunk with the tracked start name (never undefined), so the legacy nameless branch below is unreachable defensive code and the conditional always takes the named side. if (event.name !== undefined) { streamingKey = NativeToolCallParser.makeStreamingKey(event.id, event.name) } else { @@ -4248,7 +4276,6 @@ export class Task extends EventEmitter implements TaskLike { } const partialToolUse = - // Stryker disable next-line ConditionalExpression: streamingKey can never be undefined here — makeStreamingKey always returns a string and the legacy lookup branch above is unreachable (see the directive on the event.name check) — so the null side is unreachable defensive code. streamingKey === undefined ? null : NativeToolCallParser.processStreamingChunk( @@ -4257,7 +4284,6 @@ export class Task extends EventEmitter implements TaskLike { nativeToolCallParserScope, ) - // Stryker disable next-line ConditionalExpression, EqualityOperator: streamingKey is always a defined string on this path (makeStreamingKey never returns undefined; the legacy branch is unreachable per the directives above), and processStreamingChunk returns a partial ToolUse for a known key, so both defensive conditions reduce to the partialToolUse check and cannot change the observed behavior. if (partialToolUse && streamingKey !== undefined) { // Reuse the compound key built above: it is exactly the key the start event // registered under, so the delta lookup needs no second encoding. @@ -4648,7 +4674,6 @@ export class Task extends EventEmitter implements TaskLike { if (event.type === "tool_call_end") { // Create compound key for deduplication (same pattern as streaming handler). // End events carry the tool name; fall back to the id for events that don't. - // Stryker disable next-line LogicalOperator: event.name is always a string on this path — finalizeRawChunks emits end events with the tracked start name (a string, set before the tracker can start), so the ?? event.id fallback guards a nameless end event this pipeline cannot produce. let eventName = event.name ?? event.id let dedupKey = NativeToolCallParser.makeStreamingKey(event.id, eventName) @@ -4665,13 +4690,11 @@ export class Task extends EventEmitter implements TaskLike { // name (provider quirk), the compound-key lookup above misses and both the // parser entry and the dedup tracking would stay stale. Resolve the tracked // entry by id and target the real compound key for finalization and cleanup. - // Stryker disable next-line ConditionalExpression, LogicalOperator, EqualityOperator: the both-miss case is unreachable via the raw-chunk pipeline — end events carry the locked start name so the compound key resolves the registered entry, and a same-ID rejected-start end is always processed after the accepted start's end has already removed that entry; this defensive path protects against a cross-name end event the pipeline cannot produce. if (finalToolUse === null && toolUseIndex === undefined) { const resolved = NativeToolCallParser.getStreamingToolCallById( event.id, nativeToolCallParserScope, ) - // Stryker disable next-line ConditionalExpression: unreachable defensive region — getStreamingToolCallById can only resolve when the both-miss case above is reachable, which the raw-chunk pipeline cannot produce (see the directive there). if (resolved) { eventName = resolved.name dedupKey = NativeToolCallParser.makeStreamingKey(resolved.id, resolved.name) @@ -6336,6 +6359,14 @@ export class Task extends EventEmitter implements TaskLike { * @param context - Context string for logging (e.g., the calling tool name) */ public processQueuedMessages(): void { + // An aborted task must not dequeue queued messages: submitUserMessage + // rejects once abort is set, so consuming the queue here would drop the + // message instead of leaving it for the resume/abort flow. Tools await + // checkpoint saves before handing off, so a cancellation can land between + // the write and this call. + if (this.abort) { + return + } try { if (!this.messageQueueService.isEmpty()) { const queued = this.messageQueueService.dequeueMessage() diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 85325b39ff..b5eb133a43 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -6504,6 +6504,175 @@ describe("Cline", () => { }) }) + describe("task-start baseline (B1 perWriteCheckpoints)", () => { + it("records one suppressed baseline checkpoint per Task instance at loop start", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + // End the loop after the first request; abort stays false so the + // baseline block (not the loop) is what the save spy observes. + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).toHaveBeenCalledOnce() + expect(saveSpy).toHaveBeenCalledWith(true, true) + }) + + it("does not record a baseline when the task resumes from history", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + historyItem: { + id: "resumed-baseline-task", + number: 1, + ts: Date.now(), + task: "resumed baseline task", + tokensIn: 10, + tokensOut: 5, + totalCost: 0.001, + }, + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + // A history resume must not re-record the baseline: it belongs to the + // original task start (exactly once per task). + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("records the baseline checkpoint when the setting is unset (default-on)", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline unset task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + // Unset: the property is absent from the state, so default-on applies. + const unsetState = { ...state } + Reflect.deleteProperty(unsetState, "perWriteCheckpoints") + vi.spyOn(mockProvider, "getState").mockResolvedValue(unsetState as typeof state) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).toHaveBeenCalledOnce() + expect(saveSpy).toHaveBeenCalledWith(true, true) + }) + + it("does not record a baseline checkpoint when perWriteCheckpoints is disabled", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline disabled task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...state, perWriteCheckpoints: false }) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("does not record a baseline when the task is already aborted", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "aborted baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + task.abort = true + + await taskAccess.initiateTaskLoop([]) + + // A task cancelled before the loop starts must not pay for (or await) + // a baseline checkpoint it will never use. + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("does not record a baseline when cancellation arrives during the state lookup", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "racy baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + type StateResult = Awaited> + let resolveState: (value: StateResult | PromiseLike) => void = () => {} + const statePromise = new Promise((resolve) => (resolveState = resolve)) + vi.spyOn(mockProvider, "getState").mockReturnValue(statePromise) + + const loopPromise = taskAccess.initiateTaskLoop([]) + // Cancellation lands while the state lookup is still pending. + task.abort = true + resolveState({ ...state, perWriteCheckpoints: true }) + await loopPromise + + // The post-lookup recheck must skip the save for the canceled task. + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("awaits the baseline checkpoint before entering the request loop", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline await task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveSpy = vi + .spyOn(task, "checkpointSave") + .mockImplementation(() => new Promise((resolve) => (resolveSave = resolve))) + const requestSpy = vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...(await mockProvider.getState()) }) + const loopPromise = taskAccess.initiateTaskLoop([]) + + // The loop must not enter while the baseline checkpoint is still in flight. + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(saveSpy).toHaveBeenCalledOnce() + expect(requestSpy).not.toHaveBeenCalled() + + resolveSave() + await loopPromise + expect(requestSpy).toHaveBeenCalled() + }) + }) + describe("start()", () => { it("should be a no-op if the task was already started in the constructor", () => { const task = new Task({ @@ -7345,6 +7514,90 @@ describe("Cline", () => { ) }) }) + + // Change cards embed raw unified diffs of approved writes (full detail + // mode): task sync must not upload their file content to the cloud. + describe("cloud capture of change cards", () => { + const flushMicrotasks = () => new Promise((resolve) => setImmediate(resolve)) + + it("keeps change_card messages out of cloud task-message capture", async () => { + const { CloudService } = await import("@roo-code/cloud") + const enabledSpy = vi.spyOn(CloudService, "isEnabled").mockReturnValue(true) + const captureSpy = vi.fn() + // Minimal double for the singleton getter: the capture path only + // calls captureEvent on the instance, and the real singleton is + // not initialized in this harness (the getter would throw). + const cloudDouble = { captureEvent: captureSpy } as unknown as import("@roo-code/cloud").CloudService + const instanceSpy = vi.spyOn(CloudService, "instance", "get").mockReturnValue(cloudDouble) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + await flushMicrotasks() + captureSpy.mockClear() + + const cardPayload = JSON.stringify({ + checkpointId: "abc123", + files: [{ path: ".env", additions: 1, deletions: 0, diff: "+SECRET_KEY=hunter2" }], + }) + await task.say("change_card", cardPayload, undefined, undefined, undefined, undefined, { + isNonInteractive: true, + }) + await flushMicrotasks() + + const cardCaptures = captureSpy.mock.calls.filter( + (call) => + (call[0] as { properties?: { message?: { say?: string } } }).properties?.message?.say === + "change_card", + ) + expect(cardCaptures).toHaveLength(0) + + // Control: an ordinary non-partial say still syncs while task + // capture is enabled, so the exclusion is change_card-specific. + await task.say("text", "ordinary message") + await flushMicrotasks() + const textCaptures = captureSpy.mock.calls.filter( + (call) => + (call[0] as { properties?: { message?: { say?: string } } }).properties?.message?.say === "text", + ) + expect(textCaptures.length).toBeGreaterThan(0) + + enabledSpy.mockRestore() + instanceSpy.mockRestore() + }) + + it("keeps change_card messages out of the updateClineMessage capture path", async () => { + const { CloudService } = await import("@roo-code/cloud") + const enabledSpy = vi.spyOn(CloudService, "isEnabled").mockReturnValue(true) + const captureSpy = vi.fn() + const cloudDouble = { captureEvent: captureSpy } as unknown as import("@roo-code/cloud").CloudService + const instanceSpy = vi.spyOn(CloudService, "instance", "get").mockReturnValue(cloudDouble) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + await flushMicrotasks() + captureSpy.mockClear() + + const cardMessage = { + ts: Date.now(), + type: "say" as const, + say: "change_card" as const, + text: JSON.stringify({ checkpointId: "abc", files: [{ path: ".env", diff: "+SECRET=x" }] }), + } + await task["updateClineMessage"](cardMessage) + expect(captureSpy).not.toHaveBeenCalled() + + enabledSpy.mockRestore() + instanceSpy.mockRestore() + }) + }) }) describe("Queued message processing after condense", () => { @@ -7427,6 +7680,34 @@ describe("Queued message processing after condense", () => { expect(submitSpy).toHaveBeenCalledWith("queued text", ["img1.png"]) expect(task.messageQueueService.isEmpty()).toBe(true) }) + it("does not dequeue queued messages once the task is aborted", () => { + // Tools await the checkpoint before handing off queued work; a + // cancellation landing during that await must not consume the queued + // message: submitUserMessage rejects once abort is set, so dequeuing + // would drop the input instead of leaving it for the resume flow. + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } + const provider = createProvider() + const task = new Task({ + provider, + apiConfiguration: apiConfig, + task: "initial task", + startTask: false, + }) + const submitSpy = vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + + task.messageQueueService.addMessage("queued text") + task.abort = true + try { + task.processQueuedMessages() + } finally { + task.abort = false + } + + expect(submitSpy).not.toHaveBeenCalled() + expect(task.messageQueueService.isEmpty()).toBe(false) + }) it("does not cross-drain queues between separate tasks", async () => { const providerA = createProvider() diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 3b664b3bd2..275a314e1e 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -13,6 +13,8 @@ import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats" import type { ToolUse } from "../../shared/tools" +import { checkAutoApproval } from "../auto-approval" +import { checkpointSave } from "../checkpoints" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -144,9 +146,13 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { diff: diffContent, } + // Hoisted: both save branches build the same approval message, and the + // B3a per-write checkpoint (below) needs it for auto-approval parity. + let completeMessage = "" + if (isPreventFocusDisruptionEnabled) { // Direct file write without diff view - const completeMessage = JSON.stringify({ + completeMessage = JSON.stringify({ ...sharedMessageProps, diff: diffContent, content: unifiedPatch, @@ -191,7 +197,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { await task.diffViewProvider.update(diffResult.content, true) task.diffViewProvider.scrollToFirstDiff() - const completeMessage = JSON.stringify({ + completeMessage = JSON.stringify({ ...sharedMessageProps, diff: diffContent, content: unifiedPatch, @@ -224,6 +230,40 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } + // B3a: per-write checkpoint + change card for the apply_diff write, with + // the same parity as write_to_file / edit_file / apply_patch (this tool + // previously wrote with no checkpoint and no card, so its edits had no + // per-file rollback surface in chat). The applied unified diff and its + // stats were already computed above for the tool message and are reused + // verbatim; auto-approved steps always get the compact card. Live setting + // with default-on semantics: skip only when explicitly false. apply_diff + // only ever modifies an existing file (a missing file errors out before + // the diff is applied), so the operation is always "update". + const perWriteCheckpoints = state?.perWriteCheckpoints + if (perWriteCheckpoints !== false) { + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + await checkpointSave(task, false, true, { + path: relPath, + operation: "update", + ...(diffStats ? { diffStats: { additions: diffStats.added, deletions: diffStats.removed } } : {}), + ...(unifiedPatch ? { diff: unifiedPatch } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + // A persistence failure (a committed checkpoint whose journal + // update was lost) propagates to the catch below - explicit tool + // failure handling surfacing the checkpoint id and affected paths - + // instead of reporting the edit as fully persisted. + }) + } + // Track file edit operation if (relPath) { await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index 2713e08ffe..e92018510b 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -6,6 +6,8 @@ import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Task } from "../task/Task" +import { checkpointSave } from "../checkpoints" +import { checkAutoApproval } from "../auto-approval" import { formatResponse } from "../prompts/responses" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" @@ -20,6 +22,25 @@ interface ApplyPatchParams { patch: string } +/** + * B2: result of a single file operation within a patch. `succeeded` controls + * the whole-patch success state (and therefore the per-patch checkpoint), + * while `wrote` records whether the operation actually wrote a file — a no-op + * update must not produce a change-journal entry for a file that was never + * written. + */ +interface ApplyPatchFileOpResult { + succeeded: boolean + wrote: boolean + /** + * Successful per-file result message. Buffered by execute and emitted only + * after the post-patch checkpoint + journal persistence succeeded, so a + * success is never reported before its journal save completes. Failure + * results stay immediate. + */ + resultMessage?: string +} + export class ApplyPatchTool extends BaseTool<"apply_patch"> { readonly name = "apply_patch" as const @@ -56,6 +77,11 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const { patch } = params const { handleError, pushToolResult } = callbacks + // Declared before the try so the finally block can hand off queued + // messages when a write succeeded but the post-patch checkpoint failed. + const successfulChanges: ApplyPatchFileChange[] = [] + const bufferedResults: string[] = [] + try { // Validate required parameters if (!patch) { @@ -102,7 +128,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { return } - // Process each file change + // Process each file change. The handlers report whether their file + // operation succeeded (which controls the whole-patch checkpoint) and + // whether it actually wrote a file (which controls the change journal + // — a no-op update must not be journaled). A rejected approval or a + // failed local write never gets checkpointed as a success. + let patchSucceeded = true for (const change of changes) { const relPath = change.path const absolutePath = path.resolve(task.cwd, relPath) @@ -112,7 +143,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) - return + // B2 partial flush: break, not return - an earlier hunk may have + // already written a file, and those writes must still receive the + // checkpoint, journal entry, and change card. Failing the patch + // also keeps the consecutive-mistake counter from resetting. + patchSucceeded = false + break } // Check if file is write-protected @@ -120,20 +156,131 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (change.type === "add") { // Create new file - await this.handleAddFile(change, absolutePath, relPath, task, callbacks, isWriteProtected) + const addResult = await this.handleAddFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + ) + patchSucceeded = addResult.succeeded && patchSucceeded + if (addResult.wrote) { + successfulChanges.push(change) + } + if (addResult.resultMessage) { + bufferedResults.push(addResult.resultMessage) + } } else if (change.type === "delete") { // Delete file - await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected) + const deleteResult = await this.handleDeleteFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + ) + patchSucceeded = deleteResult.succeeded && patchSucceeded + if (deleteResult.wrote) { + successfulChanges.push(change) + } + if (deleteResult.resultMessage) { + bufferedResults.push(deleteResult.resultMessage) + } } else if (change.type === "update") { - // Update file - await this.handleUpdateFile(change, absolutePath, relPath, task, callbacks, isWriteProtected) + // Update file (a no-op update succeeds without writing) + const updateResult = await this.handleUpdateFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + ) + patchSucceeded = updateResult.succeeded && patchSucceeded + if (updateResult.wrote) { + successfulChanges.push(change) + } + if (updateResult.resultMessage) { + bufferedResults.push(updateResult.resultMessage) + } + } + } + + // Reset the consecutive-mistake counter only after a fully successful + // patch: a failed operation (missing file, rejected move, ...) increments + // the counter, and the count must survive a partially written patch so + // the auto-approval safety net still engages across consecutive failed + // patches. + if (patchSucceeded) { + task.consecutiveMistakeCount = 0 + } + + // B1: one checkpoint for the whole patch (not per file). Live + // setting with default-on semantics: skip only when explicitly false. + // B3a partial flush: the checkpoint and journal are also taken when at + // least one file operation wrote, even if a later hunk of the same + // patch failed - the journal then documents exactly the subset that + // was written, and the failed operation was already reported through + // pushToolResult. A fully failed patch (nothing written) leaves no + // checkpoint behind. + if (patchSucceeded || successfulChanges.length > 0) { + const perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints + if (perWriteCheckpoints !== false) { + // B2: one journal entry per file that was actually written by + // the patch (the simplest correct design for multi-file patches), + // all referencing the single checkpoint above. A no-op update + // contributes no entry because nothing was written. `movePath`, + // when present, is the file's final location. B3a: the per-file + // approval diff/stats and auto-approval state, retained by the + // handlers, feed the per-step change card. + // Awaited: a later write must not interleave with this patch's + // staging/commit/journal/change-card work. checkpointSave rejects with a + // CheckpointPartialPersistenceError when a committed checkpoint's + // journal update fails; the catch below reports it. + await checkpointSave( + task, + false, + true, + successfulChanges.map((change) => ({ + path: change.movePath ?? change.path, + operation: change.type === "add" ? "create" : change.type, + ...(change.diffStats + ? { + diffStats: { + additions: change.diffStats.added, + deletions: change.diffStats.removed, + }, + } + : {}), + ...(change.diff ? { diff: change.diff } : {}), + ...(change.autoApproved ? { autoApproved: true } : {}), + })), + ) } } - task.consecutiveMistakeCount = 0 + // CR follow-up: successful per-file results are emitted only after + // the post-patch checkpoint + journal persistence above succeeded. + // A persistence failure routes to the catch below and the buffered + // successes are dropped in favor of the tool failure result; + // failure results stayed immediate. + for (const message of bufferedResults) { + pushToolResult(message) + } } catch (error) { await handleError("apply patch", error as Error) await task.diffViewProvider.reset() + } finally { + // B2: deferred from the per-file handlers - a queued write must not + // start until the post-patch checkpoint above has persisted. The + // handoff runs even when that checkpoint/journal persistence + // rejected: the file writes themselves succeeded and upstream + // handed off after every successful write. + if (successfulChanges.length > 0) { + task.processQueuedMessages() + } } } @@ -144,7 +291,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + ): Promise { const { askApproval, pushToolResult } = callbacks // Check if file already exists @@ -155,7 +302,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `File already exists: ${relPath}. Use Update File instead.` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return + return { succeeded: false, wrote: false } } const newContent = change.newContent || "" @@ -194,6 +341,21 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { diffStats, } satisfies ClineSayTool) + // B3a: retain the approval diff/stats and auto-approval state so the + // post-loop checkpoint hook can build the per-step change card. + change.diff = sanitizedDiff + change.diffStats = diffStats + change.autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Show diff view if focus disruption prevention is disabled if (!isPreventFocusDisruptionEnabled) { await task.diffViewProvider.open(relPath) @@ -209,7 +371,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } pushToolResult("Changes were rejected by the user.") await task.diffViewProvider.reset() - return + return { succeeded: false, wrote: false } } // Save the changes @@ -224,18 +386,18 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task.didEditFile = true const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, true) - pushToolResult(message) await task.diffViewProvider.reset() - task.processQueuedMessages() + return { succeeded: true, wrote: true, resultMessage: message } } private async handleDeleteFile( + change: ApplyPatchFileChange, absolutePath: string, relPath: string, task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + ): Promise { const { askApproval, pushToolResult } = callbacks // Check if file exists @@ -246,7 +408,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `File not found: ${relPath}. Cannot delete a non-existent file.` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return + return { succeeded: false, wrote: false } } const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) @@ -264,11 +426,24 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { isProtected: isWriteProtected, } satisfies ClineSayTool) + // B3a: auto-approval state feeds the per-step change card (deletes have + // no diff to thread). + change.autoApproved = + ( + await checkAutoApproval({ + state: await task.providerRef.deref()?.getState(), + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { pushToolResult("Delete operation was rejected by the user.") - return + return { succeeded: false, wrote: false } } // Delete the file @@ -278,12 +453,11 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `Failed to delete file '${relPath}': ${error instanceof Error ? error.message : String(error)}` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return + return { succeeded: false, wrote: false } } task.didEditFile = true - pushToolResult(`Successfully deleted ${relPath}`) - task.processQueuedMessages() + return { succeeded: true, wrote: true, resultMessage: `Successfully deleted ${relPath}` } } private async handleUpdateFile( @@ -293,9 +467,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + ): Promise { const { askApproval, pushToolResult } = callbacks + // A move reports failure when the original file cannot be deleted + // after the copy (both paths would remain on disk). + let moveSucceeded = true + // Check if file exists const fileExists = await fileExistsAtPath(absolutePath) if (!fileExists) { @@ -304,7 +482,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `File not found: ${relPath}. Cannot update a non-existent file.` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return + return { succeeded: false, wrote: false } } const originalContent = change.originalContent || "" @@ -318,9 +496,12 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { // Generate and validate diff const diff = formatResponse.createPrettyPatch(relPath, originalContent, newContent) if (!diff) { - pushToolResult(`No changes needed for '${relPath}'`) + // A no-op change is not a failure: the patch processed cleanly and + // nothing was written, so the whole-patch success state is kept — + // but `wrote` stays false so the change journal does not document a + // write that never happened. await task.diffViewProvider.reset() - return + return { succeeded: true, wrote: false, resultMessage: `No changes needed for '${relPath}'` } } // Check experiment settings @@ -351,6 +532,21 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { diffStats, } satisfies ClineSayTool) + // B3a: retain the approval diff/stats and auto-approval state so the + // post-loop checkpoint hook can build the per-step change card. + change.diff = sanitizedDiff + change.diffStats = diffStats + change.autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Show diff view if focus disruption prevention is disabled if (!isPreventFocusDisruptionEnabled) { await task.diffViewProvider.open(relPath) @@ -366,7 +562,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } pushToolResult("Changes were rejected by the user.") await task.diffViewProvider.reset() - return + return { succeeded: false, wrote: false } } // Handle file move if specified @@ -379,7 +575,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.say("rooignore_error", change.movePath) pushToolResult(formatResponse.rooIgnoreError(change.movePath)) await task.diffViewProvider.reset() - return + return { succeeded: false, wrote: false } } // Check if destination path is write-protected @@ -391,7 +587,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) await task.diffViewProvider.reset() - return + return { succeeded: false, wrote: false } } // Check if destination path is outside workspace @@ -403,7 +599,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) await task.diffViewProvider.reset() - return + return { succeeded: false, wrote: false } } // Save new content to the new path @@ -422,11 +618,19 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await fs.writeFile(moveAbsolutePath, newContent, "utf8") } - // Delete the original file + // Delete the original file. A failed deletion leaves both paths on + // disk, so the move must be reported as a failure rather than + // checkpointed and journaled as a completed move. try { await fs.unlink(absolutePath) } catch (error) { + moveSucceeded = false console.error(`Failed to delete original file after move: ${error}`) + task.consecutiveMistakeCount++ + task.recordToolError("apply_patch") + const errorMessage = `Move of '${relPath}' to '${change.movePath}' failed: could not delete the original file.` + await task.say("error", errorMessage) + pushToolResult(formatResponse.toolError(errorMessage)) } await task.fileContextTracker.trackFileContext(change.movePath, "roo_edited" as RecordSource) @@ -444,9 +648,14 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task.didEditFile = true const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) - pushToolResult(message) await task.diffViewProvider.reset() - task.processQueuedMessages() + if (!moveSucceeded) { + // The destination file was written on disk before the source + // deletion failed, so the write must still be checkpointed and + // journaled; the move itself is reported as failed. + return { succeeded: false, wrote: true, resultMessage: message } + } + return { succeeded: true, wrote: true, resultMessage: message } } override async handlePartial(task: Task, block: ToolUse<"apply_patch">): Promise { diff --git a/src/core/tools/EditFileTool.ts b/src/core/tools/EditFileTool.ts index a7301e2ac9..5fe0d4ee95 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -1,7 +1,7 @@ import fs from "fs/promises" import path from "path" -import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" +import { type ClineSayTool, DEFAULT_PER_WRITE_CHECKPOINTS, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" @@ -11,6 +11,8 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" +import { checkpointSave } from "../../core/checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -146,6 +148,7 @@ export class EditFileTool extends BaseTool<"edit_file"> { const { askApproval, handleError, pushToolResult } = callbacks let relPathForErrorHandling: string | undefined let operationPreviewForErrorHandling: string | undefined + let didWrite = false const finalizePartialToolAskIfNeeded = async (relPath: string): Promise => { if (!this.didSendPartialToolAsk) { @@ -392,6 +395,7 @@ export class EditFileTool extends BaseTool<"edit_file"> { const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const perWriteCheckpoints = state?.perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -455,8 +459,39 @@ export class EditFileTool extends BaseTool<"edit_file"> { } task.didEditFile = true + didWrite = true + + if (perWriteCheckpoints) { + // B2: the change-journal entry for this edit is appended inside + // checkpointSave (the hook stays a single call site), keyed by the + // checkpoint commit that call produces. B3a threads the approval + // diff (for the change card) and whether the step was auto- + // approved (auto-approved steps always get the compact card). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Awaited: a later tool block must not interleave with this edit's + // staging/commit/journal/change-card work. checkpointSave rejects with a + // CheckpointPartialPersistenceError when a committed checkpoint's + // journal update fails; the catch below reports it. + await checkpointSave(task, false, true, { + path: relPath, + operation: isNewFile ? "create" : "update", + diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, + ...(sanitizedDiff ? { diff: sanitizedDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }) + } - // Get the formatted response message + // Emitted only after checkpoint + journal persistence succeeded: a + // persistence failure must not report the edit as fully persisted. const replacementInfo = !isNewFile && expected_replacements > 1 ? ` (${expected_replacements} replacements)` : "" const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, isNewFile) @@ -465,9 +500,6 @@ export class EditFileTool extends BaseTool<"edit_file"> { await task.diffViewProvider.reset() this.resetPartialState() - - // Process any queued messages after file edit completes - task.processQueuedMessages() } catch (error) { if (relPathForErrorHandling) { await finalizePartialToolAskIfNeeded(relPathForErrorHandling) @@ -479,6 +511,13 @@ export class EditFileTool extends BaseTool<"edit_file"> { this.didSendPartialToolAsk = false this.partialToolAskRelPath = undefined this.resetPartialState() + // Hand off queued messages whenever the write itself succeeded - + // including when the post-write checkpoint/journal persistence then + // rejected (the write is real; upstream handed off after every + // successful write). + if (didWrite) { + task.processQueuedMessages() + } } } diff --git a/src/core/tools/EditTool.ts b/src/core/tools/EditTool.ts index b121fcc096..42cce82ff0 100644 --- a/src/core/tools/EditTool.ts +++ b/src/core/tools/EditTool.ts @@ -11,6 +11,8 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" +import { checkpointSave } from "../../core/checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -28,6 +30,7 @@ export class EditTool extends BaseTool<"edit"> { async execute(params: EditParams, task: Task, callbacks: ToolCallbacks): Promise { const { file_path: relPath, old_string: oldString, new_string: newString, replace_all: replaceAll } = params const { askApproval, handleError, pushToolResult } = callbacks + let didWrite = false try { // Validate required parameters @@ -167,6 +170,7 @@ export class EditTool extends BaseTool<"edit"> { const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const perWriteCheckpoints = state?.perWriteCheckpoints ?? true const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -224,20 +228,56 @@ export class EditTool extends BaseTool<"edit"> { } task.didEditFile = true + didWrite = true + + if (perWriteCheckpoints) { + // B2: the change-journal entry for this edit is appended inside + // checkpointSave (the hook stays a single call site), keyed by the + // checkpoint commit that call produces. B3a threads the approval + // diff (for the change card) and whether the step was auto- + // approved (auto-approved steps always get the compact card). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Awaited: a later tool block must not interleave with this edit's + // staging/commit/journal/change-card work. checkpointSave rejects with a + // CheckpointPartialPersistenceError when a committed checkpoint's + // journal update fails; the catch below reports it. + await checkpointSave(task, false, true, { + path: relPath, + operation: "update", + diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, + ...(sanitizedDiff ? { diff: sanitizedDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }) + } - // Get the formatted response message + // Emitted only after checkpoint + journal persistence succeeded: a + // persistence failure must not report the edit as fully persisted. const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) pushToolResult(message) await task.diffViewProvider.reset() this.resetPartialState() - - // Process any queued messages after file edit completes - task.processQueuedMessages() } catch (error) { await handleError("edit", error as Error) await task.diffViewProvider.reset() this.resetPartialState() + } finally { + // Hand off queued messages whenever the write itself succeeded - + // including when the post-write checkpoint/journal persistence then + // rejected (the write is real; upstream handed off after every + // successful write). + if (didWrite) { + task.processQueuedMessages() + } } } diff --git a/src/core/tools/SearchReplaceTool.ts b/src/core/tools/SearchReplaceTool.ts index a0f2dadfcc..787781addd 100644 --- a/src/core/tools/SearchReplaceTool.ts +++ b/src/core/tools/SearchReplaceTool.ts @@ -11,6 +11,8 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" +import { checkpointSave } from "../../core/checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -27,6 +29,7 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { async execute(params: SearchReplaceParams, task: Task, callbacks: ToolCallbacks): Promise { const { file_path, old_string, new_string } = params const { askApproval, handleError, pushToolResult } = callbacks + let didWrite = false try { // Validate required parameters @@ -163,6 +166,7 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const perWriteCheckpoints = state?.perWriteCheckpoints ?? true const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -220,20 +224,57 @@ export class SearchReplaceTool extends BaseTool<"search_replace"> { } task.didEditFile = true + didWrite = true + + if (perWriteCheckpoints) { + // B2: the change-journal entry for this search-and-replace is appended + // inside checkpointSave (the hook stays a single call site), keyed + // by the checkpoint commit that call produces. B3a threads the + // approval diff (for the change card) and whether the step was auto- + // approved (auto-approved steps always get the compact card). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Awaited: a later tool block must not interleave with this + // search-and-replace's staging/commit/journal/change-card work. + // checkpointSave rejects with a CheckpointPartialPersistenceError + // when a committed checkpoint's journal update fails; the catch below + // reports it. + await checkpointSave(task, false, true, { + path: relPath, + operation: "update", + diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, + ...(sanitizedDiff ? { diff: sanitizedDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }) + } - // Get the formatted response message + // Emitted only after checkpoint + journal persistence succeeded: a + // persistence failure must not report the edit as fully persisted. const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) pushToolResult(message) await task.diffViewProvider.reset() this.resetPartialState() - - // Process any queued messages after file edit completes - task.processQueuedMessages() } catch (error) { await handleError("search and replace", error as Error) await task.diffViewProvider.reset() this.resetPartialState() + } finally { + // Hand off queued messages whenever the write itself succeeded - + // including when the post-write checkpoint/journal persistence then + // rejected (the write is real; upstream handed off after every + // successful write). + if (didWrite) { + task.processQueuedMessages() + } } } diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 0c5c80abb9..42bc94f411 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -1,7 +1,7 @@ import path from "path" import fs from "fs/promises" -import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" +import { type ClineSayTool, DEFAULT_PER_WRITE_CHECKPOINTS, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" @@ -12,7 +12,9 @@ import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" -import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats" +import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff, type DiffStats } from "../diff/stats" +import { checkpointSave } from "../checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -95,6 +97,8 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { isProtected: isWriteProtected, } + let didWrite = false + try { task.consecutiveMistakeCount = 0 @@ -102,11 +106,20 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const perWriteCheckpoints = state?.perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, ) + // B2: the approval-diff stats for the write, shared by both the + // approval message and the change-journal entry below. B3a also + // reuses the sanitized unified diff itself for the per-step change + // card (never recomputed). + let approvalDiffStats: DiffStats | null = null + let approvalDiff = "" + let completeMessage = "" + if (isPreventFocusDisruptionEnabled) { task.diffViewProvider.editType = fileExists ? "modify" : "create" if (fileExists) { @@ -120,10 +133,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) - const completeMessage = JSON.stringify({ + approvalDiffStats = computeDiffStats(unified) + approvalDiff = unified + completeMessage = JSON.stringify({ ...sharedMessageProps, content: unified, - diffStats: computeDiffStats(unified) || undefined, + diffStats: approvalDiffStats || undefined, } satisfies ClineSayTool) const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) @@ -151,10 +166,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) - const completeMessage = JSON.stringify({ + approvalDiffStats = computeDiffStats(unified) + approvalDiff = unified + completeMessage = JSON.stringify({ ...sharedMessageProps, content: unified, - diffStats: computeDiffStats(unified) || undefined, + diffStats: approvalDiffStats || undefined, } satisfies ClineSayTool) const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) @@ -172,7 +189,41 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { } task.didEditFile = true + didWrite = true + + if (perWriteCheckpoints) { + // B2: the change-journal entry for this write is appended inside + // checkpointSave (the hook stays a single call site), keyed by the + // checkpoint commit that call produces. Await so the checkpoint + // (staging + commit + journal) finishes before the next queued write + // starts; otherwise two writes can collapse into one commit. B3a threads + // the approval diff (for the change card) and whether the step was + // auto-approved (auto-approved steps always get the compact card). + // A persistence failure - a committed checkpoint whose journal update + // was lost - propagates to the catch below (explicit tool failure + // handling, surfacing the checkpoint id and affected paths). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + await checkpointSave(task, false, true, { + path: relPath, + operation: fileExists ? "update" : "create", + diffStats: approvalDiffStats + ? { additions: approvalDiffStats.added, deletions: approvalDiffStats.removed } + : undefined, + ...(approvalDiff ? { diff: approvalDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }) + } + // Emitted only after checkpoint + journal persistence succeeded. const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, !fileExists) pushToolResult(message) @@ -180,14 +231,20 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { await task.diffViewProvider.reset() this.resetPartialState() - task.processQueuedMessages() - return } catch (error) { await handleError("writing file", error as Error) await task.diffViewProvider.reset() this.resetPartialState() return + } finally { + // Hand off queued messages whenever the write itself succeeded - + // including when the post-write checkpoint/journal persistence then + // rejected (the write is real; upstream handed off after every + // successful write). + if (didWrite) { + task.processQueuedMessages() + } } } diff --git a/src/core/tools/__tests__/applyDiffTool.changeCard.spec.ts b/src/core/tools/__tests__/applyDiffTool.changeCard.spec.ts new file mode 100644 index 0000000000..87f6617e83 --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.changeCard.spec.ts @@ -0,0 +1,232 @@ +// npx vitest run core/tools/__tests__/applyDiffTool.changeCard.spec.ts + +import type { MockedFunction } from "vitest" + +import { fileExistsAtPath } from "../../../utils/fs" +import { checkAutoApproval } from "../../auto-approval" +import { checkpointSave } from "../../checkpoints" +import type { Task } from "../../task/Task" +import { ApplyDiffTool } from "../ApplyDiffTool" + +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn().mockResolvedValue("original file content\n"), + // The trial aggregate adds the S4b stat-pair self-observation around this + // read (ApplyDiffTool records its own read under the ReadFileTool contract); + // its `.catch(() => undefined)` fallback keeps the observation no-op when + // stat rejects, so this spec stays green on both the component branch and + // the trial aggregate. + stat: vi.fn().mockRejectedValue(new Error("stat unavailable in this spec")), + }, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((msg: string) => `Error: ${msg}`), + rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("../../diff/stats", () => ({ + // The real DiffStats shape is { added, removed } (the tool maps it to the + // change-card { additions, deletions } pair). + sanitizeUnifiedDiff: vi.fn((diff: string) => diff), + computeDiffStats: vi.fn(() => ({ added: 1, removed: 1 })), +})) + +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../../auto-approval", () => ({ + checkAutoApproval: vi.fn().mockResolvedValue({ decision: "ask" }), +})) + +describe("ApplyDiffTool.execute - per-write checkpoint and change card (B3a, epic #1375)", () => { + const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction + const mockCheckpointSave = checkpointSave as MockedFunction + const mockCheckAutoApproval = checkAutoApproval as MockedFunction + + let tool: ApplyDiffTool + let mockTask: Pick< + Task, + | "cwd" + | "consecutiveMistakeCount" + | "consecutiveMistakeCountForApplyDiff" + | "recordToolError" + | "rooIgnoreController" + | "rooProtectedController" + | "processQueuedMessages" + | "didEditFile" + | "api" + | "diffStrategy" + | "diffViewProvider" + | "providerRef" + | "fileContextTracker" + > + let mockSaveDirectly: MockedFunction<(...args: unknown[]) => Promise> + let mockGetState: MockedFunction<() => Promise>> + let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> + let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> + let mockPushToolResult: MockedFunction<(...args: unknown[]) => void> + + beforeEach(() => { + vi.clearAllMocks() + + mockedFileExistsAtPath.mockResolvedValue(true) + + // Structural stubs for the prevent-focus-disruption save path: the real + // DiffViewProvider is out of scope here, so vi.fn() doubles stand in for + // the members the tool touches. + mockSaveDirectly = vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "new content", + }) + const diffViewProviderStub = { + editType: undefined as "create" | "modify" | undefined, + originalContent: undefined as string | undefined, + saveDirectly: mockSaveDirectly, + pushToolWriteResult: vi.fn().mockResolvedValue("Saved file"), + reset: vi.fn().mockResolvedValue(undefined), + } + + mockGetState = vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + // Exercise the focus-disruption (saveDirectly) save path. + experiments: { preventFocusDisruption: true }, + }) + + mockTask = { + cwd: "/workspace/project", + consecutiveMistakeCount: 0, + consecutiveMistakeCountForApplyDiff: new Map(), + recordToolError: vi.fn(), + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + } as unknown as Task["rooIgnoreController"], + rooProtectedController: { + isWriteProtected: vi.fn().mockReturnValue(false), + } as unknown as Task["rooProtectedController"], + processQueuedMessages: vi.fn(), + didEditFile: false, + api: { + getModel: () => ({ id: "claude-sonnet-4-5" }), + } as unknown as Task["api"], + diffStrategy: { + applyDiff: vi.fn().mockResolvedValue({ success: true, content: "modified file content\n" }), + } as unknown as Task["diffStrategy"], + diffViewProvider: diffViewProviderStub as unknown as Task["diffViewProvider"], + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: mockGetState, + }), + } as unknown as Task["providerRef"], + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as unknown as Task["fileContextTracker"], + } + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + mockPushToolResult = vi.fn() + + tool = new ApplyDiffTool() + }) + + it("records a per-write checkpoint with the applied diff after a successful write", async () => { + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: "src/thing.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + }) + }) + + it("routes a journal persistence failure to tool failure handling without a success result", async () => { + // The checkpoint committed but its journal update failed: the edit must + // not be reported as fully persisted. + mockCheckpointSave.mockRejectedValueOnce( + new Error( + "Checkpoint deadbeef was committed but its change journal could not be persisted (files: src/thing.ts); rollback metadata is incomplete.", + ), + ) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockHandleError).toHaveBeenCalledWith("applying diff", expect.any(Error)) + const surfaced = String(mockHandleError.mock.calls[0]?.[1]) + expect(surfaced).toContain("deadbeef") + expect(surfaced).toContain("src/thing.ts") + expect(mockPushToolResult).not.toHaveBeenCalled() + }) + + it("marks auto-approved steps so the card renders compact", async () => { + mockCheckAutoApproval.mockResolvedValueOnce({ decision: "approve" }) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockCheckpointSave.mock.calls[0]?.[3]).toEqual(expect.objectContaining({ autoApproved: true })) + }) + + it("skips the checkpoint when perWriteCheckpoints is explicitly disabled", async () => { + mockGetState.mockResolvedValueOnce({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + perWriteCheckpoints: false, + }) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockCheckpointSave).not.toHaveBeenCalled() + // The write itself still happens (the setting gates the checkpoint only), + // and auto-approval is never consulted when there is no card to build. + expect(mockSaveDirectly).toHaveBeenCalled() + expect(mockCheckAutoApproval).not.toHaveBeenCalled() + }) + + it("records nothing when the approval is declined", async () => { + mockAskApproval.mockResolvedValue(false) + + await tool.execute({ path: "src/thing.ts", diff: "unified diff" }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockCheckpointSave).not.toHaveBeenCalled() + expect(mockSaveDirectly).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts index eb89838cad..b36c4524df 100644 --- a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts +++ b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts @@ -4,13 +4,29 @@ import type { MockedFunction } from "vitest" import { fileExistsAtPath } from "../../../utils/fs" import { isPathOutsideWorkspace } from "../../../utils/pathUtils" +import * as fsPromises from "fs/promises" import type { Task } from "../../task/Task" +import { checkpointSave } from "../../checkpoints" import { ApplyPatchTool } from "../ApplyPatchTool" +// The vi.mock factory exposes the fs/promises functions under a `default` +// property (matching the SUT's default import), which the static module type +// does not declare; cast once at this boundary rather than at each call site. +const mockedFsPromises = vi.mocked( + fsPromises as unknown as { + default: { + unlink: MockedFunction + writeFile: MockedFunction + } + }, +) + vi.mock("fs/promises", () => ({ default: { readFile: vi.fn().mockResolvedValue("original file content\n"), unlink: vi.fn().mockResolvedValue(undefined), + mkdir: vi.fn().mockResolvedValue(undefined), + writeFile: vi.fn().mockResolvedValue(undefined), }, })) @@ -22,6 +38,13 @@ vi.mock("../../../utils/pathUtils", () => ({ isPathOutsideWorkspace: vi.fn().mockReturnValue(false), })) +vi.mock("../../checkpoints", () => ({ + getCheckpointService: vi.fn(), + checkpointSave: vi.fn().mockResolvedValue(undefined), + checkpointRestore: vi.fn(), + checkpointDiff: vi.fn(), +})) + describe("ApplyPatchTool.execute - delete file success path", () => { const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction const mockedIsPathOutsideWorkspace = isPathOutsideWorkspace as MockedFunction @@ -38,6 +61,9 @@ describe("ApplyPatchTool.execute - delete file success path", () => { | "say" | "processQueuedMessages" | "didEditFile" + | "providerRef" + | "diffViewProvider" + | "fileContextTracker" > let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> @@ -52,6 +78,11 @@ describe("ApplyPatchTool.execute - delete file success path", () => { mockTask = { cwd: "/workspace/project", consecutiveMistakeCount: 0, + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({}), + }), + } as unknown as Task["providerRef"], recordToolUsage: vi.fn(), recordToolError: vi.fn(), rooIgnoreController: { @@ -63,6 +94,21 @@ describe("ApplyPatchTool.execute - delete file success path", () => { say: vi.fn().mockResolvedValue(undefined), processQueuedMessages: vi.fn(), didEditFile: false, + diffViewProvider: { + editType: "modify", + originalContent: undefined, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + revertChanges: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue({ finalContent: "saved" }), + saveChanges: vi.fn().mockResolvedValue(undefined), + pushToolWriteResult: vi.fn().mockResolvedValue("File saved successfully"), + } as unknown as Task["diffViewProvider"], + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as unknown as Task["fileContextTracker"], } mockAskApproval = vi.fn().mockResolvedValue(true) @@ -94,4 +140,672 @@ describe("ApplyPatchTool.execute - delete file success path", () => { expect(mockTask.recordToolUsage).not.toHaveBeenCalled() expect(mockTask.recordToolError).not.toHaveBeenCalled() }) + + describe("per-write checkpoints (B1)", () => { + const deletePatch = `*** Begin Patch +*** Delete File: src/obsolete.ts +*** End Patch` + const mockedCheckpointSave = checkpointSave as MockedFunction + + it("records one suppressed checkpoint for the whole patch (default-on)", async () => { + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted")) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // B2: the delete patch produces one journal write, referencing the + // single checkpoint saved for the whole patch. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/obsolete.ts", operation: "delete" }, + ]) + }) + + it("emits per-file success results only after the post-patch checkpoint persists", async () => { + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // CR follow-up: the buffered success result is emitted only after + // checkpointSave resolved - a success is never reported before its + // journal save completes. + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted")) + const saveOrder = mockedCheckpointSave.mock.invocationCallOrder[0] + const resultOrder = mockPushToolResult.mock.invocationCallOrder[0] + expect(saveOrder).toBeLessThan(resultOrder) + }) + + it("drops buffered success results and still hands off on a journal persistence failure", async () => { + mockedCheckpointSave.mockRejectedValueOnce( + new Error( + "Checkpoint abc123def was committed but its change journal could not be persisted (files: src/obsolete.ts); rollback metadata is incomplete.", + ), + ) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The delete succeeded on disk, so the queued handoff still runs, + // but no success result is emitted: the persistence failure + // surfaces through handleError instead. + expect(mockPushToolResult).not.toHaveBeenCalled() + expect(mockHandleError).toHaveBeenCalledWith("apply patch", expect.any(Error)) + expect(vi.mocked(mockTask.processQueuedMessages)).toHaveBeenCalled() + }) + it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { + // Structural cast for the test double (matches the mock style used for the controllers above). + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ perWriteCheckpoints: false }), + }) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when patch processing fails", async () => { + // A malformed patch fails at parse time, before the change loop and + // the post-loop checkpoint hook. + const badPatch = `*** Begin Patch +*** This is not a valid hunk +*** End Patch` + + await tool.execute({ patch: badPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockTask.recordToolError).toHaveBeenCalledWith("apply_patch") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + it("awaits the post-patch checkpoint before processing queued messages", async () => { + // A pending checkpoint must not let queued messages (and the + // writes they trigger) start first: execute() awaits the + // post-patch checkpointSave and only then processes queued + // messages, so a queued write's own checkpoint cannot interleave + // with this commit. + const order: string[] = [] + ;(mockTask.processQueuedMessages as MockedFunction<() => void>).mockImplementation(() => { + order.push("processQueuedMessages") + }) + mockedCheckpointSave.mockImplementationOnce(() => { + order.push("checkpointSave:start") + return new Promise((resolve) => { + setTimeout(() => { + order.push("checkpointSave:done") + resolve() + }, 10) + }).then(() => undefined) + }) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The checkpoint fully completes before queued messages are + // processed. + expect(order).toEqual(["checkpointSave:start", "checkpointSave:done", "processQueuedMessages"]) + }) + }) + + describe("checkpoint only for fully successful patches (B1)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + const deletePatch = `*** Begin Patch +*** Delete File: src/obsolete.ts +*** End Patch` + const addPatch = `*** Begin Patch +*** Add File: src/new.ts ++hello ++world +*** End Patch` + const updatePatch = `*** Begin Patch +*** Update File: src/test.ts +@@ +-original file content ++modified content +*** End Patch` + const updateNoDiffPatch = `*** Begin Patch +*** Update File: src/test.ts +@@ +-original file content ++original file content +*** End Patch` + const movePatch = `*** Begin Patch +*** Update File: src/test.ts +*** Move to: src/moved.ts +@@ +-original file content ++modified content +*** End Patch` + + it("does not record a checkpoint when the user rejects the patch", async () => { + // Rejected approval: the handler early-returns without recording a + // tool error, so the success flag must come from the handler itself. + mockAskApproval.mockResolvedValue(false) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("Delete operation was rejected by the user.") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the file to delete does not exist", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("File not found")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the delete write fails", async () => { + mockedFsPromises.default.unlink.mockRejectedValueOnce(new Error("EBUSY")) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Failed to delete file")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the added file already exists", async () => { + // fileExistsAtPath resolves true by default in beforeEach. + + await tool.execute({ patch: addPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("File already exists")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the user rejects the add", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + mockAskApproval.mockResolvedValue(false) + + await tool.execute({ patch: addPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("Changes were rejected by the user.") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("records a checkpoint when the add succeeds", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + + await tool.execute({ patch: addPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("File saved successfully") + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + + it("does not record a checkpoint when the file to update does not exist", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("File not found")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("records a checkpoint when the update is a no-op (no changes needed)", async () => { + // A no-op change is not a failure, so the whole-patch checkpoint still runs. + + await tool.execute({ patch: updateNoDiffPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("No changes needed")) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, []) + }) + + it("does not record a checkpoint when the user rejects the update", async () => { + mockAskApproval.mockResolvedValue(false) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("Changes were rejected by the user.") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the move destination is not allowed", async () => { + // First validateAccess call (source path, in the execute loop) passes; + // the move destination check inside the handler fails. + const validateAccess = ( + mockTask["rooIgnoreController"] as unknown as { validateAccess: MockedFunction<() => boolean> } + ).validateAccess + validateAccess.mockReturnValueOnce(true).mockReturnValue(false) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockTask.say).toHaveBeenCalledWith("rooignore_error", "src/moved.ts") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("checkpoints the written subset when a later hunk is access-denied", async () => { + // Hunk 1 (src/first.ts) writes; hunk 2 (src/denied.ts) is rejected by + // validateAccess. The access-denied branch must not bypass the partial + // flush: the earlier write still receives the checkpoint/journal/card. + // Hunk 2's context matches the mocked file content so the patch + // passes pre-processing; the denial happens at the per-file access check. + const partialDenyPatch = `*** Begin Patch +*** Add File: src/first.ts ++hello +*** Update File: src/denied.ts +@@ +-original file content ++new content +*** End Patch` + const validateAccess = ( + mockTask["rooIgnoreController"] as unknown as { validateAccess: MockedFunction<() => boolean> } + ).validateAccess + validateAccess.mockReturnValueOnce(true).mockReturnValueOnce(false) + // The add target does not exist, so hunk 1 writes; fileExistsAtPath + // defaults to true and would otherwise reject the add. + mockedFileExistsAtPath.mockResolvedValueOnce(false) + + await tool.execute({ patch: partialDenyPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockTask.say).toHaveBeenCalledWith("rooignore_error", "src/denied.ts") + // Only the first (written) hunk is checkpointed. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, [ + expect.objectContaining({ path: "src/first.ts", operation: "create" }), + ]) + }) + + it("does not record a checkpoint when the move destination is write-protected", async () => { + // Source path check (execute loop) passes; the move destination fails. + const isWriteProtected = ( + mockTask["rooProtectedController"] as unknown as { + isWriteProtected: MockedFunction<(p: string) => boolean> + } + ).isWriteProtected + isWriteProtected.mockReturnValueOnce(false).mockReturnValue(true) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Cannot move file to write-protected path"), + ) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the move destination is outside the workspace", async () => { + // Source path (first call) is inside; the move destination (second) + // call is outside the workspace. + mockedIsPathOutsideWorkspace.mockReturnValueOnce(false).mockReturnValue(true) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Cannot move file to path outside workspace"), + ) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("keeps the mistake count when a patch operation fails", async () => { + // Source path check (execute loop) passes; the move destination fails. + const isWriteProtected = ( + mockTask["rooProtectedController"] as unknown as { + isWriteProtected: MockedFunction<(p: string) => boolean> + } + ).isWriteProtected + isWriteProtected.mockReturnValueOnce(false).mockReturnValue(true) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The failed operation incremented the counter; the end-of-loop reset + // must only run for a fully successful patch, so the count survives. + expect(mockTask.consecutiveMistakeCount).toBe(1) + }) + + it("clears the mistake count after a fully successful patch", async () => { + mockTask.consecutiveMistakeCount = 2 + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + }) + + it("records a checkpoint when the move succeeds", async () => { + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // path is platform-dependent (Windows resolves cwd to a drive path); + // assert on the written content instead. + expect(mockedFsPromises.default.writeFile).toHaveBeenCalledWith( + expect.any(String), + "modified content\n", + "utf8", + ) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + + it("awaits the patch checkpoint before execute settles", async () => { + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveDeferred = new Promise((resolve) => (resolveSave = resolve)) + mockedCheckpointSave.mockImplementationOnce(() => saveDeferred) + + const executePromise = tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // execute must not settle while the checkpoint is still in flight: + // a later write would otherwise interleave with this patch's staged work. + let settled = false + void executePromise.finally(() => (settled = true)) + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(settled).toBe(false) + + resolveSave() + await executePromise + expect(settled).toBe(true) + }) + + it("records a checkpoint when the in-place update succeeds", async () => { + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("File saved successfully") + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + + it("records one journal write per file change for a multi-file patch", async () => { + // src/a.ts does not exist (add); src/b.ts does (update). + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(!String(filePath).toLowerCase().endsWith("a.ts")), + ) + const multiPatch = [ + "*** Begin Patch", + "*** Add File: src/a.ts", + "+alpha", + "*** Update File: src/b.ts", + "@@", + "-original file content", + "+second content", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: multiPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // B3a: each write threads the approval diff and stats computed by its + // handler so the per-step change card can reuse them verbatim. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/a.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: expect.stringContaining("+alpha"), + }, + { + path: "src/b.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: expect.stringContaining("+second content"), + }, + ]) + }) + }) + + describe("change-card threading (B3a)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + const deletePatch = `*** Begin Patch +*** Delete File: src/obsolete.ts +*** End Patch` + + it("threads autoApproved into the checkpoint writes for auto-approved steps", async () => { + // B3a: auto-approved steps carry autoApproved on every write so + // checkpointSave can force the compact (summary) change card. + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ autoApprovalEnabled: true, alwaysAllowWrite: true }), + }) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/obsolete.ts", operation: "delete", autoApproved: true }, + ]) + }) + + it("omits autoApproved when the step is not auto-approved", async () => { + // The default provider state ({}) disables auto-approval, so no write + // carries the autoApproved flag and the card follows the user setting. + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/obsolete.ts", operation: "delete" }, + ]) + }) + + it("journals only the files actually written for a mixed no-op and write patch", async () => { + // src/same.ts exists and the hunk rewrites identical content (a + // no-op update); src/new.ts does not exist (a real write). + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(!String(filePath).toLowerCase().endsWith("new.ts")), + ) + const mixedPatch = [ + "*** Begin Patch", + "*** Update File: src/same.ts", + "@@", + "-original file content", + "+original file content", + "*** Add File: src/new.ts", + "+fresh content", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: mixedPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The no-op update is reported to the model... + expect(mockPushToolResult).toHaveBeenCalledWith("No changes needed for 'src/same.ts'") + // ...but the journal documents only the file that was actually + // written, even though the whole patch succeeded. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/new.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: expect.stringContaining("+fresh content"), + }, + ]) + }) + + it("still checkpoints the successful subset when a later hunk fails", async () => { + // src/first.ts already exists (the add fails); src/second.ts does not + // (the add writes). The whole patch fails, but the written file is + // still documented by the checkpoint and journal. + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(String(filePath).toLowerCase().endsWith("first.ts")), + ) + const partialPatch = [ + "*** Begin Patch", + "*** Add File: src/first.ts", + "+boom", + "*** Add File: src/second.ts", + "+fresh", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: partialPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The failed operation is reported to the model... + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("File already exists")) + // ...and the successful subset is checkpointed and journaled. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/second.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: expect.stringContaining("+fresh"), + }, + ]) + }) + + it("reports a failed move when the original file cannot be deleted", async () => { + mockedFsPromises.default.unlink.mockRejectedValueOnce(new Error("EBUSY: resource busy")) + const movePatch = [ + "*** Begin Patch", + "*** Update File: src/old.ts", + "*** Move to: src/new-location.ts", + "@@", + "-original file content", + "+new content", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The copy succeeded but the source still exists, so the move is + // reported as a failed tool error - but the destination write was + // made on disk and must still be covered by the checkpoint/journal. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + expect.objectContaining({ path: "src/new-location.ts", operation: "update" }), + ]) + expect(mockTask.recordToolError).toHaveBeenCalledWith("apply_patch") + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("could not delete the original file"), + ) + }) + }) }) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 95a67f04d7..ab278492ca 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -7,6 +7,8 @@ import { fileExistsAtPath } from "../../../utils/fs" import { isPathOutsideWorkspace } from "../../../utils/pathUtils" import { getReadablePath } from "../../../utils/path" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" +import { checkpointSave } from "../../checkpoints" +import { computeDiffStats } from "../../diff/stats" import { editFileTool } from "../EditFileTool" vi.mock("fs/promises", () => ({ @@ -56,7 +58,16 @@ vi.mock("../../../utils/path", () => ({ vi.mock("../../diff/stats", () => ({ sanitizeUnifiedDiff: vi.fn((diff) => diff), - computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), + // The real computeDiffStats returns { added, removed } (DiffStats) — + // keep the mock faithful to the production shape. + computeDiffStats: vi.fn(() => ({ added: 1, removed: 1 })), +})) + +vi.mock("../../checkpoints", () => ({ + getCheckpointService: vi.fn(), + checkpointSave: vi.fn().mockResolvedValue(undefined), + checkpointRestore: vi.fn(), + checkpointDiff: vi.fn(), })) vi.mock("vscode", () => ({ @@ -777,4 +788,140 @@ describe("editFileTool", () => { expect(mockAskApproval).toHaveBeenCalled() }) }) + + describe("per-write checkpoints (B1)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + + it("records one suppressed checkpoint after a successful edit (default-on)", async () => { + await executeEditFileTool({}) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // B2: the write info threads the path, operation, and the approval + // diff stats into the checkpoint hook. B3a: the approval diff itself is + // threaded verbatim for the per-step change card. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + }) + }) + + it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { + mockTask.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + perWriteCheckpoints: false, + }), + }) + + await executeEditFileTool({}) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the edit fails", async () => { + mockTask.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeEditFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("edit_file", expect.any(Error)) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("records the checkpoint with a create operation for a new file", async () => { + await executeEditFileTool({ old_string: "", new_string: "New file content" }, { fileExists: false }) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + }) + }) + + it("omits diff stats from the checkpoint write when the diff has no stats", async () => { + // A null approval diff produces no diffStats on the journal write. + // The diff itself is still threaded for the change card (B3a). + vi.mocked(computeDiffStats).mockReturnValueOnce(null) + + await executeEditFileTool({}) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + diff: "mock-diff", + }) + }) + + it("threads autoApproved into the checkpoint write for auto-approved steps", async () => { + // B3a: when the step is auto-approved the checkpoint write carries + // autoApproved so checkpointSave can force the compact change card. + mockTask.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + autoApprovalEnabled: true, + alwaysAllowWrite: true, + }), + }) + + await executeEditFileTool({}) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + autoApproved: true, + }) + }) + + it("awaits the edit checkpoint before execute settles", async () => { + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveDeferred = new Promise((resolve) => (resolveSave = resolve)) + mockedCheckpointSave.mockImplementationOnce(() => saveDeferred) + + const executePromise = executeEditFileTool({}) + + // execute must not settle while the checkpoint is still in flight: + // a later tool block would otherwise interleave with this edit's staged work. + let settled = false + void executePromise.finally(() => (settled = true)) + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(settled).toBe(false) + + resolveSave() + await executePromise + expect(settled).toBe(true) + }) + it("routes a journal persistence failure to tool failure handling without a success result", async () => { + // The checkpoint committed but its journal update failed: the edit + // must not report the write as fully persisted... + mockedCheckpointSave.mockRejectedValueOnce( + new Error( + "Checkpoint abc123def was committed but its change journal could not be persisted (files: test/file.txt); rollback metadata is incomplete.", + ), + ) + + await executeEditFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("edit_file", expect.any(Error)) + expect(mockPushToolResult).not.toHaveBeenCalled() + // ...and the queued handoff still runs, because the file write + // itself succeeded (CR follow-up: hand off from finally). + expect(vi.mocked(mockTask.processQueuedMessages)).toHaveBeenCalled() + }) + }) }) diff --git a/src/core/tools/__tests__/editSearchReplaceTool.changeCard.spec.ts b/src/core/tools/__tests__/editSearchReplaceTool.changeCard.spec.ts new file mode 100644 index 0000000000..aa6a9d30cf --- /dev/null +++ b/src/core/tools/__tests__/editSearchReplaceTool.changeCard.spec.ts @@ -0,0 +1,247 @@ +// npx vitest run core/tools/__tests__/editSearchReplaceTool.changeCard.spec.ts + +import type { MockedFunction } from "vitest" + +import { fileExistsAtPath } from "../../../utils/fs" +import { checkAutoApproval } from "../../auto-approval" +import { checkpointSave } from "../../checkpoints" +import type { Task } from "../../task/Task" +import { EditTool } from "../EditTool" +import type { ToolCallbacks } from "../BaseTool" +import { SearchReplaceTool } from "../SearchReplaceTool" + +/** + * The shared surface of the two string-replacement edit tools: both execute a + * single `old_string` -> `new_string` replacement and report through the same + * ToolCallbacks, which is all the change-card tests exercise. + */ +interface EditLikeTool { + execute( + params: { file_path: string; old_string: string; new_string: string }, + task: Task, + callbacks: ToolCallbacks, + ): Promise +} + +vi.mock("fs/promises", () => ({ + default: { + // Contains exactly one occurrence of the old_string below. + readFile: vi.fn().mockResolvedValue("old line\n"), + }, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((msg: string) => `Error: ${msg}`), + rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("../../diff/stats", () => ({ + // The real DiffStats shape is { added, removed } (the tool maps it to the + // change-card { additions, deletions } pair). + sanitizeUnifiedDiff: vi.fn((diff: string) => diff), + computeDiffStats: vi.fn(() => ({ added: 1, removed: 1 })), +})) + +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../../auto-approval", () => ({ + checkAutoApproval: vi.fn().mockResolvedValue({ decision: "ask" }), +})) + +interface Stubs { + mockTask: Pick< + Task, + | "cwd" + | "consecutiveMistakeCount" + | "recordToolError" + | "rooIgnoreController" + | "rooProtectedController" + | "processQueuedMessages" + | "didEditFile" + | "diffViewProvider" + | "providerRef" + | "fileContextTracker" + > + mockSaveDirectly: MockedFunction<(...args: unknown[]) => Promise> + mockGetState: MockedFunction<() => Promise>> +} + +/** + * Structural stubs for the prevent-focus-disruption save path: the real + * DiffViewProvider is out of scope here, so vi.fn() doubles stand in for the + * members the edit tools touch. + */ +function buildStubs(): Stubs { + const mockSaveDirectly = vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "new line\n", + }) + const diffViewProviderStub = { + editType: undefined as "create" | "modify" | undefined, + originalContent: undefined as string | undefined, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + saveDirectly: mockSaveDirectly, + saveChanges: vi.fn().mockResolvedValue({ + newProblemsMessage: "", + userEdits: undefined, + finalContent: "new line\n", + }), + revertChanges: vi.fn().mockResolvedValue(undefined), + pushToolWriteResult: vi.fn().mockResolvedValue("Saved file"), + reset: vi.fn().mockResolvedValue(undefined), + } + const mockGetState = vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + // Exercise the focus-disruption (saveDirectly) save path. + experiments: { preventFocusDisruption: true }, + }) + const mockTask: Stubs["mockTask"] = { + cwd: "/workspace/project", + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + } as unknown as Task["rooIgnoreController"], + rooProtectedController: { + isWriteProtected: vi.fn().mockReturnValue(false), + } as unknown as Task["rooProtectedController"], + processQueuedMessages: vi.fn(), + didEditFile: false, + diffViewProvider: diffViewProviderStub as unknown as Task["diffViewProvider"], + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: mockGetState, + }), + } as unknown as Task["providerRef"], + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as unknown as Task["fileContextTracker"], + } + return { mockTask, mockSaveDirectly, mockGetState } +} + +function cardTests(getTool: () => EditLikeTool) { + const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction + const mockCheckpointSave = checkpointSave as MockedFunction + const mockCheckAutoApproval = checkAutoApproval as MockedFunction + + let tool: EditLikeTool + let stubs: Stubs + let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise> + let mockHandleError: MockedFunction<(...args: unknown[]) => Promise> + let mockPushToolResult: MockedFunction<(...args: unknown[]) => void> + + beforeEach(() => { + vi.clearAllMocks() + mockedFileExistsAtPath.mockResolvedValue(true) + stubs = buildStubs() + tool = getTool() + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn().mockResolvedValue(undefined) + mockPushToolResult = vi.fn() + }) + + it("records a per-write checkpoint with the approval diff after a successful write", async () => { + await tool.execute( + { file_path: "src/thing.ts", old_string: "old line", new_string: "new line" }, + stubs.mockTask as Task, + { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }, + ) + + expect(mockCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockCheckpointSave).toHaveBeenCalledWith(stubs.mockTask, false, true, { + path: "src/thing.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + }) + }) + + it("marks auto-approved steps so the card renders compact", async () => { + mockCheckAutoApproval.mockResolvedValueOnce({ decision: "approve" }) + + await tool.execute( + { file_path: "src/thing.ts", old_string: "old line", new_string: "new line" }, + stubs.mockTask as Task, + { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }, + ) + + expect(mockCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockCheckpointSave.mock.calls[0]?.[3]).toEqual(expect.objectContaining({ autoApproved: true })) + }) + + it("skips the checkpoint when perWriteCheckpoints is explicitly disabled", async () => { + stubs.mockGetState.mockResolvedValueOnce({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + perWriteCheckpoints: false, + }) + + await tool.execute( + { file_path: "src/thing.ts", old_string: "old line", new_string: "new line" }, + stubs.mockTask as Task, + { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }, + ) + + expect(mockCheckpointSave).not.toHaveBeenCalled() + // The write itself still happens (the setting gates the checkpoint only), + // and auto-approval is never consulted when there is no card to build. + expect(stubs.mockSaveDirectly).toHaveBeenCalled() + expect(mockCheckAutoApproval).not.toHaveBeenCalled() + }) + + it("records nothing when the approval is declined", async () => { + mockAskApproval.mockResolvedValue(false) + + await tool.execute( + { file_path: "src/thing.ts", old_string: "old line", new_string: "new line" }, + stubs.mockTask as Task, + { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }, + ) + + expect(mockCheckpointSave).not.toHaveBeenCalled() + expect(stubs.mockSaveDirectly).not.toHaveBeenCalled() + }) +} + +describe("EditTool.execute - per-write checkpoint and change card (B3a, epic #1375)", () => { + cardTests(() => new EditTool()) +}) + +describe("SearchReplaceTool.execute - per-write checkpoint and change card (B3a, epic #1375)", () => { + cardTests(() => new SearchReplaceTool()) +}) diff --git a/src/core/tools/__tests__/editTool.spec.ts b/src/core/tools/__tests__/editTool.spec.ts index fbb14d7ed2..2a6c552010 100644 --- a/src/core/tools/__tests__/editTool.spec.ts +++ b/src/core/tools/__tests__/editTool.spec.ts @@ -8,6 +8,7 @@ import { isPathOutsideWorkspace } from "../../../utils/pathUtils" import { getReadablePath } from "../../../utils/path" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" import { editTool } from "../EditTool" +import { checkpointSave } from "../../checkpoints" vi.mock("fs/promises", () => ({ default: { @@ -59,6 +60,14 @@ vi.mock("../../diff/stats", () => ({ computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../../auto-approval", () => ({ + checkAutoApproval: vi.fn().mockResolvedValue({ decision: "ask" }), +})) + vi.mock("vscode", () => ({ window: { showWarningMessage: vi.fn().mockResolvedValue(undefined), @@ -425,5 +434,21 @@ describe("editTool", () => { expect(mockTask.fileContextTracker.trackFileContext).toHaveBeenCalledWith(testFilePath, "roo_edited") }) + + it("routes a journal persistence failure to tool failure handling without a success result", async () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + mockedCheckpointSave.mockRejectedValueOnce( + new Error( + "Checkpoint abc123def was committed but its change journal could not be persisted (files: test/file.txt); rollback metadata is incomplete.", + ), + ) + + await executeEditTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("edit", expect.any(Error)) + expect(mockPushToolResult).not.toHaveBeenCalled() + // The write itself succeeded: the queued handoff still runs. + expect(vi.mocked(mockTask.processQueuedMessages)).toHaveBeenCalled() + }) }) }) diff --git a/src/core/tools/__tests__/searchReplaceTool.spec.ts b/src/core/tools/__tests__/searchReplaceTool.spec.ts index 65311462cb..6914a520e7 100644 --- a/src/core/tools/__tests__/searchReplaceTool.spec.ts +++ b/src/core/tools/__tests__/searchReplaceTool.spec.ts @@ -8,6 +8,7 @@ import { isPathOutsideWorkspace } from "../../../utils/pathUtils" import { getReadablePath } from "../../../utils/path" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" import { searchReplaceTool } from "../SearchReplaceTool" +import { checkpointSave } from "../../checkpoints" vi.mock("fs/promises", () => ({ default: { @@ -59,6 +60,14 @@ vi.mock("../../diff/stats", () => ({ computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../../auto-approval", () => ({ + checkAutoApproval: vi.fn().mockResolvedValue({ decision: "ask" }), +})) + vi.mock("vscode", () => ({ window: { showWarningMessage: vi.fn().mockResolvedValue(undefined), @@ -440,5 +449,21 @@ describe("searchReplaceTool", () => { expect(mockCline.consecutiveMistakeCount).toBe(0) expect(mockAskApproval).toHaveBeenCalled() }) + + it("routes a journal persistence failure to tool failure handling without a success result", async () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + mockedCheckpointSave.mockRejectedValueOnce( + new Error( + "Checkpoint abc123def was committed but its change journal could not be persisted (files: test/file.txt); rollback metadata is incomplete.", + ), + ) + + await executeSearchReplaceTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("search and replace", expect.any(Error)) + expect(mockPushToolResult).not.toHaveBeenCalled() + // The write itself succeeded: the queued handoff still runs. + expect(vi.mocked(mockCline.processQueuedMessages)).toHaveBeenCalled() + }) }) }) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 7cb9d01585..e2fa2bca14 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -8,7 +8,10 @@ import { getReadablePath } from "../../../utils/path" import { unescapeHtmlEntities } from "../../../utils/text-normalization" import { everyLineHasLineNumbers, stripLineNumbers } from "../../../integrations/misc/extract-text" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" +import { checkpointSave } from "../../checkpoints" +import { formatResponse } from "../../prompts/responses" import { writeToFileTool } from "../WriteToFileTool" +import { convertNewFileToUnifiedDiff, sanitizeUnifiedDiff } from "../../diff/stats" vi.mock("path", async () => { const originalPath = await vi.importActual("path") @@ -89,6 +92,10 @@ vi.mock("../../ignore/RooIgnoreController", () => ({ }, })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + describe("writeToFileTool", () => { // Test data const testFilePath = "test/file.txt" @@ -96,6 +103,10 @@ describe("writeToFileTool", () => { const testContent = "Line 1\nLine 2\nLine 3" const testContentWithMarkdown = "```javascript\nLine 1\nLine 2\n```" + // The exact approval diff the tool computes for a new file (B3a threads it + // into the checkpoint write for the per-step change card). + const newFileApprovalDiff = sanitizeUnifiedDiff(convertNewFileToUnifiedDiff(testContent, testFilePath)) + // Mocked functions with correct types const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction const mockedCreateDirectoriesForFile = createDirectoriesForFile as MockedFunction @@ -106,7 +117,9 @@ describe("writeToFileTool", () => { const mockedStripLineNumbers = stripLineNumbers as MockedFunction const mockedPathResolve = path.resolve as MockedFunction - const mockCline: any = {} + const mockCline: any = { + processQueuedMessages: vi.fn(), + } let mockAskApproval: ReturnType> let mockHandleError: ReturnType> let mockPushToolResult: ReturnType> @@ -156,6 +169,7 @@ describe("writeToFileTool", () => { userEdits: null, finalContent: "final content", }), + saveDirectly: vi.fn().mockResolvedValue({ finalContent: "saved" }), scrollToFirstDiff: vi.fn(), updateDiagnosticSettings: vi.fn(), pushToolWriteResult: vi.fn().mockImplementation(async function ( @@ -473,4 +487,173 @@ describe("writeToFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) }) }) + + describe("per-write checkpoints (B1)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + + it("records one suppressed checkpoint after a successful write (default-on)", async () => { + await executeWriteFileTool({}) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // B2: the write info threads the path, operation, and the approval + // diff stats (3 added lines, 0 removed) into the checkpoint hook. + // B3a: the approval diff itself is threaded verbatim for the + // per-step change card. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + diff: newFileApprovalDiff, + }) + }) + + it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { + mockCline.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi + .fn() + .mockResolvedValue({ diagnosticsEnabled: true, writeDelayMs: 1000, perWriteCheckpoints: false }), + }) + + await executeWriteFileTool({}) + + expect(mockCline.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the write fails", async () => { + mockCline.diffViewProvider.open.mockRejectedValue(new Error("write failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("routes a journal persistence failure to tool failure handling without a success result", async () => { + // The checkpoint committed but its journal update failed: the tool + // must not report the write as a complete success. + mockedCheckpointSave.mockRejectedValueOnce( + new Error( + "Checkpoint abc123def was committed but its change journal could not be persisted (files: test/file.txt); rollback metadata is incomplete.", + ), + ) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + const surfaced = String(mockHandleError.mock.calls[0]?.[1]) + expect(surfaced).toContain("abc123def") + expect(surfaced).toContain("test/file.txt") + // The normal successful tool result is not emitted. + expect(mockPushToolResult).not.toHaveBeenCalled() + + // The write itself succeeded: the queued handoff still runs (finally). + expect(mockCline.processQueuedMessages).toHaveBeenCalled() + }) + + it("waits for the per-write checkpoint before the tool completes", async () => { + let checkpointStarted = false + let releaseCheckpoint: () => void = () => {} + mockedCheckpointSave.mockImplementationOnce(() => { + checkpointStarted = true + return new Promise((resolve) => { + releaseCheckpoint = () => resolve(undefined) + }) + }) + const processQueuedSpy = vi.fn() + mockCline.processQueuedMessages = processQueuedSpy + + const toolPromise = executeWriteFileTool({}) + + // Advance microtasks until the tool reaches the checkpoint call (all + // preceding awaits are mocked resolutions, no real timers involved). + for (let i = 0; i < 50 && !checkpointStarted; i++) { + await Promise.resolve() + } + expect(checkpointStarted).toBe(true) + + let settled = false + void toolPromise.then(() => { + settled = true + }) + + // The tool must not complete while the checkpoint is still + // staging/committing: a later write started by the task loop would + // otherwise collapse into the same (or a missing) commit. + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(settled).toBe(false) + expect(processQueuedSpy).not.toHaveBeenCalled() + + releaseCheckpoint() + await toolPromise + expect(settled).toBe(true) + expect(processQueuedSpy).toHaveBeenCalledOnce() + }) + + it("threads write info with approval diff stats when the prevent-focus-disruption experiment is enabled", async () => { + mockCline.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + + await executeWriteFileTool({}) + + // The experiment branch saves directly (no diff view) and still + // journals the write through the same single checkpoint hook, carrying + // the approval diff for the per-step change card (B3a). + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + testContent, + false, + true, + 1000, + ) + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + diff: newFileApprovalDiff, + }) + }) + + it("omits diff stats from the checkpoint write when the approval diff is empty", async () => { + // Writing identical content to an existing file produces an empty + // approval diff, so the checkpoint write carries no diffStats. + vi.mocked(formatResponse.createPrettyPatch).mockReturnValueOnce("") + + await executeWriteFileTool({}, { fileExists: true }) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "update", + }) + }) + + it("threads autoApproved into the checkpoint write for auto-approved steps", async () => { + // B3a: when the step is auto-approved the checkpoint write carries + // autoApproved so checkpointSave can force the compact change card. + mockCline.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + autoApprovalEnabled: true, + alwaysAllowWrite: true, + }), + }) + + await executeWriteFileTool({}) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + diff: newFileApprovalDiff, + autoApproved: true, + }) + }) + }) }) diff --git a/src/core/tools/apply-patch/apply.ts b/src/core/tools/apply-patch/apply.ts index 4ab377f732..64009d27bf 100644 --- a/src/core/tools/apply-patch/apply.ts +++ b/src/core/tools/apply-patch/apply.ts @@ -29,6 +29,15 @@ export interface ApplyPatchFileChange { originalContent?: string /** New content (for add/update) */ newContent?: string + /** + * B3a: the unified approval diff for this file (computed by the tool + * handler), reused by the per-step change card. + */ + diff?: string + /** B3a: { added, removed } stats of the approval diff, when computable. */ + diffStats?: { added: number; removed: number } + /** B3a: whether this file's approval was auto-approved. */ + autoApproved?: boolean } /** diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index cdd0975c5d..4d2f9d14bf 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -54,6 +54,8 @@ import { ORGANIZATION_ALLOW_ALL, DEFAULT_MODES, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_PER_WRITE_CHECKPOINTS, + DEFAULT_CHANGE_CARD_DETAIL, getModelId, isRetiredProvider, providerIdentifiers, @@ -2598,6 +2600,8 @@ export class ClineProvider ttsSpeed, enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, + changeCardDetail, soundVolume, writeDelayMs, diffFuzzyThreshold, @@ -2766,6 +2770,8 @@ export class ClineProvider ttsSpeed: ttsSpeed ?? 1.0, enableCheckpoints: enableCheckpoints ?? true, checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, + changeCardDetail: changeCardDetail ?? DEFAULT_CHANGE_CARD_DETAIL, shouldShowAnnouncement: telemetrySetting !== "unset" && lastShownAnnouncementId !== this.latestAnnouncementId, allowedCommands: mergedAllowedCommands, @@ -3005,6 +3011,8 @@ export class ClineProvider ttsSpeed: stateValues.ttsSpeed ?? 1.0, enableCheckpoints: stateValues.enableCheckpoints ?? true, checkpointTimeout: stateValues.checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: stateValues.perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, + changeCardDetail: stateValues.changeCardDetail ?? DEFAULT_CHANGE_CARD_DETAIL, soundVolume: stateValues.soundVolume, writeDelayMs: stateValues.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS, diffFuzzyThreshold: stateValues.diffFuzzyThreshold ?? DEFAULT_DIFF_FUZZY_THRESHOLD, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 29b823cf50..9109d1f686 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1167,6 +1167,8 @@ describe("ClineProvider", () => { soundEnabled: false, ttsEnabled: false, enableCheckpoints: false, + perWriteCheckpoints: false, + changeCardDetail: "summary", writeDelayMs: 1000, mcpEnabled: true, mode: defaultModeSlug, @@ -2126,6 +2128,81 @@ describe("ClineProvider", () => { expect(state.destructiveCommandGuardEnabled).toBe(false) }) + test("getState returns the saved per-write checkpoints setting", async () => { + await provider.contextProxy.setValue("perWriteCheckpoints", false) + + const state = await provider.getState() + + expect(state.perWriteCheckpoints).toBe(false) + }) + + test("getState defaults per-write checkpoints to true when unset", async () => { + const state = await provider.getState() + + expect(state.perWriteCheckpoints).toBe(true) + }) + + test("getStateToPostToWebview returns the saved per-write checkpoints setting", async () => { + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("perWriteCheckpoints", true) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(true) + }) + + test("getStateToPostToWebview returns false when per-write checkpoints is saved as false", async () => { + // The default is also true, so only an explicit false proves that the + // stored value (rather than the default) reaches the webview state. + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("perWriteCheckpoints", false) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(false) + }) + + test("getStateToPostToWebview defaults per-write checkpoints to true when unset", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(true) + }) + + test("getState returns the saved changeCardDetail setting", async () => { + await provider.contextProxy.setValue("changeCardDetail", "full") + + const state = await provider.getState() + + expect(state.changeCardDetail).toBe("full") + }) + + test("getState defaults changeCardDetail to summary when unset", async () => { + const state = await provider.getState() + + expect(state.changeCardDetail).toBe("summary") + }) + + test("getStateToPostToWebview returns the saved changeCardDetail setting", async () => { + // The default is "summary", so only an explicit "full" proves that the + // stored value (rather than the default) reaches the webview state. + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("changeCardDetail", "full") + + const state = await provider.getStateToPostToWebview() + + expect(state.changeCardDetail).toBe("full") + }) + + test("getStateToPostToWebview defaults changeCardDetail to summary when unset", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const state = await provider.getStateToPostToWebview() + + expect(state.changeCardDetail).toBe("summary") + }) + test("getStateToPostToWebview returns the saved blanket auto-deny setting", async () => { await provider.resolveWebviewView(mockWebviewView) await provider.contextProxy.setValue("alwaysDenyUnapprovedCommands", true) diff --git a/webview-ui/src/components/settings/CheckpointSettings.tsx b/webview-ui/src/components/settings/CheckpointSettings.tsx index dd28f6615f..e6c8558f16 100644 --- a/webview-ui/src/components/settings/CheckpointSettings.tsx +++ b/webview-ui/src/components/settings/CheckpointSettings.tsx @@ -13,17 +13,26 @@ import { DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, MAX_CHECKPOINT_TIMEOUT_SECONDS, MIN_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_PER_WRITE_CHECKPOINTS, + DEFAULT_CHANGE_CARD_DETAIL, + type ChangeCardDetail, } from "@roo-code/types" type CheckpointSettingsProps = HTMLAttributes & { enableCheckpoints?: boolean checkpointTimeout?: number - setCachedStateField: SetCachedStateField<"enableCheckpoints" | "checkpointTimeout"> + perWriteCheckpoints?: boolean + changeCardDetail?: ChangeCardDetail + setCachedStateField: SetCachedStateField< + "enableCheckpoints" | "checkpointTimeout" | "perWriteCheckpoints" | "changeCardDetail" + > } export const CheckpointSettings = ({ enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, + changeCardDetail, setCachedStateField, ...props }: CheckpointSettingsProps) => { @@ -33,6 +42,39 @@ export const CheckpointSettings = ({ {t("settings:sections.checkpoints")}
+ + { + setCachedStateField("perWriteCheckpoints", e.target.checked) + }}> + {t("settings:checkpoints.perWrite.label")} + +
+ {t("settings:checkpoints.perWrite.description")} +
+ + + + { + setCachedStateField("changeCardDetail", e.target.checked ? "full" : "summary") + }} + data-testid="change-card-detail-checkbox"> + {t("settings:checkpoints.changeCardDetail.label")} + +
+ {t("settings:checkpoints.changeCardDetail.description")} +
+ + (({ onDone, t autoCondenseContextPercent, enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, + changeCardDetail, experiments, maxOpenTabsContext, maxWorkspaceFiles, @@ -412,6 +416,8 @@ const SettingsView = forwardRef(({ onDone, t ttsSpeed, enableCheckpoints: enableCheckpoints ?? false, checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, + changeCardDetail: changeCardDetail ?? DEFAULT_CHANGE_CARD_DETAIL, writeDelayMs, diffFuzzyThreshold, terminalShellIntegrationTimeout: terminalShellIntegrationTimeout ?? 30_000, @@ -850,6 +856,8 @@ const SettingsView = forwardRef(({ onDone, t )} diff --git a/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx b/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx new file mode 100644 index 0000000000..f60291d2e4 --- /dev/null +++ b/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx @@ -0,0 +1,232 @@ +// npx vitest src/components/settings/__tests__/CheckpointSettings.spec.tsx + +import type { CSSProperties, ReactNode } from "react" +import { render, screen, fireEvent } from "@/utils/test-utils" +import { CheckpointSettings } from "../CheckpointSettings" + +// Mock the translation hook +vi.mock("@/i18n/TranslationContext", () => ({ + useAppTranslation: () => ({ + t: (key: string) => { + if (key === "settings:checkpoints.perWrite.label") { + return "Checkpoint after each file write" + } + if (key === "settings:checkpoints.perWrite.description") { + return "Record a checkpoint snapshot after every successful file write by the agent" + } + if (key === "settings:checkpoints.changeCardDetail.label") { + return "Show full diff in change cards" + } + if (key === "settings:checkpoints.changeCardDetail.description") { + return "Include the full unified diff inline for every file in per-step change cards" + } + return key + }, + }), +})) + +// Mock the UI components (async factory: vi.importActual resolves asynchronously). +vi.mock("@/components/ui", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + // Narrow typed double: only the props CheckpointSettings consumes, so + // drift in the Slider contract is a compile error here, not `any`. + Slider: ({ + defaultValue, + onValueChange, + "data-testid": dataTestId, + }: { + defaultValue?: number[] + onValueChange?: (value: number[]) => void + "data-testid"?: string + }) => ( + onValueChange?.([100])} + data-testid={dataTestId} + role="slider" + /> + ), + } +}) + +// Mock vscode utilities +vi.mock("@/utils/vscode", () => ({ + vscode: { + postMessage: vi.fn(), + }, +})) + +// Mock VSCode components to behave like standard HTML elements +vi.mock("@vscode/webview-ui-toolkit/react", () => { + // Narrow event double: the real toolkit dispatches a native Event whose + // currentTarget is the web component with a boolean `checked`; the mock + // forwards the input's checked state on both target and currentTarget so + // handlers can be typed against either surface. + type CheckboxChangeEvent = { + target: { checked: boolean } + currentTarget: { checked: boolean } + } + return { + VSCodeCheckbox: ({ + checked, + onChange, + children, + "data-testid": dataTestId, + }: { + checked?: boolean + onChange?: (e: CheckboxChangeEvent) => void + children?: ReactNode + "data-testid"?: string + }) => ( + + ), + VSCodeLink: ({ children, href, style }: { children?: ReactNode; href?: string; style?: CSSProperties }) => ( + + {children} + + ), + } +}) + +describe("CheckpointSettings", () => { + const setCachedStateField = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + }) + + it("renders the per-write checkpoints checkbox checked by default when the value is unset", () => { + render() + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + expect(checkbox).toBeChecked() + }) + + it("unchecks the per-write checkpoints checkbox when the saved value is false", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + expect(checkbox).not.toBeChecked() + }) + + it("keeps the per-write checkpoints checkbox checked when the saved value is true", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + expect(checkbox).toBeChecked() + }) + + it("caches a toggle to enable per-write checkpoints when the user checks the box", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + fireEvent.click(checkbox) + + expect(setCachedStateField).toHaveBeenCalledWith("perWriteCheckpoints", true) + }) + + it("caches a toggle to disable per-write checkpoints when the user unchecks the box", () => { + render() + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + fireEvent.click(checkbox) + + expect(setCachedStateField).toHaveBeenCalledWith("perWriteCheckpoints", false) + }) + + it("renders the change card detail checkbox unchecked by default when the value is unset", () => { + render() + + const checkbox = screen.getByRole("checkbox", { name: "Show full diff in change cards" }) + expect(checkbox).not.toBeChecked() + }) + + it("renders the change card detail checkbox checked when the saved value is full", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Show full diff in change cards" }) + expect(checkbox).toBeChecked() + }) + + it("renders the change card detail checkbox unchecked when the saved value is summary", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Show full diff in change cards" }) + expect(checkbox).not.toBeChecked() + }) + + it("caches the changeCardDetail full value when the user checks the box", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Show full diff in change cards" }) + fireEvent.click(checkbox) + + expect(setCachedStateField).toHaveBeenCalledWith("changeCardDetail", "full") + }) + + it("caches the changeCardDetail summary value when the user unchecks the box", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Show full diff in change cards" }) + fireEvent.click(checkbox) + + expect(setCachedStateField).toHaveBeenCalledWith("changeCardDetail", "summary") + }) +}) diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 97b390f6f4..0f46190bb0 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -2,6 +2,7 @@ import { renderWithExtensionState, screen, fireEvent, within, waitFor } from "@/utils/test-utils" import { act } from "@testing-library/react" +import userEvent from "@testing-library/user-event" import { vscode } from "@/utils/vscode" import { DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, type ProviderSettings } from "@roo-code/types" @@ -487,6 +488,78 @@ describe("SettingsView - Sound Settings", () => { ) }) + it("saves the changeCardDetail full selection on save", async () => { + const { activateTab, getSettingsContent } = renderSettingsView({ + settingsImportedAt: new Date().toISOString(), + }) + + activateTab("checkpoints") + const content = getSettingsContent() + const checkbox = await within(content).findByTestId("change-card-detail-checkbox") + expect(checkbox).not.toBeChecked() + + const user = userEvent.setup() + await user.click(checkbox) + await user.click(screen.getByTestId("save-button")) + + await waitFor(() => + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ changeCardDetail: "full" }), + }), + ), + ) + }) + + it("saves the changeCardDetail summary default when the value is unset", async () => { + const { activateTab, getSettingsContent } = renderSettingsView({ + settingsImportedAt: new Date().toISOString(), + }) + + activateTab("checkpoints") + const content = getSettingsContent() + const checkbox = await within(content).findByTestId("change-card-detail-checkbox") + expect(checkbox).not.toBeChecked() + + const user = userEvent.setup() + await user.click(screen.getByTestId("save-button")) + + await waitFor(() => + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ changeCardDetail: "summary" }), + }), + ), + ) + }) + + it("reflects the saved changeCardDetail full value in the checkbox and saves summary when unchecked", async () => { + const { activateTab, getSettingsContent } = renderSettingsView({ + changeCardDetail: "full", + settingsImportedAt: new Date().toISOString(), + }) + + activateTab("checkpoints") + const content = getSettingsContent() + const checkbox = await within(content).findByTestId("change-card-detail-checkbox") + await waitFor(() => expect(checkbox).toBeChecked()) + + const user = userEvent.setup() + await user.click(checkbox) + await user.click(screen.getByTestId("save-button")) + + await waitFor(() => + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ changeCardDetail: "summary" }), + }), + ), + ) + }) + it("shows tts slider when sound is enabled", () => { // Render once and get the activateTab helper const { activateTab, getSettingsContent } = renderSettingsView() diff --git a/webview-ui/src/context/ExtensionStateContext.tsx b/webview-ui/src/context/ExtensionStateContext.tsx index c4fa00f479..daf0bd8f6f 100644 --- a/webview-ui/src/context/ExtensionStateContext.tsx +++ b/webview-ui/src/context/ExtensionStateContext.tsx @@ -213,6 +213,8 @@ export const createInitialExtensionState = (): ExtensionState => ({ ttsEnabled: false, ttsSpeed: 1.0, enableCheckpoints: true, + perWriteCheckpoints: true, + changeCardDetail: "summary", checkpointTimeout: DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, // Default to 15 seconds language: "en", // Default language code // Placeholder before the extension state hydrates: use the shared default diff --git a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx index dc33af38d1..a8576d9268 100644 --- a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx +++ b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx @@ -352,6 +352,17 @@ describe("ExtensionStateContext", () => { } }) + it("initializes the change-card defaults before hydration", () => { + // The initializer itself (not a merge fixture) must carry the change-card + // defaults: a regression that dropped either key from + // createInitialExtensionState would otherwise stay hidden because the + // merge tests supply the keys manually. + const state = createInitialExtensionState() + + expect(state.changeCardDetail).toBe("summary") + expect(state.perWriteCheckpoints).toBe(true) + }) + it("updates apiConfiguration through setApiConfiguration", () => { render( @@ -426,6 +437,8 @@ describe("mergeExtensionState", () => { taskHistory: [], shouldShowAnnouncement: false, enableCheckpoints: true, + perWriteCheckpoints: true, + changeCardDetail: "summary", // Matches the shared DEFAULT_WRITE_DELAY_MS (pre-hydration placeholder // must not disagree with the extension's own fallback). writeDelayMs: 0, @@ -458,12 +471,16 @@ describe("mergeExtensionState", () => { const prevState: ExtensionState = { ...baseState, + // Non-default checkpoint keys so a merge regression that drops or + // resets them cannot hide behind the initial defaults. + perWriteCheckpoints: false, + changeCardDetail: "full", apiConfiguration: { modelMaxTokens: 1234, modelMaxThinkingTokens: 123 }, experiments: {} as Record, checkpointTimeout: DEFAULT_CHECKPOINT_TIMEOUT_SECONDS - 5, } - const newState: ExtensionState = { + const newState: Partial = { ...baseState, apiConfiguration: { modelMaxThinkingTokens: 456, modelTemperature: 0.3 }, experiments: { @@ -475,6 +492,11 @@ describe("mergeExtensionState", () => { checkpointTimeout: DEFAULT_CHECKPOINT_TIMEOUT_SECONDS + 5, } + // A partial state push may omit the checkpoint keys entirely; the + // merge must preserve the previous non-default values. + delete newState.perWriteCheckpoints + delete newState.changeCardDetail + const result = mergeExtensionState(prevState, newState) expect(result.apiConfiguration).toEqual({ @@ -488,6 +510,11 @@ describe("mergeExtensionState", () => { runSlashCommand: false, customTools: false, }) + + // A partial push that omits the checkpoint keys must keep the previous + // non-default values. + expect(result.perWriteCheckpoints).toBe(false) + expect(result.changeCardDetail).toBe("full") }) describe("clineMessagesSeq protection", () => { @@ -498,6 +525,8 @@ describe("mergeExtensionState", () => { taskHistory: [], shouldShowAnnouncement: false, enableCheckpoints: true, + perWriteCheckpoints: true, + changeCardDetail: "summary", // Matches the shared DEFAULT_WRITE_DELAY_MS (pre-hydration placeholder // must not disagree with the extension's own fallback). writeDelayMs: 0, diff --git a/webview-ui/src/i18n/locales/ca/settings.json b/webview-ui/src/i18n/locales/ca/settings.json index f599e40db9..d11a62dd22 100644 --- a/webview-ui/src/i18n/locales/ca/settings.json +++ b/webview-ui/src/i18n/locales/ca/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Habilitar punts de control automàtics", "description": "Quan està habilitat, Zoo crearà automàticament punts de control durant l'execució de tasques, facilitant la revisió de canvis o la reversió a estats anteriors. <0>Més informació" + }, + "perWrite": { + "label": "Punt de control després de cada escriptura de fitxer", + "description": "Registra una instantània de punt de control després de cada escriptura de fitxer reeixida de l’agent" + }, + "changeCardDetail": { + "label": "Mostra la diff completa a les targetes de canvis", + "description": "Inclou la diff unificada completa en línia per a cada fitxer a les targetes de canvis per pas. Desactivada, les targetes només mostren la llista de fitxers amb les línies afegides/eliminades." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/de/settings.json b/webview-ui/src/i18n/locales/de/settings.json index 845c9e575f..a654b5a187 100644 --- a/webview-ui/src/i18n/locales/de/settings.json +++ b/webview-ui/src/i18n/locales/de/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Automatische Kontrollpunkte aktivieren", "description": "Wenn aktiviert, erstellt Zoo automatisch Kontrollpunkte während der Aufgabenausführung, was die Überprüfung von Änderungen oder die Rückkehr zu früheren Zuständen erleichtert. <0>Mehr erfahren" + }, + "perWrite": { + "label": "Kontrollpunkt nach jedem Dateischreibvorgang", + "description": "Ein Kontrollpunkt-Snapshot wird nach jedem erfolgreichen Dateischreibvorgang des Agents erfasst" + }, + "changeCardDetail": { + "label": "Volle Diff in Änderungskarten anzeigen", + "description": "Enthält die vollständige Unified-Diff inline für jede Datei in den Änderungskarten pro Schritt. Deaktiviert zeigen die Karten nur die Dateiliste mit hinzugefügten/entfernten Zeilen." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/en/settings.json b/webview-ui/src/i18n/locales/en/settings.json index de36f7ac3a..152ce35de2 100644 --- a/webview-ui/src/i18n/locales/en/settings.json +++ b/webview-ui/src/i18n/locales/en/settings.json @@ -798,6 +798,14 @@ "enable": { "label": "Enable automatic checkpoints", "description": "When enabled, Zoo will automatically create checkpoints during task execution, making it easy to review changes or revert to earlier states. <0>Learn more" + }, + "perWrite": { + "label": "Checkpoint after each file write", + "description": "Record a checkpoint snapshot after every successful file write by the agent" + }, + "changeCardDetail": { + "label": "Show full diff in change cards", + "description": "Include the full unified diff inline for every file in per-step change cards. When off, cards show only the file list with added/removed line counts." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/es/settings.json b/webview-ui/src/i18n/locales/es/settings.json index 1e6f87d553..0826809e87 100644 --- a/webview-ui/src/i18n/locales/es/settings.json +++ b/webview-ui/src/i18n/locales/es/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Habilitar puntos de control automáticos", "description": "Cuando está habilitado, Zoo creará automáticamente puntos de control durante la ejecución de tareas, facilitando la revisión de cambios o la reversión a estados anteriores. <0>Más información" + }, + "perWrite": { + "label": "Punto de control después de cada escritura de archivo", + "description": "Registra una instantánea de punto de control después de cada escritura de archivo exitosa del agente" + }, + "changeCardDetail": { + "label": "Mostrar la diff completa en las tarjetas de cambios", + "description": "Incluye la diff unificada completa en línea para cada archivo en las tarjetas de cambios por paso. Al desactivarla, las tarjetas muestran solo la lista de archivos con las líneas añadidas/eliminadas." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/fr/settings.json b/webview-ui/src/i18n/locales/fr/settings.json index ec6ee0f692..3b44074763 100644 --- a/webview-ui/src/i18n/locales/fr/settings.json +++ b/webview-ui/src/i18n/locales/fr/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Activer les points de contrôle automatiques", "description": "Lorsque cette option est activée, Zoo créera automatiquement des points de contrôle pendant l'exécution des tâches, facilitant la révision des modifications ou le retour à des états antérieurs. <0>En savoir plus" + }, + "perWrite": { + "label": "Point de contrôle après chaque écriture de fichier", + "description": "Enregistre un instantané de point de contrôle après chaque écriture de fichier réussie par l’agent" + }, + "changeCardDetail": { + "label": "Afficher la diff complète dans les cartes de modifications", + "description": "Inclut la diff unifiée complète en ligne pour chaque fichier dans les cartes de modifications par étape. Désactivée, les cartes n'affichent que la liste des fichiers avec les lignes ajoutées/retirées." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/hi/settings.json b/webview-ui/src/i18n/locales/hi/settings.json index fa725cd2c5..af4aacbffd 100644 --- a/webview-ui/src/i18n/locales/hi/settings.json +++ b/webview-ui/src/i18n/locales/hi/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "स्वचालित चेकपॉइंट सक्षम करें", "description": "जब सक्षम होता है, तो Zoo कार्य निष्पादन के दौरान स्वचालित रूप से चेकपॉइंट बनाएगा, जिससे परिवर्तनों की समीक्षा करना या पहले की स्थितियों पर वापस जाना आसान हो जाएगा। <0>अधिक जानें" + }, + "perWrite": { + "label": "हर फ़ाइल लिखने के बाद चेकपॉइंट", + "description": "एजेंट द्वारा हर सफल फ़ाइल लिखने के बाद एक चेकपॉइंट स्नैपशॉट दर्ज किया जाता है" + }, + "changeCardDetail": { + "label": "बदलाव कार्डों में पूर्ण diff दिखाएँ", + "description": "प्रति-चरण बदलाव कार्डों में हर फ़ाइल के लिए पूर्ण unified diff इनलाइन शामिल करता है। बंद होने पर कार्ड केवल जोड़ी/हटाई गई पंक्तियों के साथ फ़ाइल सूची दिखाते हैं।" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/id/settings.json b/webview-ui/src/i18n/locales/id/settings.json index bd3e0fcc96..c3beef17fb 100644 --- a/webview-ui/src/i18n/locales/id/settings.json +++ b/webview-ui/src/i18n/locales/id/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Aktifkan checkpoint otomatis", "description": "Ketika diaktifkan, Zoo akan secara otomatis membuat checkpoint selama eksekusi tugas, memudahkan untuk meninjau perubahan atau kembali ke state sebelumnya. <0>Pelajari lebih lanjut" + }, + "perWrite": { + "label": "Checkpoint setelah setiap penulisan file", + "description": "Merekam snapshot checkpoint setelah setiap penulisan file yang berhasil oleh agen" + }, + "changeCardDetail": { + "label": "Tampilkan diff lengkap di kartu perubahan", + "description": "Termasuk diff terpadu lengkap secara inline untuk setiap file di kartu perubahan per langkah. Saat dimatikan, kartu hanya menampilkan daftar file dengan baris yang ditambahkan/dihapus." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/it/settings.json b/webview-ui/src/i18n/locales/it/settings.json index 2b604e720f..0822d92f6a 100644 --- a/webview-ui/src/i18n/locales/it/settings.json +++ b/webview-ui/src/i18n/locales/it/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Abilita punti di controllo automatici", "description": "Quando abilitato, Zoo creerà automaticamente punti di controllo durante l'esecuzione dei compiti, facilitando la revisione delle modifiche o il ritorno a stati precedenti. <0>Scopri di più" + }, + "perWrite": { + "label": "Punto di controllo dopo ogni scrittura del file", + "description": "Registra uno snapshot di punto di controllo dopo ogni scrittura del file riuscita dell’agente" + }, + "changeCardDetail": { + "label": "Mostra la diff completa nelle card dei cambiamenti", + "description": "Include la diff unificata completa in linea per ogni file nelle card dei cambiamenti per passo. Se disattivata, le card mostrano solo l'elenco dei file con le righe aggiunte/rimosse." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ja/settings.json b/webview-ui/src/i18n/locales/ja/settings.json index 2ca3ef0396..e90517a7ad 100644 --- a/webview-ui/src/i18n/locales/ja/settings.json +++ b/webview-ui/src/i18n/locales/ja/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "自動チェックポイントを有効化", "description": "有効にすると、Zooはタスク実行中に自動的にチェックポイントを作成し、変更の確認や以前の状態への復帰を容易にします。 <0>詳細情報" + }, + "perWrite": { + "label": "ファイルの書き込みごとにチェックポイント", + "description": "エージェントによる各ファイルの書き込み成功後にチェックポイントのスナップショットを記録します" + }, + "changeCardDetail": { + "label": "変更カードに完全な diff を表示", + "description": "ステップごとの変更カードに各ファイルの完全な unified diff をインラインで含めます。オフにすると、カードは追加/削除行数付きのファイル一覧のみを表示します。" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ko/settings.json b/webview-ui/src/i18n/locales/ko/settings.json index 89dcd74c40..17f0e34f22 100644 --- a/webview-ui/src/i18n/locales/ko/settings.json +++ b/webview-ui/src/i18n/locales/ko/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "자동 체크포인트 활성화", "description": "활성화되면 Zoo는 작업 실행 중에 자동으로 체크포인트를 생성하여 변경 사항을 검토하거나 이전 상태로 되돌리기 쉽게 합니다. <0>더 알아보기" + }, + "perWrite": { + "label": "파일을 쓸 때마다 체크포인트", + "description": "에이전트가 파일 쓰기에 성공할 때마다 체크포인트 스냅샷을 기록합니다" + }, + "changeCardDetail": { + "label": "변경 카드에 전체 diff 표시", + "description": "단계별 변경 카드에 각 파일의 전체 unified diff를 인라인으로 포함합니다. 끄면 카드는 추가/제거 줄 수만 있는 파일 목록만 표시합니다." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/nl/settings.json b/webview-ui/src/i18n/locales/nl/settings.json index 2eeea89e92..e6b7f07e71 100644 --- a/webview-ui/src/i18n/locales/nl/settings.json +++ b/webview-ui/src/i18n/locales/nl/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Automatische checkpoints inschakelen", "description": "Indien ingeschakeld, maakt Zoo automatisch checkpoints tijdens het uitvoeren van taken, zodat je eenvoudig wijzigingen kunt bekijken of terugzetten. <0>Meer informatie" + }, + "perWrite": { + "label": "Checkpoint na elke bestandsschrijving", + "description": "Neemt een checkpoint-snapshot op na elke succesvolle bestandsschrijving door de agent" + }, + "changeCardDetail": { + "label": "Volledige diff tonen in wijzigingskaarten", + "description": "Bevat de volledige unified diff inline voor elk bestand in wijzigingskaarten per stap. Wanneer deze optie is uitgeschakeld, tonen de kaarten alleen de bestandslijst met toegevoegde/verwijderde regels." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/pl/settings.json b/webview-ui/src/i18n/locales/pl/settings.json index 0de3a36c4e..1d8aecc3a8 100644 --- a/webview-ui/src/i18n/locales/pl/settings.json +++ b/webview-ui/src/i18n/locales/pl/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Włącz automatyczne punkty kontrolne", "description": "Gdy włączone, Zoo automatycznie utworzy punkty kontrolne podczas wykonywania zadań, ułatwiając przeglądanie zmian lub powrót do wcześniejszych stanów. <0>Dowiedz się więcej" + }, + "perWrite": { + "label": "Punkt kontrolny po każdym zapisaniu pliku", + "description": "Rejestruje migawkę punktu kontrolnego po każdym udanym zapisaniu pliku przez agenta" + }, + "changeCardDetail": { + "label": "Pokaż pełny diff w kartach zmian", + "description": "Zawiera pełny spójny diff inline dla każdego pliku w kartach zmian per krok. Po wyłączeniu karty pokazują tylko listę plików z dodanymi/usuniętymi liniami." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/pt-BR/settings.json b/webview-ui/src/i18n/locales/pt-BR/settings.json index 893504febe..2e350c42a9 100644 --- a/webview-ui/src/i18n/locales/pt-BR/settings.json +++ b/webview-ui/src/i18n/locales/pt-BR/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Ativar pontos de verificação automáticos", "description": "Quando ativado, o Zoo criará automaticamente pontos de verificação durante a execução de tarefas, facilitando a revisão de alterações ou o retorno a estados anteriores. <0>Saiba mais" + }, + "perWrite": { + "label": "Ponto de verificação após cada gravação de arquivo", + "description": "Registra um snapshot de ponto de verificação após cada gravação de arquivo bem-sucedida pelo agente" + }, + "changeCardDetail": { + "label": "Mostrar diff completo nos cartões de alterações", + "description": "Inclui a diff unificada completa em linha para cada arquivo nos cartões de alterações por etapa. Quando desativado, os cartões mostram apenas a lista de arquivos com linhas adicionadas/removidas." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ru/settings.json b/webview-ui/src/i18n/locales/ru/settings.json index 3d55ee12d3..300b52f250 100644 --- a/webview-ui/src/i18n/locales/ru/settings.json +++ b/webview-ui/src/i18n/locales/ru/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Включить автоматические контрольные точки", "description": "Если включено, Zoo будет автоматически создавать контрольные точки во время выполнения задач, что упрощает просмотр изменений или возврат к предыдущим состояниям. <0>Подробнее" + }, + "perWrite": { + "label": "Контрольная точка после каждой записи файла", + "description": "Записывает снимок контрольной точки после каждой успешной записи файла агентом" + }, + "changeCardDetail": { + "label": "Показывать полный diff в карточках изменений", + "description": "Включает полную unified diff для каждого файла в карточках изменений по шагам. При выключенном показе карточки отображают только список файлов с количеством добавленных/удалённых строк." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/tr/settings.json b/webview-ui/src/i18n/locales/tr/settings.json index 5f93ec8035..03ac7fa9ad 100644 --- a/webview-ui/src/i18n/locales/tr/settings.json +++ b/webview-ui/src/i18n/locales/tr/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Otomatik kontrol noktalarını etkinleştir", "description": "Etkinleştirildiğinde, Zoo görev yürütme sırasında otomatik olarak kontrol noktaları oluşturarak değişiklikleri gözden geçirmeyi veya önceki durumlara dönmeyi kolaylaştırır. <0>Daha fazla bilgi" + }, + "perWrite": { + "label": "Her dosya yazımından sonra kontrol noktası", + "description": "Ajanın her başarılı dosya yazımından sonra bir kontrol noktası görüntüsü kaydeder" + }, + "changeCardDetail": { + "label": "Değişiklik kartlarında tam diff'i göster", + "description": "Adım başına değişiklik kartlarında her dosya için tam birleşik diff'i satır içi olarak içerir. Kapalıyken kartlar yalnızca eklenen/çıkarılan satır sayılarıyla birlikte dosya listesini gösterir." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/vi/settings.json b/webview-ui/src/i18n/locales/vi/settings.json index 5ec6e8c105..251e159980 100644 --- a/webview-ui/src/i18n/locales/vi/settings.json +++ b/webview-ui/src/i18n/locales/vi/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "Bật điểm kiểm tra tự động", "description": "Khi được bật, Zoo sẽ tự động tạo các điểm kiểm tra trong quá trình thực hiện nhiệm vụ, giúp dễ dàng xem lại các thay đổi hoặc quay lại trạng thái trước đó. <0>Tìm hiểu thêm" + }, + "perWrite": { + "label": "Điểm kiểm tra sau mỗi lần ghi file", + "description": "Ghi lại ảnh chụp nhanh điểm kiểm tra sau mỗi lần ghi file thành công của agent" + }, + "changeCardDetail": { + "label": "Hiển thị diff đầy đủ trong thẻ thay đổi", + "description": "Bao gồm diff thống nhất đầy đủ nội tuyến cho từng tệp trong thẻ thay đổi theo bước. Khi tắt, thẻ chỉ hiển thị danh sách tệp với số dòng thêm/xóa." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/zh-CN/settings.json b/webview-ui/src/i18n/locales/zh-CN/settings.json index 9e0a94d908..829513a12a 100644 --- a/webview-ui/src/i18n/locales/zh-CN/settings.json +++ b/webview-ui/src/i18n/locales/zh-CN/settings.json @@ -718,6 +718,14 @@ "enable": { "label": "启用自动存档点", "description": "开启后自动创建任务存档点，方便回溯修改。 <0>了解更多" + }, + "perWrite": { + "label": "每次文件写入后创建存档点", + "description": "智能体每次成功写入文件后都会记录一个存档点快照" + }, + "changeCardDetail": { + "label": "在变更卡片中显示完整 diff", + "description": "在逐步变更卡片中为每个文件内嵌完整 unified diff。关闭时，卡片仅显示文件列表与新增/删除行数。" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/zh-TW/settings.json b/webview-ui/src/i18n/locales/zh-TW/settings.json index c2fd65c755..67f1aa64fa 100644 --- a/webview-ui/src/i18n/locales/zh-TW/settings.json +++ b/webview-ui/src/i18n/locales/zh-TW/settings.json @@ -745,6 +745,14 @@ "enable": { "label": "啟用自動檢查點", "description": "啟用後，Zoo 將在工作執行期間自動建立檢查點，方便檢視變更或回到較早的狀態。 <0>了解更多" + }, + "perWrite": { + "label": "每次檔案寫入後建立檢查點", + "description": "代理每次成功寫入檔案後都會記錄一個檢查點快照" + }, + "changeCardDetail": { + "label": "在變更卡片中顯示完整 diff", + "description": "在逐步變更卡片中為每個檔案內嵌完整 unified diff。關閉時，卡片僅顯示檔案清單與新增/刪除行數。" } }, "notifications": {