Skip to content

Commit 976f7e6

Browse files
committed
feat(checkpoints): per-write checkpoints, task-start baseline, and perWriteCheckpoints setting (B1, #1375)
1 parent 78c712a commit 976f7e6

35 files changed

Lines changed: 823 additions & 19 deletions

packages/types/src/global-settings.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,13 @@ export const MAX_CHECKPOINT_TIMEOUT_SECONDS = 60
9999
*/
100100
export const DEFAULT_CHECKPOINT_TIMEOUT_SECONDS = 15
101101

102+
/**
103+
* Whether per-write checkpoints and task-start baseline are enabled by default.
104+
* Master switch for the B cluster of checkpoint features.
105+
* @default true
106+
*/
107+
export const DEFAULT_PER_WRITE_CHECKPOINTS = true
108+
102109
/**
103110
* GlobalSettings
104111
*/
@@ -200,6 +207,12 @@ export const globalSettingsSchema = z.object({
200207
.min(MIN_CHECKPOINT_TIMEOUT_SECONDS)
201208
.max(MAX_CHECKPOINT_TIMEOUT_SECONDS)
202209
.optional(),
210+
/**
211+
* Whether to record a shadow-git checkpoint after every successful write_to_file,
212+
* edit_file, and apply_patch (per-write checkpoints), plus a task-start baseline.
213+
* @default true
214+
*/
215+
perWriteCheckpoints: z.boolean().optional(),
203216

204217
ttsEnabled: z.boolean().optional(),
205218
ttsSpeed: z.number().optional(),

packages/types/src/vscode-extension-host.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,7 @@ export type ExtensionState = Pick<
348348

349349
enableCheckpoints: boolean
350350
checkpointTimeout: number // Timeout for checkpoint initialization in seconds (default: 15)
351+
perWriteCheckpoints: boolean
351352
maxOpenTabsContext: number // Maximum number of VSCode open tabs to include in context (0-500)
352353
maxWorkspaceFiles: number // Maximum number of files to include in current working directory details (0-500)
353354
showRooIgnoredFiles: boolean // Whether to show .rooignore'd files in listings

src/core/task/Task.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
314314
public lastMessageTs?: number
315315
private autoApprovalTimeoutRef?: NodeJS.Timeout
316316

317+
// B1: task-start baseline checkpoint recorded at most once per Task instance
318+
// (initiateTaskLoop also runs on resume-from-history, hence the guard).
319+
private taskStartBaselineDone = false
320+
317321
// Tool Use
318322
consecutiveMistakeCount: number = 0
319323
consecutiveMistakeLimit: number
@@ -2492,6 +2496,17 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
24922496
// arm needed.
24932497
void getCheckpointService(this)
24942498

2499+
// B1 task-start baseline: record one suppressed checkpoint per Task instance
2500+
// so the shadow repo has a clean pre-task root. Gated by the live
2501+
// perWriteCheckpoints setting (default-on: only skip when explicitly false).
2502+
if (!this.taskStartBaselineDone) {
2503+
this.taskStartBaselineDone = true
2504+
const baselineEnabled = (await this.providerRef.deref()?.getState())?.perWriteCheckpoints
2505+
if (baselineEnabled !== false) {
2506+
void this.checkpointSave(false, true)
2507+
}
2508+
}
2509+
24952510
let nextUserContent = userContent
24962511
let includeFileDetails = true
24972512

src/core/task/__tests__/Task.spec.ts

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3289,6 +3289,73 @@ describe("Cline", () => {
32893289
})
32903290
})
32913291

3292+
describe("task-start baseline (B1 perWriteCheckpoints)", () => {
3293+
it("records one suppressed baseline checkpoint per Task instance at loop start", async () => {
3294+
const task = new Task({
3295+
provider: mockProvider,
3296+
apiConfiguration: mockApiConfig,
3297+
task: "baseline task",
3298+
startTask: false,
3299+
})
3300+
const taskAccess = getTaskTestAccess(task)
3301+
const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined)
3302+
const state = await mockProvider.getState()
3303+
vi.spyOn(mockProvider, "getState").mockResolvedValue(state)
3304+
3305+
// Abort before the loop so only the pre-loop setup runs.
3306+
task.abort = true
3307+
3308+
await taskAccess.initiateTaskLoop([])
3309+
await taskAccess.initiateTaskLoop([])
3310+
3311+
expect(saveSpy).toHaveBeenCalledOnce()
3312+
expect(saveSpy).toHaveBeenCalledWith(false, true)
3313+
})
3314+
3315+
it("records the baseline checkpoint when the setting is unset (default-on)", async () => {
3316+
const task = new Task({
3317+
provider: mockProvider,
3318+
apiConfiguration: mockApiConfig,
3319+
task: "baseline unset task",
3320+
startTask: false,
3321+
})
3322+
const taskAccess = getTaskTestAccess(task)
3323+
const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined)
3324+
const state = await mockProvider.getState()
3325+
// Unset: the property is absent from the state object, so only the
3326+
// default-on semantics (skip only when explicitly false) apply.
3327+
const unsetState = { ...state }
3328+
Reflect.deleteProperty(unsetState, "perWriteCheckpoints")
3329+
vi.spyOn(mockProvider, "getState").mockResolvedValue(unsetState as typeof state)
3330+
3331+
task.abort = true
3332+
3333+
await taskAccess.initiateTaskLoop([])
3334+
3335+
expect(saveSpy).toHaveBeenCalledOnce()
3336+
expect(saveSpy).toHaveBeenCalledWith(false, true)
3337+
})
3338+
3339+
it("does not record a baseline checkpoint when perWriteCheckpoints is disabled", async () => {
3340+
const task = new Task({
3341+
provider: mockProvider,
3342+
apiConfiguration: mockApiConfig,
3343+
task: "baseline disabled task",
3344+
startTask: false,
3345+
})
3346+
const taskAccess = getTaskTestAccess(task)
3347+
const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined)
3348+
const state = await mockProvider.getState()
3349+
vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...state, perWriteCheckpoints: false })
3350+
3351+
task.abort = true
3352+
3353+
await taskAccess.initiateTaskLoop([])
3354+
3355+
expect(saveSpy).not.toHaveBeenCalled()
3356+
})
3357+
})
3358+
32923359
describe("start()", () => {
32933360
it("should be a no-op if the task was already started in the constructor", () => {
32943361
const task = new Task({

src/core/tools/ApplyPatchTool.ts

Lines changed: 49 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
66
import { getReadablePath } from "../../utils/path"
77
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
88
import { Task } from "../task/Task"
9+
import { checkpointSave } from "../checkpoints"
910
import { formatResponse } from "../prompts/responses"
1011
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
1112
import { fileExistsAtPath } from "../../utils/fs"
@@ -102,7 +103,10 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
102103
return
103104
}
104105

105-
// Process each file change
106+
// Process each file change. The handlers report whether their file
107+
// operation succeeded, so a rejected approval or a failed local write
108+
// does not get checkpointed as if the patch had succeeded.
109+
let patchSucceeded = true
106110
for (const change of changes) {
107111
const relPath = change.path
108112
const absolutePath = path.resolve(task.cwd, relPath)
@@ -120,17 +124,39 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
120124

121125
if (change.type === "add") {
122126
// Create new file
123-
await this.handleAddFile(change, absolutePath, relPath, task, callbacks, isWriteProtected)
127+
patchSucceeded =
128+
(await this.handleAddFile(change, absolutePath, relPath, task, callbacks, isWriteProtected)) &&
129+
patchSucceeded
124130
} else if (change.type === "delete") {
125131
// Delete file
126-
await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected)
132+
patchSucceeded =
133+
(await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected)) &&
134+
patchSucceeded
127135
} else if (change.type === "update") {
128136
// Update file
129-
await this.handleUpdateFile(change, absolutePath, relPath, task, callbacks, isWriteProtected)
137+
patchSucceeded =
138+
(await this.handleUpdateFile(
139+
change,
140+
absolutePath,
141+
relPath,
142+
task,
143+
callbacks,
144+
isWriteProtected,
145+
)) && patchSucceeded
130146
}
131147
}
132148

133149
task.consecutiveMistakeCount = 0
150+
151+
// B1: one checkpoint for the whole patch (not per file), and only when
152+
// every file operation succeeded. Live setting with default-on
153+
// semantics: skip only when explicitly false.
154+
if (patchSucceeded) {
155+
const perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints
156+
if (perWriteCheckpoints !== false) {
157+
void checkpointSave(task, false, true).catch(() => {})
158+
}
159+
}
134160
} catch (error) {
135161
await handleError("apply patch", error as Error)
136162
await task.diffViewProvider.reset()
@@ -144,7 +170,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
144170
task: Task,
145171
callbacks: ToolCallbacks,
146172
isWriteProtected: boolean,
147-
): Promise<void> {
173+
): Promise<boolean> {
148174
const { askApproval, pushToolResult } = callbacks
149175

150176
// Check if file already exists
@@ -155,7 +181,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
155181
const errorMessage = `File already exists: ${relPath}. Use Update File instead.`
156182
await task.say("error", errorMessage)
157183
pushToolResult(formatResponse.toolError(errorMessage))
158-
return
184+
return false
159185
}
160186

161187
const newContent = change.newContent || ""
@@ -209,7 +235,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
209235
}
210236
pushToolResult("Changes were rejected by the user.")
211237
await task.diffViewProvider.reset()
212-
return
238+
return false
213239
}
214240

