diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index eb51c1d67d..067774e950 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -114,6 +114,13 @@ export const MAX_CHECKPOINT_TIMEOUT_SECONDS = 60 */ export const DEFAULT_CHECKPOINT_TIMEOUT_SECONDS = 15 +/** + * Whether per-write checkpoints and task-start baseline are enabled by default. + * Master switch for the B cluster of checkpoint features. + * @default true + */ +export const DEFAULT_PER_WRITE_CHECKPOINTS = true + /** * GlobalSettings */ @@ -223,6 +230,12 @@ export const globalSettingsSchema = z.object({ .min(MIN_CHECKPOINT_TIMEOUT_SECONDS) .max(MAX_CHECKPOINT_TIMEOUT_SECONDS) .optional(), + /** + * Whether to record a shadow-git checkpoint after every successful write_to_file, + * edit_file, and apply_patch (per-write checkpoints), plus a task-start baseline. + * @default true + */ + perWriteCheckpoints: z.boolean().optional(), ttsEnabled: z.boolean().optional(), ttsSpeed: z.number().optional(), diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index a5393ddc66..07baaf9ce8 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -364,6 +364,7 @@ export type ExtensionState = Pick< enableCheckpoints: boolean checkpointTimeout: number // Timeout for checkpoint initialization in seconds (default: 15) + perWriteCheckpoints: boolean maxOpenTabsContext: number // Maximum number of VSCode open tabs to include in context (0-500) maxWorkspaceFiles: number // Maximum number of files to include in current working directory details (0-500) showRooIgnoredFiles: boolean // Whether to show .rooignore'd files in listings diff --git a/src/core/checkpoints/__tests__/checkpoint.test.ts b/src/core/checkpoints/__tests__/checkpoint.test.ts index 9b2fc1d1aa..9150ca9c21 100644 --- a/src/core/checkpoints/__tests__/checkpoint.test.ts +++ b/src/core/checkpoints/__tests__/checkpoint.test.ts @@ -113,6 +113,34 @@ 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("should wait for checkpoint service initialization before saving", async () => { // Set up task with uninitialized service mockCheckpointService.isInitialized = false 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 de424b03f7..9a1ad89517 100644 --- a/src/core/checkpoints/index.ts +++ b/src/core/checkpoints/index.ts @@ -113,6 +113,15 @@ export async function getCheckpointService(task: Task, { interval = 250 }: { int const service = RepoPerTaskCheckpointService.create(options) task.checkpointServiceInitializing = true await checkGitInstallation(task, service, log, provider) + if (!service.isInitialized) { + // checkGitInstallation disables checkpoints when Git is unavailable and + // logs initShadowGit failures without throwing: storing such a service + // would hand later saves (and the fast path above) an uninitialized + // handle whose isInitialized can never flip, so a failed + // initialization must resolve to no service at all. + task.checkpointServiceInitializing = false + return undefined + } task.checkpointService = service if (task.enableCheckpoints) { sendCheckpointInitWarn(task) @@ -200,6 +209,10 @@ async function checkGitInstallation( } catch (err) { log(`[Task#getCheckpointService] initShadowGit -> ${err.message}`) task.enableCheckpoints = false + // The "initialize" event never fires for a failed initShadowGit, so + // clear the flag here or later lookups wait on an initialization + // that already died. + task.checkpointServiceInitializing = false } } catch (err) { log(`[Task#getCheckpointService] Unexpected error during Git check: ${err.message}`) @@ -209,6 +222,29 @@ async function checkGitInstallation( } } +// Serialize checkpoint saves per task. saveCheckpoint() runs stageAll + commit, so two +// concurrent saves for the same task (for example the task-start baseline racing the first +// per-write save after the baseline timeout) can stage each other's writes or interleave +// commits. A per-task promise queue makes every save wait for the previous one, including +// a baseline that is still running. +const checkpointSaveQueues = new Map>() + +function enqueueCheckpointSave(taskId: string, operation: () => Promise): Promise { + const previous = checkpointSaveQueues.get(taskId) ?? Promise.resolve() + const next = previous.then(operation, operation) + const tail = next.then( + () => undefined, + () => undefined, + ) + checkpointSaveQueues.set(taskId, tail) + void tail.finally(() => { + if (checkpointSaveQueues.get(taskId) === tail) { + checkpointSaveQueues.delete(taskId) + } + }) + return next +} + export async function checkpointSave(task: Task, force = false, suppressMessage = false) { const service = await getCheckpointService(task) @@ -218,13 +254,14 @@ 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 save already + // running for this task so stageAll + commit never overlap. + return enqueueCheckpointSave(task.taskId, () => + 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 + }) } export type CheckpointRestoreOptions = { diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index dc7880644c..d9bea9b048 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -468,6 +468,9 @@ export class Task extends EventEmitter implements TaskLike { public lastMessageTs?: number private autoApprovalTimeoutRef?: NodeJS.Timeout + // B1: task-start baseline, recorded at most once (initiateTaskLoop also runs on resume). + private taskStartBaselineDone = false + // Tool Use consecutiveMistakeCount: number = 0 consecutiveMistakeLimit: number @@ -3833,6 +3836,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 @@ -4267,6 +4336,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, ) @@ -4320,6 +4390,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 { @@ -4337,6 +4408,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( @@ -4346,6 +4418,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. @@ -4743,6 +4816,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) @@ -4760,12 +4834,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) @@ -6445,6 +6519,14 @@ export class Task extends EventEmitter implements TaskLike { * @param context - Context string for logging (e.g., the calling tool name) */ public processQueuedMessages(): void { + // An aborted task must not dequeue queued messages: submitUserMessage + // rejects once abort is set, so consuming the queue here would drop the + // message instead of leaving it for the resume/abort flow. Tools await + // checkpoint saves before handing off, so a cancellation can land between + // the write and this call. + if (this.abort) { + return + } try { if (!this.messageQueueService.isEmpty()) { const queued = this.messageQueueService.dequeueMessage() diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 3066613263..f0afa0c45a 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -6824,6 +6824,283 @@ describe("Cline", () => { }) }) + describe("task-start baseline (B1 perWriteCheckpoints)", () => { + it("records one suppressed baseline checkpoint per Task instance at loop start", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + // A committed baseline: the done flag is set only on a real + // commit, so the double must resolve one for the once-per-instance + // assertion to exercise the flag flip. + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue({ commit: "baseline-commit" }) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + // End the loop after the first request; abort stays false so the + // baseline block (not the loop) is what the save spy observes. + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).toHaveBeenCalledOnce() + expect(saveSpy).toHaveBeenCalledWith(true, true) + }) + + it("does not record a baseline when the task resumes from history", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + historyItem: { + id: "resumed-baseline-task", + number: 1, + ts: Date.now(), + task: "resumed baseline task", + tokensIn: 10, + tokensOut: 5, + totalCost: 0.001, + }, + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + // A history resume must not re-record the baseline: it belongs to the + // original task start (exactly once per task). + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("records the baseline checkpoint when the setting is unset (default-on)", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline unset task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + // Unset: the property is absent from the state, so default-on applies. + const unsetState = { ...state } + Reflect.deleteProperty(unsetState, "perWriteCheckpoints") + vi.spyOn(mockProvider, "getState").mockResolvedValue(unsetState as typeof state) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).toHaveBeenCalledOnce() + expect(saveSpy).toHaveBeenCalledWith(true, true) + }) + + it("does not record a baseline checkpoint when perWriteCheckpoints is disabled", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline disabled task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...state, perWriteCheckpoints: false }) + + vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + + await taskAccess.initiateTaskLoop([]) + + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("does not record a baseline when the task is already aborted", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "aborted baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue(state) + + task.abort = true + + await taskAccess.initiateTaskLoop([]) + + // A task cancelled before the loop starts must not pay for (or await) + // a baseline checkpoint it will never use. + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("does not record a baseline when cancellation arrives during the state lookup", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "racy baseline task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined) + const state = await mockProvider.getState() + type StateResult = Awaited> + let resolveState: (value: StateResult | PromiseLike) => void = () => {} + const statePromise = new Promise((resolve) => (resolveState = resolve)) + vi.spyOn(mockProvider, "getState").mockReturnValue(statePromise) + + const loopPromise = taskAccess.initiateTaskLoop([]) + // Cancellation lands while the state lookup is still pending. + task.abort = true + resolveState({ ...state, perWriteCheckpoints: true }) + await loopPromise + + // The post-lookup recheck must skip the save for the canceled task. + expect(saveSpy).not.toHaveBeenCalled() + }) + + it("awaits the baseline checkpoint before entering the request loop", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "baseline await task", + startTask: false, + }) + const taskAccess = getTaskTestAccess(task) + type SaveResult = Awaited> + let resolveSave: (value: SaveResult | PromiseLike) => void = () => {} + const saveSpy = vi + .spyOn(task, "checkpointSave") + .mockImplementation(() => new Promise((resolve) => (resolveSave = resolve))) + const requestSpy = vi.spyOn(task, "recursivelyMakeClineRequests").mockResolvedValue(true) + vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...(await mockProvider.getState()) }) + const loopPromise = taskAccess.initiateTaskLoop([]) + + // The loop must not enter while the baseline checkpoint is still in flight. + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(saveSpy).toHaveBeenCalledOnce() + expect(requestSpy).not.toHaveBeenCalled() + + resolveSave() + await loopPromise + expect(requestSpy).toHaveBeenCalled() + }) + + it("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("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({ @@ -7747,6 +8024,34 @@ describe("Queued message processing after condense", () => { expect(submitSpy).toHaveBeenCalledWith("queued text", ["img1.png"]) expect(task.messageQueueService.isEmpty()).toBe(true) }) + it("does not dequeue queued messages once the task is aborted", () => { + // Tools await the checkpoint before handing off queued work; a + // cancellation landing during that await must not consume the queued + // message: submitUserMessage rejects once abort is set, so dequeuing + // would drop the input instead of leaving it for the resume flow. + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } + const provider = createProvider() + const task = new Task({ + provider, + apiConfiguration: apiConfig, + task: "initial task", + startTask: false, + }) + const submitSpy = vi.spyOn(task, "submitUserMessage").mockResolvedValue(undefined) + + task.messageQueueService.addMessage("queued text") + task.abort = true + try { + task.processQueuedMessages() + } finally { + task.abort = false + } + + expect(submitSpy).not.toHaveBeenCalled() + expect(task.messageQueueService.isEmpty()).toBe(false) + }) it("does not cross-drain queues between separate tasks", async () => { const providerA = createProvider() diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index 2713e08ffe..14ad81bef7 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,7 +308,7 @@ 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 @@ -221,12 +320,14 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { // Track file edit operation await task.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) + mutated = true + mutation.committed = true task.didEditFile = true 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 +336,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 +349,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 +371,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 +381,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 +398,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 +411,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 +425,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 +475,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 +488,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 +500,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 +512,169 @@ 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, + // 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") + } + + // 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(() => {}) + } + if (destinationBackup !== undefined) { + await fs.rename(backupPath, moveAbsolutePath).catch(() => {}) + } + task.consecutiveMistakeCount++ + task.recordToolError("apply_patch") + const writeErrorMessage = `Failed to write move destination '${change.movePath}': ${writeError instanceof Error ? writeError.message : String(writeError)}` + await task.say("error", writeErrorMessage) + pushToolResult(formatResponse.toolError(writeErrorMessage)) + await task.diffViewProvider.reset() + return { ok: false, mutated } } // Delete the original file try { 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 { + await fs.writeFile(recoveryCandidate, newContent, "utf8") + 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 +684,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 +694,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..4034de3615 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,696 @@ 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("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("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. + mockedFsPromises.default.unlink.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 + expect(checkpointAt).toBeLessThan(handoffAt) + }) + 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 + expect(checkpointAt).toBeLessThan(handoffAt) + expect(mockHandleError).toHaveBeenCalled() + }) + 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 3d88df632b..84eae7ac26 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -55,6 +55,7 @@ import { ORGANIZATION_ALLOW_ALL, DEFAULT_MODES, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_PER_WRITE_CHECKPOINTS, getModelId, isRetiredProvider, modelIdKeys, @@ -2747,6 +2748,7 @@ export class ClineProvider ttsSpeed, enableCheckpoints, checkpointTimeout, + perWriteCheckpoints, soundVolume, writeDelayMs, diffFuzzyThreshold, @@ -2916,6 +2918,7 @@ export class ClineProvider ttsSpeed: ttsSpeed ?? 1.0, enableCheckpoints: enableCheckpoints ?? true, checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, shouldShowAnnouncement: telemetrySetting !== "unset" && lastShownAnnouncementId !== this.latestAnnouncementId, allowedCommands: mergedAllowedCommands, @@ -3157,6 +3160,7 @@ export class ClineProvider ttsSpeed: stateValues.ttsSpeed ?? 1.0, enableCheckpoints: stateValues.enableCheckpoints ?? true, checkpointTimeout: stateValues.checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: stateValues.perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, soundVolume: stateValues.soundVolume, writeDelayMs: stateValues.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS, diffFuzzyThreshold: stateValues.diffFuzzyThreshold ?? DEFAULT_DIFF_FUZZY_THRESHOLD, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 93543435e2..ec9892a844 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1209,6 +1209,7 @@ describe("ClineProvider", () => { soundEnabled: false, ttsEnabled: false, enableCheckpoints: false, + perWriteCheckpoints: false, writeDelayMs: 1000, mcpEnabled: true, mode: defaultModeSlug, @@ -2168,6 +2169,48 @@ describe("ClineProvider", () => { expect(state.destructiveCommandGuardEnabled).toBe(false) }) + test("getState returns the saved per-write checkpoints setting", async () => { + await provider.contextProxy.setValue("perWriteCheckpoints", false) + + const state = await provider.getState() + + expect(state.perWriteCheckpoints).toBe(false) + }) + + test("getState defaults per-write checkpoints to true when unset", async () => { + const state = await provider.getState() + + expect(state.perWriteCheckpoints).toBe(true) + }) + + test("getStateToPostToWebview returns the saved per-write checkpoints setting", async () => { + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("perWriteCheckpoints", true) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(true) + }) + + test("getStateToPostToWebview returns false when per-write checkpoints is saved as false", async () => { + // The default is also true, so only an explicit false proves that the + // stored value (rather than the default) reaches the webview state. + await provider.resolveWebviewView(mockWebviewView) + await provider.contextProxy.setValue("perWriteCheckpoints", false) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(false) + }) + + test("getStateToPostToWebview defaults per-write checkpoints to true when unset", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const state = await provider.getStateToPostToWebview() + + expect(state.perWriteCheckpoints).toBe(true) + }) + test("getStateToPostToWebview returns the saved blanket auto-deny setting", async () => { await provider.resolveWebviewView(mockWebviewView) await provider.contextProxy.setValue("alwaysDenyUnapprovedCommands", true) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bb3368f063..9f6ab0e987 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" @@ -1154,9 +1155,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 a740ebed5c..e8b2ad6678 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -14,7 +14,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 @@ -791,9 +799,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( @@ -811,12 +825,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() @@ -829,9 +864,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, @@ -413,6 +415,7 @@ const SettingsView = forwardRef(({ onDone, t ttsSpeed, enableCheckpoints: enableCheckpoints ?? false, checkpointTimeout: checkpointTimeout ?? DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + perWriteCheckpoints: perWriteCheckpoints ?? DEFAULT_PER_WRITE_CHECKPOINTS, writeDelayMs, diffFuzzyThreshold, terminalShellIntegrationTimeout: terminalShellIntegrationTimeout ?? 30_000, @@ -852,6 +855,7 @@ const SettingsView = forwardRef(({ onDone, t )} diff --git a/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx b/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx new file mode 100644 index 0000000000..b08c15ace7 --- /dev/null +++ b/webview-ui/src/components/settings/__tests__/CheckpointSettings.spec.tsx @@ -0,0 +1,125 @@ +// npx vitest src/components/settings/__tests__/CheckpointSettings.spec.tsx + +import { render, screen, fireEvent } from "@/utils/test-utils" +import { CheckpointSettings } from "../CheckpointSettings" + +// Mock the translation hook +vi.mock("@/i18n/TranslationContext", () => ({ + useAppTranslation: () => ({ + t: (key: string) => { + if (key === "settings:checkpoints.perWrite.label") { + return "Checkpoint after each file write" + } + if (key === "settings:checkpoints.perWrite.description") { + return "Record a checkpoint snapshot after every successful file write by the agent" + } + return key + }, + }), +})) + +// Mock the UI components (async factory: vi.importActual resolves asynchronously). +vi.mock("@/components/ui", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + Slider: ({ defaultValue, onValueChange, "data-testid": dataTestId }: any) => ( + onValueChange?.([100])} + data-testid={dataTestId} + role="slider" + /> + ), + } +}) + +// Mock vscode utilities +vi.mock("@/utils/vscode", () => ({ + vscode: { + postMessage: vi.fn(), + }, +})) + +// Mock VSCode components to behave like standard HTML elements +vi.mock("@vscode/webview-ui-toolkit/react", () => ({ + VSCodeCheckbox: ({ checked, onChange, children, ...props }: any) => ( + + ), + VSCodeLink: ({ children, ...props }: any) => {children}, +})) + +describe("CheckpointSettings", () => { + const setCachedStateField = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + }) + + it("renders the per-write checkpoints checkbox checked by default when the value is unset", () => { + render() + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + expect(checkbox).toBeChecked() + }) + + it("unchecks the per-write checkpoints checkbox when the saved value is false", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + expect(checkbox).not.toBeChecked() + }) + + it("keeps the per-write checkpoints checkbox checked when the saved value is true", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + expect(checkbox).toBeChecked() + }) + + it("caches a toggle to enable per-write checkpoints when the user checks the box", () => { + render( + , + ) + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + fireEvent.click(checkbox) + + expect(setCachedStateField).toHaveBeenCalledWith("perWriteCheckpoints", true) + }) + + it("caches a toggle to disable per-write checkpoints when the user unchecks the box", () => { + render() + + const checkbox = screen.getByRole("checkbox", { name: "Checkpoint after each file write" }) + fireEvent.click(checkbox) + + expect(setCachedStateField).toHaveBeenCalledWith("perWriteCheckpoints", false) + }) +}) diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 0835f255f9..8df0853acf 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -1049,3 +1049,50 @@ describe("SettingsView - openAiStrictToolSchemas save round trip", () => { expect(posted()?.apiConfiguration?.openAiStrictToolSchemas).toBeUndefined() }) }) + +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 c4fa00f479..c6cd635ee7 100644 --- a/webview-ui/src/context/ExtensionStateContext.tsx +++ b/webview-ui/src/context/ExtensionStateContext.tsx @@ -213,6 +213,7 @@ export const createInitialExtensionState = (): ExtensionState => ({ ttsEnabled: false, ttsSpeed: 1.0, enableCheckpoints: true, + perWriteCheckpoints: true, checkpointTimeout: DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, // Default to 15 seconds language: "en", // Default language code // Placeholder before the extension state hydrates: use the shared default diff --git a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx index dc33af38d1..9b4910ecb3 100644 --- a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx +++ b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx @@ -415,6 +415,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", () => { @@ -426,6 +436,7 @@ describe("mergeExtensionState", () => { taskHistory: [], shouldShowAnnouncement: false, enableCheckpoints: true, + perWriteCheckpoints: true, // Matches the shared DEFAULT_WRITE_DELAY_MS (pre-hydration placeholder // must not disagree with the extension's own fallback). writeDelayMs: 0, @@ -498,6 +509,7 @@ describe("mergeExtensionState", () => { taskHistory: [], shouldShowAnnouncement: false, enableCheckpoints: true, + perWriteCheckpoints: true, // Matches the shared DEFAULT_WRITE_DELAY_MS (pre-hydration placeholder // must not disagree with the extension's own fallback). writeDelayMs: 0, diff --git a/webview-ui/src/i18n/locales/ca/settings.json b/webview-ui/src/i18n/locales/ca/settings.json index d39d8edd6a..de8e5b095f 100644 --- a/webview-ui/src/i18n/locales/ca/settings.json +++ b/webview-ui/src/i18n/locales/ca/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Habilitar punts de control automàtics", "description": "Quan està habilitat, Zoo crearà automàticament punts de control durant l'execució de tasques, facilitant la revisió de canvis o la reversió a estats anteriors. <0>Més informació" + }, + "perWrite": { + "label": "Punt de control després de cada escriptura de fitxer", + "description": "Registra una instantània de punt de control després de cada operació d’escriptura reeixida; un pegat de diversos fitxers genera un únic punt de control." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/de/settings.json b/webview-ui/src/i18n/locales/de/settings.json index 983db1df5d..86bb801ee0 100644 --- a/webview-ui/src/i18n/locales/de/settings.json +++ b/webview-ui/src/i18n/locales/de/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Automatische Kontrollpunkte aktivieren", "description": "Wenn aktiviert, erstellt Zoo automatisch Kontrollpunkte während der Aufgabenausführung, was die Überprüfung von Änderungen oder die Rückkehr zu früheren Zuständen erleichtert. <0>Mehr erfahren" + }, + "perWrite": { + "label": "Kontrollpunkt nach jedem Dateischreibvorgang", + "description": "Erstellt nach jedem erfolgreichen Schreibvorgang einen Checkpoint-Schnappschuss; ein Patch mit mehreren Dateien erzeugt genau einen Checkpoint." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/en/settings.json b/webview-ui/src/i18n/locales/en/settings.json index ea924ce79e..215ba566ff 100644 --- a/webview-ui/src/i18n/locales/en/settings.json +++ b/webview-ui/src/i18n/locales/en/settings.json @@ -814,6 +814,10 @@ "enable": { "label": "Enable automatic checkpoints", "description": "When enabled, Zoo will automatically create checkpoints during task execution, making it easy to review changes or revert to earlier states. <0>Learn more" + }, + "perWrite": { + "label": "Checkpoint after each file write", + "description": "Record a checkpoint snapshot after each successful write operation; a multi-file patch produces a single checkpoint." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/es/settings.json b/webview-ui/src/i18n/locales/es/settings.json index 57612bf3e5..2d148cde55 100644 --- a/webview-ui/src/i18n/locales/es/settings.json +++ b/webview-ui/src/i18n/locales/es/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Habilitar puntos de control automáticos", "description": "Cuando está habilitado, Zoo creará automáticamente puntos de control durante la ejecución de tareas, facilitando la revisión de cambios o la reversión a estados anteriores. <0>Más información" + }, + "perWrite": { + "label": "Punto de control después de cada escritura de archivo", + "description": "Registra una instantánea de punto de control después de cada operación de escritura exitosa; un parche de varios archivos produce un único punto de control." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/fr/settings.json b/webview-ui/src/i18n/locales/fr/settings.json index a91a5137d3..287a7b861e 100644 --- a/webview-ui/src/i18n/locales/fr/settings.json +++ b/webview-ui/src/i18n/locales/fr/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Activer les points de contrôle automatiques", "description": "Lorsque cette option est activée, Zoo créera automatiquement des points de contrôle pendant l'exécution des tâches, facilitant la révision des modifications ou le retour à des états antérieurs. <0>En savoir plus" + }, + "perWrite": { + "label": "Point de contrôle après chaque écriture de fichier", + "description": "Enregistre un instantané de point de contrôle après chaque opération d’écriture réussie ; un correctif multi-fichiers produit un seul point de contrôle." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/hi/settings.json b/webview-ui/src/i18n/locales/hi/settings.json index bc6e973623..be08e00920 100644 --- a/webview-ui/src/i18n/locales/hi/settings.json +++ b/webview-ui/src/i18n/locales/hi/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "स्वचालित चेकपॉइंट सक्षम करें", "description": "जब सक्षम होता है, तो Zoo कार्य निष्पादन के दौरान स्वचालित रूप से चेकपॉइंट बनाएगा, जिससे परिवर्तनों की समीक्षा करना या पहले की स्थितियों पर वापस जाना आसान हो जाएगा। <0>अधिक जानें" + }, + "perWrite": { + "label": "हर फ़ाइल लिखने के बाद चेकपॉइंट", + "description": "प्रत्येक सफल लेखन कार्रवाई के बाद एक चेकपॉइंट स्नैपशॉट रिकॉर्ड करें; एक मल्टी-फ़ाइल पैच केवल एक चेकपॉइंट बनाता है।" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/id/settings.json b/webview-ui/src/i18n/locales/id/settings.json index 3ed57f373e..f59460b5b2 100644 --- a/webview-ui/src/i18n/locales/id/settings.json +++ b/webview-ui/src/i18n/locales/id/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Aktifkan checkpoint otomatis", "description": "Ketika diaktifkan, Zoo akan secara otomatis membuat checkpoint selama eksekusi tugas, memudahkan untuk meninjau perubahan atau kembali ke state sebelumnya. <0>Pelajari lebih lanjut" + }, + "perWrite": { + "label": "Checkpoint setelah setiap penulisan file", + "description": "Rekam snapshot titik pemeriksaan setelah setiap operasi tulis berhasil; patch multi-file menghasilkan satu titik pemeriksaan." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/it/settings.json b/webview-ui/src/i18n/locales/it/settings.json index b708304e63..3a1ef7898e 100644 --- a/webview-ui/src/i18n/locales/it/settings.json +++ b/webview-ui/src/i18n/locales/it/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Abilita punti di controllo automatici", "description": "Quando abilitato, Zoo creerà automaticamente punti di controllo durante l'esecuzione dei compiti, facilitando la revisione delle modifiche o il ritorno a stati precedenti. <0>Scopri di più" + }, + "perWrite": { + "label": "Punto di controllo dopo ogni scrittura del file", + "description": "Registra un’istantanea del punto di controllo dopo ogni operazione di scrittura riuscita; una patch con più file produce un unico punto di controllo." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ja/settings.json b/webview-ui/src/i18n/locales/ja/settings.json index f6959b50dc..286b55cf50 100644 --- a/webview-ui/src/i18n/locales/ja/settings.json +++ b/webview-ui/src/i18n/locales/ja/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "自動チェックポイントを有効化", "description": "有効にすると、Zooはタスク実行中に自動的にチェックポイントを作成し、変更の確認や以前の状態への復帰を容易にします。 <0>詳細情報" + }, + "perWrite": { + "label": "ファイルの書き込みごとにチェックポイント", + "description": "各書き込み操作の成功後にチェックポイントスナップショットを記録します。複数ファイルのパッチは単一のチェックポイントを作成します。" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ko/settings.json b/webview-ui/src/i18n/locales/ko/settings.json index 6cadbd8a3b..14226cede5 100644 --- a/webview-ui/src/i18n/locales/ko/settings.json +++ b/webview-ui/src/i18n/locales/ko/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "자동 체크포인트 활성화", "description": "활성화되면 Zoo는 작업 실행 중에 자동으로 체크포인트를 생성하여 변경 사항을 검토하거나 이전 상태로 되돌리기 쉽게 합니다. <0>더 알아보기" + }, + "perWrite": { + "label": "파일을 쓸 때마다 체크포인트", + "description": "각 쓰기 작업이 성공할 때마다 체크포인트 스냅샷을 기록합니다. 다중 파일 패치는 단일 체크포인트를 생성합니다." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/nl/settings.json b/webview-ui/src/i18n/locales/nl/settings.json index 992e9c53e0..c3d14e4416 100644 --- a/webview-ui/src/i18n/locales/nl/settings.json +++ b/webview-ui/src/i18n/locales/nl/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Automatische checkpoints inschakelen", "description": "Indien ingeschakeld, maakt Zoo automatisch checkpoints tijdens het uitvoeren van taken, zodat je eenvoudig wijzigingen kunt bekijken of terugzetten. <0>Meer informatie" + }, + "perWrite": { + "label": "Checkpoint na elke bestandsschrijving", + "description": "Leg na elke succesvolle schrijfbewerking een checkpointmomentopname vast; een patch met meerdere bestanden produceert één checkpoint." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/pl/settings.json b/webview-ui/src/i18n/locales/pl/settings.json index 459675b0e1..ac934f6b5d 100644 --- a/webview-ui/src/i18n/locales/pl/settings.json +++ b/webview-ui/src/i18n/locales/pl/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Włącz automatyczne punkty kontrolne", "description": "Gdy włączone, Zoo automatycznie utworzy punkty kontrolne podczas wykonywania zadań, ułatwiając przeglądanie zmian lub powrót do wcześniejszych stanów. <0>Dowiedz się więcej" + }, + "perWrite": { + "label": "Punkt kontrolny po każdym zapisaniu pliku", + "description": "Zapisuje migawkę punktu kontrolnego po każdej udanej operacji zapisu; patch obejmujący wiele plików tworzy jeden punkt kontrolny." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/pt-BR/settings.json b/webview-ui/src/i18n/locales/pt-BR/settings.json index 500babaaa8..9001d7c65c 100644 --- a/webview-ui/src/i18n/locales/pt-BR/settings.json +++ b/webview-ui/src/i18n/locales/pt-BR/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Ativar pontos de verificação automáticos", "description": "Quando ativado, o Zoo criará automaticamente pontos de verificação durante a execução de tarefas, facilitando a revisão de alterações ou o retorno a estados anteriores. <0>Saiba mais" + }, + "perWrite": { + "label": "Ponto de verificação após cada gravação de arquivo", + "description": "Registra um snapshot de ponto de verificação após cada operação de gravação bem-sucedida; um patch com vários arquivos produz um único ponto de verificação." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/ru/settings.json b/webview-ui/src/i18n/locales/ru/settings.json index 053ff0684d..e982e198f1 100644 --- a/webview-ui/src/i18n/locales/ru/settings.json +++ b/webview-ui/src/i18n/locales/ru/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Включить автоматические контрольные точки", "description": "Если включено, Zoo будет автоматически создавать контрольные точки во время выполнения задач, что упрощает просмотр изменений или возврат к предыдущим состояниям. <0>Подробнее" + }, + "perWrite": { + "label": "Контрольная точка после каждой записи файла", + "description": "Записывает снимок контрольной точки после каждой успешной операции записи; патч с несколькими файлами создаёт одну контрольную точку." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/tr/settings.json b/webview-ui/src/i18n/locales/tr/settings.json index b37f28d44d..9a1bc8663a 100644 --- a/webview-ui/src/i18n/locales/tr/settings.json +++ b/webview-ui/src/i18n/locales/tr/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Otomatik kontrol noktalarını etkinleştir", "description": "Etkinleştirildiğinde, Zoo görev yürütme sırasında otomatik olarak kontrol noktaları oluşturarak değişiklikleri gözden geçirmeyi veya önceki durumlara dönmeyi kolaylaştırır. <0>Daha fazla bilgi" + }, + "perWrite": { + "label": "Her dosya yazımından sonra kontrol noktası", + "description": "Her başarılı yazma işleminden sonra bir kontrol noktası anlık görüntüsü kaydeder; çok dosyalı bir yama tek bir kontrol noktası üretir." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/vi/settings.json b/webview-ui/src/i18n/locales/vi/settings.json index 94843217ae..3c3161526b 100644 --- a/webview-ui/src/i18n/locales/vi/settings.json +++ b/webview-ui/src/i18n/locales/vi/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "Bật điểm kiểm tra tự động", "description": "Khi được bật, Zoo sẽ tự động tạo các điểm kiểm tra trong quá trình thực hiện nhiệm vụ, giúp dễ dàng xem lại các thay đổi hoặc quay lại trạng thái trước đó. <0>Tìm hiểu thêm" + }, + "perWrite": { + "label": "Điểm kiểm tra sau mỗi lần ghi file", + "description": "Ghi ảnh chụp điểm kiểm tra sau mỗi thao tác ghi thành công; một vá nhiều tệp chỉ tạo một điểm kiểm tra." } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/zh-CN/settings.json b/webview-ui/src/i18n/locales/zh-CN/settings.json index 8be0fe1532..66cd5c3bd3 100644 --- a/webview-ui/src/i18n/locales/zh-CN/settings.json +++ b/webview-ui/src/i18n/locales/zh-CN/settings.json @@ -734,6 +734,10 @@ "enable": { "label": "启用自动存档点", "description": "开启后自动创建任务存档点,方便回溯修改。 <0>了解更多" + }, + "perWrite": { + "label": "每次文件写入后创建存档点", + "description": "在每次成功写入操作后记录检查点快照;多文件补丁只会生成一个检查点。" } }, "notifications": { diff --git a/webview-ui/src/i18n/locales/zh-TW/settings.json b/webview-ui/src/i18n/locales/zh-TW/settings.json index 65eb5c67a5..c6510db2e1 100644 --- a/webview-ui/src/i18n/locales/zh-TW/settings.json +++ b/webview-ui/src/i18n/locales/zh-TW/settings.json @@ -761,6 +761,10 @@ "enable": { "label": "啟用自動檢查點", "description": "啟用後,Zoo 將在工作執行期間自動建立檢查點,方便檢視變更或回到較早的狀態。 <0>了解更多" + }, + "perWrite": { + "label": "每次檔案寫入後建立檢查點", + "description": "在每次成功的寫入操作後記錄檢查點快照;多檔案修補程式只會產生一個檢查點。" } }, "notifications": {