diff --git a/src/entrypoints/run.ts b/src/entrypoints/run.ts index 4efbef41..71b429b4 100644 --- a/src/entrypoints/run.ts +++ b/src/entrypoints/run.ts @@ -159,6 +159,9 @@ async function run() { let context: GitHubContext | undefined; let octokit: Octokits | undefined; let workloadIdentity: WorkloadIdentityHandle | undefined; + // Paths reverted to the PR base branch, which cleanup must not commit back + // onto the PR author's branch. Empty unless restoreConfigFromBase ran. + let restoredConfigPaths: string[] = []; // Track whether we've completed prepare phase, so we can attribute errors correctly let prepareCompleted = false; try { @@ -268,7 +271,7 @@ async function run() { validateBranchName(restoreBase); } if (restoreBase) { - restoreConfigFromBase(restoreBase); + restoredConfigPaths = restoreConfigFromBase(restoreBase); } } @@ -348,6 +351,7 @@ async function run() { prepareSuccess, prepareError, useCommitSigning: context.inputs.useCommitSigning, + restoredConfigPaths, }); } catch (error) { console.error("Error updating comment with job link:", error); diff --git a/src/entrypoints/update-comment-link.ts b/src/entrypoints/update-comment-link.ts index c0963e86..db5dd798 100644 --- a/src/entrypoints/update-comment-link.ts +++ b/src/entrypoints/update-comment-link.ts @@ -30,6 +30,12 @@ export type UpdateCommentLinkParams = { prepareSuccess: boolean; prepareError?: string; useCommitSigning: boolean; + /** + * Paths restored from the PR base branch by restoreConfigFromBase. The + * auto-commit in checkAndCommitOrDeleteBranch must leave these alone, or it + * commits the revert onto the PR author's branch. + */ + restoredConfigPaths?: string[]; }; export async function updateCommentLink( @@ -43,6 +49,7 @@ export async function updateCommentLink( context, octokit, useCommitSigning, + restoredConfigPaths = [], } = params; const { owner, repo } = context.repository; @@ -116,6 +123,7 @@ export async function updateCommentLink( claudeBranch, baseBranch, useCommitSigning, + restoredConfigPaths, ); // Check if we need to add PR URL when we have a new branch diff --git a/src/github/operations/branch-cleanup.ts b/src/github/operations/branch-cleanup.ts index 88de6de7..41397977 100644 --- a/src/github/operations/branch-cleanup.ts +++ b/src/github/operations/branch-cleanup.ts @@ -9,10 +9,32 @@ export async function checkAndCommitOrDeleteBranch( claudeBranch: string | undefined, baseBranch: string, useCommitSigning: boolean, + restoredConfigPaths: string[] = [], ): Promise<{ shouldDeleteBranch: boolean; branchLink: string }> { let branchLink = ""; let shouldDeleteBranch = false; + // On pull requests, restoreConfigFromBase replaces .claude/, CLAUDE.md and + // friends with the base branch's versions and leaves them unstaged so the + // revert does not reach a commit. Auto-committing with a bare `git add -A` + // would stage them anyway and push a silent revert of the PR author's own + // config onto their branch. + // + // The exclusion is driven by what was actually restored rather than applied + // unconditionally: this path also runs for issues, where no restore happens + // and Claude may legitimately have been asked to edit CLAUDE.md or + // .claude/settings.json. Excluding those there would silently drop the work. + const pathspecArgs = + restoredConfigPaths.length > 0 + ? ["--", ".", ...restoredConfigPaths.map((p) => `:(exclude)${p}`)] + : []; + + if (pathspecArgs.length > 0) { + console.log( + `Excluding base-restored config from auto-commit: ${restoredConfigPaths.join(", ")}`, + ); + } + if (claudeBranch) { // First check if the branch exists remotely let branchExistsRemotely = false; @@ -57,15 +79,19 @@ export async function checkAndCommitOrDeleteBranch( // Check for uncommitted changes using git status try { - const gitStatus = await $`git status --porcelain`.quiet(); + // Scoped the same way as the staging below: if the restored config + // is the only dirty entry there is no real work, and the branch + // should be treated as empty rather than receiving a pure revert. + const gitStatus = + await $`git status --porcelain ${pathspecArgs}`.quiet(); const hasUncommittedChanges = gitStatus.stdout.toString().trim().length > 0; if (hasUncommittedChanges) { console.log("Found uncommitted changes, committing them..."); - // Add all changes - await $`git add -A`; + // Add all changes, minus anything restored from the base branch + await $`git add -A ${pathspecArgs}`; // Commit with a descriptive message const runId = process.env.GITHUB_RUN_ID || "unknown"; diff --git a/src/github/operations/restore-config.ts b/src/github/operations/restore-config.ts index e04c6bb0..bdd55761 100644 --- a/src/github/operations/restore-config.ts +++ b/src/github/operations/restore-config.ts @@ -23,7 +23,7 @@ import { fetchDepthArgs } from "./fetch-depth"; // .gitconfig — git reads ~/.gitconfig and .git/config, never cwd/.gitconfig. // .bashrc etc. — shells source these from $HOME; checkout cannot reach $HOME. // .vscode/.idea— IDE config; nothing in the CLI's startup path reads them. -const SENSITIVE_PATHS = [ +export const SENSITIVE_PATHS = [ ".claude", ".mcp.json", ".claude.json", @@ -262,8 +262,11 @@ function ensureClaudePrExcludedFromGit(): void { * * @param baseBranch - PR base branch name. Must be pre-validated (branch.ts * calls validateBranchName on it before returning). + * @returns The paths whose working-tree state now comes from the base branch + * rather than the PR. Callers that stage files must exclude these, or they + * will commit the revert back onto the PR author's branch. */ -export function restoreConfigFromBase(baseBranch: string): void { +export function restoreConfigFromBase(baseBranch: string): string[] { console.log( `Restoring ${SENSITIVE_PATHS.join(", ")} from origin/${baseBranch} (PR head is untrusted)`, ); @@ -338,4 +341,9 @@ export function restoreConfigFromBase(baseBranch: string): void { } catch { // Nothing was staged, or paths don't exist on HEAD — either is fine. } + + // Every sensitive path is reported, not just the ones that changed: the + // restore also deletes paths the PR added that are absent on base, and those + // deletions are stageable too. + return [...SENSITIVE_PATHS]; } diff --git a/test/branch-cleanup-restored-config.test.ts b/test/branch-cleanup-restored-config.test.ts new file mode 100644 index 00000000..54dda18f --- /dev/null +++ b/test/branch-cleanup-restored-config.test.ts @@ -0,0 +1,262 @@ +#!/usr/bin/env bun + +/** + * Tests the interaction between restoreConfigFromBase and the auto-commit in + * checkAndCommitOrDeleteBranch. + * + * On pull requests the restore replaces .claude/, CLAUDE.md and friends with + * the base branch's versions and leaves them unstaged, so the revert does not + * reach a commit. A bare `git add -A` re-staged them anyway and pushed a silent + * revert of the PR author's own config onto their branch. + * + * These run against real git — the fix is a pathspec, so a mock would only + * assert that the arguments were passed, not that git honours them. + */ + +import { describe, test, expect, beforeEach, afterEach, spyOn } from "bun:test"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { checkAndCommitOrDeleteBranch } from "../src/github/operations/branch-cleanup"; +import { SENSITIVE_PATHS } from "../src/github/operations/restore-config"; +import type { Octokits } from "../src/github/api/client"; + +const BRANCH = "claude/issue-1-20260101-0000"; + +let workDir: string; +let remoteDir: string; +let originalCwd: string; +let logSpy: ReturnType; +let errorSpy: ReturnType; + +function git(...args: string[]): string { + return execFileSync("git", args, { cwd: workDir, encoding: "utf-8" }).trim(); +} + +function write(relative: string, contents: string) { + const full = join(workDir, relative); + mkdirSync(join(full, ".."), { recursive: true }); + writeFileSync(full, contents); +} + +/** Branch exists, and has no commits ahead of base, so cleanup inspects git. */ +const mockOctokit = { + rest: { + repos: { + getBranch: async () => ({ data: {} }), + compareCommitsWithBasehead: async () => ({ + data: { total_commits: 0 }, + }), + }, + git: { deleteRef: async () => ({ data: {} }) }, + }, +} as unknown as Octokits; + +beforeEach(() => { + originalCwd = process.cwd(); + const root = mkdtempSync(join(tmpdir(), "branch-cleanup-")); + remoteDir = join(root, "remote.git"); + workDir = join(root, "work"); + + execFileSync("git", ["init", "-q", "--bare", remoteDir]); + execFileSync("git", ["init", "-q", "-b", "main", workDir]); + git("config", "user.email", "test@example.com"); + git("config", "user.name", "Test"); + git("remote", "add", "origin", remoteDir); + + write(".claude/settings.json", '{"from":"base"}\n'); + write("CLAUDE.md", "base docs\n"); + write("src/app.ts", "base code\n"); + git("add", "-A"); + git("commit", "-qm", "base"); + git("push", "-q", "origin", "main"); + git("checkout", "-qb", BRANCH); + git("push", "-q", "origin", BRANCH); + + process.chdir(workDir); + logSpy = spyOn(console, "log").mockImplementation(() => {}); + errorSpy = spyOn(console, "error").mockImplementation(() => {}); +}); + +afterEach(() => { + process.chdir(originalCwd); + logSpy.mockRestore(); + errorSpy.mockRestore(); + rmSync(join(workDir, ".."), { recursive: true, force: true }); +}); + +/** Files touched by the most recent commit. */ +function filesInHeadCommit(): string[] { + return git("show", "--name-only", "--format=", "HEAD") + .split("\n") + .filter(Boolean) + .sort(); +} + +/** + * Reproduce the working-tree state restoreConfigFromBase leaves behind: the + * PR-authored config overwritten with the base branch's content, unstaged, so + * git reports it as a plain modification. + */ +function simulateRestoredConfig() { + write(".claude/settings.json", '{"from":"base"}\n'); + write("CLAUDE.md", "base docs\n"); +} + +function authorPrConfigEdits() { + write(".claude/settings.json", '{"from":"pr-author"}\n'); + write("CLAUDE.md", "pr author docs\n"); + git("commit", "-qam", "PR author edits config"); +} + +describe("auto-commit with restored config paths", () => { + test("does not commit the base-branch revert onto the PR branch", async () => { + authorPrConfigEdits(); + simulateRestoredConfig(); // config now reverted + unstaged + write("src/app.ts", "claude's real change\n"); + + const result = await checkAndCommitOrDeleteBranch( + mockOctokit, + "owner", + "repo", + BRANCH, + "main", + false, + [...SENSITIVE_PATHS], + ); + + expect(filesInHeadCommit()).toEqual(["src/app.ts"]); + expect(result.shouldDeleteBranch).toBe(false); + }); + + test("leaves the reverted config dirty in the working tree", async () => { + authorPrConfigEdits(); + simulateRestoredConfig(); + write("src/app.ts", "claude's real change\n"); + + await checkAndCommitOrDeleteBranch( + mockOctokit, + "owner", + "repo", + BRANCH, + "main", + false, + [...SENSITIVE_PATHS], + ); + + // --name-only gives bare paths, avoiding porcelain's status-column prefix. + const stillDirty = git("diff", "--name-only") + .split("\n") + .filter(Boolean) + .sort(); + expect(stillDirty).toEqual([".claude/settings.json", "CLAUDE.md"]); + }); + + test("treats a branch whose only change is the revert as empty", async () => { + // No real work — just the reverted config. Committing here would push a + // pure revert and keep an otherwise-empty branch alive. + authorPrConfigEdits(); + simulateRestoredConfig(); + + const before = git("rev-parse", "HEAD"); + const result = await checkAndCommitOrDeleteBranch( + mockOctokit, + "owner", + "repo", + BRANCH, + "main", + false, + [...SENSITIVE_PATHS], + ); + + expect(git("rev-parse", "HEAD")).toBe(before); + expect(result.shouldDeleteBranch).toBe(true); + expect(result.branchLink).toBe(""); + }); + + test("still commits Claude's own changes to non-config files", async () => { + authorPrConfigEdits(); + simulateRestoredConfig(); + write("src/app.ts", "changed\n"); + write("src/new-file.ts", "added\n"); + + await checkAndCommitOrDeleteBranch( + mockOctokit, + "owner", + "repo", + BRANCH, + "main", + false, + [...SENSITIVE_PATHS], + ); + + expect(filesInHeadCommit()).toEqual(["src/app.ts", "src/new-file.ts"]); + }); + + test("pushes the commit to the branch", async () => { + authorPrConfigEdits(); + simulateRestoredConfig(); + write("src/app.ts", "claude's real change\n"); + + const result = await checkAndCommitOrDeleteBranch( + mockOctokit, + "owner", + "repo", + BRANCH, + "main", + false, + [...SENSITIVE_PATHS], + ); + + const remoteHead = execFileSync( + "git", + ["--git-dir", remoteDir, "rev-parse", BRANCH], + { encoding: "utf-8" }, + ).trim(); + expect(remoteHead).toBe(git("rev-parse", "HEAD")); + expect(result.branchLink).toContain(BRANCH); + }); +}); + +describe("without restored config paths (the issue path)", () => { + test("commits config changes normally, since no revert happened", async () => { + // Reached for issues, where restoreConfigFromBase never runs and Claude may + // have been asked to edit CLAUDE.md. Excluding it here would drop the work. + write("CLAUDE.md", "claude wrote these docs\n"); + write(".claude/settings.json", '{"written":"by claude"}\n'); + write("src/app.ts", "and some code\n"); + + await checkAndCommitOrDeleteBranch( + mockOctokit, + "owner", + "repo", + BRANCH, + "main", + false, + [], + ); + + expect(filesInHeadCommit()).toEqual([ + ".claude/settings.json", + "CLAUDE.md", + "src/app.ts", + ]); + }); + + test("defaults to committing everything when the argument is omitted", async () => { + // Backwards compatibility: the parameter is optional. + write("CLAUDE.md", "claude wrote these docs\n"); + + await checkAndCommitOrDeleteBranch( + mockOctokit, + "owner", + "repo", + BRANCH, + "main", + false, + ); + + expect(filesInHeadCommit()).toEqual(["CLAUDE.md"]); + }); +});