Skip to content

Fix checkpoint restore when the saved commit is missing #2269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
edelauna merged 15 commits into from
Nov 3, 2026
Merged

Fix checkpoint restore when the saved commit is missing #2269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e619a65
fix: validate checkpoint before restoring workspace
Oct 30, 2026
e7a5c57
fix(checkpoints): only treat missing objects as missing checkpoints
Oct 30, 2026
a044e48
fix(checkpoints): surface restore failures to the user and recover sa…
Nov 1, 2026
8f29a9a
fix: scope failed-edit cleanup to its own staged edit and recheck tas…
Nov 1, 2026
c90b7f3
fix: skip in-place restart replacement when another task became current
Nov 1, 2026
965b60e
fix: scope history profile restore to the rehydrated task and skip re…
Nov 1, 2026
0171bab
fix: skip mode-config persistence on history profile restore; tighten…
Nov 1, 2026
10d1c8d
test: tighten sticky-profile assertions for mismatched task and mode-…
Nov 1, 2026
22cb84b
test: assert setModeConfig mode and profile id
Nov 1, 2026
86777f9
fix: reject unrecorded or malformed checkpoint hashes before any Git …
Nov 1, 2026
584f720
fix: accept SHA-256 checkpoint hashes in restore validation
Nov 1, 2026
8a83890
test: cover SHA-256 checkpoint diff and invalid-hash locale contract
Nov 1, 2026
7710024
fix: remove listener bookkeeping for a rehydrated task discarded by t…
Nov 1, 2026
ba408c1
fix: make task-listener cleanup a module helper so provider doubles k…
Nov 1, 2026
94711ae
fix: validate restore hash before cancelling task; compare task insta…
Nov 2, 2026
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
176 changes: 123 additions & 53 deletions src/core/checkpoints/__tests__/checkpoint.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
import { checkpointSave, checkpointRestore, checkpointDiff, getCheckpointService } from "../index"
import { MessageManager } from "../../message-manager"
import { RepoPerTaskCheckpointService } from "../../../services/checkpoints"
import * as vscode from "vscode"

const RESTORE_HASH = "a".repeat(40)
const HASH_1 = "1".repeat(40)
const HASH_2 = "2".repeat(40)

