From 56e00cf8b0d06776333610776de4a5d3b7906098 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 28 Sep 2026 20:12:40 +0200 Subject: [PATCH 1/3] fix(checkpoints): keep files ignored at checkpoint time on restore restoreCheckpoint ran `git clean -f -d -f` against the workspace's current ignore rules. Files ignored when a checkpoint was saved were never committed to it, so after .gitignore changed (for example, an agent overwrote it) they were no longer ignored and the clean deleted them. Each checkpoint now records the paths ignored when it was saved (`git ls-files --others --ignored --exclude-standard --directory`). Restore removes only untracked files ignored by neither the current rules nor that record, then prunes directories left empty, as `git clean -d` did. Checkpoints saved before this change have no record and keep the previous clean. Fixes #1832 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../checkpoints/ShadowCheckpointService.ts | 87 ++++++++++++++++++- .../__tests__/ShadowCheckpointService.spec.ts | 64 ++++++++++++++ 2 files changed, 150 insertions(+), 1 deletion(-) diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index 3e3d3d0653..ac6843593a 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -218,6 +218,7 @@ export abstract class ShadowCheckpointService extends EventEmitter { await this.stageAll(git) const { commit } = await git.commit("initial commit", { "--allow-empty": null }) this.baseHash = commit + await this.recordIgnoredPaths(git, commit) created = true } @@ -263,6 +264,83 @@ export abstract class ShadowCheckpointService extends EventEmitter { } } + // Ignore rules are read from the live workspace, so a .gitignore changed after a checkpoint + // would make `git clean` delete files that were ignored (and so never checkpointed) at the + // time (#1832). Each checkpoint therefore records the paths ignored when it was saved. + private ignoredRecordPath(commitHash: string) { + return path.join(this.dotGitDir, "zoo-checkpoint-ignored", commitHash) + } + + private async listIgnoredPaths(git: SimpleGit): Promise { + // --directory collapses fully ignored directories (e.g. node_modules/) to one entry. + const output = await git.raw(["ls-files", "--others", "--ignored", "--exclude-standard", "--directory", "-z"]) + return output.split("\0").filter(Boolean) + } + + private async readIgnoredRecord(commitHash: string): Promise { + try { + const content = await fs.readFile(this.ignoredRecordPath(commitHash), "utf8") + return content.split("\0").filter(Boolean) + } catch { + return undefined + } + } + + /** Adds the currently ignored paths to the record for a checkpoint; never fails the save. */ + private async recordIgnoredPaths(git: SimpleGit, commitHash: string) { + try { + const ignored = new Set([ + ...((await this.readIgnoredRecord(commitHash)) ?? []), + ...(await this.listIgnoredPaths(git)), + ]) + await fs.mkdir(path.dirname(this.ignoredRecordPath(commitHash)), { recursive: true }) + await fs.writeFile(this.ignoredRecordPath(commitHash), [...ignored].join("\0")) + } catch (error) { + this.log( + `[${this.constructor.name}#recordIgnoredPaths] failed to record ignored paths: ${error instanceof Error ? error.message : String(error)}`, + ) + } + } + + /** + * Removes untracked files like `git clean -f -d -f`, but only those ignored by neither the + * current rules nor the rules recorded when the checkpoint was saved. + */ + private async removeUntrackedFiles(git: SimpleGit, ignoredAtCheckpoint: string[]) { + const ignoredEntries = new Set(ignoredAtCheckpoint) + const ignoredDirs = ignoredAtCheckpoint.filter((entry) => entry.endsWith("/")) + const wasIgnored = (relPath: string) => + ignoredEntries.has(relPath) || ignoredDirs.some((dir) => relPath.startsWith(dir)) + + // --exclude-standard applies the current rules; nested repositories are listed as "dir/". + const output = await git.raw(["ls-files", "--others", "--exclude-standard", "-z"]) + const removedDirs = new Set() + for (const relPath of output.split("\0").filter(Boolean)) { + if (wasIgnored(relPath)) { + continue + } + const absPath = path.join(this.workspaceDir, relPath) + await fs.rm(absPath, { force: true, recursive: relPath.endsWith("/") }) + removedDirs.add(path.dirname(absPath)) + } + + // Like `git clean -d`, remove directories left empty by the removal. + for (const dir of [...removedDirs].sort((a, b) => b.length - a.length)) { + let current = dir + while (current.startsWith(this.workspaceDir + path.sep)) { + try { + await fs.rmdir(current) + } catch (error) { + const code = error && typeof error === "object" && "code" in error ? error.code : undefined + if (code !== "ENOENT") { + break // Not empty: stop walking up. + } + } + current = path.dirname(current) + } + } + } + private async getNestedGitRepository(): Promise { try { // Find all .git/HEAD files that are not at the root level. @@ -348,6 +426,7 @@ export abstract class ShadowCheckpointService extends EventEmitter { const fromHash = this._checkpoints[this._checkpoints.length - 1] ?? this.baseHash! const toHash = result.commit || fromHash this._checkpoints.push(toHash) + await this.recordIgnoredPaths(this.git, toHash) const duration = Date.now() - startTime if (result.commit) { @@ -386,7 +465,13 @@ export abstract class ShadowCheckpointService extends EventEmitter { } const start = Date.now() - await this.git.clean("f", ["-d", "-f"]) + const ignoredAtCheckpoint = await this.readIgnoredRecord(commitHash) + if (ignoredAtCheckpoint) { + await this.removeUntrackedFiles(this.git, ignoredAtCheckpoint) + } else { + // Checkpoints saved before ignore records existed keep the previous behavior. + await this.git.clean("f", ["-d", "-f"]) + } await this.git.reset(["--hard", commitHash]) // Remove all checkpoints after the specified commitHash. diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index 1710cc97e8..bd43b7c139 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -320,6 +320,70 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( await expect(fs.readFile(untrackedFile, "utf-8")).rejects.toThrow() }) + describe("restore with changed ignore rules (#1832)", () => { + it("keeps files that were ignored at checkpoint time when .gitignore was overwritten", async () => { + // The reported setup: .gitignore ignores itself, so it is never in a checkpoint. + const gitignore = path.join(service.workspaceDir, ".gitignore") + const secret = path.join(service.workspaceDir, "secrets", "key.txt") + await fs.writeFile(gitignore, ".gitignore\nsecrets/\n") + await fs.mkdir(path.dirname(secret), { recursive: true }) + await fs.writeFile(secret, "untracked and ignored") + const checkpoint = await service.saveCheckpoint("before write", { allowEmpty: true }) + + // An agent overwrites .gitignore, so secrets/ is no longer ignored, and creates a file. + await fs.writeFile(gitignore, "dist/\n") + const createdAfter = path.join(service.workspaceDir, "created-after.txt") + await fs.writeFile(createdAfter, "new") + + await service.restoreCheckpoint(checkpoint!.commit) + + expect(await fs.readFile(secret, "utf-8")).toBe("untracked and ignored") + // .gitignore was never checkpointed, so the overwritten version is kept rather than deleted. + expect(await fs.readFile(gitignore, "utf-8")).toBe("dist/\n") + expect(await fileExistsAtPath(createdAfter)).toBe(false) + }) + + it("keeps files that were ignored at checkpoint time when a checkpointed .gitignore changed", async () => { + const gitignore = path.join(service.workspaceDir, ".gitignore") + const secret = path.join(service.workspaceDir, "secrets", "key.txt") + await fs.writeFile(gitignore, "secrets/\n") + await fs.mkdir(path.dirname(secret), { recursive: true }) + await fs.writeFile(secret, "untracked and ignored") + const checkpoint = await service.saveCheckpoint("with .gitignore") + expect(checkpoint?.commit).toBeTruthy() + + await fs.writeFile(gitignore, "dist/\n") + const createdAfter = path.join(service.workspaceDir, "nested", "created-after.txt") + await fs.mkdir(path.dirname(createdAfter), { recursive: true }) + await fs.writeFile(createdAfter, "new") + + await service.restoreCheckpoint(checkpoint!.commit) + + expect(await fs.readFile(secret, "utf-8")).toBe("untracked and ignored") + // Normalize line endings: core.autocrlf may rewrite the restored file on Windows. + expect((await fs.readFile(gitignore, "utf-8")).replace(/\r\n/g, "\n")).toBe("secrets/\n") + // Files created after the checkpoint are still rolled back, including their new directory. + expect(await fileExistsAtPath(path.dirname(createdAfter))).toBe(false) + }) + + it("restores checkpoints saved before ignore records existed with the previous clean", async () => { + await fs.writeFile(testFile, "checkpointed") + const checkpoint = await service.saveCheckpoint("legacy checkpoint") + // Simulate a checkpoint saved by an older version, which wrote no ignore record. + await fs.rm(path.join(service.checkpointsDir, ".git", "zoo-checkpoint-ignored"), { + recursive: true, + force: true, + }) + const createdAfter = path.join(service.workspaceDir, "created-after.txt") + await fs.writeFile(createdAfter, "new") + + await service.restoreCheckpoint(checkpoint!.commit) + + expect(await fs.readFile(testFile, "utf-8")).toBe("checkpointed") + expect(await fileExistsAtPath(createdAfter)).toBe(false) + }) + }) + it("does not create a checkpoint for ignored files", async () => { // Create a file that matches an ignored pattern (e.g., .log file). const ignoredFile = path.join(service.workspaceDir, "ignored.log") From ee86053cb29a49b9754818f99b1a5ea44c2ad231 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 5 Oct 2026 18:20:34 +0200 Subject: [PATCH 2/3] Fix checkpoint ignore record durability and empty directory cleanup --- .../checkpoints/ShadowCheckpointService.ts | 43 +++++----- .../__tests__/ShadowCheckpointService.spec.ts | 83 +++++++++++++++++++ 2 files changed, 107 insertions(+), 19 deletions(-) diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index ac6843593a..e72912f338 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -288,17 +288,23 @@ export abstract class ShadowCheckpointService extends EventEmitter { /** Adds the currently ignored paths to the record for a checkpoint; never fails the save. */ private async recordIgnoredPaths(git: SimpleGit, commitHash: string) { + const recordPath = this.ignoredRecordPath(commitHash) + const tempPath = `${recordPath}.${crypto.randomUUID()}.tmp` try { const ignored = new Set([ ...((await this.readIgnoredRecord(commitHash)) ?? []), ...(await this.listIgnoredPaths(git)), ]) - await fs.mkdir(path.dirname(this.ignoredRecordPath(commitHash)), { recursive: true }) - await fs.writeFile(this.ignoredRecordPath(commitHash), [...ignored].join("\0")) + await fs.mkdir(path.dirname(recordPath), { recursive: true }) + // A no-change save reuses the same commit. Never truncate its existing protection. + await fs.writeFile(tempPath, [...ignored].join("\0")) + await fs.rename(tempPath, recordPath) } catch (error) { this.log( `[${this.constructor.name}#recordIgnoredPaths] failed to record ignored paths: ${error instanceof Error ? error.message : String(error)}`, ) + } finally { + await fs.rm(tempPath, { force: true }).catch(() => {}) } } @@ -307,37 +313,36 @@ export abstract class ShadowCheckpointService extends EventEmitter { * current rules nor the rules recorded when the checkpoint was saved. */ private async removeUntrackedFiles(git: SimpleGit, ignoredAtCheckpoint: string[]) { - const ignoredEntries = new Set(ignoredAtCheckpoint) - const ignoredDirs = ignoredAtCheckpoint.filter((entry) => entry.endsWith("/")) + const ignoredEntries = new Set([...ignoredAtCheckpoint, ...(await this.listIgnoredPaths(git))]) + const ignoredDirs = [...ignoredEntries].filter((entry) => entry.endsWith("/")) const wasIgnored = (relPath: string) => ignoredEntries.has(relPath) || ignoredDirs.some((dir) => relPath.startsWith(dir)) // --exclude-standard applies the current rules; nested repositories are listed as "dir/". const output = await git.raw(["ls-files", "--others", "--exclude-standard", "-z"]) - const removedDirs = new Set() for (const relPath of output.split("\0").filter(Boolean)) { if (wasIgnored(relPath)) { continue } const absPath = path.join(this.workspaceDir, relPath) await fs.rm(absPath, { force: true, recursive: relPath.endsWith("/") }) - removedDirs.add(path.dirname(absPath)) } - // Like `git clean -d`, remove directories left empty by the removal. - for (const dir of [...removedDirs].sort((a, b) => b.length - a.length)) { - let current = dir - while (current.startsWith(this.workspaceDir + path.sep)) { - try { - await fs.rmdir(current) - } catch (error) { - const code = error && typeof error === "object" && "code" in error ? error.code : undefined - if (code !== "ENOENT") { - break // Not empty: stop walking up. - } - } - current = path.dirname(current) + // --directory includes paths that never contained files. Walk these bottom-up, + // using only rmdir so a directory containing protected files is never deleted. + const removeEmptyDirectories = async (relDir: string): Promise => { + if (wasIgnored(relDir)) return + const absDir = path.join(this.workspaceDir, relDir) + for (const entry of await fs.readdir(absDir, { withFileTypes: true })) { + if (entry.isDirectory()) await removeEmptyDirectories(`${relDir}${entry.name}/`) } + await fs.rmdir(absDir).catch((error: NodeJS.ErrnoException) => { + if (error.code !== "ENOTEMPTY" && error.code !== "EEXIST" && error.code !== "ENOENT") throw error + }) + } + const directories = await git.raw(["ls-files", "--others", "--exclude-standard", "--directory", "-z"]) + for (const relPath of directories.split("\0").filter((entry) => entry.endsWith("/"))) { + await removeEmptyDirectories(relPath) } } diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index bd43b7c139..967c8dcbb0 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -328,6 +328,9 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( await fs.writeFile(gitignore, ".gitignore\nsecrets/\n") await fs.mkdir(path.dirname(secret), { recursive: true }) await fs.writeFile(secret, "untracked and ignored") + const shadowGit = simpleGit(service.checkpointsDir) + expect(await shadowGit.raw(["ls-files", "--", "secrets/key.txt"])).toBe("") + expect((await shadowGit.raw(["check-ignore", "secrets/key.txt"])).trim()).toBe("secrets/key.txt") const checkpoint = await service.saveCheckpoint("before write", { allowEmpty: true }) // An agent overwrites .gitignore, so secrets/ is no longer ignored, and creates a file. @@ -349,6 +352,9 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( await fs.writeFile(gitignore, "secrets/\n") await fs.mkdir(path.dirname(secret), { recursive: true }) await fs.writeFile(secret, "untracked and ignored") + const shadowGit = simpleGit(service.checkpointsDir) + expect(await shadowGit.raw(["ls-files", "--", "secrets/key.txt"])).toBe("") + expect((await shadowGit.raw(["check-ignore", "secrets/key.txt"])).trim()).toBe("secrets/key.txt") const checkpoint = await service.saveCheckpoint("with .gitignore") expect(checkpoint?.commit).toBeTruthy() @@ -367,6 +373,15 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( }) it("restores checkpoints saved before ignore records existed with the previous clean", async () => { + const gitignore = path.join(service.workspaceDir, ".gitignore") + const secret = path.join(service.workspaceDir, "legacy-secret.txt") + await fs.writeFile(gitignore, ".gitignore\nlegacy-secret.txt\n") + await fs.writeFile(secret, "ignored at save time") + const shadowGit = simpleGit(service.checkpointsDir) + expect(await shadowGit.raw(["ls-files", "--", "legacy-secret.txt"])).toBe("") + expect((await shadowGit.raw(["check-ignore", "legacy-secret.txt"])).trim()).toBe( + "legacy-secret.txt", + ) await fs.writeFile(testFile, "checkpointed") const checkpoint = await service.saveCheckpoint("legacy checkpoint") // Simulate a checkpoint saved by an older version, which wrote no ignore record. @@ -376,11 +391,79 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( }) const createdAfter = path.join(service.workspaceDir, "created-after.txt") await fs.writeFile(createdAfter, "new") + await fs.writeFile(gitignore, ".gitignore\n") await service.restoreCheckpoint(checkpoint!.commit) expect(await fs.readFile(testFile, "utf-8")).toBe("checkpointed") expect(await fileExistsAtPath(createdAfter)).toBe(false) + // Unlike recorded protection, legacy git clean only consults the current ignore rules. + expect(await fileExistsAtPath(secret)).toBe(false) + }) + + it.each(["write", "rename"] as const)( + "preserves the existing ignore record when %s fails", + async (failure) => { + const gitignore = path.join(service.workspaceDir, ".gitignore") + const secret = path.join(service.workspaceDir, "secret.txt") + await fs.writeFile(gitignore, ".gitignore\nsecret.txt\nnew-secret.txt\n") + await fs.writeFile(secret, "keep me") + const checkpoint = await service.saveCheckpoint("protected secret", { allowEmpty: true }) + const recordDir = path.join(service.checkpointsDir, ".git", "zoo-checkpoint-ignored") + const recordPath = path.join(recordDir, checkpoint!.commit) + const previous = await fs.readFile(recordPath, "utf8") + const previousFiles = await fs.readdir(recordDir) + await fs.writeFile(path.join(service.workspaceDir, "new-secret.txt"), "new ignored file") + const writeFile = fs.writeFile.bind(fs) + const failureSpy = + failure === "write" + ? vitest.spyOn(fs, "writeFile").mockImplementationOnce(async (file) => { + await writeFile(file, "") // Simulate truncation before a failed write. + throw new Error("record write failed") + }) + : vitest.spyOn(fs, "rename").mockRejectedValueOnce(new Error("record rename failed")) + + await expect(service.saveCheckpoint("no tracked changes")).resolves.toBeUndefined() + expect(failureSpy).toHaveBeenCalledOnce() + failureSpy.mockRestore() + expect(await fs.readFile(recordPath, "utf8")).toBe(previous) + expect(await fs.readdir(recordDir)).toEqual(previousFiles) + await fs.writeFile(gitignore, ".gitignore\n") + await service.restoreCheckpoint(checkpoint!.commit) + expect(await fs.readFile(secret, "utf8")).toBe("keep me") + }, + ) + + it("removes empty untracked directories without deleting protected contents", async () => { + const gitignore = path.join(service.workspaceDir, ".gitignore") + await fs.writeFile(gitignore, ".gitignore\nmixed/keep.txt\nignored-empty/\n") + await fs.mkdir(path.join(service.workspaceDir, "mixed")) + await fs.writeFile(path.join(service.workspaceDir, "mixed", "keep.txt"), "protected") + // Keep Git from collapsing the whole mixed directory into an ignored entry. + await fs.writeFile(path.join(service.workspaceDir, "mixed", "tracked.txt"), "tracked") + await fs.mkdir(path.join(service.workspaceDir, "ignored-empty")) + await fs.mkdir(path.join(service.workspaceDir, "tracked")) + await fs.writeFile(path.join(service.workspaceDir, "tracked", "keep.txt"), "tracked") + const checkpoint = await service.saveCheckpoint("with protected paths") + await fs.writeFile(gitignore, ".gitignore\ncurrent-empty/\n") + for (const dir of ["empty/deep", "mixed/empty", "tracked/empty", "current-empty"]) { + await fs.mkdir(path.join(service.workspaceDir, dir), { recursive: true }) + } + + await service.restoreCheckpoint(checkpoint!.commit) + + for (const dir of ["empty", "mixed/empty", "tracked/empty"]) { + expect(await fileExistsAtPath(path.join(service.workspaceDir, dir)), dir).toBe(false) + } + for (const dir of ["ignored-empty", "current-empty"]) { + expect(await fileExistsAtPath(path.join(service.workspaceDir, dir)), dir).toBe(true) + } + expect(await fs.readFile(path.join(service.workspaceDir, "mixed", "keep.txt"), "utf8")).toBe( + "protected", + ) + expect(await fs.readFile(path.join(service.workspaceDir, "tracked", "keep.txt"), "utf8")).toBe( + "tracked", + ) }) }) From f5c88a4c3fb9c7593f4019e08bae217c3d117aad Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 5 Oct 2026 19:14:53 +0200 Subject: [PATCH 3/3] fix(checkpoints): fail safely when restore metadata is unavailable Amp-Thread-ID: https://ampcode.com/threads/T-01a10cc3-928d-7028-bd4b-83dcc6816f1b Co-authored-by: Amp --- .../checkpoints/ShadowCheckpointService.ts | 63 +++++--- .../__tests__/ShadowCheckpointService.spec.ts | 145 ++++++++++++++++-- 2 files changed, 181 insertions(+), 27 deletions(-) diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index e72912f338..80a401cc9a 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -16,6 +16,9 @@ import { t } from "../../i18n" import { CheckpointDiff, CheckpointResult, CheckpointEventMap } from "./types" import { getExcludePatterns } from "./excludes" +// Stored in the commit itself so an interrupted sidecar write cannot look like a legacy checkpoint. +const IGNORED_RECORD_HEADER = "Zoo-Checkpoint-Ignored: v1" + /** * Environment variables stripped before passing the env to simple-git. * @@ -216,7 +219,7 @@ export abstract class ShadowCheckpointService extends EventEmitter { await git.addConfig("user.email", "noreply@example.com") await this.writeExcludeFile() await this.stageAll(git) - const { commit } = await git.commit("initial commit", { "--allow-empty": null }) + const { commit } = await git.commit(`initial commit\n\n${IGNORED_RECORD_HEADER}`, { "--allow-empty": null }) this.baseHash = commit await this.recordIgnoredPaths(git, commit) created = true @@ -277,32 +280,43 @@ export abstract class ShadowCheckpointService extends EventEmitter { return output.split("\0").filter(Boolean) } - private async readIgnoredRecord(commitHash: string): Promise { + private async readIgnoredRecord(commitHash: string, required = false): Promise { + let content: string try { - const content = await fs.readFile(this.ignoredRecordPath(commitHash), "utf8") - return content.split("\0").filter(Boolean) - } catch { - return undefined + content = await fs.readFile(this.ignoredRecordPath(commitHash), "utf8") + } catch (error) { + if (error instanceof Error && "code" in error && error.code === "ENOENT" && !required) return undefined + throw error + } + const prefix = `${IGNORED_RECORD_HEADER}\n` + if (content.startsWith(prefix)) { + const paths: unknown = JSON.parse(content.slice(prefix.length)) + if ( + !Array.isArray(paths) || + !paths.every((entry): entry is string => typeof entry === "string" && !!entry) + ) { + throw new Error(`Invalid ignored-path record for checkpoint ${commitHash}`) + } + return paths } + if (required) throw new Error(`Invalid ignored-path record for checkpoint ${commitHash}`) + // Records from the first implementation were NUL-delimited, without a commit marker. + return content.split("\0").filter(Boolean) } - /** Adds the currently ignored paths to the record for a checkpoint; never fails the save. */ - private async recordIgnoredPaths(git: SimpleGit, commitHash: string) { + /** Publish protection before reporting success; preserve the previous record on failure. */ + private async recordIgnoredPaths(git: SimpleGit, commitHash: string, requireExisting = false) { const recordPath = this.ignoredRecordPath(commitHash) const tempPath = `${recordPath}.${crypto.randomUUID()}.tmp` try { const ignored = new Set([ - ...((await this.readIgnoredRecord(commitHash)) ?? []), + ...((await this.readIgnoredRecord(commitHash, requireExisting)) ?? []), ...(await this.listIgnoredPaths(git)), ]) await fs.mkdir(path.dirname(recordPath), { recursive: true }) // A no-change save reuses the same commit. Never truncate its existing protection. - await fs.writeFile(tempPath, [...ignored].join("\0")) + await fs.writeFile(tempPath, `${IGNORED_RECORD_HEADER}\n${JSON.stringify([...ignored])}`, { flush: true }) await fs.rename(tempPath, recordPath) - } catch (error) { - this.log( - `[${this.constructor.name}#recordIgnoredPaths] failed to record ignored paths: ${error instanceof Error ? error.message : String(error)}`, - ) } finally { await fs.rm(tempPath, { force: true }).catch(() => {}) } @@ -427,11 +441,15 @@ export abstract class ShadowCheckpointService extends EventEmitter { const startTime = Date.now() await this.stageAll(this.git) const commitArgs = options?.allowEmpty ? { "--allow-empty": null } : undefined - const result = await this.git.commit(message, commitArgs) + const result = await this.git.commit(`${message}\n\n${IGNORED_RECORD_HEADER}`, commitArgs) const fromHash = this._checkpoints[this._checkpoints.length - 1] ?? this.baseHash! - const toHash = result.commit || fromHash + const toHash = result.commit || (await this.git.revparse(["HEAD"])) + // A no-op save cannot reconstruct missing checkpoint-time protection from today's ignore rules. + const requireExisting = + !result.commit && + (await this.git.show(["-s", "--format=%B", toHash])).split(/\r?\n/).includes(IGNORED_RECORD_HEADER) + await this.recordIgnoredPaths(this.git, toHash, requireExisting) this._checkpoints.push(toHash) - await this.recordIgnoredPaths(this.git, toHash) const duration = Date.now() - startTime if (result.commit) { @@ -465,12 +483,21 @@ export abstract class ShadowCheckpointService extends EventEmitter { try { this.log(`[${this.constructor.name}#restoreCheckpoint] starting checkpoint restore`) + // Never use webview input as a path or Git revision expression. + if (!/^(?:[a-f0-9]{40}|[a-f0-9]{64})$/.test(commitHash)) { + throw new Error("Invalid checkpoint commit hash") + } if (!this.git) { throw new Error("Shadow git repo not initialized") } const start = Date.now() - const ignoredAtCheckpoint = await this.readIgnoredRecord(commitHash) + if ((await this.git.raw(["cat-file", "-t", commitHash])).trim() !== "commit") { + throw new Error("Checkpoint hash does not identify a commit") + } + const commitMessage = await this.git.show(["-s", "--format=%B", commitHash]) + const recordRequired = commitMessage.split(/\r?\n/).includes(IGNORED_RECORD_HEADER) + const ignoredAtCheckpoint = await this.readIgnoredRecord(commitHash, recordRequired) if (ignoredAtCheckpoint) { await this.removeUntrackedFiles(this.git, ignoredAtCheckpoint) } else { diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index 967c8dcbb0..54a6a6c428 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -321,6 +321,127 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( }) describe("restore with changed ignore rules (#1832)", () => { + it.each(["../HEAD", "..\\HEAD", "HEAD", "--help", "a".repeat(39), "a".repeat(41)])( + "rejects invalid hash %s before reading files or running Git", + async (hash) => { + service.on("error", vitest.fn()) + const read = vitest.spyOn(fs, "readFile") + const git = service["git"]! + const raw = vitest.spyOn(git, "raw") + const clean = vitest.spyOn(git, "clean") + const reset = vitest.spyOn(git, "reset") + await expect(service.restoreCheckpoint(hash)).rejects.toThrow("Invalid checkpoint commit hash") + expect(read).not.toHaveBeenCalled() + expect(raw).not.toHaveBeenCalled() + expect(clean).not.toHaveBeenCalled() + expect(reset).not.toHaveBeenCalled() + }, + ) + + it.each(["missing", "blob"])("rejects a %s object before cleanup", async (kind) => { + service.on("error", vitest.fn()) + const git = service["git"]! + const hash = kind === "blob" ? await git.revparse(["HEAD:test.txt"]) : "a".repeat(40) + const read = vitest.spyOn(fs, "readFile") + const clean = vitest.spyOn(git, "clean") + const reset = vitest.spyOn(git, "reset") + await expect(service.restoreCheckpoint(hash)).rejects.toThrow() + expect(read).not.toHaveBeenCalled() + expect(clean).not.toHaveBeenCalled() + expect(reset).not.toHaveBeenCalled() + }) + + it("records files ignored before initialization for a baseHash restore", async () => { + const gitignore = path.join(service.workspaceDir, ".gitignore") + const secret = path.join(service.workspaceDir, "initial-secret.txt") + await fs.writeFile(gitignore, ".gitignore\ninitial-secret.txt\n") + await fs.writeFile(secret, "initial protection") + const initial = new klass( + taskId, + `${service.checkpointsDir}-initial`, + service.workspaceDir, + () => {}, + ) + await initial.initShadowGit() + const git = simpleGit(initial.checkpointsDir) + expect(await git.raw(["ls-files", "--", "initial-secret.txt"])).toBe("") + expect((await git.raw(["check-ignore", "initial-secret.txt"])).trim()).toBe("initial-secret.txt") + await fs.writeFile(gitignore, ".gitignore\n") + await fs.writeFile(testFile, "modified") + await initial.restoreCheckpoint(initial.baseHash!) + expect(await fs.readFile(secret, "utf8")).toBe("initial protection") + expect(await fs.readFile(testFile, "utf8")).toBe("Hello, world!") + }) + + it.each(["missing", "truncated", "invalid", "unreadable"])( + "refuses a new checkpoint with a %s record without changing workspace files", + async (failure) => { + service.on("error", vitest.fn()) + const checkpoint = await service.saveCheckpoint("protected", { allowEmpty: true }) + const recordPath = path.join( + service.checkpointsDir, + ".git", + "zoo-checkpoint-ignored", + checkpoint!.commit, + ) + if (failure === "missing") await fs.rm(recordPath) + if (failure === "truncated") await fs.writeFile(recordPath, 'Zoo-Checkpoint-Ignored: v1\n["') + if (failure === "invalid") await fs.writeFile(recordPath, "Zoo-Checkpoint-Ignored: v1\n[7]") + await fs.writeFile(testFile, "must not reset") + const untracked = path.join(service.workspaceDir, "must-not-clean.txt") + await fs.writeFile(untracked, "must not clean") + if (failure === "unreadable") { + vitest + .spyOn(fs, "readFile") + .mockRejectedValueOnce(Object.assign(new Error("denied"), { code: "EACCES" })) + } + await expect(service.restoreCheckpoint(checkpoint!.commit)).rejects.toThrow() + expect(await fs.readFile(testFile, "utf8")).toBe("must not reset") + expect(await fs.readFile(untracked, "utf8")).toBe("must not clean") + }, + ) + + it.each(["initial", "saved"])( + "fails a %s checkpoint with unpublished metadata and refuses restore after restart", + async (phase) => { + const gitignore = path.join(service.workspaceDir, ".gitignore") + const secret = path.join(service.workspaceDir, "secret.txt") + await fs.writeFile(gitignore, ".gitignore\nsecret.txt\n") + await fs.writeFile(secret, "must survive") + if (phase === "initial") { + service = new klass( + taskId, + `${service.checkpointsDir}-failed`, + service.workspaceDir, + () => {}, + ) + } + service.on("error", vitest.fn()) + const checkpointEvent = vitest.fn() + service.on("checkpoint", checkpointEvent) + const rename = vitest + .spyOn(fs, "rename") + .mockRejectedValueOnce(new Error("metadata publish failed")) + await expect( + phase === "initial" + ? service.initShadowGit() + : service.saveCheckpoint("failed", { allowEmpty: true }), + ).rejects.toThrow("metadata publish failed") + rename.mockRestore() + expect(service.getCheckpoints()).toEqual([]) + expect(checkpointEvent).not.toHaveBeenCalled() + const failedHash = await simpleGit(service.checkpointsDir).revparse(["HEAD"]) + service = new klass(taskId, service.checkpointsDir, service.workspaceDir, () => {}) + service.on("error", vitest.fn()) + await service.initShadowGit() + // A no-op save must not silently rebuild the lost record with later observations. + await expect(service.saveCheckpoint("retry without changes")).rejects.toThrow() + await fs.writeFile(gitignore, ".gitignore\n") + await expect(service.restoreCheckpoint(failedHash)).rejects.toThrow() + expect(await fs.readFile(secret, "utf8")).toBe("must survive") + }, + ) + it("keeps files that were ignored at checkpoint time when .gitignore was overwritten", async () => { // The reported setup: .gitignore ignores itself, so it is never in a checkpoint. const gitignore = path.join(service.workspaceDir, ".gitignore") @@ -372,7 +493,7 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( expect(await fileExistsAtPath(path.dirname(createdAfter))).toBe(false) }) - it("restores checkpoints saved before ignore records existed with the previous clean", async () => { + it.each([false, true])("restores a legacy checkpoint (NUL-delimited record: %s)", async (hasRecord) => { const gitignore = path.join(service.workspaceDir, ".gitignore") const secret = path.join(service.workspaceDir, "legacy-secret.txt") await fs.writeFile(gitignore, ".gitignore\nlegacy-secret.txt\n") @@ -383,12 +504,15 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( "legacy-secret.txt", ) await fs.writeFile(testFile, "checkpointed") - const checkpoint = await service.saveCheckpoint("legacy checkpoint") - // Simulate a checkpoint saved by an older version, which wrote no ignore record. - await fs.rm(path.join(service.checkpointsDir, ".git", "zoo-checkpoint-ignored"), { - recursive: true, - force: true, - }) + // An old commit has no metadata-required marker in its message. + await shadowGit.add(".") + const checkpoint = await shadowGit.commit("legacy checkpoint") + if (hasRecord) { + await fs.writeFile( + path.join(service.checkpointsDir, ".git", "zoo-checkpoint-ignored", checkpoint.commit), + ".gitignore\0legacy-secret.txt", + ) + } const createdAfter = path.join(service.workspaceDir, "created-after.txt") await fs.writeFile(createdAfter, "new") await fs.writeFile(gitignore, ".gitignore\n") @@ -398,7 +522,7 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( expect(await fs.readFile(testFile, "utf-8")).toBe("checkpointed") expect(await fileExistsAtPath(createdAfter)).toBe(false) // Unlike recorded protection, legacy git clean only consults the current ignore rules. - expect(await fileExistsAtPath(secret)).toBe(false) + expect(await fileExistsAtPath(secret)).toBe(hasRecord) }) it.each(["write", "rename"] as const)( @@ -423,7 +547,10 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( }) : vitest.spyOn(fs, "rename").mockRejectedValueOnce(new Error("record rename failed")) - await expect(service.saveCheckpoint("no tracked changes")).resolves.toBeUndefined() + service.on("error", vitest.fn()) + await expect(service.saveCheckpoint("no tracked changes")).rejects.toThrow( + `record ${failure} failed`, + ) expect(failureSpy).toHaveBeenCalledOnce() failureSpy.mockRestore() expect(await fs.readFile(recordPath, "utf8")).toBe(previous)