From ffa2db8dcfa9b08f4f24cfdc18ae697db2a2b618 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Wed, 2 Sep 2026 21:15:34 -0400 Subject: [PATCH] fix(core): preserve external files during snapshot restore Plan ancestor restoration before descendant deletion and keep missing paths beneath restored non-directory entries away from filesystem removal. Validate canonical parents for remaining deletions while leaving present entries to native Git checkout. Handle historical ignore queries beneath restored symlinks so Session Undo and Redo finish without manual repair. Cover external-file preservation, repeated and reversed restoration, mixed trees, typed failures, and unchanged user index state. --- packages/core/src/git.ts | 73 +++++--- packages/core/src/snapshot.ts | 31 +++- packages/core/test/session-revert.test.ts | 111 +++++++++--- packages/core/test/snapshot.test.ts | 206 ++++++++++++++++++++++ 4 files changed, 369 insertions(+), 52 deletions(-) diff --git a/packages/core/src/git.ts b/packages/core/src/git.ts index da745e5f3697..3f1db810d30a 100644 --- a/packages/core/src/git.ts +++ b/packages/core/src/git.ts @@ -564,7 +564,7 @@ const layer = Layer.effect( ) }) - const hasEntry = Effect.fnUntraced(function* (repository: Repository, tree: TreeID, file: RelativePath) { + const entryType = Effect.fnUntraced(function* (repository: Repository, tree: TreeID, file: RelativePath) { const text = (yield* repositoryOperation("restore", repository, [ "ls-tree", "-z", @@ -572,42 +572,67 @@ const layer = Layer.effect( "--", file, ])).text.replace(/\0$/, "") - if (!text) return false - if (!/^\d+\s+\w+\s+[0-9a-f]+\t/.test(text)) + if (!text) return undefined + const entry = /^\d+\s+(\w+)\s+[0-9a-f]+\t/.exec(text) + if (!entry) return yield* new OperationError({ operation: "restore", directory: repository.worktree, message: `Invalid tree entry for ${file}`, }) - return true + return entry[1] }) const restore = Effect.fn("Git.tree.restore")( (input: { repository: Repository; files: ReadonlyMap }) => locked( input.repository, - Effect.forEach( - input.files, - ([file, tree]) => - Effect.gen(function* () { - if (yield* hasEntry(input.repository, tree, file)) { - yield* repositoryOperation("restore", input.repository, ["checkout", tree, "--", file]) - return - } - yield* fs.remove(path.join(input.repository.worktree, file), { recursive: true, force: true }).pipe( - Effect.mapError( - (cause) => - new OperationError({ + Effect.gen(function* () { + const entries = yield* Effect.forEach(input.files, ([file, tree]) => + Effect.map(entryType(input.repository, tree, file), (type) => ({ file, tree, type })), + ) + const ordered = entries.toSorted((a, b) => a.file.split("/").length - b.file.split("/").length) + yield* Effect.forEach( + ordered, + (entry) => + Effect.gen(function* () { + if (entry.type) { + yield* repositoryOperation("restore", input.repository, ["checkout", entry.tree, "--", entry.file]) + return + } + // A restored symlink, file, or missing ancestor has no project-owned descendants to delete. + const ancestor = ordered.findLast((parent) => entry.file.startsWith(`${parent.file}/`)) + if (ancestor && ancestor.type !== "tree") return + const absolute = path.join(input.repository.worktree, entry.file) + yield* Effect.gen(function* () { + const parent = yield* fs + .realPath(path.dirname(absolute)) + .pipe(Effect.catchReason("PlatformError", "NotFound", () => Effect.undefined)) + if (parent === undefined) return + const worktree = yield* fs.realPath(input.repository.worktree) + if (!FSUtil.contains(worktree, parent)) + return yield* new OperationError({ operation: "restore", directory: input.repository.worktree, - message: `Failed to remove ${file}`, - cause, - }), - ), - ) - }), - { discard: true }, - ), + message: `Path escapes the project: ${entry.file}`, + }) + yield* fs.remove(absolute, { recursive: true, force: true }) + }).pipe( + Effect.catchTag("PlatformError", (cause) => + Effect.fail( + new OperationError({ + operation: "restore", + directory: input.repository.worktree, + message: `Failed to remove ${entry.file}`, + cause, + }), + ), + ), + ) + }), + { discard: true }, + ) + }), ), ) diff --git a/packages/core/src/snapshot.ts b/packages/core/src/snapshot.ts index 25e79d29e095..a96148cd8901 100644 --- a/packages/core/src/snapshot.ts +++ b/packages/core/src/snapshot.ts @@ -139,9 +139,34 @@ const layer = Layer.effect( to: Git.TreeID.make(input.to), } const files = yield* git.tree.files(comparison).pipe(Effect.mapError((cause) => failure(operation, cause))) - const ignored = yield* git.index - .ignored({ repository: repo.source, paths: files }) - .pipe(Effect.mapError((cause) => failure(operation, cause))) + const ignored = yield* git.index.ignored({ repository: repo.source, paths: files }).pipe( + Effect.catch((cause) => + Effect.gen(function* () { + // Git cannot check historical descendants below a current symlink; check the link itself instead. + const paths = yield* Effect.forEach(files, (file) => + Effect.gen(function* () { + const parts = file.split("/") + for (let index = 1; index < parts.length; index++) { + const parent = RelativePath.make(parts.slice(0, index).join("/")) + const symlink = yield* fs.readLink(path.join(repo.worktree, parent)).pipe( + Effect.as(true), + Effect.orElseSucceed(() => false), + ) + if (symlink) return { file, query: parent } + } + return { file, query: file } + }), + ) + if (paths.every((entry) => entry.file === entry.query)) return yield* Effect.fail(cause) + const ignored = yield* git.index.ignored({ + repository: repo.source, + paths: Array.from(new Set(paths.map((entry) => entry.query))), + }) + return new Set(paths.filter((entry) => ignored.has(entry.query)).map((entry) => entry.file)) + }), + ), + Effect.mapError((cause) => failure(operation, cause)), + ) return { input: comparison, files, diff --git a/packages/core/test/session-revert.test.ts b/packages/core/test/session-revert.test.ts index 629a5f3693e9..51cae80b5d49 100644 --- a/packages/core/test/session-revert.test.ts +++ b/packages/core/test/session-revert.test.ts @@ -64,31 +64,10 @@ describe("Session.revert files", () => { yield* SessionInbox.promote(database.db, bus, created.id, "steer") yield* Effect.gen(function* () { - const plugins = yield* Plugin.Service - yield* plugins.awaitActivation - const snapshot = yield* Snapshot.Service - const before = yield* snapshot.capture() - if (!before) throw new Error("Initial snapshot missing") - const assistantMessageID = SessionMessage.ID.create() - yield* bus.publish(SessionEvent.Step.Started, { - sessionID: created.id, - assistantMessageID, - agent: Agent.defaultID, - model: { id: Model.ID.make("test-model"), providerID: Provider.ID.make("test-provider") }, - snapshot: before, - }) - yield* Effect.promise(() => fs.rename(original, renamed)) - const after = yield* snapshot.capture() - if (!after) throw new Error("Renamed snapshot missing") - yield* bus.publish(SessionEvent.Step.Ended, { - sessionID: created.id, - assistantMessageID, - finish: "stop", - cost: Money.USD.zero, - tokens: { input: 0, output: 0, reasoning: 0, cache: { read: 0, write: 0 } }, - snapshot: after, - files: yield* snapshot.files({ from: before, to: after }), - }) + yield* recordStep( + created.id, + Effect.promise(() => fs.rename(original, renamed)), + ) yield* Effect.promise(() => Bun.write(path.join(directory, "unrelated.txt"), "Keep this later edit.\n")) const reverted = yield* session.revert.stage({ sessionID: created.id, messageID: prompt.id }) @@ -117,4 +96,86 @@ describe("Session.revert files", () => { // Real Location/plugin startup and Git snapshots can exceed five seconds under CI load. { timeout: 15_000 }, ) + + it.live( + "undoes and redoes a symlink replacement without changing its external target", + () => + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + const directory = path.join(tmp.path, "project") + const external = path.join(tmp.path, "external") + const assets = path.join(directory, "assets") + yield* Effect.promise(async () => { + await fs.mkdir(directory) + await fs.mkdir(external) + await Bun.write(path.join(external, "logo.svg"), "External content must survive.\n") + await fs.symlink(external, assets, "dir") + await $`git init -q`.cwd(directory).quiet() + await $`git -c core.fsmonitor=false add .`.cwd(directory).quiet() + }) + + const session = yield* Session.Service + const database = yield* Database.Service + const bus = yield* Bus.Service + const created = yield* session.create({ location: { directory: AbsolutePath.make(directory) } }) + const prompt = yield* session.prompt({ sessionID: created.id, text: "Replace the symlink", resume: false }) + yield* SessionInbox.promote(database.db, bus, created.id, "steer") + + yield* Effect.gen(function* () { + yield* recordStep( + created.id, + Effect.promise(async () => { + await fs.unlink(assets) + await fs.mkdir(assets) + await Bun.write(path.join(assets, "logo.svg"), "Project content.\n") + }), + ) + // Repeating Undo must remain safe even once its symlink has already been restored. + yield* session.revert.stage({ sessionID: created.id, messageID: prompt.id }) + yield* session.revert.stage({ sessionID: created.id, messageID: prompt.id }) + expect(yield* Effect.promise(() => fs.readlink(assets))).toBe(external) + expect(yield* Effect.promise(() => Bun.file(path.join(external, "logo.svg")).text())).toBe( + "External content must survive.\n", + ) + + yield* session.revert.clear(created.id) + expect(yield* Effect.promise(async () => (await fs.lstat(assets)).isDirectory())).toBe(true) + expect(yield* Effect.promise(() => Bun.file(path.join(assets, "logo.svg")).text())).toBe("Project content.\n") + expect(yield* Effect.promise(() => Bun.file(path.join(external, "logo.svg")).text())).toBe( + "External content must survive.\n", + ) + expect((yield* session.get(created.id)).revert).toBeUndefined() + }).pipe(Effect.provide(LocationServiceMap.Service.get(created.location))) + }), + { timeout: 15_000 }, + ) +}) + +const recordStep = Effect.fnUntraced(function* (sessionID: Session.ID, change: Effect.Effect) { + const plugins = yield* Plugin.Service + yield* plugins.awaitActivation + const snapshot = yield* Snapshot.Service + const bus = yield* Bus.Service + const before = yield* snapshot.capture() + if (!before) throw new Error("Initial snapshot missing") + const assistantMessageID = SessionMessage.ID.create() + yield* bus.publish(SessionEvent.Step.Started, { + sessionID, + assistantMessageID, + agent: Agent.defaultID, + model: { id: Model.ID.make("test-model"), providerID: Provider.ID.make("test-provider") }, + snapshot: before, + }) + yield* change + const after = yield* snapshot.capture() + if (!after) throw new Error("Changed snapshot missing") + yield* bus.publish(SessionEvent.Step.Ended, { + sessionID, + assistantMessageID, + finish: "stop", + cost: Money.USD.zero, + tokens: { input: 0, output: 0, reasoning: 0, cache: { read: 0, write: 0 } }, + snapshot: after, + files: yield* snapshot.files({ from: before, to: after }), + }) }) diff --git a/packages/core/test/snapshot.test.ts b/packages/core/test/snapshot.test.ts index 9e1b03956acf..42ae6ca3882b 100644 --- a/packages/core/test/snapshot.test.ts +++ b/packages/core/test/snapshot.test.ts @@ -14,6 +14,212 @@ import { tmpdir } from "./fixture/tmpdir" import { testEffect } from "./lib/effect" describe("Snapshot", () => { + for (const reverse of [false, true]) { + testEffect(Layer.empty).live( + `restores symlink and directory round trips without escaping (reverse=${reverse})`, + () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => + Effect.gen(function* () { + const project = path.join(tmp.path, "project") + const external = path.join(tmp.path, "external") + const assets = path.join(project, "assets") + yield* Effect.promise(async () => { + await fs.mkdir(project) + await fs.mkdir(external) + await Bun.write(path.join(external, "logo.svg"), "External content must survive.\n") + await fs.symlink(external, assets, "dir") + await initGit(project) + }) + const index = yield* Effect.promise(() => Bun.file(path.join(project, ".git", "index")).bytes()) + yield* Effect.gen(function* () { + const snapshot = yield* Snapshot.Service + const before = yield* snapshot.capture() + if (!before) throw new Error("Initial snapshot missing") + yield* Effect.promise(async () => { + await fs.unlink(assets) + await fs.mkdir(assets) + await Bun.write(path.join(assets, "logo.svg"), "Project content.\n") + }) + const after = yield* snapshot.capture() + if (!after) throw new Error("Changed snapshot missing") + const changed = yield* snapshot.files({ from: before, to: after }) + expect(changed).toEqual([RelativePath.make("assets"), RelativePath.make("assets/logo.svg")]) + const files = reverse ? changed.toReversed() : changed + + for (let attempt = 0; attempt < 2; attempt++) { + yield* snapshot.restore({ files: new Map(files.map((file) => [file, before])) }) + expect(yield* Effect.promise(() => Bun.file(path.join(external, "logo.svg")).exists())).toBe(true) + expect(yield* read(path.join(external, "logo.svg"))).toBe("External content must survive.\n") + expect(yield* Effect.promise(() => fs.readlink(assets))).toBe(external) + } + + for (let attempt = 0; attempt < 2; attempt++) { + yield* snapshot.restore({ files: new Map(files.map((file) => [file, after])) }) + expect(yield* read(path.join(external, "logo.svg"))).toBe("External content must survive.\n") + expect(yield* Effect.promise(async () => (await fs.lstat(assets)).isDirectory())).toBe(true) + expect(yield* read(path.join(assets, "logo.svg"))).toBe("Project content.\n") + } + expect(yield* Effect.promise(() => Bun.file(path.join(project, ".git", "index")).bytes())).toEqual( + index, + ) + }).pipe(Effect.provide(snapshotLayer(tmp.path, project))) + }), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ), + ) + } + + for (const present of [false, true]) { + testEffect(Layer.empty).live(`does not follow an existing external symlink (present=${present})`, () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => + Effect.gen(function* () { + const project = path.join(tmp.path, "project") + const external = path.join(tmp.path, "external") + const assets = path.join(project, "assets") + yield* Effect.promise(async () => { + await fs.mkdir(assets, { recursive: true }) + await fs.mkdir(external) + await Bun.write(path.join(assets, "logo.svg"), "Project content.\n") + await Bun.write(path.join(external, "logo.svg"), "External content must survive.\n") + await initGit(project) + }) + yield* Effect.gen(function* () { + const snapshot = yield* Snapshot.Service + const before = yield* snapshot.capture() + if (!before) throw new Error("Initial snapshot missing") + yield* Effect.promise(() => fs.unlink(path.join(assets, "logo.svg"))) + const after = yield* snapshot.capture() + if (!after) throw new Error("Changed snapshot missing") + yield* Effect.promise(async () => { + await fs.rmdir(assets) + await fs.symlink(external, assets, "dir") + }) + + const result = yield* snapshot + .restore({ files: new Map([[RelativePath.make("assets/logo.svg"), present ? before : after]]) }) + .pipe(Effect.exit) + expect(yield* Effect.promise(() => Bun.file(path.join(external, "logo.svg")).exists())).toBe(true) + expect(yield* read(path.join(external, "logo.svg"))).toBe("External content must survive.\n") + if (present) { + yield* result + expect(yield* read(path.join(assets, "logo.svg"))).toBe("Project content.\n") + expect(yield* Effect.promise(async () => (await fs.lstat(assets)).isDirectory())).toBe(true) + return + } + const error = yield* result.pipe(Effect.flip) + expect(error).toBeInstanceOf(Snapshot.Error) + expect(error).toMatchObject({ + operation: "restore", + message: "Path escapes the project: assets/logo.svg", + }) + expect(yield* Effect.promise(() => fs.readlink(assets))).toBe(external) + }).pipe(Effect.provide(snapshotLayer(tmp.path, project))) + }), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ), + ) + } + + testEffect(Layer.empty).live("uses the closest restored ancestor for mixed-tree deletions", () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => + Effect.gen(function* () { + const project = path.join(tmp.path, "project") + const external = path.join(tmp.path, "external") + const assets = path.join(project, "assets") + yield* Effect.promise(async () => { + await fs.mkdir(project) + await fs.mkdir(external) + await Bun.write(path.join(external, "logo.svg"), "External content must survive.\n") + await fs.symlink(external, assets, "dir") + await initGit(project) + }) + yield* Effect.gen(function* () { + const snapshot = yield* Snapshot.Service + const link = yield* snapshot.capture() + if (!link) throw new Error("Symlink snapshot missing") + yield* Effect.promise(async () => { + await fs.unlink(assets) + await fs.mkdir(path.join(assets, "nested"), { recursive: true }) + await Bun.write(path.join(assets, "nested", "logo.svg"), "Project content.\n") + }) + const directory = yield* snapshot.capture() + if (!directory) throw new Error("Directory snapshot missing") + yield* Effect.promise(() => fs.unlink(path.join(assets, "nested", "logo.svg"))) + const removed = yield* snapshot.capture() + if (!removed) throw new Error("Removed-file snapshot missing") + + yield* snapshot.restore({ + files: new Map([ + [RelativePath.make("assets/nested/logo.svg"), removed], + [RelativePath.make("assets/nested"), directory], + [RelativePath.make("assets"), link], + ]), + }) + expect(yield* Effect.promise(async () => (await fs.lstat(path.join(assets, "nested"))).isDirectory())).toBe( + true, + ) + expect(yield* Effect.promise(() => Bun.file(path.join(assets, "nested", "logo.svg")).exists())).toBe(false) + expect(yield* read(path.join(external, "logo.svg"))).toBe("External content must survive.\n") + }).pipe(Effect.provide(snapshotLayer(tmp.path, project))) + }), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ), + ) + + testEffect(Layer.empty).live("checks symlink ignore rules without hiding fatal Git errors", () => + Effect.acquireUseRelease( + Effect.promise(() => tmpdir()), + (tmp) => + Effect.gen(function* () { + const project = path.join(tmp.path, "project") + const external = path.join(tmp.path, "external") + const assets = path.join(project, "assets") + yield* Effect.promise(async () => { + await fs.mkdir(project) + await fs.mkdir(external) + await fs.symlink(external, assets, "dir") + await initGit(project) + }) + yield* Effect.gen(function* () { + const snapshot = yield* Snapshot.Service + const before = yield* snapshot.capture() + if (!before) throw new Error("Initial snapshot missing") + yield* Effect.promise(async () => { + await fs.unlink(assets) + await fs.mkdir(assets) + await Bun.write(path.join(assets, "logo.svg"), "Project content.\n") + }) + const after = yield* snapshot.capture() + if (!after) throw new Error("Directory snapshot missing") + yield* snapshot.restore({ + files: new Map([ + [RelativePath.make("assets"), before], + [RelativePath.make("assets/logo.svg"), before], + ]), + }) + expect(yield* snapshot.files({ from: after, to: before })).toEqual([ + RelativePath.make("assets"), + RelativePath.make("assets/logo.svg"), + ]) + yield* Effect.promise(() => Bun.write(path.join(project, ".gitignore"), "assets\n")) + expect(yield* snapshot.files({ from: after, to: before })).toEqual([]) + yield* Effect.promise(() => Bun.write(path.join(project, ".git", "config"), "[broken\n")) + const error = yield* snapshot.files({ from: after, to: before }).pipe(Effect.flip) + expect(error).toBeInstanceOf(Snapshot.Error) + expect(error.operation).toBe("files") + expect(error.message).toContain("bad config line") + }).pipe(Effect.provide(snapshotLayer(tmp.path, project))) + }), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ), + ) + testEffect(Layer.empty).live("keeps lazy repository discovery after the first caller is interrupted", () => Effect.acquireUseRelease( Effect.promise(() => tmpdir()),