diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index 95f246dbe7..3896c711c0 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -12,6 +12,7 @@ import { } from "./provider-settings.js" import { telemetrySettingsSchema } from "./telemetry.js" import { toolNamesSchema } from "./tool.js" +import { changeCardDetailSchema, type ChangeCardDetail } from "./message.js" import { type Keys } from "./type-fu.js" import { languagesSchema } from "./vscode.js" @@ -99,6 +100,21 @@ export const MAX_CHECKPOINT_TIMEOUT_SECONDS = 60 */ export const DEFAULT_CHECKPOINT_TIMEOUT_SECONDS = 15 +/** + * Whether per-write checkpoints and task-start baseline are enabled by default. + * Master switch for the B cluster of checkpoint features. + * @default true + */ +export const DEFAULT_PER_WRITE_CHECKPOINTS = true + +/** + * Default detail level for per-step change cards (B3a). + * "summary" keeps cards compact (file list with +/− counts; the UI fetches + * diffs lazily); "full" carries the unified diff inline per file. + * @default "summary" + */ +export const DEFAULT_CHANGE_CARD_DETAIL: ChangeCardDetail = "summary" + /** * GlobalSettings */ @@ -200,6 +216,19 @@ export const globalSettingsSchema = z.object({ .min(MIN_CHECKPOINT_TIMEOUT_SECONDS) .max(MAX_CHECKPOINT_TIMEOUT_SECONDS) .optional(), + /** + * Whether to record a shadow-git checkpoint after every successful write_to_file, + * edit_file, and apply_patch (per-write checkpoints), plus a task-start baseline. + * @default true + */ + perWriteCheckpoints: z.boolean().optional(), + /** + * Detail level for per-step change cards: "full" includes the unified diff + * inline for every changed file, "summary" carries only the file list with + * +/− counts (diffs are fetched lazily by the UI). + * @default "summary" + */ + changeCardDetail: changeCardDetailSchema.optional(), ttsEnabled: z.boolean().optional(), ttsSpeed: z.number().optional(), diff --git a/packages/types/src/message.ts b/packages/types/src/message.ts index 28d5af82ac..b7940d00a4 100644 --- a/packages/types/src/message.ts +++ b/packages/types/src/message.ts @@ -134,6 +134,7 @@ export function isNonBlockingAsk(ask: ClineAsk): ask is NonBlockingAsk { * - `mcp_server_response`: Response received from MCP server * - `subtask_result`: Result of a completed subtask * - `checkpoint_saved`: Indicates a checkpoint has been saved + * - `change_card`: Per-step change card summarizing the files a completed tool step wrote (B3a) * - `rooignore_error`: Error related to .rooignore file processing * - `diff_error`: Error occurred while applying a diff/patch * - `condense_context`: Context condensation/summarization has started @@ -162,6 +163,7 @@ export const clineSays = [ "mcp_server_response", "subtask_result", "checkpoint_saved", + "change_card", "rooignore_error", "diff_error", "condense_context", @@ -235,6 +237,49 @@ export const contextTruncationSchema = z.object({ export type ContextTruncation = z.infer +/** + * ChangeCard + * + * Payload of the per-step change card (B3a). The extension host emits one + * `say: "change_card"` message per completed tool write step, keyed by the + * shadow-git checkpoint the step produced. The JSON payload (see + * {@link ChangeCardData}) is carried in the message `text` field, the same + * way tool approval messages carry their serialized ClineSayTool. + * + * `detail: "full"` carries the unified diff inline for every file so the UI + * can render it directly; `detail: "summary"` carries only the file list with + * +/− counts and the UI fetches diffs lazily (B3b). Auto-approved steps are + * always emitted with `detail: "summary"` regardless of the user setting. + */ +export const changeCardDetailSchema = z.enum(["full", "summary"]) + +export type ChangeCardDetail = z.infer + +export const changeCardFileSchema = z.object({ + path: z.string(), + additions: z.number(), + deletions: z.number(), + /** + * Unified diff for this file. Only present when the card was emitted with + * `detail: "full"`; summary cards leave it out to stay compact. + */ + diff: z.string().optional(), +}) + +export type ChangeCardFile = z.infer + +export const changeCardSchema = z.object({ + /** Opaque step identifier, reserved for future tool-step tracking. */ + stepId: z.string().optional(), + /** Checkpoint commit SHAs produced by the step (one per per-write checkpoint). */ + checkpointIds: z.array(z.string()), + files: z.array(changeCardFileSchema), + totalFiles: z.number(), + detail: changeCardDetailSchema, +}) + +export type ChangeCardData = z.infer + /** * ClineMessage * diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 5f6b579779..5b3d739683 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -5,7 +5,7 @@ import type { ProviderSettings, ProviderSettingsEntry } from "./provider-setting import type { HistoryItem } from "./history.js" import type { ModeConfig, PromptComponent } from "./mode.js" import type { Experiments } from "./experiment.js" -import type { ClineMessage, QueuedMessage } from "./message.js" +import type { ChangeCardDetail, ClineMessage, QueuedMessage } from "./message.js" import type { MarketplaceItem, MarketplaceInstalledMetadata, InstallMarketplaceItemOptions } from "./marketplace.js" import type { TodoItem } from "./todo.js" import type { CloudUserInfo, CloudOrganizationMembership, OrganizationAllowList, ShareVisibility } from "./cloud.js" @@ -108,6 +108,7 @@ export interface ExtensionMessage { | "fileContent" | "rooHistoryImportProgress" | "themeFixtureProbeRequest" + | "checkpointRollbackResult" text?: string /** For fileContent: { path, content, error? } */ fileContent?: { path: string; content: string | null; error?: string } @@ -254,6 +255,34 @@ export interface ExtensionMessage { copyProgressItemName?: string // folderSelected path?: string + /** For checkpointRollbackResult: outcome of a change-card rollback request (B3b). */ + checkpointRollbackResult?: CheckpointRollbackResult +} + +/** + * CheckpointRollbackResult + * + * Outcome of a change-card restore request (B3b), posted back to the webview + * that sent `checkpointRollbackFile` / `checkpointRollbackStep` / + * `checkpointRestoreLatestFile`. `cardTs` echoes the change-card message + * timestamp so the requesting card can correlate the result: per-file results + * carry `filePath`, per-step results carry the per-file outcomes in + * `files`, and `kind` tells a per-file result which control it belongs to + * (absent = rollback, so results posted before `kind` existed still route). + */ +export interface CheckpointRollbackResult { + /** The `ts` of the change_card message the result belongs to. */ + cardTs: number + /** Per-file scope: the file that was restored. */ + filePath?: string + success: boolean + error?: string + /** Per-step scope: the per-file outcomes. */ + files?: { filePath: string; success: boolean; error?: string }[] + /** Per-file scope: which control the result belongs to. Absent = rollback. */ + kind?: "rollback" | "restore-latest" + /** Per-file scope: true when a restore-latest found no recorded write and left the file as-is. */ + noOp?: boolean } export interface OpenAiCodexRateLimitsMessage { @@ -348,6 +377,8 @@ export type ExtensionState = Pick< enableCheckpoints: boolean checkpointTimeout: number // Timeout for checkpoint initialization in seconds (default: 15) + perWriteCheckpoints: boolean + changeCardDetail: ChangeCardDetail maxOpenTabsContext: number // Maximum number of VSCode open tabs to include in context (0-500) maxWorkspaceFiles: number // Maximum number of files to include in current working directory details (0-500) showRooIgnoredFiles: boolean // Whether to show .rooignore'd files in listings @@ -544,6 +575,9 @@ export interface WebviewMessage { | "openCustomModesSettings" | "checkpointDiff" | "checkpointRestore" + | "checkpointRollbackFile" + | "checkpointRollbackStep" + | "checkpointRestoreLatestFile" | "completionCheckpointDiff" | "completionCheckpointRestore" | "deleteMcpServer" @@ -786,6 +820,46 @@ export const checkoutRestorePayloadSchema = z.object({ export type CheckpointRestorePayload = z.infer +/** + * Payload of the `checkpointRollbackFile` webview message (B3b): restore one + * change-card file to the checkpoint commit the card was keyed by. + */ +export const checkpointRollbackFilePayloadSchema = z.object({ + /** The `ts` of the change_card message the request comes from (echoed on the result). */ + cardTs: z.number(), + checkpointId: z.string(), + filePath: z.string(), +}) + +export type CheckpointRollbackFilePayload = z.infer + +/** + * Payload of the `checkpointRollbackStep` webview message (B3b): restore + * every file of a change-card step to the step's checkpoint. + */ +export const checkpointRollbackStepPayloadSchema = z.object({ + cardTs: z.number(), + /** The step's checkpoint commit (the card's first checkpointId); optional. */ + checkpointId: z.string().optional(), + filePaths: z.array(z.string()).min(1), +}) + +export type CheckpointRollbackStepPayload = z.infer + +/** + * Payload of the `checkpointRestoreLatestFile` webview message (B3b): restore + * one change-card file to the latest recorded version of that file (the + * content of its most recent write checkpoint — the forward direction to a + * rollback). + */ +export const checkpointRestoreLatestFilePayloadSchema = z.object({ + /** The `ts` of the change_card message the request comes from (echoed on the result). */ + cardTs: z.number(), + filePath: z.string(), +}) + +export type CheckpointRestoreLatestFilePayload = z.infer + export interface IndexingStatusPayload { state: "Standby" | "Indexing" | "Indexed" | "Error" | "Stopping" message: string @@ -799,6 +873,9 @@ export interface IndexClearedPayload { export type WebViewMessagePayload = | CheckpointDiffPayload | CheckpointRestorePayload + | CheckpointRollbackFilePayload + | CheckpointRollbackStepPayload + | CheckpointRestoreLatestFilePayload | IndexingStatusPayload | IndexClearedPayload | UpdateTodoListPayload diff --git a/src/core/checkpoints/__tests__/changeCard.spec.ts b/src/core/checkpoints/__tests__/changeCard.spec.ts new file mode 100644 index 0000000000..82190638a1 --- /dev/null +++ b/src/core/checkpoints/__tests__/changeCard.spec.ts @@ -0,0 +1,108 @@ +import { describe, expect, it } from "vitest" + +import type { ChangeCardData } from "@roo-code/types" + +import { + buildChangeCard, + buildChangeCardPayload, + isAutoApprovedStep, + resolveChangeCardDetail, + type ChangeCardWrite, +} from "../changeCard" + +describe("changeCard (B3a)", () => { + function write(overrides: Partial = {}): ChangeCardWrite { + return { + path: "src/a.ts", + diffStats: { additions: 2, deletions: 1 }, + diff: "--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1 +1,2 @@\n-old\n+new-1\n+new-2", + ...overrides, + } + } + + describe("isAutoApprovedStep", () => { + it("returns false for an empty step", () => { + expect(isAutoApprovedStep([])).toBe(false) + }) + + it("returns true only when every write was auto-approved", () => { + expect(isAutoApprovedStep([write({ autoApproved: true }), write({ autoApproved: true })])).toBe(true) + expect(isAutoApprovedStep([write({ autoApproved: true }), write()])).toBe(false) + expect(isAutoApprovedStep([write()])).toBe(false) + }) + }) + + describe("resolveChangeCardDetail", () => { + it("forces summary for auto-approved steps even when the setting is full", () => { + const writes = [write({ autoApproved: true })] + expect(resolveChangeCardDetail(writes, "full")).toBe("summary") + expect(resolveChangeCardDetail(writes, undefined)).toBe("summary") + }) + + it("follows the setting for interactive steps, defaulting to summary when unset", () => { + const writes = [write()] + expect(resolveChangeCardDetail(writes, "full")).toBe("full") + expect(resolveChangeCardDetail(writes, "summary")).toBe("summary") + expect(resolveChangeCardDetail(writes, undefined)).toBe("summary") + }) + }) + + describe("buildChangeCard", () => { + it("carries the inline diff per file for full detail on a multi-file step", () => { + const card = buildChangeCard( + "sha-1", + [write(), write({ path: "src/b.ts", diffStats: { additions: 1, deletions: 0 }, diff: "+b" })], + "full", + ) + + expect(card).toEqual({ + checkpointIds: ["sha-1"], + files: [ + { + path: "src/a.ts", + additions: 2, + deletions: 1, + diff: "--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1 +1,2 @@\n-old\n+new-1\n+new-2", + }, + { path: "src/b.ts", additions: 1, deletions: 0, diff: "+b" }, + ], + totalFiles: 2, + detail: "full", + }) + }) + + it("omits the diff per file for summary detail (lazy fetch is B3b)", () => { + const card = buildChangeCard("sha-1", [write()], "summary") + + expect(card.files).toEqual([{ path: "src/a.ts", additions: 2, deletions: 1 }]) + expect(card.files[0]).not.toHaveProperty("diff") + expect(card.detail).toBe("summary") + expect(card.totalFiles).toBe(1) + }) + + it("defaults missing diffStats to zero counts and keeps full detail without diff for a write without one", () => { + const card = buildChangeCard("sha-1", [write({ diffStats: undefined, diff: undefined })], "full") + + expect(card.files[0]).toEqual({ path: "src/a.ts", additions: 0, deletions: 0 }) + }) + }) + + describe("buildChangeCardPayload", () => { + it("resolves the detail level and builds the payload in one call", () => { + // The expectations are typed against the shared ChangeCardData + // contract in @roo-code/types, so the builder's output is checked + // against the same single source of truth the webview consumes. + // Interactive step with the full setting: diff inline. + const full: ChangeCardData = buildChangeCardPayload("sha-1", [write()], "full") + expect(full.detail).toBe("full") + expect(full.files[0].diff).toBe("--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1 +1,2 @@\n-old\n+new-1\n+new-2") + + // Auto-approved step with the full setting: compact summary, no diff. + const compact: ChangeCardData = buildChangeCardPayload("sha-1", [write({ autoApproved: true })], "full") + expect(compact.detail).toBe("summary") + expect(compact.files[0]).not.toHaveProperty("diff") + expect(compact.checkpointIds).toEqual(["sha-1"]) + expect(compact.totalFiles).toBe(1) + }) + }) +}) diff --git a/src/core/checkpoints/__tests__/changeJournal.spec.ts b/src/core/checkpoints/__tests__/changeJournal.spec.ts new file mode 100644 index 0000000000..aff2e086f3 --- /dev/null +++ b/src/core/checkpoints/__tests__/changeJournal.spec.ts @@ -0,0 +1,148 @@ +import fs from "fs/promises" +import os from "os" +import path from "path" + +import { afterEach, beforeEach, describe, expect, it } 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) + }) + }) + + describe("loadChanges", () => { + it("returns [] for an absent journal file", async () => { + expect(await loadChanges(tmpRoot, taskId)).toEqual([]) + }) + + it("returns [] for an empty journal file", async () => { + await fs.mkdir(path.dirname(journalPath(tmpRoot, taskId)), { recursive: true }) + await fs.writeFile(journalPath(tmpRoot, taskId), "") + + expect(await loadChanges(tmpRoot, taskId)).toEqual([]) + }) + + it("propagates non-ENOENT read failures instead of reporting an empty journal", async () => { + // A directory at the journal path makes readFile fail with EISDIR — + // a stand-in for any permission or I/O failure (EACCES etc.). Such a + // failure must not be swallowed into "no changes": it would let a + // rollback report a no-op success without reading the history. + await fs.mkdir(journalPath(tmpRoot, taskId), { recursive: true }) + + await expect(loadChanges(tmpRoot, taskId)).rejects.toMatchObject({ code: "EISDIR" }) + }) + + it("parses all entries in order with a clean tail", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "x" })) + await appendChange(tmpRoot, taskId, entry({ checkpointId: "y" })) + await appendChange(tmpRoot, taskId, entry({ checkpointId: "z" })) + + const result = await loadChanges(tmpRoot, taskId) + expect(result).toHaveLength(3) + expect(result[0].checkpointId).toBe("x") + expect(result[1].checkpointId).toBe("y") + expect(result[2].checkpointId).toBe("z") + }) + + it("parses a journal whose final line has no trailing newline", async () => { + await appendChange(tmpRoot, taskId, entry({ checkpointId: "ok" })) + + // Rewrite the file without the trailing newline of the last line. + const filePath = journalPath(tmpRoot, taskId) + const content = (await fs.readFile(filePath, "utf8")).replace(/\n$/, "") + await fs.writeFile(filePath, content) + + const result = await loadChanges(tmpRoot, taskId) + expect(result).toHaveLength(1) + expect(result[0].checkpointId).toBe("ok") + }) + + it("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() + }) + }) +}) diff --git a/src/core/checkpoints/__tests__/checkpointJournal.test.ts b/src/core/checkpoints/__tests__/checkpointJournal.test.ts new file mode 100644 index 0000000000..8671bb491c --- /dev/null +++ b/src/core/checkpoints/__tests__/checkpointJournal.test.ts @@ -0,0 +1,218 @@ +import fs from "fs/promises" +import os from "os" +import path from "path" + +import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from "vitest" + +import type { Task } from "../../task/Task" +import { loadChanges } from "../changeJournal" +import { checkpointSave, type CheckpointWriteInfo } from "../index" + +// Mock the VS Code API surface (index.ts imports vscode at module level). +vi.mock("vscode", () => ({ + window: { + showErrorMessage: vi.fn(), + showInformationMessage: vi.fn(), + createTextEditorDecorationType: vi.fn(() => ({})), + }, + Uri: { + file: vi.fn((p: string) => ({ fsPath: p })), + parse: vi.fn((uri: string) => ({ with: vi.fn(() => ({})) })), + }, + commands: { + executeCommand: vi.fn(), + }, +})) + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureCheckpointCreated: vi.fn(), + captureCheckpointRestored: vi.fn(), + captureCheckpointDiffed: vi.fn(), + }, + }, +})) + +vi.mock("../../../utils/path", () => ({ + getWorkspacePath: vi.fn(() => "/test/workspace"), +})) + +vi.mock("../../../utils/git", () => ({ + checkGitInstalled: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../../i18n", () => ({ + t: vi.fn((key: string) => key), +})) + +vi.mock("p-wait-for", () => ({ + default: vi.fn(), +})) + +// The real service would require a git repo; the checkpointSave under test +// only needs the pre-initialized service on the task. +vi.mock("../../../services/checkpoints") + +const TASK_ID = "journal-test-task" +const COMMIT = "test-commit-hash" + +interface ServiceLike { + isInitialized: boolean + saveCheckpoint: (...args: unknown[]) => Promise +} + +interface ProviderLike { + context: { globalStorageUri: { fsPath: string } } + log: (...args: unknown[]) => void + postMessageToWebview: (...args: unknown[]) => void + getState: () => Promise> +} + +interface TaskLike { + taskId: string + enableCheckpoints: boolean + checkpointService: ServiceLike + checkpointServiceInitializing: boolean + providerRef: { deref: () => ProviderLike | undefined } + say: (...args: unknown[]) => Promise +} + +describe("checkpointSave change-journal wiring (B2)", () => { + let tmpStorageDir: string + let saveCheckpointSpy: Mock + let mockProvider: ProviderLike + let mockTask: TaskLike + const write: CheckpointWriteInfo = { + path: "src/foo.ts", + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + } + + beforeEach(async () => { + tmpStorageDir = await fs.mkdtemp(path.join(os.tmpdir(), "b2-journal-wiring-")) + saveCheckpointSpy = vi.fn().mockResolvedValue({ commit: COMMIT }) + mockProvider = { + context: { globalStorageUri: { fsPath: tmpStorageDir } }, + log: vi.fn(), + postMessageToWebview: vi.fn(), + // B3a: the card emission reads the live settings through getState. + getState: vi.fn().mockResolvedValue({}), + } + // Structural test double for Task (the class is not instantiated at + // this unit layer); the cast is safe because the fields checkpointSave + // reads are exactly these. + mockTask = { + taskId: TASK_ID, + enableCheckpoints: true, + checkpointService: { isInitialized: true, saveCheckpoint: saveCheckpointSpy }, + checkpointServiceInitializing: false, + providerRef: { deref: () => mockProvider }, + // B3a: the card emission calls task.say; a resolved double keeps the + // test double complete instead of letting the emission take the + // error path. + say: vi.fn().mockResolvedValue(undefined), + } + }) + + afterEach(async () => { + vi.restoreAllMocks() + await fs.rm(tmpStorageDir, { recursive: true, force: true }) + }) + + it("appends exactly one journal line referencing the B1 checkpoint id for a per-write save", async () => { + await checkpointSave(mockTask as Task, false, true, write) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toHaveLength(1) + expect(entries[0]).toEqual({ + path: "src/foo.ts", + operation: "create", + checkpointId: COMMIT, + diffStats: { additions: 3, deletions: 0 }, + }) + + // The raw file holds exactly one JSON line. + const journalFile = path.join(tmpStorageDir, "tasks", TASK_ID, "checkpoints", "changes.jsonl") + const raw = await fs.readFile(journalFile, "utf8") + expect(raw.split("\n").filter((line) => line !== "")).toHaveLength(1) + }) + + it("omits diffStats in the journal entry when not provided", async () => { + await checkpointSave(mockTask as Task, false, true, { path: "src/bar.ts", operation: "update" }) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toHaveLength(1) + expect(entries[0].path).toBe("src/bar.ts") + expect(entries[0].operation).toBe("update") + expect(entries[0].checkpointId).toBe(COMMIT) + expect(entries[0].diffStats).toBeUndefined() + }) + + it("does not write a journal entry for non-write checkpoint saves (task-start baseline)", async () => { + await checkpointSave(mockTask as Task) + + const journalFile = path.join(tmpStorageDir, "tasks", TASK_ID, "checkpoints", "changes.jsonl") + await expect(fs.stat(journalFile)).rejects.toThrow() + expect(await loadChanges(tmpStorageDir, TASK_ID)).toEqual([]) + }) + + it("appends one entry per file change for a multi-file write (apply-patch shape)", async () => { + await checkpointSave(mockTask as Task, false, true, [ + { path: "src/a.ts", operation: "create" }, + { path: "src/b.ts", operation: "update" }, + { path: "src/c.ts", operation: "delete" }, + ]) + + const entries = await loadChanges(tmpStorageDir, TASK_ID) + expect(entries).toHaveLength(3) + // Every entry references the single checkpoint of the whole patch. + expect(entries.map((entry) => entry.checkpointId)).toEqual([COMMIT, COMMIT, COMMIT]) + expect(entries.map((entry) => entry.path)).toEqual(["src/a.ts", "src/b.ts", "src/c.ts"]) + expect(entries.map((entry) => entry.operation)).toEqual(["create", "update", "delete"]) + }) + + it("keeps the existing error-swallowing behavior 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("logs and continues when the journal cannot be written (checkpoints stay enabled)", async () => { + // Block the per-task checkpoint dir so the journal mkdir/append fails. + const taskDir = path.join(tmpStorageDir, "tasks", TASK_ID) + await fs.mkdir(taskDir, { recursive: true }) + await fs.writeFile(path.join(taskDir, "checkpoints"), "blocker") + + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined) + + // The journal failure is swallowed (logged, not rethrown), so the + // checkpoint result still resolves exactly as without journaling. + await expect(checkpointSave(mockTask as Task, false, true, write)).resolves.toMatchObject({ commit: COMMIT }) + + expect(mockTask.enableCheckpoints).toBe(true) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("failed to append change journal entry"), + expect.anything(), + ) + }) +}) diff --git a/src/core/checkpoints/__tests__/checkpointSave.spec.ts b/src/core/checkpoints/__tests__/checkpointSave.spec.ts new file mode 100644 index 0000000000..25afe4cad7 --- /dev/null +++ b/src/core/checkpoints/__tests__/checkpointSave.spec.ts @@ -0,0 +1,223 @@ +import fs from "fs/promises" +import os from "os" +import path from "path" + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" + +import type { Task } from "../../task/Task" +import { journalPath } from "../changeJournal" +import { checkpointSave } from "../index" + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureCheckpointCreated: vi.fn(), + captureCheckpointRestored: vi.fn(), + captureCheckpointDiffed: vi.fn(), + }, + }, +})) + +/** + * Minimal Task double for the checkpointSave wiring tests. Only the members + * touched by getCheckpointService + checkpointSave are provided: a pre-set + * checkpointService (so no git installation happens), the provider context + * (journal dir + setting state), and say. Structural cast at the boundary, + * matching the documented test-double style of the tool specs. + */ +function makeTask(options: { saveCheckpoint?: unknown; state?: Record; enableCheckpoints?: boolean }) { + const say = vi.fn().mockResolvedValue(undefined) + // An explicit `saveCheckpoint: undefined` (a checkpoint that produced no + // commit) must be preserved as-is; only an omitted option falls back to + // the default commit result. + const saveCheckpoint = vi + .fn() + .mockResolvedValue("saveCheckpoint" in options ? options.saveCheckpoint : { commit: "sha-card-1" }) + const providerDeref = { + context: { globalStorageUri: { fsPath: globalStorageDir } }, + getState: vi.fn().mockResolvedValue(options.state ?? {}), + } + + const task = { + taskId: "task-card", + cwd: "/workspace", + enableCheckpoints: options.enableCheckpoints ?? true, + checkpointService: { + saveCheckpoint, + isInitialized: true, + }, + providerRef: { deref: vi.fn().mockReturnValue(providerDeref) }, + say, + } as unknown as Task + + return { task, say, saveCheckpoint } +} + +let globalStorageDir: string + +beforeEach(async () => { + globalStorageDir = await fs.mkdtemp(path.join(os.tmpdir(), "b3a-save-")) +}) + +afterEach(async () => { + await fs.rm(globalStorageDir, { recursive: true, force: true }) +}) + +describe("checkpointSave change-card emission (B3a)", () => { + it("emits a full-detail change card after a successful per-write checkpoint and still appends the journal", async () => { + const { task, say, saveCheckpoint } = makeTask({ state: { changeCardDetail: "full" } }) + + await checkpointSave(task, false, true, { + path: "src/a.ts", + operation: "create", + diffStats: { additions: 2, deletions: 1 }, + diff: "+line-a\n+line-b\n-old", + }) + + expect(saveCheckpoint).toHaveBeenCalledWith(expect.stringContaining("task-card"), expect.any(Object)) + + const cardCalls = say.mock.calls.filter(([type]) => type === "change_card") + expect(cardCalls).toHaveLength(1) + const [type, text, images, partial, sayOptions, _progress, options] = cardCalls[0] as unknown as [ + string, + string, + undefined, + undefined, + undefined, + undefined, + { isNonInteractive?: boolean }, + ] + expect(type).toBe("change_card") + expect(images).toBeUndefined() + expect(options).toEqual({ isNonInteractive: true }) + const card = JSON.parse(text as string) as { + checkpointIds: string[] + files: Array<{ path: string; additions: number; deletions: number; diff?: string }> + totalFiles: number + detail: string + } + expect(card.checkpointIds).toEqual(["sha-card-1"]) + expect(card.totalFiles).toBe(1) + expect(card.detail).toBe("full") + expect(card.files).toEqual([{ path: "src/a.ts", additions: 2, deletions: 1, diff: "+line-a\n+line-b\n-old" }]) + + // B2 regression: the journal entry is still appended with the commit id. + const journalRaw = await fs.readFile(journalPath(globalStorageDir, "task-card"), "utf8") + const entries = journalRaw + .split("\n") + .filter((line) => line !== "") + .map((line) => JSON.parse(line)) + expect(entries).toHaveLength(1) + expect(entries[0]).toMatchObject({ path: "src/a.ts", operation: "create", checkpointId: "sha-card-1" }) + }) + + it("emits a summary card without diffs for the default (summary) setting", async () => { + const { task, say } = makeTask({}) + + await checkpointSave(task, false, true, { + path: "src/a.ts", + operation: "update", + diffStats: { additions: 1, deletions: 0 }, + diff: "+x", + }) + + const cardCall = say.mock.calls.find(([type]) => type === "change_card") + expect(cardCall).toBeDefined() + const card = JSON.parse((cardCall as unknown as [string, string])[1]) as { + files: Array> + detail: string + } + expect(card.detail).toBe("summary") + expect(card.files[0]).not.toHaveProperty("diff") + }) + + it("emits a compact card for auto-approved steps even when the setting is full", async () => { + const { task, say } = makeTask({ state: { changeCardDetail: "full" } }) + + await checkpointSave(task, false, true, { + path: "src/a.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: "+x", + autoApproved: true, + }) + + const cardCall = say.mock.calls.find(([type]) => type === "change_card") + expect(cardCall).toBeDefined() + const card = JSON.parse((cardCall as unknown as [string, string])[1]) as { + files: Array> + detail: string + } + expect(card.detail).toBe("summary") + expect(card.files[0]).not.toHaveProperty("diff") + }) + + it("emits one card with all writes for a multi-file step", async () => { + const { task, say } = makeTask({ state: { changeCardDetail: "full" } }) + + await checkpointSave(task, false, true, [ + { path: "src/a.ts", operation: "create", diffStats: { additions: 2, deletions: 0 }, diff: "+a1\n+a2" }, + { path: "src/b.ts", operation: "delete", diffStats: { additions: 0, deletions: 3 } }, + ]) + + const cardCalls = say.mock.calls.filter(([type]) => type === "change_card") + expect(cardCalls).toHaveLength(1) + const card = JSON.parse((cardCalls[0] as unknown as [string, string])[1]) as { + files: Array<{ path: string; diff?: string }> + totalFiles: number + } + expect(card.totalFiles).toBe(2) + expect(card.files.map((file) => file.path)).toEqual(["src/a.ts", "src/b.ts"]) + expect(card.files[0].diff).toBe("+a1\n+a2") + expect(card.files[1]).not.toHaveProperty("diff") + }) + + it("emits no change card for baseline checkpoints without write info", async () => { + const { task, say } = makeTask({}) + + await checkpointSave(task) + + // `say` is invoked with seven arguments, so a three-argument + // `toHaveBeenCalledWith` negative assertion can never fail; filter the + // recorded calls by type instead. + const cardCalls = say.mock.calls.filter(([type]) => type === "change_card") + expect(cardCalls).toHaveLength(0) + }) + + it("emits no change card when the checkpoint produced no commit", async () => { + const { task, say } = makeTask({ saveCheckpoint: undefined }) + + await checkpointSave(task, false, true, { path: "src/a.ts", operation: "create" }) + + const cardCalls = say.mock.calls.filter(([type]) => type === "change_card") + expect(cardCalls).toHaveLength(0) + }) + + it("emits no change card when checkpoints are disabled for the task", async () => { + const { task, say } = makeTask({ enableCheckpoints: false }) + + await checkpointSave(task, false, true, { path: "src/a.ts", operation: "create", diff: "+x" }) + + expect(say).not.toHaveBeenCalled() + }) + + it("keeps the journal append when a card emission failure occurs", async () => { + const { task, say } = makeTask({}) + say.mockImplementation(async (type: string) => { + if (type === "change_card") { + throw new Error("task aborted") + } + }) + + await checkpointSave(task, false, true, { + path: "src/a.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: "+x", + }) + + // The say failure is contained: the journal is still written. + const journalRaw = await fs.readFile(journalPath(globalStorageDir, "task-card"), "utf8") + expect(journalRaw).toContain("src/a.ts") + }) +}) diff --git a/src/core/checkpoints/__tests__/rollback.spec.ts b/src/core/checkpoints/__tests__/rollback.spec.ts new file mode 100644 index 0000000000..0af6384113 --- /dev/null +++ b/src/core/checkpoints/__tests__/rollback.spec.ts @@ -0,0 +1,455 @@ +import fs from "fs/promises" +import os from "os" +import path from "path" + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" + +import type { Task } from "../../task/Task" +import type { ChangeJournalEntry } from "../changeJournal" +import { getCheckpointService } from "../index" +import * as changeJournal from "../changeJournal" +import { appendChange, journalPath } from "../changeJournal" +import { restoreLatestFile, rollbackFile, rollbackStep } from "../rollback" + +vi.mock("../index", () => ({ + getCheckpointService: vi.fn(), + checkpointSave: vi.fn(), + checkpointRestore: vi.fn(), + checkpointDiff: vi.fn(), +})) + +const mockedGetCheckpointService = getCheckpointService as unknown as ReturnType + +function makeTask(): Task { + return { + taskId: "task-rollback", + providerRef: { + deref: vi.fn().mockReturnValue({ context: { globalStorageUri: { fsPath: globalStorageDir } } }), + }, + } as unknown as Task +} + +/** A checkpoint-service double with a recording restoreFile and a baseline. */ +function serviceWith(baseHash: string | undefined) { + const restoreFile = vi.fn().mockResolvedValue(undefined) + return { baseHash, restoreFile } +} + +async function seedJournal(entries: ChangeJournalEntry[]): Promise { + for (const entry of entries) { + await appendChange(globalStorageDir, "task-rollback", entry) + } +} + +let globalStorageDir: string + +beforeEach(async () => { + globalStorageDir = await fs.mkdtemp(path.join(os.tmpdir(), "b3c-rollback-")) + mockedGetCheckpointService.mockReset() +}) + +afterEach(async () => { + await fs.rm(globalStorageDir, { recursive: true, force: true }) +}) + +describe("rollbackFile (B3c: undo the step's write to the file)", () => { + it("restores the file to the PREVIOUS step's checkpoint when an earlier entry exists", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-2", "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: true }) + // The pre-step state is the previous step's post-write checkpoint — not + // the step's own (post-write) checkpoint. + expect(service.restoreFile).toHaveBeenCalledTimes(1) + expect(service.restoreFile).toHaveBeenCalledWith("sha-1", "src/a.ts") + }) + + it("restores from the task-start baseline when the file has no earlier entry", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: true }) + expect(service.restoreFile).toHaveBeenCalledWith("base-0", "src/a.ts") + }) + + it("resolves through the first entry of a multi-write step", async () => { + // One patch writes the same file twice: two entries share the step + // checkpoint. The pre-step state is still the entry before the first + // one of the step. + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-2", "src/a.ts") + + expect(outcome.success).toBe(true) + expect(service.restoreFile).toHaveBeenCalledWith("sha-1", "src/a.ts") + }) + + it("rejects rolling back a step that is not the file's latest change", async () => { + // The file was written again by a later step (sha-2): rolling back the + // older step (sha-1) would overwrite the newer state, so it is + // rejected instead of silently destroying it. + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "File was modified in a later step; roll back the latest change card first", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails when the journal location is unavailable (no global storage)", async () => { + // No context on the provider double → the journal cannot even be + // located: a clear failure, not a silent miss on the file lookup. + const task = { + taskId: "task-rollback", + providerRef: { deref: vi.fn().mockReturnValue(undefined) }, + } as unknown as Task + + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(task, "sha-1", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "Change journal is unavailable for this task", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails cleanly when the file is not part of the given step checkpoint", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-2", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "File is not part of this step's checkpoint", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails cleanly when no earlier checkpoint exists and the baseline is unavailable", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith(undefined) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "No checkpoint available to restore", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails cleanly when checkpoints are not enabled", async () => { + mockedGetCheckpointService.mockResolvedValue(undefined) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "Checkpoints are not enabled for this task", + }) + }) + + it("reports the service error without throwing", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + service.restoreFile.mockRejectedValue(new Error("pathspec did not match")) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome.success).toBe(false) + expect(outcome.error).toContain("pathspec did not match") + }) + + it("stringifies non-Error rejections into the outcome", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + service.restoreFile.mockRejectedValue("raw failure") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackFile(makeTask(), "sha-1", "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: false, error: "raw failure" }) + }) +}) + +describe("rollbackStep (B3c: undo every file of the step)", () => { + it("restores every step file to its pre-step state", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + { path: "src/b.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/b.ts"], "sha-2") + + expect(outcome.checkpointId).toBe("sha-2") + expect(outcome.files).toEqual([ + { filePath: "src/a.ts", success: true }, + { filePath: "src/b.ts", success: true }, + ]) + expect(service.restoreFile).toHaveBeenCalledTimes(2) + expect(service.restoreFile).toHaveBeenNthCalledWith(1, "sha-1", "src/a.ts") + // src/b.ts has no earlier entry: its pre-step state is the baseline. + expect(service.restoreFile).toHaveBeenNthCalledWith(2, "base-0", "src/b.ts") + }) + + it("keeps per-file failures isolated from the other step files", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + { path: "src/b.ts", operation: "create", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + service.restoreFile.mockRejectedValueOnce(new Error("pathspec did not match")) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/b.ts"], "sha-2") + + expect(outcome.files[0].success).toBe(false) + expect(outcome.files[0].error).toContain("pathspec did not match") + expect(outcome.files[1]).toEqual({ filePath: "src/b.ts", success: true }) + }) + + it("fails the file cleanly when no earlier checkpoint exists and the baseline is unavailable", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-2" }]) + const service = serviceWith(undefined) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts"], "sha-2") + + expect(outcome.checkpointId).toBe("sha-2") + expect(outcome.files).toEqual([ + { filePath: "src/a.ts", success: false, error: "No checkpoint available to restore" }, + ]) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("rejects a file that is not part of the given step checkpoint", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-2" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/other.ts"], "sha-2") + + expect(outcome.files[0]).toEqual({ filePath: "src/a.ts", success: true }) + expect(outcome.files[1].success).toBe(false) + expect(outcome.files[1].error).toBe("File is not part of this step's checkpoint") + expect(service.restoreFile).toHaveBeenCalledTimes(1) + }) + + it("rejects the stale file of a step while restoring the others", async () => { + // src/a.ts was written again after this step's checkpoint, so its + // sha-2 entry is no longer the file's latest: only src/b.ts (whose + // latest entry IS sha-2) is restored. + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + { path: "src/b.ts", operation: "create", checkpointId: "sha-2" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-3" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/b.ts"], "sha-2") + + expect(outcome.files[0].success).toBe(false) + expect(outcome.files[0].error).toBe("File was modified in a later step; roll back the latest change card first") + expect(outcome.files[1]).toEqual({ filePath: "src/b.ts", success: true }) + expect(service.restoreFile).toHaveBeenCalledTimes(1) + expect(service.restoreFile).toHaveBeenCalledWith("base-0", "src/b.ts") + }) + + it("falls back to the latest journal entry per file without a step checkpoint id", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts"]) + + expect(outcome.checkpointId).toBeUndefined() + expect(outcome.files).toEqual([{ filePath: "src/a.ts", success: true }]) + expect(service.restoreFile).toHaveBeenCalledWith("sha-2", "src/a.ts") + }) + + it("fails listed files without journal entries", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts", "src/missing.ts"]) + + expect(outcome.files[0]).toEqual({ filePath: "src/a.ts", success: true }) + expect(outcome.files[1].success).toBe(false) + expect(outcome.files[1].error).toBe("No change journal entry for this file") + }) + + it("fails per file when the journal location is unavailable (no global storage)", async () => { + // No context on the provider double → the journal cannot even be + // located. That is a failure, not an empty journal: reporting the + // step as merely "not part of this checkpoint" would be misleading. + const task = { + taskId: "task-rollback", + providerRef: { deref: vi.fn().mockReturnValue(undefined) }, + } as unknown as Task + + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await rollbackStep(task, ["src/a.ts"], "sha-2") + + expect(outcome.checkpointId).toBe("sha-2") + expect(outcome.files[0].success).toBe(false) + expect(outcome.files[0].error).toBe("Change journal is unavailable for this task") + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails every file when checkpoints are not enabled", async () => { + mockedGetCheckpointService.mockResolvedValue(undefined) + + const outcome = await rollbackStep(makeTask(), ["src/a.ts"], "sha-2") + + expect(outcome.checkpointId).toBe("sha-2") + expect(outcome.files).toEqual([ + { filePath: "src/a.ts", success: false, error: "Checkpoints are not enabled for this task" }, + ]) + }) +}) + +describe("restoreLatestFile (B3c: forward direction)", () => { + it("restores the file to its most recent recorded write checkpoint", async () => { + await seedJournal([ + { path: "src/a.ts", operation: "create", checkpointId: "sha-1" }, + { path: "src/b.ts", operation: "update", checkpointId: "sha-1" }, + { path: "src/a.ts", operation: "update", checkpointId: "sha-2" }, + ]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: true }) + expect(service.restoreFile).toHaveBeenCalledWith("sha-2", "src/a.ts") + }) + + it("is a successful no-op for a file the task never wrote", async () => { + await seedJournal([{ path: "src/b.ts", operation: "update", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome).toEqual({ filePath: "src/a.ts", success: true, noOp: true }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails when the journal location is unavailable (no global storage)", async () => { + // An unavailable journal is not "the task wrote nothing": a no-op + // success would claim a restore that never happened. + const task = { + taskId: "task-rollback", + providerRef: { deref: vi.fn().mockReturnValue(undefined) }, + } as unknown as Task + + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(task, "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "Change journal is unavailable for this task", + }) + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails when the journal cannot be read (an I/O error is not an empty journal)", async () => { + // A directory at the journal path makes readFile fail with EISDIR — + // a stand-in for any permission or I/O failure (EACCES etc.). + await fs.mkdir(journalPath(globalStorageDir, "task-rollback"), { recursive: true }) + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome.success).toBe(false) + expect(outcome.error).toContain("Change journal could not be read") + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("stringifies a non-Error journal read failure into the outcome", async () => { + vi.spyOn(changeJournal, "loadChanges").mockRejectedValueOnce("raw journal failure") + const service = serviceWith("base-0") + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome.success).toBe(false) + expect(outcome.error).toBe("Change journal could not be read: raw journal failure") + expect(service.restoreFile).not.toHaveBeenCalled() + }) + + it("fails cleanly when checkpoints are not enabled", async () => { + mockedGetCheckpointService.mockResolvedValue(undefined) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome).toEqual({ + filePath: "src/a.ts", + success: false, + error: "Checkpoints are not enabled for this task", + }) + }) + + it("reports the service error without throwing", async () => { + await seedJournal([{ path: "src/a.ts", operation: "create", checkpointId: "sha-1" }]) + const service = serviceWith("base-0") + service.restoreFile.mockRejectedValue(new Error("index.lock held")) + mockedGetCheckpointService.mockResolvedValue(service) + + const outcome = await restoreLatestFile(makeTask(), "src/a.ts") + + expect(outcome.success).toBe(false) + expect(outcome.error).toContain("index.lock") + }) +}) diff --git a/src/core/checkpoints/changeCard.ts b/src/core/checkpoints/changeCard.ts new file mode 100644 index 0000000000..b13f03320d --- /dev/null +++ b/src/core/checkpoints/changeCard.ts @@ -0,0 +1,91 @@ +/** + * Per-step change card builder (B3a). + * + * The card is emitted from `checkpointSave` (index.ts) once the per-write + * checkpoint commit exists, so the payload can key the card by the real + * checkpoint ID and reuse the approval diff + stats the tool already + * computed (threaded through {@link CheckpointWriteInfo}). The card is + * informational and always emitted for write steps, including auto-approved + * ones — which always get the compact ("summary") form regardless of the + * `changeCardDetail` setting. + */ +import { DEFAULT_CHANGE_CARD_DETAIL, type ChangeCardData, type ChangeCardDetail } from "@roo-code/types" + +/** + * The write data a change card is built from. Structurally compatible with + * `CheckpointWriteInfo` (src/core/checkpoints/index.ts), minus the + * `operation` field the card does not need. + */ +export interface ChangeCardWrite { + /** The file path as the tool knows it (relative to the task cwd). */ + path: string + /** { additions, deletions } from the approval diff, when computable. */ + diffStats?: { additions: number; deletions: number } + /** The unified approval diff for this file (reused, not recomputed). */ + diff?: string + /** Whether the tool step was auto-approved (no human interaction). */ + autoApproved?: boolean +} + +/** + * Whether every write of the step was auto-approved. Empty steps are not + * auto-approved (there is nothing for the user to have skipped). + */ +export function isAutoApprovedStep(writes: readonly ChangeCardWrite[]): boolean { + return writes.length > 0 && writes.every((write) => write.autoApproved === true) +} + +/** + * Resolve the card detail level for a step: + * - auto-approved steps always get the compact "summary" card, regardless of + * the user setting (cards for steps the user never saw approving are + * informational only); + * - otherwise the `changeCardDetail` setting applies, defaulting to + * "summary" when unset. + */ +export function resolveChangeCardDetail( + writes: readonly ChangeCardWrite[], + setting: ChangeCardDetail | undefined, +): ChangeCardDetail { + if (isAutoApprovedStep(writes)) { + return "summary" + } + return setting ?? DEFAULT_CHANGE_CARD_DETAIL +} + +/** + * Build the typed change-card payload for one step (one per-write checkpoint). + * + * With `detail: "full"` each file carries its unified diff inline; with + * `detail: "summary"` the diff is omitted and the UI fetches it lazily + * (B3b). + */ +export function buildChangeCard( + checkpointId: string, + writes: readonly ChangeCardWrite[], + detail: ChangeCardDetail, +): ChangeCardData { + return { + checkpointIds: [checkpointId], + files: writes.map((write) => ({ + path: write.path, + additions: write.diffStats?.additions ?? 0, + deletions: write.diffStats?.deletions ?? 0, + ...(detail === "full" && write.diff ? { diff: write.diff } : {}), + })), + totalFiles: writes.length, + detail, + } +} + +/** + * Convenience wrapper: resolve the detail level from the step + setting, then + * build the payload. This is what `checkpointSave` calls. + */ +export function buildChangeCardPayload( + checkpointId: string, + writes: readonly ChangeCardWrite[], + setting: ChangeCardDetail | undefined, +): ChangeCardData { + return buildChangeCard(checkpointId, writes, resolveChangeCardDetail(writes, setting)) +} diff --git a/src/core/checkpoints/changeJournal.ts b/src/core/checkpoints/changeJournal.ts new file mode 100644 index 0000000000..79ee802eb7 --- /dev/null +++ b/src/core/checkpoints/changeJournal.ts @@ -0,0 +1,106 @@ +import fs from "fs/promises" +import * as path from "path" + +/** + * A single entry in the per-task change journal (changes.jsonl). + * + * One line is appended for every successful file write that goes through a + * B1 per-write checkpoint hook. WriteToFileTool and EditFileTool emit one + * entry per write; ApplyPatchTool emits one entry per file change of a fully + * successful patch — those entries all reference the single B1 checkpoint + * that the patch's post-loop hook saves for the whole patch. The task-start + * baseline never produces an entry (it is not a file write). + */ +export interface ChangeJournalEntry { + /** The file path as the tool knows it (relative to task cwd). */ + path: string + /** "create" | "update" | "delete" — derived from what the tool did. */ + operation: "create" | "update" | "delete" + /** The B1 checkpoint commit SHA for this write (from checkpointSave result). */ + checkpointId: string + /** { additions, deletions } from the approval diff; null/omit when not computable. */ + diffStats?: { additions: number; deletions: number } +} + +const JOURNAL_FILENAME = "changes.jsonl" + +/** 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 { + return path.join(taskCheckpointDir(globalStorageDir, taskId), JOURNAL_FILENAME) +} + +/** + * Append one change-journal entry to the per-task changes.jsonl file. + * + * Uses appendFile so each write is a single syscall — minimal torn-write risk. + * 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 }) + const line = JSON.stringify(entry) + "\n" + await fs.appendFile(filePath, line) +} + +/** + * Load all change-journal entries for a task. + * + * Torn-tail repair: if the final line is truncated (JSON.parse fails), it is + * silently discarded. The rest of the file is returned in order. An absent + * or empty journal returns [] — but only an ABSENT file. Any other read + * failure (permissions, I/O) is rethrown: a journal that cannot be read must + * not be indistinguishable from one that is legitimately empty. + */ +export async function loadChanges( + globalStorageDir: string, + taskId: string, +): Promise { + const filePath = journalPath(globalStorageDir, taskId) + + let content: string + try { + content = await fs.readFile(filePath, "utf8") + } catch (error) { + // A missing journal is a legitimate empty history; any other read + // failure (permissions, I/O) must propagate. Swallowing it would let + // a rollback report a no-op success without reading the history. + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + return [] + } + throw error + } + + if (!content.trim()) { + return [] + } + + const lines = content.split("\n") + // Remove trailing empty line from a file that ends with \n. + if (lines[lines.length - 1] === "") { + lines.pop() + } + + const entries: ChangeJournalEntry[] = [] + for (let i = 0; i < lines.length; i++) { + try { + entries.push(JSON.parse(lines[i]) 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 +} diff --git a/src/core/checkpoints/index.ts b/src/core/checkpoints/index.ts index 26a137b939..d206767ad4 100644 --- a/src/core/checkpoints/index.ts +++ b/src/core/checkpoints/index.ts @@ -16,6 +16,9 @@ import { DIFF_VIEW_URI_SCHEME } from "../../integrations/editor/DiffViewProvider import { CheckpointServiceOptions, RepoPerTaskCheckpointService } from "../../services/checkpoints" +import { appendChange, ChangeJournalEntry } from "./changeJournal" +import { buildChangeCardPayload } from "./changeCard" + const WARNING_THRESHOLD_MS = 5000 function sendCheckpointInitWarn(task: Task, type?: "WAIT_TIMEOUT" | "INIT_TIMEOUT", timeout?: number) { @@ -209,7 +212,38 @@ async function checkGitInstallation( } } -export async function checkpointSave(task: Task, force = false, suppressMessage = false) { +/** + * Write metadata for the per-task change journal (B2). + * + * `path` is the file path as the tool knows it (relative to the task cwd), + * consistent with what the B1 per-write checkpoint hooks see. `diffStats` is + * the { additions, deletions } pair from the approval diff when it was + * computable; omitted otherwise. ApplyPatchTool passes one entry per file + * change of a fully successful patch (an array), all sharing the single + * checkpoint the patch's post-loop hook saves. + */ +export type CheckpointWriteInfo = { + path: string + operation: "create" | "update" | "delete" + diffStats?: { additions: number; deletions: number } + /** + * The unified approval diff for this write, reused verbatim by the B3a + * change card (never recomputed). + */ + diff?: string + /** + * Whether the tool step was auto-approved (no human interaction). Auto- + * approved steps always get the compact ("summary") change card. + */ + autoApproved?: boolean +} + +export async function checkpointSave( + task: Task, + force = false, + suppressMessage = false, + write?: CheckpointWriteInfo | CheckpointWriteInfo[], +) { const service = await getCheckpointService(task) if (!service) { @@ -221,6 +255,48 @@ export async function checkpointSave(task: Task, force = false, suppressMessage // Start the checkpoint process in the background. return service .saveCheckpoint(`Task: ${task.taskId}, Time: ${Date.now()}`, { allowEmpty: force, suppressMessage }) + .then(async (result) => { + // B2: record successful file writes in the per-task change journal. + // Only a real commit produces an entry (an empty or failed save + // resolves to undefined / rejects), and non-write checkpoint calls + // (e.g. the task-start baseline) pass no `write` value at all. + if (result?.commit && write) { + const writes = Array.isArray(write) ? write : [write] + const globalStorageDir = task.providerRef.deref()?.context.globalStorageUri.fsPath + if (globalStorageDir) { + // Append sequentially so journal lines preserve write order. A + // journal failure is logged here and never propagates to the + // checkpoint error handler (checkpoints stay enabled). + try { + for (const w of writes) { + await appendChange(globalStorageDir, task.taskId, { + path: w.path, + operation: w.operation, + checkpointId: result.commit, + ...(w.diffStats ? { diffStats: w.diffStats } : {}), + }) + } + } catch (err) { + console.error("[Task#checkpointSave] failed to append change journal entry", err) + } + } + + // B3a: emit the per-step change card now that the checkpoint commit + // exists. The card reuses the approval diff/stats the tool already + // computed and is always emitted (auto-approved steps included); + // a card failure is logged and never disables checkpoints. + try { + const state = await task.providerRef.deref()?.getState() + const card = buildChangeCardPayload(result.commit, writes, state?.changeCardDetail) + await task.say("change_card", JSON.stringify(card), undefined, undefined, undefined, undefined, { + isNonInteractive: true, + }) + } catch (err) { + console.error("[Task#checkpointSave] failed to emit change card", err) + } + } + return result + }) .catch((err) => { console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err) task.enableCheckpoints = false diff --git a/src/core/checkpoints/rollback.ts b/src/core/checkpoints/rollback.ts new file mode 100644 index 0000000000..508789c957 --- /dev/null +++ b/src/core/checkpoints/rollback.ts @@ -0,0 +1,277 @@ +/** + * Per-file / per-step checkpoint rollback (B3c). + * + * "Rollback" means UNDOING the change-card step: every file the step touched + * is restored to the state it had BEFORE the step ran. The restore target is + * resolved from the B2 change journal (`changes.jsonl`), whose entries record + * — in write order — the checkpoint commit each successful write produced: + * + * - a file written by an earlier step resolves to that earlier entry's + * checkpoint (the file's post-write state after the previous step, i.e. its + * pre-step state); + * - a file no earlier step wrote resolves to the task-start baseline + * (`service.baseHash`): undoing the step that created a file removes it, + * and undoing the step that deleted one brings it back; + * - `restoreLatestFile` is the forward direction: it brings a file back to + * the content of its most recent recorded write (a successful no-op when + * the task never wrote the file). + * + * A file is only rolled back from the card of its most recent step: undoing + * an older step for a file that a later step wrote again would overwrite the + * newer state, so such a rollback is rejected (a full checkpoint restore + * still reaches any older state). A journal that cannot be located or read + * fails the restore instead of masquerading as "the task wrote nothing". + * + * Restores reuse the existing shadow-git service (`getCheckpointService` → + * `RepoPerTaskCheckpointService.restoreFile`, the same instance whose + * `restoreCheckpoint` the checkpoints UI uses) — nothing is forked. Only the + * named file's working-tree content is replaced; the shadow repo's HEAD and + * the checkpoint list are untouched (unlike a full `restoreCheckpoint`). + */ +import type { Task } from "../task/Task" + +import { getCheckpointService } from "./index" +import { loadChanges, type ChangeJournalEntry } from "./changeJournal" + +export interface RollbackFileOutcome { + filePath: string + success: boolean + error?: string + /** True when `restoreLatestFile` found no recorded write: the working tree was left as-is. */ + noOp?: boolean +} + +export interface RollbackStepOutcome { + /** The step checkpoint the files were resolved against, when provided. */ + checkpointId?: string + files: RollbackFileOutcome[] +} + +const NOT_ENABLED_ERROR = "Checkpoints are not enabled for this task" +const NO_TARGET_ERROR = "No checkpoint available to restore" +const NOT_IN_STEP_ERROR = "File is not part of this step's checkpoint" +const NO_ENTRY_ERROR = "No change journal entry for this file" +const NO_JOURNAL_ERROR = "Change journal is unavailable for this task" +const NOT_LATEST_ERROR = "File was modified in a later step; roll back the latest change card first" + +type CheckpointService = NonNullable>> + +/** A readable journal (possibly legitimately empty) or the reason it could not be loaded. */ +type LoadedJournal = { entries: ChangeJournalEntry[] } | { error: string } + +// `undefined` = the journal cannot be located (provider reference gone); read failures (permissions, I/O) propagate. +async function loadTaskEntries(task: Task): Promise { + const globalStorageDir = task.providerRef.deref()?.context.globalStorageUri.fsPath + + return globalStorageDir ? loadChanges(globalStorageDir, task.taskId) : undefined +} + +// Single discriminated result for callers: a readable journal (possibly empty) or a failure. +async function loadTaskJournal(task: Task): Promise { + try { + const entries = await loadTaskEntries(task) + + if (entries === undefined) { + return { error: NO_JOURNAL_ERROR } + } + + return { entries } + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + return { error: `Change journal could not be read: ${message}` } + } +} + +/** The most recent journal entry recorded for `filePath`, if any. */ +function latestEntry(entries: ChangeJournalEntry[], filePath: string): ChangeJournalEntry | undefined { + for (let i = entries.length - 1; i >= 0; i--) { + if (entries[i].path === filePath) { + return entries[i] + } + } + return undefined +} + +/** + * Resolve the checkpoint a file must be restored from in order to undo the + * step identified by `stepCheckpointId` (see the module docstring for the + * resolution rules). + */ +function preStepRestoreTarget( + entries: ChangeJournalEntry[], + filePath: string, + stepCheckpointId: string, +): { target?: string; baseline?: boolean; error?: string } { + const fileEntries = entries.filter((entry) => entry.path === filePath) + const stepIndex = fileEntries.findIndex((entry) => entry.checkpointId === stepCheckpointId) + + if (stepIndex === -1) { + return { error: NOT_IN_STEP_ERROR } + } + + // Only the file's most recent step may be rolled back: restoring an older + // state would overwrite the file's newer writes. A multi-write step shares + // one checkpoint id, so compare on the latest entry's checkpoint id. + const latest = fileEntries[fileEntries.length - 1] + + if (latest.checkpointId !== stepCheckpointId) { + return { error: NOT_LATEST_ERROR } + } + + if (stepIndex === 0) { + return { baseline: true } + } + return { target: fileEntries[stepIndex - 1].checkpointId } +} + +/** + * Run one `restoreFile` and shape the outcome. A failed restore never throws + * out of the rollback API; it is reported on the per-file outcome instead. + */ +async function performRestore( + service: CheckpointService, + target: string, + filePath: string, +): Promise { + try { + await service.restoreFile(target, filePath) + return { filePath, success: true } + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + console.error(`[checkpointRollback] failed to restore ${filePath} from checkpoint ${target}: ${message}`) + return { filePath, success: false, error: message } + } +} + +/** + * Restore a single file to the state it had BEFORE the change-card step + * identified by `stepCheckpointId` — undoing that step's write to the file. + */ +export async function rollbackFile( + task: Task, + stepCheckpointId: string, + filePath: string, +): Promise { + const service = await getCheckpointService(task) + + if (!service) { + return { filePath, success: false, error: NOT_ENABLED_ERROR } + } + + const journal = await loadTaskJournal(task) + + if ("error" in journal) { + return { filePath, success: false, error: journal.error } + } + + const resolved = preStepRestoreTarget(journal.entries, filePath, stepCheckpointId) + + if (resolved.error) { + return { filePath, success: false, error: resolved.error } + } + + const target = resolved.baseline ? service.baseHash : resolved.target + + if (!target) { + return { filePath, success: false, error: NO_TARGET_ERROR } + } + + return performRestore(service, target, filePath) +} + +/** + * Restore every file of a step to the state it had before the step ran. + * + * `stepFiles` comes from the change-card payload (the B2 journal entries for + * the step's checkpoint id). Each file is resolved to its pre-step checkpoint + * through the journal (see the module docstring); without a step checkpoint id + * the latest journal entry per file is used instead (restoring to the file's + * last recorded state, the same direction as `restoreLatestFile`). + */ +export async function rollbackStep( + task: Task, + stepFiles: string[], + stepCheckpointId?: string, +): Promise { + const service = await getCheckpointService(task) + + if (!service) { + return { + checkpointId: stepCheckpointId, + files: stepFiles.map((filePath) => ({ filePath, success: false, error: NOT_ENABLED_ERROR })), + } + } + + const journal = await loadTaskJournal(task) + const files: RollbackFileOutcome[] = [] + + if ("error" in journal) { + const journalError = journal.error + return { + checkpointId: stepCheckpointId, + files: stepFiles.map((filePath) => ({ filePath, success: false, error: journalError })), + } + } + + const entries = journal.entries + + for (const filePath of stepFiles) { + if (stepCheckpointId) { + const resolved = preStepRestoreTarget(entries, filePath, stepCheckpointId) + + if (resolved.error) { + files.push({ filePath, success: false, error: resolved.error }) + continue + } + + const target = resolved.baseline ? service.baseHash : resolved.target + + if (!target) { + files.push({ filePath, success: false, error: NO_TARGET_ERROR }) + continue + } + + files.push(await performRestore(service, target, filePath)) + continue + } + + const latest = latestEntry(entries, filePath) + + if (!latest) { + files.push({ filePath, success: false, error: NO_ENTRY_ERROR }) + continue + } + + files.push(await performRestore(service, latest.checkpointId, filePath)) + } + + return { checkpointId: stepCheckpointId, files } +} + +/** + * Restore one file to the latest recorded version: the content of its most + * recent successful write checkpoint (the forward direction to a rollback). + * A file the task never wrote has no recorded version — the working tree is + * left as-is and the outcome is a successful no-op. + */ +export async function restoreLatestFile(task: Task, filePath: string): Promise { + const service = await getCheckpointService(task) + + if (!service) { + return { filePath, success: false, error: NOT_ENABLED_ERROR } + } + + const journal = await loadTaskJournal(task) + + if ("error" in journal) { + return { filePath, success: false, error: journal.error } + } + + const latest = latestEntry(journal.entries, filePath) + + if (!latest) { + return { filePath, success: true, noOp: true } + } + + return performRestore(service, latest.checkpointId, filePath) +} diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 349d9c51d3..61203a3046 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -314,6 +314,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 @@ -2492,6 +2495,17 @@ export class Task extends EventEmitter implements TaskLike { // arm needed. void getCheckpointService(this) + // B1 task-start baseline: a suppressed pre-task root commit (default-on). + if (!this.taskStartBaselineDone) { + this.taskStartBaselineDone = true + const baselineEnabled = (await this.providerRef.deref()?.getState())?.perWriteCheckpoints + if (baselineEnabled !== false) { + // allowEmpty=true so a clean workspace still produces the baseline + // commit; awaited so the first per-write checkpoint cannot interleave. + await this.checkpointSave(true, true) + } + } + let nextUserContent = userContent let includeFileDetails = true diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 37e228f887..4ca5d7495c 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -3289,6 +3289,98 @@ describe("Cline", () => { }) }) + describe("task-start baseline (B1 perWriteCheckpoints)", () => { + it("records one suppressed baseline checkpoint per Task instance at loop start", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + task.abort = true + + await taskAccess.initiateTaskLoop([]) + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).toHaveBeenCalledOnce() + expect(saveSpy).toHaveBeenCalledWith(true, true) + }) + + 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) + + task.abort = 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 }) + + task.abort = true + + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("awaits the baseline checkpoint before entering the request loop", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline await task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveSpy = vi + .spyOn(task, "checkpointSave") + .mockImplementation(() => new Promise((resolve) => (resolveSave = resolve))) + const requestSpy = vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...(await mockProvider.getState()) }) + const loopPromise = taskAccess.initiateTaskLoop([]) + + // The loop must not enter while the baseline checkpoint is still in flight. + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(saveSpy).toHaveBeenCalledOnce() + expect(requestSpy).not.toHaveBeenCalled() + + resolveSave() + await loopPromise + expect(requestSpy).toHaveBeenCalled() + }) + }) + describe("start()", () => { it("should be a no-op if the task was already started in the constructor", () => { const task = new Task({ diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index 56b2bf8909..bba826ea4d 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -6,6 +6,8 @@ import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Task } from "../task/Task" +import { checkpointSave } from "../checkpoints" +import { checkAutoApproval } from "../auto-approval" import { formatResponse } from "../prompts/responses" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" @@ -20,6 +22,18 @@ interface ApplyPatchParams { patch: string } +/** + * B2: result of a single file operation within a patch. `succeeded` controls + * the whole-patch success state (and therefore the per-patch checkpoint), + * while `wrote` records whether the operation actually wrote a file — a no-op + * update must not produce a change-journal entry for a file that was never + * written. + */ +interface ApplyPatchFileOpResult { + succeeded: boolean + wrote: boolean +} + export class ApplyPatchTool extends BaseTool<"apply_patch"> { readonly name = "apply_patch" as const @@ -102,7 +116,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { return } - // Process each file change + // Process each file change. The handlers report whether their file + // operation succeeded (which controls the whole-patch checkpoint) and + // whether it actually wrote a file (which controls the change journal + // — a no-op update must not be journaled). A rejected approval or a + // failed local write never gets checkpointed as a success. + let patchSucceeded = true + const successfulChanges: ApplyPatchFileChange[] = [] for (const change of changes) { const relPath = change.path const absolutePath = path.resolve(task.cwd, relPath) @@ -112,7 +132,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,17 +145,100 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (change.type === "add") { // Create new file - await this.handleAddFile(change, absolutePath, relPath, task, callbacks, isWriteProtected) + const addResult = await this.handleAddFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + ) + patchSucceeded = addResult.succeeded && patchSucceeded + if (addResult.wrote) { + successfulChanges.push(change) + } } else if (change.type === "delete") { // Delete file - await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected) + const deleteResult = await this.handleDeleteFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + ) + patchSucceeded = deleteResult.succeeded && patchSucceeded + if (deleteResult.wrote) { + successfulChanges.push(change) + } } else if (change.type === "update") { - // Update file - await this.handleUpdateFile(change, absolutePath, relPath, task, callbacks, isWriteProtected) + // Update file (a no-op update succeeds without writing) + const updateResult = await this.handleUpdateFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + ) + patchSucceeded = updateResult.succeeded && patchSucceeded + if (updateResult.wrote) { + successfulChanges.push(change) + } } } - 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) { + const perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints + if (perWriteCheckpoints !== false) { + // B2: one journal entry per file that was actually written by + // the patch (the simplest correct design for multi-file patches), + // all referencing the single checkpoint above. A no-op update + // contributes no entry because nothing was written. `movePath`, + // when present, is the file's final location. B3a: the per-file + // approval diff/stats and auto-approval state, retained by the + // handlers, feed the per-step change card. + // Awaited: a later write must not interleave with this patch's + // staging/commit/journal/change-card work. checkpointSave never + // rejects (service call wrapped in try/catch upstream). + await checkpointSave( + task, + false, + true, + successfulChanges.map((change) => ({ + path: change.movePath ?? change.path, + operation: change.type === "add" ? "create" : change.type, + ...(change.diffStats + ? { + diffStats: { + additions: change.diffStats.added, + deletions: change.diffStats.removed, + }, + } + : {}), + ...(change.diff ? { diff: change.diff } : {}), + ...(change.autoApproved ? { autoApproved: true } : {}), + })), + ) + } + } } catch (error) { await handleError("apply patch", error as Error) await task.diffViewProvider.reset() @@ -144,7 +252,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + ): Promise { const { askApproval, pushToolResult } = callbacks // Check if file already exists @@ -155,7 +263,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `File already exists: ${relPath}. Use Update File instead.` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return + return { succeeded: false, wrote: false } } const newContent = change.newContent || "" @@ -194,6 +302,21 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { diffStats, } satisfies ClineSayTool) + // B3a: retain the approval diff/stats and auto-approval state so the + // post-loop checkpoint hook can build the per-step change card. + change.diff = sanitizedDiff + change.diffStats = diffStats + change.autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Show diff view if focus disruption prevention is disabled if (!isPreventFocusDisruptionEnabled) { await task.diffViewProvider.open(relPath) @@ -209,7 +332,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 @@ -227,15 +350,17 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { pushToolResult(message) await task.diffViewProvider.reset() task.processQueuedMessages() + return { succeeded: true, wrote: true } } private async handleDeleteFile( + change: ApplyPatchFileChange, absolutePath: string, relPath: string, task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + ): Promise { const { askApproval, pushToolResult } = callbacks // Check if file exists @@ -246,7 +371,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `File not found: ${relPath}. Cannot delete a non-existent file.` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return + return { succeeded: false, wrote: false } } const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) @@ -264,11 +389,24 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { isProtected: isWriteProtected, } satisfies ClineSayTool) + // B3a: auto-approval state feeds the per-step change card (deletes have + // no diff to thread). + change.autoApproved = + ( + await checkAutoApproval({ + state: await task.providerRef.deref()?.getState(), + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) if (!didApprove) { pushToolResult("Delete operation was rejected by the user.") - return + return { succeeded: false, wrote: false } } // Delete the file @@ -278,12 +416,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const errorMessage = `Failed to delete file '${relPath}': ${error instanceof Error ? error.message : String(error)}` await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) - return + return { succeeded: false, wrote: false } } task.didEditFile = true pushToolResult(`Successfully deleted ${relPath}`) task.processQueuedMessages() + return { succeeded: true, wrote: true } } private async handleUpdateFile( @@ -293,9 +432,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + ): Promise { const { askApproval, pushToolResult } = callbacks + // A move reports failure when the original file cannot be deleted + // after the copy (both paths would remain on disk). + let moveSucceeded = true + // Check if file exists const fileExists = await fileExistsAtPath(absolutePath) if (!fileExists) { @@ -304,7 +447,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 +461,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 @@ -351,6 +498,21 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { diffStats, } satisfies ClineSayTool) + // B3a: retain the approval diff/stats and auto-approval state so the + // post-loop checkpoint hook can build the per-step change card. + change.diff = sanitizedDiff + change.diffStats = diffStats + change.autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Show diff view if focus disruption prevention is disabled if (!isPreventFocusDisruptionEnabled) { await task.diffViewProvider.open(relPath) @@ -366,7 +528,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 +541,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 +553,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 +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 } } // Save new content to the new path @@ -422,11 +584,19 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await fs.writeFile(moveAbsolutePath, newContent, "utf8") } - // Delete the original file + // Delete the original file. A failed deletion leaves both paths on + // disk, so the move must be reported as a failure rather than + // checkpointed and journaled as a completed move. try { await fs.unlink(absolutePath) } catch (error) { + moveSucceeded = false console.error(`Failed to delete original file after move: ${error}`) + task.consecutiveMistakeCount++ + task.recordToolError("apply_patch") + const errorMessage = `Move of '${relPath}' to '${change.movePath}' failed: could not delete the original file.` + await task.say("error", errorMessage) + pushToolResult(formatResponse.toolError(errorMessage)) } await task.fileContextTracker.trackFileContext(change.movePath, "roo_edited" as RecordSource) @@ -447,6 +617,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { pushToolResult(message) await task.diffViewProvider.reset() task.processQueuedMessages() + if (!moveSucceeded) { + // The destination file was written on disk before the source + // deletion failed, so the write must still be checkpointed and + // journaled; the move itself is reported as failed. + return { succeeded: false, wrote: true } + } + return { succeeded: true, wrote: true } } 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..03c20ccee2 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -11,6 +11,8 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" +import { checkpointSave } from "../../core/checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -392,6 +394,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 ?? true const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -463,6 +466,34 @@ 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. B3a threads the approval + // diff (for the change card) and whether the step was auto- + // approved (auto-approved steps always get the compact card). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + // Awaited: a later tool block must not interleave with this edit's + // staging/commit/journal/change-card work. checkpointSave never + // rejects (service call wrapped in try/catch upstream). + await checkpointSave(task, false, true, { + path: relPath, + operation: isNewFile ? "create" : "update", + diffStats: diffStats ? { additions: diffStats.added, deletions: diffStats.removed } : undefined, + ...(sanitizedDiff ? { diff: sanitizedDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }) + } + await task.diffViewProvider.reset() this.resetPartialState() diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index ae026b4b86..ea9aec48f1 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -13,7 +13,9 @@ import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { unescapeHtmlEntities } from "../../utils/text-normalization" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" -import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats" +import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff, type DiffStats } from "../diff/stats" +import { checkpointSave } from "../checkpoints" +import { checkAutoApproval } from "../auto-approval" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -103,11 +105,20 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const perWriteCheckpoints = state?.perWriteCheckpoints ?? true const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, ) + // B2: the approval-diff stats for the write, shared by both the + // approval message and the change-journal entry below. B3a also + // reuses the sanitized unified diff itself for the per-step change + // card (never recomputed). + let approvalDiffStats: DiffStats | null = null + let approvalDiff = "" + let completeMessage = "" + if (isPreventFocusDisruptionEnabled) { task.diffViewProvider.editType = fileExists ? "modify" : "create" if (fileExists) { @@ -121,10 +132,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) - const completeMessage = JSON.stringify({ + approvalDiffStats = computeDiffStats(unified) + approvalDiff = unified + completeMessage = JSON.stringify({ ...sharedMessageProps, content: unified, - diffStats: computeDiffStats(unified) || undefined, + diffStats: approvalDiffStats || undefined, } satisfies ClineSayTool) const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) @@ -153,10 +166,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { ? formatResponse.createPrettyPatch(relPath, task.diffViewProvider.originalContent, newContent) : convertNewFileToUnifiedDiff(newContent, relPath) unified = sanitizeUnifiedDiff(unified) - const completeMessage = JSON.stringify({ + approvalDiffStats = computeDiffStats(unified) + approvalDiff = unified + completeMessage = JSON.stringify({ ...sharedMessageProps, content: unified, - diffStats: computeDiffStats(unified) || undefined, + diffStats: approvalDiffStats || undefined, } satisfies ClineSayTool) const didApprove = await askApproval("tool", completeMessage, undefined, isWriteProtected) @@ -179,6 +194,35 @@ 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. B3a threads + // the approval diff (for the change card) and whether the step was + // auto-approved (auto-approved steps always get the compact card). + const autoApproved = + ( + await checkAutoApproval({ + state, + cwd: task.cwd, + ask: "tool", + text: completeMessage, + isProtected: isWriteProtected, + }) + ).decision === "approve" + await checkpointSave(task, false, true, { + path: relPath, + operation: fileExists ? "update" : "create", + diffStats: approvalDiffStats + ? { additions: approvalDiffStats.added, deletions: approvalDiffStats.removed } + : undefined, + ...(approvalDiff ? { diff: approvalDiff } : {}), + ...(autoApproved ? { autoApproved: true } : {}), + }).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 72ffb112bc..10c90d36a6 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) @@ -93,4 +139,578 @@ 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, + }) + + 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, + }) + + 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, + }) + + expect(mockTask.recordToolError).toHaveBeenCalledWith("apply_patch") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + }) + + describe("checkpoint only for fully successful patches (B1)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + const deletePatch = `*** Begin Patch +*** Delete File: src/obsolete.ts +*** End Patch` + const addPatch = `*** Begin Patch +*** Add File: src/new.ts ++hello ++world +*** End Patch` + const updatePatch = `*** Begin Patch +*** Update File: src/test.ts +@@ +-original file content ++modified content +*** End Patch` + const updateNoDiffPatch = `*** Begin Patch +*** Update File: src/test.ts +@@ +-original file content ++original file content +*** End Patch` + const movePatch = `*** Begin Patch +*** Update File: src/test.ts +*** Move to: src/moved.ts +@@ +-original file content ++modified content +*** End Patch` + + it("does not record a checkpoint when the user rejects the patch", async () => { + // Rejected approval: the handler early-returns without recording a + // tool error, so the success flag must come from the handler itself. + mockAskApproval.mockResolvedValue(false) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + 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, + }) + + 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, + }) + + 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, + }) + + 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, + }) + + 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, + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("File saved successfully") + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + + it("does not record a checkpoint when the file to update does not exist", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + 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, + }) + + 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, + }) + + 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, + }) + + 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, + }) + + 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, + }) + + 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, + }) + + 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, + }) + + // 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, + }) + + 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, + }) + + // path is platform-dependent (Windows resolves cwd to a drive path); + // assert on the written content instead. + expect(mockedFsPromises.default.writeFile).toHaveBeenCalledWith( + expect.any(String), + "modified content\n", + "utf8", + ) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + + it("awaits the patch checkpoint before execute settles", async () => { + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveDeferred = new Promise((resolve) => (resolveSave = resolve)) + mockedCheckpointSave.mockImplementationOnce(() => saveDeferred) + + const executePromise = tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // execute must not settle while the checkpoint is still in flight: + // a later write would otherwise interleave with this patch's staged work. + let settled = false + void executePromise.finally(() => (settled = true)) + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(settled).toBe(false) + + resolveSave() + await executePromise + expect(settled).toBe(true) + }) + + it("records a checkpoint when the in-place update succeeds", async () => { + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("File saved successfully") + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + + it("records one journal write per file change for a multi-file patch", async () => { + // src/a.ts does not exist (add); src/b.ts does (update). + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(!String(filePath).toLowerCase().endsWith("a.ts")), + ) + const multiPatch = [ + "*** Begin Patch", + "*** Add File: src/a.ts", + "+alpha", + "*** Update File: src/b.ts", + "@@", + "-original file content", + "+second content", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: multiPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // B3a: each write threads the approval diff and stats computed by its + // handler so the per-step change card can reuse them verbatim. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/a.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: expect.stringContaining("+alpha"), + }, + { + path: "src/b.ts", + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: expect.stringContaining("+second content"), + }, + ]) + }) + }) + + describe("change-card threading (B3a)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + const deletePatch = `*** Begin Patch + *** Delete File: src/obsolete.ts + *** End Patch` + + it("threads autoApproved into the checkpoint writes for auto-approved steps", async () => { + // B3a: auto-approved steps carry autoApproved on every write so + // checkpointSave can force the compact (summary) change card. + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ autoApprovalEnabled: true, alwaysAllowWrite: true }), + }) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/obsolete.ts", operation: "delete", autoApproved: true }, + ]) + }) + + it("omits autoApproved when the step is not auto-approved", async () => { + // The default provider state ({}) disables auto-approval, so no write + // carries the autoApproved flag and the card follows the user setting. + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { path: "src/obsolete.ts", operation: "delete" }, + ]) + }) + + it("journals only the files actually written for a mixed no-op and write patch", async () => { + // src/same.ts exists and the hunk rewrites identical content (a + // no-op update); src/new.ts does not exist (a real write). + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(!String(filePath).toLowerCase().endsWith("new.ts")), + ) + const mixedPatch = [ + "*** Begin Patch", + "*** Update File: src/same.ts", + "@@", + "-original file content", + "+original file content", + "*** Add File: src/new.ts", + "+fresh content", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: mixedPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The no-op update is reported to the model... + expect(mockPushToolResult).toHaveBeenCalledWith("No changes needed for 'src/same.ts'") + // ...but the journal documents only the file that was actually + // written, even though the whole patch succeeded. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/new.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: expect.stringContaining("+fresh content"), + }, + ]) + }) + + it("still checkpoints the successful subset when a later hunk fails", async () => { + // src/first.ts already exists (the add fails); src/second.ts does not + // (the add writes). The whole patch fails, but the written file is + // still documented by the checkpoint and journal. + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(String(filePath).toLowerCase().endsWith("first.ts")), + ) + const partialPatch = [ + "*** Begin Patch", + "*** Add File: src/first.ts", + "+boom", + "*** Add File: src/second.ts", + "+fresh", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: partialPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The failed operation is reported to the model... + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("File already exists")) + // ...and the successful subset is checkpointed and journaled. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true, [ + { + path: "src/second.ts", + operation: "create", + diffStats: { additions: 1, deletions: 0 }, + diff: expect.stringContaining("+fresh"), + }, + ]) + }) + + it("reports a failed move when the original file cannot be deleted", async () => { + mockedFsPromises.default.unlink.mockRejectedValueOnce(new Error("EBUSY: resource busy")) + const movePatch = [ + "*** Begin Patch", + "*** Update File: src/old.ts", + "*** Move to: src/new-location.ts", + "@@", + "-original file content", + "+new content", + "*** End Patch", + ].join(String.fromCharCode(10)) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + }) + + // The copy succeeded but the source still exists, so the move is + // reported as a failed tool error - but the destination write was + // made on disk and must still be covered by the checkpoint/journal. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith( + mockTask as Task, + false, + true, + [expect.objectContaining({ path: "src/new-location.ts", operation: "update" })], + ) + expect(mockTask.recordToolError).toHaveBeenCalledWith("apply_patch") + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("could not delete the original file"), + ) + }) + }) }) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 1ff8d52a8d..2f975966b0 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", () => ({ @@ -774,4 +785,123 @@ describe("editFileTool", () => { expect(mockAskApproval).toHaveBeenCalled() }) }) + + describe("per-write checkpoints (B1)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + + it("records one suppressed checkpoint after a successful edit (default-on)", async () => { + await executeEditFileTool({}) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // B2: the write info threads the path, operation, and the approval + // diff stats into the checkpoint hook. B3a: the approval diff itself is + // threaded verbatim for the per-step change card. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + }) + }) + + it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { + mockTask.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + perWriteCheckpoints: false, + }), + }) + + await executeEditFileTool({}) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the edit fails", async () => { + mockTask.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeEditFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("edit_file", expect.any(Error)) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("records the checkpoint with a create operation for a new file", async () => { + await executeEditFileTool({ old_string: "", new_string: "New file content" }, { fileExists: false }) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + }) + }) + + it("omits diff stats from the checkpoint write when the diff has no stats", async () => { + // A null approval diff produces no diffStats on the journal write. + // The diff itself is still threaded for the change card (B3a). + vi.mocked(computeDiffStats).mockReturnValueOnce(null) + + await executeEditFileTool({}) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + diff: "mock-diff", + }) + }) + + it("threads autoApproved into the checkpoint write for auto-approved steps", async () => { + // B3a: when the step is auto-approved the checkpoint write carries + // autoApproved so checkpointSave can force the compact change card. + mockTask.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + autoApprovalEnabled: true, + alwaysAllowWrite: true, + }), + }) + + await executeEditFileTool({}) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true, { + path: testFilePath, + operation: "update", + diffStats: { additions: 1, deletions: 1 }, + diff: "mock-diff", + autoApproved: true, + }) + }) + + it("awaits the edit checkpoint before execute settles", async () => { + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveDeferred = new Promise((resolve) => (resolveSave = resolve)) + mockedCheckpointSave.mockImplementationOnce(() => saveDeferred) + + const executePromise = executeEditFileTool({}) + + // execute must not settle while the checkpoint is still in flight: + // a later tool block would otherwise interleave with this edit's staged work. + let settled = false + void executePromise.finally(() => (settled = true)) + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(settled).toBe(false) + + resolveSave() + await executePromise + expect(settled).toBe(true) + }) + }) }) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 52a7e3c052..cc2446288f 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -8,7 +8,10 @@ import { getReadablePath } from "../../../utils/path" import { unescapeHtmlEntities } from "../../../utils/text-normalization" import { everyLineHasLineNumbers, stripLineNumbers } from "../../../integrations/misc/extract-text" import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools" +import { checkpointSave } from "../../checkpoints" +import { formatResponse } from "../../prompts/responses" import { writeToFileTool } from "../WriteToFileTool" +import { convertNewFileToUnifiedDiff, sanitizeUnifiedDiff } from "../../diff/stats" vi.mock("path", async () => { const originalPath = await vi.importActual("path") @@ -89,6 +92,10 @@ vi.mock("../../ignore/RooIgnoreController", () => ({ }, })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + describe("writeToFileTool", () => { // Test data const testFilePath = "test/file.txt" @@ -96,6 +103,10 @@ describe("writeToFileTool", () => { const testContent = "Line 1\nLine 2\nLine 3" const testContentWithMarkdown = "```javascript\nLine 1\nLine 2\n```" + // The exact approval diff the tool computes for a new file (B3a threads it + // into the checkpoint write for the per-step change card). + const newFileApprovalDiff = sanitizeUnifiedDiff(convertNewFileToUnifiedDiff(testContent, testFilePath)) + // Mocked functions with correct types const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction const mockedCreateDirectoriesForFile = createDirectoriesForFile as MockedFunction @@ -156,6 +167,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 ( @@ -472,4 +484,151 @@ describe("writeToFileTool", () => { expect(mockHandleError).toHaveBeenCalledWith("handling partial write_to_file", expect.any(Error)) }) }) + + describe("per-write checkpoints (B1)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + + it("records one suppressed checkpoint after a successful write (default-on)", async () => { + await executeWriteFileTool({}) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // B2: the write info threads the path, operation, and the approval + // diff stats (3 added lines, 0 removed) into the checkpoint hook. + // B3a: the approval diff itself is threaded verbatim for the + // per-step change card. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + diff: newFileApprovalDiff, + }) + }) + + it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { + mockCline.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi + .fn() + .mockResolvedValue({ diagnosticsEnabled: true, writeDelayMs: 1000, perWriteCheckpoints: false }), + }) + + await executeWriteFileTool({}) + + expect(mockCline.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the write fails", async () => { + mockCline.diffViewProvider.open.mockRejectedValue(new Error("write failed")) + + await executeWriteFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("writing file", expect.any(Error)) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("waits for the per-write checkpoint before the tool completes", async () => { + let checkpointStarted = false + let releaseCheckpoint: () => void = () => {} + mockedCheckpointSave.mockImplementationOnce(() => { + checkpointStarted = true + return new Promise((resolve) => { + releaseCheckpoint = () => resolve(undefined) + }) + }) + const processQueuedSpy = vi.fn() + mockCline.processQueuedMessages = processQueuedSpy + + const toolPromise = executeWriteFileTool({}) + + // Advance microtasks until the tool reaches the checkpoint call (all + // preceding awaits are mocked resolutions, no real timers involved). + for (let i = 0; i < 50 && !checkpointStarted; i++) { + await Promise.resolve() + } + expect(checkpointStarted).toBe(true) + + let settled = false + void toolPromise.then(() => { + settled = true + }) + + // The tool must not complete while the checkpoint is still + // staging/committing: a later write started by the task loop would + // otherwise collapse into the same (or a missing) commit. + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(settled).toBe(false) + expect(processQueuedSpy).not.toHaveBeenCalled() + + releaseCheckpoint() + await toolPromise + expect(settled).toBe(true) + expect(processQueuedSpy).toHaveBeenCalledOnce() + }) + + it("threads write info with approval diff stats when the prevent-focus-disruption experiment is enabled", async () => { + mockCline.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: { preventFocusDisruption: true }, + }), + }) + + await executeWriteFileTool({}) + + // The experiment branch saves directly (no diff view) and still + // journals the write through the same single checkpoint hook, carrying + // the approval diff for the per-step change card (B3a). + expect(mockCline.diffViewProvider.saveDirectly).toHaveBeenCalledWith( + testFilePath, + testContent, + false, + true, + 1000, + ) + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + diff: newFileApprovalDiff, + }) + }) + + it("omits diff stats from the checkpoint write when the approval diff is empty", async () => { + // Writing identical content to an existing file produces an empty + // approval diff, so the checkpoint write carries no diffStats. + vi.mocked(formatResponse.createPrettyPatch).mockReturnValueOnce("") + + await executeWriteFileTool({}, { fileExists: true }) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "update", + }) + }) + + it("threads autoApproved into the checkpoint write for auto-approved steps", async () => { + // B3a: when the step is auto-approved the checkpoint write carries + // autoApproved so checkpointSave can force the compact change card. + mockCline.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + autoApprovalEnabled: true, + alwaysAllowWrite: true, + }), + }) + + await executeWriteFileTool({}) + + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true, { + path: testFilePath, + operation: "create", + diffStats: { additions: 3, deletions: 0 }, + diff: newFileApprovalDiff, + autoApproved: true, + }) + }) + }) }) diff --git a/src/core/tools/apply-patch/apply.ts b/src/core/tools/apply-patch/apply.ts index 4ab377f732..64009d27bf 100644 --- a/src/core/tools/apply-patch/apply.ts +++ b/src/core/tools/apply-patch/apply.ts @@ -29,6 +29,15 @@ export interface ApplyPatchFileChange { originalContent?: string /** New content (for add/update) */ newContent?: string + /** + * B3a: the unified approval diff for this file (computed by the tool + * handler), reused by the per-step change card. + */ + diff?: string + /** B3a: { added, removed } stats of the approval diff, when computable. */ + diffStats?: { added: number; removed: number } + /** B3a: whether this file's approval was auto-approved. */ + autoApproved?: boolean } /** diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 4621cb3fc4..52bb209922 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -51,6 +51,8 @@ import { ORGANIZATION_ALLOW_ALL, DEFAULT_MODES, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_PER_WRITE_CHECKPOINTS, + DEFAULT_CHANGE_CARD_DETAIL, getModelId, isRetiredProvider, providerIdentifiers, @@ -2556,6 +2558,8 @@ export class ClineProvider ttsSpeed, enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, + changeCardDetail, soundVolume, writeDelayMs, diffFuzzyThreshold, @@ -2715,6 +2719,8 @@ export class ClineProvider ttsSpeed: ttsSpeed ?? 1.0, enableCheckpoints: enableCheckpoints ?? true, checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, + changeCardDetail: changeCardDetail ?? DEFAULT_CHANGE_CARD_DETAIL, shouldShowAnnouncement: telemetrySetting !== "unset" && lastShownAnnouncementId !== this.latestAnnouncementId, allowedCommands: mergedAllowedCommands, @@ -2951,6 +2957,8 @@ export class ClineProvider ttsSpeed: stateValues.ttsSpeed ?? 1.0, enableCheckpoints: stateValues.enableCheckpoints ?? true, checkpointTimeout: stateValues.checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: stateValues.perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, + changeCardDetail: stateValues.changeCardDetail ?? DEFAULT_CHANGE_CARD_DETAIL, soundVolume: stateValues.soundVolume, writeDelayMs: stateValues.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS, diffFuzzyThreshold: stateValues.diffFuzzyThreshold ?? DEFAULT_DIFF_FUZZY_THRESHOLD, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 731124cccc..caf6bfd002 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -720,6 +720,8 @@ describe("ClineProvider", () => { soundEnabled: false, ttsEnabled: false, enableCheckpoints: false, + perWriteCheckpoints: false, + changeCardDetail: "summary", writeDelayMs: 1000, mcpEnabled: true, mode: defaultModeSlug, @@ -1401,6 +1403,81 @@ describe("ClineProvider", () => { expect(state.destructiveCommandGuardEnabled).toBe(false) }) + test("getState returns the saved per-write checkpoints setting", async () => { + await provider.contextProxy.setValue("perWriteCheckpoints", false) + + const state = await provider.getState() + + expect(state.perWriteCheckpoints).toBe(false) + }) + + test("getState defaults per-write checkpoints to true when unset", async () => { + const state = await provider.getState() + + expect(state.perWriteCheckpoints).toBe(true) + }) + + test("getStateToPostToWebview returns the saved per-write checkpoints setting", async () => { + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("perWriteCheckpoints", true) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(true) + }) + + test("getStateToPostToWebview returns false when per-write checkpoints is saved as false", async () => { + // The default is also true, so only an explicit false proves that the + // stored value (rather than the default) reaches the webview state. + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("perWriteCheckpoints", false) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(false) + }) + + test("getStateToPostToWebview defaults per-write checkpoints to true when unset", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(true) + }) + + test("getState returns the saved changeCardDetail setting", async () => { + await provider.contextProxy.setValue("changeCardDetail", "full") + + const state = await provider.getState() + + expect(state.changeCardDetail).toBe("full") + }) + + test("getState defaults changeCardDetail to summary when unset", async () => { + const state = await provider.getState() + + expect(state.changeCardDetail).toBe("summary") + }) + + test("getStateToPostToWebview returns the saved changeCardDetail setting", async () => { + // The default is "summary", so only an explicit "full" proves that the + // stored value (rather than the default) reaches the webview state. + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("changeCardDetail", "full") + + const state = await provider.getStateToPostToWebview() + + expect(state.changeCardDetail).toBe("full") + }) + + test("getStateToPostToWebview defaults changeCardDetail to summary when unset", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const state = await provider.getStateToPostToWebview() + + expect(state.changeCardDetail).toBe("summary") + }) + test("language is set to VSCode language", async () => { // Mock VSCode language as Spanish ;(vscode.env as any).language = "pt-BR" diff --git a/src/core/webview/__tests__/webviewMessageHandler.rollback.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.rollback.spec.ts new file mode 100644 index 0000000000..238e1daad5 --- /dev/null +++ b/src/core/webview/__tests__/webviewMessageHandler.rollback.spec.ts @@ -0,0 +1,374 @@ +// npx vitest run src/core/webview/__tests__/webviewMessageHandler.rollback.spec.ts +import { describe, expect, it, vi, beforeEach } from "vitest" + +import type { ExtensionMessage, WebviewMessage } from "@roo-code/types" + +import { webviewMessageHandler } from "../webviewMessageHandler" +import { restoreLatestFile, rollbackFile, rollbackStep } from "../../checkpoints/rollback" +import type { Task } from "../../task/Task" +import type { ClineProvider } from "../ClineProvider" + +// The rollback cases only call these two provider methods, so the provider +// double below is cast once at this boundary; the spy is shared so results +// can be asserted after the handler runs. +vi.mock("../../checkpoints/rollback", () => ({ + rollbackFile: vi.fn(), + rollbackStep: vi.fn(), + restoreLatestFile: vi.fn(), +})) + +vi.mock("vscode", () => ({ + window: { + showErrorMessage: vi.fn(), + }, + workspace: { + workspaceFolders: undefined, + }, +})) + +// The no-task failure posts localized copy; the extension i18n loader only +// populates resources outside tests, so the spec pins the English values the +// handler asks for (path is relative to this file: ../../../i18n = src/i18n). +vi.mock("../../../i18n", () => ({ + changeLanguage: vi.fn(), + t: (key: string) => { + const values: Record = { + "common:errors.message.no_active_task_to_roll_back": "No active task to roll back from", + "common:errors.message.no_active_task_to_restore": "No active task to restore from", + } + return values[key] ?? key + }, +})) + +// Structural mock: the handler only needs the task identity for these cases. +const mockTask = {} as Task +const postMessageToWebview = vi.fn(async (_message: ExtensionMessage) => undefined) + +function makeProvider(task: Task | undefined): ClineProvider { + const provider = { + getCurrentTask: () => task, + postMessageToWebview, + } + // Cast at the spec boundary: the rollback cases only read getCurrentTask() + // and observe postMessageToWebview calls on the structural double. + return provider as unknown as ClineProvider +} + +const provider = makeProvider(mockTask) + +describe("webviewMessageHandler - change card rollback", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + describe("checkpointRollbackFile", () => { + it("restores the file and posts the success outcome back to the webview", async () => { + vi.mocked(rollbackFile).mockResolvedValueOnce({ filePath: "src/a.ts", success: true }) + + await webviewMessageHandler(provider, { + type: "checkpointRollbackFile", + payload: { cardTs: 1000, checkpointId: "abc123", filePath: "src/a.ts" }, + }) + + expect(rollbackFile).toHaveBeenCalledWith(mockTask, "abc123", "src/a.ts") + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { cardTs: 1000, filePath: "src/a.ts", success: true }, + }) + }) + + it("posts the error outcome when the restore fails", async () => { + vi.mocked(rollbackFile).mockResolvedValueOnce({ + filePath: "src/a.ts", + success: false, + error: "checkpoint not found", + }) + + await webviewMessageHandler(provider, { + type: "checkpointRollbackFile", + payload: { cardTs: 1000, checkpointId: "abc123", filePath: "src/a.ts" }, + }) + + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + filePath: "src/a.ts", + success: false, + error: "checkpoint not found", + }, + }) + }) + + it("posts a correlated failure result when there is no current task", async () => { + const emptyProvider = makeProvider(undefined) + + await webviewMessageHandler(emptyProvider, { + type: "checkpointRollbackFile", + payload: { cardTs: 1000, checkpointId: "abc123", filePath: "src/a.ts" }, + }) + + expect(rollbackFile).not.toHaveBeenCalled() + // The requesting card must clear its pending state, so the handler + // posts a correlated failure instead of nothing. + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + filePath: "src/a.ts", + success: false, + error: "No active task to roll back from", + }, + }) + }) + + it("posts a correlated failure when the rollback itself throws", async () => { + vi.mocked(rollbackFile).mockRejectedValueOnce(new Error("git restore failed")) + + await webviewMessageHandler(provider, { + type: "checkpointRollbackFile", + payload: { cardTs: 1000, checkpointId: "abc123", filePath: "src/a.ts" }, + }) + + // The card must not stay pending: the handler turns the throw into a + // correlated failure result instead of dropping the message. + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + filePath: "src/a.ts", + success: false, + error: "Rollback failed: git restore failed", + }, + }) + }) + }) + + describe("checkpointRollbackStep", () => { + it("restores every step file and posts the aggregated outcome", async () => { + vi.mocked(rollbackStep).mockResolvedValueOnce({ + checkpointId: "abc123", + files: [ + { filePath: "src/a.ts", success: true }, + { filePath: "src/b.ts", success: true }, + ], + }) + + await webviewMessageHandler(provider, { + type: "checkpointRollbackStep", + payload: { cardTs: 1000, checkpointId: "abc123", filePaths: ["src/a.ts", "src/b.ts"] }, + }) + + expect(rollbackStep).toHaveBeenCalledWith(mockTask, ["src/a.ts", "src/b.ts"], "abc123") + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + success: true, + files: [ + { filePath: "src/a.ts", success: true }, + { filePath: "src/b.ts", success: true }, + ], + }, + }) + }) + + it("reports success false with the first failing file's error when a step file fails", async () => { + vi.mocked(rollbackStep).mockResolvedValueOnce({ + checkpointId: "abc123", + files: [ + { filePath: "src/a.ts", success: true }, + { filePath: "src/b.ts", success: false, error: "boom" }, + ], + }) + + await webviewMessageHandler(provider, { + type: "checkpointRollbackStep", + payload: { cardTs: 1000, filePaths: ["src/a.ts", "src/b.ts"] }, + }) + + // Without an explicit step checkpoint id the journal lookup is used. + expect(rollbackStep).toHaveBeenCalledWith(mockTask, ["src/a.ts", "src/b.ts"], undefined) + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + success: false, + error: "boom", + files: [ + { filePath: "src/a.ts", success: true }, + { filePath: "src/b.ts", success: false, error: "boom" }, + ], + }, + }) + }) + + it("posts a correlated failure result when there is no current task", async () => { + const emptyProvider = makeProvider(undefined) + + await webviewMessageHandler(emptyProvider, { + type: "checkpointRollbackStep", + payload: { cardTs: 1000, filePaths: ["src/a.ts"] }, + }) + + expect(rollbackStep).not.toHaveBeenCalled() + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + success: false, + error: "No active task to roll back from", + }, + }) + }) + + it("posts a correlated failure when the step rollback itself throws", async () => { + vi.mocked(rollbackStep).mockRejectedValueOnce(new Error("journal unreadable")) + + await webviewMessageHandler(provider, { + type: "checkpointRollbackStep", + payload: { cardTs: 1000, filePaths: ["src/a.ts"] }, + }) + + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + success: false, + error: "Rollback failed: journal unreadable", + }, + }) + }) + + it("ignores payloads that do not match the schema", async () => { + await webviewMessageHandler(provider, { + // Malformed on purpose (only the webview produces this message): the cast + // lets the spec reach the handler's safeParse rejection without `any`. + type: "checkpointRollbackFile", + payload: { cardTs: 1000 } as unknown as WebviewMessage["payload"], + }) + await webviewMessageHandler(provider, { + type: "checkpointRollbackStep", + payload: { cardTs: 1000, filePaths: [] }, + }) + await webviewMessageHandler(provider, { + type: "checkpointRestoreLatestFile", + payload: { cardTs: 1000 } as unknown as WebviewMessage["payload"], + }) + + expect(rollbackFile).not.toHaveBeenCalled() + expect(rollbackStep).not.toHaveBeenCalled() + expect(restoreLatestFile).not.toHaveBeenCalled() + expect(postMessageToWebview).not.toHaveBeenCalled() + }) + }) + + describe("checkpointRestoreLatestFile", () => { + it("restores the file to its latest recorded version and posts the success outcome", async () => { + vi.mocked(restoreLatestFile).mockResolvedValueOnce({ filePath: "src/a.ts", success: true }) + + await webviewMessageHandler(provider, { + type: "checkpointRestoreLatestFile", + payload: { cardTs: 1000, filePath: "src/a.ts" }, + }) + + expect(restoreLatestFile).toHaveBeenCalledWith(mockTask, "src/a.ts") + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + kind: "restore-latest", + filePath: "src/a.ts", + success: true, + }, + }) + }) + + it("flags a no-op restore-latest so the card can report it", async () => { + vi.mocked(restoreLatestFile).mockResolvedValueOnce({ filePath: "src/a.ts", success: true, noOp: true }) + + await webviewMessageHandler(provider, { + type: "checkpointRestoreLatestFile", + payload: { cardTs: 1000, filePath: "src/a.ts" }, + }) + + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + kind: "restore-latest", + filePath: "src/a.ts", + success: true, + noOp: true, + }, + }) + }) + + it("posts the error outcome when the restore fails", async () => { + vi.mocked(restoreLatestFile).mockResolvedValueOnce({ + filePath: "src/a.ts", + success: false, + error: "Checkpoints are not enabled for this task", + }) + + await webviewMessageHandler(provider, { + type: "checkpointRestoreLatestFile", + payload: { cardTs: 1000, filePath: "src/a.ts" }, + }) + + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + kind: "restore-latest", + filePath: "src/a.ts", + success: false, + error: "Checkpoints are not enabled for this task", + }, + }) + }) + + it("posts a correlated failure result when there is no current task", async () => { + const emptyProvider = makeProvider(undefined) + + await webviewMessageHandler(emptyProvider, { + type: "checkpointRestoreLatestFile", + payload: { cardTs: 1000, filePath: "src/a.ts" }, + }) + + expect(restoreLatestFile).not.toHaveBeenCalled() + // The requesting card must clear its pending state, so the handler + // posts a correlated failure instead of nothing. + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + kind: "restore-latest", + filePath: "src/a.ts", + success: false, + error: "No active task to restore from", + }, + }) + }) + + it("posts a correlated failure when the restore itself throws", async () => { + vi.mocked(restoreLatestFile).mockRejectedValueOnce(new Error("git checkout failed")) + + await webviewMessageHandler(provider, { + type: "checkpointRestoreLatestFile", + payload: { cardTs: 1000, filePath: "src/a.ts" }, + }) + + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + kind: "restore-latest", + filePath: "src/a.ts", + success: false, + error: "Restore failed: git checkout failed", + }, + }) + }) + }) +}) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 0dad65a480..2ba76f70c7 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -19,6 +19,9 @@ import { TelemetryEventName, RooCodeSettings, ExperimentId, + checkpointRollbackFilePayloadSchema, + checkpointRollbackStepPayloadSchema, + checkpointRestoreLatestFilePayloadSchema, checkoutDiffPayloadSchema, checkoutRestorePayloadSchema, getCompletionCheckpoint, @@ -1598,6 +1601,169 @@ export const webviewMessageHandler = async ( break } + case "checkpointRollbackFile": { + // B3b: restore one change-card file to its step checkpoint and report + // the outcome back to the requesting card (correlated by cardTs). + const result = checkpointRollbackFilePayloadSchema.safeParse(message.payload) + + if (result.success) { + const task = provider.getCurrentTask() + + if (task) { + // Lazy import: the rollback module pulls the checkpoint service and the + // editor integrations (DiffViewProvider) into the import graph. Loading it + // only when a rollback is requested keeps specs that mock `vscode` minimally + // from executing editor module-scope code at import time. + try { + const { rollbackFile } = await import("../checkpoints/rollback") + const outcome = await rollbackFile(task, result.data.checkpointId, result.data.filePath) + await provider.postMessageToWebview({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: result.data.cardTs, + filePath: outcome.filePath, + success: outcome.success, + ...(outcome.error ? { error: outcome.error } : {}), + }, + }) + } catch (error) { + // Correlated failure: a throw between the request and the result post + // (import, journal read, git restore) would otherwise leave the + // requesting card pending forever. + await provider.postMessageToWebview({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: result.data.cardTs, + filePath: result.data.filePath, + success: false, + error: `Rollback failed: ${error instanceof Error ? error.message : String(error)}`, + }, + }) + } + } else { + // No active task: the rollback cannot run. Post the correlated + // failure so the requesting card can clear its pending state + // instead of waiting on a result that will never arrive. + await provider.postMessageToWebview({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: result.data.cardTs, + filePath: result.data.filePath, + success: false, + error: t("common:errors.message.no_active_task_to_roll_back"), + }, + }) + } + } + + break + } + case "checkpointRollbackStep": { + // B3b: restore every file of a change-card step to the step checkpoint. + const result = checkpointRollbackStepPayloadSchema.safeParse(message.payload) + + if (result.success) { + const task = provider.getCurrentTask() + + if (task) { + // Lazy import (see the checkpointRollbackFile case above). + try { + const { rollbackStep } = await import("../checkpoints/rollback") + const outcome = await rollbackStep(task, result.data.filePaths, result.data.checkpointId) + const firstFailure = outcome.files.find((file) => !file.success) + await provider.postMessageToWebview({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: result.data.cardTs, + success: outcome.files.every((file) => file.success), + ...(firstFailure ? { error: firstFailure.error } : {}), + files: outcome.files, + }, + }) + } catch (error) { + // Correlated failure (see the checkpointRollbackFile case). + await provider.postMessageToWebview({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: result.data.cardTs, + success: false, + error: `Rollback failed: ${error instanceof Error ? error.message : String(error)}`, + }, + }) + } + } else { + // No active task: post the correlated failure so the requesting + // card can clear its pending state. + await provider.postMessageToWebview({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: result.data.cardTs, + success: false, + error: t("common:errors.message.no_active_task_to_roll_back"), + }, + }) + } + } + + break + } + case "checkpointRestoreLatestFile": { + // B3b: restore one change-card file to the latest recorded version of + // that file (the forward direction to a rollback) and report the + // outcome back to the requesting card (correlated by cardTs, kind + // "restore-latest"). + const result = checkpointRestoreLatestFilePayloadSchema.safeParse(message.payload) + + if (result.success) { + const task = provider.getCurrentTask() + + if (task) { + // Lazy import (see the checkpointRollbackFile case above). + try { + const { restoreLatestFile } = await import("../checkpoints/rollback") + const outcome = await restoreLatestFile(task, result.data.filePath) + await provider.postMessageToWebview({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: result.data.cardTs, + kind: "restore-latest", + filePath: outcome.filePath, + success: outcome.success, + ...(outcome.noOp ? { noOp: true } : {}), + ...(outcome.error ? { error: outcome.error } : {}), + }, + }) + } catch (error) { + // Correlated failure (see the checkpointRollbackFile case). + await provider.postMessageToWebview({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: result.data.cardTs, + kind: "restore-latest", + filePath: result.data.filePath, + success: false, + error: `Restore failed: ${error instanceof Error ? error.message : String(error)}`, + }, + }) + } + } else { + // No active task: post the correlated failure so the requesting + // card can clear its pending state. + await provider.postMessageToWebview({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: result.data.cardTs, + kind: "restore-latest", + filePath: result.data.filePath, + success: false, + error: t("common:errors.message.no_active_task_to_restore"), + }, + }) + } + } + + break + } case "completionCheckpointDiff": { const currentCline = provider.getCurrentTask() const checkpoint = currentCline ? resolveCompletionCheckpoint(currentCline) : undefined diff --git a/src/i18n/locales/ca/common.json b/src/i18n/locales/ca/common.json index 24ae3f310c..9500c6e35b 100644 --- a/src/i18n/locales/ca/common.json +++ b/src/i18n/locales/ca/common.json @@ -102,6 +102,8 @@ }, "message": { "no_active_task_to_delete": "No hi ha cap tasca activa de la qual eliminar missatges", + "no_active_task_to_roll_back": "No hi ha cap tasca activa per desfer", + "no_active_task_to_restore": "No hi ha cap tasca activa per restaurar", "invalid_timestamp_for_deletion": "Marca de temps del missatge no vàlida per a l'eliminació", "cannot_delete_missing_timestamp": "No es pot eliminar el missatge: falta la marca de temps", "cannot_delete_invalid_timestamp": "No es pot eliminar el missatge: marca de temps no vàlida", diff --git a/src/i18n/locales/de/common.json b/src/i18n/locales/de/common.json index 54fa0b3c22..b1affc0257 100644 --- a/src/i18n/locales/de/common.json +++ b/src/i18n/locales/de/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "Keine aktive Aufgabe, aus der Nachrichten gelöscht werden können", + "no_active_task_to_roll_back": "Keine aktive Aufgabe, von der aus zurückgerollt werden kann", + "no_active_task_to_restore": "Keine aktive Aufgabe, von der aus wiederhergestellt werden kann", "invalid_timestamp_for_deletion": "Ungültiger Nachrichten-Zeitstempel zum Löschen", "cannot_delete_missing_timestamp": "Nachricht kann nicht gelöscht werden: fehlender Zeitstempel", "cannot_delete_invalid_timestamp": "Nachricht kann nicht gelöscht werden: ungültiger Zeitstempel", diff --git a/src/i18n/locales/en/common.json b/src/i18n/locales/en/common.json index 516a3d4f88..8fd0c2049e 100644 --- a/src/i18n/locales/en/common.json +++ b/src/i18n/locales/en/common.json @@ -100,6 +100,8 @@ }, "message": { "no_active_task_to_delete": "No active task to delete messages from", + "no_active_task_to_roll_back": "No active task to roll back from", + "no_active_task_to_restore": "No active task to restore from", "invalid_timestamp_for_deletion": "Invalid message timestamp for deletion", "cannot_delete_missing_timestamp": "Cannot delete message: missing timestamp", "cannot_delete_invalid_timestamp": "Cannot delete message: invalid timestamp", diff --git a/src/i18n/locales/es/common.json b/src/i18n/locales/es/common.json index 71dc994516..9dcf4cd27b 100644 --- a/src/i18n/locales/es/common.json +++ b/src/i18n/locales/es/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "No hay tarea activa de la cual eliminar mensajes", + "no_active_task_to_roll_back": "No hay ninguna tarea activa desde la que deshacer los cambios", + "no_active_task_to_restore": "No hay ninguna tarea activa desde la que restaurar los cambios", "invalid_timestamp_for_deletion": "Marca de tiempo del mensaje no válida para eliminación", "cannot_delete_missing_timestamp": "No se puede eliminar el mensaje: falta marca de tiempo", "cannot_delete_invalid_timestamp": "No se puede eliminar el mensaje: marca de tiempo no válida", diff --git a/src/i18n/locales/fr/common.json b/src/i18n/locales/fr/common.json index 87009ee988..e6cc7e3cb7 100644 --- a/src/i18n/locales/fr/common.json +++ b/src/i18n/locales/fr/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "Aucune tâche active pour supprimer des messages", + "no_active_task_to_roll_back": "Aucune tâche active pour annuler les modifications", + "no_active_task_to_restore": "Aucune tâche active pour restaurer les modifications", "invalid_timestamp_for_deletion": "Horodatage du message invalide pour la suppression", "cannot_delete_missing_timestamp": "Impossible de supprimer le message : horodatage manquant", "cannot_delete_invalid_timestamp": "Impossible de supprimer le message : horodatage invalide", diff --git a/src/i18n/locales/hi/common.json b/src/i18n/locales/hi/common.json index f4bd1c3055..4e4933134f 100644 --- a/src/i18n/locales/hi/common.json +++ b/src/i18n/locales/hi/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "संदेशों को हटाने के लिए कोई सक्रिय कार्य नहीं", + "no_active_task_to_roll_back": "वापस करने के लिए कोई सक्रिय कार्य नहीं है", + "no_active_task_to_restore": "पुनर्स्थापित करने के लिए कोई सक्रिय कार्य नहीं है", "invalid_timestamp_for_deletion": "हटाने के लिए अमान्य संदेश टाइमस्टैम्प", "cannot_delete_missing_timestamp": "संदेश हटाया नहीं जा सकता: टाइमस्टैम्प गुम है", "cannot_delete_invalid_timestamp": "संदेश हटाया नहीं जा सकता: अमान्य टाइमस्टैम्प", diff --git a/src/i18n/locales/id/common.json b/src/i18n/locales/id/common.json index bcee321af5..47e37f4323 100644 --- a/src/i18n/locales/id/common.json +++ b/src/i18n/locales/id/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "Tidak ada tugas aktif untuk menghapus pesan", + "no_active_task_to_roll_back": "Tidak ada tugas aktif untuk di-rollback", + "no_active_task_to_restore": "Tidak ada tugas aktif untuk dipulihkan", "invalid_timestamp_for_deletion": "Timestamp pesan tidak valid untuk penghapusan", "cannot_delete_missing_timestamp": "Tidak dapat menghapus pesan: timestamp tidak ada", "cannot_delete_invalid_timestamp": "Tidak dapat menghapus pesan: timestamp tidak valid", diff --git a/src/i18n/locales/it/common.json b/src/i18n/locales/it/common.json index 395be16b84..5c3920390b 100644 --- a/src/i18n/locales/it/common.json +++ b/src/i18n/locales/it/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "Nessuna attività attiva da cui eliminare messaggi", + "no_active_task_to_roll_back": "Nessun task attivo da cui annullare le modifiche", + "no_active_task_to_restore": "Nessun task attivo da cui ripristinare le modifiche", "invalid_timestamp_for_deletion": "Timestamp del messaggio non valido per l'eliminazione", "cannot_delete_missing_timestamp": "Impossibile eliminare il messaggio: timestamp mancante", "cannot_delete_invalid_timestamp": "Impossibile eliminare il messaggio: timestamp non valido", diff --git a/src/i18n/locales/ja/common.json b/src/i18n/locales/ja/common.json index 7dccfcd837..2fb3351331 100644 --- a/src/i18n/locales/ja/common.json +++ b/src/i18n/locales/ja/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "メッセージを削除するアクティブなタスクがありません", + "no_active_task_to_roll_back": "ロールバックできるアクティブなタスクがありません", + "no_active_task_to_restore": "復元できるアクティブなタスクがありません", "invalid_timestamp_for_deletion": "削除用のメッセージタイムスタンプが無効です", "cannot_delete_missing_timestamp": "メッセージを削除できません：タイムスタンプがありません", "cannot_delete_invalid_timestamp": "メッセージを削除できません：タイムスタンプが無効です", diff --git a/src/i18n/locales/ko/common.json b/src/i18n/locales/ko/common.json index 0ca65be687..23eb829e21 100644 --- a/src/i18n/locales/ko/common.json +++ b/src/i18n/locales/ko/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "메시지를 삭제할 활성 작업이 없습니다", + "no_active_task_to_roll_back": "되돌릴 활성 작업이 없습니다", + "no_active_task_to_restore": "복원할 활성 작업이 없습니다", "invalid_timestamp_for_deletion": "삭제를 위한 메시지 타임스탬프가 유효하지 않습니다", "cannot_delete_missing_timestamp": "메시지를 삭제할 수 없습니다: 타임스탬프가 없습니다", "cannot_delete_invalid_timestamp": "메시지를 삭제할 수 없습니다: 타임스탬프가 유효하지 않습니다", diff --git a/src/i18n/locales/nl/common.json b/src/i18n/locales/nl/common.json index a38415edfd..21cafafad4 100644 --- a/src/i18n/locales/nl/common.json +++ b/src/i18n/locales/nl/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "Geen actieve taak om berichten uit te verwijderen", + "no_active_task_to_roll_back": "Geen actieve taak om terug te draaien", + "no_active_task_to_restore": "Geen actieve taak om te herstellen", "invalid_timestamp_for_deletion": "Ongeldig bericht tijdstempel voor verwijdering", "cannot_delete_missing_timestamp": "Kan bericht niet verwijderen: tijdstempel ontbreekt", "cannot_delete_invalid_timestamp": "Kan bericht niet verwijderen: ongeldig tijdstempel", diff --git a/src/i18n/locales/pl/common.json b/src/i18n/locales/pl/common.json index ff898e8987..41ef2d7a70 100644 --- a/src/i18n/locales/pl/common.json +++ b/src/i18n/locales/pl/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "Brak aktywnego zadania do usunięcia wiadomości", + "no_active_task_to_roll_back": "Brak aktywnego zadania do cofnięcia", + "no_active_task_to_restore": "Brak aktywnego zadania do przywrócenia", "invalid_timestamp_for_deletion": "Nieprawidłowy znacznik czasu wiadomości do usunięcia", "cannot_delete_missing_timestamp": "Nie można usunąć wiadomości: brak znacznika czasu", "cannot_delete_invalid_timestamp": "Nie można usunąć wiadomości: nieprawidłowy znacznik czasu", diff --git a/src/i18n/locales/pt-BR/common.json b/src/i18n/locales/pt-BR/common.json index d3c31ed2dd..d76488a7e5 100644 --- a/src/i18n/locales/pt-BR/common.json +++ b/src/i18n/locales/pt-BR/common.json @@ -103,6 +103,8 @@ }, "message": { "no_active_task_to_delete": "Nenhuma tarefa ativa para excluir mensagens", + "no_active_task_to_roll_back": "Nenhuma tarefa ativa para desfazer", + "no_active_task_to_restore": "Nenhuma tarefa ativa para restaurar", "invalid_timestamp_for_deletion": "Timestamp da mensagem inválido para exclusão", "cannot_delete_missing_timestamp": "Não é possível excluir mensagem: timestamp ausente", "cannot_delete_invalid_timestamp": "Não é possível excluir mensagem: timestamp inválido", diff --git a/src/i18n/locales/ru/common.json b/src/i18n/locales/ru/common.json index 08d2e2aa2c..57b06a5325 100644 --- a/src/i18n/locales/ru/common.json +++ b/src/i18n/locales/ru/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "Нет активной задачи для удаления сообщений", + "no_active_task_to_roll_back": "Нет активного задания для отката", + "no_active_task_to_restore": "Нет активного задания для восстановления", "invalid_timestamp_for_deletion": "Недействительная временная метка сообщения для удаления", "cannot_delete_missing_timestamp": "Невозможно удалить сообщение: отсутствует временная метка", "cannot_delete_invalid_timestamp": "Невозможно удалить сообщение: недействительная временная метка", diff --git a/src/i18n/locales/tr/common.json b/src/i18n/locales/tr/common.json index 716ccbc6de..3b49e740fd 100644 --- a/src/i18n/locales/tr/common.json +++ b/src/i18n/locales/tr/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "Mesaj silinecek aktif görev yok", + "no_active_task_to_roll_back": "Geri alınacak etkin görev yok", + "no_active_task_to_restore": "Geri yüklenecek etkin görev yok", "invalid_timestamp_for_deletion": "Silme için geçersiz mesaj zaman damgası", "cannot_delete_missing_timestamp": "Mesaj silinemiyor: zaman damgası eksik", "cannot_delete_invalid_timestamp": "Mesaj silinemiyor: geçersiz zaman damgası", diff --git a/src/i18n/locales/vi/common.json b/src/i18n/locales/vi/common.json index 69c6343c31..3f5a5abb58 100644 --- a/src/i18n/locales/vi/common.json +++ b/src/i18n/locales/vi/common.json @@ -99,6 +99,8 @@ }, "message": { "no_active_task_to_delete": "Không có nhiệm vụ hoạt động để xóa tin nhắn", + "no_active_task_to_roll_back": "Không có tác vụ hoạt động để hoàn tác", + "no_active_task_to_restore": "Không có tác vụ hoạt động để khôi phục", "invalid_timestamp_for_deletion": "Dấu thời gian tin nhắn không hợp lệ để xóa", "cannot_delete_missing_timestamp": "Không thể xóa tin nhắn: thiếu dấu thời gian", "cannot_delete_invalid_timestamp": "Không thể xóa tin nhắn: dấu thời gian không hợp lệ", diff --git a/src/i18n/locales/zh-CN/common.json b/src/i18n/locales/zh-CN/common.json index 3600f0aa7c..0cac675500 100644 --- a/src/i18n/locales/zh-CN/common.json +++ b/src/i18n/locales/zh-CN/common.json @@ -104,6 +104,8 @@ }, "message": { "no_active_task_to_delete": "没有可删除消息的活跃任务", + "no_active_task_to_roll_back": "没有可回滚的活动任务", + "no_active_task_to_restore": "没有可恢复的活动任务", "invalid_timestamp_for_deletion": "删除操作的消息时间戳无效", "cannot_delete_missing_timestamp": "无法删除消息：缺少时间戳", "cannot_delete_invalid_timestamp": "无法删除消息：时间戳无效", diff --git a/src/i18n/locales/zh-TW/common.json b/src/i18n/locales/zh-TW/common.json index c635769891..a11e2ac30f 100644 --- a/src/i18n/locales/zh-TW/common.json +++ b/src/i18n/locales/zh-TW/common.json @@ -98,6 +98,8 @@ }, "message": { "no_active_task_to_delete": "沒有可刪除訊息的活躍工作", + "no_active_task_to_roll_back": "沒有可還原的活動任務", + "no_active_task_to_restore": "沒有可復原的活動任務", "invalid_timestamp_for_deletion": "刪除操作的訊息時間戳無效", "cannot_delete_missing_timestamp": "無法刪除訊息：缺少時間戳", "cannot_delete_invalid_timestamp": "無法刪除訊息：時間戳無效", diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index 3e3d3d0653..a4d5a56756 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -407,6 +407,106 @@ export abstract class ShadowCheckpointService extends EventEmitter { } } + /** + * Restore a single file to its state at `commitHash` without moving the + * branch or truncating the checkpoint list (unlike + * {@link restoreCheckpoint}). + * + * If the file did not exist at `commitHash`, it is removed from the + * working tree instead — rolling a file back to before it was created. + */ + public async restoreFile(commitHash: string, filePath: string): Promise { + try { + this.log(`[${this.constructor.name}#restoreFile] restoring ${filePath} from ${commitHash}`) + + if (!this.git) { + throw new Error("Shadow git repo not initialized") + } + + // Git pathspecs are always POSIX: normalize a native (Windows + // backslash) path before the git calls. Without this, `cat-file -e` on + // a backslashed path never matches, the file is treated as absent at + // the checkpoint, and the delete branch below would remove a file the + // checkpoint actually contains. The local fs.rm join keeps the native + // form, since the OS treats both separators interchangeably there. + const gitPath = filePath.toPosix() + + // Constrain the path to the workspace before either branch: `..` + // segments would otherwise normalize (path.join / git pathspec) to a + // location outside `this.workspaceDir`, and the delete branch could + // remove an unrelated file. `path.resolve` normalizes the segments; + // the trailing-separator prefix check is the containment guard. + const resolvedTarget = path.resolve(this.workspaceDir, filePath) + const workspaceRoot = this.workspaceDir.endsWith(path.sep) + ? this.workspaceDir + : this.workspaceDir + path.sep + if (resolvedTarget !== this.workspaceDir && !resolvedTarget.startsWith(workspaceRoot)) { + throw new Error(`restoreFile target is outside the workspace: ${filePath}`) + } + + // The lexical check cannot see through a symlinked ancestor: a link + // inside the workspace pointing outside it passes the prefix check + // while the real target resolves elsewhere. Re-check containment on + // the resolved (real) paths whenever the target file currently + // exists — that is the destructive case. A target that does not exist + // cannot be deleted, and the checkout branch only writes files the + // task-owned shadow repo recorded. Resolving both sides keeps a + // legitimate symlinked workspace root working. + if (await fileExistsAtPath(resolvedTarget)) { + const realWorkspaceRoot = await fs.realpath(this.workspaceDir) + const realRoot = realWorkspaceRoot.endsWith(path.sep) ? realWorkspaceRoot : realWorkspaceRoot + path.sep + const realTarget = await fs.realpath(resolvedTarget) + if (realTarget !== realWorkspaceRoot && !realTarget.startsWith(realRoot)) { + throw new Error(`restoreFile target resolves outside the workspace: ${filePath}`) + } + } + + const start = Date.now() + const existed = await this.fileExistsInCommit(commitHash, gitPath) + + if (existed) { + await this.git.checkout([commitHash, "--", gitPath]) + } else { + await fs.rm(path.join(this.workspaceDir, filePath), { force: true }) + } + + const duration = Date.now() - start + this.emit("restore", { type: "restore", commitHash, duration }) + this.log(`[${this.constructor.name}#restoreFile] restored ${filePath} in ${duration}ms`) + } catch (e) { + const error = e instanceof Error ? e : new Error(String(e)) + this.log(`[${this.constructor.name}#restoreFile] failed to restore file: ${error.message}`) + this.emit("error", { type: "error", error }) + throw error + } + } + + /** Whether `filePath` exists in the tree of `commitHash`. */ + private async fileExistsInCommit(commitHash: string, filePath: string): Promise { + // A failed lookup is not evidence the file is absent. If the commit object + // itself cannot be read (invalid hash, corrupt or missing shadow repo), + // falling through to the restoreFile delete branch would remove a live + // file. Verify the object first and fail the restore loudly instead. + // + // Verification must be evidence-based: simple-git's raw() only rejects + // when git writes a fatal to stderr, and `git cat-file -e ` + // fails *silently* (exit 1, no output) — a silent resolution would be + // read as "valid commit". `rev-parse --verify` emits the resolved id on + // success and a stderr fatal on every failure mode, so the reject is + // reliable. + try { + await this.git!.raw(["rev-parse", "--verify", `${commitHash}^{commit}`]) + } catch { + throw new Error(`Checkpoint unavailable: ${commitHash}`) + } + try { + await this.git!.raw(["cat-file", "-e", `${commitHash}:${filePath}`]) + return true + } catch { + return false + } + } + public async getDiff({ from, to }: { from?: string; to?: string }): Promise { if (!this.git) { throw new Error("Shadow git repo not initialized") diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index 1710cc97e8..32be9b67e9 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -165,6 +165,170 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( }) }) + describe(`${klass.name}#restoreFile`, () => { + it("restores a modified file to a previous checkpoint without touching other files", async () => { + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + const newFile = path.join(service.workspaceDir, "new.txt") + await fs.writeFile(newFile, "New file content") + const commit2 = await service.saveCheckpoint("Second checkpoint") + expect(commit2?.commit).toBeTruthy() + + // Drift both files, then roll back only test.txt to commit 1. + await fs.writeFile(testFile, "Changed after checkpoint") + await fs.writeFile(newFile, "Also changed") + + await service.restoreFile(commit1!.commit, "test.txt") + + expect(await fs.readFile(testFile, "utf-8")).toBe("Ahoy, world!") + // new.txt is not part of the restore and keeps its drifted content. + expect(await fs.readFile(newFile, "utf-8")).toBe("Also changed") + }) + + it("deletes a file that did not exist at the checkpoint", async () => { + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + const newFile = path.join(service.workspaceDir, "new.txt") + await fs.writeFile(newFile, "Created after the checkpoint") + + await service.restoreFile(commit1!.commit, "new.txt") + + expect(await fileExistsAtPath(newFile)).toBe(false) + // The untouched file keeps its checkpoint-1 content. + expect(await fs.readFile(testFile, "utf-8")).toBe("Ahoy, world!") + }) + + it("accepts a native backslashed path on Windows (git pathspecs are POSIX)", async () => { + if (process.platform !== "win32") { + // Off-Windows the journal paths are already POSIX, so there is + // nothing to normalize; the toPosix() call is still exercised + // as a no-op by every other restoreFile test. + return + } + + // A nested file, so the relative path actually contains a + // separator that Windows writes as a backslash. + const subFile = path.join(path.dirname(testFile), "subdir", "inner.txt") + await fs.mkdir(path.dirname(subFile), { recursive: true }) + await fs.writeFile(subFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + await fs.writeFile(subFile, "Drifted") + + // The change journal and the webview hand backslashed paths to the + // rollback service on Windows; restoreFile must normalize them for + // the git calls. Without the normalization, `cat-file -e` would + // never match and the delete branch would remove a file the + // checkpoint actually contains. + await service.restoreFile(commit1!.commit, "subdir" + path.sep + "inner.txt") + + expect(await fs.readFile(subFile, "utf-8")).toBe("Ahoy, world!") + }) + + it("rejects a restore target that escapes the workspace", async () => { + // `..` segments must be normalized and contained, so neither the + // checkout nor the delete branch can touch a file outside the + // workspace (CWE-22). + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + await expect(service.restoreFile(commit1!.commit, path.join("..", "escape.txt"))).rejects.toThrow( + /outside the workspace/, + ) + }) + + it("rejects an unavailable checkpoint instead of deleting the live file", async () => { + // A corrupt journal entry can hand the rollback service a checkpoint + // id that does not resolve to a shadow-repo object. The lookup + // failure must not be read as "file absent at the checkpoint" — + // that would route the restore into the delete branch and remove a + // live file. The restore must fail loudly and leave the file intact. + await fs.writeFile(testFile, "Ahoy, world!") + await service.saveCheckpoint("First checkpoint") + + await expect( + service.restoreFile("0000000000000000000000000000000000000000", "test.txt"), + ).rejects.toThrow("Checkpoint unavailable") + + expect(await fileExistsAtPath(testFile)).toBe(true) + expect(await fs.readFile(testFile, "utf-8")).toBe("Ahoy, world!") + }) + + it("rejects a target that escapes the workspace through a symlinked ancestor", async () => { + // A lexical prefix check passes for a path that goes through a + // symlink inside the workspace pointing outside it; the resolved + // (real) target must be contained as well, or the delete/checkout + // branch would mutate a file the task never owned (CWE-22). + if (process.platform === "win32") { + // Creating symlinks needs elevated privileges on Windows; the + // lexical-containment test above still covers the portable case. + return + } + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + const outsideDir = path.join(tmpDir, `outside-${Date.now()}`) + await fs.mkdir(outsideDir, { recursive: true }) + const outsideFile = path.join(outsideDir, "sneaky.txt") + await fs.writeFile(outsideFile, "outside") + + // A directory link inside the workspace pointing at the outside dir: + // lexically `link/sneaky.txt` is inside the workspace. + await fs.symlink(outsideDir, path.join(service.workspaceDir, "link"), "dir") + + // "resolves outside the workspace" is the real-path (symlink) + // guard's message; the bare "outside the workspace" would also + // match the lexical guard's error. + await expect(service.restoreFile(commit1!.commit, path.join("link", "sneaky.txt"))).rejects.toThrow( + /resolves outside the workspace/, + ) + + // The outside file survives: the restore failed before any mutation. + expect(await fileExistsAtPath(outsideFile)).toBe(true) + expect(await fs.readFile(outsideFile, "utf-8")).toBe("outside") + }) + + it("emits a restore event when a file is restored", async () => { + const restoreListener = vi.fn() + service.on("restore", restoreListener) + + await fs.writeFile(testFile, "Ahoy, world!") + const commit1 = await service.saveCheckpoint("First checkpoint") + expect(commit1?.commit).toBeTruthy() + + await fs.writeFile(testFile, "Drifted") + await service.restoreFile(commit1!.commit, "test.txt") + + expect(restoreListener).toHaveBeenCalledWith( + expect.objectContaining({ type: "restore", commitHash: commit1!.commit }), + ) + }) + + it("throws and emits an error event when the shadow repo is not initialized", async () => { + // A service that never ran initShadowGit has no git handle; restoreFile + // must fail cleanly and surface the error event. + const raw = await klass.create({ + taskId, + shadowDir: path.join(tmpDir, `noinit-${Date.now()}`), + workspaceDir: path.join(tmpDir, `ws-noinit-${Date.now()}`), + log: () => {}, + }) + + const errorListener = vi.fn() + raw.on("error", errorListener) + + await expect(raw.restoreFile("sha-x", "test.txt")).rejects.toThrow("Shadow git repo not initialized") + expect(errorListener).toHaveBeenCalledWith(expect.objectContaining({ type: "error" })) + }) + }) + describe(`${klass.name}#saveCheckpoint`, () => { it("creates a checkpoint if there are pending changes", async () => { await fs.writeFile(testFile, "Ahoy, world!") diff --git a/webview-ui/src/components/chat/ChangeCard.tsx b/webview-ui/src/components/chat/ChangeCard.tsx new file mode 100644 index 0000000000..14986621bc --- /dev/null +++ b/webview-ui/src/components/chat/ChangeCard.tsx @@ -0,0 +1,445 @@ +import { useEffect, useMemo, useState } from "react" +import { useTranslation } from "react-i18next" +import { VSCodeProgressRing } from "@vscode/webview-ui-toolkit/react" +import { Check, FileDiff, History, RotateCcw, X } from "lucide-react" +import { safeJsonParse } from "@roo/core" + +import { changeCardSchema, type ClineMessage, type ExtensionMessage } from "@roo-code/types" + +import { Button, StandardTooltip } from "@/components/ui" +import { vscode } from "@src/utils/vscode" +import { formatPathTooltip } from "@src/utils/formatPathTooltip" + +import CodeAccordion from "../common/CodeAccordion" + +type RollbackStatus = "idle" | "confirming" | "pending" | "success" | "error" + +type RollbackState = { + status: RollbackStatus + error?: string +} + +const IDLE: RollbackState = { status: "idle" } + +const successState: RollbackState = { status: "success" } + +/** + * Per-step change card (B3a payload, B3b UI): header with the file count, a + * per-file list with +/− diff badges, and per-file / per-step restore + * controls wired to the extension host through the + * `checkpointRollbackFile` / `checkpointRollbackStep` / + * `checkpointRestoreLatestFile` messages. A rollback restores a file to the + * state it had BEFORE the step (undoing the step's write); a restore-latest + * brings the file back to its most recent recorded version (the forward + * direction, available from every card of the task). Diffs come from the + * payload's per-file `diff` field: `full` cards expand by default, + * `summary` cards expand lazily on toggle, compact cards carry no diff. + */ +export const ChangeCard = ({ message }: { message: ClineMessage }) => { + const { t } = useTranslation() + + const card = useMemo(() => { + // Validate the shape, not just the JSON-ness: this text is persisted + // task history, so a truncated or pre-series record must not throw + // during render. Records that fail to parse (safeJsonParse returns + // `undefined` without a default) or fail shape validation fall through + // to the null path (an inert card row). + const parsed = safeJsonParse(message.text) + if (parsed === undefined) { + return null + } + const validated = changeCardSchema.safeParse(parsed) + return validated.success ? validated.data : null + }, [message.text]) + + // Files whose diff is currently expanded. "full" cards expand inline by + // default; "summary" cards keep the diff collapsed until toggled. + const [expandedFiles, setExpandedFiles] = useState>(() => { + if (!card || card.detail !== "full") { + return new Set() + } + return new Set(card.files.filter((file) => file.diff != null).map((file) => file.path)) + }) + + const [fileRollbacks, setFileRollbacks] = useState>({}) + const [fileRestores, setFileRestores] = useState>({}) + const [stepRollback, setStepRollback] = useState(IDLE) + + const checkpointId = card?.checkpointIds[0] + + // Correlate extension restore results with this card by message ts. + useEffect(() => { + const handler = (event: MessageEvent) => { + const data = event.data as ExtensionMessage | undefined + const result = data?.checkpointRollbackResult + if (data?.type !== "checkpointRollbackResult" || !result) { + return + } + if (result.cardTs !== message.ts) { + return + } + const filePath = result.filePath + if (filePath !== undefined) { + // Per-file results route to the control that requested them: + // restore-latest results update the restore control; everything + // else (including results posted before `kind` existed) the + // rollback control. + const state: RollbackState = result.success ? successState : { status: "error", error: result.error } + if (result.kind === "restore-latest") { + setFileRestores((prev) => ({ ...prev, [filePath]: state })) + } else { + setFileRollbacks((prev) => ({ ...prev, [filePath]: state })) + } + } + if (result.files) { + const fileUpdates: Record = {} + for (const file of result.files) { + fileUpdates[file.filePath] = file.success ? successState : { status: "error", error: file.error } + } + setFileRollbacks((prev) => ({ ...prev, ...fileUpdates })) + const firstFailure = result.files.find((file) => !file.success) + setStepRollback( + result.success ? successState : { status: "error", error: firstFailure?.error ?? result.error }, + ) + } else if (filePath === undefined) { + // Step-level result with no per-file payload (for example the + // missing-task response: success: false, no files). Without this the + // step button would stay in the in-progress state forever. + setStepRollback(result.success ? successState : { status: "error", error: result.error }) + } + } + window.addEventListener("message", handler) + return () => window.removeEventListener("message", handler) + }, [message.ts]) + + if (!card || !checkpointId) { + return null + } + + const toggleFile = (path: string) => { + setExpandedFiles((prev) => { + const next = new Set(prev) + if (next.has(path)) { + next.delete(path) + } else { + next.add(path) + } + return next + }) + } + + const requestFileRollback = (path: string) => { + vscode.postMessage({ + type: "checkpointRollbackFile", + payload: { cardTs: message.ts, checkpointId, filePath: path }, + }) + setFileRollbacks((prev) => ({ ...prev, [path]: { status: "pending" } })) + } + + const requestFileRestoreLatest = (path: string) => { + vscode.postMessage({ + type: "checkpointRestoreLatestFile", + payload: { cardTs: message.ts, filePath: path }, + }) + setFileRestores((prev) => ({ ...prev, [path]: { status: "pending" } })) + } + + const requestStepRollback = () => { + vscode.postMessage({ + type: "checkpointRollbackStep", + payload: { cardTs: message.ts, checkpointId, filePaths: card.files.map((file) => file.path) }, + }) + setStepRollback({ status: "pending" }) + } + + // Open the changed file in the editor. The extension host resolves relative + // paths against the current cwd (webviewMessageHandler "openFile"), so + // normalize the "./" prefix the same way FileChangesPanel does. + const openFileInEditor = (path: string) => { + vscode.postMessage({ type: "openFile", text: path.startsWith("./") ? path : "./" + path }) + } + + const diffBadges = (additions: number, deletions: number) => + additions > 0 || deletions > 0 ? ( + + +{additions} + -{deletions} + + ) : null + + const fileRollbackControls = (path: string, index: number) => { + const state = fileRollbacks[path] ?? IDLE + const confirmTestId = `change-card-file-confirm-${index}` + const cancelTestId = `change-card-file-cancel-${index}` + + switch (state.status) { + case "confirming": + return ( + + + {t("chat:changeCard.rollbackFileWarning")} + + + + + ) + case "pending": + return ( + + + + ) + case "success": + return ( + + + {t("chat:changeCard.rolledBack")} + + ) + case "error": + return ( + + {/* Focusable status so the error detail is reachable by keyboard and + screen-reader users, not only via the hover tooltip. */} + + + {t("chat:changeCard.rollbackFailed")} + + + ) + default: + return ( + + + + ) + } + } + + // Per-file forward direction: restore the file to its most recent recorded + // version. Independent of the rollback control so a user can undo a + // rollback (or a manual edit) from any card of the task. + const fileRestoreLatestControls = (path: string, index: number) => { + const state = fileRestores[path] ?? IDLE + const confirmTestId = `change-card-file-restore-confirm-${index}` + const cancelTestId = `change-card-file-restore-cancel-${index}` + + switch (state.status) { + case "confirming": + return ( + + + {t("chat:changeCard.restoreLatestWarning")} + + + + + ) + case "pending": + return ( + + + + ) + case "success": + return ( + + + {t("chat:changeCard.restored")} + + ) + case "error": + return ( + + {/* Focusable status (see the file-rollback error span above). */} + + + {t("chat:changeCard.restoreFailed")} + + + ) + default: + return ( + + + + ) + } + } + + const stepRollbackControls = () => { + switch (stepRollback.status) { + case "confirming": + return ( + + + {t("chat:changeCard.rollbackWarning")} + + + + + ) + case "pending": + return ( + + + {t("chat:changeCard.rollingBack")} + + ) + case "success": + return ( + + + {t("chat:changeCard.stepRolledBack")} + + ) + case "error": + return ( + + {/* Focusable status (see the file-rollback error span above). */} + + + {t("chat:changeCard.rollbackFailed")} + + + ) + default: + return ( + + ) + } + } + + return ( +
+
+ + + {t("chat:changeCard.header", { count: card.totalFiles })} + + + {stepRollbackControls()} +
+
+ {card.files.map((file, index) => ( +
+
+ {file.diff != null ? ( + toggleFile(file.path)} + onJumpToFile={() => openFileInEditor(file.path)} + diffStats={{ added: file.additions, removed: file.deletions }} + /> + ) : ( +
+ + {formatPathTooltip(file.path)} + + + {diffBadges(file.additions, file.deletions)} + {/* Native button (not a span) so keyboard users can open the + file from a compact row: Enter/Space activate it for free. */} + +
+ )} +
+
+ {fileRollbackControls(file.path, index)} + {fileRestoreLatestControls(file.path, index)} +
+
+ ))} +
+
+ ) +} diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index 952322084f..6d13bae0a0 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -39,6 +39,7 @@ import McpResourceRow from "../mcp/McpResourceRow" import { Mention } from "./Mention" import { CheckpointSaved } from "./checkpoints/CheckpointSaved" +import { ChangeCard } from "./ChangeCard" import { FollowUpSuggest } from "./FollowUpSuggest" import { BatchFilePermission } from "./BatchFilePermission" import { BatchDiffApproval } from "./BatchDiffApproval" @@ -1374,6 +1375,8 @@ export const ChatRowContent = ({ onJumpToPreviousCheckpoint={onJumpToPreviousCheckpoint} /> ) + case "change_card": + return case "condense_context": // In-progress state if (message.partial) { diff --git a/webview-ui/src/components/chat/__tests__/ChangeCard.spec.tsx b/webview-ui/src/components/chat/__tests__/ChangeCard.spec.tsx new file mode 100644 index 0000000000..92bd18fc21 --- /dev/null +++ b/webview-ui/src/components/chat/__tests__/ChangeCard.spec.tsx @@ -0,0 +1,563 @@ +// npx vitest run src/components/chat/__tests__/ChangeCard.spec.tsx + +import React from "react" +import { fireEvent, renderWithExtensionState, screen } from "@/utils/test-utils" +import type { ChangeCardData, ClineMessage } from "@roo-code/types" + +const mockPostMessage = vi.fn() + +vi.mock("@src/utils/vscode", () => ({ + vscode: { + postMessage: (...args: unknown[]) => mockPostMessage(...args), + }, +})) + +// Mock i18n (same pattern as the other ChatRow specs) +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ + t: (key: string, options?: { count?: number; path?: string }) => { + const map: Record = { + "chat:changeCard.header": `${options?.count ?? 0} file(s) changed this step`, + "chat:changeCard.rollbackFile": "Rollback this file", + "chat:changeCard.rollbackFileWarning": "Restores this file to the content it had before this step.", + "chat:changeCard.restoreLatest": "Restore latest version", + "chat:changeCard.restoreLatestWarning": "Restores this file to the latest recorded version.", + "chat:changeCard.restored": "Restored latest version", + "chat:changeCard.restoreFailed": "Restore failed", + "chat:changeCard.rollbackStep": "Rollback step", + "chat:changeCard.rollbackWarning": "Restores the previous content of this step's files.", + "chat:changeCard.confirm": "Confirm", + "chat:changeCard.cancel": "Cancel", + "chat:changeCard.rollingBack": "Rolling back...", + "chat:changeCard.rolledBack": "Rolled back", + "chat:changeCard.stepRolledBack": "Step rolled back", + "chat:changeCard.rollbackFailed": "Rollback failed", + "chat:changeCard.openFile": "Open file: {{path}}", + } + // {{path}} interpolation: the compact row labels embed the file path + // (aria-label / title), mirroring the CodeAccordion label format. + const value = map[key] || key + return value.replace(/{{path}}/g, String(options?.path ?? "")) + }, + }), + initReactI18next: { type: "3rdParty", init: () => {} }, +})) + +// Mock DiffView so the diff text is directly assertable (the real one runs a +// syntax highlighter, which is irrelevant to the lazy-expansion behavior). +vi.mock("@src/components/common/DiffView", () => ({ + default: ({ source }: { source: string }) =>
{source}
, +})) + +import { ChangeCard } from "../ChangeCard" +import { ChatRowContent } from "../ChatRow" + +function makeCardMessage(overrides: Partial = {}, ts = 1000): ClineMessage { + const card: ChangeCardData = { + checkpointIds: ["abc123"], + files: [ + { path: "src/a.ts", additions: 12, deletions: 3 }, + { path: "src/b.ts", additions: 1, deletions: 1 }, + ], + totalFiles: 2, + detail: "summary", + ...overrides, + } + return { + type: "say", + say: "change_card", + ts, + partial: false, + text: JSON.stringify(card), + } +} + +const DIFF_A = "@@ -1,1 +1,2 @@\n-old\n+new-a\n+extra\n" +const DIFF_B = "@@ -1,1 +1,1 @@\n-old\n+new-b\n" + +function fireRollbackResult(data: Record) { + fireEvent(window, new MessageEvent("message", { data })) +} + +describe("ChangeCard", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("renders the header count and per-file list from a multi-file payload", () => { + renderWithExtensionState() + + expect(screen.getByTestId("change-card-header")).toHaveTextContent("2 file(s) changed this step") + expect(screen.getByText((text) => text.includes("src/a.ts"))).toBeInTheDocument() + expect(screen.getByText((text) => text.includes("src/b.ts"))).toBeInTheDocument() + expect(screen.getByText("+12")).toBeInTheDocument() + expect(screen.getByText("-3")).toBeInTheDocument() + expect(screen.getByText("+1")).toBeInTheDocument() + expect(screen.getByText("-1")).toBeInTheDocument() + }) + + it("keeps the diff hidden in summary cards until the file row is expanded", () => { + renderWithExtensionState( + , + ) + + // Collapsed by default: the diff text is not rendered. + expect(screen.queryByTestId("diff-view")).toBeNull() + expect(screen.queryByText((content) => content.includes("+new-a"))).toBeNull() + + // Expand the file row. + fireEvent.click(screen.getByText((text) => text.includes("src/a.ts"))) + + // The diff text comes from the payload and is rendered lazily on expand. + expect(screen.getByTestId("diff-view").textContent).toContain(DIFF_A.trim()) + + // Collapse again. + fireEvent.click(screen.getByText((text) => text.includes("src/a.ts"))) + expect(screen.queryByTestId("diff-view")).toBeNull() + }) + + it("renders the diff inline by default in full cards", () => { + renderWithExtensionState( + , + ) + + const [diffA, diffB] = screen.getAllByTestId("diff-view") + expect(diffA.textContent).toContain(DIFF_A.trim()) + expect(diffB.textContent).toContain(DIFF_B.trim()) + }) + + it("renders compact rows without a diff section when the payload carries no diffs", () => { + // Auto-approved steps are always emitted as summary cards without any + // per-file diff field; the card then renders file rows with stats only. + renderWithExtensionState() + + expect(screen.getByTestId("change-card-header")).toBeInTheDocument() + expect(screen.queryByTestId("diff-view")).toBeNull() + expect(screen.getByText((text) => text.includes("src/a.ts"))).toBeInTheDocument() + expect(screen.getByText((text) => text.includes("src/b.ts"))).toBeInTheDocument() + }) + + it("renders nothing for an unparseable card payload", () => { + const { container } = renderWithExtensionState( + , + ) + + expect(container.innerHTML).toBe("") + }) + + it("renders nothing for a schema-invalid card payload", () => { + // Syntactically valid JSON that fails changeCardSchema: the + // safeParse rejection path (not only the parse-failure path) must + // also yield an inert card row. + const { container } = renderWithExtensionState( + , + ) + + expect(container.innerHTML).toBe("") + }) + + it("rolls back one file through the checkpointRollbackFile message and shows pending + success", async () => { + renderWithExtensionState() + + // Open the confirm step for the first file. + fireEvent.click(screen.getByTestId("change-card-file-rollback-0")) + expect(screen.getByTestId("change-card-file-confirm-0")).toBeInTheDocument() + + // Confirm sends the webview->extension message and goes pending. + fireEvent.click(screen.getByText("Confirm")) + expect(mockPostMessage).toHaveBeenCalledWith({ + type: "checkpointRollbackFile", + payload: { cardTs: 1000, checkpointId: "abc123", filePath: "src/a.ts" }, + }) + expect(screen.getByTestId("change-card-file-pending-0")).toBeInTheDocument() + + // The extension ack resolves the pending state. + fireRollbackResult({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { cardTs: 1000, filePath: "src/a.ts", success: true }, + }) + await screen.findByTestId("change-card-file-success-0") + expect(screen.getByTestId("change-card-file-success-0")).toHaveTextContent("Rolled back") + }) + + it("shows the file rollback error state on a failed ack", async () => { + renderWithExtensionState() + + fireEvent.click(screen.getByTestId("change-card-file-rollback-0")) + fireEvent.click(screen.getByText("Confirm")) + + fireRollbackResult({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + filePath: "src/a.ts", + success: false, + error: "checkpoint not found", + }, + }) + + expect(await screen.findByTestId("change-card-file-error-0")).toHaveTextContent("Rollback failed") + }) + + it("rolls back the whole step through the checkpointRollbackStep message and shows pending + success", async () => { + renderWithExtensionState() + + // Open the confirm step for the step-level rollback. + fireEvent.click(screen.getByTestId("change-card-step-rollback")) + expect(screen.getByTestId("change-card-step-confirm")).toBeInTheDocument() + expect(screen.getByText("Restores the previous content of this step's files.")).toBeInTheDocument() + + // Confirm sends the step message with the step's file list. + fireEvent.click(screen.getByText("Confirm")) + expect(mockPostMessage).toHaveBeenCalledWith({ + type: "checkpointRollbackStep", + payload: { cardTs: 1000, checkpointId: "abc123", filePaths: ["src/a.ts", "src/b.ts"] }, + }) + expect(screen.getByTestId("change-card-step-pending")).toBeInTheDocument() + + // The extension ack (per-step result carries the per-file outcomes). + fireRollbackResult({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + success: true, + files: [ + { filePath: "src/a.ts", success: true }, + { filePath: "src/b.ts", success: true }, + ], + }, + }) + expect(await screen.findByTestId("change-card-step-success")).toHaveTextContent("Step rolled back") + // Per-file rows resolve to success as well. + expect(screen.getByTestId("change-card-file-success-0")).toBeInTheDocument() + expect(screen.getByTestId("change-card-file-success-1")).toBeInTheDocument() + }) + + it("shows the step rollback error state with the first failing file's error", async () => { + renderWithExtensionState() + + fireEvent.click(screen.getByTestId("change-card-step-rollback")) + fireEvent.click(screen.getByText("Confirm")) + + fireRollbackResult({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + success: false, + files: [ + { filePath: "src/a.ts", success: true }, + { filePath: "src/b.ts", success: false, error: "boom" }, + ], + }, + }) + + expect(await screen.findByTestId("change-card-step-error")).toHaveTextContent("Rollback failed") + expect(screen.getByTestId("change-card-file-error-1")).toBeInTheDocument() + expect(screen.getByTestId("change-card-file-success-0")).toBeInTheDocument() + + // The error detail is exposed to assistive technology: each failed + // control is a focusable status element (not just hover tooltip text). + const fileError = screen.getByTestId("change-card-file-error-1") + expect(fileError).toHaveAttribute("role", "status") + expect(fileError).toHaveAttribute("tabindex", "0") + expect(fileError).toHaveAttribute("aria-label", "boom") + expect(screen.getByTestId("change-card-step-error")).toHaveAttribute("role", "status") + expect(screen.getByTestId("change-card-step-error")).toHaveAttribute("tabindex", "0") + }) + + it("resolves the step state from a failure result that carries no files", async () => { + // The missing-task response is a step-level result with success: false + // and no per-file payload (no files, no filePath). Without handling the + // empty shape the step button would stay in the pending state forever. + renderWithExtensionState() + + fireEvent.click(screen.getByTestId("change-card-step-rollback")) + fireEvent.click(screen.getByText("Confirm")) + expect(screen.getByTestId("change-card-step-pending")).toBeInTheDocument() + + fireRollbackResult({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { + cardTs: 1000, + success: false, + error: "Checkpoints are not enabled for this task", + }, + }) + + // The error detail rides in the tooltip content; the visible state is + // the rollback-failed label. The assertion that matters here is that the + // step left the pending state at all (previously it would stay pending). + const stepError = await screen.findByTestId("change-card-step-error") + expect(stepError).toHaveTextContent("Rollback failed") + // Focusable status with the actual error as its accessible name. + expect(stepError).toHaveAttribute("role", "status") + expect(stepError).toHaveAttribute("tabindex", "0") + expect(stepError).toHaveAttribute("aria-label", "Checkpoints are not enabled for this task") + }) + + it("resolves the step state from a success result that carries no files", async () => { + renderWithExtensionState() + + fireEvent.click(screen.getByTestId("change-card-step-rollback")) + fireEvent.click(screen.getByText("Confirm")) + expect(screen.getByTestId("change-card-step-pending")).toBeInTheDocument() + + fireRollbackResult({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { cardTs: 1000, success: true }, + }) + + expect(await screen.findByTestId("change-card-step-success")).toBeInTheDocument() + }) + + it("ignores rollback results for other change cards", () => { + renderWithExtensionState() + + // Unrelated extension messages are dropped by the card's listener. + fireEvent(window, new MessageEvent("message", { data: { type: "state", text: "x" } })) + + fireEvent.click(screen.getByTestId("change-card-step-rollback")) + fireEvent.click(screen.getByText("Confirm")) + expect(screen.getByTestId("change-card-step-pending")).toBeInTheDocument() + + // A result for a different card ts must not resolve this card. + fireRollbackResult({ + type: "checkpointRollbackResult", + checkpointRollbackResult: { cardTs: 999, success: true, files: [] }, + }) + + expect(screen.getByTestId("change-card-step-pending")).toBeInTheDocument() + }) + + it("cancels the file and step rollback confirmations without sending a message", () => { + renderWithExtensionState() + + // File-level cancel returns to idle without a rollback message. + fireEvent.click(screen.getByTestId("change-card-file-rollback-0")) + fireEvent.click(screen.getByTestId("change-card-file-cancel-0")) + expect(screen.getByTestId("change-card-file-rollback-0")).toBeInTheDocument() + expect(mockPostMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "checkpointRollbackFile" })) + + // Step-level cancel returns to idle as well. + fireEvent.click(screen.getByTestId("change-card-step-rollback")) + fireEvent.click(screen.getByTestId("change-card-step-cancel")) + expect(screen.getByTestId("change-card-step-rollback")).toBeInTheDocument() + expect(mockPostMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "checkpointRollbackStep" })) + }) + + it("posts an openFile message from both the diff-row jump icon and the no-diff-row button", () => { + renderWithExtensionState( + , + ) + + // Diff row: the CodeAccordion header jump icon (own aria-label). + fireEvent.click(screen.getByLabelText("Open file: src/a.ts")) + expect(mockPostMessage).toHaveBeenCalledWith({ type: "openFile", text: "./src/a.ts" }) + mockPostMessage.mockClear() + + // No-diff row: the open control on the plain path row. + fireEvent.click(screen.getByTestId("change-card-file-open-1")) + expect(mockPostMessage).toHaveBeenCalledWith({ type: "openFile", text: "./src/b.ts" }) + }) + + it("does not double-prefix paths that already carry the ./ marker", () => { + renderWithExtensionState( + , + ) + + fireEvent.click(screen.getByTestId("change-card-file-open-0")) + expect(mockPostMessage).toHaveBeenCalledWith({ type: "openFile", text: "./src/c.ts" }) + }) + + it("renders the no-diff row open control as a native button so keyboard users can activate it", () => { + renderWithExtensionState( + , + ) + + // A native