diff --git a/src/github/operations/restore-config.ts b/src/github/operations/restore-config.ts index bcacc5bb..f915c872 100644 --- a/src/github/operations/restore-config.ts +++ b/src/github/operations/restore-config.ts @@ -3,11 +3,16 @@ import { appendFileSync, cpSync, existsSync, + lstatSync, mkdirSync, readFileSync, + readlinkSync, + realpathSync, rmSync, + statSync, + writeFileSync, } from "fs"; -import { dirname } from "path"; +import { dirname, join, posix, relative, sep } from "path"; // Paths that are both PR-controllable and read from cwd at CLI startup. // @@ -30,18 +35,173 @@ const SENSITIVE_PATHS = [ const CLAUDE_PR_EXCLUDE_PATTERN = "/.claude-pr/"; -function snapshotSensitivePath(src: string, dest: string): void { - try { - cpSync(src, dest, { recursive: true, dereference: true }); - } catch (error) { - // Symlinks whose targets are absent on the PR head (e.g. `.claude/CLAUDE.md` - // -> `../AGENTS.md` when the PR deleted the target) make dereferenced - // copies throw ENOENT. Preserve the symlink for the review snapshot instead. - if (error instanceof Error && "code" in error && error.code === "ENOENT") { - cpSync(src, dest, { recursive: true }); - return; +function isSameOrInside(child: string, parent: string): boolean { + return child === parent || child.startsWith(`${parent}${sep}`); +} + +// Repository paths (relative to cwd, `/`-separated) that a link may resolve +// to: `files` are tracked files whose working-tree content is unchanged from +// HEAD, `dirs` are directories that contain at least one tracked file. +type TrackedPaths = { files: Set; dirs: Set }; + +// Built from the superproject only: `git ls-files` reports a submodule as a +// single entry, so paths inside a checked-out submodule are in neither set and +// links into one are recorded as placeholders. +function listTrackedPaths(): TrackedPaths { + const gitPathList = (args: string[]) => + execFileSync("git", args, { encoding: "utf8", maxBuffer: Infinity }) + .split("\0") + .filter(Boolean); + const modified = new Set( + gitPathList([ + "diff", + "--name-only", + "-z", + "--relative", + "--ignore-submodules", + "HEAD", + "--", + ]), + ); + const tracked: TrackedPaths = { files: new Set(), dirs: new Set() }; + for (const file of gitPathList(["ls-files", "-z"])) { + if (!modified.has(file)) { + tracked.files.add(file); } - throw error; + for ( + let dir = posix.dirname(file); + dir !== "." && !tracked.dirs.has(dir); + dir = posix.dirname(dir) + ) { + tracked.dirs.add(dir); + } + } + return tracked; +} + +// The snapshot is scoped to tracked repository content and never contains +// links. An entry is copied with its content only when all of these hold: +// 1. its real target (through any links) lies inside the working tree; +// 2. no component of the target's path inside the tree is `.git`, and the +// target is not inside the snapshot directory itself; +// 3. the target does not contain a directory already on the entry's own +// path (which would recurse); +// 4. if the entry is reached through a link (it is one, or a directory above +// it inside the sensitive path is), a file target must be tracked in the +// checkout with its content unchanged from HEAD, and a directory target +// must contain at least one tracked file (see listTrackedPaths). Directory +// targets that pass are descended into and their children are checked +// individually. +// Files and directories at their literal, non-linked location are unaffected +// by rule 4 and are copied as-is. Every other entry — targets outside the +// tree, dangling or looping links, git metadata, submodule contents, untracked +// or locally modified files reached through a link — is recorded as a +// placeholder file (see recordPlaceholder), so nothing in the snapshot +// resolves anywhere else. +function shouldSnapshotContent( + entryPath: string, + workTreeRealPath: string, + tracked: TrackedPaths, +): boolean { + try { + const targetRealPath = realpathSync(entryPath); + if (!isSameOrInside(targetRealPath, workTreeRealPath)) { + return false; + } + const targetParts = relative(workTreeRealPath, targetRealPath).split(sep); + if (targetParts.includes(".git") || targetParts[0] === ".claude-pr") { + return false; + } + for (let dir = dirname(entryPath); ; dir = dirname(dir)) { + if (isSameOrInside(realpathSync(dir), targetRealPath)) { + return false; + } + if (dir === dirname(dir)) { + break; + } + } + const literalPath = join( + workTreeRealPath, + relative(process.cwd(), entryPath), + ); + if (targetRealPath === literalPath) { + return true; + } + const targetRepoPath = targetParts.join("/"); + return statSync(targetRealPath).isDirectory() + ? tracked.dirs.has(targetRepoPath) + : tracked.files.has(targetRepoPath); + } catch { + return false; + } +} + +// Writes a short regular file at `dest` describing the entry that was left +// out, so the snapshot records that something was there without linking to it. +function recordPlaceholder(src: string, dest: string): void { + console.warn( + `Snapshot: ${src} not included in snapshot; recording a placeholder`, + ); + let description = "is not included in this snapshot"; + try { + description = `was a symbolic link to ${JSON.stringify(readlinkSync(src))}; the link target is not included in this snapshot`; + } catch { + // Not a link (or no longer present). + } + mkdirSync(dirname(dest), { recursive: true }); + writeFileSync(dest, `Snapshot placeholder: ${src} ${description}.\n`); +} + +/** + * Copies a sensitive path into the review snapshot. Entries that pass the + * check above are copied dereferenced (reviewers see the effective content); + * every other entry is recorded as a placeholder file, never as a link. + * Applies per entry, including links nested inside a real directory. + */ +function snapshotSensitivePath( + src: string, + dest: string, + workTreeRealPath: string, + tracked: TrackedPaths, +): void { + const excluded: Array<{ src: string; dest: string }> = []; + const keepOrExclude = + (keep: (entry: string) => boolean) => + (entrySrc: string, entryDest: string) => { + if (keep(entrySrc)) { + return true; + } + excluded.push({ src: entrySrc, dest: entryDest }); + return false; + }; + try { + cpSync(src, dest, { + recursive: true, + dereference: true, + filter: keepOrExclude((entry) => + shouldSnapshotContent(entry, workTreeRealPath, tracked), + ), + }); + } catch (error) { + // Dangling links are normally caught by the filter above. If a target + // disappears between that check and the copy, the dereferencing copy + // throws ENOENT; start over without following links, recording every link + // as a placeholder, instead of failing the restore. + if ( + !(error instanceof Error && "code" in error && error.code === "ENOENT") + ) { + throw error; + } + rmSync(dest, { recursive: true, force: true }); + excluded.length = 0; + cpSync(src, dest, { + recursive: true, + filter: keepOrExclude((entry) => !lstatSync(entry).isSymbolicLink()), + }); + } + + for (const entry of excluded) { + recordPlaceholder(entry.src, entry.dest); } } @@ -110,11 +270,15 @@ export function restoreConfigFromBase(baseBranch: string): void { // Snapshot every PR-authored sensitive path into .claude-pr/ before deletion // so review agents can inspect what the PR changes without those files ever // being executed. Captured before the security delete so it reflects the - // PR-authored version. + // PR-authored version. Links are followed only to tracked, unmodified content + // inside the working tree; anything else is recorded as a placeholder file, + // so the snapshot itself never contains links. rmSync(".claude-pr", { recursive: true, force: true }); + const workTreeRealPath = realpathSync(process.cwd()); + const tracked = listTrackedPaths(); for (const p of SENSITIVE_PATHS) { - if (existsSync(p)) { - snapshotSensitivePath(p, `.claude-pr/${p}`); + if (lstatSync(p, { throwIfNoEntry: false })) { + snapshotSensitivePath(p, `.claude-pr/${p}`, workTreeRealPath, tracked); } } if (existsSync(".claude-pr")) { diff --git a/test/restore-config.test.ts b/test/restore-config.test.ts index 43dbf751..c63eb4f6 100644 --- a/test/restore-config.test.ts +++ b/test/restore-config.test.ts @@ -5,6 +5,7 @@ import { lstatSync, mkdtempSync, mkdirSync, + readdirSync, readFileSync, rmSync, symlinkSync, @@ -147,7 +148,7 @@ describe("restoreConfigFromBase", () => { ); }); - test("snapshots symlinked sensitive paths even when the PR head target is missing", () => { + test("records dangling links as placeholders, including top-level ones", () => { setupSymlinkedMainBranch(); git(["checkout", "pr"]); @@ -157,14 +158,229 @@ describe("restoreConfigFromBase", () => { restoreConfigFromBase("main"); - expect(lstatRepoFile(".claude-pr/.claude/CLAUDE.md").isSymbolicLink()).toBe( - true, - ); + expectPlaceholder(".claude-pr/CLAUDE.md"); + expectPlaceholder(".claude-pr/.claude/CLAUDE.md"); + expectNoLinksInSnapshot(); expect(readRepoFile(".claude/settings.json")).toBe( `${JSON.stringify({ source: "base" })}\n`, ); }); + test("snapshots links to tracked in-tree files as dereferenced content", () => { + setupSymlinkedMainBranch(); + + git(["checkout", "pr"]); + + restoreConfigFromBase("main"); + + expect(lstatRepoFile(".claude-pr/CLAUDE.md").isFile()).toBe(true); + expect(lstatRepoFile(".claude-pr/.claude/CLAUDE.md").isFile()).toBe(true); + expect(readRepoFile(".claude-pr/CLAUDE.md")).toBe( + "shared agent instructions\n", + ); + expect(readRepoFile(".claude-pr/.claude/CLAUDE.md")).toBe( + "shared agent instructions\n", + ); + expectNoLinksInSnapshot(); + }); + + test("records CLAUDE.md links to targets outside the working tree as placeholders", () => { + const outsideFile = writeOutsideFile("notes.md", "outside notes\n"); + + rmSync(join(repoDir, "CLAUDE.md"), { force: true }); + symlinkRepoFile("CLAUDE.md", outsideFile); + git(["add", "-A"]); + git(["commit", "-m", "pr links CLAUDE.md outside the repo"]); + + restoreConfigFromBase("main"); + + expectPlaceholder(".claude-pr/CLAUDE.md"); + expect(readRepoFile(".claude-pr/CLAUDE.md")).not.toBe("outside notes\n"); + expect(snapshotRegularFileContents()).not.toContain("outside notes\n"); + expectNoLinksInSnapshot(); + expect(readRepoFile("CLAUDE.md")).toBe("base claude instructions\n"); + }); + + test("records nested links to targets outside the working tree as placeholders", () => { + const outsideFile = writeOutsideFile( + "secret.txt", + "outside file content\n", + ); + writeOutsideFile("dir/inner.txt", "outside dir content\n"); + const outsideDir = join(tempDir, "outside", "dir"); + + symlinkRepoFile(".claude/linked-file.md", outsideFile); + symlinkRepoFile(".claude/linked-dir", outsideDir); + git(["add", "-A"]); + git(["commit", "-m", "pr adds nested links outside the repo"]); + + restoreConfigFromBase("main"); + + expect(readRepoFile(".claude-pr/.claude/settings.json")).toBe( + `${JSON.stringify({ source: "pr" })}\n`, + ); + expectPlaceholder(".claude-pr/.claude/linked-file.md"); + expectPlaceholder(".claude-pr/.claude/linked-dir"); + const contents = snapshotRegularFileContents(); + expect(contents).not.toContain("outside file content\n"); + expect(contents).not.toContain("outside dir content\n"); + expectNoLinksInSnapshot(); + expect(readRepoFile(".claude/settings.json")).toBe( + `${JSON.stringify({ source: "base" })}\n`, + ); + }); + + test("records links into git metadata as placeholders", () => { + symlinkRepoFile(".claude/git-config", "../.git/config"); + git(["add", "-A"]); + git(["commit", "-m", "pr links into git metadata"]); + + restoreConfigFromBase("main"); + + expectPlaceholder(".claude-pr/.claude/git-config"); + expect(snapshotRegularFileContents()).not.toContain( + readRepoFile(".git/config"), + ); + expectNoLinksInSnapshot(); + }); + + test("records relative links that only resolve from inside the snapshot as placeholders", () => { + // Both targets dangle at their source location but would resolve to the + // repository's .git/config if re-created one directory deeper. + symlinkRepoFile(".claude/x", "../../.git/config"); + rmSync(join(repoDir, "CLAUDE.md"), { force: true }); + symlinkRepoFile("CLAUDE.md", "../.git/config"); + git(["add", "-A"]); + git(["commit", "-m", "pr adds relative links"]); + + restoreConfigFromBase("main"); + + const gitConfig = readRepoFile(".git/config"); + for (const path of [".claude-pr/.claude/x", ".claude-pr/CLAUDE.md"]) { + expectPlaceholder(path); + expect(readRepoFile(path)).not.toBe(gitConfig); + } + expect(snapshotRegularFileContents()).not.toContain(gitConfig); + expectNoLinksInSnapshot(); + }); + + test("records links into nested git metadata inside the working tree as placeholders", () => { + writeRepoFile("other/.git/config", "nested checkout config\n"); + symlinkRepoFile(".claude/x", "../other/.git/config"); + + restoreConfigFromBase("main"); + + expectPlaceholder(".claude-pr/.claude/x"); + expect(snapshotRegularFileContents()).not.toContain( + "nested checkout config\n", + ); + expectNoLinksInSnapshot(); + }); + + test("records links to untracked in-tree files as placeholders", () => { + writeRepoFile(".env", "untracked env contents\n"); + symlinkRepoFile(".claude/env", "../.env"); + git(["add", ".claude/env"]); + git(["commit", "-m", "pr links to an untracked file"]); + + restoreConfigFromBase("main"); + + expectPlaceholder(".claude-pr/.claude/env"); + expect(snapshotRegularFileContents()).not.toContain( + "untracked env contents\n", + ); + expect(readRepoFile(".claude-pr/.claude/settings.json")).toBe( + `${JSON.stringify({ source: "pr" })}\n`, + ); + expectNoLinksInSnapshot(); + }); + + test("records links to tracked files modified after checkout as placeholders", () => { + writeRepoFile(".env", "PLACEHOLDER=1\n"); + symlinkRepoFile(".claude/env", "../.env"); + git(["add", ".env", ".claude/env"]); + git(["commit", "-m", "pr links to a tracked file"]); + writeRepoFile(".env", "written after checkout\n"); + + restoreConfigFromBase("main"); + + expectPlaceholder(".claude-pr/.claude/env"); + expect(snapshotRegularFileContents()).not.toContain( + "written after checkout\n", + ); + expectNoLinksInSnapshot(); + }); + + test("snapshots a sensitive path that links to a tracked in-tree directory", () => { + rmSync(join(repoDir, ".claude"), { recursive: true, force: true }); + writeRepoFile( + "config/claude/settings.json", + `${JSON.stringify({ source: "linked-dir" })}\n`, + ); + writeRepoFile("config/claude/agents/reviewer.md", "reviewer agent\n"); + writeRepoFile("docs/agents/writer.md", "writer agent\n"); + symlinkRepoFile("config/claude/more-agents", "../../docs/agents"); + symlinkRepoFile(".claude", "config/claude"); + git(["add", "-A"]); + git(["commit", "-m", "pr links .claude to a tracked directory"]); + writeRepoFile("config/claude/local.txt", "untracked file\n"); + writeRepoFile("config/claude/cache/entry.txt", "untracked dir entry\n"); + + restoreConfigFromBase("main"); + + expect(lstatRepoFile(".claude-pr/.claude").isDirectory()).toBe(true); + expect(readRepoFile(".claude-pr/.claude/settings.json")).toBe( + `${JSON.stringify({ source: "linked-dir" })}\n`, + ); + expect(readRepoFile(".claude-pr/.claude/agents/reviewer.md")).toBe( + "reviewer agent\n", + ); + expect(readRepoFile(".claude-pr/.claude/more-agents/writer.md")).toBe( + "writer agent\n", + ); + expectPlaceholder(".claude-pr/.claude/local.txt"); + expectPlaceholder(".claude-pr/.claude/cache"); + const contents = snapshotRegularFileContents(); + expect(contents).not.toContain("untracked file\n"); + expect(contents).not.toContain("untracked dir entry\n"); + expectNoLinksInSnapshot(); + expect(lstatRepoFile(".claude").isDirectory()).toBe(true); + expect(readRepoFile(".claude/settings.json")).toBe( + `${JSON.stringify({ source: "base" })}\n`, + ); + }); + + test("records links to untracked in-tree directories as a single placeholder", () => { + writeRepoFile("build/out/a.js", "generated a\n"); + writeRepoFile("build/out/b.js", "generated b\n"); + symlinkRepoFile(".claude/build", "../build"); + git(["add", ".claude/build"]); + git(["commit", "-m", "pr links to an untracked directory"]); + + restoreConfigFromBase("main"); + + expectPlaceholder(".claude-pr/.claude/build"); + const contents = snapshotRegularFileContents(); + expect(contents).not.toContain("generated a\n"); + expect(contents).not.toContain("generated b\n"); + expectNoLinksInSnapshot(); + }); + + test("records links back into a parent directory as placeholders", () => { + symlinkRepoFile(".claude/parent-dir", ".."); + git(["add", "-A"]); + git(["commit", "-m", "pr adds a link back to the repo root"]); + + restoreConfigFromBase("main"); + + expectPlaceholder(".claude-pr/.claude/parent-dir"); + expect(existsRepoFile(".claude-pr/.claude/parent-dir/src")).toBe(false); + expect(readRepoFile(".claude-pr/.claude/settings.json")).toBe( + `${JSON.stringify({ source: "pr" })}\n`, + ); + expectNoLinksInSnapshot(); + }); + test("does not modify an existing .gitignore", () => { writeRepoFile(".gitignore", "node_modules\n"); git(["add", ".gitignore"]); @@ -196,6 +412,55 @@ describe("restoreConfigFromBase", () => { return readFileSync(join(repoDir, path), "utf8"); } + function writeOutsideFile(path: string, contents: string): string { + const fullPath = join(tempDir, "outside", path); + mkdirSync(dirname(fullPath), { recursive: true }); + writeFileSync(fullPath, contents); + return fullPath; + } + + // Contents of every regular file recorded in the snapshot, without following + // links, so tests can assert what actually got copied into the repository. + function snapshotRegularFileContents(): string[] { + const contents: string[] = []; + const visit = (dir: string) => { + for (const entry of readdirSync(dir)) { + const entryPath = join(dir, entry); + const stats = lstatSync(entryPath); + if (stats.isDirectory()) { + visit(entryPath); + } else if (stats.isFile()) { + contents.push(readFileSync(entryPath, "utf8")); + } + } + }; + visit(join(repoDir, ".claude-pr")); + return contents; + } + + // The snapshot must never contain links: every entry is a regular file or a + // real directory. + function expectNoLinksInSnapshot(): void { + const visit = (dir: string) => { + for (const entry of readdirSync(dir)) { + const entryPath = join(dir, entry); + const stats = lstatSync(entryPath); + expect(stats.isSymbolicLink()).toBe(false); + if (stats.isDirectory()) { + visit(entryPath); + } + } + }; + visit(join(repoDir, ".claude-pr")); + } + + function expectPlaceholder(path: string): void { + const stats = lstatRepoFile(path); + expect(stats.isSymbolicLink()).toBe(false); + expect(stats.isFile()).toBe(true); + expect(readRepoFile(path)).toStartWith("Snapshot placeholder: "); + } + function existsRepoFile(path: string): boolean { return existsSync(join(repoDir, path)); }