diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index eb51c1d67d..067774e950 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -114,6 +114,13 @@ 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 + /** * GlobalSettings */ @@ -223,6 +230,12 @@ 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(), ttsEnabled: z.boolean().optional(), ttsSpeed: z.number().optional(), diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index a5393ddc66..07baaf9ce8 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -364,6 +364,7 @@ export type ExtensionState = Pick< enableCheckpoints: boolean checkpointTimeout: number // Timeout for checkpoint initialization in seconds (default: 15) + perWriteCheckpoints: boolean 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__/changeJournal.spec.ts b/src/core/checkpoints/__tests__/changeJournal.spec.ts new file mode 100644 index 0000000000..722827039a --- /dev/null +++ b/src/core/checkpoints/__tests__/changeJournal.spec.ts @@ -0,0 +1,310 @@ +import fs from "fs/promises" +import os from "os" +import path from "path" + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" + +import { appendChange, 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"]) + }) + + 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("propagates a non-ENOENT tail-inspection failure instead of appending blind", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "good" })) + const filePath = journalPath(tmpRoot, taskId) + const raw = await fs.readFile(filePath, "utf8") + await fs.writeFile(filePath, raw + '{"path":"src/half.ts","operation":"upd') + // The tail cannot be inspected (I/O error): appending would merge the + // new entry into the fragment, so appendChange must fail loudly. + const statSpy = vi + .spyOn(fs, "stat") + .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 { + statSpy.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 === "a") { + 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("fsyncs the containing directory when the append creates the journal file", async () => { + const opened: string[] = [] + const realOpen = fs.open + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (pathish, flags) => { + opened.push(String(pathish)) + const openAs = realOpen as (p: string | URL, f?: string) => Promise + return openAs(pathish as string | URL, flags as string | undefined) + }) + try { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "first" })) + } finally { + openSpy.mockRestore() + } + // The creation path must sync the journal's parent directory so the new + // file's directory entry (and any directory fs.mkdir just created) + // survives a crash before the append is reported as committed. + expect(opened).toContain(path.dirname(journalPath(tmpRoot, taskId))) + }) + + it("does not re-open the directory for appends to an existing journal", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "first" })) + const dir = path.dirname(journalPath(tmpRoot, taskId)) + const opened: string[] = [] + const realOpen = fs.open + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (pathish, flags) => { + opened.push(String(pathish)) + const openAs = realOpen as (p: string | URL, f?: string) => Promise + return openAs(pathish as string | URL, flags as string | undefined) + }) + try { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "second" })) + } finally { + openSpy.mockRestore() + } + // The directory entry is already durable; re-syncing on every append + // would add a syscall to the hot path for no durability gain. + expect(opened).not.toContain(dir) + }) + + it("tolerates platforms where the journal directory cannot be synced", async () => { + const dir = path.dirname(journalPath(tmpRoot, taskId)) + const realOpen = fs.open + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const openSpy = vi.spyOn(fs, "open").mockImplementation(async (pathish, flags) => { + if (String(pathish) === dir) { + throw Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + } + const openAs = realOpen as (p: string | URL, f?: string) => Promise + return openAs(pathish as string | URL, flags as string | undefined) + }) + try { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "first" })) + } finally { + openSpy.mockRestore() + warnSpy.mockRestore() + } + // The entry itself is durable via the file sync; the platform without + // directory sync degrades to a warning instead of failing the append. + const result = await loadChanges(tmpRoot, taskId) + expect(result.map((e) => e.checkpointId)).toEqual(["first"]) + }) + }) + + describe("task ID containment", () => { + it("rejects task IDs that could escape the tasks directory", async () => { + for (const bad of ["../../outside", "a/b", "a\\b", "", ".", ".."]) { + expect(() => journalPath(tmpRoot, bad)).toThrow(/Unsafe task ID/) + await expect(appendChange(tmpRoot, bad, entry({ checkpointId: "x" }))).rejects.toThrow(/Unsafe task ID/) + await expect(loadChanges(tmpRoot, bad)).rejects.toThrow(/Unsafe task ID/) + // Nothing was created outside the task directory. + await expect(fs.stat(path.join(tmpRoot, "..", "outside"))).rejects.toMatchObject({ code: "ENOENT" }) + } + }) + + it("keeps a safe task ID under the tasks directory", () => { + expect(journalPath(tmpRoot, "task-42")).toBe( + path.join(tmpRoot, "tasks", "task-42", "checkpoints", "changes.jsonl"), + ) + }) + }) + + describe("loadChanges", () => { + it("returns [] for an absent journal file", async () => { + expect(await loadChanges(tmpRoot, taskId)).toEqual([]) + }) + + it("returns [] for an empty journal file", async () => { + await fs.mkdir(path.dirname(journalPath(tmpRoot, taskId)), { recursive: true }) + await fs.writeFile(journalPath(tmpRoot, taskId), "") + + expect(await loadChanges(tmpRoot, taskId)).toEqual([]) + }) + + it("parses all entries in order with a clean tail", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "x" })) + await appendChange(tmpRoot, taskId, entry({ checkpointId: "y" })) + await appendChange(tmpRoot, taskId, entry({ checkpointId: "z" })) + + const result = await loadChanges(tmpRoot, taskId) + expect(result).toHaveLength(3) + expect(result[0].checkpointId).toBe("x") + expect(result[1].checkpointId).toBe("y") + expect(result[2].checkpointId).toBe("z") + }) + + it("parses a journal whose final line has no trailing newline", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "ok" })) + + // Rewrite the file without the trailing newline of the last line. + const filePath = journalPath(tmpRoot, taskId) + const content = (await fs.readFile(filePath, "utf8")).replace(/\n$/, "") + await fs.writeFile(filePath, content) + + const result = await loadChanges(tmpRoot, taskId) + expect(result).toHaveLength(1) + expect(result[0].checkpointId).toBe("ok") + }) + + it("discards a torn final line and returns the complete entries", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "ok" })) + + // Append a second line truncated mid-content, with no trailing newline. + await fs.appendFile(journalPath(tmpRoot, taskId), '{"path":"src/half.ts","operation":"upd') + + const result = await loadChanges(tmpRoot, taskId) + expect(result).toHaveLength(1) + expect(result[0].checkpointId).toBe("ok") + }) + + 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("does not throw when the entire journal is torn", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "first" })) + + // Truncate to a single character — definitely invalid JSON. + await fs.writeFile(journalPath(tmpRoot, taskId), "{") + + expect(await loadChanges(tmpRoot, taskId)).toEqual([]) + }) + + 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() + }) + + it("propagates a non-ENOENT failure from loadChanges instead of reporting an empty history", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "good" })) + // A transient read failure is not an empty rollback history: a + // restore that saw [] would report a no-op success without ever + // reading the real entries (appendChange already propagates + // non-ENOENT errors; the loader must agree). + const readSpy = vi + .spyOn(fs, "readFile") + .mockRejectedValueOnce(Object.assign(new Error("simulated I/O error"), { code: "EIO" })) + try { + await expect(loadChanges(tmpRoot, taskId)).rejects.toMatchObject({ code: "EIO" }) + } finally { + readSpy.mockRestore() + } + }) + }) +}) diff --git a/src/core/checkpoints/__tests__/checkpointJournal.test.ts b/src/core/checkpoints/__tests__/checkpointJournal.test.ts new file mode 100644 index 0000000000..43b106a5da --- /dev/null +++ b/src/core/checkpoints/__tests__/checkpointJournal.test.ts @@ -0,0 +1,353 @@ +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 { appendChange, loadChanges, pendingJournalPath, writePendingJournal } 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 +} + +interface TaskLike { + taskId: string + enableCheckpoints: boolean + checkpointService: ServiceLike + checkpointServiceInitializing: boolean + providerRef: { deref: () => ProviderLike | undefined } +} + +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(), + } + // 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 }, + } + }) + + 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 and skips the journal on save failure", async () => { + saveCheckpointSpy.mockRejectedValueOnce(new Error("git exploded")) + + await expect(checkpointSave(mockTask as Task, false, true, write)).resolves.toBeUndefined() + expect(mockTask.enableCheckpoints).toBe(false) + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + }) + + 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("does not crash when the provider has no globalStorageDir", async () => { + mockTask.providerRef = { deref: () => undefined } + + await expect(checkpointSave(mockTask as Task, false, true, write)).resolves.toMatchObject({ commit: COMMIT }) + expect(mockTask.enableCheckpoints).toBe(true) + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + }) + + it("skips the checkpoint when the pending marker cannot be written durably", async () => { + // Block the per-task checkpoint dir so the pending-marker mkdir/write 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) + + // A committed checkpoint whose pending marker never landed could lose its journal + // state, so the marker is a hard prerequisite: the save is skipped and no commit is + // produced. + await expect(checkpointSave(mockTask as Task, false, true, write)).resolves.toBeUndefined() + expect(saveCheckpointSpy).not.toHaveBeenCalled() + expect(mockTask.enableCheckpoints).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("not running the checkpoint"), + expect.anything(), + ) + }) +}) + +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" }]) + }) +}) + +describe("checkpoint/journal transaction recovery (B2)", () => { + let tmpStorageDir: string + + beforeEach(async () => { + tmpStorageDir = await fs.mkdtemp(path.join(os.tmpdir(), "b2-journal-recovery-")) + }) + + afterEach(async () => { + await fs.rm(tmpStorageDir, { recursive: true, force: true }) + }) + + it("folds a durable ready marker into the journal on the next load", async () => { + await writePendingJournal(tmpStorageDir, TASK_ID, { + state: "ready", + createdAt: Date.now(), + entries: [{ path: "src/a.ts", operation: "create", checkpointId: "c1" }], + }) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toHaveLength(1) + expect(entries[0]).toMatchObject({ path: "src/a.ts", operation: "create", checkpointId: "c1" }) + // The marker is consumed once its entries are durable. + await expect(fs.stat(pendingJournalPath(tmpStorageDir, TASK_ID))).rejects.toMatchObject({ code: "ENOENT" }) + }) + + it("drops an awaiting-checkpoint marker without inventing entries", async () => { + await writePendingJournal(tmpStorageDir, TASK_ID, { state: "awaiting-checkpoint", createdAt: Date.now() }) + + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + await expect(fs.stat(pendingJournalPath(tmpStorageDir, TASK_ID))).rejects.toMatchObject({ code: "ENOENT" }) + }) + + it("does not duplicate entries already appended before a crash", async () => { + await appendChange(tmpStorageDir, TASK_ID, { path: "src/a.ts", operation: "create", checkpointId: "c1" }) + await writePendingJournal(tmpStorageDir, TASK_ID, { + state: "ready", + createdAt: Date.now(), + entries: [ + { path: "src/a.ts", operation: "create", checkpointId: "c1" }, + { path: "src/b.ts", operation: "update", checkpointId: "c1" }, + ], + }) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries.map((entry) => entry.path)).toEqual(["src/a.ts", "src/b.ts"]) + }) + + it.each(["not-json", JSON.stringify({ state: "unknown" }), JSON.stringify({ state: "ready" })])( + "discards an unreadable or unrecognized pending marker (%s)", + async (rawMarker) => { + const markerPath = pendingJournalPath(tmpStorageDir, TASK_ID) + await fs.mkdir(path.dirname(markerPath), { recursive: true }) + await fs.writeFile(markerPath, rawMarker) + + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + // The unreadable marker is removed so it cannot shadow later writes. + await expect(fs.stat(markerPath)).rejects.toMatchObject({ code: "ENOENT" }) + }, + ) + + it("writes the pending marker with the exact state literals and no extra fields", async () => { + const readyEntry = { path: "src/a.ts", operation: "create" as const, checkpointId: "c1" } + await writePendingJournal(tmpStorageDir, TASK_ID, { state: "awaiting-checkpoint", createdAt: 1 }) + const awaiting = JSON.parse(await fs.readFile(pendingJournalPath(tmpStorageDir, TASK_ID), "utf8")) + // Exact shape: a changed literal or an extra object property would leave the + // recovery read unable to classify the marker. + expect(awaiting).toEqual({ state: "awaiting-checkpoint", createdAt: 1 }) + + await writePendingJournal(tmpStorageDir, TASK_ID, { state: "ready", createdAt: 2, entries: [readyEntry] }) + const ready = JSON.parse(await fs.readFile(pendingJournalPath(tmpStorageDir, TASK_ID), "utf8")) + expect(ready).toEqual({ state: "ready", createdAt: 2, entries: [readyEntry] }) + }) + + it("folds a marker left by an earlier crash before writing the new awaiting marker", async () => { + // A earlier run committed a checkpoint but left its ready marker behind. + await writePendingJournal(tmpStorageDir, TASK_ID, { + state: "ready", + createdAt: 1, + entries: [{ path: "src/earlier.ts", operation: "update", checkpointId: "c0" }], + }) + + // The next save must fold that recovery record in, not overwrite it. + const saveCheckpointSpy = vi.fn().mockResolvedValue({ commit: COMMIT }) + const provider = { + context: { globalStorageUri: { fsPath: tmpStorageDir } }, + log: vi.fn(), + postMessageToWebview: vi.fn(), + } + const task = { + taskId: TASK_ID, + enableCheckpoints: true, + checkpointService: { isInitialized: true, saveCheckpoint: saveCheckpointSpy }, + checkpointServiceInitializing: false, + providerRef: { deref: () => provider }, + } + await checkpointSave(task as unknown as Task, false, true, { + path: "src/foo.ts", + operation: "create", + }) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries.map((entry) => entry.checkpointId)).toEqual(["c0", COMMIT]) + expect(entries.map((entry) => entry.path)).toEqual(["src/earlier.ts", "src/foo.ts"]) + }) +}) diff --git a/src/core/checkpoints/__tests__/checkpointJournalPromotion.test.ts b/src/core/checkpoints/__tests__/checkpointJournalPromotion.test.ts new file mode 100644 index 0000000000..5b33485249 --- /dev/null +++ b/src/core/checkpoints/__tests__/checkpointJournalPromotion.test.ts @@ -0,0 +1,193 @@ +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 { incompleteJournalPath, loadChanges, pendingJournalPath } 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() })) +vi.mock("../../../services/checkpoints") + +type JournalControls = { + readyFailures: number + incompleteFails: boolean + reconcileCalls: number + reconcileFailAfter: number +} +const controls = vi.hoisted(() => ({ + readyFailures: 0, + incompleteFails: false, + reconcileCalls: 0, + reconcileFailAfter: -1, +})) + +// The journal helpers are delegated to the real implementation; only the failure +// paths the tests drive are overridden, so recovery stays real. +vi.mock("../changeJournal", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + writePendingJournal: vi.fn(async (dir: string, taskId: string, state: { state: string }) => { + if (state.state === "ready" && controls.readyFailures > 0) { + controls.readyFailures-- + throw new Error("ready marker write failed") + } + return actual.writePendingJournal(dir, taskId, state as never) + }), + writeIncompleteJournal: vi.fn(async (dir: string, taskId: string, entries: never) => { + if (controls.incompleteFails) { + throw new Error("incomplete marker write failed") + } + return actual.writeIncompleteJournal(dir, taskId, entries) + }), + reconcilePendingJournal: vi.fn(async (dir: string, taskId: string) => { + controls.reconcileCalls++ + if (controls.reconcileFailAfter >= 0 && controls.reconcileCalls > controls.reconcileFailAfter) { + throw new Error("reconcile failed") + } + return actual.reconcilePendingJournal(dir, taskId) + }), + } +}) + +const TASK_ID = "journal-promotion-task" +const COMMIT = "promotion-commit-hash" + +describe("checkpointSave journal promotion failure (B2)", () => { + let tmpStorageDir: string + let saveCheckpointSpy: Mock + let mockTask: { + taskId: string + enableCheckpoints: boolean + checkpointService: { isInitialized: boolean; saveCheckpoint: (...args: unknown[]) => Promise } + checkpointServiceInitializing: boolean + providerRef: { deref: () => unknown } + } + const write: CheckpointWriteInfo = { path: "src/foo.ts", operation: "create" } + + beforeEach(async () => { + controls.readyFailures = 0 + controls.incompleteFails = false + controls.reconcileCalls = 0 + controls.reconcileFailAfter = -1 + tmpStorageDir = await fs.mkdtemp(path.join(os.tmpdir(), "b2-journal-promotion-")) + saveCheckpointSpy = vi.fn().mockResolvedValue({ commit: COMMIT }) + const mockProvider = { + context: { globalStorageUri: { fsPath: tmpStorageDir } }, + log: vi.fn(), + postMessageToWebview: vi.fn(), + } + mockTask = { + taskId: TASK_ID, + enableCheckpoints: true, + checkpointService: { isInitialized: true, saveCheckpoint: saveCheckpointSpy }, + checkpointServiceInitializing: false, + providerRef: { deref: () => mockProvider }, + } + }) + + afterEach(async () => { + vi.restoreAllMocks() + await fs.rm(tmpStorageDir, { recursive: true, force: true }) + }) + + it("retries the ready promotion once and journals the entries when the retry succeeds", async () => { + controls.readyFailures = 1 + + await expect(checkpointSave(mockTask as unknown as Task, false, true, write)).resolves.toEqual({ + commit: COMMIT, + }) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toHaveLength(1) + expect(entries[0]).toMatchObject({ path: "src/foo.ts", operation: "create", checkpointId: COMMIT }) + await expect(fs.stat(incompleteJournalPath(tmpStorageDir, TASK_ID))).rejects.toMatchObject({ code: "ENOENT" }) + }) + + it("records the entries in the incomplete marker and does not report success", async () => { + controls.readyFailures = 99 + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined) + + // The ready promotion fails twice; the incomplete marker is the recovery record. + await expect(checkpointSave(mockTask as unknown as Task, false, true, write)).resolves.toBeUndefined() + + expect(saveCheckpointSpy).toHaveBeenCalledOnce() + expect(mockTask.enableCheckpoints).toBe(false) + const marker = JSON.parse(await fs.readFile(incompleteJournalPath(tmpStorageDir, TASK_ID), "utf8")) + expect(marker.entries).toEqual([{ path: "src/foo.ts", operation: "create", checkpointId: COMMIT }]) + expect(consoleErrorSpy).toHaveBeenCalled() + }) + + it("folds the incomplete marker into the journal on the next read", async () => { + controls.readyFailures = 99 + vi.spyOn(console, "error").mockImplementation(() => undefined) + + await checkpointSave(mockTask as unknown as Task, false, true, write) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + + expect(entries).toHaveLength(1) + expect(entries[0]).toMatchObject({ path: "src/foo.ts", operation: "create", checkpointId: COMMIT }) + await expect(fs.stat(incompleteJournalPath(tmpStorageDir, TASK_ID))).rejects.toMatchObject({ code: "ENOENT" }) + }) + + it("keeps the awaiting marker and disables checkpoints when neither marker can be written", async () => { + controls.readyFailures = 99 + controls.incompleteFails = true + vi.spyOn(console, "error").mockImplementation(() => undefined) + + await expect(checkpointSave(mockTask as unknown as Task, false, true, write)).resolves.toBeUndefined() + + expect(mockTask.enableCheckpoints).toBe(false) + // The awaiting marker written before the commit stays as the on-disk state; it is + // not claimed to recover entries (only a ready/incomplete marker carries them). + const marker = JSON.parse(await fs.readFile(pendingJournalPath(tmpStorageDir, TASK_ID), "utf8")) + expect(marker.state).toBe("awaiting-checkpoint") + }) + + it("reports the checkpoint result and logs when the reconciliation fails", async () => { + // First reconcile runs before the awaiting marker, the second after promotion. + controls.reconcileFailAfter = 1 + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined) + + await expect(checkpointSave(mockTask as unknown as Task, false, true, write)).resolves.toEqual({ + commit: COMMIT, + }) + + expect(mockTask.enableCheckpoints).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("failed to reconcile the pending change journal"), + expect.anything(), + ) + }) +}) diff --git a/src/core/checkpoints/__tests__/getCheckpointService.spec.ts b/src/core/checkpoints/__tests__/getCheckpointService.spec.ts new file mode 100644 index 0000000000..99a8b5b788 --- /dev/null +++ b/src/core/checkpoints/__tests__/getCheckpointService.spec.ts @@ -0,0 +1,126 @@ +import { beforeEach, describe, expect, it, vi } from "vitest" + +import type { Task } from "../../task/Task" +import { RepoPerTaskCheckpointService } from "../../../services/checkpoints" +import { checkGitInstalled } from "../../../utils/git" +import { getCheckpointService } from "../index" + +vi.mock("../../../services/checkpoints", () => ({ + RepoPerTaskCheckpointService: { create: vi.fn() }, +})) + +vi.mock("../../../utils/git", () => ({ + checkGitInstalled: vi.fn(), +})) + +vi.mock("../../../utils/path", () => ({ + getWorkspacePath: vi.fn(() => "/workspace"), +})) + +vi.mock("../../../i18n", () => ({ + t: (key: string) => key, +})) + +/** + * Structural service double: getCheckpointService only registers listeners, + * awaits initShadowGit, and reads isInitialized. The double assertion at the + * create() call sites is confined to this file (AGENTS.md last-resort cast): + * the real class carries a Git handle and event surface the test never + * exercises. + */ +function serviceDouble(initShadowGitImpl: () => Promise) { + const listeners = new Map void>() + let initialized = false + const svc = { + on: vi.fn((event: string, cb: (...args: unknown[]) => void) => { + listeners.set(event, cb) + }), + initShadowGit: vi.fn(() => initShadowGitImpl()), + get isInitialized() { + return initialized + }, + completeInit: () => { + initialized = true + listeners.get("initialize")?.() + }, + } + return svc +} + +function makeTask(): Task { + return { + taskId: "task-init", + cwd: "/workspace", + enableCheckpoints: true, + checkpointService: undefined, + checkpointServiceInitializing: false, + checkpointTimeout: 1, + providerRef: { + deref: vi.fn().mockReturnValue({ + context: { globalStorageUri: { fsPath: "/tmp/b3-init-storage" } }, + postMessageToWebview: vi.fn(), + log: vi.fn(), + }), + }, + say: vi.fn().mockResolvedValue(undefined), + } as unknown as Task +} + +describe("getCheckpointService initialization failures", () => { + beforeEach(() => { + vi.mocked(checkGitInstalled).mockReset() + vi.mocked(RepoPerTaskCheckpointService.create).mockReset() + }) + + it("does not store a service whose shadow-git initialization failed", async () => { + vi.mocked(checkGitInstalled).mockResolvedValue(true) + const failing = serviceDouble(async () => { + throw new Error("shadow git init exploded") + }) + vi.mocked(RepoPerTaskCheckpointService.create).mockReturnValue( + failing as unknown as RepoPerTaskCheckpointService, + ) + + const task = makeTask() + const service = await getCheckpointService(task) + + // The failed service must not be stored: a stored uninitialized handle + // would be returned by the fast path to every later save, and the + // initializing flag must not strand later lookups in a wait for an + // initialization that already died. + expect(service).toBeUndefined() + expect(task.checkpointService).toBeUndefined() + expect(task.checkpointServiceInitializing).toBe(false) + expect(task.enableCheckpoints).toBe(false) + }) + + it("does not store a service when Git is not installed", async () => { + vi.mocked(checkGitInstalled).mockResolvedValue(false) + const svc = serviceDouble(async () => {}) + vi.mocked(RepoPerTaskCheckpointService.create).mockReturnValue(svc as unknown as RepoPerTaskCheckpointService) + + const task = makeTask() + const service = await getCheckpointService(task) + + expect(service).toBeUndefined() + expect(task.checkpointService).toBeUndefined() + expect(task.checkpointServiceInitializing).toBe(false) + expect(task.enableCheckpoints).toBe(false) + }) + + it("stores an initialized service on the success path", async () => { + vi.mocked(checkGitInstalled).mockResolvedValue(true) + const svc = serviceDouble(async () => { + svc.completeInit() + }) + vi.mocked(RepoPerTaskCheckpointService.create).mockReturnValue(svc as unknown as RepoPerTaskCheckpointService) + + const task = makeTask() + const service = await getCheckpointService(task) + + expect(service).toBe(svc) + expect(task.checkpointService).toBe(svc) + expect(task.checkpointServiceInitializing).toBe(false) + expect(task.enableCheckpoints).toBe(true) + }) +}) diff --git a/src/core/checkpoints/changeJournal.ts b/src/core/checkpoints/changeJournal.ts new file mode 100644 index 0000000000..6b1c283c8e --- /dev/null +++ b/src/core/checkpoints/changeJournal.ts @@ -0,0 +1,390 @@ +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 actually written + * by the patch (including the written subset of a partially failed patch) — + * those entries all reference the single B1 checkpoint that the patch's + * post-loop hook saves. 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) +} + +const PENDING_FILENAME = "changes.pending.json" + +/** + * On-disk state for the checkpoint/journal transaction. + * + * `awaiting-checkpoint` is written before the Git checkpoint is created: if the + * process dies before the checkpoint confirms, there is no durable commit to + * attribute entries to. `ready` carries the complete entries (with their commit + * id) and is written before any append: a crash or an append failure then leaves + * a durable marker that the next read reconciles into the journal. + */ +export interface PendingJournalState { + state: "awaiting-checkpoint" | "ready" + createdAt: number + entries?: ChangeJournalEntry[] +} + +/** + * Pending-marker path for a given task. + * @internal Exported for the focused transaction-recovery tests. + */ +export function pendingJournalPath(globalStorageDir: string, taskId: string): string { + assertSafeTaskId(taskId) + return path.join(taskCheckpointDir(globalStorageDir, taskId), PENDING_FILENAME) +} + +/** + * Runtime shape guard for entries read back from a marker or journal line: + * JSON.parse output is untrusted, so consumers validate instead of casting. + */ +function isChangeJournalEntry(value: unknown): value is ChangeJournalEntry { + if (value === null || typeof value !== "object") { + return false + } + const candidate = value as Partial + return ( + typeof candidate.path === "string" && + typeof candidate.checkpointId === "string" && + (candidate.operation === "create" || candidate.operation === "update" || candidate.operation === "delete") + ) +} + +/** Write the pending marker atomically (staging file + rename in the same dir). */ +export async function writePendingJournal( + globalStorageDir: string, + taskId: string, + state: PendingJournalState, +): Promise { + const filePath = pendingJournalPath(globalStorageDir, taskId) + await fs.mkdir(path.dirname(filePath), { recursive: true }) + const stagingPath = `${filePath}.${process.pid}.${Date.now()}.staging` + try { + await fs.writeFile(stagingPath, JSON.stringify(state), "utf8") + await fs.rename(stagingPath, filePath) + } catch (error) { + await fs.rm(stagingPath, { force: true }).catch(() => {}) + throw error + } +} + +/** Remove the pending marker after the journal has durably recorded it. */ +export async function clearPendingJournal(globalStorageDir: string, taskId: string): Promise { + await fs.rm(pendingJournalPath(globalStorageDir, taskId), { force: true }) +} + +const INCOMPLETE_FILENAME = "changes.incomplete.json" + +/** + * Marker written when a committed checkpoint's journal entries could not be made + * durable through the pending marker or a direct append. It carries the complete + * entries so the next read can recover them; unlike the pending marker it is never + * interpreted as "nothing to do". + * @internal Exported for the recovery tests. + */ +export function incompleteJournalPath(globalStorageDir: string, taskId: string): string { + assertSafeTaskId(taskId) + return path.join(taskCheckpointDir(globalStorageDir, taskId), INCOMPLETE_FILENAME) +} + +/** Write the incomplete-journal recovery marker atomically. */ +export async function writeIncompleteJournal( + globalStorageDir: string, + taskId: string, + entries: ChangeJournalEntry[], +): Promise { + const filePath = incompleteJournalPath(globalStorageDir, taskId) + await fs.mkdir(path.dirname(filePath), { recursive: true }) + const stagingPath = `${filePath}.${process.pid}.${Date.now()}.staging` + try { + await fs.writeFile(stagingPath, JSON.stringify({ entries }), "utf8") + await fs.rename(stagingPath, filePath) + } catch (error) { + await fs.rm(stagingPath, { force: true }).catch(() => {}) + throw error + } +} + +/** + * Fold an incomplete-journal marker into the journal. Idempotent like the pending + * marker: entries already present are skipped, and the marker is removed only once + * every entry is durable. + */ +export async function reconcileIncompleteJournal(globalStorageDir: string, taskId: string): Promise { + const filePath = incompleteJournalPath(globalStorageDir, taskId) + let raw: string + try { + raw = await fs.readFile(filePath, "utf8") + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + return + } + throw error + } + let entries: ChangeJournalEntry[] = [] + try { + const parsed = JSON.parse(raw) as { entries?: unknown } + if (Array.isArray(parsed.entries)) { + entries = parsed.entries.filter(isChangeJournalEntry) + } + } catch { + console.warn("[changeJournal] discarding an unreadable incomplete-journal marker") + await fs.rm(filePath, { force: true }).catch(() => {}) + return + } + const existing = await readJournalEntries(globalStorageDir, taskId) + const seen = new Set(existing.map((entry) => `${entry.path}\u0000${entry.operation}\u0000${entry.checkpointId}`)) + for (const entry of entries) { + const key = `${entry.path}\u0000${entry.operation}\u0000${entry.checkpointId}` + if (seen.has(key)) { + continue + } + await appendChange(globalStorageDir, taskId, entry) + seen.add(key) + } + await fs.rm(filePath, { force: true }) +} + +/** + * Fold a durable pending marker into the journal. + * + * Idempotent: entries already present (same path, operation and checkpoint id) + * are skipped, so a crash between the append and the marker removal cannot + * duplicate them. A marker that is still `awaiting-checkpoint` is discarded — + * there is no committed checkpoint whose id could be attributed to the writes. + */ +export async function reconcilePendingJournal(globalStorageDir: string, taskId: string): Promise { + const filePath = pendingJournalPath(globalStorageDir, taskId) + let raw: string + try { + raw = await fs.readFile(filePath, "utf8") + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + return + } + throw error + } + let marker: PendingJournalState | undefined + try { + marker = JSON.parse(raw) as PendingJournalState + } catch { + marker = undefined + } + if (!marker || typeof marker !== "object" || (marker.state !== "ready" && marker.state !== "awaiting-checkpoint")) { + console.warn("[changeJournal] discarding an unreadable pending-journal marker") + await clearPendingJournal(globalStorageDir, taskId) + return + } + if (marker.state === "ready" && Array.isArray(marker.entries) && marker.entries.length > 0) { + const existing = await readJournalEntries(globalStorageDir, taskId) + const seen = new Set( + existing.map((entry) => `${entry.path}\u0000${entry.operation}\u0000${entry.checkpointId}`), + ) + for (const entry of marker.entries) { + const key = `${entry.path}\u0000${entry.operation}\u0000${entry.checkpointId}` + if (seen.has(key)) { + continue + } + await appendChange(globalStorageDir, taskId, entry) + seen.add(key) + } + } + await clearPendingJournal(globalStorageDir, taskId) +} + +/** + * Append one change-journal entry to the per-task changes.jsonl file. + * + * Torn-tail repair: a crash mid-append can leave the file without a trailing + * newline; appending directly would splice the new entry into the fragment and + * lose both at load time, so the tail is terminated before the new line is + * written. The append is fsynced so a durable checkpoint implies a durable + * journal entry; a crash mid-write can still tear the appended line itself, + * which loadChanges tolerates as a torn tail. Creates parent directories if + * they don't exist yet (e.g. first checkpoint). + */ +export async function appendChange(globalStorageDir: string, taskId: string, entry: ChangeJournalEntry): Promise { + const filePath = journalPath(globalStorageDir, taskId) + await fs.mkdir(path.dirname(filePath), { recursive: true }) + let payload = JSON.stringify(entry) + "\n" + let journalWasCreated = false + try { + const stat = await fs.stat(filePath) + if (stat.size > 0) { + const tail = await fs.open(filePath, "r") + try { + const last = Buffer.alloc(1) + await tail.read(last, 0, 1, stat.size - 1) + if (last[0] !== 0x0a) { + payload = "\n" + payload + } + } finally { + await tail.close() + } + } + } catch (error) { + // A missing journal is a fresh append. Any other inspection failure + // means we cannot know whether a separator is required, so propagate + // instead of appending blind and losing the entry to a merged line. + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error + } + journalWasCreated = true + } + const data = Buffer.from(payload) + const handle = await fs.open(filePath, "a") + try { + // A single write can complete partially; sync only after the whole + // record is on the handle, or a short write would persist a torn entry + // while appendChange reports success. + 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() + } + if (journalWasCreated) { + // The append created the journal file (and possibly its parent + // directory via fs.mkdir): fsync the containing directory so the new + // file's directory entry survives a crash before the append is reported + // as committed. Platforms that cannot open the directory for sync + // degrade to a warning, mirroring the rename path's tolerance list. + try { + const dirHandle = await fs.open(path.dirname(filePath), "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 creation (${syncCode}); crash durability reduced`, + ) + } + } +} + +/** + * 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. + * @internal Wired by a later unit in the split chain; remove when the chain completes. + */ +async function readJournalEntries(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 ( + parsed && + typeof parsed === "object" && + typeof (parsed as ChangeJournalEntry).path === "string" && + typeof (parsed as ChangeJournalEntry).checkpointId === "string" && + ["create", "update", "delete"].includes((parsed as ChangeJournalEntry).operation) + ) { + entries.push(parsed as ChangeJournalEntry) + } + } 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 still discarded as a torn tail — `continue` at the last index + // ends the loop either way. + continue + } + } + + return entries +} + +/** + * Load all change-journal entries for a task, first folding in a durable + * pending marker left by a crash or a failed append (see reconcilePendingJournal). + * @internal Wired by a later unit in the split chain; remove when the chain completes. + */ +export async function loadChanges(globalStorageDir: string, taskId: string): Promise { + await reconcilePendingJournal(globalStorageDir, taskId) + await reconcileIncompleteJournal(globalStorageDir, taskId) + return readJournalEntries(globalStorageDir, taskId) +} diff --git a/src/core/checkpoints/index.ts b/src/core/checkpoints/index.ts index de424b03f7..a1210b9eb2 100644 --- a/src/core/checkpoints/index.ts +++ b/src/core/checkpoints/index.ts @@ -16,6 +16,13 @@ import { DIFF_VIEW_URI_SCHEME } from "../../integrations/editor/DiffViewProvider import { CheckpointServiceOptions, RepoPerTaskCheckpointService } from "../../services/checkpoints" +import { + clearPendingJournal, + reconcilePendingJournal, + writeIncompleteJournal, + writePendingJournal, +} from "./changeJournal" + const WARNING_THRESHOLD_MS = 5000 function sendCheckpointInitWarn(task: Task, type?: "WAIT_TIMEOUT" | "INIT_TIMEOUT", timeout?: number) { @@ -113,6 +120,15 @@ export async function getCheckpointService(task: Task, { interval = 250 }: { int const service = RepoPerTaskCheckpointService.create(options) task.checkpointServiceInitializing = true await checkGitInstallation(task, service, log, provider) + if (!service.isInitialized) { + // checkGitInstallation disables checkpoints when Git is unavailable and + // logs initShadowGit failures without throwing: storing such a service + // would hand later saves (and the fast path above) an uninitialized + // handle whose isInitialized can never flip, so a failed + // initialization must resolve to no service at all. + task.checkpointServiceInitializing = false + return undefined + } task.checkpointService = service if (task.enableCheckpoints) { sendCheckpointInitWarn(task) @@ -200,6 +216,10 @@ async function checkGitInstallation( } catch (err) { log(`[Task#getCheckpointService] initShadowGit -> ${err.message}`) task.enableCheckpoints = false + // The "initialize" event never fires for a failed initShadowGit, so + // clear the flag here or later lookups wait on an initialization + // that already died. + task.checkpointServiceInitializing = false } } catch (err) { log(`[Task#getCheckpointService] Unexpected error during Git check: ${err.message}`) @@ -209,7 +229,28 @@ 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 } +} + +export async function checkpointSave( + task: Task, + force = false, + suppressMessage = false, + write?: CheckpointWriteInfo | CheckpointWriteInfo[], +) { const service = await getCheckpointService(task) if (!service) { @@ -218,9 +259,107 @@ export async function checkpointSave(task: Task, force = false, suppressMessage TelemetryService.instance.captureCheckpointCreated(task.taskId) + const writes = write ? (Array.isArray(write) ? write : [write]) : [] + const globalStorageDir = task.providerRef.deref()?.context.globalStorageUri.fsPath + const journalStorageDir = writes.length > 0 ? globalStorageDir : undefined + + // B2: the checkpoint and its change-journal entries are one transaction. The pending + // marker is recorded before the Git commit and is a hard prerequisite: a committed + // checkpoint whose marker never landed could lose its journal state, so when the + // marker cannot be written durably this save does not run at all. + if (journalStorageDir !== undefined) { + try { + // A marker left by a crash or a failed append is folded in before this save + // overwrites the marker, so its entries are not lost. + await reconcilePendingJournal(journalStorageDir, task.taskId) + await writePendingJournal(journalStorageDir, task.taskId, { + state: "awaiting-checkpoint", + createdAt: Date.now(), + }) + } catch (err) { + console.error( + "[Task#checkpointSave] not running the checkpoint: the pending change journal could not be recorded", + err, + ) + return + } + } + // Start the checkpoint process in the background. return service .saveCheckpoint(`Task: ${task.taskId}, Time: ${Date.now()}`, { allowEmpty: force, suppressMessage }) + .then(async (result) => { + // Only a real commit produces entries (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. + if (journalStorageDir !== undefined) { + if (result?.commit) { + const entries = writes.map((w) => ({ + path: w.path, + operation: w.operation, + checkpointId: result.commit, + ...(w.diffStats ? { diffStats: w.diffStats } : {}), + })) + // After the commit the entries must become recoverable. Retry the durable + // 'ready' promotion; when it still fails, append the entries directly (the + // earlier 'awaiting' marker stays as the recovery record for the next read). + // Returning success with neither a ready marker nor appended entries would + // recreate the committed-checkpoint-without-journal state. + let promoted = false + for (let attempt = 0; attempt < 2 && !promoted; attempt++) { + try { + await writePendingJournal(journalStorageDir, task.taskId, { + state: "ready", + createdAt: Date.now(), + entries, + }) + promoted = true + } catch (promotionError) { + console.error( + "[Task#checkpointSave] failed to promote the pending change journal", + promotionError, + ) + } + } + if (promoted) { + try { + // Appends in order, then clears the marker; idempotent if a previous + // attempt already appended part of the entries. + await reconcilePendingJournal(journalStorageDir, task.taskId) + } catch (reconcileError) { + console.error( + "[Task#checkpointSave] failed to reconcile the pending change journal", + reconcileError, + ) + } + } else { + // The ready marker could not be written. Persist the complete entries in the + // dedicated incomplete marker (which the next read folds in) and surface the + // failure: a committed checkpoint must never be reported as successful while + // its journal state is missing. + let incompletePersisted = false + try { + await writeIncompleteJournal(journalStorageDir, task.taskId, entries) + incompletePersisted = true + } catch (incompleteError) { + console.error( + "[Task#checkpointSave] failed to persist the incomplete change journal marker", + incompleteError, + ) + } + throw new Error( + incompletePersisted + ? "The checkpoint committed but its journal entries could only be recorded in the incomplete marker" + : "The checkpoint committed but its journal entries could not be persisted", + ) + } + } else { + // No commit means no journal entry is claimable. + await clearPendingJournal(journalStorageDir, task.taskId).catch(() => {}) + } + } + return result + }) .catch((err) => { console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err) task.enableCheckpoints = false diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index dc7880644c..3df186a350 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -468,6 +468,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 @@ -3833,6 +3836,47 @@ 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) { + let baselineEnabled: boolean | undefined + try { + baselineEnabled = (await this.providerRef.deref()?.getState())?.perWriteCheckpoints + } catch (error) { + // A rejected state lookup must not abort task startup: fall back + // to the default-on semantics used when the setting is absent. + console.error("[Task] task-start baseline settings read failed:", error) + } + // 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) { + this.taskStartBaselineDone = true + } else { + try { + // allowEmpty=true so a clean workspace still produces the baseline + // commit; awaited so the first per-write checkpoint cannot interleave. + const baseline = await this.checkpointSave(true, true) + // The flag is set only once the baseline actually committed: a save + // that no-ops behind a failed or timed-out service initialization is + // retried on a later loop entry instead of being marked done without + // a rollback point. + if (baseline?.commit) { + this.taskStartBaselineDone = true + } + } catch (error) { + // The baseline is best-effort and runs before TaskStarted: a + // checkpoint error must not abort task startup (startTask rethrows + // non-abort errors from initiateTaskLoop). Log and continue; the + // next loop entry retries the baseline. + console.error("[Task] task-start checkpoint baseline failed:", error) + } + } + } + let nextUserContent = userContent let includeFileDetails = true @@ -4266,7 +4310,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: equivalent mutant, only tool_use and mcp_tool_use entries carry an id, so the type check cannot change entry.id === event.id (entry.type === "tool_use" || entry.type === "mcp_tool_use") && (entry as { id?: string }).id === event.id, ) @@ -4319,7 +4362,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: equivalent mutant, event.name is always a string here, so the legacy nameless branch is unreachable if (event.name !== undefined) { streamingKey = NativeToolCallParser.makeStreamingKey(event.id, event.name) } else { @@ -4336,7 +4378,6 @@ export class Task extends EventEmitter implements TaskLike { } const partialToolUse = - // Stryker disable next-line ConditionalExpression: equivalent mutant, streamingKey is always a string here (makeStreamingKey), so the null side is unreachable streamingKey === undefined ? null : NativeToolCallParser.processStreamingChunk( @@ -4345,7 +4386,6 @@ export class Task extends EventEmitter implements TaskLike { nativeToolCallParserScope, ) - // Stryker disable next-line ConditionalExpression, EqualityOperator: equivalent mutants, streamingKey is always defined here, so both conditions reduce to the partialToolUse check 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. @@ -4742,7 +4782,7 @@ 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: equivalent mutant, finalizeRawChunks emits end events with a string name, so the ?? event.id fallback is unreachable + let eventName = event.name ?? event.id let dedupKey = NativeToolCallParser.makeStreamingKey(event.id, eventName) @@ -4759,13 +4799,12 @@ 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: equivalent mutants, the both-miss case is unreachable via the raw-chunk pipeline + 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) @@ -6445,6 +6484,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 3066613263..153032a34d 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -6824,6 +6824,230 @@ 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) + // A committed baseline: the done flag is set only on a real + // commit, so the double must resolve one for the once-per-instance + // assertion to exercise the flag flip. + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue({ commit: "baseline-commit" }) + 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() + }) + + it("retries the baseline on a later loop entry when a save produced no commit", async () => { + // getCheckpointService can resolve to no service (init failure or a + // bounded init wait timing out): the save then completes without a + // commit, and the baseline must not be marked done - a later loop + // entry gets to retry it instead of losing it forever. + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "retry baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi + .spyOn(task, "checkpointSave") + .mockResolvedValueOnce(undefined) + .mockResolvedValue({ commit: "baseline-commit" }) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + expect(saveSpy).toHaveBeenCalledOnce() + + await taskAccess.initiateTaskLoop([]) + expect(saveSpy).toHaveBeenCalledTimes(2) + }) + + it("starts the task default-on when the baseline settings read rejects", async () => { + // ClineProvider.getState awaits custom-mode loading and can reject: + // a failed settings read must not stop task startup. Default-on + // semantics apply (the setting is treated as absent) and the loop + // still runs. + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "rejected-read baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue({ commit: "baseline-commit" }) + vi.spyOn(mockProvider, "getState").mockRejectedValueOnce(new Error("custom mode load failed")) + + const requestSpy = vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).toHaveBeenCalledOnce() + expect(saveSpy).toHaveBeenCalledWith(true, true) + expect(requestSpy).toHaveBeenCalled() + }) + }) + describe("start()", () => { it("should be a no-op if the task was already started in the constructor", () => { const task = new Task({ @@ -7747,6 +7971,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..8348b27fb4 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -6,11 +6,12 @@ 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 { formatResponse } from "../prompts/responses" 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 { sanitizeUnifiedDiff, computeDiffStats, type DiffStats } from "../diff/stats" import { BaseTool, ToolCallbacks } from "./BaseTool" import type { ToolUse } from "../../shared/tools" import { parsePatch, ParseError, processAllHunks } from "./apply-patch" @@ -20,6 +21,20 @@ 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. `diffStats` carries the approval-diff statistics of a written file + * so the journal entry can report the size of the change. + */ +interface ApplyPatchFileOpResult { + succeeded: boolean + wrote: boolean + diffStats?: DiffStats +} + export class ApplyPatchTool extends BaseTool<"apply_patch"> { readonly name = "apply_patch" as const @@ -56,6 +71,35 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const { patch } = params const { handleError, pushToolResult } = callbacks + // Set once the success-path flush actually proceeds to checkpointSave, so + // the catch below does not persist the same writes twice while a + // settings-read rejection before that decision leaves the catch flush + // armed with default-on semantics. + let writeMetadataFlushed = false + // Cached perWriteCheckpoints lookup shared by the success-path flush and + // the catch flush: the catch must not call getState() again, because a + // rejecting settings read must not skip persisting writes already on disk. + let perWriteCheckpoints: boolean | undefined + const successfulChanges: { change: ApplyPatchFileChange; diffStats?: DiffStats }[] = [] + // Journal payload builder shared by the success-path flush and the + // catch-path flush for writes recorded before an unexpected throw. + const journalEntries = () => + successfulChanges.flatMap(({ change, diffStats }) => { + const written = { + path: change.movePath ?? change.path, + operation: change.type === "add" ? ("create" as const) : change.type, + diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, + } + // A completed move also DELETES its source: journal that deletion + // after the destination write so a step rollback restores the + // removed file from its pre-step state. A move whose source unlink + // failed left the source on disk and gets no deletion entry. + if (change.movePath && change.moveSourceDeleted) { + return [written, { path: change.path, operation: "delete" as const, diffStats: undefined }] + } + return [written] + }) + try { // Validate required parameters if (!patch) { @@ -102,7 +146,14 @@ 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); each + // write is recorded through the onWrote callback at the moment it lands + // on disk, so a throw from post-write bookkeeping cannot lose the + // journal entry for a completed write (a no-op update never calls the + // callback). 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 +163,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,19 +176,114 @@ 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, + (diffStats) => successfulChanges.push({ change, diffStats }), + ) + patchSucceeded = addResult.succeeded && patchSucceeded } else if (change.type === "delete") { // Delete file - await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected) + const deleteResult = await this.handleDeleteFile( + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + () => successfulChanges.push({ change }), + ) + patchSucceeded = deleteResult.succeeded && patchSucceeded } 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, + (diffStats) => successfulChanges.push({ change, diffStats }), + ) + patchSucceeded = updateResult.succeeded && patchSucceeded } } - task.consecutiveMistakeCount = 0 + // 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) { + perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints + if (perWriteCheckpoints !== false) { + writeMetadataFlushed = true + // 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, and a completed move + // additionally journals its source deletion. Each entry carries the + // approval-diff statistics the handler computed. + // Await so the checkpoint (staging + commit + journal) + // completes before queued messages are processed below; + // otherwise a queued write's own checkpoint could interleave + // with this commit. + await checkpointSave(task, false, true, journalEntries()).catch(() => {}) + } + } + + // B2: deferred from the per-file handlers — a queued write must not + // start until the post-patch checkpoint above has persisted. The + // handlers used to process queued messages right after each + // successful write; a write happened if and only if + // `successfulChanges` is non-empty, so one call here preserves that. + if (successfulChanges.length > 0) { + task.processQueuedMessages() + } } catch (error) { - await handleError("apply patch", error as Error) + // A handler that throws after recording its write (post-write + // bookkeeping such as trackFileContext) skips the success-path flush + // above. Persist the accumulated write metadata here, awaited, before + // the generic error handling: the writes are on disk and the journal + // must document them. A flush that already ran (or was skipped by the + // setting) is not attempted twice. + let reportedError: unknown = error + if (!writeMetadataFlushed && successfulChanges.length > 0) { + if (perWriteCheckpoints !== false) { + let flushError: unknown + await checkpointSave(task, false, true, journalEntries()).catch((flushFailure) => { + flushError = flushFailure + }) + if (flushError !== undefined) { + // The completed write lacks rollback metadata: report the + // persistence failure alongside the bookkeeping error without + // replacing it as the reported failure. + const flushMessage = flushError instanceof Error ? flushError.message : String(flushError) + const baseMessage = error instanceof Error ? error.message : String(error) + console.error("[ApplyPatchTool] catch-path checkpoint flush failed:", flushError) + reportedError = new Error(`${baseMessage} (checkpoint flush also failed: ${flushMessage})`, { + cause: error, + }) + } + } + } + await handleError("apply patch", reportedError as Error) await task.diffViewProvider.reset() } } @@ -144,7 +295,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + onWrote?: (diffStats?: DiffStats) => void, + ): Promise { const { askApproval, pushToolResult } = callbacks // Check if file already exists @@ -155,7 +307,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 || "" @@ -209,7 +361,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 @@ -219,6 +371,11 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } + // Record the completed write before post-write bookkeeping can fail: + // a throw from trackFileContext must not lose the journal entry for + // a file that is already on disk. + onWrote?.(diffStats) + // Track file edit operation await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) task.didEditFile = true @@ -226,7 +383,9 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, true) pushToolResult(message) await task.diffViewProvider.reset() - task.processQueuedMessages() + // B2: processQueuedMessages is deferred to execute()'s post-patch hook + // so it runs only after the post-patch checkpoint has completed. + return { succeeded: true, wrote: true, diffStats } } private async handleDeleteFile( @@ -235,7 +394,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + onWrote?: () => void, + ): Promise { const { askApproval, pushToolResult } = callbacks // Check if file exists @@ -246,7 +406,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) @@ -268,7 +428,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (!didApprove) { pushToolResult("Delete operation was rejected by the user.") - return + return { succeeded: false, wrote: false } } // Delete the file @@ -278,12 +438,17 @@ 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 } } + // The deletion is complete; record it before the result handoff can fail. + onWrote?.() + task.didEditFile = true pushToolResult(`Successfully deleted ${relPath}`) - task.processQueuedMessages() + // B2: processQueuedMessages is deferred to execute()'s post-patch hook + // so it runs only after the post-patch checkpoint has completed. + return { succeeded: true, wrote: true } } private async handleUpdateFile( @@ -293,9 +458,14 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + onWrote?: (diffStats?: DiffStats) => void, + ): 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 +474,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 +488,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { // Generate and validate diff const diff = formatResponse.createPrettyPatch(relPath, originalContent, newContent) if (!diff) { + // 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. pushToolResult(`No changes needed for '${relPath}'`) await task.diffViewProvider.reset() - return + return { succeeded: true, wrote: false } } // Check experiment settings @@ -366,7 +540,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 +553,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 +565,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 +577,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,13 +596,26 @@ 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) + change.moveSourceDeleted = true } 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)) } + // The destination write is on disk whether or not the source unlink + // succeeded; record it before bookkeeping can fail. + onWrote?.(diffStats) + await task.fileContextTracker.trackFileContext(change.movePath, "roo_edited" as RecordSource) } else { // Save changes to the same file @@ -438,6 +625,9 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } + // Record the completed write before post-write bookkeeping can fail. + onWrote?.(diffStats) + await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) } @@ -446,7 +636,15 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) pushToolResult(message) await task.diffViewProvider.reset() - task.processQueuedMessages() + // B2: processQueuedMessages is deferred to execute()'s post-patch hook + // so it runs only after the post-patch checkpoint has completed. + 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, diffStats } + } + return { succeeded: true, wrote: true, diffStats } } 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..e107b7957d 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,7 @@ 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 type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -392,6 +393,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, @@ -463,6 +465,21 @@ export class EditFileTool extends BaseTool<"edit_file"> { pushToolResult(message + replacementInfo) + 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. + // Await so the checkpoint (staging + commit + journal) completes + // before the diff state resets and queued work proceeds; fire-and-forget + // lets a queued write's own checkpoint interleave and lets shutdown + // lose the commit entirely. + await checkpointSave(task, false, true, { + path: relPath, + operation: isNewFile ? "create" : "update", + diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, + }).catch(() => {}) + } + await task.diffViewProvider.reset() this.resetPartialState() diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 0c5c80abb9..642762a77d 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,8 @@ 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 type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -102,11 +103,16 @@ 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. + let approvalDiffStats: DiffStats | null = null + if (isPreventFocusDisruptionEnabled) { task.diffViewProvider.editType = fileExists ? "modify" : "create" if (fileExists) { @@ -120,10 +126,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) + approvalDiffStats = computeDiffStats(unified) const 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 +158,11 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) + approvalDiffStats = computeDiffStats(unified) const completeMessage = JSON.stringify({ ...sharedMessageProps, content: unified, - diffStats: computeDiffStats(unified) || undefined, + diffStats: approvalDiffStats || undefined, } satisfies ClineSayTool) const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) @@ -177,6 +185,21 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) + 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) finishes before the next queued write starts; + // otherwise two writes can collapse into one commit. + await checkpointSave(task, false, true, { + path: relPath, + operation: fileExists ? "update" : "create", + diffStats: approvalDiffStats + ? { additions: approvalDiffStats.added, deletions: approvalDiffStats.removed } + : undefined, + }).catch(() => {}) + } + await task.diffViewProvider.reset() this.resetPartialState() diff --git a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts index eb89838cad..8c68b77e48 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,636 @@ 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("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 patches that wrote at least one file (B1/B2)", () => { + 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() + // B2: the journal entry for a successful add carries the + // approval-diff statistics (two added lines, no removals). + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/new.ts", operation: "create", diffStats: { additions: 2, deletions: 0 } }, + ]) + }) + + 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() + }) + + 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() + // B2: the journal entries for a successful move are the destination + // write (keyed by the final path, carrying the approval-diff + // statistics) and the source deletion (a pure delete record that + // lets a step rollback restore the removed file). + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/moved.ts", operation: "update", diffStats: { additions: 1, deletions: 1 } }, + { path: "src/test.ts", operation: "delete", diffStats: undefined }, + ]) + }) + + 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() + // B2: the journal entry for a successful update carries the + // approval-diff statistics. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/test.ts", operation: "update", diffStats: { additions: 1, deletions: 1 } }, + ]) + }) + + it("journals the recorded write when a handler throws after its write", async () => { + // Post-write bookkeeping (trackFileContext) fails after the file is + // already on disk: the write recorded at write time must still receive + // its checkpoint and journal entry through the catch-path flush. + vi.mocked(mockTask.fileContextTracker.trackFileContext).mockRejectedValueOnce(new Error("tracker boom")) + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockedCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/test.ts", operation: "update", diffStats: { additions: 1, deletions: 1 } }, + ]) + expect(mockHandleError).toHaveBeenCalledWith("apply patch", expect.anything()) + }) + + it("flushes the recorded write from the catch path when the settings read rejects", async () => { + // The handler's settings lookup succeeds; the success-path flush's + // lookup rejects. The flush flag must not have been set before that + // rejection, so the catch path still persists the write already on + // disk using the cached (undefined = default-on) setting. + const getState = vi.fn().mockResolvedValueOnce({}).mockRejectedValueOnce(new Error("state boom")) + mockTask.providerRef = { deref: vi.fn().mockReturnValue({ getState }) } as unknown as Task["providerRef"] + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockedCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/test.ts", operation: "update", diffStats: { additions: 1, deletions: 1 } }, + ]) + }) + + it("reports the catch-path flush failure alongside the bookkeeping error", async () => { + vi.mocked(mockTask.fileContextTracker.trackFileContext).mockRejectedValueOnce(new Error("tracker boom")) + mockedCheckpointSave.mockRejectedValueOnce(new Error("journal boom")) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The user must see that the completed write lacks rollback + // metadata: the reported error keeps the original message and + // appends the persistence failure instead of replacing it. + expect(mockHandleError).toHaveBeenCalledTimes(1) + const reported = mockHandleError.mock.calls[0][1] as Error + expect(reported.message).toContain("tracker boom") + expect(reported.message).toContain("checkpoint flush also failed: journal boom") + }) + + 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() + // B2: each journal entry carries the approval-diff statistics of + // its file change. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/a.ts", operation: "create", diffStats: { additions: 1, deletions: 0 } }, + { path: "src/b.ts", operation: "update", diffStats: { additions: 1, deletions: 1 } }, + ]) + }) + + 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 } }, + ]) + }) + + 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, + // with the written file's diff statistics. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/second.ts", operation: "create", diffStats: { additions: 1, deletions: 0 } }, + ]) + }) + + 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. + // The single-entry payload below also guards that the source + // deletion is NOT journaled: the source was never removed. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + expect.objectContaining({ path: "src/new-location.ts", operation: "update" }), + ]) + // B2: even a failed move's destination write carries the + // approval-diff statistics in the journal payload. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/new-location.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + }, + ]) + 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..ceefd1e11b 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,93 @@ 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. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + }) + }) + + it("awaits the checkpoint before resetting the diff state", async () => { + // Fire-and-forget would let diffViewProvider.reset() and the queued-work + // handoff race an unfinished checkpoint commit (and lose it at + // shutdown): the save must complete before the reset. + let releaseCheckpoint: () => void = () => {} + mockedCheckpointSave.mockImplementationOnce( + () => new Promise((resolve) => (releaseCheckpoint = resolve)), + ) + let settled = false + const run = executeEditFileTool({}).then(() => { + settled = true + }) + await vi.waitFor(() => expect(mockedCheckpointSave).toHaveBeenCalledOnce()) + expect(settled).toBe(false) + expect(mockTask.diffViewProvider.reset).not.toHaveBeenCalled() + releaseCheckpoint() + await run + expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() + }) + + 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 }, + }) + }) + + 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. + 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 --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 7cb9d01585..70f6be986c 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -8,6 +8,8 @@ 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" vi.mock("path", async () => { @@ -89,6 +91,10 @@ vi.mock("../../ignore/RooIgnoreController", () => ({ }, })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + describe("writeToFileTool", () => { // Test data const testFilePath = "test/file.txt" @@ -156,6 +162,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 +480,123 @@ 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. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + }) + }) + + 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("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. + 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 }, + }) + }) + + 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", + }) + }) + }) }) diff --git a/src/core/tools/apply-patch/apply.ts b/src/core/tools/apply-patch/apply.ts index 4ab377f732..81d0c3caac 100644 --- a/src/core/tools/apply-patch/apply.ts +++ b/src/core/tools/apply-patch/apply.ts @@ -29,6 +29,12 @@ export interface ApplyPatchFileChange { originalContent?: string /** New content (for add/update) */ newContent?: string + /** + * Set by the tool handler when a move's source unlink succeeded: the + * change journal then records the source deletion alongside the + * destination write so a step rollback can restore the removed file. + */ + moveSourceDeleted?: boolean } /** diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 3d88df632b..84eae7ac26 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -55,6 +55,7 @@ import { ORGANIZATION_ALLOW_ALL, DEFAULT_MODES, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_PER_WRITE_CHECKPOINTS, getModelId, isRetiredProvider, modelIdKeys, @@ -2747,6 +2748,7 @@ export class ClineProvider ttsSpeed, enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, soundVolume, writeDelayMs, diffFuzzyThreshold, @@ -2916,6 +2918,7 @@ export class ClineProvider ttsSpeed: ttsSpeed ?? 1.0, enableCheckpoints: enableCheckpoints ?? true, checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, shouldShowAnnouncement: telemetrySetting !== "unset" && lastShownAnnouncementId !== this.latestAnnouncementId, allowedCommands: mergedAllowedCommands, @@ -3157,6 +3160,7 @@ 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, 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 93543435e2..ec9892a844 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1209,6 +1209,7 @@ describe("ClineProvider", () => { soundEnabled: false, ttsEnabled: false, enableCheckpoints: false, + perWriteCheckpoints: false, writeDelayMs: 1000, mcpEnabled: true, mode: defaultModeSlug, @@ -2168,6 +2169,48 @@ 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("getStateToPostToWebview returns the saved blanket auto-deny setting", async () => { await provider.resolveWebviewView(mockWebviewView) await provider.contextProxy.setValue("alwaysDenyUnapprovedCommands", true) diff --git a/webview-ui/src/components/settings/CheckpointSettings.tsx b/webview-ui/src/components/settings/CheckpointSettings.tsx index dd28f6615f..7ea12ef873 100644 --- a/webview-ui/src/components/settings/CheckpointSettings.tsx +++ b/webview-ui/src/components/settings/CheckpointSettings.tsx @@ -13,17 +13,20 @@ import { DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, MAX_CHECKPOINT_TIMEOUT_SECONDS, MIN_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_PER_WRITE_CHECKPOINTS, } from "@roo-code/types" type CheckpointSettingsProps = HTMLAttributes & { enableCheckpoints?: boolean checkpointTimeout?: number - setCachedStateField: SetCachedStateField<"enableCheckpoints" | "checkpointTimeout"> + perWriteCheckpoints?: boolean + setCachedStateField: SetCachedStateField<"enableCheckpoints" | "checkpointTimeout" | "perWriteCheckpoints"> } export const CheckpointSettings = ({ enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, setCachedStateField, ...props }: CheckpointSettingsProps) => { @@ -33,6 +36,22 @@ export const CheckpointSettings = ({ {t("settings:sections.checkpoints")}
+ + { + setCachedStateField("perWriteCheckpoints", e.target.checked) + }}> + {t("settings:checkpoints.perWrite.label")} + +
+ {t("settings:checkpoints.perWrite.description")} +
+ + (({ onDone, t autoCondenseContextPercent, enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, experiments, maxOpenTabsContext, maxWorkspaceFiles, @@ -413,6 +415,7 @@ const SettingsView = forwardRef(({ onDone, t ttsSpeed, enableCheckpoints: enableCheckpoints ?? false, checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, writeDelayMs, diffFuzzyThreshold, terminalShellIntegrationTimeout: terminalShellIntegrationTimeout ?? 30_000, @@ -852,6 +855,7 @@ 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..b08c15ace7 --- /dev/null +++ b/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx @@ -0,0 +1,125 @@ +// 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" + } + 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) + }) +}) diff --git a/webview-ui/src/context/ExtensionStateContext.tsx b/webview-ui/src/context/ExtensionStateContext.tsx index c4fa00f479..c6cd635ee7 100644 --- a/webview-ui/src/context/ExtensionStateContext.tsx +++ b/webview-ui/src/context/ExtensionStateContext.tsx @@ -213,6 +213,7 @@ export const createInitialExtensionState = (): ExtensionState => ({ ttsEnabled: false, ttsSpeed: 1.0, enableCheckpoints: true, + perWriteCheckpoints: true, 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..df5ffc9943 100644 --- a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx +++ b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx @@ -426,6 +426,7 @@ describe("mergeExtensionState", () => { taskHistory: [], shouldShowAnnouncement: false, enableCheckpoints: true, + perWriteCheckpoints: true, // Matches the shared DEFAULT_WRITE_DELAY_MS (pre-hydration placeholder // must not disagree with the extension's own fallback). writeDelayMs: 0, @@ -498,6 +499,7 @@ describe("mergeExtensionState", () => { taskHistory: [], shouldShowAnnouncement: false, enableCheckpoints: true, + perWriteCheckpoints: true, // 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 d39d8edd6a..de8e5b095f 100644 --- a/webview-ui/src/i18n/locales/ca/settings.json +++ b/webview-ui/src/i18n/locales/ca/settings.json @@ -734,6 +734,10 @@ "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 operació d’escriptura reeixida; un pegat de diversos fitxers genera un únic punt de control." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/de/settings.json b/webview-ui/src/i18n/locales/de/settings.json index 983db1df5d..86bb801ee0 100644 --- a/webview-ui/src/i18n/locales/de/settings.json +++ b/webview-ui/src/i18n/locales/de/settings.json @@ -734,6 +734,10 @@ "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": "Erstellt nach jedem erfolgreichen Schreibvorgang einen Checkpoint-Schnappschuss; ein Patch mit mehreren Dateien erzeugt genau einen Checkpoint." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/en/settings.json b/webview-ui/src/i18n/locales/en/settings.json index ea924ce79e..215ba566ff 100644 --- a/webview-ui/src/i18n/locales/en/settings.json +++ b/webview-ui/src/i18n/locales/en/settings.json @@ -814,6 +814,10 @@ "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 each successful write operation; a multi-file patch produces a single checkpoint." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/es/settings.json b/webview-ui/src/i18n/locales/es/settings.json index 57612bf3e5..2d148cde55 100644 --- a/webview-ui/src/i18n/locales/es/settings.json +++ b/webview-ui/src/i18n/locales/es/settings.json @@ -734,6 +734,10 @@ "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 operación de escritura exitosa; un parche de varios archivos produce un único punto de control." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/fr/settings.json b/webview-ui/src/i18n/locales/fr/settings.json index a91a5137d3..287a7b861e 100644 --- a/webview-ui/src/i18n/locales/fr/settings.json +++ b/webview-ui/src/i18n/locales/fr/settings.json @@ -734,6 +734,10 @@ "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 opération d’écriture réussie ; un correctif multi-fichiers produit un seul point de contrôle." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/hi/settings.json b/webview-ui/src/i18n/locales/hi/settings.json index bc6e973623..be08e00920 100644 --- a/webview-ui/src/i18n/locales/hi/settings.json +++ b/webview-ui/src/i18n/locales/hi/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "स्वचालित चेकपॉइंट सक्षम करें", "description": "जब सक्षम होता है, तो Zoo कार्य निष्पादन के दौरान स्वचालित रूप से चेकपॉइंट बनाएगा, जिससे परिवर्तनों की समीक्षा करना या पहले की स्थितियों पर वापस जाना आसान हो जाएगा। <0>अधिक जानें" + }, + "perWrite": { + "label": "हर फ़ाइल लिखने के बाद चेकपॉइंट", + "description": "प्रत्येक सफल लेखन कार्रवाई के बाद एक चेकपॉइंट स्नैपशॉट रिकॉर्ड करें; एक मल्टी-फ़ाइल पैच केवल एक चेकपॉइंट बनाता है।" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/id/settings.json b/webview-ui/src/i18n/locales/id/settings.json index 3ed57f373e..f59460b5b2 100644 --- a/webview-ui/src/i18n/locales/id/settings.json +++ b/webview-ui/src/i18n/locales/id/settings.json @@ -734,6 +734,10 @@ "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": "Rekam snapshot titik pemeriksaan setelah setiap operasi tulis berhasil; patch multi-file menghasilkan satu titik pemeriksaan." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/it/settings.json b/webview-ui/src/i18n/locales/it/settings.json index b708304e63..3a1ef7898e 100644 --- a/webview-ui/src/i18n/locales/it/settings.json +++ b/webview-ui/src/i18n/locales/it/settings.json @@ -734,6 +734,10 @@ "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 un’istantanea del punto di controllo dopo ogni operazione di scrittura riuscita; una patch con più file produce un unico punto di controllo." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ja/settings.json b/webview-ui/src/i18n/locales/ja/settings.json index f6959b50dc..286b55cf50 100644 --- a/webview-ui/src/i18n/locales/ja/settings.json +++ b/webview-ui/src/i18n/locales/ja/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "自動チェックポイントを有効化", "description": "有効にすると、Zooはタスク実行中に自動的にチェックポイントを作成し、変更の確認や以前の状態への復帰を容易にします。 <0>詳細情報" + }, + "perWrite": { + "label": "ファイルの書き込みごとにチェックポイント", + "description": "各書き込み操作の成功後にチェックポイントスナップショットを記録します。複数ファイルのパッチは単一のチェックポイントを作成します。" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ko/settings.json b/webview-ui/src/i18n/locales/ko/settings.json index 6cadbd8a3b..14226cede5 100644 --- a/webview-ui/src/i18n/locales/ko/settings.json +++ b/webview-ui/src/i18n/locales/ko/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "자동 체크포인트 활성화", "description": "활성화되면 Zoo는 작업 실행 중에 자동으로 체크포인트를 생성하여 변경 사항을 검토하거나 이전 상태로 되돌리기 쉽게 합니다. <0>더 알아보기" + }, + "perWrite": { + "label": "파일을 쓸 때마다 체크포인트", + "description": "각 쓰기 작업이 성공할 때마다 체크포인트 스냅샷을 기록합니다. 다중 파일 패치는 단일 체크포인트를 생성합니다." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/nl/settings.json b/webview-ui/src/i18n/locales/nl/settings.json index 992e9c53e0..c3d14e4416 100644 --- a/webview-ui/src/i18n/locales/nl/settings.json +++ b/webview-ui/src/i18n/locales/nl/settings.json @@ -734,6 +734,10 @@ "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": "Leg na elke succesvolle schrijfbewerking een checkpointmomentopname vast; een patch met meerdere bestanden produceert één checkpoint." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/pl/settings.json b/webview-ui/src/i18n/locales/pl/settings.json index 459675b0e1..ac934f6b5d 100644 --- a/webview-ui/src/i18n/locales/pl/settings.json +++ b/webview-ui/src/i18n/locales/pl/settings.json @@ -734,6 +734,10 @@ "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": "Zapisuje migawkę punktu kontrolnego po każdej udanej operacji zapisu; patch obejmujący wiele plików tworzy jeden punkt kontrolny." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/pt-BR/settings.json b/webview-ui/src/i18n/locales/pt-BR/settings.json index 500babaaa8..9001d7c65c 100644 --- a/webview-ui/src/i18n/locales/pt-BR/settings.json +++ b/webview-ui/src/i18n/locales/pt-BR/settings.json @@ -734,6 +734,10 @@ "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 operação de gravação bem-sucedida; um patch com vários arquivos produz um único ponto de verificação." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ru/settings.json b/webview-ui/src/i18n/locales/ru/settings.json index 053ff0684d..e982e198f1 100644 --- a/webview-ui/src/i18n/locales/ru/settings.json +++ b/webview-ui/src/i18n/locales/ru/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Включить автоматические контрольные точки", "description": "Если включено, Zoo будет автоматически создавать контрольные точки во время выполнения задач, что упрощает просмотр изменений или возврат к предыдущим состояниям. <0>Подробнее" + }, + "perWrite": { + "label": "Контрольная точка после каждой записи файла", + "description": "Записывает снимок контрольной точки после каждой успешной операции записи; патч с несколькими файлами создаёт одну контрольную точку." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/tr/settings.json b/webview-ui/src/i18n/locales/tr/settings.json index b37f28d44d..9a1bc8663a 100644 --- a/webview-ui/src/i18n/locales/tr/settings.json +++ b/webview-ui/src/i18n/locales/tr/settings.json @@ -734,6 +734,10 @@ "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": "Her başarılı yazma işleminden sonra bir kontrol noktası anlık görüntüsü kaydeder; çok dosyalı bir yama tek bir kontrol noktası üretir." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/vi/settings.json b/webview-ui/src/i18n/locales/vi/settings.json index 94843217ae..ede0d6816b 100644 --- a/webview-ui/src/i18n/locales/vi/settings.json +++ b/webview-ui/src/i18n/locales/vi/settings.json @@ -734,6 +734,10 @@ "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 ảnh chụp điểm kiểm tra sau mỗi thao tác ghi thành công; một bản vá nhiều tệp chỉ tạo một điểm kiểm tra." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/zh-CN/settings.json b/webview-ui/src/i18n/locales/zh-CN/settings.json index 8be0fe1532..66cd5c3bd3 100644 --- a/webview-ui/src/i18n/locales/zh-CN/settings.json +++ b/webview-ui/src/i18n/locales/zh-CN/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "启用自动存档点", "description": "开启后自动创建任务存档点，方便回溯修改。 <0>了解更多" + }, + "perWrite": { + "label": "每次文件写入后创建存档点", + "description": "在每次成功写入操作后记录检查点快照；多文件补丁只会生成一个检查点。" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/zh-TW/settings.json b/webview-ui/src/i18n/locales/zh-TW/settings.json index 65eb5c67a5..c6510db2e1 100644 --- a/webview-ui/src/i18n/locales/zh-TW/settings.json +++ b/webview-ui/src/i18n/locales/zh-TW/settings.json @@ -761,6 +761,10 @@ "enable": { "label": "啟用自動檢查點", "description": "啟用後，Zoo 將在工作執行期間自動建立檢查點，方便檢視變更或回到較早的狀態。 <0>了解更多" + }, + "perWrite": { + "label": "每次檔案寫入後建立檢查點", + "description": "在每次成功的寫入操作後記錄檢查點快照；多檔案修補程式只會產生一個檢查點。" } }, "notifications": {