215241
// Save the changes
@@ -227,6 +253,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
227253
pushToolResult(message)
228254
await task.diffViewProvider.reset()
229255
task.processQueuedMessages()
256+
return true
230257
}
231258

232259
private async handleDeleteFile(
@@ -235,7 +262,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
235262
task: Task,
236263
callbacks: ToolCallbacks,
237264
isWriteProtected: boolean,
238-
): Promise<void> {
265+
): Promise<boolean> {
239266
const { askApproval, pushToolResult } = callbacks
240267

241268
// Check if file exists
@@ -246,7 +273,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
246273
const errorMessage = `File not found: ${relPath}. Cannot delete a non-existent file.`
247274
await task.say("error", errorMessage)
248275
pushToolResult(formatResponse.toolError(errorMessage))
249-
return
276+
return false
250277
}
251278

252279
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
@@ -268,7 +295,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
268295

269296
if (!didApprove) {
270297
pushToolResult("Delete operation was rejected by the user.")
271-
return
298+
return false
272299
}
273300

274301
// Delete the file
@@ -278,12 +305,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
278305
const errorMessage = `Failed to delete file '${relPath}': ${error instanceof Error ? error.message : String(error)}`
279306
await task.say("error", errorMessage)
280307
pushToolResult(formatResponse.toolError(errorMessage))
281-
return
308+
return false
282309
}
283310

