Skip to content

Commit 58f9492

Browse files
committed
fix: prevent data loss in checkpoint restoration by removing dangerous git clean
- Removed the dangerous `git clean -d -f` command that was deleting all untracked files - Simplified restoration to use only `git reset --hard` which preserves untracked files - Added validation to ensure worktree configuration is correct before restoration - Added check to verify commit exists before attempting restoration - Added recovery methods (listStashes, recoverFromStash) for emergency data recovery - Updated tests to reflect the safer behavior This fixes the critical data loss issue where users were losing their entire codebase when restoring checkpoints, especially when switching between different tools like Copilot and Roo Code. Fixes #6209
1 parent d62a260 commit 58f9492

File tree

2 files changed

+232
-2
lines changed

2 files changed

+232
-2
lines changed

src/services/checkpoints/ShadowCheckpointService.ts

Lines changed: 100 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,18 @@ export abstract class ShadowCheckpointService extends EventEmitter {
6464
throw new Error("Shadow git repo already initialized")
6565
}
6666

67+
// Additional validation to ensure workspace directory is valid
68+
if (!this.workspaceDir || this.workspaceDir.trim() === "") {
69+
throw new Error("Invalid workspace directory: empty or undefined")
70+
}
71+
72+
// Ensure workspace directory exists
73+
try {
74+
await fs.access(this.workspaceDir)
75+
} catch (error) {
76+
throw new Error(`Workspace directory does not exist: ${this.workspaceDir}`)
77+
}
78+
6779
const hasNestedGitRepos = await this.hasNestedGitRepositories()
6880

