Skip to content

Commit 9e7119e

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

35 files changed

Lines changed: 480 additions & 1 deletion

‎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: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3289,6 +3289,49 @@ 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("does not record a baseline checkpoint when perWriteCheckpoints is disabled", async () => {
3316+
const task = new Task({
3317+
provider: mockProvider,
3318+
apiConfiguration: mockApiConfig,
3319+
task: "baseline disabled 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+
vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...state, perWriteCheckpoints: false })
3326+
3327+
task.abort = true
3328+
3329+
await taskAccess.initiateTaskLoop([])
3330+
3331+
expect(saveSpy).not.toHaveBeenCalled()
3332+
})
3333+
})
3334+
32923335
describe("start()", () => {
32933336
it("should be a no-op if the task was already started in the constructor", () => {
32943337
const task = new Task({

‎src/core/tools/ApplyPatchTool.ts‎

Lines changed: 8 additions & 0 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"
@@ -131,6 +132,13 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
131132
}
132133

133134
task.consecutiveMistakeCount = 0
135+
136+
// B1: one checkpoint for the whole patch (not per file). Live setting
137+
// with default-on semantics: skip only when explicitly false.
138+
const perWriteCheckpoints = (await task.providerRef?.deref()?.getState())?.perWriteCheckpoints
139+
if (perWriteCheckpoints !== false) {
140+
void checkpointSave(task, false, true).catch(() => {})
141+
}
134142
} catch (error) {
135143
await handleError("apply patch", error as Error)
136144
await task.diffViewProvider.reset()

‎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

‎src/core/tools/WriteToFileTool.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { isPathOutsideWorkspace } from "../../utils/pathUtils"
1414
import { unescapeHtmlEntities } from "../../utils/text-normalization"
1515
import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
1616
import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats"
17+
import { checkpointSave } from "../checkpoints"
1718
import type { ToolUse } from "../../shared/tools"
1819

1920
import { BaseTool, ToolCallbacks } from "./BaseTool"
@@ -103,6 +104,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
103104
const state = await provider?.getState()
104105
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
105106
const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS
107+
const perWriteCheckpoints = state?.perWriteCheckpoints ?? true
106108
const isPreventFocusDisruptionEnabled = experiments.isEnabled(
107109
state?.experiments ?? {},
108110
EXPERIMENT_IDS.PREVENT_FOCUS_DISRUPTION,
@@ -179,6 +181,10 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
179181

180182
pushToolResult(message)
181183

184+
if (perWriteCheckpoints) {
185+
void checkpointSave(task, false, true).catch(() => {})
186+
}
187+
182188
await task.diffViewProvider.reset()
183189
this.resetPartialState()
184190

‎src/core/tools/__tests__/applyPatchTool.execute.spec.ts‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import type { MockedFunction } from "vitest"
55
import { fileExistsAtPath } from "../../../utils/fs"
66
import { isPathOutsideWorkspace } from "../../../utils/pathUtils"
77
import type { Task } from "../../task/Task"
8+
import { checkpointSave } from "../../checkpoints"
89
import { ApplyPatchTool } from "../ApplyPatchTool"
910

1011
vi.mock("fs/promises", () => ({
@@ -22,6 +23,10 @@ vi.mock("../../../utils/pathUtils", () => ({
2223
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
2324
}))
2425

26+
vi.mock("../../checkpoints", () => ({
27+
checkpointSave: vi.fn().mockResolvedValue(undefined),
28+
}))
29+
2530
describe("ApplyPatchTool.execute - delete file success path", () => {
2631
const mockedFileExistsAtPath = fileExistsAtPath as MockedFunction<typeof fileExistsAtPath>
2732
const mockedIsPathOutsideWorkspace = isPathOutsideWorkspace as MockedFunction<typeof isPathOutsideWorkspace>
@@ -38,6 +43,7 @@ describe("ApplyPatchTool.execute - delete file success path", () => {
3843
| "say"
3944
| "processQueuedMessages"
4045
| "didEditFile"
46+
| "providerRef"
4147
>
4248
let mockAskApproval: MockedFunction<(...args: unknown[]) => Promise<boolean>>
4349
let mockHandleError: MockedFunction<(...args: unknown[]) => Promise<void>>
@@ -52,6 +58,11 @@ describe("ApplyPatchTool.execute - delete file success path", () => {
5258
mockTask = {
5359
cwd: "/workspace/project",
5460
consecutiveMistakeCount: 0,
61+
providerRef: {
62+
deref: vi.fn().mockReturnValue({
63+
getState: vi.fn().mockResolvedValue({}),
64+
}),
65+
} as unknown as Task["providerRef"],
5566
recordToolUsage: vi.fn(),
5667
recordToolError: vi.fn(),
5768
rooIgnoreController: {
@@ -93,4 +104,57 @@ describe("ApplyPatchTool.execute - delete file success path", () => {
93104
expect(mockTask.recordToolUsage).not.toHaveBeenCalled()
94105
expect(mockTask.recordToolError).not.toHaveBeenCalled()
95106
})
107+
108+
describe("per-write checkpoints (B1)", () => {
109+
const deletePatch = `*** Begin Patch
110+
*** Delete File: src/obsolete.ts
111+
*** End Patch`
112+
const mockedCheckpointSave = checkpointSave as MockedFunction<typeof checkpointSave>
113+
114+
it("records one suppressed checkpoint for the whole patch (default-on)", async () => {
115+
await tool.execute({ patch: deletePatch }, mockTask as Task, {
116+
askApproval: mockAskApproval,
117+
handleError: mockHandleError,
118+
pushToolResult: mockPushToolResult,
119+
})
120+
121+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted"))
122+
expect(mockedCheckpointSave).toHaveBeenCalledOnce()
123+
expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask as Task, false, true)
124+
})
125+
126+
it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => {
127+
// Structural cast for the test double (matches the mock style used for the controllers above).
128+
const ref = (mockTask["providerRef"] as unknown as { deref: MockedFunction<() => unknown> }).deref
129+
ref.mockReturnValue({
130+
getState: vi.fn().mockResolvedValue({ perWriteCheckpoints: false }),
131+
})
132+
133+
await tool.execute({ patch: deletePatch }, mockTask as Task, {
134+
askApproval: mockAskApproval,
135+
handleError: mockHandleError,
136+
pushToolResult: mockPushToolResult,
137+
})
138+
139+
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Successfully deleted"))
140+
expect(mockedCheckpointSave).not.toHaveBeenCalled()
141+
})
142+
143+
it("does not record a checkpoint when patch processing fails", async () => {
144+
// A malformed patch fails at parse time, before the change loop and
145+
// the post-loop checkpoint hook.
146+
const badPatch = `*** Begin Patch
147+
*** This is not a valid hunk
148+
*** End Patch`
149+
150+
await tool.execute({ patch: badPatch }, mockTask as Task, {
151+
askApproval: mockAskApproval,
152+
handleError: mockHandleError,
153+
pushToolResult: mockPushToolResult,
154+
})
155+
156+
expect(mockTask.recordToolError).toHaveBeenCalledWith("apply_patch")
157+
expect(mockedCheckpointSave).not.toHaveBeenCalled()
158+
})
159+
})
96160
})

‎src/core/tools/__tests__/editFileTool.spec.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { fileExistsAtPath } from "../../../utils/fs"
77
import { isPathOutsideWorkspace } from "../../../utils/pathUtils"
88
import { getReadablePath } from "../../../utils/path"
99
import { ToolUse, ToolResponse, AskApproval, HandleError, PushToolResult } from "../../../shared/tools"
10+
import { checkpointSave } from "../../checkpoints"
1011
import { editFileTool } from "../EditFileTool"
1112

1213
vi.mock("fs/promises", () => ({
@@ -59,6 +60,10 @@ vi.mock("../../diff/stats", () => ({
5960
computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })),
6061
}))
6162

63+
vi.mock("../../checkpoints", () => ({
64+
checkpointSave: vi.fn().mockResolvedValue(undefined),
65+
}))
66+
6267
vi.mock("vscode", () => ({
6368
window: {
6469
showWarningMessage: vi.fn().mockResolvedValue(undefined),
@@ -774,4 +779,41 @@ describe("editFileTool", () => {
774779
expect(mockAskApproval).toHaveBeenCalled()
775780
})
776781
})
782+
783+
describe("per-write checkpoints (B1)", () => {
784+
const mockedCheckpointSave = checkpointSave as MockedFunction<typeof checkpointSave>
785+
786+
it("records one suppressed checkpoint after a successful edit (default-on)", async () => {
787+
await executeEditFileTool({})
788+
789+
expect(mockTask.consecutiveMistakeCount).toBe(0)
790+
expect(mockedCheckpointSave).toHaveBeenCalledOnce()
791+
expect(mockedCheckpointSave).toHaveBeenCalledWith(mockTask, false, true)
792+
})
793+
794+
it("does not record a checkpoint when perWriteCheckpoints is disabled", async () => {
795+
mockTask.providerRef.deref = vi.fn().mockReturnValue({
796+
getState: vi.fn().mockResolvedValue({
797+
diagnosticsEnabled: true,
798+
writeDelayMs: 1000,
799+
experiments: {},
800+
perWriteCheckpoints: false,
801+
}),
802+
})
803+
804+
await executeEditFileTool({})
805+
806+
expect(mockTask.consecutiveMistakeCount).toBe(0)
807+
expect(mockedCheckpointSave).not.toHaveBeenCalled()
808+
})
809+
810+
it("does not record a checkpoint when the edit fails", async () => {
811+
mockTask.diffViewProvider.saveChanges.mockRejectedValue(new Error("save failed"))
812+
813+
await executeEditFileTool({})
814+
815+
expect(mockHandleError).toHaveBeenCalledWith("edit_file", expect.any(Error))
816+
expect(mockedCheckpointSave).not.toHaveBeenCalled()
817+
})
818+
})
777819
})

0 commit comments

Comments
 (0)