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/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..e5ac58aed9 --- /dev/null +++ b/src/core/checkpoints/__tests__/changeJournal.spec.ts @@ -0,0 +1,466 @@ +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("propagates non-ENOENT read failures instead of reporting an empty journal", async () => { + // A directory at the journal path makes readFile fail with EISDIR — + // a stand-in for any permission or I/O failure (EACCES etc.). Such a + // failure must not be swallowed into "no changes": it would let a + // rollback report a no-op success without reading the history. + await fs.mkdir(journalPath(tmpRoot, taskId), { recursive: true }) + + await expect(loadChanges(tmpRoot, taskId)).rejects.toMatchObject({ code: "EISDIR" }) + }) + + 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/__tests__/rollback.spec.ts b/src/core/checkpoints/__tests__/rollback.spec.ts new file mode 100644 index 0000000000..acfc6c3ec9 --- /dev/null +++ b/src/core/checkpoints/__tests__/rollback.spec.ts @@ -0,0 +1,487 @@ +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 type { ChangeJournalEntry } from "../changeJournal" +import { getCheckpointService } from "../index" +import * as changeJournal from "../changeJournal" +import { appendChange, journalPath } from "../changeJournal" +import { restoreLatestFile, rollbackFile, rollbackStep } from "../rollback" + +vi.mock("../index", () => ({ + getCheckpointService: vi.fn(), + checkpointSave: vi.fn(), + checkpointRestore: vi.fn(), + checkpointDiff: vi.fn(), +})) + +const mockedGetCheckpointService = getCheckpointService as unknown as ReturnType + +function makeTask(): Task { + return { + taskId: "task-rollback", + providerRef: { + deref: vi.fn().mockReturnValue({ context: { globalStorageUri: { fsPath: globalStorageDir } } }), + }, + } as unknown as Task +} + +/** A checkpoint-service double with a recording restoreFile and a baseline. */ +function serviceWith(baseHash: string | undefined) { + const restoreFile = vi.fn().mockResolvedValue(undefined) + return { baseHash, restoreFile } +} + +async function seedJournal(entries: ChangeJournalEntry[]): Promise { + for (const entry of entries) { + await appendChange(globalStorageDir, "task-rollback", entry) + } +} + +let globalStorageDir: string + +beforeEach(async () => { + globalStorageDir = await fs.mkdtemp(path.join(os.tmpdir(), "b3c-rollback-")) + mockedGetCheckpointService.mockReset() +}) + +afterEach(async () => { + await fs.rm(globalStorageDir, { recursive: true, force: true }) +}) + +describe("rollbackFile (B3c: undo the step's write to the file)", () => { + it("restores the file to the PREVIOUS step's checkpoint when an earlier entry exists", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-2", "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: true }) + // The pre-step state is the previous step's post-write checkpoint — not + // the step's own (post-write) checkpoint. + expect(service.restoreFile).toHaveBeenCalledTimes(1) + expect(service.restoreFile).toHaveBeenCalledWith("sha-1", "src/a.ts") + }) + + it("restores from the task-start baseline when the file has no earlier entry", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: true }) + expect(service.restoreFile).toHaveBeenCalledWith("base-0", "src/a.ts") + }) + + it("resolves through the first entry of a multi-write step", async () => { + // One patch writes the same file twice: two entries share the step + // checkpoint. The pre-step state is still the entry before the first + // one of the step. + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-2", "src/a.ts") + + expect(outcome.success).toBe(true) + expect(service.restoreFile).toHaveBeenCalledWith("sha-1", "src/a.ts") + }) + + it("rejects rolling back a step that is not the file's latest change", async () => { + // The file was written again by a later step (sha-2): rolling back the + // older step (sha-1) would overwrite the newer state, so it is + // rejected instead of silently destroying it. + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "File was modified in a later step; roll back the latest change card first", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails when the journal location is unavailable (no global storage)", async () => { + // No context on the provider double → the journal cannot even be + // located: a clear failure, not a silent miss on the file lookup. + const task = { + taskId: "task-rollback", + providerRef: { deref: vi.fn().mockReturnValue(undefined) }, + } as unknown as Task + + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(task, "sha-1", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "Change journal is unavailable for this task", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails cleanly when the file is not part of the given step checkpoint", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-2", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "File is not part of this step's checkpoint", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails cleanly when no earlier checkpoint exists and the baseline is unavailable", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith(undefined) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "No checkpoint available to restore", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails cleanly when checkpoints are not enabled", async () => { + mockedGetCheckpointService.mockResolvedValue(undefined) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "Checkpoints are not enabled for this task", + }) + }) + + it("reports the service error without throwing", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + service.restoreFile.mockRejectedValue(new Error("pathspec did not match")) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome.success).toBe(false) + expect(outcome.error).toContain("pathspec did not match") + }) + + it("stringifies non-Error rejections into the outcome", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + service.restoreFile.mockRejectedValue("raw failure") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: false, error: "raw failure" }) + }) +}) + +describe("rollbackStep (B3c: undo every file of the step)", () => { + it("restores every step file to its pre-step state", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + { path: "src/b.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/b.ts"], "sha-2") + + expect(outcome.checkpointId).toBe("sha-2") + expect(outcome.files).toEqual([ + { filePath: "src/a.ts", success: true }, + { filePath: "src/b.ts", success: true }, + ]) + expect(service.restoreFile).toHaveBeenCalledTimes(2) + expect(service.restoreFile).toHaveBeenNthCalledWith(1, "sha-1", "src/a.ts") + // src/b.ts has no earlier entry: its pre-step state is the baseline. + expect(service.restoreFile).toHaveBeenNthCalledWith(2, "base-0", "src/b.ts") + }) + + it("keeps per-file failures isolated from the other step files", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + { path: "src/b.ts", operation: "create", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + service.restoreFile.mockRejectedValueOnce(new Error("pathspec did not match")) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/b.ts"], "sha-2") + + expect(outcome.files[0].success).toBe(false) + expect(outcome.files[0].error).toContain("pathspec did not match") + expect(outcome.files[1]).toEqual({ filePath: "src/b.ts", success: true }) + }) + + it("fails the file cleanly when no earlier checkpoint exists and the baseline is unavailable", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-2" }]) + const service = serviceWith(undefined) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts"], "sha-2") + + expect(outcome.checkpointId).toBe("sha-2") + expect(outcome.files).toEqual([ + { filePath: "src/a.ts", success: false, error: "No checkpoint available to restore" }, + ]) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("rejects a file that is not part of the given step checkpoint", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-2" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/other.ts"], "sha-2") + + expect(outcome.files[0]).toEqual({ filePath: "src/a.ts", success: true }) + expect(outcome.files[1].success).toBe(false) + expect(outcome.files[1].error).toBe("File is not part of this step's checkpoint") + expect(service.restoreFile).toHaveBeenCalledTimes(1) + }) + + it("rejects the stale file of a step while restoring the others", async () => { + // src/a.ts was written again after this step's checkpoint, so its + // sha-2 entry is no longer the file's latest: only src/b.ts (whose + // latest entry IS sha-2) is restored. + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + { path: "src/b.ts", operation: "create", checkpointId: "sha-2" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-3" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/b.ts"], "sha-2") + + expect(outcome.files[0].success).toBe(false) + expect(outcome.files[0].error).toBe("File was modified in a later step; roll back the latest change card first") + expect(outcome.files[1]).toEqual({ filePath: "src/b.ts", success: true }) + expect(service.restoreFile).toHaveBeenCalledTimes(1) + expect(service.restoreFile).toHaveBeenCalledWith("base-0", "src/b.ts") + }) + + it("falls back to the latest journal entry per file without a step checkpoint id", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts"]) + + expect(outcome.checkpointId).toBeUndefined() + expect(outcome.files).toEqual([{ filePath: "src/a.ts", success: true }]) + expect(service.restoreFile).toHaveBeenCalledWith("sha-2", "src/a.ts") + }) + + it("fails listed files without journal entries", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/missing.ts"]) + + expect(outcome.files[0]).toEqual({ filePath: "src/a.ts", success: true }) + expect(outcome.files[1].success).toBe(false) + expect(outcome.files[1].error).toBe("No change journal entry for this file") + }) + + it("fails per file when the journal location is unavailable (no global storage)", async () => { + // No context on the provider double → the journal cannot even be + // located. That is a failure, not an empty journal: reporting the + // step as merely "not part of this checkpoint" would be misleading. + const task = { + taskId: "task-rollback", + providerRef: { deref: vi.fn().mockReturnValue(undefined) }, + } as unknown as Task + + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(task, ["src/a.ts"], "sha-2") + + expect(outcome.checkpointId).toBe("sha-2") + expect(outcome.files[0].success).toBe(false) + expect(outcome.files[0].error).toBe("Change journal is unavailable for this task") + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails every file when checkpoints are not enabled", async () => { + mockedGetCheckpointService.mockResolvedValue(undefined) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts"], "sha-2") + + expect(outcome.checkpointId).toBe("sha-2") + expect(outcome.files).toEqual([ + { filePath: "src/a.ts", success: false, error: "Checkpoints are not enabled for this task" }, + ]) + }) +}) + +describe("restoreLatestFile (B3c: forward direction)", () => { + it("restores the file to its most recent recorded write checkpoint", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/b.ts", operation: "update", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: true }) + expect(service.restoreFile).toHaveBeenCalledWith("sha-2", "src/a.ts") + }) + + it("is a successful no-op for a file the task never wrote", async () => { + await seedJournal([{ path: "src/b.ts", operation: "update", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: true, noOp: true }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails when the journal location is unavailable (no global storage)", async () => { + // An unavailable journal is not "the task wrote nothing": a no-op + // success would claim a restore that never happened. + const task = { + taskId: "task-rollback", + providerRef: { deref: vi.fn().mockReturnValue(undefined) }, + } as unknown as Task + + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(task, "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "Change journal is unavailable for this task", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails when the journal cannot be read (an I/O error is not an empty journal)", async () => { + // A directory at the journal path makes readFile fail with EISDIR — + // a stand-in for any permission or I/O failure (EACCES etc.). + await fs.mkdir(journalPath(globalStorageDir, "task-rollback"), { recursive: true }) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome.success).toBe(false) + expect(outcome.error).toContain("Change journal could not be read") + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("stringifies a non-Error journal read failure into the outcome", async () => { + vi.spyOn(changeJournal, "loadChanges").mockRejectedValueOnce("raw journal failure") + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome.success).toBe(false) + expect(outcome.error).toBe("Change journal could not be read: raw journal failure") + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails restore-latest on a corrupt final journal record without restoring", async () => { + // A committed write whose final journal record was truncated or + // corrupted must surface as an explicit failure: reporting a + // successful no-op would hide that rollback metadata is incomplete. + await seedJournal([{ path: "src/a.ts", operation: "update", checkpointId: "sha-1" }]) + await fs.appendFile(journalPath(globalStorageDir, "task-rollback"), '{"path":"src/a.ts","ope') + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome.success).toBe(false) + expect(outcome.error).toContain("Corrupt change journal entry at line 2") + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails rollbackFile and rollbackStep on a corrupt final journal record without restoring", async () => { + await seedJournal([{ path: "src/a.ts", operation: "update", checkpointId: "sha-1" }]) + await fs.appendFile(journalPath(globalStorageDir, "task-rollback"), '{"path":"src/a.ts","ope') + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const single = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + expect(single.success).toBe(false) + expect(single.error).toContain("Corrupt change journal entry at line 2") + + const step = await rollbackStep(makeTask(), ["src/a.ts"], "sha-1") + expect(step.files[0].success).toBe(false) + expect(step.files[0].error).toContain("Corrupt change journal entry at line 2") + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails cleanly when checkpoints are not enabled", async () => { + mockedGetCheckpointService.mockResolvedValue(undefined) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "Checkpoints are not enabled for this task", + }) + }) + + it("reports the service error without throwing", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + service.restoreFile.mockRejectedValue(new Error("index.lock held")) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome.success).toBe(false) + expect(outcome.error).toContain("index.lock") + }) +}) 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..7b8ec3e42a --- /dev/null +++ b/src/core/checkpoints/changeJournal.ts @@ -0,0 +1,285 @@ +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 [] — but only an ABSENT file. Any other read + * failure (permissions, I/O) is rethrown: a journal that cannot be read must + * not be indistinguishable from one that is legitimately empty. + */ +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 (error) { + // A missing journal is a legitimate empty history; any other read + // failure (permissions, I/O) must propagate. Swallowing it would let + // a rollback report a no-op success without reading the history. + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + return [] + } + throw error + } + + 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/checkpoints/rollback.ts b/src/core/checkpoints/rollback.ts new file mode 100644 index 0000000000..508789c957 --- /dev/null +++ b/src/core/checkpoints/rollback.ts @@ -0,0 +1,277 @@ +/** + * Per-file / per-step checkpoint rollback (B3c). + * + * "Rollback" means UNDOING the change-card step: every file the step touched + * is restored to the state it had BEFORE the step ran. The restore target is + * resolved from the B2 change journal (`changes.jsonl`), whose entries record + * — in write order — the checkpoint commit each successful write produced: + * + * - a file written by an earlier step resolves to that earlier entry's + * checkpoint (the file's post-write state after the previous step, i.e. its + * pre-step state); + * - a file no earlier step wrote resolves to the task-start baseline + * (`service.baseHash`): undoing the step that created a file removes it, + * and undoing the step that deleted one brings it back; + * - `restoreLatestFile` is the forward direction: it brings a file back to + * the content of its most recent recorded write (a successful no-op when + * the task never wrote the file). + * + * A file is only rolled back from the card of its most recent step: undoing + * an older step for a file that a later step wrote again would overwrite the + * newer state, so such a rollback is rejected (a full checkpoint restore + * still reaches any older state). A journal that cannot be located or read + * fails the restore instead of masquerading as "the task wrote nothing". + * + * Restores reuse the existing shadow-git service (`getCheckpointService` → + * `RepoPerTaskCheckpointService.restoreFile`, the same instance whose + * `restoreCheckpoint` the checkpoints UI uses) — nothing is forked. Only the + * named file's working-tree content is replaced; the shadow repo's HEAD and + * the checkpoint list are untouched (unlike a full `restoreCheckpoint`). + */ +import type { Task } from "../task/Task" + +import { getCheckpointService } from "./index" +import { loadChanges, type ChangeJournalEntry } from "./changeJournal" + +export interface RollbackFileOutcome { + filePath: string + success: boolean + error?: string + /** True when `restoreLatestFile` found no recorded write: the working tree was left as-is. */ + noOp?: boolean +} + +export interface RollbackStepOutcome { + /** The step checkpoint the files were resolved against, when provided. */ + checkpointId?: string + files: RollbackFileOutcome[] +} + +const NOT_ENABLED_ERROR = "Checkpoints are not enabled for this task" +const NO_TARGET_ERROR = "No checkpoint available to restore" +const NOT_IN_STEP_ERROR = "File is not part of this step's checkpoint" +const NO_ENTRY_ERROR = "No change journal entry for this file" +const NO_JOURNAL_ERROR = "Change journal is unavailable for this task" +const NOT_LATEST_ERROR = "File was modified in a later step; roll back the latest change card first" + +type CheckpointService = NonNullable>> + +/** A readable journal (possibly legitimately empty) or the reason it could not be loaded. */ +type LoadedJournal = { entries: ChangeJournalEntry[] } | { error: string } + +// `undefined` = the journal cannot be located (provider reference gone); read failures (permissions, I/O) propagate. +async function loadTaskEntries(task: Task): Promise { + const globalStorageDir = task.providerRef.deref()?.context.globalStorageUri.fsPath + + return globalStorageDir ? loadChanges(globalStorageDir, task.taskId) : undefined +} + +// Single discriminated result for callers: a readable journal (possibly empty) or a failure. +async function loadTaskJournal(task: Task): Promise { + try { + const entries = await loadTaskEntries(task) + + if (entries === undefined) { + return { error: NO_JOURNAL_ERROR } + } + + return { entries } + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + return { error: `Change journal could not be read: ${message}` } + } +} + +/** The most recent journal entry recorded for `filePath`, if any. */ +function latestEntry(entries: ChangeJournalEntry[], filePath: string): ChangeJournalEntry | undefined { + for (let i = entries.length - 1; i >= 0; i--) { + if (entries[i].path === filePath) { + return entries[i] + } + } + return undefined +} + +/** + * Resolve the checkpoint a file must be restored from in order to undo the + * step identified by `stepCheckpointId` (see the module docstring for the + * resolution rules). + */ +function preStepRestoreTarget( + entries: ChangeJournalEntry[], + filePath: string, + stepCheckpointId: string, +): { target?: string; baseline?: boolean; error?: string } { + const fileEntries = entries.filter((entry) => entry.path === filePath) + const stepIndex = fileEntries.findIndex((entry) => entry.checkpointId === stepCheckpointId) + + if (stepIndex === -1) { + return { error: NOT_IN_STEP_ERROR } + } + + // Only the file's most recent step may be rolled back: restoring an older + // state would overwrite the file's newer writes. A multi-write step shares + // one checkpoint id, so compare on the latest entry's checkpoint id. + const latest = fileEntries[fileEntries.length - 1] + + if (latest.checkpointId !== stepCheckpointId) { + return { error: NOT_LATEST_ERROR } + } + + if (stepIndex === 0) { + return { baseline: true } + } + return { target: fileEntries[stepIndex - 1].checkpointId } +} + +/** + * Run one `restoreFile` and shape the outcome. A failed restore never throws + * out of the rollback API; it is reported on the per-file outcome instead. + */ +async function performRestore( + service: CheckpointService, + target: string, + filePath: string, +): Promise { + try { + await service.restoreFile(target, filePath) + return { filePath, success: true } + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + console.error(`[checkpointRollback] failed to restore ${filePath} from checkpoint ${target}: ${message}`) + return { filePath, success: false, error: message } + } +} + +/** + * Restore a single file to the state it had BEFORE the change-card step + * identified by `stepCheckpointId` — undoing that step's write to the file. + */ +export async function rollbackFile( + task: Task, + stepCheckpointId: string, + filePath: string, +): Promise { + const service = await getCheckpointService(task) + + if (!service) { + return { filePath, success: false, error: NOT_ENABLED_ERROR } + } + + const journal = await loadTaskJournal(task) + + if ("error" in journal) { + return { filePath, success: false, error: journal.error } + } + + const resolved = preStepRestoreTarget(journal.entries, filePath, stepCheckpointId) + + if (resolved.error) { + return { filePath, success: false, error: resolved.error } + } + + const target = resolved.baseline ? service.baseHash : resolved.target + + if (!target) { + return { filePath, success: false, error: NO_TARGET_ERROR } + } + + return performRestore(service, target, filePath) +} + +/** + * Restore every file of a step to the state it had before the step ran. + * + * `stepFiles` comes from the change-card payload (the B2 journal entries for + * the step's checkpoint id). Each file is resolved to its pre-step checkpoint + * through the journal (see the module docstring); without a step checkpoint id + * the latest journal entry per file is used instead (restoring to the file's + * last recorded state, the same direction as `restoreLatestFile`). + */ +export async function rollbackStep( + task: Task, + stepFiles: string[], + stepCheckpointId?: string, +): Promise { + const service = await getCheckpointService(task) + + if (!service) { + return { + checkpointId: stepCheckpointId, + files: stepFiles.map((filePath) => ({ filePath, success: false, error: NOT_ENABLED_ERROR })), + } + } + + const journal = await loadTaskJournal(task) + const files: RollbackFileOutcome[] = [] + + if ("error" in journal) { + const journalError = journal.error + return { + checkpointId: stepCheckpointId, + files: stepFiles.map((filePath) => ({ filePath, success: false, error: journalError })), + } + } + + const entries = journal.entries + + for (const filePath of stepFiles) { + if (stepCheckpointId) { + const resolved = preStepRestoreTarget(entries, filePath, stepCheckpointId) + + if (resolved.error) { + files.push({ filePath, success: false, error: resolved.error }) + continue + } + + const target = resolved.baseline ? service.baseHash : resolved.target + + if (!target) { + files.push({ filePath, success: false, error: NO_TARGET_ERROR }) + continue + } + + files.push(await performRestore(service, target, filePath)) + continue + } + + const latest = latestEntry(entries, filePath) + + if (!latest) { + files.push({ filePath, success: false, error: NO_ENTRY_ERROR }) + continue + } + + files.push(await performRestore(service, latest.checkpointId, filePath)) + } + + return { checkpointId: stepCheckpointId, files } +} + +/** + * Restore one file to the latest recorded version: the content of its most + * recent successful write checkpoint (the forward direction to a rollback). + * A file the task never wrote has no recorded version — the working tree is + * left as-is and the outcome is a successful no-op. + */ +export async function restoreLatestFile(task: Task, filePath: string): Promise { + const service = await getCheckpointService(task) + + if (!service) { + return { filePath, success: false, error: NOT_ENABLED_ERROR } + } + + const journal = await loadTaskJournal(task) + + if ("error" in journal) { + return { filePath, success: false, error: journal.error } + } + + const latest = latestEntry(journal.entries, filePath) + + if (!latest) { + return { filePath, success: true, noOp: true } + } + + return performRestore(service, latest.checkpointId, filePath) +} 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/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/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__/applyPatchTool.execute.spec.ts b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts index eb89838cad..eda5460bce 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__/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/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index 3e3d3d0653..7ebf4bbf8d 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -407,6 +407,163 @@ export abstract class ShadowCheckpointService extends EventEmitter { } } + /** + * Restore a single file to its state at `commitHash` without moving the + * branch or truncating the checkpoint list (unlike + * {@link restoreCheckpoint}). + * + * If the file did not exist at `commitHash`, it is removed from the + * working tree instead — rolling a file back to before it was created. + */ + public async restoreFile(commitHash: string, filePath: string): Promise { + try { + this.log(`[${this.constructor.name}#restoreFile] restoring ${filePath} from ${commitHash}`) + + if (!this.git) { + throw new Error("Shadow git repo not initialized") + } + + // Constrain the path to the workspace before either branch: `..` + // segments would otherwise normalize (path.join / git pathspec) to a + // location outside `this.workspaceDir`, and the delete branch could + // remove an unrelated file. `path.resolve` normalizes the segments; + // the trailing-separator prefix check is the containment guard. + const resolvedTarget = path.resolve(this.workspaceDir, filePath) + const workspaceRoot = this.workspaceDir.endsWith(path.sep) + ? this.workspaceDir + : this.workspaceDir + path.sep + if (resolvedTarget !== this.workspaceDir && !resolvedTarget.startsWith(workspaceRoot)) { + throw new Error(`restoreFile target is outside the workspace: ${filePath}`) + } + + // The lexical check cannot see through a symlinked ancestor: a link + // inside the workspace pointing outside it passes the prefix check + // while the real target resolves elsewhere. Validate containment on + // real paths for every target, not only existing ones: the checkout + // branch writes through a symlinked ancestor even when the target + // itself does not exist. Walk up to the deepest ancestor that has a + // directory entry (lstat, so a dangling symlink counts as present and + // is then rejected by realpath instead of being skipped by an + // exists()-style check, which would validate the root and let the + // checkout report a false success). Resolving both sides keeps a + // legitimate symlinked workspace root working. + const realWorkspaceRoot = await fs.realpath(this.workspaceDir) + const realRoot = realWorkspaceRoot.endsWith(path.sep) ? realWorkspaceRoot : realWorkspaceRoot + path.sep + let realCandidate = resolvedTarget + while (true) { + try { + await fs.lstat(realCandidate) + break + } catch (err) { + const code = (err as NodeJS.ErrnoException).code + if (code !== "ENOENT" && code !== "ENOTDIR") throw err + } + const parent = path.dirname(realCandidate) + if (parent === realCandidate) { + throw new Error(`restoreFile target resolves outside the workspace: ${filePath}`) + } + realCandidate = parent + } + let realChecked: string + try { + realChecked = await fs.realpath(realCandidate) + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "ENOENT") { + throw new Error(`restoreFile target resolves through a dangling link: ${filePath}`) + } + throw err + } + if (realChecked !== realWorkspaceRoot && !realChecked.startsWith(realRoot)) { + throw new Error(`restoreFile target resolves outside the workspace: ${filePath}`) + } + + // Git pathspecs are always POSIX, but the path handed to git must be + // the same path containment just validated. Deriving it from the raw + // request with a blanket backslash conversion would reinterpret + // POSIX filename backslashes as separators: `link\secret.txt` (a + // legal POSIX filename) would reach git as `link/secret.txt` and + // could write through a symlinked directory the validated path never + // traversed. Derive the pathspec from the validated native path and + // convert only its real separators; on Windows this is exactly the + // native-to-POSIX normalization the journal and webview hand back. + const gitPath = path.relative(this.workspaceDir, resolvedTarget).split(path.sep).join("/") + + // restoreFile restores exactly one regular file: a directory pathspec + // would recursively check out the whole subtree from the checkpoint, + // so reject directory targets - including "." and "" resolving to the + // workspace root - before either branch runs. + try { + const targetStat = await fs.lstat(resolvedTarget) + if (targetStat.isDirectory()) { + throw new Error(`restoreFile target is a directory: ${filePath}`) + } + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "ENOENT") throw err + } + + const start = Date.now() + const existed = await this.fileExistsInCommit(commitHash, gitPath) + + if (existed) { + // The checkpoint entry can be a tree or a symlink (whose object + // is also a blob); only regular-file modes may be restored through + // this single-file path. ls-tree reports the mode, unlike cat-file. + const entry = await this.git!.raw(["ls-tree", commitHash, "--", `:(literal)${gitPath}`]) + const mode = entry.trim() ? entry.split("\t")[0].split(" ")[0] : "" + if (mode !== "100644" && mode !== "100755") { + throw new Error(`restoreFile target is not a regular file in the checkpoint: ${filePath}`) + } + // The checkout pathspec must be literal too: a filename containing + // glob characters ([a].ts, foo?bar.ts) would otherwise match - or + // miss - other files, restoring more (or the wrong file) than the + // single requested path. + await this.git.checkout([commitHash, "--", `:(literal)${gitPath}`]) + } else { + // Use the validated path, not a re-join of the raw request: the + // delete branch must remove exactly what containment approved. + await fs.rm(resolvedTarget, { force: true }) + } + + const duration = Date.now() - start + this.emit("restore", { type: "restore", commitHash, duration }) + this.log(`[${this.constructor.name}#restoreFile] restored ${filePath} in ${duration}ms`) + } catch (e) { + const error = e instanceof Error ? e : new Error(String(e)) + this.log(`[${this.constructor.name}#restoreFile] failed to restore file: ${error.message}`) + this.emit("error", { type: "error", error }) + throw error + } + } + + /** Whether `filePath` exists in the tree of `commitHash`. */ + private async fileExistsInCommit(commitHash: string, filePath: string): Promise { + // A failed lookup is not evidence the file is absent. If the commit object + // itself cannot be read (invalid hash, corrupt or missing shadow repo), + // falling through to the restoreFile delete branch would remove a live + // file. Verify the object first and fail the restore loudly instead. + // + // Verification must be evidence-based: simple-git's raw() only rejects + // when git writes a fatal to stderr, and `git cat-file -e ` + // fails *silently* (exit 1, no output) — a silent resolution would be + // read as "valid commit". `rev-parse --verify` emits the resolved id on + // success and a stderr fatal on every failure mode, so the reject is + // reliable. + try { + await this.git!.raw(["rev-parse", "--verify", `${commitHash}^{commit}`]) + } catch { + throw new Error(`Checkpoint unavailable: ${commitHash}`) + } + // Existence must distinguish "absent" from "unreadable": ls-tree exits 0 + // with empty output only when the path is genuinely absent from the + // tree, while a corrupt or missing tree object makes it fail - and that + // failure must propagate rather than be misread as absence, which would + // route restoreFile into the delete branch and rm a live file the + // checkpoint does contain. The :(literal) magic keeps paths containing + // glob characters exact. + const entry = await this.git!.raw(["ls-tree", commitHash, "--", `:(literal)${filePath}`]) + return entry.trim().length > 0 + } + public async getDiff({ from, to }: { from?: string; to?: string }): Promise { if (!this.git) { throw new Error("Shadow git repo not initialized") diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index 76e98367a5..2d8d0532de 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -165,6 +165,336 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( }) }) + describe(`${klass.name}#restoreFile`, () => { + it("restores a modified file to a previous checkpoint without touching other files", async () => { + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + const newFile = path.join(service.workspaceDir, "new.txt") + await fs.writeFile(newFile, "New file content") + const commit2 = await service.saveCheckpoint("Second checkpoint") + expect(commit2?.commit).toBeTruthy() + + // Drift both files, then roll back only test.txt to commit 1. + await fs.writeFile(testFile, "Changed after checkpoint") + await fs.writeFile(newFile, "Also changed") + + await service.restoreFile(commit1!.commit, "test.txt") + + expect(await fs.readFile(testFile, "utf-8")).toBe("Ahoy, world!") + // new.txt is not part of the restore and keeps its drifted content. + expect(await fs.readFile(newFile, "utf-8")).toBe("Also changed") + }) + + it("deletes a file that did not exist at the checkpoint", async () => { + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + const newFile = path.join(service.workspaceDir, "new.txt") + await fs.writeFile(newFile, "Created after the checkpoint") + + await service.restoreFile(commit1!.commit, "new.txt") + + expect(await fileExistsAtPath(newFile)).toBe(false) + // The untouched file keeps its checkpoint-1 content. + expect(await fs.readFile(testFile, "utf-8")).toBe("Ahoy, world!") + }) + + it("accepts a native backslashed path on Windows (git pathspecs are POSIX)", async () => { + if (process.platform !== "win32") { + // Off-Windows the journal paths are already POSIX, so there is + // nothing to normalize; the native-separator handling is still + // exercised by every other restoreFile test (and the POSIX-only + // backslash case has its own test below). + return + } + + // A nested file, so the relative path actually contains a + // separator that Windows writes as a backslash. + const subFile = path.join(path.dirname(testFile), "subdir", "inner.txt") + await fs.mkdir(path.dirname(subFile), { recursive: true }) + await fs.writeFile(subFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + await fs.writeFile(subFile, "Drifted") + + // The change journal and the webview hand backslashed paths to the + // rollback service on Windows; restoreFile must normalize them for + // the git calls. Without the normalization, `cat-file -e` would + // never match and the delete branch would remove a file the + // checkpoint actually contains. + await service.restoreFile(commit1!.commit, "subdir" + path.sep + "inner.txt") + + expect(await fs.readFile(subFile, "utf-8")).toBe("Ahoy, world!") + }) + + it("does not let a POSIX backslash path reach a symlinked directory through git", async () => { + // On POSIX a backslash is a legal filename character. Containment + // validates the literal `link\secret.txt` inside the workspace; a + // blanket backslash conversion for the git pathspec would hand git + // `link/secret.txt` - a path through a directory the validated + // path never traversed - and the checkout would act on it: git + // replaces the symlinked directory with a real one and restores a + // file the caller never requested (CWE-22 family: git acting on + // an unvalidated path; verified against real git on Linux). + if (process.platform === "win32") { + // Backslash is a separator on Windows; the scenario is POSIX-only. + return + } + const innerDir = path.join(service.workspaceDir, "link") + await fs.mkdir(innerDir) + await fs.writeFile(path.join(innerDir, "secret.txt"), "inside") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + const outsideDir = path.join(tmpDir, `outside-${Date.now()}`) + await fs.mkdir(outsideDir, { recursive: true }) + const outsideFile = path.join(outsideDir, "secret.txt") + await fs.writeFile(outsideFile, "outside") + + // Replace the committed directory with a symlink to the outside dir. + await fs.rm(innerDir, { recursive: true }) + await fs.symlink(outsideDir, innerDir, "dir") + + await service.restoreFile(commit1!.commit, "link\\secret.txt") + + // The literal name is not in the checkpoint, so the delete branch + // runs against the validated (literal) path: the symlink survives + // untouched (under the old pathspec it was replaced by a real + // directory holding a file nobody asked to restore) and the + // outside file behind it is untouched. + const linkStat = await fs.lstat(innerDir) + expect(linkStat.isSymbolicLink()).toBe(true) + expect(await fs.readFile(outsideFile, "utf-8")).toBe("outside") + }) + + it("rejects a restore target that escapes the workspace", async () => { + // `..` segments must be normalized and contained, so neither the + // checkout nor the delete branch can touch a file outside the + // workspace (CWE-22). + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + await expect(service.restoreFile(commit1!.commit, path.join("..", "escape.txt"))).rejects.toThrow( + /outside the workspace/, + ) + }) + + it("rejects an unavailable checkpoint instead of deleting the live file", async () => { + // A corrupt journal entry can hand the rollback service a checkpoint + // id that does not resolve to a shadow-repo object. The lookup + // failure must not be read as "file absent at the checkpoint" — + // that would route the restore into the delete branch and remove a + // live file. The restore must fail loudly and leave the file intact. + await fs.writeFile(testFile, "Ahoy, world!") + await service.saveCheckpoint("First checkpoint") + + await expect( + service.restoreFile("0000000000000000000000000000000000000000", "test.txt"), + ).rejects.toThrow("Checkpoint unavailable") + + expect(await fileExistsAtPath(testFile)).toBe(true) + expect(await fs.readFile(testFile, "utf-8")).toBe("Ahoy, world!") + }) + + it("does not delete a live file when the checkpoint tree object is unreadable", async () => { + // The commit object can resolve while its tree is corrupt or missing + // (partial shadow-repo damage). An existence lookup that swallows the + // read failure would report "absent" and delete a live file the + // checkpoint does contain; the failure must propagate instead. + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + const git = service["git"]! + const gitDir = (await git.raw(["rev-parse", "--absolute-git-dir"])).trim() + const treeOid = (await git.raw(["show", "-s", "--format=%T", commit1!.commit])).trim() + await fs.rm(path.join(gitDir, "objects", treeOid.slice(0, 2), treeOid.slice(2)), { force: true }) + + await expect(service.restoreFile(commit1!.commit, "test.txt")).rejects.toThrow() + + expect(await fileExistsAtPath(testFile)).toBe(true) + expect(await fs.readFile(testFile, "utf-8")).toBe("Ahoy, world!") + }) + + it("restores exactly the requested file when its name contains glob characters", async () => { + // A plain pathspec treats [a].ts as a character class matching + // a.ts, so checkout would restore the wrong file and leave the + // requested one modified. The restore contract is exactly one + // path: the checkout pathspec must be literal. + const workspaceDir = path.dirname(testFile) + const globFile = path.join(workspaceDir, "[a].ts") + const plainFile = path.join(workspaceDir, "a.ts") + await fs.writeFile(globFile, "glob content") + await fs.writeFile(plainFile, "plain content") + const commit = await service.saveCheckpoint("Glob-name checkpoint") + expect(commit?.commit).toBeTruthy() + + await fs.writeFile(globFile, "glob modified") + await fs.writeFile(plainFile, "plain modified") + + await service.restoreFile(commit!.commit, "[a].ts") + + expect(await fs.readFile(globFile, "utf-8")).toBe("glob content") + expect(await fs.readFile(plainFile, "utf-8")).toBe("plain modified") + }) + + it("rejects a target that escapes the workspace through a symlinked ancestor", async () => { + // A lexical prefix check passes for a path that goes through a + // symlink inside the workspace pointing outside it; the resolved + // (real) target must be contained as well, or the delete/checkout + // branch would mutate a file the task never owned (CWE-22). + if (process.platform === "win32") { + // Creating symlinks needs elevated privileges on Windows; the + // lexical-containment test above still covers the portable case. + return + } + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + const outsideDir = path.join(tmpDir, `outside-${Date.now()}`) + await fs.mkdir(outsideDir, { recursive: true }) + const outsideFile = path.join(outsideDir, "sneaky.txt") + await fs.writeFile(outsideFile, "outside") + + // A directory link inside the workspace pointing at the outside dir: + // lexically `link/sneaky.txt` is inside the workspace. + await fs.symlink(outsideDir, path.join(service.workspaceDir, "link"), "dir") + + // "resolves outside the workspace" is the real-path (symlink) + // guard's message; the bare "outside the workspace" would also + // match the lexical guard's error. + await expect(service.restoreFile(commit1!.commit, path.join("link", "sneaky.txt"))).rejects.toThrow( + /resolves outside the workspace/, + ) + + // The outside file survives: the restore failed before any mutation. + expect(await fileExistsAtPath(outsideFile)).toBe(true) + expect(await fs.readFile(outsideFile, "utf-8")).toBe("outside") + }) + + it("rejects a nonexistent target under a symlinked ancestor before checkout", async () => { + // The checkout branch writes through symlinked ancestors even when + // the target file itself does not exist yet, so containment must be + // validated on the deepest existing ancestor for every target + // (CWE-22 defense-in-depth). "junction" resolves without elevation + // on Windows and behaves as a directory symlink on POSIX. + const innerDir = path.join(service.workspaceDir, "sub") + await fs.mkdir(innerDir) + await fs.writeFile(path.join(innerDir, "newfile.txt"), "v1") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + const outsideDir = path.join(tmpDir, `outside-${Date.now()}`) + await fs.mkdir(outsideDir, { recursive: true }) + + // Replace the real dir with a link to the outside dir: the commit + // still carries sub/newfile.txt, but the working-tree location now + // escapes through the link, and the target does not exist. + await fs.rm(innerDir, { recursive: true, force: true }) + await fs.symlink(outsideDir, innerDir, "junction") + + await expect(service.restoreFile(commit1!.commit, path.join("sub", "newfile.txt"))).rejects.toThrow( + /resolves outside the workspace/, + ) + + // The checkout never wrote through the link. + expect(await fileExistsAtPath(path.join(outsideDir, "newfile.txt"))).toBe(false) + }) + + it("rejects a target under a dangling symlink ancestor instead of a false success", async () => { + // A dangling link is invisible to exists()-style checks (they follow + // the link), so an exists()-based containment walk would validate the + // workspace root and let the checkout branch report success without + // ever creating the file. The lstat walk must see the link entry and + // reject through realpath. "junction" needs no elevation on Windows + // and behaves as a directory symlink on POSIX. + const innerDir = path.join(service.workspaceDir, "sub") + await fs.mkdir(innerDir) + await fs.writeFile(path.join(innerDir, "newfile.txt"), "v1") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + await fs.rm(innerDir, { recursive: true, force: true }) + await fs.symlink(path.join(tmpDir, `gone-${Date.now()}`), innerDir, "junction") + + await expect(service.restoreFile(commit1!.commit, path.join("sub", "newfile.txt"))).rejects.toThrow( + /dangling link/, + ) + + // Nothing was created through the dangling link. + expect(await fileExistsAtPath(path.join(innerDir, "newfile.txt"))).toBe(false) + }) + + it("rejects restoring the workspace root as a single file", async () => { + // "." passes the lexical and realpath guards (it IS the workspace) + // but as a pathspec it would recursively restore the entire tree + // from the checkpoint - a blast radius far beyond one file. + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + await expect(service.restoreFile(commit1!.commit, ".")).rejects.toThrow( + /restoreFile target is a directory/, + ) + }) + + it("rejects a directory target that is a tree in the checkpoint", async () => { + // The on-disk directory check cannot catch a directory the task + // already deleted; only the checkpoint tree-entry mode check can + // stop git checkout from recursively restoring the subtree. + const innerDir = path.join(service.workspaceDir, "sub") + await fs.mkdir(innerDir) + await fs.writeFile(path.join(innerDir, "a.txt"), "a") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + await fs.rm(innerDir, { recursive: true, force: true }) + await expect(service.restoreFile(commit1!.commit, "sub")).rejects.toThrow( + /not a regular file in the checkpoint/, + ) + expect(await fileExistsAtPath(path.join(innerDir, "a.txt"))).toBe(false) + }) + + it("emits a restore event when a file is restored", async () => { + const restoreListener = vi.fn() + service.on("restore", restoreListener) + + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + await fs.writeFile(testFile, "Drifted") + await service.restoreFile(commit1!.commit, "test.txt") + + expect(restoreListener).toHaveBeenCalledWith( + expect.objectContaining({ type: "restore", commitHash: commit1!.commit }), + ) + }) + + it("throws and emits an error event when the shadow repo is not initialized", async () => { + // A service that never ran initShadowGit has no git handle; restoreFile + // must fail cleanly and surface the error event. + const raw = await klass.create({ + taskId, + shadowDir: path.join(tmpDir, `noinit-${Date.now()}`), + workspaceDir: path.join(tmpDir, `ws-noinit-${Date.now()}`), + log: () => {}, + }) + + const errorListener = vi.fn() + raw.on("error", errorListener) + + await expect(raw.restoreFile("sha-x", "test.txt")).rejects.toThrow("Shadow git repo not initialized") + expect(errorListener).toHaveBeenCalledWith(expect.objectContaining({ type: "error" })) + }) + }) + describe(`${klass.name}#saveCheckpoint`, () => { it("creates a checkpoint if there are pending changes", async () => { await fs.writeFile(testFile, "Ahoy, world!") 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..7d20c2537e --- /dev/null +++ b/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx @@ -0,0 +1,194 @@ +// npx vitest src/components/settings/__tests__/CheckpointSettings.spec.tsx + +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, + Slider: ({ defaultValue, onValueChange, "data-testid": dataTestId }: any) => ( + 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", () => ({ + VSCodeCheckbox: ({ checked, onChange, children, ...props }: any) => ( + + ), + VSCodeLink: ({ children, ...props }: any) => {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..ed9c1743a3 100644 --- a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx +++ b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx @@ -426,6 +426,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 +460,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 +481,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 +499,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 +514,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": {