6981
if (hasNestedGitRepos) {
@@ -85,12 +97,26 @@ export abstract class ShadowCheckpointService extends EventEmitter {
8597
this.log(`[${this.constructor.name}#initShadowGit] shadow git repo already exists at ${this.dotGitDir}`)
8698
const worktree = await this.getShadowGitConfigWorktree(git)
8799

88-
if (worktree !== this.workspaceDir) {
100+
if (!worktree) {
101+
// If worktree is not set, try to repair it
102+
this.log(
103+
`[${this.constructor.name}#initShadowGit] worktree not configured, setting to ${this.workspaceDir}`,
104+
)
105+
await git.addConfig("core.worktree", this.workspaceDir)
106+
this.shadowGitConfigWorktree = this.workspaceDir
107+
} else if (worktree !== this.workspaceDir) {
89108
throw new Error(
90109
`Checkpoints can only be used in the original workspace: ${worktree} !== ${this.workspaceDir}`,
91110
)
92111
}
93112

113+
// Validate that the shadow git repo is not corrupted
114+
try {
115+
await git.status()
116+
} catch (statusError) {
117+
throw new Error(`Shadow git repository appears to be corrupted: ${statusError.message}`)
118+
}
119+
94120
await this.writeExcludeFile()
95121
this.baseHash = await git.revparse(["HEAD"])
96122
} else {
@@ -246,10 +272,30 @@ export abstract class ShadowCheckpointService extends EventEmitter {
246272
throw new Error("Shadow git repo not initialized")
247273
}
248274

275+
// Validate worktree configuration before proceeding
276+
const worktree = await this.getShadowGitConfigWorktree(this.git)
277+
if (worktree !== this.workspaceDir) {
278+
throw new Error(
279+
`Worktree mismatch detected. Expected: ${this.workspaceDir}, Found: ${worktree}. Aborting restore to prevent data loss.`,
280+
)
281+
}
282+
283+
// Verify the commit exists
284+
try {
285+
await this.git.catFile(["-t", commitHash])
286+
} catch (error) {
287+
throw new Error(`Invalid commit hash: ${commitHash}`)
288+
}
289+
249290
const start = Date.now()
250-
await this.git.clean("f", ["-d", "-f"])
291+
292+
// Simply reset to the target commit
293+
// This is much safer than using git clean -d -f
251294
await this.git.reset(["--hard", commitHash])
252295

296+
// Important: We do NOT use git clean here because it would remove untracked files
297+
// git reset --hard only affects tracked files, leaving untracked files alone
298+
253299
// Remove all checkpoints after the specified commitHash.
254300
const checkpointIndex = this._checkpoints.indexOf(commitHash)
255301

@@ -322,6 +368,58 @@ export abstract class ShadowCheckpointService extends EventEmitter {
322368
return super.once(event, listener)
323369
}
324370

371+
/**
372+
* Recovery methods for data loss scenarios
373+
*/
374+
375+
public async listStashes(): Promise<Array<{ index: number; message: string; date: string }>> {
376+
if (!this.git) {
377+
throw new Error("Shadow git repo not initialized")
378+
}
379+
380+
try {
381+
const stashList = await this.git.stashList()
382+
return stashList.all.map((stash, index) => ({
383+
index,
384+
message: stash.message,
385+
date: stash.date || new Date().toISOString(),
386+
}))
387+
} catch (error) {
388+
const errorMessage = error instanceof Error ? error.message : String(error)
389+
this.log(`[${this.constructor.name}#listStashes] failed to list stashes: ${errorMessage}`)
390+
return []
391+
}
392+
}
393+
394+
public async recoverFromStash(stashIndex: number): Promise<void> {
395+
if (!this.git) {
396+
throw new Error("Shadow git repo not initialized")
397+
}
398+
399+
try {
400+
this.log(`[${this.constructor.name}#recoverFromStash] attempting to recover from stash@{${stashIndex}}`)
401+
402+
// Apply the stash without removing it from the stash list
403+
// Use --index to also restore staged changes
404+
await this.git.stash(["apply", "--index", `stash@{${stashIndex}}`])
405+
406+
this.log(`[${this.constructor.name}#recoverFromStash] successfully recovered from stash@{${stashIndex}}`)
407+
} catch (error) {
408+
// If applying with --index fails, try without it
409+
try {
410+
this.log(`[${this.constructor.name}#recoverFromStash] retrying without --index`)
411+
await this.git.stash(["apply", `stash@{${stashIndex}}`])
412+
this.log(
413+
`[${this.constructor.name}#recoverFromStash] successfully recovered from stash@{${stashIndex}} (without --index)`,
414+
)
415+
} catch (retryError) {
416+
const errorMessage = retryError instanceof Error ? retryError.message : String(retryError)
417+
this.log(`[${this.constructor.name}#recoverFromStash] failed to recover from stash: ${errorMessage}`)
418+
throw new Error(`Failed to recover from stash: ${errorMessage}`)
419+
}
420+
}
421+
}
422+
325423
/**
326424
* Storage
327425
*/

src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -822,5 +822,137 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(
822822
expect(await fs.readFile(testFile, "utf-8")).toBe("Hello, world!")
823823
})
824824
})
825+
826+
describe(`${klass.name}#restoreCheckpoint safety`, () => {
827+
it("does not use dangerous git clean command", async () => {
828+
// This test verifies that we don't use git clean -d -f which would
829+
// delete untracked files and potentially cause data loss
830+
831+
// Create initial checkpoint
832+
await fs.writeFile(testFile, "Initial content")
833+
const commit1 = await service.saveCheckpoint("Initial checkpoint")
834+
expect(commit1?.commit).toBeTruthy()
835+
836+
// Make changes and create another checkpoint
837+
await fs.writeFile(testFile, "Modified content")
838+
const commit2 = await service.saveCheckpoint("Second checkpoint")
839+
expect(commit2?.commit).toBeTruthy()
840+
841+
// Create a spy to monitor git commands
842+
const gitSpy = vitest.spyOn(service["git"]!, "clean")
843+
844+
// Restore to first checkpoint
845+
await service.restoreCheckpoint(commit1!.commit)
846+
847+
// Verify tracked file was restored
848+
expect(await fs.readFile(testFile, "utf-8")).toBe("Initial content")
849+
850+
// Verify that git clean was NOT called
851+
expect(gitSpy).not.toHaveBeenCalled()
852+
853+
gitSpy.mockRestore()
854+
})
855+
856+
it("validates worktree configuration before restoration", async () => {
857+
// Create a checkpoint
858+
await fs.writeFile(testFile, "Test content")
859+
const commit = await service.saveCheckpoint("Test checkpoint")
860+
expect(commit?.commit).toBeTruthy()
861+
862+
// Create a new service instance with corrupted worktree
863+
const corruptedShadowDir = path.join(tmpDir, `corrupted-${Date.now()}`)
864+
const corruptedService = await klass.create({
865+
taskId: "corrupted-test",
866+
shadowDir: corruptedShadowDir,
867+
workspaceDir: service.workspaceDir,
868+
log: () => {},
869+
})
870+
await corruptedService.initShadowGit()
871+
872+
// Manually corrupt the worktree configuration by modifying the internal state
873+
// This simulates a corrupted git config without actually setting an invalid path
874+
corruptedService["shadowGitConfigWorktree"] = "/some/wrong/path"
875+
876+
// Attempt to restore should fail with worktree mismatch error
877+
await expect(corruptedService.restoreCheckpoint(commit!.commit)).rejects.toThrow(
878+
"Worktree mismatch detected",
879+
)
880+
881+
// Using the original service (with correct worktree) should succeed
882+
await service.restoreCheckpoint(commit!.commit)
883+
expect(await fs.readFile(testFile, "utf-8")).toBe("Test content")
884+
885+
// Clean up
886+
await fs.rm(corruptedShadowDir, { recursive: true, force: true })
887+
})
888+
889+
it("validates commit hash before attempting restoration", async () => {
890+
// Try to restore with an invalid commit hash
891+
const invalidHash = "invalid-commit-hash-12345"
892+
893+
await expect(service.restoreCheckpoint(invalidHash)).rejects.toThrow(
894+
"Invalid commit hash: " + invalidHash,
895+
)
896+
})
897+
898+
it("safely restores without data loss", async () => {
899+
// Create initial checkpoint
900+
await fs.writeFile(testFile, "Initial content")
901+
const commit1 = await service.saveCheckpoint("Initial checkpoint")
902+
expect(commit1?.commit).toBeTruthy()
903+
904+
// Make changes and save another checkpoint
905+
await fs.writeFile(testFile, "Modified content")
906+
const newFile = path.join(service.workspaceDir, "new-file.txt")
907+
await fs.writeFile(newFile, "New file content")
908+
const commit2 = await service.saveCheckpoint("Second checkpoint")
909+
expect(commit2?.commit).toBeTruthy()
910+
911+
// Restore to initial checkpoint
912+
await service.restoreCheckpoint(commit1!.commit)
913+
914+
// Verify restoration worked correctly
915+
expect(await fs.readFile(testFile, "utf-8")).toBe("Initial content")
916+
917+
// The new file should not exist in the first checkpoint
918+
await expect(fs.readFile(newFile, "utf-8")).rejects.toThrow()
919+
})
920+
921+
it("handles restoration when no changes need to be stashed", async () => {
922+
// Create and immediately restore a checkpoint (no changes to stash)
923+
await fs.writeFile(testFile, "Test content")
924+
const commit = await service.saveCheckpoint("Test checkpoint")
925+
expect(commit?.commit).toBeTruthy()
926+
927+
// Restore immediately without making any changes
928+
await expect(service.restoreCheckpoint(commit!.commit)).resolves.not.toThrow()
929+
expect(await fs.readFile(testFile, "utf-8")).toBe("Test content")
930+
})
931+
932+
it("properly removes files that should not exist in restored checkpoint", async () => {
933+
// Create initial checkpoint
934+
await fs.writeFile(testFile, "Initial content")
935+
const commit1 = await service.saveCheckpoint("Initial checkpoint")
936+
expect(commit1?.commit).toBeTruthy()
937+
938+
// Add a new tracked file
939+
const trackedFile = path.join(service.workspaceDir, "tracked-file.txt")
940+
await fs.writeFile(trackedFile, "This file will be tracked")
941+
const commit2 = await service.saveCheckpoint("Added tracked file")
942+
expect(commit2?.commit).toBeTruthy()
943+
944+
// Verify file exists
945+
expect(await fs.readFile(trackedFile, "utf-8")).toBe("This file will be tracked")
946+
947+
// Restore to initial checkpoint (before the file existed)
948+
await service.restoreCheckpoint(commit1!.commit)
949+
950+
// Verify the tracked file was removed
951+
await expect(fs.readFile(trackedFile, "utf-8")).rejects.toThrow()
952+
953+
// Verify original file is still correct
954+
expect(await fs.readFile(testFile, "utf-8")).toBe("Initial content")
955+
})
956+
})
825957
},
826958
)

0 commit comments

Comments
 (0)