Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions packages/types/src/global-settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,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
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* GlobalSettings
*/
Expand Down Expand Up @@ -200,6 +207,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(),
Expand Down
1 change: 1 addition & 0 deletions packages/types/src/vscode-extension-host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,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
Expand Down
14 changes: 14 additions & 0 deletions src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@ export class Task extends EventEmitter<TaskEvents> 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
Expand Down Expand Up @@ -2492,6 +2495,17 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
// arm needed.
void getCheckpointService(this)

// B1 task-start baseline: a suppressed pre-task root commit (default-on).
if (!this.taskStartBaselineDone) {
this.taskStartBaselineDone = true
const baselineEnabled = (await this.providerRef.deref()?.getState())?.perWriteCheckpoints
if (baselineEnabled !== false) {
// allowEmpty=true so a clean workspace still produces the baseline
// commit; awaited so the first per-write checkpoint cannot interleave.
await this.checkpointSave(true, true)
}
}

let nextUserContent = userContent
let includeFileDetails = true

Expand Down
92 changes: 92 additions & 0 deletions src/core/task/__tests__/Task.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3289,6 +3289,98 @@ describe("Cline", () => {
})
})

describe("task-start baseline (B1 perWriteCheckpoints)", () => {
it("records one suppressed baseline checkpoint per Task instance at loop start", async () => {
const task = new Task({
provider: mockProvider,
apiConfiguration: mockApiConfig,
task: "baseline task",
startTask: false,
})
const taskAccess = getTaskTestAccess(task)
const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined)
const state = await mockProvider.getState()
vi.spyOn(mockProvider, "getState").mockResolvedValue(state)

task.abort = true

await taskAccess.initiateTaskLoop([])
await taskAccess.initiateTaskLoop([])

expect(saveSpy).toHaveBeenCalledOnce()
expect(saveSpy).toHaveBeenCalledWith(true, true)
})

it("records the baseline checkpoint when the setting is unset (default-on)", async () => {
const task = new Task({
provider: mockProvider,
apiConfiguration: mockApiConfig,
task: "baseline unset task",
startTask: false,
})
const taskAccess = getTaskTestAccess(task)
const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined)
const state = await mockProvider.getState()
// Unset: the property is absent from the state, so default-on applies.
const unsetState = { ...state }
Reflect.deleteProperty(unsetState, "perWriteCheckpoints")
vi.spyOn(mockProvider, "getState").mockResolvedValue(unsetState as typeof state)

task.abort = true

await taskAccess.initiateTaskLoop([])

expect(saveSpy).toHaveBeenCalledOnce()
expect(saveSpy).toHaveBeenCalledWith(true, true)
})

it("does not record a baseline checkpoint when perWriteCheckpoints is disabled", async () => {
const task = new Task({
provider: mockProvider,
apiConfiguration: mockApiConfig,
task: "baseline disabled task",
startTask: false,
})
const taskAccess = getTaskTestAccess(task)
const saveSpy = vi.spyOn(task, "checkpointSave").mockResolvedValue(undefined)
const state = await mockProvider.getState()
vi.spyOn(mockProvider, "getState").mockResolvedValue({ ...state, perWriteCheckpoints: false })

task.abort = true

await taskAccess.initiateTaskLoop([])

expect(saveSpy).not.toHaveBeenCalled()
})

it("awaits the baseline checkpoint before entering the request loop", async () => {
const task = new Task({
provider: mockProvider,
apiConfiguration: mockApiConfig,
task: "baseline await task",
startTask: false,
})
const taskAccess = getTaskTestAccess(task)
type SaveResult = Awaited<ReturnType<typeof task.checkpointSave>>
let resolveSave: (value: SaveResult | PromiseLike<SaveResult>) => void = () => {}
const saveSpy = vi
.spyOn(task, "checkpointSave")
.mockImplementation(() => new Promise<SaveResult>((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()
})
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.

describe("start()", () => {
it("should be a no-op if the task was already started in the constructor", () => {
const task = new Task({
Expand Down
67 changes: 49 additions & 18 deletions src/core/tools/ApplyPatchTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -102,7 +103,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)
Expand All @@ -120,17 +124,39 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {

if (change.type === "add") {
// Create new file
await this.handleAddFile(change, absolutePath, relPath, task, callbacks, isWriteProtected)
patchSucceeded =
(await this.handleAddFile(change, absolutePath, relPath, task, callbacks, isWriteProtected)) &&
patchSucceeded
} else if (change.type === "delete") {
// Delete file
await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected)
patchSucceeded =
(await this.handleDeleteFile(absolutePath, relPath, task, callbacks, isWriteProtected)) &&
patchSucceeded
} else if (change.type === "update") {
// Update file
await this.handleUpdateFile(change, absolutePath, relPath, task, callbacks, isWriteProtected)
patchSucceeded =
(await this.handleUpdateFile(
change,
absolutePath,
relPath,
task,
callbacks,
isWriteProtected,
)) && 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) {
void checkpointSave(task, false, true).catch(() => {})
}
}
} catch (error) {
await handleError("apply patch", error as Error)
await task.diffViewProvider.reset()
Expand All @@ -144,7 +170,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
task: Task,
callbacks: ToolCallbacks,
isWriteProtected: boolean,
): Promise<void> {
): Promise<boolean> {
const { askApproval, pushToolResult } = callbacks

// Check if file already exists
Expand All @@ -155,7 +181,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 false
}

const newContent = change.newContent || ""
Expand Down Expand Up @@ -209,7 +235,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
}
pushToolResult("Changes were rejected by the user.")
await task.diffViewProvider.reset()
return
return false
}

// Save the changes
Expand All @@ -227,6 +253,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
pushToolResult(message)
await task.diffViewProvider.reset()
task.processQueuedMessages()
return true
}

private async handleDeleteFile(
Expand All @@ -235,7 +262,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
task: Task,
callbacks: ToolCallbacks,
isWriteProtected: boolean,
): Promise<void> {
): Promise<boolean> {
const { askApproval, pushToolResult } = callbacks

// Check if file exists
Expand All @@ -246,7 +273,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 false
}

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

if (!didApprove) {
pushToolResult("Delete operation was rejected by the user.")
return
return false
}

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

task.didEditFile = true
pushToolResult(`Successfully deleted ${relPath}`)
task.processQueuedMessages()
return true
}

private async handleUpdateFile(
Expand All @@ -293,7 +321,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
task: Task,
callbacks: ToolCallbacks,
isWriteProtected: boolean,
): Promise<void> {
): Promise<boolean> {
const { askApproval, pushToolResult } = callbacks

// Check if file exists
Expand All @@ -304,7 +332,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 false
}

const originalContent = change.originalContent || ""
Expand All @@ -318,9 +346,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 true
}

// Check experiment settings
Expand Down Expand Up @@ -366,7 +396,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
}
pushToolResult("Changes were rejected by the user.")
await task.diffViewProvider.reset()
return
return false
}

// Handle file move if specified
Expand All @@ -379,7 +409,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 false
}

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

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

// Save new content to the new path
Expand Down Expand Up @@ -447,6 +477,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
pushToolResult(message)
await task.diffViewProvider.reset()
task.processQueuedMessages()
return true
}

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