284311
task.didEditFile = true
285312
pushToolResult(`Successfully deleted ${relPath}`)
286313
task.processQueuedMessages()
314+
return true
287315
}
288316

289317
private async handleUpdateFile(
@@ -293,7 +321,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
293321
task: Task,
294322
callbacks: ToolCallbacks,
295323
isWriteProtected: boolean,
296-
): Promise<void> {
324+
): Promise<boolean> {
297325
const { askApproval, pushToolResult } = callbacks
298326

299327
// Check if file exists
@@ -304,7 +332,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
304332
const errorMessage = `File not found: ${relPath}. Cannot update a non-existent file.`
305333
await task.say("error", errorMessage)
306334
pushToolResult(formatResponse.toolError(errorMessage))
307-
return
335+
return false
308336
}
309337

310338
const originalContent = change.originalContent || ""
@@ -318,9 +346,11 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
318346
// Generate and validate diff
319347
const diff = formatResponse.createPrettyPatch(relPath, originalContent, newContent)
320348
if (!diff) {
349+
// A no-op change is not a failure: the patch processed cleanly and
350+
// nothing was written, so the whole-patch success state is kept.
321351
pushToolResult(`No changes needed for '${relPath}'`)
322352
await task.diffViewProvider.reset()
323-
return
353+
return true
324354
}
325355

326356
// Check experiment settings
@@ -366,7 +396,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
366396
}
367397
pushToolResult("Changes were rejected by the user.")
368398
await task.diffViewProvider.reset()
369-
return
399+
return false
370400
}
371401

372402
// Handle file move if specified
@@ -379,7 +409,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
379409
await task.say("rooignore_error", change.movePath)
380410
pushToolResult(formatResponse.rooIgnoreError(change.movePath))
381411
await task.diffViewProvider.reset()
382-
return
412+
return false
383413
}
384414

385415
// Check if destination path is write-protected
@@ -391,7 +421,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
391421
await task.say("error", errorMessage)
392422
pushToolResult(formatResponse.toolError(errorMessage))
393423
await task.diffViewProvider.reset()
394-
return
424+
return false
395425
}
396426

397427
// Check if destination path is outside workspace
@@ -403,7 +433,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
403433
await task.say("error", errorMessage)
404434
pushToolResult(formatResponse.toolError(errorMessage))
405435
await task.diffViewProvider.reset()
406-
return
436+
return false
407437
}
408438

409439
// Save new content to the new path
@@ -447,6 +477,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
447477
pushToolResult(message)
448478
await task.diffViewProvider.reset()
449479
task.processQueuedMessages()
480+
return true
450481
}
451482

452483
override async handlePartial(task: Task, block: ToolUse<"apply_patch">): Promise<void> {

src/core/tools/EditFileTool.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
1111
import { fileExistsAtPath } from "../../utils/fs"
1212
import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
1313
import { sanitizeUnifiedDiff, computeDiffStats } from "../diff/stats"
14+
import { checkpointSave } from "../../core/checkpoints"
1415
import type { ToolUse } from "../../shared/tools"
1516

1617
import { BaseTool, ToolCallbacks } from "./BaseTool"
@@ -392,6 +393,7 @@ export class EditFileTool extends BaseTool<"edit_file"> {
392393
const state = await provider?.getState()
393394
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
394395
const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS
396+
const perWriteCheckpoints = state?.perWriteCheckpoints ?? true
395397
const isPreventFocusDisruptionEnabled = experiments.isEnabled(
396398
state?.experiments ?? {},
397399
EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION,
@@ -463,6 +465,10 @@ export class EditFileTool extends BaseTool<"edit_file"> {
463465

464466
pushToolResult(message + replacementInfo)
465467

468+
if (perWriteCheckpoints) {
469+
void checkpointSave(task, false, true).catch(() => {})
470+
}
471+
466472
await task.diffViewProvider.reset()
467473
this.resetPartialState()
468474

0 commit comments

Comments
 (0)