diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index f5757a51c7..7d45033f22 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -114,6 +114,13 @@ export const MAX_CHECKPOINT_TIMEOUT_SECONDS = 60 */ export const DEFAULT_CHECKPOINT_TIMEOUT_SECONDS = 15 +/** + * Whether per-write checkpoints and task-start baseline are enabled by default. + * Master switch for the B cluster of checkpoint features. + * @default true + */ +export const DEFAULT_PER_WRITE_CHECKPOINTS = true + /** * GlobalSettings */ @@ -223,6 +230,12 @@ export const globalSettingsSchema = z.object({ .min(MIN_CHECKPOINT_TIMEOUT_SECONDS) .max(MAX_CHECKPOINT_TIMEOUT_SECONDS) .optional(), + /** + * Whether to record a shadow-git checkpoint after every successful write_to_file, + * edit_file, and apply_patch (per-write checkpoints), plus a task-start baseline. + * @default true + */ + perWriteCheckpoints: z.boolean().optional(), ttsEnabled: z.boolean().optional(), ttsSpeed: z.number().optional(), diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 8a006c0639..5dde6a4e8a 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -370,6 +370,7 @@ export type ExtensionState = Pick< enableCheckpoints: boolean checkpointTimeout: number // Timeout for checkpoint initialization in seconds (default: 15) + perWriteCheckpoints: boolean maxOpenTabsContext: number // Maximum number of VSCode open tabs to include in context (0-500) maxWorkspaceFiles: number // Maximum number of files to include in current working directory details (0-500) showRooIgnoredFiles: boolean // Whether to show .rooignore'd files in listings diff --git a/src/core/checkpoints/__tests__/checkpoint.test.ts b/src/core/checkpoints/__tests__/checkpoint.test.ts index 1d56126f0a..570eb8e71e 100644 --- a/src/core/checkpoints/__tests__/checkpoint.test.ts +++ b/src/core/checkpoints/__tests__/checkpoint.test.ts @@ -112,6 +112,179 @@ describe("Checkpoint functionality", () => { }) describe("checkpointSave", () => { + it("serializes concurrent saves for the same task so stageAll + commit never overlap", async () => { + const order: string[] = [] + let resolveFirst: (value: { commit: string }) => void = () => {} + mockCheckpointService.saveCheckpoint + .mockImplementationOnce(() => { + order.push("first:start") + return new Promise((resolve) => { + resolveFirst = resolve + }) + }) + .mockImplementationOnce(async () => { + order.push("second:start") + return { commit: "second-commit" } + }) + + const first = checkpointSave(mockTask, false, true) + const second = checkpointSave(mockTask, false, true) + + // The second save must not reach the service while the first is still running: + // a queued save owns the stageAll + commit pair until it settles. + await new Promise((resolve) => setTimeout(resolve, 10)) + expect(order).toEqual(["first:start"]) + + resolveFirst({ commit: "first-commit" }) + await Promise.all([first, second]) + expect(order).toEqual(["first:start", "second:start"]) + }) + + it("drops a queued save whose checkpoint window expired behind a stalled predecessor", async () => { + // A stalled baseline releases the task after checkpointTimeout, but the queue used to + // start it anyway, however late, committing whatever the workspace had become by then. + // It must not: the save is dropped and the caller resolves with no commit. + mockTask.checkpointTimeout = 0.01 + const order: string[] = [] + let resolveFirst: (value: { commit: string }) => void = () => {} + mockCheckpointService.saveCheckpoint + .mockImplementationOnce(() => { + order.push("stalled:start") + return new Promise((resolve) => { + resolveFirst = resolve + }) + }) + .mockImplementation(async () => { + order.push("late:start") + return { commit: "late-commit" } + }) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + const stalled = checkpointSave(mockTask, true, true) + const queued = await (async () => { + await new Promise((resolve) => setTimeout(resolve, 5)) + const second = checkpointSave(mockTask, false, true) + // Give the expired request its turn behind the still-running baseline, then release + // the baseline so the queue drains. + await new Promise((resolve) => setTimeout(resolve, 30)) + resolveFirst({ commit: "stalled-commit" }) + return second + })() + + const second = await queued + await stalled + // Only the stalled baseline ever reached the shadow git; the expired save never started, + // so no commit describes a workspace that has since moved on. + expect(order).toEqual(["stalled:start"]) + expect(mockCheckpointService.saveCheckpoint).toHaveBeenCalledTimes(1) + expect(second).toBeUndefined() + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("skipping queued per-write checkpoint save")) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("expired behind a slower checkpoint")) + warnSpy.mockRestore() + }) + + it("drops a queued save for a task that was canceled while it waited", async () => { + mockTask.checkpointTimeout = 15 + const order: string[] = [] + let resolveFirst: (value: { commit: string }) => void = () => {} + mockCheckpointService.saveCheckpoint + .mockImplementationOnce(() => { + order.push("stalled:start") + return new Promise((resolve) => { + resolveFirst = resolve + }) + }) + .mockImplementation(async () => { + order.push("canceled:start") + return { commit: "canceled-commit" } + }) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + mockTask.abort = false + + const stalled = checkpointSave(mockTask, true, true) + const queued = checkpointSave(mockTask, false, true) + await new Promise((resolve) => setTimeout(resolve, 5)) + // Cancellation arrives while the save is queued, before its turn. + mockTask.abort = true + resolveFirst({ commit: "stalled-commit" }) + + const second = await queued + await stalled + + expect(order).toEqual(["stalled:start"]) + expect(mockCheckpointService.saveCheckpoint).toHaveBeenCalledTimes(1) + expect(second).toBeUndefined() + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("task canceled")) + warnSpy.mockRestore() + mockTask.abort = false + }) + + it("runs a save requested after a stalled predecessor settles, exactly once each", async () => { + // The gate must not poison the queue: the dropped request stays dropped, while a fresh + // request after the stall settles gets its normal turn. + mockTask.checkpointTimeout = 0.01 + const order: string[] = [] + let resolveFirst: (value: { commit: string }) => void = () => {} + mockCheckpointService.saveCheckpoint + .mockImplementationOnce(() => { + order.push("stalled:start") + return new Promise((resolve) => { + resolveFirst = resolve + }) + }) + .mockImplementation(async () => { + order.push("fresh:start") + return { commit: "fresh-commit" } + }) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + const stalled = checkpointSave(mockTask, true, true) + // Requested while its window is still open, so the drop below is about the wait behind + // the stalled baseline rather than about an already-expired request. + const expired = checkpointSave(mockTask, false, true) + await new Promise((resolve) => setTimeout(resolve, 30)) + resolveFirst({ commit: "stalled-commit" }) + const expiredResult = await expired + const settled = await stalled + const fresh = await checkpointSave(mockTask, false, true) + + expect(order).toEqual(["stalled:start", "fresh:start"]) + expect(mockCheckpointService.saveCheckpoint).toHaveBeenCalledTimes(2) + // The dropped request resolves exactly the way a missing service does: no commit value, + // while the predecessor that held the queue keeps its own result. + expect(expiredResult).toBeUndefined() + expect(settled).toEqual({ commit: "stalled-commit" }) + expect(fresh).toEqual({ commit: "fresh-commit" }) + warnSpy.mockRestore() + }) + + it("never drops a user-initiated restore queued behind a stalled save", async () => { + // Restores carry no expiry: dropping one would silently discard an explicit user action. + mockTask.checkpointTimeout = 0.01 + mockTask.clineMessages = [{ ts: 2, type: "say", say: "checkpoint_saved", text: RESTORE_HASH }] + const order: string[] = [] + let resolveSave: (value: { commit: string }) => void = () => {} + mockCheckpointService.saveCheckpoint.mockImplementationOnce(() => { + order.push("save:start") + return new Promise((resolve) => { + resolveSave = resolve + }) + }) + mockCheckpointService.restoreCheckpoint.mockImplementationOnce(() => { + order.push("restore:start") + return Promise.resolve(undefined) + }) + + const stalled = checkpointSave(mockTask, true, true) + const restore = checkpointRestore(mockTask, { ts: 2, commitHash: RESTORE_HASH, mode: "preview" }) + await new Promise((resolve) => setTimeout(resolve, 30)) + resolveSave({ commit: "stalled-commit" }) + await Promise.all([stalled, restore]) + + expect(order).toEqual(["save:start", "restore:start"]) + expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith(RESTORE_HASH) + }) + it("should wait for checkpoint service initialization before saving", async () => { // Set up task with uninitialized service mockCheckpointService.isInitialized = false @@ -280,6 +453,37 @@ describe("Checkpoint functionality", () => { expect(mockCheckpointService.restoreCheckpoint).not.toHaveBeenCalled() }) + it("serializes a restore against a queued save so no save stages over a restore", async () => { + const order: string[] = [] + let resolveRestore: () => void = () => {} + mockCheckpointService.restoreCheckpoint.mockImplementationOnce( + () => + new Promise((resolve) => { + order.push("restore:start") + resolveRestore = resolve as () => void + }), + ) + mockCheckpointService.saveCheckpoint.mockImplementationOnce(async () => { + order.push("save:start") + return { commit: "queued-commit" } + }) + + const restore = checkpointRestore(mockTask, { ts: 2, commitHash: RESTORE_HASH, mode: "preview" }) + // Let the restore reach the shadow-git reset. + await new Promise((resolve) => setTimeout(resolve, 10)) + + const save = checkpointSave(mockTask, false, true) + // The save must not reach stageAll + commit while the restore is resetting the + // workspace: that commit would record the restored bytes and drop the write's own + // checkpoint. + await new Promise((resolve) => setTimeout(resolve, 10)) + expect(order).toEqual(["restore:start"]) + + resolveRestore() + await Promise.all([restore, save]) + expect(order).toEqual(["restore:start", "save:start"]) + }) + it.each(["delete", "edit"] as const)( "propagates restore failures for %s without rewinding", async (operation) => { diff --git a/src/core/checkpoints/__tests__/getCheckpointService.spec.ts b/src/core/checkpoints/__tests__/getCheckpointService.spec.ts new file mode 100644 index 0000000000..99a8b5b788 --- /dev/null +++ b/src/core/checkpoints/__tests__/getCheckpointService.spec.ts @@ -0,0 +1,126 @@ +import { beforeEach, describe, expect, it, vi } from "vitest" + +import type { Task } from "../../task/Task" +import { RepoPerTaskCheckpointService } from "../../../services/checkpoints" +import { checkGitInstalled } from "../../../utils/git" +import { getCheckpointService } from "../index" + +vi.mock("../../../services/checkpoints", () => ({ + RepoPerTaskCheckpointService: { create: vi.fn() }, +})) + +vi.mock("../../../utils/git", () => ({ + checkGitInstalled: vi.fn(), +})) + +vi.mock("../../../utils/path", () => ({ + getWorkspacePath: vi.fn(() => "/workspace"), +})) + +vi.mock("../../../i18n", () => ({ + t: (key: string) => key, +})) + +/** + * Structural service double: getCheckpointService only registers listeners, + * awaits initShadowGit, and reads isInitialized. The double assertion at the + * create() call sites is confined to this file (AGENTS.md last-resort cast): + * the real class carries a Git handle and event surface the test never + * exercises. + */ +function serviceDouble(initShadowGitImpl: () => Promise) { + const listeners = new Map void>() + let initialized = false + const svc = { + on: vi.fn((event: string, cb: (...args: unknown[]) => void) => { + listeners.set(event, cb) + }), + initShadowGit: vi.fn(() => initShadowGitImpl()), + get isInitialized() { + return initialized + }, + completeInit: () => { + initialized = true + listeners.get("initialize")?.() + }, + } + return svc +} + +function makeTask(): Task { + return { + taskId: "task-init", + cwd: "/workspace", + enableCheckpoints: true, + checkpointService: undefined, + checkpointServiceInitializing: false, + checkpointTimeout: 1, + providerRef: { + deref: vi.fn().mockReturnValue({ + context: { globalStorageUri: { fsPath: "/tmp/b3-init-storage" } }, + postMessageToWebview: vi.fn(), + log: vi.fn(), + }), + }, + say: vi.fn().mockResolvedValue(undefined), + } as unknown as Task +} + +describe("getCheckpointService initialization failures", () => { + beforeEach(() => { + vi.mocked(checkGitInstalled).mockReset() + vi.mocked(RepoPerTaskCheckpointService.create).mockReset() + }) + + it("does not store a service whose shadow-git initialization failed", async () => { + vi.mocked(checkGitInstalled).mockResolvedValue(true) + const failing = serviceDouble(async () => { + throw new Error("shadow git init exploded") + }) + vi.mocked(RepoPerTaskCheckpointService.create).mockReturnValue( + failing as unknown as RepoPerTaskCheckpointService, + ) + + const task = makeTask() + const service = await getCheckpointService(task) + + // The failed service must not be stored: a stored uninitialized handle + // would be returned by the fast path to every later save, and the + // initializing flag must not strand later lookups in a wait for an + // initialization that already died. + expect(service).toBeUndefined() + expect(task.checkpointService).toBeUndefined() + expect(task.checkpointServiceInitializing).toBe(false) + expect(task.enableCheckpoints).toBe(false) + }) + + it("does not store a service when Git is not installed", async () => { + vi.mocked(checkGitInstalled).mockResolvedValue(false) + const svc = serviceDouble(async () => {}) + vi.mocked(RepoPerTaskCheckpointService.create).mockReturnValue(svc as unknown as RepoPerTaskCheckpointService) + + const task = makeTask() + const service = await getCheckpointService(task) + + expect(service).toBeUndefined() + expect(task.checkpointService).toBeUndefined() + expect(task.checkpointServiceInitializing).toBe(false) + expect(task.enableCheckpoints).toBe(false) + }) + + it("stores an initialized service on the success path", async () => { + vi.mocked(checkGitInstalled).mockResolvedValue(true) + const svc = serviceDouble(async () => { + svc.completeInit() + }) + vi.mocked(RepoPerTaskCheckpointService.create).mockReturnValue(svc as unknown as RepoPerTaskCheckpointService) + + const task = makeTask() + const service = await getCheckpointService(task) + + expect(service).toBe(svc) + expect(task.checkpointService).toBe(svc) + expect(task.checkpointServiceInitializing).toBe(false) + expect(task.enableCheckpoints).toBe(true) + }) +}) diff --git a/src/core/checkpoints/index.ts b/src/core/checkpoints/index.ts index 0bf73a726e..2818b0dc4b 100644 --- a/src/core/checkpoints/index.ts +++ b/src/core/checkpoints/index.ts @@ -2,6 +2,7 @@ import pWaitFor from "p-wait-for" import * as vscode from "vscode" import type { ClineApiReqInfo } from "@roo-code/types" +import { DEFAULT_CHECKPOINT_TIMEOUT_SECONDS } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { Task } from "../task/Task" @@ -115,6 +116,15 @@ export async function getCheckpointService(task: Task, { interval = 250 }: { int const service = RepoPerTaskCheckpointService.create(options) task.checkpointServiceInitializing = true await checkGitInstallation(task, service, log, provider) + if (!service.isInitialized) { + // checkGitInstallation disables checkpoints when Git is unavailable and + // logs initShadowGit failures without throwing: storing such a service + // would hand later saves (and the fast path above) an uninitialized + // handle whose isInitialized can never flip, so a failed + // initialization must resolve to no service at all. + task.checkpointServiceInitializing = false + return undefined + } task.checkpointService = service if (task.enableCheckpoints) { sendCheckpointInitWarn(task) @@ -202,6 +212,10 @@ async function checkGitInstallation( } catch (err) { log(`[Task#getCheckpointService] initShadowGit -> ${err.message}`) task.enableCheckpoints = false + // The "initialize" event never fires for a failed initShadowGit, so + // clear the flag here or later lookups wait on an initialization + // that already died. + task.checkpointServiceInitializing = false } } catch (err) { log(`[Task#getCheckpointService] Unexpected error during Git check: ${err.message}`) @@ -211,6 +225,65 @@ async function checkGitInstallation( } } +// Serialize checkpoint work per task. saveCheckpoint() runs stageAll + commit and +// restoreCheckpoint() resets the workspace through the same shadow git, so two concurrent +// operations for the same task (for example the task-start baseline racing the first +// per-write save after the baseline timeout, or a restore while a per-write save is still +// queued) can stage each other's writes or interleave commits. A per-task promise queue +// makes every save and every restore wait for the previous one, including a baseline that +// is still running. +const checkpointWorkQueues = new Map>() + +type QueuedCheckpointWorkOptions = { + /** Human-readable name used in the skip warning, e.g. "queued per-write checkpoint save". */ + label: string + /** Wall-clock deadline after which the operation must not start: a save requested behind a + * stalled predecessor stages whatever the workspace looks like when it finally gets its turn, + * which is not the state the caller asked to record. Omit for work that must run whenever its + * turn arrives, such as a user-initiated restore. */ + expiresAt?: number + /** Checked when the operation's turn arrives, not when it is requested: a task that was canceled + * while its work was queued performs no checkpoint work. */ + isCanceled?: () => boolean +} + +const DEFAULT_QUEUED_WORK_OPTIONS: QueuedCheckpointWorkOptions = { label: "checkpoint work" } + +function enqueueCheckpointWork( + taskId: string, + operation: () => Promise, + options: QueuedCheckpointWorkOptions = DEFAULT_QUEUED_WORK_OPTIONS, +): Promise { + const previous = checkpointWorkQueues.get(taskId) ?? Promise.resolve() + // Both arms of the previous settlement get the same gate, so a failed predecessor does not + // smuggle an expired or canceled operation past it. + // attempt may return the operation's promise or nothing at all; Promise.then flattens the + // former and resolves the latter, which is exactly the two outcomes callers need. + const attempt = (): T | Promise | undefined => { + const expired = options.expiresAt !== undefined && Date.now() > options.expiresAt + if (expired || options.isCanceled?.()) { + const reason = expired ? "expired behind a slower checkpoint" : "task canceled" + console.warn( + `[checkpoints] skipping ${options.label} for task ${taskId}: ${reason}; it never reached the shadow git`, + ) + return undefined + } + return operation() + } + const next = previous.then(attempt, attempt) + const tail = next.then( + () => undefined, + () => undefined, + ) + checkpointWorkQueues.set(taskId, tail) + void tail.finally(() => { + if (checkpointWorkQueues.get(taskId) === tail) { + checkpointWorkQueues.delete(taskId) + } + }) + return next +} + export async function checkpointSave(task: Task, force = false, suppressMessage = false) { const service = await getCheckpointService(task) @@ -220,13 +293,27 @@ export async function checkpointSave(task: Task, force = false, suppressMessage TelemetryService.instance.captureCheckpointCreated(task.taskId) - // Start the checkpoint process in the background. - return service - .saveCheckpoint(`Task: ${task.taskId}, Time: ${Date.now()}`, { allowEmpty: force, suppressMessage }) - .catch((err) => { - console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err) - task.enableCheckpoints = false - }) + // Start the checkpoint process in the background, queued behind any checkpoint work + // already running for this task so stageAll + commit never overlap. The queue is not only a + // lock: a save that is still queued past the caller's own checkpoint window is describing a + // workspace that has moved on, so it is dropped rather than started late, and a save queued by a + // task that has since been canceled performs no checkpoint work at all. A dropped save is + // reported to the caller the same way a missing service is (a resolve with no commit), which is + // how the task-start baseline already treats it - by retrying on a later loop entry. + const timeoutSeconds = task.checkpointTimeout > 0 ? task.checkpointTimeout : DEFAULT_CHECKPOINT_TIMEOUT_SECONDS + return enqueueCheckpointWork( + task.taskId, + () => + service.saveCheckpoint(`Task: ${task.taskId}, Time: ${Date.now()}`, { allowEmpty: force, suppressMessage }), + { + label: force ? "queued task-start checkpoint baseline" : "queued per-write checkpoint save", + expiresAt: Date.now() + Math.max(1, timeoutSeconds * 1000), + isCanceled: () => task.abort, + }, + ).catch((err) => { + console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err) + task.enableCheckpoints = false + }) } export type CheckpointRestoreOptions = { @@ -257,7 +344,11 @@ export async function checkpointRestore( const provider = task.providerRef.deref() try { - await service.restoreCheckpoint(commitHash) + // The same per-task lock the saves use, in both directions: a restore resets the + // workspace through shadow git, so a queued save must not stage and commit while it + // is running (that save would record the restored bytes and drop the write's own + // checkpoint), and a restore requested during a save waits for that save to settle. + await enqueueCheckpointWork(task.taskId, () => service.restoreCheckpoint(commitHash)) TelemetryService.instance.captureCheckpointRestored(task.taskId) await provider?.postMessageToWebview({ type: "currentCheckpointUpdated", text: commitHash }) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index aa6026e56b..9105836002 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -470,6 +470,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 @@ -3901,6 +3904,72 @@ export class Task extends EventEmitter implements TaskLike { // arm needed. void getCheckpointService(this) + // B1 task-start baseline: a suppressed pre-task root commit (default-on). + // History resumes skip it: the baseline belongs to the original task start + // (exactly once per task), and the resumed task's shadow repo already carries + // a baseline for the first post-resume write checkpoint to diff against. + // An already-aborted task skips it too: the loop will never run, so the + // save would only add latency to a task that is being disposed. + if (!this.taskStartBaselineDone && !this._isHistoryTask && !this.abort) { + let baselineEnabled: boolean | undefined + try { + baselineEnabled = (await this.providerRef.deref()?.getState())?.perWriteCheckpoints + } catch (error) { + // A rejected state lookup must not abort task startup: fall back + // to the default-on semantics used when the setting is absent. + console.error("[Task] task-start baseline settings read failed:", error) + } + // Cancellation can arrive while the state lookup is pending; recheck + // before the save so a canceled task performs no checkpoint work. + if (baselineEnabled === false || this.abort) { + this.taskStartBaselineDone = true + } else { + try { + // allowEmpty=true so a clean workspace still produces the baseline + // commit; awaited so the first per-write checkpoint cannot interleave. + // The await is bounded: a stalled git add/commit must not hold task + // startup before TaskStarted. A baseline that finishes late is + // harmless - checkpointSave serializes per task, so a still-running + // baseline cannot interleave with the first per-write save. + const savePromise = this.checkpointSave(true, true) + // A timeout that wins must not leave an unhandled rejection behind. + savePromise.catch(() => undefined) + const BASELINE_TIMED_OUT = Symbol("baseline-timed-out") + let baselineTimer: ReturnType | undefined + const baseline = await Promise.race([ + savePromise, + new Promise((resolve) => { + baselineTimer = setTimeout( + () => resolve(BASELINE_TIMED_OUT), + Math.max(1, this.checkpointTimeout * 1000), + ) + }), + ]).finally(() => { + if (baselineTimer !== undefined) { + clearTimeout(baselineTimer) + } + }) + if (baseline === BASELINE_TIMED_OUT) { + console.warn( + `[Task] task-start checkpoint baseline still running after ${this.checkpointTimeout}s; entering the request loop and leaving the baseline to the per-task checkpoint lock.`, + ) + } else if (baseline?.commit) { + // The flag is set only once the baseline actually committed: a save + // that no-ops behind a failed or timed-out service initialization is + // retried on a later loop entry instead of being marked done without + // a rollback point. + this.taskStartBaselineDone = true + } + } catch (error) { + // The baseline is best-effort and runs before TaskStarted: a + // checkpoint error must not abort task startup (startTask rethrows + // non-abort errors from initiateTaskLoop). Log and continue; the + // next loop entry retries the baseline. + console.error("[Task] task-start checkpoint baseline failed:", error) + } + } + } + let nextUserContent = userContent let includeFileDetails = true @@ -4344,6 +4413,7 @@ export class Task extends EventEmitter implements TaskLike { const idAlreadyUsed = this.assistantMessageContent.some( (entry) => // Stryker disable next-line ConditionalExpression: equivalent mutant, only tool_use and mcp_tool_use entries carry an id, so the type check cannot change entry.id === event.id + (entry.type === "tool_use" || entry.type === "mcp_tool_use") && (entry as { id?: string }).id === event.id, ) @@ -4397,6 +4467,7 @@ export class Task extends EventEmitter implements TaskLike { let streamingKey: string | undefined // Stryker disable next-line ConditionalExpression: equivalent mutant, event.name is always a string here, so the legacy nameless branch is unreachable + if (event.name !== undefined) { streamingKey = NativeToolCallParser.makeStreamingKey(event.id, event.name) } else { @@ -4414,6 +4485,7 @@ export class Task extends EventEmitter implements TaskLike { const partialToolUse = // Stryker disable next-line ConditionalExpression: equivalent mutant, streamingKey is always a string here (makeStreamingKey), so the null side is unreachable + streamingKey === undefined ? null : NativeToolCallParser.processStreamingChunk( @@ -4423,6 +4495,7 @@ export class Task extends EventEmitter implements TaskLike { ) // Stryker disable next-line ConditionalExpression, EqualityOperator: equivalent mutants, streamingKey is always defined here, so both conditions reduce to the partialToolUse check + if (partialToolUse && streamingKey !== undefined) { // Reuse the compound key built above: it is exactly the key the start event // registered under, so the delta lookup needs no second encoding. @@ -4842,6 +4915,7 @@ export class Task extends EventEmitter implements TaskLike { // Create compound key for deduplication (same pattern as streaming handler). // End events carry the tool name; fall back to the id for events that don't. // Stryker disable next-line LogicalOperator: equivalent mutant, finalizeRawChunks emits end events with a string name, so the ?? event.id fallback is unreachable + let eventName = event.name ?? event.id let dedupKey = NativeToolCallParser.makeStreamingKey(event.id, eventName) @@ -4859,12 +4933,12 @@ export class Task extends EventEmitter implements TaskLike { // parser entry and the dedup tracking would stay stale. Resolve the tracked // entry by id and target the real compound key for finalization and cleanup. // Stryker disable next-line ConditionalExpression, LogicalOperator, EqualityOperator: equivalent mutants, the both-miss case is unreachable via the raw-chunk pipeline + if (finalToolUse === null && toolUseIndex === undefined) { const resolved = NativeToolCallParser.getStreamingToolCallById( event.id, nativeToolCallParserScope, ) - // Stryker disable next-line ConditionalExpression: unreachable defensive region — getStreamingToolCallById can only resolve when the both-miss case above is reachable, which the raw-chunk pipeline cannot produce (see the directive there). if (resolved) { eventName = resolved.name dedupKey = NativeToolCallParser.makeStreamingKey(resolved.id, resolved.name) @@ -6526,6 +6600,14 @@ export class Task extends EventEmitter implements TaskLike { * @param context - Context string for logging (e.g., the calling tool name) */ public processQueuedMessages(): void { + // An aborted task must not dequeue queued messages: submitUserMessage + // rejects once abort is set, so consuming the queue here would drop the + // message instead of leaving it for the resume/abort flow. Tools await + // checkpoint saves before handing off, so a cancellation can land between + // the write and this call. + if (this.abort) { + return + } try { if (!this.messageQueueService.isEmpty()) { const queued = this.messageQueueService.dequeueMessage() diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index c56d5291f6..843d52013e 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -7166,6 +7166,326 @@ describe("Cline", () => { }) }) + describe("task-start baseline (B1 perWriteCheckpoints)", () => { + it("records one suppressed baseline checkpoint per Task instance at loop start", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + // A committed baseline: the done flag is set only on a real + // commit, so the double must resolve one for the once-per-instance + // assertion to exercise the flag flip. + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue({ commit: "baseline-commit" }) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + // End the loop after the first request; abort stays false so the + // baseline block (not the loop) is what the save spy observes. + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).toHaveBeenCalledOnce() + expect(saveSpy).toHaveBeenCalledWith(true, true) + }) + + it("does not record a baseline when the task resumes from history", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + historyItem: { + id: "resumed-baseline-task", + number: 1, + ts: Date.now(), + task: "resumed baseline task", + tokensIn: 10, + tokensOut: 5, + totalCost: 0.001, + }, + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + // A history resume must not re-record the baseline: it belongs to the + // original task start (exactly once per task). + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("records the baseline checkpoint when the setting is unset (default-on)", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline unset task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + // Unset: the property is absent from the state, so default-on applies. + const unsetState = { ...state } + Reflect.deleteProperty(unsetState, "perWriteCheckpoints") + vi.spyOn(mockProvider, "getState").mockResolvedValue(unsetState as typeof state) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).toHaveBeenCalledOnce() + expect(saveSpy).toHaveBeenCalledWith(true, true) + }) + + it("does not record a baseline checkpoint when perWriteCheckpoints is disabled", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline disabled task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...state, perWriteCheckpoints: false }) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("does not record a baseline when the task is already aborted", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "aborted baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + task.requestAbort(false) + + await taskAccess.initiateTaskLoop([]) + + // A task cancelled before the loop starts must not pay for (or await) + // a baseline checkpoint it will never use. + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("does not record a baseline when cancellation arrives during the state lookup", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "racy baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + type StateResult = Awaited> + let resolveState: (value: StateResult | PromiseLike) => void = () => {} + const statePromise = new Promise((resolve) => (resolveState = resolve)) + vi.spyOn(mockProvider, "getState").mockReturnValue(statePromise) + + const loopPromise = taskAccess.initiateTaskLoop([]) + // Cancellation lands while the state lookup is still pending. + // requestAbort is the class's public, synchronous way to raise the abort + // latch; main (5319fe91f, #2283) exposes abort as a get-only accessor fed by + // the run-state latches, so assigning task.abort does not typecheck there. + task.requestAbort(false) + resolveState({ ...state, perWriteCheckpoints: true }) + await loopPromise + + // The post-lookup recheck must skip the save for the canceled task. + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("awaits the baseline checkpoint before entering the request loop", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline await task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveSpy = vi + .spyOn(task, "checkpointSave") + .mockImplementation(() => new Promise((resolve) => (resolveSave = resolve))) + const requestSpy = vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...(await mockProvider.getState()) }) + const loopPromise = taskAccess.initiateTaskLoop([]) + + // The loop must not enter while the baseline checkpoint is still in flight. + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(saveSpy).toHaveBeenCalledOnce() + expect(requestSpy).not.toHaveBeenCalled() + + resolveSave() + await loopPromise + expect(requestSpy).toHaveBeenCalled() + }) + + it("enters the request loop when the baseline save stalls past checkpointTimeout", async () => { + // A stalled git add/commit must not hold startup before TaskStarted. The + // await is bounded; the late baseline is left to the per-task checkpoint + // lock, which serializes it with the first per-write save. + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline stall task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + type SaveResult = Awaited> + vi.spyOn(task, "checkpointSave").mockImplementation(() => new Promise(() => {})) + const requestSpy = vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + task.checkpointTimeout = 0.01 + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await taskAccess.initiateTaskLoop([]) + + expect(requestSpy).toHaveBeenCalled() + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("task-start checkpoint baseline still running"), + ) + warnSpy.mockRestore() + }) + + it("retries the baseline on a later loop entry when the stalled save commits after the timeout", async () => { + // The reviewer's question, tested at the layer that owns it: what becomes of the + // baseline once a save the task already gave up on finally settles. A late commit + // must not retire the baseline, because writes have been performed in the meantime, + // so a later loop entry re-takes it. + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "late baseline commit task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + type SaveResult = Awaited> + let resolveStalled: (value: SaveResult) => void = () => {} + const saveSpy = vi + .spyOn(task, "checkpointSave") + .mockImplementationOnce(() => new Promise((resolve) => (resolveStalled = resolve))) + .mockResolvedValueOnce({ commit: "retry-baseline-commit" } satisfies Awaited< + ReturnType + >) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + const requestSpy = vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + task.checkpointTimeout = 0.01 + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await taskAccess.initiateTaskLoop([]) + expect(saveSpy).toHaveBeenCalledOnce() + expect(requestSpy).toHaveBeenCalled() + + // The abandoned save commits long after the task moved on. + resolveStalled({ commit: "late-baseline-commit" } as SaveResult) + await new Promise((resolve) => setTimeout(resolve, 0)) + + await taskAccess.initiateTaskLoop([]) + // The late commit did not mark the baseline done: this loop entry asked for it again. + expect(saveSpy).toHaveBeenCalledTimes(2) + warnSpy.mockRestore() + }) + + it("continues task startup when the baseline checkpoint fails", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline failure task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const boom = new Error("baseline boom") + vi.spyOn(task, "checkpointSave").mockRejectedValue(boom) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + const requestSpy = vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + // The baseline runs before TaskStarted: a checkpoint error must not + // abort task startup (startTask rethrows non-abort errors from + // initiateTaskLoop). The loop must still be entered. + await taskAccess.initiateTaskLoop([]) + + expect(requestSpy).toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith("[Task] task-start checkpoint baseline failed:", boom) + consoleErrorSpy.mockRestore() + }) + + it("retries the baseline on a later loop entry when a save produced no commit", async () => { + // getCheckpointService can resolve to no service (init failure or a + // bounded init wait timing out): the save then completes without a + // commit, and the baseline must not be marked done - a later loop + // entry gets to retry it instead of losing it forever. + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "retry baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi + .spyOn(task, "checkpointSave") + .mockResolvedValueOnce(undefined) + .mockResolvedValue({ commit: "baseline-commit" }) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + expect(saveSpy).toHaveBeenCalledOnce() + + await taskAccess.initiateTaskLoop([]) + expect(saveSpy).toHaveBeenCalledTimes(2) + }) + + it("starts the task default-on when the baseline settings read rejects", async () => { + // ClineProvider.getState awaits custom-mode loading and can reject: + // a failed settings read must not stop task startup. Default-on + // semantics apply (the setting is treated as absent) and the loop + // still runs. + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "rejected-read baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue({ commit: "baseline-commit" }) + vi.spyOn(mockProvider, "getState").mockRejectedValueOnce(new Error("custom mode load failed")) + + const requestSpy = vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).toHaveBeenCalledOnce() + expect(saveSpy).toHaveBeenCalledWith(true, true) + expect(requestSpy).toHaveBeenCalled() + }) + }) + describe("start()", () => { it("should be a no-op if the task was already started in the constructor", () => { const task = new Task({ @@ -8089,6 +8409,32 @@ describe("Queued message processing after condense", () => { expect(submitSpy).toHaveBeenCalledWith("queued text", ["img1.png"]) expect(task.messageQueueService.isEmpty()).toBe(true) }) + it("does not dequeue queued messages once the task is aborted", () => { + // Tools await the checkpoint before handing off queued work; a + // cancellation landing during that await must not consume the queued + // message: submitUserMessage rejects once abort is set, so dequeuing + // would drop the input instead of leaving it for the resume flow. + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } + const provider = createProvider() + const task = new Task({ + provider, + apiConfiguration: apiConfig, + task: "initial task", + startTask: false, + }) + const submitSpy = vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + + task.messageQueueService.addMessage("queued text") + // requestAbort raises the latch without tearing the task down: abortTask() + // would dispose it and drain the queue, which is not the state this row reads. + task.requestAbort(false) + task.processQueuedMessages() + + expect(submitSpy).not.toHaveBeenCalled() + expect(task.messageQueueService.isEmpty()).toBe(false) + }) it("does not cross-drain queues between separate tasks", async () => { const providerA = createProvider() diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index 2713e08ffe..0b0ea7dbf6 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -6,6 +6,7 @@ import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Task } from "../task/Task" +import { checkpointSave } from "../checkpoints" import { formatResponse } from "../prompts/responses" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" @@ -20,6 +21,9 @@ interface ApplyPatchParams { patch: string } +/** Result of one file operation: `mutated` is true when the workspace changed even if the operation eventually failed. */ +type PatchFileResult = { ok: boolean; mutated: boolean } + export class ApplyPatchTool extends BaseTool<"apply_patch"> { readonly name = "apply_patch" as const @@ -56,6 +60,17 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const { patch } = params const { handleError, pushToolResult } = callbacks + // The queued-message handoff is deferred to the finally block below, after + // the whole-patch checkpoint attempt: a handler that hands off early lets a + // queued turn start while the checkpoint is still staging or committing. + + let handoffPending = false + // Committed-mutation tracker: a handler can commit a workspace write and then + // reject during later bookkeeping, so the outer catch must still persist the + // partial-state checkpoint. The flag is therefore owned here, outside the + // handler-return boundary. + const mutation = { committed: false } + try { // Validate required parameters if (!patch) { @@ -102,7 +117,10 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { return } - // Process each file change + // Process each file change. The handlers report whether their file + // operation succeeded, so a rejected approval or a failed local write + // does not get checkpointed as if the patch had succeeded. + let patchSucceeded = true for (const change of changes) { const relPath = change.path const absolutePath = path.resolve(task.cwd, relPath) @@ -112,7 +130,11 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (!accessAllowed) { await task.say("rooignore_error", relPath) pushToolResult(formatResponse.rooIgnoreError(relPath)) - return + // Partial failure: earlier successful writes are retained; break so the + // explicit partial-state checkpoint below persists them before the + // queued-message handoff in the finally block. + patchSucceeded = false + break } // Check if file is write-protected @@ -120,20 +142,95 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (change.type === "add") { // Create new file - await this.handleAddFile(change, absolutePath, relPath, task, callbacks, isWriteProtected) + const result = await this.handleAddFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + mutation, + ) + handoffPending = handoffPending || result.mutated + patchSucceeded = result.ok && patchSucceeded } else if (change.type === "delete") { // Delete file - await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected) + const result = await this.handleDeleteFile( + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + mutation, + ) + handoffPending = handoffPending || result.mutated + patchSucceeded = result.ok && patchSucceeded } else if (change.type === "update") { // Update file - await this.handleUpdateFile(change, absolutePath, relPath, task, callbacks, isWriteProtected) + const result = await this.handleUpdateFile( + change, + absolutePath, + relPath, + task, + callbacks, + isWriteProtected, + mutation, + ) + handoffPending = handoffPending || result.mutated + patchSucceeded = result.ok && patchSucceeded } } task.consecutiveMistakeCount = 0 + + // B1: one checkpoint for the whole patch (not per file), and only when + // every file operation succeeded. Live setting with default-on + // semantics: skip only when explicitly false. + if (patchSucceeded) { + const perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints + if (perWriteCheckpoints !== false) { + // Await so the whole-patch checkpoint persists before the tool + // completes; an unawaited save lets queued work interleave its own + // checkpoint and lets shutdown lose the commit. Called only after + // every filesystem operation succeeded. + await checkpointSave(task, false, true).catch(() => {}) + } + } else if (handoffPending) { + // Partial failure: the earlier successful operations keep their writes + // in the workspace. Persist an explicit partial-state checkpoint before + // the queued-message handoff (the finally block below) so those retained + // writes have rollback metadata instead of being carried forward + // unpersisted; the failed operation already reported itself. + const perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints + if (perWriteCheckpoints !== false) { + await checkpointSave(task, false, true).catch(() => {}) + } + } } catch (error) { + // A later file operation can reject outright (e.g. the approval + // stream dies) after earlier handlers succeeded. This catch skips + // both checkpoint branches above, but the finally block still + // hands off queued messages, so persist an explicit partial-state + // checkpoint here (same default-on semantics as the branches + // above) instead of letting the queued turn start with the + // retained writes carried forward without rollback metadata. + if (handoffPending || mutation.committed) { + const perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints + if (perWriteCheckpoints !== false) { + await checkpointSave(task, false, true).catch(() => {}) + } + } await handleError("apply patch", error as Error) await task.diffViewProvider.reset() + } finally { + // One handoff per successful patch, after the checkpoint attempt and + // independent of the checkpoint setting or outcome: a queued turn can + // never start while the patch checkpoint is still staging or + // committing, and no path (disabled checkpoints, failed save, early + // return, error) strands the queued messages. + if (handoffPending || mutation.committed) { + task.processQueuedMessages() + } } } @@ -144,8 +241,10 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + mutation: { committed: boolean }, + ): Promise { const { askApproval, pushToolResult } = callbacks + let mutated = false // Check if file already exists const fileExists = await fileExistsAtPath(absolutePath) @@ -155,7 +254,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 { ok: false, mutated } } const newContent = change.newContent || "" @@ -209,14 +308,31 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } pushToolResult("Changes were rejected by the user.") await task.diffViewProvider.reset() - return + return { ok: false, mutated } } - // Save the changes - if (isPreventFocusDisruptionEnabled) { - await task.diffViewProvider.saveDirectly(relPath, newContent, true, diagnosticsEnabled, writeDelayMs) - } else { - await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + // The write lands inside the save call below, which can still reject afterwards + // (opening the document, collecting diagnostics) once the new file is on disk. The + // mutation is therefore recorded at the write boundary: this handler never returns in + // that case, and without the flag the retained file would hand off the queued turn + // with no partial-state checkpoint (see the catch and finally in execute()). + mutated = true + mutation.committed = true + try { + if (isPreventFocusDisruptionEnabled) { + await task.diffViewProvider.saveDirectly(relPath, newContent, true, diagnosticsEnabled, writeDelayMs) + } else { + await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) + } + } catch (error) { + // A save that failed before writing any bytes left no file behind, so the add did + // not mutate the workspace after all and must not be checkpointed as a retained + // write. Untrack it and let the outer error path report the failure. + if (!(await fileExistsAtPath(absolutePath))) { + mutated = false + mutation.committed = false + } + throw error } // Track file edit operation @@ -226,7 +342,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, true) pushToolResult(message) await task.diffViewProvider.reset() - task.processQueuedMessages() + return { ok: true, mutated } } private async handleDeleteFile( @@ -235,8 +351,10 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + mutation: { committed: boolean }, + ): Promise { const { askApproval, pushToolResult } = callbacks + let mutated = false // Check if file exists const fileExists = await fileExistsAtPath(absolutePath) @@ -246,7 +364,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 { ok: false, mutated } } const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) @@ -268,7 +386,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { if (!didApprove) { pushToolResult("Delete operation was rejected by the user.") - return + return { ok: false, mutated } } // Delete the file @@ -278,12 +396,14 @@ 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 { ok: false, mutated } } + mutated = true + mutation.committed = true task.didEditFile = true pushToolResult(`Successfully deleted ${relPath}`) - task.processQueuedMessages() + return { ok: true, mutated } } private async handleUpdateFile( @@ -293,8 +413,10 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { task: Task, callbacks: ToolCallbacks, isWriteProtected: boolean, - ): Promise { + mutation: { committed: boolean }, + ): Promise { const { askApproval, pushToolResult } = callbacks + let mutated = false // Check if file exists const fileExists = await fileExistsAtPath(absolutePath) @@ -304,7 +426,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 { ok: false, mutated } } const originalContent = change.originalContent || "" @@ -318,9 +440,11 @@ 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. pushToolResult(`No changes needed for '${relPath}'`) await task.diffViewProvider.reset() - return + return { ok: true, mutated } } // Check experiment settings @@ -366,7 +490,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { } pushToolResult("Changes were rejected by the user.") await task.diffViewProvider.reset() - return + return { ok: false, mutated } } // Handle file move if specified @@ -379,7 +503,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 { ok: false, mutated } } // Check if destination path is write-protected @@ -391,7 +515,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) await task.diffViewProvider.reset() - return + return { ok: false, mutated } } // Check if destination path is outside workspace @@ -403,32 +527,204 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.say("error", errorMessage) pushToolResult(formatResponse.toolError(errorMessage)) await task.diffViewProvider.reset() - return + return { ok: false, mutated } } - // Save new content to the new path - if (isPreventFocusDisruptionEnabled) { - await task.diffViewProvider.saveDirectly( - change.movePath, - newContent, - false, - diagnosticsEnabled, - writeDelayMs, + // A patch whose "Move to" resolves back onto its own source path is a rewrite of + // that path, not a relocation: once the staged content is renamed onto the destination + // there is no second file left, and unlinking the source would delete the content that + // was just committed. Computed from resolved paths because case-insensitive and + // normalized spellings can name the same file differently. + const isMoveOntoSource = path.resolve(moveAbsolutePath) === path.resolve(absolutePath) + + // Capture the destination state before either supported write path: a + // failed move must restore an existing destination instead of deleting + // whatever lived there. A read error other than ENOENT means the + // destination cannot be inspected, so the move is refused before any + // write rather than risking an unrecoverable clobber. + let destinationBackup: Buffer | undefined + try { + destinationBackup = await fs.readFile(moveAbsolutePath) + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + if (code !== "ENOENT") { + task.consecutiveMistakeCount++ + task.recordToolError("apply_patch") + const inspectErrorMessage = `Cannot inspect move destination '${change.movePath}': ${error instanceof Error ? error.message : String(error)}` + await task.say("error", inspectErrorMessage) + pushToolResult(formatResponse.toolError(inspectErrorMessage)) + await task.diffViewProvider.reset() + return { ok: false, mutated } + } + } + + // Capture the destination mode before replacing it: the atomic staging write + // would otherwise install the process-default permissions and widen a 0600 + // or executable destination. + let destinationMode: number | undefined + if (destinationBackup !== undefined) { + destinationMode = await fs.stat(moveAbsolutePath).then( + (stat) => stat.mode, + () => undefined, ) - } else { - // Write to new path and delete old file - const parentDir = path.dirname(moveAbsolutePath) - await fs.mkdir(parentDir, { recursive: true }) - await fs.writeFile(moveAbsolutePath, newContent, "utf8") } - // Delete the original file + // Stage the move so it is all-or-nothing: the pre-move destination bytes + // stay on disk under a backup name, the new content is written to a + // staging file and renamed into place atomically, and only then is the + // source unlinked. Any failure renames the backup back, so the workspace + // never shows a half-applied move. + const moveStamp = `${process.pid}.${Date.now()}` + const moveDir = path.dirname(moveAbsolutePath) + const moveBase = path.basename(moveAbsolutePath) + const stagingPath = path.join(moveDir, `.${moveBase}.${moveStamp}.staging`) + const backupPath = path.join(moveDir, `.${moveBase}.${moveStamp}.backup`) + await fs.mkdir(moveDir, { recursive: true }) + + let destinationCommitted = false + try { + // Owner-only from creation: the staged content must never be readable by + // others, even between the write and the rename. + await fs.writeFile(stagingPath, newContent, { encoding: "utf8", mode: 0o600 }) + if (destinationMode !== undefined) { + await fs.chmod(stagingPath, destinationMode) + } + if (destinationBackup !== undefined) { + // Preserve the pre-move bytes on disk (atomic rename): the in-memory + // copy alone cannot restore the destination after a crash. + await fs.rename(moveAbsolutePath, backupPath) + } + await fs.rename(stagingPath, moveAbsolutePath) + destinationCommitted = true + // The destination content changed: even a later failed source unlink (which + // rolls the destination back) is a workspace mutation the checkpoint must see. + mutated = true + mutation.committed = true + if (isPreventFocusDisruptionEnabled) { + // Diagnostics/bookkeeping only: the bytes were committed above, and + // saveDirectly writes the same content through the same atomic write. + await task.diffViewProvider.saveDirectly( + change.movePath, + newContent, + false, + diagnosticsEnabled, + writeDelayMs, + ) + } + } catch (writeError) { + await fs.rm(stagingPath, { force: true }).catch(() => {}) + if (destinationCommitted) { + await fs.rm(moveAbsolutePath, { force: true }).catch(() => {}) + } + let destinationMissing = false + if (destinationBackup !== undefined) { + // Put the captured destination bytes back with one rename. If that rename + // fails as well, the destination is missing and its content survives only + // under the backup name, so the report has to say where it is. + await fs.rename(backupPath, moveAbsolutePath).catch(() => { + destinationMissing = true + }) + } + if (destinationMissing) { + // A missing destination is a workspace mutation: record it so the + // partial-state checkpoint runs before the queued-message handoff, exactly + // as the failed source deletion below does. + mutated = true + mutation.committed = true + } + task.consecutiveMistakeCount++ + task.recordToolError("apply_patch") + const writeErrorMessage = + `Failed to write move destination '${change.movePath}': ${writeError instanceof Error ? writeError.message : String(writeError)}` + + (destinationMissing && backupPath !== undefined + ? ` The original content is preserved at '${getReadablePath(task.cwd, path.relative(task.cwd, backupPath))}'.` + : "") + await task.say("error", writeErrorMessage) + pushToolResult(formatResponse.toolError(writeErrorMessage)) + await task.diffViewProvider.reset() + return { ok: false, mutated } + } + + // Delete the original file, unless the move renamed its own path: there is no + // second copy to remove in that case, and this is the step that would erase the + // freshly written content. try { - await fs.unlink(absolutePath) + if (!isMoveOntoSource) { + await fs.unlink(absolutePath) + } } catch (error) { - console.error(`Failed to delete original file after move: ${error}`) + // The source is still present (locked or protected): undo the destination + // so the file does not live in two places, and report the move as + // failed. Returning true here would let the whole-patch checkpoint persist + // a duplicated file as if the move had succeeded. + let restoreFailure: unknown + let recoveryPath: string | undefined + try { + if (destinationBackup !== undefined) { + // Atomic restore of the captured destination bytes: the rename + // replaces the clobbered destination in one step, so there is no + // window where the file is missing. + await fs.rename(backupPath, moveAbsolutePath) + } else { + await fs.rm(moveAbsolutePath, { force: true }) + } + } catch (restoreError) { + restoreFailure = restoreError + } + if (restoreFailure !== undefined) { + if (destinationBackup !== undefined) { + // The original destination bytes are still on disk under the backup + // name: keep them as the recovery artifact and report where they are. + // Writing a copy of the approved new content here would hide the + // original content that this branch is supposed to preserve. + console.error( + `Failed to restore pre-move state: ${restoreFailure instanceof Error ? restoreFailure.message : String(restoreFailure)}; the original content is preserved at ${backupPath}`, + ) + } else { + // There was no pre-existing destination, so the approved content is + // the only thing that could be lost: persist it under a recovery name. + const recoveryCandidate = path.join(moveDir, `${moveBase}.zoo-recovery-${moveStamp}`) + try { + // Same owner-only creation as the staging write above, and O_EXCL rather than + // truncation: a recovery copy holds private content, and silently overwriting + // whatever already stands at that name would destroy a file instead of preserving one. + await fs.writeFile(recoveryCandidate, newContent, { + encoding: "utf8", + mode: 0o600, + flag: "wx", + }) + recoveryPath = recoveryCandidate + console.error( + `Failed to restore pre-move state: ${restoreFailure instanceof Error ? restoreFailure.message : String(restoreFailure)}; kept the approved content at ${recoveryCandidate}`, + ) + } catch (recoveryError) { + console.error(`Failed to persist a move recovery copy: ${recoveryError}`) + } + } + } + // Best-effort cleanup of the staging file; the backup is gone once the + // restore rename committed (or is the only copy left on failure). + await fs.rm(stagingPath, { force: true }).catch(() => {}) + const restoreSuffix = + restoreFailure === undefined + ? "" + : ` The destination could not be restored and may still contain the new content: ${restoreFailure instanceof Error ? restoreFailure.message : String(restoreFailure)}` + const recoverySuffix = + destinationBackup !== undefined && restoreFailure !== undefined && recoveryPath === undefined + ? ` The original content is preserved at '${getReadablePath(task.cwd, path.relative(task.cwd, backupPath))}'.` + : recoveryPath === undefined + ? "" + : ` The approved new content was kept at '${getReadablePath(task.cwd, path.relative(task.cwd, recoveryPath))}'.` + const moveErrorMessage = `Failed to delete original file after move '${relPath}': ${error instanceof Error ? error.message : String(error)}${restoreSuffix}${recoverySuffix}` + await task.say("error", moveErrorMessage) + pushToolResult(formatResponse.toolError(moveErrorMessage)) + await task.diffViewProvider.reset() + return { ok: false, mutated } } + // The move committed: drop the destination backup. + await fs.rm(backupPath, { force: true }).catch(() => {}) + await task.fileContextTracker.trackFileContext(change.movePath, "roo_edited" as RecordSource) } else { // Save changes to the same file @@ -438,6 +734,8 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { await task.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs) } + mutated = true + mutation.committed = true await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) } @@ -446,7 +744,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { const message = await task.diffViewProvider.pushToolWriteResult(task, task.cwd, false) pushToolResult(message) await task.diffViewProvider.reset() - task.processQueuedMessages() + return { ok: true, mutated } } 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..9f17cd5c2a 100644 --- a/src/core/tools/EditFileTool.ts +++ b/src/core/tools/EditFileTool.ts @@ -1,7 +1,7 @@ import fs from "fs/promises" import path from "path" -import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" +import { type ClineSayTool, DEFAULT_PER_WRITE_CHECKPOINTS, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" @@ -11,6 +11,7 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" import { EXPERIMENT_IDS, experiments } from "../../shared/experiments" import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats" +import { checkpointSave } from "../../core/checkpoints" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -392,6 +393,7 @@ export class EditFileTool extends BaseTool<"edit_file"> { const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const perWriteCheckpoints = state?.perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -463,6 +465,14 @@ export class EditFileTool extends BaseTool<"edit_file"> { pushToolResult(message + replacementInfo) + if (perWriteCheckpoints) { + // Await so the checkpoint (staging + commit) completes before the + // diff state resets and queued work proceeds; fire-and-forget lets a + // queued write's own checkpoint interleave and lets shutdown lose the + // commit entirely. + await checkpointSave(task, false, true).catch(() => {}) + } + await task.diffViewProvider.reset() this.resetPartialState() diff --git a/src/core/tools/WriteToFileTool.ts b/src/core/tools/WriteToFileTool.ts index 0c5c80abb9..e3e7bf1728 100644 --- a/src/core/tools/WriteToFileTool.ts +++ b/src/core/tools/WriteToFileTool.ts @@ -1,7 +1,7 @@ import path from "path" import fs from "fs/promises" -import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" +import { type ClineSayTool, DEFAULT_PER_WRITE_CHECKPOINTS, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" @@ -13,6 +13,7 @@ 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 { checkpointSave } from "../checkpoints" import type { ToolUse } from "../../shared/tools" import { BaseTool, ToolCallbacks } from "./BaseTool" @@ -102,6 +103,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { const state = await provider?.getState() const diagnosticsEnabled = state?.diagnosticsEnabled ?? true const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS + const perWriteCheckpoints = state?.perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS const isPreventFocusDisruptionEnabled = experiments.isEnabled( state?.experiments ?? {}, EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION, @@ -177,6 +179,12 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> { pushToolResult(message) + if (perWriteCheckpoints) { + // Await so the checkpoint (staging + commit) finishes before the next + // queued write starts; otherwise two writes can collapse into one commit. + await checkpointSave(task, false, true).catch(() => {}) + } + await task.diffViewProvider.reset() this.resetPartialState() diff --git a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts index eb89838cad..6be71df1f3 100644 --- a/src/core/tools/__tests__/applyPatchTool.execute.spec.ts +++ b/src/core/tools/__tests__/applyPatchTool.execute.spec.ts @@ -4,13 +4,38 @@ 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: { + readFile: MockedFunction + unlink: MockedFunction + rm: MockedFunction + rename: MockedFunction + stat: MockedFunction + chmod: MockedFunction + writeFile: MockedFunction + } + }, +) + vi.mock("fs/promises", () => ({ default: { readFile: vi.fn().mockResolvedValue("original file content\n"), unlink: vi.fn().mockResolvedValue(undefined), + rm: vi.fn().mockResolvedValue(undefined), + rename: vi.fn().mockResolvedValue(undefined), + stat: vi.fn().mockResolvedValue({ mode: 0o755, uid: 0, gid: 0 }), + chmod: vi.fn().mockResolvedValue(undefined), + mkdir: vi.fn().mockResolvedValue(undefined), + writeFile: vi.fn().mockResolvedValue(undefined), }, })) @@ -22,6 +47,10 @@ vi.mock("../../../utils/pathUtils", () => ({ isPathOutsideWorkspace: vi.fn().mockReturnValue(false), })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + describe("ApplyPatchTool.execute - delete file success path", () => { const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction const mockedIsPathOutsideWorkspace = isPathOutsideWorkspace as MockedFunction @@ -38,6 +67,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 +84,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 +100,21 @@ describe("ApplyPatchTool.execute - delete file success path", () => { say: vi.fn().mockResolvedValue(undefined), processQueuedMessages: vi.fn(), didEditFile: false, + diffViewProvider: { + editType: "modify", + originalContent: undefined, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + revertChanges: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue({ finalContent: "saved" }), + saveChanges: vi.fn().mockResolvedValue(undefined), + pushToolWriteResult: vi.fn().mockResolvedValue("File saved successfully"), + } as unknown as Task["diffViewProvider"], + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as unknown as Task["fileContextTracker"], } mockAskApproval = vi.fn().mockResolvedValue(true) @@ -94,4 +146,826 @@ 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 the partial-state checkpoint when post-write bookkeeping rejects after the write", async () => { + const patch = `*** Begin Patch +*** Add File: src/new.ts ++file content +*** End Patch` + mockedFileExistsAtPath.mockResolvedValueOnce(false) + // The write commits, then a later bookkeeping call rejects before the handler can + // return: the committed mutation is tracked outside the handler-return boundary, so + // the partial-state checkpoint and the queued-message handoff must still happen. + vi.mocked(mockTask.diffViewProvider.pushToolWriteResult).mockRejectedValueOnce( + new Error("diagnostics unavailable"), + ) + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockTask.processQueuedMessages).toHaveBeenCalledOnce() + expect(mockHandleError).toHaveBeenCalled() + }) + + it("keeps the written content when a move targets the file's own path", async () => { + // A patch whose Move to resolves back onto its Update File path rewrites that path; + // it does not create a second copy. Unlinking the source afterwards - the same inode + // the destination now holds - would delete the content the patch just committed. + const patch = `*** Begin Patch +*** Update File: src/old.ts +*** Move to: src/old.ts +@@ +-original file content ++new content +*** End Patch` + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The staged content was renamed onto the path, and nothing was unlinked. + expect(mockedFsPromises.default.rename).toHaveBeenCalledWith( + expect.stringContaining(".staging"), + expect.stringMatching(/[\\/]old\.ts$/), + ) + expect(mockedFsPromises.default.unlink).not.toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockTask.diffViewProvider.pushToolWriteResult).toHaveBeenCalledOnce() + }) + + it("restores the pre-move state and fails the operation when source deletion fails", async () => { + const patch = `*** Begin Patch +*** Update File: src/old.ts +*** Move to: src/new.ts +@@ +-original file content ++new content +*** End Patch` + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // The move destination does not exist: the rollback must remove the + // destination rather than restore content that was never there. + mockedFsPromises.default.readFile.mockImplementation(async (p: unknown) => { + if (String(p).includes("new.ts")) { + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + return "original file content\n" + }) + mockedFsPromises.default.unlink.mockRejectedValueOnce( + Object.assign(new Error("EBUSY: resource busy or locked"), { code: "EBUSY" }), + ) + // The restore itself fails too: the failure is logged, not swallowed. + mockedFsPromises.default.rm.mockRejectedValueOnce(new Error("EROFS: read-only file system")) + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The destination write is undone so the file does not live in two places. + expect(mockedFsPromises.default.rm).toHaveBeenCalledWith(expect.stringContaining("new.ts"), { force: true }) + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("Failed to restore pre-move state")) + errorSpy.mockRestore() + // The move is reported as failed rather than as a successful write, and the + // failed rollback is part of the message: the destination may still hold the + // new content, which the model and user must know. + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Failed to delete original file after move"), + ) + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("could not be restored and may still contain the new content"), + ) + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("EROFS")) + // The approved content survives the failed rollback as an explicit recovery artifact. + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("The approved new content was kept at"), + ) + // ...and no whole-patch checkpoint is recorded for the failed move. + // The destination was written (then rolled back): that is a workspace mutation, so + // the partial-state checkpoint is recorded even though the patch reports failure. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // Restore the default readFile behavior for later tests. + mockedFsPromises.default.readFile.mockResolvedValue("original file content\n") + }) + + it("restores an existing destination when source deletion fails after the move clobbered it", async () => { + const patch = `*** Begin Patch +*** Update File: src/old.ts +*** Move to: src/new.ts +@@ +-original file content ++new content +*** End Patch` + // The destination already exists with its own content; the move writes + // over it before the source unlink runs. + mockedFsPromises.default.readFile.mockImplementation(async (p: unknown) => { + if (String(p).includes("new.ts")) { + return "existing destination content" + } + return "original file content\n" + }) + mockedFsPromises.default.unlink.mockRejectedValueOnce( + Object.assign(new Error("EBUSY: resource busy or locked"), { code: "EBUSY" }), + ) + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The captured destination content is restored by renaming the on-disk backup + // over the clobbered destination (one atomic replacement), never by rewriting + // the captured bytes and never by deleting a destination that existed before. + expect(mockedFsPromises.default.writeFile).not.toHaveBeenCalledWith( + expect.stringContaining("new.ts"), + "existing destination content", + ) + expect(mockedFsPromises.default.rename).toHaveBeenCalledWith( + expect.stringContaining(".backup"), + expect.stringMatching(/[\\/]new\.ts$/), + ) + // The replacement keeps the destination's mode instead of installing the + // process defaults on the staging file. + expect(mockedFsPromises.default.chmod).toHaveBeenCalledWith(expect.stringContaining(".staging"), 0o755) + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Failed to delete original file after move"), + ) + // The destination was written (then rolled back): that is a workspace mutation, so + // the partial-state checkpoint is recorded even though the patch reports failure. + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + // Restore the default readFile behavior for later tests. + mockedFsPromises.default.readFile.mockResolvedValue("original file content\n") + }) + + it("names the retained backup and records a checkpoint when the move write fails and the restore rename fails", async () => { + const patch = `*** Begin Patch +*** Update File: src/old.ts +*** Move to: src/new.ts +@@ +-original file content ++new content +*** End Patch` + // The destination exists, so the write-catch path puts it back by renaming the + // on-disk backup over the destination. + mockedFsPromises.default.readFile.mockImplementation(async (p: unknown) => { + if (String(p).includes("new.ts")) { + return "existing destination content" + } + return "original file content\n" + }) + // The staging write succeeds, the staging -> destination rename fails, and the + // backup -> destination restore rename fails as well: the destination is now + // missing and its content survives only under the backup name. + mockedFsPromises.default.rename.mockImplementation(async (from: unknown, to: unknown) => { + const source = String(from) + const target = String(to) + if (target.includes("new.ts") && source.includes(".staging")) { + throw Object.assign(new Error("EACCES: permission denied, rename"), { code: "EACCES" }) + } + if (target.includes("new.ts") && source.includes(".backup")) { + throw Object.assign(new Error("EPERM: operation not permitted, restore"), { code: "EPERM" }) + } + return undefined + }) + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Failed to write move destination")) + // A user who lost the destination has to be told where its bytes are. + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringMatching(/preserved at '[^']*\.backup[^']*'/)) + // A missing destination is a workspace mutation: the partial-state checkpoint and + // the queued-message handoff both have to run. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true) + expect(mockTask.processQueuedMessages).toHaveBeenCalledOnce() + // Restore the default mock behavior for later tests. + mockedFsPromises.default.readFile.mockResolvedValue("original file content\n") + mockedFsPromises.default.rename.mockResolvedValue(undefined) + }) + + it("refuses the move before writing when the destination cannot be inspected", async () => { + const patch = `*** Begin Patch +*** Update File: src/old.ts +*** Move to: src/new.ts +@@ +-original file content ++new content +*** End Patch` + // An EACCES on the destination is not "absent": treating every read + // failure as missing could delete an inaccessible destination. + mockedFsPromises.default.readFile.mockImplementation(async (p: unknown) => { + if (String(p).includes("new.ts")) { + throw Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + } + return "original file content\n" + }) + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The destination is never written and the source is never unlinked. + expect(mockedFsPromises.default.writeFile).not.toHaveBeenCalled() + expect(mockedFsPromises.default.unlink).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Cannot inspect move destination")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + // Restore the default readFile behavior for later tests. + mockedFsPromises.default.readFile.mockResolvedValue("original file content\n") + }) + + it("persists an explicit partial-state checkpoint before handoff when a later operation fails", async () => { + const patch = `*** Begin Patch +*** Delete File: src/obsolete.ts +*** Delete File: src/locked.ts +*** End Patch` + // The first delete succeeds; the second fails. unlink is one shared double and + // mockRejectedValueOnce orders by call, not by path, so the rejection has to be + // armed after a resolved first call - otherwise this case exercises the wrong delete. + mockedFsPromises.default.unlink + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error("EPERM: operation not permitted")) + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The retained successful delete gets an explicit partial-state + // checkpoint before the queued-message handoff. + expect(mockedCheckpointSave).toHaveBeenCalledTimes(1) + const checkpointAt = mockedCheckpointSave.mock.invocationCallOrder[0] ?? Infinity + const handoffAt = vi.mocked(mockTask.processQueuedMessages).mock.invocationCallOrder[0] ?? Infinity + // A handoff that never happened used to be the strongest pass in the file: the + // ?? Infinity default makes any checkpoint order beat an absent handoff, so the + // count is asserted before the order is compared. + expect(vi.mocked(mockTask.processQueuedMessages).mock.calls.length).toBeGreaterThan(0) + expect(checkpointAt).toBeLessThan(handoffAt) + // Which delete ran is what the mock ordering is for: both paths were attempted + // and exactly two unlinks happened - a first-delete failure would have stopped + // the loop before the second path was reached. + expect(mockedFsPromises.default.unlink).toHaveBeenCalledTimes(2) + expect(mockedFsPromises.default.unlink).toHaveBeenCalledWith(expect.stringMatching(/[\\/]obsolete\.ts$/)) + expect(mockedFsPromises.default.unlink).toHaveBeenCalledWith(expect.stringMatching(/[\\/]locked\.ts$/)) + }) + it("persists a partial-state checkpoint when a later handler rejects outright after an earlier write", async () => { + const patch = `*** Begin Patch +*** Delete File: src/obsolete.ts +*** Delete File: src/locked.ts +*** End Patch` + // The first delete approves and succeeds; the second handler rejects + // outright (e.g. the approval stream dies), so the outer catch runs + // and the partial-state checkpoint branch above it is skipped. + mockAskApproval.mockResolvedValueOnce(true).mockRejectedValueOnce(new Error("approval stream died")) + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The retained first delete still gets rollback metadata before the + // queued-message handoff in the finally block. + expect(mockedCheckpointSave).toHaveBeenCalledTimes(1) + const checkpointAt = mockedCheckpointSave.mock.invocationCallOrder[0] ?? Infinity + const handoffAt = vi.mocked(mockTask.processQueuedMessages).mock.invocationCallOrder[0] ?? Infinity + // Same vacuity as the case above: assert the handoff happened, then its order. + expect(vi.mocked(mockTask.processQueuedMessages).mock.calls.length).toBeGreaterThan(0) + expect(checkpointAt).toBeLessThan(handoffAt) + expect(mockHandleError).toHaveBeenCalled() + }) + it("records the partial-state checkpoint when the add save rejects after its bytes landed", async () => { + const patch = `*** Begin Patch +*** Add File: src/new.ts ++file content +*** End Patch` + // The add does not exist yet, and the focus-preserving save path is enabled, so the + // bytes are written by saveDirectly. + mockedFileExistsAtPath.mockResolvedValueOnce(false) + const provider = { + getState: vi.fn().mockResolvedValue({ experiments: { preventFocusDisruption: true } }), + } + mockTask.providerRef = { deref: () => provider } as unknown as Task["providerRef"] + // The file lands on disk inside the save, which then rejects while finishing the + // document, so the handler never returns and never reaches its own tracking lines. + vi.mocked(mockTask.diffViewProvider.saveDirectly).mockRejectedValueOnce( + new Error("EPERM: operation not permitted, open 'src/new.ts'"), + ) + + await tool.execute({ patch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockTask.diffViewProvider.saveDirectly).toHaveBeenCalledOnce() + // The failure is the save itself, not the bookkeeping after it. + expect(mockTask.diffViewProvider.pushToolWriteResult).not.toHaveBeenCalled() + expect(mockHandleError).toHaveBeenCalledWith("apply patch", expect.any(Error)) + // The retained file is a workspace mutation, so the partial-state checkpoint runs + // before the queued-message handoff even though the patch reports failure. + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true) + expect(mockTask.processQueuedMessages).toHaveBeenCalledOnce() + }) + + it("records one suppressed checkpoint for the whole patch (default-on)", async () => { + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted")) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true) + }) + + it("awaits the checkpoint before completing the patch", async () => { + // An unawaited save let the tool complete (and queued work start) while + // the checkpoint was still committing - a queued write could interleave + // its own checkpoint and shutdown could lose the commit. + let releaseCheckpoint: () => void = () => {} + mockedCheckpointSave.mockImplementationOnce( + () => new Promise((resolve) => (releaseCheckpoint = resolve)), + ) + let settled = false + const run = tool + .execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + .then(() => { + settled = true + }) + await vi.waitFor(() => expect(mockedCheckpointSave).toHaveBeenCalledOnce()) + expect(settled).toBe(false) + releaseCheckpoint() + await run + expect(settled).toBe(true) + }) + it("defers the queued-message handoff until the checkpoint completes", async () => { + // processQueuedMessages schedules submitUserMessage on a timer; a + // handler that hands off before the whole-patch checkpoint completes + // lets the queued turn start while the commit is still running. + let releaseCheckpoint: () => void = () => {} + mockedCheckpointSave.mockImplementationOnce( + () => new Promise((resolve) => (releaseCheckpoint = resolve)), + ) + const run = tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + await vi.waitFor(() => expect(mockedCheckpointSave).toHaveBeenCalledOnce()) + expect(mockTask.processQueuedMessages).not.toHaveBeenCalled() + releaseCheckpoint() + await run + expect(mockTask.processQueuedMessages).toHaveBeenCalled() + }) + + it("hands off queued messages when checkpoints are disabled", async () => { + // The handoff must not depend on the checkpoint setting: disabling + // checkpoints must not strand queued messages. + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ perWriteCheckpoints: false }), + }) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockedCheckpointSave).not.toHaveBeenCalled() + expect(mockTask.processQueuedMessages).toHaveBeenCalled() + }) + + it("does not hand off queued messages when the patch fails", async () => { + // A failed file operation must not hand off: the model is waiting + // on the error result, not on queued input. + mockedFsPromises.default.unlink.mockRejectedValueOnce(new Error("EBUSY")) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockTask.processQueuedMessages).not.toHaveBeenCalled() + }) + + it("hands off queued messages after a partial patch failure", async () => { + // Fidelity: upstream handed off in each handler right after every + // successful write, so a patch whose first write succeeded and whose + // later write failed still drained the queue. The deferral preserves + // that condition (a write happened) and only moves its timing after + // the checkpoint attempt; requiring full-patch success would strand + // queued input on any partial failure. + const mixedPatch = `*** Begin Patch +*** Add File: src/new.ts ++hello +*** Delete File: src/obsolete.ts +*** End Patch` + // Add sees no existing file and succeeds; the delete's unlink fails. + mockedFileExistsAtPath.mockResolvedValueOnce(false) + mockedFsPromises.default.unlink.mockRejectedValueOnce(new Error("EBUSY")) + + await tool.execute({ patch: mixedPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The retained successful write gets an explicit partial-state + // checkpoint before the queued-message handoff. + expect(mockedCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockTask.processQueuedMessages).toHaveBeenCalled() + expect(mockedCheckpointSave.mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(mockTask.processQueuedMessages).mock.invocationCallOrder[0] ?? Infinity, + ) + }) + + it("hands off queued messages when a later file fails the access check", async () => { + // The rooignored path makes execute return early, but the earlier + // successful write had already handed off upstream - the deferred + // handoff preserves that too. + const mixedPatch = `*** Begin Patch +*** Delete File: src/obsolete.ts +*** Add File: src/secret.ts ++hidden +*** End Patch` + const validateAccess = mockTask.rooIgnoreController?.validateAccess as MockedFunction< + (path: string) => boolean + > + validateAccess.mockReturnValueOnce(true).mockReturnValueOnce(false) + + await tool.execute({ patch: mixedPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // The earlier successful delete is retained: an explicit partial-state + // checkpoint persists it before the deferred handoff. + expect(mockedCheckpointSave).toHaveBeenCalledTimes(1) + expect(mockTask.processQueuedMessages).toHaveBeenCalledTimes(1) + // The access-failed file is never written and the loop stops there. + expect(mockTask.fileContextTracker.trackFileContext).not.toHaveBeenCalledWith( + "src/secret.ts", + expect.anything(), + ) + }) + + it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { + // Structural cast for the test double (matches the mock style used for the controllers above). + const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref + ref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ perWriteCheckpoints: false }), + }) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when patch processing fails", async () => { + // A malformed patch fails at parse time, before the change loop and + // the post-loop checkpoint hook. + const badPatch = `*** Begin Patch +*** This is not a valid hunk +*** End Patch` + + await tool.execute({ patch: badPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockTask.recordToolError).toHaveBeenCalledWith("apply_patch") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + }) + + describe("checkpoint only for fully successful patches (B1)", () => { + const mockedCheckpointSave = checkpointSave as MockedFunction + const deletePatch = `*** Begin Patch +*** Delete File: src/obsolete.ts +*** End Patch` + const addPatch = `*** Begin Patch +*** Add File: src/new.ts ++hello ++world +*** End Patch` + const updatePatch = `*** Begin Patch +*** Update File: src/test.ts +@@ +-original file content ++modified content +*** End Patch` + const updateNoDiffPatch = `*** Begin Patch +*** Update File: src/test.ts +@@ +-original file content ++original file content +*** End Patch` + const movePatch = `*** Begin Patch +*** Update File: src/test.ts +*** Move to: src/moved.ts +@@ +-original file content ++modified content +*** End Patch` + + it("does not record a checkpoint when the user rejects the patch", async () => { + // Rejected approval: the handler early-returns without recording a + // tool error, so the success flag must come from the handler itself. + mockAskApproval.mockResolvedValue(false) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("Delete operation was rejected by the user.") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the file to delete does not exist", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("File not found")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the delete write fails", async () => { + mockedFsPromises.default.unlink.mockRejectedValueOnce(new Error("EBUSY")) + + await tool.execute({ patch: deletePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Failed to delete file")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the added file already exists", async () => { + // fileExistsAtPath resolves true by default in beforeEach. + + await tool.execute({ patch: addPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("File already exists")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the user rejects the add", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + mockAskApproval.mockResolvedValue(false) + + await tool.execute({ patch: addPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("Changes were rejected by the user.") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("records a checkpoint when the add succeeds", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + + await tool.execute({ patch: addPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("File saved successfully") + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + + it("does not record a checkpoint when the file to update does not exist", async () => { + mockedFileExistsAtPath.mockResolvedValue(false) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("File not found")) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("records a checkpoint when the update is a no-op (no changes needed)", async () => { + // A no-op change is not a failure, so the whole-patch checkpoint still runs. + + await tool.execute({ patch: updateNoDiffPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("No changes needed")) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + + it("does not record a checkpoint when the user rejects the update", async () => { + mockAskApproval.mockResolvedValue(false) + + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith("Changes were rejected by the user.") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the move destination is not allowed", async () => { + // First validateAccess call (source path, in the execute loop) passes; + // the move destination check inside the handler fails. + const validateAccess = ( + mockTask["rooIgnoreController"] as unknown as { validateAccess: MockedFunction<() => boolean> } + ).validateAccess + validateAccess.mockReturnValueOnce(true).mockReturnValue(false) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockTask.say).toHaveBeenCalledWith("rooignore_error", "src/moved.ts") + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the move destination is write-protected", async () => { + // Source path check (execute loop) passes; the move destination fails. + const isWriteProtected = ( + mockTask["rooProtectedController"] as unknown as { + isWriteProtected: MockedFunction<(p: string) => boolean> + } + ).isWriteProtected + isWriteProtected.mockReturnValueOnce(false).mockReturnValue(true) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Cannot move file to write-protected path"), + ) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the move destination is outside the workspace", async () => { + // Source path (first call) is inside; the move destination (second) + // call is outside the workspace. + mockedIsPathOutsideWorkspace.mockReturnValueOnce(false).mockReturnValue(true) + + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockPushToolResult).toHaveBeenCalledWith( + expect.stringContaining("Cannot move file to path outside workspace"), + ) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("records a checkpoint when the move succeeds", async () => { + await tool.execute({ patch: movePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + // path is platform-dependent (Windows resolves cwd to a drive path); + // assert on the written content instead. + // The staging file is created owner-only; the destination mode is re-applied + // separately before the atomic rename. + expect(mockedFsPromises.default.writeFile).toHaveBeenCalledWith(expect.any(String), "modified content\n", { + encoding: "utf8", + mode: 0o600, + }) + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + + it("records a checkpoint when the in-place update succeeds", async () => { + await tool.execute({ patch: updatePatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true) + }) + + it("records exactly one checkpoint for a multi-file patch", async () => { + // B1's core contract: one checkpoint per patch, not per file - a + // regression to per-change saves would double the checkpoint count. + const multiPatch = [ + "*** Begin Patch", + "*** Add File: src/new.ts", + "+hello", + "+world", + "*** Update File: src/test.ts", + "@@", + "-original file content", + "+modified content", + "*** End Patch", + ].join(String.fromCharCode(10)) + mockedFileExistsAtPath.mockImplementation((filePath: string) => + Promise.resolve(!String(filePath).toLowerCase().endsWith("new.ts")), + ) + + await tool.execute({ patch: multiPatch }, mockTask as Task, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + toolCallId: "test-tool-call-id", + }) + + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockedCheckpointSave).toHaveBeenCalledOnce() + }) + }) }) diff --git a/src/core/tools/__tests__/editFileTool.spec.ts b/src/core/tools/__tests__/editFileTool.spec.ts index 95a67f04d7..b743fc9c38 100644 --- a/src/core/tools/__tests__/editFileTool.spec.ts +++ b/src/core/tools/__tests__/editFileTool.spec.ts @@ -7,6 +7,7 @@ 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 { editFileTool } from "../EditFileTool" vi.mock("fs/promises", () => ({ @@ -59,6 +60,10 @@ vi.mock("../../diff/stats", () => ({ computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + vi.mock("vscode", () => ({ window: { showWarningMessage: vi.fn().mockResolvedValue(undefined), @@ -777,4 +782,61 @@ 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() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true) + }) + + it("awaits the checkpoint before resetting the diff state", async () => { + // Fire-and-forget would let diffViewProvider.reset() and the queued-work + // handoff race an unfinished checkpoint commit (and lose it at + // shutdown): the save must complete before the reset. + let releaseCheckpoint: () => void = () => {} + mockedCheckpointSave.mockImplementationOnce( + () => new Promise((resolve) => (releaseCheckpoint = resolve)), + ) + let settled = false + const run = executeEditFileTool({}).then(() => { + settled = true + }) + await vi.waitFor(() => expect(mockedCheckpointSave).toHaveBeenCalledOnce()) + expect(settled).toBe(false) + expect(mockTask.diffViewProvider.reset).not.toHaveBeenCalled() + releaseCheckpoint() + await run + expect(mockTask.diffViewProvider.reset).toHaveBeenCalled() + }) + + it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => { + mockTask.providerRef.deref = vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: true, + writeDelayMs: 1000, + experiments: {}, + perWriteCheckpoints: false, + }), + }) + + await executeEditFileTool({}) + + expect(mockTask.consecutiveMistakeCount).toBe(0) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + + it("does not record a checkpoint when the edit fails", async () => { + mockTask.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed")) + + await executeEditFileTool({}) + + expect(mockHandleError).toHaveBeenCalledWith("edit_file", expect.any(Error)) + expect(mockedCheckpointSave).not.toHaveBeenCalled() + }) + }) }) diff --git a/src/core/tools/__tests__/writeToFileTool.spec.ts b/src/core/tools/__tests__/writeToFileTool.spec.ts index 7cb9d01585..6d4dbf5b1c 100644 --- a/src/core/tools/__tests__/writeToFileTool.spec.ts +++ b/src/core/tools/__tests__/writeToFileTool.spec.ts @@ -8,6 +8,7 @@ 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 { writeToFileTool } from "../WriteToFileTool" vi.mock("path", async () => { @@ -89,6 +90,10 @@ vi.mock("../../ignore/RooIgnoreController", () => ({ }, })) +vi.mock("../../checkpoints", () => ({ + checkpointSave: vi.fn().mockResolvedValue(undefined), +})) + describe("writeToFileTool", () => { // Test data const testFilePath = "test/file.txt" @@ -473,4 +478,76 @@ 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() + expect(mockedCheckpointSave).toHaveBeenCalledWith(mockCline, false, true) + }) + + 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() + }) + }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6cec3bdbe7..9f1b9b101a 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -70,6 +70,7 @@ import { ORGANIZATION_ALLOW_ALL, DEFAULT_MODES, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_PER_WRITE_CHECKPOINTS, getModelId, isRetiredProvider, modelIdKeys, @@ -3128,6 +3129,7 @@ export class ClineProvider ttsSpeed, enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, soundVolume, writeDelayMs, diffFuzzyThreshold, @@ -3300,6 +3302,7 @@ export class ClineProvider ttsSpeed: ttsSpeed ?? 1.0, enableCheckpoints: enableCheckpoints ?? true, checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, shouldShowAnnouncement: telemetrySetting !== "unset" && lastShownAnnouncementId !== this.latestAnnouncementId, allowedCommands: mergedAllowedCommands, @@ -3544,6 +3547,7 @@ export class ClineProvider ttsSpeed: stateValues.ttsSpeed ?? 1.0, enableCheckpoints: stateValues.enableCheckpoints ?? true, checkpointTimeout: stateValues.checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: stateValues.perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, soundVolume: stateValues.soundVolume, writeDelayMs: stateValues.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS, diffFuzzyThreshold: stateValues.diffFuzzyThreshold ?? DEFAULT_DIFF_FUZZY_THRESHOLD, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 5cb05f62db..23b4972818 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3182,6 +3182,7 @@ describe("ClineProvider", () => { soundEnabled: false, ttsEnabled: false, enableCheckpoints: false, + perWriteCheckpoints: false, writeDelayMs: 1000, mcpEnabled: true, mode: defaultModeSlug, @@ -4308,6 +4309,65 @@ 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("persists per-write checkpoints sent through updateSettings to the message handler", async () => { + await provider.resolveWebviewView(mockWebviewView) + // No cast: this file is already at its suppressed no-explicit-any budget (195), and + // the double's call list is untyped, so the handler comes out of it directly. + const messageHandler = mockWebviewView.webview.onDidReceiveMessage.mock.calls[0][0] + + await messageHandler({ type: "updateSettings", updatedSettings: { perWriteCheckpoints: false } }) + + // Through the handler rather than straight into the proxy: every other per-write + // assertion in this file drives contextProxy directly, so a handler that dropped + // this key would have left all of them green. + expect(updateGlobalStateSpy).toHaveBeenCalledWith("perWriteCheckpoints", false) + expect(mockContext.globalState.update).toHaveBeenCalledWith("perWriteCheckpoints", false) + const state = await provider.getState() + expect(state.perWriteCheckpoints).toBe(false) + }) + + test("getStateToPostToWebview returns the saved per-write checkpoints setting", async () => { + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("perWriteCheckpoints", true) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(true) + }) + + test("getStateToPostToWebview returns false when per-write checkpoints is saved as false", async () => { + // The default is also true, so only an explicit false proves that the + // stored value (rather than the default) reaches the webview state. + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("perWriteCheckpoints", false) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(false) + }) + + test("getStateToPostToWebview defaults per-write checkpoints to true when unset", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(true) + }) + test("getStateToPostToWebview returns the saved blanket auto-deny setting", async () => { await provider.resolveWebviewView(mockWebviewView) await provider.contextProxy.setValue("alwaysDenyUnapprovedCommands", true) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 3c1d33ae28..d56d08aed1 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -14,6 +14,7 @@ import { } from "@roo-code/types" import { createDirectoriesForFile } from "../../utils/fs" +import { writeFileAtomically } from "./atomicWrite" import { arePathsEqual, getReadablePath } from "../../utils/path" import { formatResponse } from "../../core/prompts/responses" import { diagnosticsToProblemsString, getNewDiagnostics } from "../diagnostics" @@ -1155,9 +1156,10 @@ export class DiffViewProvider { // Get diagnostics before editing the file this.preDiagnostics = vscode.languages.getDiagnostics() - // Write the content directly to the file + // Write the content directly to the file, atomically: a crash mid-write must + // not leave the destination truncated. await createDirectoriesForFile(absolutePath) - await fs.writeFile(absolutePath, content, "utf-8") + await writeFileAtomically(absolutePath, content) // Open the document to ensure diagnostics are loaded // When openFile is false (PREVENT_FOCUS_DISRUPTION enabled), we only open in memory diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index e94a66e1db..ad5372e88b 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -17,7 +17,15 @@ vi.mock("delay", () => ({ vi.mock("fs/promises", () => ({ readFile: vi.fn().mockResolvedValue("file content"), writeFile: vi.fn().mockResolvedValue(undefined), + rename: vi.fn().mockResolvedValue(undefined), + rm: vi.fn().mockResolvedValue(undefined), access: vi.fn().mockResolvedValue(undefined), + // The atomic writer resolves an existing destination and preserves its mode; + // the default double behaves like a brand-new file (ENOENT). + realpath: vi.fn().mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })), + stat: vi.fn().mockResolvedValue({ mode: 0o644, uid: 0, gid: 0 }), + chmod: vi.fn().mockResolvedValue(undefined), + chown: vi.fn().mockResolvedValue(undefined), })) // Mock utils @@ -782,9 +790,15 @@ describe("DiffViewProvider", () => { const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 2000) - // Verify file was written + // Verify the content was written through an atomic staging file + rename const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + expect(fs.writeFile).toHaveBeenCalledWith(expect.stringContaining(".staging"), "new content", { + encoding: "utf-8", + mode: 0o600, + }) + const stagingPath = vi.mocked(fs.writeFile).mock.calls.find((call) => call[1] === "new content")?.[0] + expect(stagingPath).toContain(".test.ts.") + expect(fs.rename).toHaveBeenCalledWith(stagingPath, `${mockCwd}/test.ts`) // Verify file was opened without focus expect(vscode.window.showTextDocument).toHaveBeenCalledWith( @@ -802,12 +816,33 @@ describe("DiffViewProvider", () => { expect(result.finalContent).toBe("new content") }) + it("keeps the same error and cleans up the staging file when the final rename fails", async () => { + const fs = await import("fs/promises") + const renameError = Object.assign(new Error("EPERM: rename blocked"), { code: "EPERM" }) + vi.mocked(fs.rename).mockRejectedValueOnce(renameError) + + await expect(diffViewProvider.saveDirectly("test.ts", "new content", true, true, 1000)).rejects.toBe( + renameError, + ) + + const stagingPath = vi.mocked(fs.writeFile).mock.calls.find((call) => call[1] === "new content")?.[0] + expect(renameError).toBeInstanceOf(Error) + expect(fs.rename).toHaveBeenCalledWith(stagingPath, `${mockCwd}/test.ts`) + expect(fs.rm).toHaveBeenCalledWith(stagingPath, { force: true }) + }) + it("should not open file when openWithoutFocus is false", async () => { await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 1000) - // Verify file was written + // Verify the content was written through an atomic staging file + rename const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + expect(fs.writeFile).toHaveBeenCalledWith(expect.stringContaining(".staging"), "new content", { + encoding: "utf-8", + mode: 0o600, + }) + const stagingPath = vi.mocked(fs.writeFile).mock.calls.find((call) => call[1] === "new content")?.[0] + expect(stagingPath).toContain(".test.ts.") + expect(fs.rename).toHaveBeenCalledWith(stagingPath, `${mockCwd}/test.ts`) // Verify file was NOT opened expect(vscode.window.showTextDocument).not.toHaveBeenCalled() @@ -820,9 +855,15 @@ describe("DiffViewProvider", () => { await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 1000) - // Verify file was written + // Verify the content was written through an atomic staging file + rename const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + expect(fs.writeFile).toHaveBeenCalledWith(expect.stringContaining(".staging"), "new content", { + encoding: "utf-8", + mode: 0o600, + }) + const stagingPath = vi.mocked(fs.writeFile).mock.calls.find((call) => call[1] === "new content")?.[0] + expect(stagingPath).toContain(".test.ts.") + expect(fs.rename).toHaveBeenCalledWith(stagingPath, `${mockCwd}/test.ts`) // Verify delay was NOT called expect(mockDelay).not.toHaveBeenCalled() diff --git a/src/integrations/editor/__tests__/atomicWrite.spec.ts b/src/integrations/editor/__tests__/atomicWrite.spec.ts new file mode 100644 index 0000000000..7b74e05768 --- /dev/null +++ b/src/integrations/editor/__tests__/atomicWrite.spec.ts @@ -0,0 +1,64 @@ +// npx vitest run integrations/editor/__tests__/atomicWrite.spec.ts + +import fs from "fs/promises" +import os from "os" +import path from "path" + +import { afterEach, beforeEach, describe, expect, it } from "vitest" + +import { writeFileAtomically } from "../atomicWrite" + +describe("writeFileAtomically", () => { + let dir: string + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), "atomic-write-")) + }) + + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }) + }) + + it("creates a new file with the complete content and no staging leftovers", async () => { + const target = path.join(dir, "new.txt") + + await writeFileAtomically(target, "hello") + + expect(await fs.readFile(target, "utf8")).toBe("hello") + expect(await fs.readdir(dir)).toEqual(["new.txt"]) + }) + + // Windows only maps the read-only attribute onto chmod, so POSIX mode bits are + // not observable there; the ubuntu lane covers this regression. + const posixOnly = it.skipIf(process.platform === "win32") + + posixOnly("preserves the destination mode for an executable file", async () => { + const target = path.join(dir, "script.sh") + await fs.writeFile(target, "old", "utf8") + await fs.chmod(target, 0o755) + + await writeFileAtomically(target, "new") + + expect(await fs.readFile(target, "utf8")).toBe("new") + const stat = await fs.stat(target) + expect(stat.mode & 0o777).toBe(0o755) + }) + + posixOnly("updates a symlink through its target without replacing the symlink", async () => { + const target = path.join(dir, "real.txt") + const link = path.join(dir, "link.txt") + await fs.writeFile(target, "old", "utf8") + try { + await fs.symlink(target, link) + } catch (error) { + // Creating symlinks needs Developer Mode or admin rights on Windows. + if ((error as NodeJS.ErrnoException).code === "EPERM") return + throw error + } + + await writeFileAtomically(link, "new") + + expect(await fs.readFile(target, "utf8")).toBe("new") + expect((await fs.lstat(link)).isSymbolicLink()).toBe(true) + }) +}) diff --git a/src/integrations/editor/atomicWrite.ts b/src/integrations/editor/atomicWrite.ts new file mode 100644 index 0000000000..6bef23ec4f --- /dev/null +++ b/src/integrations/editor/atomicWrite.ts @@ -0,0 +1,52 @@ +import * as fs from "fs/promises" + +/** + * Write a file so readers only ever observe the previous bytes or the complete + * new bytes: the content goes to a sibling staging file first and is renamed into + * place (same directory, so the rename is atomic on one volume). + * + * The destination is resolved first so a symlink is updated through its target + * instead of being replaced by a regular file, and the resolved target's mode (and, + * where permitted, ownership) is applied to the staging file: a fresh staging file + * carries the process defaults, which would otherwise widen a 0600 or executable + * destination. + */ +export async function writeFileAtomically(absolutePath: string, content: string, mode?: number): Promise { + let targetPath = absolutePath + let targetMode = mode + let targetOwnership: { uid: number; gid: number } | undefined + try { + targetPath = await fs.realpath(absolutePath) + const stat = await fs.stat(targetPath) + targetMode = targetMode ?? stat.mode & 0o7777 + targetOwnership = { uid: stat.uid, gid: stat.gid } + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") { + throw error + } + } + // Derive the sibling staging path with plain string operations so the helper stays + // usable in unit tests that mock the "path" module down to a few functions. + const separatorIndex = Math.max(targetPath.lastIndexOf("/"), targetPath.lastIndexOf("\\")) + const directory = separatorIndex >= 0 ? targetPath.slice(0, separatorIndex) : "." + const baseName = separatorIndex >= 0 ? targetPath.slice(separatorIndex + 1) : targetPath + const stagingPath = `${directory}/.${baseName}.${process.pid}.${Date.now()}.staging` + try { + // Owner-only from creation: staged content must never be readable by others, + // even between this write and the final rename. + await fs.writeFile(stagingPath, content, { encoding: "utf-8", mode: 0o600 }) + if (targetMode !== undefined) { + await fs.chmod(stagingPath, targetMode) + } + if (targetOwnership !== undefined) { + // Preserve ownership where the platform permits it; a chown the writer may + // not perform (EPERM without privileges) leaves the writer as owner, which is + // also the normal outcome for newly created files. + await fs.chown(stagingPath, targetOwnership.uid, targetOwnership.gid).catch(() => {}) + } + await fs.rename(stagingPath, targetPath) + } catch (error) { + await fs.rm(stagingPath, { force: true }).catch(() => {}) + throw error + } +} diff --git a/webview-ui/src/components/settings/CheckpointSettings.tsx b/webview-ui/src/components/settings/CheckpointSettings.tsx index dd28f6615f..7ea12ef873 100644 --- a/webview-ui/src/components/settings/CheckpointSettings.tsx +++ b/webview-ui/src/components/settings/CheckpointSettings.tsx @@ -13,17 +13,20 @@ import { DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, MAX_CHECKPOINT_TIMEOUT_SECONDS, MIN_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_PER_WRITE_CHECKPOINTS, } from "@roo-code/types" type CheckpointSettingsProps = HTMLAttributes & { enableCheckpoints?: boolean checkpointTimeout?: number - setCachedStateField: SetCachedStateField<"enableCheckpoints" | "checkpointTimeout"> + perWriteCheckpoints?: boolean + setCachedStateField: SetCachedStateField<"enableCheckpoints" | "checkpointTimeout" | "perWriteCheckpoints"> } export const CheckpointSettings = ({ enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, setCachedStateField, ...props }: CheckpointSettingsProps) => { @@ -33,6 +36,22 @@ export const CheckpointSettings = ({ {t("settings:sections.checkpoints")}
+ + { + setCachedStateField("perWriteCheckpoints", e.target.checked) + }}> + {t("settings:checkpoints.perWrite.label")} + +
+ {t("settings:checkpoints.perWrite.description")} +
+ + (({ onDone, t autoCondenseContextPercent, enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, experiments, maxOpenTabsContext, maxWorkspaceFiles, @@ -444,6 +446,7 @@ const SettingsView = forwardRef(({ onDone, t ttsSpeed, enableCheckpoints: enableCheckpoints ?? false, checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, writeDelayMs, diffFuzzyThreshold, terminalShellIntegrationTimeout: terminalShellIntegrationTimeout ?? 30_000, @@ -911,6 +914,7 @@ const SettingsView = forwardRef(({ onDone, t )} diff --git a/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx b/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx new file mode 100644 index 0000000000..59a2e11f15 --- /dev/null +++ b/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx @@ -0,0 +1,201 @@ +// npx vitest src/components/settings/__tests__/CheckpointSettings.spec.tsx + +import { render, screen, fireEvent } from "@/utils/test-utils" +import { CheckpointSettings } from "../CheckpointSettings" +import enSettings from "@/i18n/locales/en/settings.json" +import caSettings from "@/i18n/locales/ca/settings.json" +import deSettings from "@/i18n/locales/de/settings.json" +import esSettings from "@/i18n/locales/es/settings.json" +import frSettings from "@/i18n/locales/fr/settings.json" +import hiSettings from "@/i18n/locales/hi/settings.json" +import idSettings from "@/i18n/locales/id/settings.json" +import itSettings from "@/i18n/locales/it/settings.json" +import jaSettings from "@/i18n/locales/ja/settings.json" +import koSettings from "@/i18n/locales/ko/settings.json" +import nlSettings from "@/i18n/locales/nl/settings.json" +import plSettings from "@/i18n/locales/pl/settings.json" +import ptBRSettings from "@/i18n/locales/pt-BR/settings.json" +import ruSettings from "@/i18n/locales/ru/settings.json" +import trSettings from "@/i18n/locales/tr/settings.json" +import viSettings from "@/i18n/locales/vi/settings.json" +import zhCNSettings from "@/i18n/locales/zh-CN/settings.json" +import zhTWSettings from "@/i18n/locales/zh-TW/settings.json" +// The per-write checkpoint setting ships a description in every locale file, and the render +// above mocks the translation hook, so the shipped copy is asserted against the locale +// resources themselves. Access goes through a guard rather than a cast so a missing key +// fails loudly instead of comparing undefined. +// The three tools whose successful writes record a checkpoint. +const WRITE_TOOLS = ["write_to_file", "edit_file", "apply_patch"] + +const localeSettings: Record = { + en: enSettings, + ca: caSettings, + de: deSettings, + es: esSettings, + fr: frSettings, + hi: hiSettings, + id: idSettings, + it: itSettings, + ja: jaSettings, + ko: koSettings, + nl: nlSettings, + pl: plSettings, + "pt-BR": ptBRSettings, + ru: ruSettings, + tr: trSettings, + vi: viSettings, + "zh-CN": zhCNSettings, + "zh-TW": zhTWSettings, +} + +function perWriteDescription(settings: unknown): string { + const resources = settings as { checkpoints?: { perWrite?: { description?: unknown } } } + const description = resources.checkpoints?.perWrite?.description + if (typeof description !== "string") { + throw new Error("settings.checkpoints.perWrite.description is missing") + } + return description +} + +// Mock the translation hook +vi.mock("@/i18n/TranslationContext", () => ({ + useAppTranslation: () => ({ + t: (key: string) => { + if (key === "settings:checkpoints.perWrite.label") { + return "Checkpoint after each file write" + } + if (key === "settings:checkpoints.perWrite.description") { + return "Record a checkpoint snapshot after every successful file write by the agent" + } + return key + }, + }), +})) + +// Mock the UI components (async factory: vi.importActual resolves asynchronously). +vi.mock("@/components/ui", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + Slider: ({ defaultValue, onValueChange, "data-testid": dataTestId }: any) => ( + onValueChange?.([100])} + data-testid={dataTestId} + role="slider" + /> + ), + } +}) + +// Mock vscode utilities +vi.mock("@/utils/vscode", () => ({ + vscode: { + postMessage: vi.fn(), + }, +})) + +// Mock VSCode components to behave like standard HTML elements +vi.mock("@vscode/webview-ui-toolkit/react", () => ({ + VSCodeCheckbox: ({ checked, onChange, children, ...props }: any) => ( + + ), + VSCodeLink: ({ children, ...props }: any) => {children}, +})) + +describe("CheckpointSettings", () => { + const setCachedStateField = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + }) + + it("renders the per-write checkpoints checkbox checked by default when the value is unset", () => { + render() + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + expect(checkbox).toBeChecked() + }) + + it("unchecks the per-write checkpoints checkbox when the saved value is false", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + expect(checkbox).not.toBeChecked() + }) + + it("keeps the per-write checkpoints checkbox checked when the saved value is true", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + expect(checkbox).toBeChecked() + }) + + it("caches a toggle to enable per-write checkpoints when the user checks the box", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + fireEvent.click(checkbox) + + expect(setCachedStateField).toHaveBeenCalledWith("perWriteCheckpoints", true) + }) + + it("caches a toggle to disable per-write checkpoints when the user unchecks the box", () => { + render() + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + fireEvent.click(checkbox) + + expect(setCachedStateField).toHaveBeenCalledWith("perWriteCheckpoints", false) + }) + + it.each(Object.entries(localeSettings).map(([locale, settings]) => ({ locale, settings })))( + "names the three writing tools in the $locale per-write checkpoint description", + ({ locale, settings }) => { + const description = perWriteDescription(settings) + // The tool identifiers are language-neutral, so every locale can be held to the same + // names: the setting records a checkpoint for these tools only, and the copy has to + // say so instead of promising a snapshot after every write operation. + expect(WRITE_TOOLS.filter((toolName) => !description.includes(toolName))).toEqual([]) + if (locale !== "en") { + // The description is genuinely localized: a locale left on the English copy is a + // missing translation, not a parity match. + expect(description).not.toBe(perWriteDescription(localeSettings.en)) + } + }, + ) + + it("states that saving from the editor records no checkpoint", () => { + // The checkpoint call sites are the agent's three writing tools; an editor save does + // not go through them, so the copy must not imply that it does. + expect(perWriteDescription(localeSettings.en)).toContain("Saving a file from the editor does not record one") + }) +}) diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index d8958f87c6..710c96a23f 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -1197,3 +1197,50 @@ describe("SettingsView - promptCacheTtl save round trip", () => { expect(posted()?.apiConfiguration?.promptCacheTtl).toBe("1h") }) }) + +describe("SettingsView - per-write checkpoints save (B1)", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("saves the default-on payload when Save is clicked with the setting unset", () => { + // An unset setting shows (and saves) the shared default-on value: + // the cached state seeds DEFAULT_PER_WRITE_CHECKPOINTS, so the host + // receives an explicit boolean instead of an omitted key. + const { activateTab, getSettingsContent } = renderSettingsView() + activateTab("checkpoints") + + const content = getSettingsContent() + const perWrite = within(content).getByRole("checkbox", { name: "settings:checkpoints.perWrite.label" }) + expect(perWrite).toBeChecked() + // Toggle a sibling setting so Save is enabled without touching perWrite. + fireEvent.click(within(content).getByRole("checkbox", { name: "settings:checkpoints.enable.label" })) + fireEvent.click(screen.getByTestId("save-button")) + + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ perWriteCheckpoints: true }), + }), + ) + }) + + it("saves an explicit false payload after unchecking and clicking Save", () => { + const { activateTab, getSettingsContent } = renderSettingsView({ perWriteCheckpoints: true }) + activateTab("checkpoints") + + const content = getSettingsContent() + const perWrite = within(content).getByRole("checkbox", { name: "settings:checkpoints.perWrite.label" }) + expect(perWrite).toBeChecked() + fireEvent.click(perWrite) + expect(perWrite).not.toBeChecked() + fireEvent.click(screen.getByTestId("save-button")) + + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "updateSettings", + updatedSettings: expect.objectContaining({ perWriteCheckpoints: false }), + }), + ) + }) +}) diff --git a/webview-ui/src/context/ExtensionStateContext.tsx b/webview-ui/src/context/ExtensionStateContext.tsx index 770f051dc5..3f4d267fae 100644 --- a/webview-ui/src/context/ExtensionStateContext.tsx +++ b/webview-ui/src/context/ExtensionStateContext.tsx @@ -255,6 +255,7 @@ export const createInitialExtensionState = (): ExtensionState => ({ ttsEnabled: false, ttsSpeed: 1.0, enableCheckpoints: true, + perWriteCheckpoints: true, checkpointTimeout: DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, // Default to 15 seconds language: "en", // Default language code // Placeholder before the extension state hydrates: use the shared default diff --git a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx index 424255245a..13a65d94a5 100644 --- a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx +++ b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx @@ -883,6 +883,16 @@ describe("ExtensionStateContext", () => { }), ) }) + + it("initializes the per-write checkpoint default before hydration", () => { + // The initializer itself (not a merge fixture) must carry the per-write + // checkpoint default: a regression that dropped it from + // createInitialExtensionState would otherwise stay hidden because the + // merge tests supply the key manually. + const state = createInitialExtensionState() + + expect(state.perWriteCheckpoints).toBe(true) + }) }) describe("mergeExtensionState", () => { @@ -894,6 +904,7 @@ describe("mergeExtensionState", () => { taskHistory: [], shouldShowAnnouncement: false, enableCheckpoints: true, + perWriteCheckpoints: true, // Matches the shared DEFAULT_WRITE_DELAY_MS (pre-hydration placeholder // must not disagree with the extension's own fallback). writeDelayMs: 0, @@ -966,6 +977,7 @@ describe("mergeExtensionState", () => { taskHistory: [], shouldShowAnnouncement: false, enableCheckpoints: true, + perWriteCheckpoints: true, // Matches the shared DEFAULT_WRITE_DELAY_MS (pre-hydration placeholder // must not disagree with the extension's own fallback). writeDelayMs: 0, diff --git a/webview-ui/src/i18n/locales/ca/settings.json b/webview-ui/src/i18n/locales/ca/settings.json index 74adb11620..17dc2b3f8c 100644 --- a/webview-ui/src/i18n/locales/ca/settings.json +++ b/webview-ui/src/i18n/locales/ca/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Habilitar punts de control automàtics", "description": "Quan està habilitat, Zoo crearà automàticament punts de control durant l'execució de tasques, facilitant la revisió de canvis o la reversió a estats anteriors. <0>Més informació" + }, + "perWrite": { + "label": "Punt de control després de cada escriptura de fitxer", + "description": "Registra un punt de verificació després de cada escriptura correcta de les eines write_to_file, edit_file o apply_patch de l'agent; un pegat de diversos fitxers produeix un únic punt de verificació. Desarxivar un fitxer des de l'editor no en registra cap." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/de/settings.json b/webview-ui/src/i18n/locales/de/settings.json index 078c5c6ebf..c8ce2c102f 100644 --- a/webview-ui/src/i18n/locales/de/settings.json +++ b/webview-ui/src/i18n/locales/de/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Automatische Kontrollpunkte aktivieren", "description": "Wenn aktiviert, erstellt Zoo automatisch Kontrollpunkte während der Aufgabenausführung, was die Überprüfung von Änderungen oder die Rückkehr zu früheren Zuständen erleichtert. <0>Mehr erfahren" + }, + "perWrite": { + "label": "Kontrollpunkt nach jedem Dateischreibvorgang", + "description": "Erstellt nach jedem erfolgreichen Schreiben der Agent-Tools write_to_file, edit_file oder apply_patch einen Kontrollpunkt; ein Patch mit mehreren Dateien erzeugt einen einzigen Kontrollpunkt. Eine Datei im Editor zu speichern erzeugt keinen." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/en/settings.json b/webview-ui/src/i18n/locales/en/settings.json index 67a9f146e4..0d4e8bbbdf 100644 --- a/webview-ui/src/i18n/locales/en/settings.json +++ b/webview-ui/src/i18n/locales/en/settings.json @@ -768,6 +768,10 @@ "enable": { "label": "Enable automatic checkpoints", "description": "When enabled, Zoo will automatically create checkpoints during task execution, making it easy to review changes or revert to earlier states. <0>Learn more" + }, + "perWrite": { + "label": "Checkpoint after each file write", + "description": "Record a checkpoint after each successful write by the agent's write_to_file, edit_file or apply_patch tools; a multi-file patch produces a single checkpoint. Saving a file from the editor does not record one." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/es/settings.json b/webview-ui/src/i18n/locales/es/settings.json index 81aeb1744c..e020541aae 100644 --- a/webview-ui/src/i18n/locales/es/settings.json +++ b/webview-ui/src/i18n/locales/es/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Habilitar puntos de control automáticos", "description": "Cuando está habilitado, Zoo creará automáticamente puntos de control durante la ejecución de tareas, facilitando la revisión de cambios o la reversión a estados anteriores. <0>Más información" + }, + "perWrite": { + "label": "Punto de control después de cada escritura de archivo", + "description": "Registra un punto de control tras cada escritura correcta de las herramientas write_to_file, edit_file o apply_patch del agente; un parche de varios archivos produce un único punto de control. Guardar un archivo desde el editor no registra ninguno." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/fr/settings.json b/webview-ui/src/i18n/locales/fr/settings.json index 555b088322..6a937aa823 100644 --- a/webview-ui/src/i18n/locales/fr/settings.json +++ b/webview-ui/src/i18n/locales/fr/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Activer les points de contrôle automatiques", "description": "Lorsque cette option est activée, Zoo créera automatiquement des points de contrôle pendant l'exécution des tâches, facilitant la révision des modifications ou le retour à des états antérieurs. <0>En savoir plus" + }, + "perWrite": { + "label": "Point de contrôle après chaque écriture de fichier", + "description": "Enregistre un point de contrôle après chaque écriture réussie des outils write_to_file, edit_file ou apply_patch de l'agent ; un correctif touche plusieurs fichiers et ne produit qu'un seul point de contrôle. Enregistrer un fichier depuis l'éditeur n'en enregistre pas." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/hi/settings.json b/webview-ui/src/i18n/locales/hi/settings.json index 5c33bf5ccb..afef7deaac 100644 --- a/webview-ui/src/i18n/locales/hi/settings.json +++ b/webview-ui/src/i18n/locales/hi/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "स्वचालित चेकपॉइंट सक्षम करें", "description": "जब सक्षम होता है, तो Zoo कार्य निष्पादन के दौरान स्वचालित रूप से चेकपॉइंट बनाएगा, जिससे परिवर्तनों की समीक्षा करना या पहले की स्थितियों पर वापस जाना आसान हो जाएगा। <0>अधिक जानें" + }, + "perWrite": { + "label": "हर फ़ाइल लिखने के बाद चेकपॉइंट", + "description": "एजेंट के write_to_file, edit_file या apply_patch उपकरणों की हर सफल लिखत के बाद चेकपॉइंट दर्ज करें; कई फ़ाइलों वाला पैच केवल एक ही चेकपॉइंट बनाता है। संपादक से फ़ाइल सहेजने पर चेकपॉइंट दर्ज नहीं होता।" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/id/settings.json b/webview-ui/src/i18n/locales/id/settings.json index 62d91e4d06..c949b4704e 100644 --- a/webview-ui/src/i18n/locales/id/settings.json +++ b/webview-ui/src/i18n/locales/id/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Aktifkan checkpoint otomatis", "description": "Ketika diaktifkan, Zoo akan secara otomatis membuat checkpoint selama eksekusi tugas, memudahkan untuk meninjau perubahan atau kembali ke state sebelumnya. <0>Pelajari lebih lanjut" + }, + "perWrite": { + "label": "Checkpoint setelah setiap penulisan file", + "description": "Rekam checkpoint setelah setiap penulisan berhasil oleh perkakas write_to_file, edit_file, atau apply_patch agen; patch multi-berkas menghasilkan satu checkpoint. Menyimpan berkas dari editor tidak merekam checkpoint." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/it/settings.json b/webview-ui/src/i18n/locales/it/settings.json index eb090e3f47..9789d14064 100644 --- a/webview-ui/src/i18n/locales/it/settings.json +++ b/webview-ui/src/i18n/locales/it/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Abilita punti di controllo automatici", "description": "Quando abilitato, Zoo creerà automaticamente punti di controllo durante l'esecuzione dei compiti, facilitando la revisione delle modifiche o il ritorno a stati precedenti. <0>Scopri di più" + }, + "perWrite": { + "label": "Punto di controllo dopo ogni scrittura del file", + "description": "Registra un checkpoint dopo ogni scrittura riuscita degli strumenti write_to_file, edit_file o apply_patch dell'agente; una patch con più file produce un solo checkpoint. Salvare un file dall'editor non ne registra uno." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ja/settings.json b/webview-ui/src/i18n/locales/ja/settings.json index a7939d2e3d..4778f788d7 100644 --- a/webview-ui/src/i18n/locales/ja/settings.json +++ b/webview-ui/src/i18n/locales/ja/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "自動チェックポイントを有効化", "description": "有効にすると、Zooはタスク実行中に自動的にチェックポイントを作成し、変更の確認や以前の状態への復帰を容易にします。 <0>詳細情報" + }, + "perWrite": { + "label": "ファイルの書き込みごとにチェックポイント", + "description": "エージェントの write_to_file、edit_file、apply_patch ツールによる各書き込み成功後にチェックポイントを記録します。複数ファイルのパッチは 1 件のチェックポイントになります。エディタからファイルを保存してもチェックポイントは記録されません。" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ko/settings.json b/webview-ui/src/i18n/locales/ko/settings.json index 572141342a..50e299a0b1 100644 --- a/webview-ui/src/i18n/locales/ko/settings.json +++ b/webview-ui/src/i18n/locales/ko/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "자동 체크포인트 활성화", "description": "활성화되면 Zoo는 작업 실행 중에 자동으로 체크포인트를 생성하여 변경 사항을 검토하거나 이전 상태로 되돌리기 쉽게 합니다. <0>더 알아보기" + }, + "perWrite": { + "label": "파일을 쓸 때마다 체크포인트", + "description": "에이전트의 write_to_file, edit_file, apply_patch 도구가 파일을 성공적으로 쓸 때마다 체크포인트를 기록합니다. 여러 파일의 패치는 체크포인트 한 개만 만듭니다. 에디터에서 파일을 저장해도 체크포인트가 기록되지 않습니다." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/nl/settings.json b/webview-ui/src/i18n/locales/nl/settings.json index 2103bfd08c..8cb93f12ad 100644 --- a/webview-ui/src/i18n/locales/nl/settings.json +++ b/webview-ui/src/i18n/locales/nl/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Automatische checkpoints inschakelen", "description": "Indien ingeschakeld, maakt Zoo automatisch checkpoints tijdens het uitvoeren van taken, zodat je eenvoudig wijzigingen kunt bekijken of terugzetten. <0>Meer informatie" + }, + "perWrite": { + "label": "Checkpoint na elke bestandsschrijving", + "description": "Leg na elke succesvolle schrijfactie van de write_to_file-, edit_file- of apply_patch-tool van de agent een checkpoint vast; een patch met meerdere bestanden levert één checkpoint op. Een bestand opslaan vanuit de editor doet dat niet." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/pl/settings.json b/webview-ui/src/i18n/locales/pl/settings.json index 7a0ea345ec..986cf5baff 100644 --- a/webview-ui/src/i18n/locales/pl/settings.json +++ b/webview-ui/src/i18n/locales/pl/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Włącz automatyczne punkty kontrolne", "description": "Gdy włączone, Zoo automatycznie utworzy punkty kontrolne podczas wykonywania zadań, ułatwiając przeglądanie zmian lub powrót do wcześniejszych stanów. <0>Dowiedz się więcej" + }, + "perWrite": { + "label": "Punkt kontrolny po każdym zapisaniu pliku", + "description": "Zapisz punkt kontrolny po każdym pomyślnym zapisie pliku narzędziami agenta write_to_file, edit_file lub apply_patch; łata obejmująca wiele plików tworzy jeden punkt kontrolny. Zapisanie pliku w edytorze nie tworzy punktu kontrolnego." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/pt-BR/settings.json b/webview-ui/src/i18n/locales/pt-BR/settings.json index 3c755f7ee2..9744536935 100644 --- a/webview-ui/src/i18n/locales/pt-BR/settings.json +++ b/webview-ui/src/i18n/locales/pt-BR/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Ativar pontos de verificação automáticos", "description": "Quando ativado, o Zoo criará automaticamente pontos de verificação durante a execução de tarefas, facilitando a revisão de alterações ou o retorno a estados anteriores. <0>Saiba mais" + }, + "perWrite": { + "label": "Ponto de verificação após cada gravação de arquivo", + "description": "Registra um checkpoint após cada gravação bem-sucedida das ferramentas write_to_file, edit_file ou apply_patch do agente; um patch com vários arquivos produz um único checkpoint. Salvar um arquivo pelo editor não registra nenhum." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ru/settings.json b/webview-ui/src/i18n/locales/ru/settings.json index 69486d9e72..8ab19d3ef0 100644 --- a/webview-ui/src/i18n/locales/ru/settings.json +++ b/webview-ui/src/i18n/locales/ru/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Включить автоматические контрольные точки", "description": "Если включено, Zoo будет автоматически создавать контрольные точки во время выполнения задач, что упрощает просмотр изменений или возврат к предыдущим состояниям. <0>Подробнее" + }, + "perWrite": { + "label": "Контрольная точка после каждой записи файла", + "description": "Записывает контрольную точку после каждого успешной записи инструментами агента write_to_file, edit_file или apply_patch; патч для нескольких файлов создаёт одну контрольную точку. Сохранение файла в редакторе её не создаёт." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/tr/settings.json b/webview-ui/src/i18n/locales/tr/settings.json index 760990b0c2..4476e661d5 100644 --- a/webview-ui/src/i18n/locales/tr/settings.json +++ b/webview-ui/src/i18n/locales/tr/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Otomatik kontrol noktalarını etkinleştir", "description": "Etkinleştirildiğinde, Zoo görev yürütme sırasında otomatik olarak kontrol noktaları oluşturarak değişiklikleri gözden geçirmeyi veya önceki durumlara dönmeyi kolaylaştırır. <0>Daha fazla bilgi" + }, + "perWrite": { + "label": "Her dosya yazımından sonra kontrol noktası", + "description": "Aracın write_to_file, edit_file veya apply_patch araçlarıyla yapılan her başarılı dosya yazmasından sonra bir kontrol noktası kaydedin; çok dosyalı bir yama tek bir kontrol noktası üretir. Bir dosyayı düzenleyiciden kaydetmek kontrol noktası kaydetmez." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/vi/settings.json b/webview-ui/src/i18n/locales/vi/settings.json index 07c2596d43..33c34c979f 100644 --- a/webview-ui/src/i18n/locales/vi/settings.json +++ b/webview-ui/src/i18n/locales/vi/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "Bật điểm kiểm tra tự động", "description": "Khi được bật, Zoo sẽ tự động tạo các điểm kiểm tra trong quá trình thực hiện nhiệm vụ, giúp dễ dàng xem lại các thay đổi hoặc quay lại trạng thái trước đó. <0>Tìm hiểu thêm" + }, + "perWrite": { + "label": "Điểm kiểm tra sau mỗi lần ghi file", + "description": "Ghi điểm kiểm tra sau mỗi lần các công cụ write_to_file, edit_file hoặc apply_patch của tác nhân ghi tệp thành công; một patch nhiều tệp chỉ tạo một điểm kiểm tra duy nhất. Lưu tệp từ trình soạn thảo không ghi điểm kiểm tra." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/zh-CN/settings.json b/webview-ui/src/i18n/locales/zh-CN/settings.json index 12fd5afe63..b0d6e104cf 100644 --- a/webview-ui/src/i18n/locales/zh-CN/settings.json +++ b/webview-ui/src/i18n/locales/zh-CN/settings.json @@ -695,6 +695,10 @@ "enable": { "label": "启用自动存档点", "description": "开启后自动创建任务存档点，方便回溯修改。 <0>了解更多" + }, + "perWrite": { + "label": "每次文件写入后创建存档点", + "description": "在代理的 write_to_file、edit_file 或 apply_patch 工具每次成功写入后记录一个检查点；涉及多个文件的补丁只产生一个检查点。在编辑器中保存文件不会记录检查点。" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/zh-TW/settings.json b/webview-ui/src/i18n/locales/zh-TW/settings.json index 6760aa6458..715aea2e3d 100644 --- a/webview-ui/src/i18n/locales/zh-TW/settings.json +++ b/webview-ui/src/i18n/locales/zh-TW/settings.json @@ -722,6 +722,10 @@ "enable": { "label": "啟用自動檢查點", "description": "啟用後，Zoo 將在工作執行期間自動建立檢查點，方便檢視變更或回到較早的狀態。 <0>了解更多" + }, + "perWrite": { + "label": "每次檔案寫入後建立檢查點", + "description": "在代理的 write_to_file、edit_file 或 apply_patch 工具每次成功寫入後記錄一個檢查點；涉及多個檔案的補丁只產生一個檢查點。在編輯器中儲存檔案不會記錄檢查點。" } }, "notifications": {