// Mock vscode
vi.mock("vscode", () => ({
window: {
Expand Down Expand Up @@ -30,39 +35,32 @@ vi.mock("@roo-code/telemetry", () => ({
},
}))

vi.mock("../../../utils/path", () => ({
getWorkspacePath: vi.fn(() => "/test/workspace"),
}))

vi.mock("../../../utils/git", () => ({
checkGitInstalled: vi.fn().mockResolvedValue(true),
}))

vi.mock("../../../i18n", () => ({
t: vi.fn((key: string, options?: Record<string, any>) => {
if (key === "common:errors.wait_checkpoint_long_time") {
return `Checkpoint initialization is taking longer than ${options?.timeout} seconds...`
}
if (key === "common:errors.init_checkpoint_fail_long_time") {
return `Checkpoint initialization failed after ${options?.timeout} seconds`
}
return key
}),
}))

// Mock p-wait-for to control timeout behavior
vi.mock("p-wait-for", () => ({
default: vi.fn(),
}))

vi.mock("../../../services/checkpoints")

describe("Checkpoint functionality", () => {
let mockProvider: any
let mockTask: any
let mockCheckpointService: any

beforeEach(async () => {
const pathModule = await import("../../../utils/path")
const gitModule = await import("../../../utils/git")
const i18nModule = await import("../../../i18n")
vi.spyOn(pathModule, "getWorkspacePath").mockReturnValue("/test/workspace")
vi.spyOn(gitModule, "checkGitInstalled").mockResolvedValue(true)
vi.spyOn(i18nModule, "t").mockImplementation((key: string, options?: Record<string, unknown>) => {
if (key === "common:errors.wait_checkpoint_long_time") {
return `Checkpoint initialization is taking longer than ${options?.timeout} seconds...`
}
if (key === "common:errors.init_checkpoint_fail_long_time") {
return `Checkpoint initialization failed after ${options?.timeout} seconds`
}
return key
})

// Create mock checkpoint service
mockCheckpointService = {
isInitialized: true,
Expand Down Expand Up @@ -105,11 +103,12 @@ describe("Checkpoint functionality", () => {

// Update the mock to return our mockCheckpointService
const checkpointsModule = await import("../../../services/checkpoints")
vi.mocked(checkpointsModule.RepoPerTaskCheckpointService.create).mockReturnValue(mockCheckpointService)
vi.spyOn(checkpointsModule.RepoPerTaskCheckpointService, "create").mockReturnValue(mockCheckpointService)
})

afterEach(() => {
vi.clearAllMocks()
vi.restoreAllMocks()
})

describe("checkpointSave", () => {
Expand Down Expand Up @@ -213,6 +212,7 @@ describe("Checkpoint functionality", () => {
{ ts: 1, say: "user", text: "Message 1" },
{ ts: 2, say: "assistant", text: "Message 2" },
{ ts: 3, say: "user", text: "Message 3" },
{ ts: 4, say: "checkpoint_saved", text: RESTORE_HASH },
]
mockTask.apiConversationHistory = [
{ ts: 1, role: "user", content: [{ type: "text", text: "Message 1" }] },
Expand All @@ -224,12 +224,12 @@ describe("Checkpoint functionality", () => {
it("should restore checkpoint for delete operation", async () => {
await checkpointRestore(mockTask, {
ts: 2,
commitHash: "abc123",
commitHash: RESTORE_HASH,
mode: "restore",
operation: "delete",
})

expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith("abc123")
expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith(RESTORE_HASH)
expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([
{ ts: 1, role: "user", content: [{ type: "text", text: "Message 1" }] },
])
Expand All @@ -240,12 +240,12 @@ describe("Checkpoint functionality", () => {
it("should restore checkpoint for edit operation", async () => {
await checkpointRestore(mockTask, {
ts: 2,
commitHash: "abc123",
commitHash: RESTORE_HASH,
mode: "restore",
operation: "edit",
})

expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith("abc123")
expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith(RESTORE_HASH)
expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([
{ ts: 1, role: "user", content: [{ type: "text", text: "Message 1" }] },
])
Expand All @@ -260,11 +260,11 @@ describe("Checkpoint functionality", () => {
it("should handle preview mode without modifying messages", async () => {
await checkpointRestore(mockTask, {
ts: 2,
commitHash: "abc123",
commitHash: RESTORE_HASH,
mode: "preview",
})

expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith("abc123")
expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith(RESTORE_HASH)
expect(mockTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
expect(mockTask.overwriteClineMessages).not.toHaveBeenCalled()
expect(mockProvider.cancelTask).toHaveBeenCalled()
Expand All @@ -273,34 +273,76 @@ describe("Checkpoint functionality", () => {
it("should handle missing message gracefully", async () => {
await checkpointRestore(mockTask, {
ts: 999, // Non-existent timestamp
commitHash: "abc123",
commitHash: RESTORE_HASH,
mode: "restore",
})

expect(mockCheckpointService.restoreCheckpoint).not.toHaveBeenCalled()
})

it("should disable checkpoints on error", async () => {
mockCheckpointService.restoreCheckpoint.mockRejectedValue(new Error("Restore failed"))
it.each(["delete", "edit"] as const)(
"propagates restore failures for %s without rewinding",
async (operation) => {
const restoreError = new Error("Restore failed")
mockCheckpointService.restoreCheckpoint.mockRejectedValueOnce(restoreError)
const messagesBeforeRestore = [...mockTask.clineMessages]
const historyBeforeRestore = [...mockTask.apiConversationHistory]

await expect(
checkpointRestore(mockTask, {
ts: 2,
commitHash: RESTORE_HASH,
mode: "restore",
operation,
}),
).rejects.toBe(restoreError)

expect(mockTask.enableCheckpoints).toBe(true)
expect(mockTask.clineMessages).toEqual(messagesBeforeRestore)
expect(mockTask.apiConversationHistory).toEqual(historyBeforeRestore)
expect(mockTask.overwriteClineMessages).not.toHaveBeenCalled()
expect(mockTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
expect(mockTask.say).not.toHaveBeenCalled()
expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled()
expect(mockProvider.cancelTask).not.toHaveBeenCalled()
expect(mockProvider.log).toHaveBeenCalledWith("[checkpointRestore] failed to restore checkpoint")
},
)

await checkpointRestore(mockTask, {
ts: 2,
commitHash: "abc123",
mode: "restore",
})
it("accepts a recorded SHA-256 checkpoint hash", async () => {
const sha256Hash = "c".repeat(64)
mockTask.clineMessages.push({ ts: 5, say: "checkpoint_saved", text: sha256Hash })

expect(mockTask.enableCheckpoints).toBe(false)
expect(mockProvider.log).toHaveBeenCalledWith("[checkpointRestore] disabling checkpoints for this task")
await checkpointRestore(mockTask, { ts: 2, commitHash: sha256Hash, mode: "preview" })

expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith(sha256Hash)
})

it.each([
["a malformed hash", "HEAD~1"],
["an abbreviated hash", "a".repeat(7)],
["a hash between SHA-1 and SHA-256 length", "a".repeat(41)],
["a well-formed but unrecorded hash", "b".repeat(40)],
["a non-string", 42],
])("rejects %s before any Git call", async (_label, commitHash) => {
await expect(
checkpointRestore(mockTask, { ts: 2, commitHash: commitHash as string, mode: "restore" }),
).rejects.toThrow("common:errors.checkpoint_invalid_hash")

expect(RepoPerTaskCheckpointService.create).not.toHaveBeenCalled()
expect(mockCheckpointService.restoreCheckpoint).not.toHaveBeenCalled()
expect(mockTask.overwriteClineMessages).not.toHaveBeenCalled()
expect(mockProvider.cancelTask).not.toHaveBeenCalled()
})
})

describe("checkpointDiff", () => {
beforeEach(() => {
mockTask.clineMessages = [
{ ts: 1, say: "user", text: "Message 1" },
{ ts: 2, say: "checkpoint_saved", text: "commit1" },
{ ts: 2, say: "checkpoint_saved", text: HASH_1 },
{ ts: 3, say: "user", text: "Message 2" },
{ ts: 4, say: "checkpoint_saved", text: "commit2" },
{ ts: 4, say: "checkpoint_saved", text: HASH_2 },
]
})

Expand All @@ -315,12 +357,12 @@ describe("Checkpoint functionality", () => {

await checkpointDiff(mockTask, {
ts: 4,
commitHash: "commit2",
commitHash: HASH_2,
mode: "to-current",
})

expect(mockCheckpointService.getDiff).toHaveBeenCalledWith({
from: "commit2",
from: HASH_2,
to: undefined,
})
expect(vscode.commands.executeCommand).toHaveBeenCalledWith(
Expand All @@ -340,13 +382,13 @@ describe("Checkpoint functionality", () => {
mockCheckpointService.getDiff.mockResolvedValue(mockChanges)
await checkpointDiff(mockTask, {
ts: 4,
commitHash: "commit1",
commitHash: HASH_1,
mode: "checkpoint",
})

expect(mockCheckpointService.getDiff).toHaveBeenCalledWith({
from: "commit1",
to: "commit2",
from: HASH_1,
to: HASH_2,
})
expect(vscode.commands.executeCommand).toHaveBeenCalledWith(
"vscode.changes",
Expand All @@ -366,13 +408,13 @@ describe("Checkpoint functionality", () => {

await checkpointDiff(mockTask, {
ts: 4,
commitHash: "commit1",
commitHash: HASH_1,
mode: "checkpoint",
})

expect(mockCheckpointService.getDiff).toHaveBeenCalledWith({
from: "commit1", // Should find the next checkpoint
to: "commit2",
from: HASH_1, // Should find the next checkpoint
to: HASH_2,
})
})

Expand All @@ -381,20 +423,48 @@ describe("Checkpoint functionality", () => {

await checkpointDiff(mockTask, {
ts: 4,
commitHash: "commit2",
commitHash: HASH_2,
mode: "to-current",
})

expect(vscode.window.showInformationMessage).toHaveBeenCalledWith("common:errors.checkpoint_no_changes")
expect(vscode.commands.executeCommand).not.toHaveBeenCalled()
})

it("should show diff for a recorded SHA-256 checkpoint hash", async () => {
const sha256Hash = "d".repeat(64)
mockTask.clineMessages.push({ ts: 5, say: "checkpoint_saved", text: sha256Hash })
mockCheckpointService.getDiff.mockResolvedValue([
{
paths: { absolute: "/test/file.ts", relative: "file.ts" },
content: { before: "old content", after: "new content" },
},
])

await checkpointDiff(mockTask, { ts: 5, commitHash: sha256Hash, mode: "to-current" })

expect(mockCheckpointService.getDiff).toHaveBeenCalledWith({ from: sha256Hash, to: undefined })
})

it.each([
["a malformed hash", "HEAD~1"],
["a well-formed but unrecorded hash", "b".repeat(40)],
["a non-string", 42],
])("rejects %s before any Git call", async (_label, commitHash) => {
await checkpointDiff(mockTask, { commitHash: commitHash as string, mode: "to-current" })

expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("common:errors.checkpoint_invalid_hash")
expect(RepoPerTaskCheckpointService.create).not.toHaveBeenCalled()
expect(mockCheckpointService.getDiff).not.toHaveBeenCalled()
expect(mockTask.enableCheckpoints).toBe(true)
})

it("should disable checkpoints on error", async () => {
mockCheckpointService.getDiff.mockRejectedValue(new Error("Diff failed"))

await checkpointDiff(mockTask, {
ts: 4,
commitHash: "commit2",
commitHash: HASH_2,
mode: "to-current",
})

Expand Down Expand Up @@ -426,7 +496,7 @@ describe("Checkpoint functionality", () => {
mockTask.checkpointService = undefined
mockTask.checkpointServiceInitializing = false

getCheckpointService(mockTask)
await getCheckpointService(mockTask)

const checkpointsModule = await import("../../../services/checkpoints")
expect(vi.mocked(checkpointsModule.RepoPerTaskCheckpointService.create)).toHaveBeenCalledWith({
Expand All @@ -439,7 +509,7 @@ describe("Checkpoint functionality", () => {

it("should disable checkpoints if workspace path is not found", async () => {
const pathModule = await import("../../../utils/path")
vi.mocked(pathModule.getWorkspacePath).mockReturnValue(null as any)
vi.mocked(pathModule.getWorkspacePath).mockReturnValue("")

mockTask.checkpointService = undefined
mockTask.checkpointServiceInitializing = false
Expand Down
17 changes: 14 additions & 3 deletions src/core/checkpoints/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

import { CheckpointServiceOptions, RepoPerTaskCheckpointService } from "../../services/checkpoints"

import { assertRecordedCheckpoint } from "./recordedCheckpoint"

const WARNING_THRESHOLD_MS = 5000

function sendCheckpointInitWarn(task: Task, type?: "WAIT_TIMEOUT" | "INIT_TIMEOUT", timeout?: number) {
Expand Down Expand Up @@ -238,6 +240,8 @@
task: Task,
{ ts, commitHash, mode, operation = "delete" }: CheckpointRestoreOptions,
) {
assertRecordedCheckpoint(task, commitHash)

@edelauna edelauna Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The webview handler calls provider.cancelTask() before this check runs (webviewMessageHandler.ts:1787). If the hash is malformed or unrecorded, the user's running task is cancelled and then the restore is rejected. Should the hash check run in the handler before the cancel, or is the cancel intended in this case?

@simurg79 simurg79 Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 94711ae. The checkpointRestore handler now runs assertRecordedCheckpoint against the current task before provider.cancelTask(), so a malformed or unrecorded hash shows the localized error and leaves the running task alone. The check still also runs inside checkpointRestore. New tests in webviewMessageHandler.checkpoint.spec.ts cover malformed and unrecorded hashes (no cancel, no restore) and confirm a recorded hash still cancels before restoring.


const service = await getCheckpointService(task)

if (!service) {
Expand Down Expand Up @@ -295,9 +299,9 @@
// task flow and the communication between the webview and the
// `Task` instance.
provider?.cancelTask()
} catch {
provider?.log("[checkpointRestore] disabling checkpoints for this task")
task.enableCheckpoints = false
} catch (error) {
provider?.log("[checkpointRestore] failed to restore checkpoint")

Check warning on line 303 in src/core/checkpoints/index.ts

View workflow job for this annotation

GitHub Actions / mutation-diff

Mutation test advisory 

src/core/checkpoints/index.ts:303: Survived OptionalChaining mutant (replacement: provider.log). See the job summary for the complete list and resolution guidance.
throw error
}
}

Expand All @@ -315,6 +319,13 @@
}

export async function checkpointDiff(task: Task, { commitHash, mode }: CheckpointDiffOptions) {
try {
assertRecordedCheckpoint(task, commitHash)
} catch (error) {
void vscode.window.showErrorMessage(error instanceof Error ? error.message : String(error))
return
}

const service = await getCheckpointService(task)

if (!service) {
Expand Down
Loading
Loading