diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index 3e3d3d0653..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,8 +219,9 @@ 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 } @@ -263,6 +267,99 @@ 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, required = false): Promise { + let content: string + try { + 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) + } + + /** 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, 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_RECORD_HEADER}\n${JSON.stringify([...ignored])}`, { flush: true }) + await fs.rename(tempPath, recordPath) + } finally { + await fs.rm(tempPath, { force: true }).catch(() => {}) + } + } + + /** + * 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, ...(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"]) + 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("/") }) + } + + // --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) + } + } + private async getNestedGitRepository(): Promise { try { // Find all .git/HEAD files that are not at the root level. @@ -344,9 +441,14 @@ 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) const duration = Date.now() - startTime @@ -381,12 +483,27 @@ 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() - await this.git.clean("f", ["-d", "-f"]) + 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 { + // 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..54a6a6c428 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -320,6 +320,280 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( await expect(fs.readFile(untrackedFile, "utf-8")).rejects.toThrow() }) + 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") + 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 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. + 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 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() + + 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.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") + 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") + // 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") + + 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(hasRecord) + }) + + 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")) + + 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) + 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", + ) + }) + }) + 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")