diff --git a/src/github/operations/branch.ts b/src/github/operations/branch.ts index dccde680..a55b307b 100644 --- a/src/github/operations/branch.ts +++ b/src/github/operations/branch.ts @@ -13,6 +13,7 @@ import type { GitHubPullRequest } from "../types"; import type { Octokits } from "../api/client"; import type { FetchDataResult } from "../data/fetcher"; import { generateBranchName } from "../../utils/branch-template"; +import { fetchDepthArgs } from "./fetch-depth"; /** * Extracts the first label from GitHub data, or returns undefined if no labels exist @@ -175,12 +176,19 @@ export async function setupBranch( const branchName = prData.headRefName; - // Determine optimal fetch depth based on PR commit count, with a minimum of 20 + // Determine optimal fetch depth based on PR commit count, with a minimum + // of 20. Only applied to a checkout that is already shallow — see + // fetchDepthArgs. const commitCount = prData.commits.totalCount; const fetchDepth = Math.max(commitCount, 20); + const depthArgs = fetchDepthArgs(fetchDepth); console.log( - `PR #${entityNumber}: ${commitCount} commits, using fetch depth ${fetchDepth}`, + `PR #${entityNumber}: ${commitCount} commits, ${ + depthArgs.length > 0 + ? `using fetch depth ${fetchDepth}` + : "fetching without a depth limit (checkout has full history)" + }`, ); // Validate branch names before use to prevent command injection @@ -195,13 +203,13 @@ export async function setupBranch( execGit([ "fetch", "origin", - `--depth=${fetchDepth}`, + ...depthArgs, `pull/${entityNumber}/head:${branchName}`, ]); } else { // Execute git commands to checkout PR branch (dynamic depth based on PR size) // Using execFileSync instead of shell template literals for security - execGit(["fetch", "origin", `--depth=${fetchDepth}`, branchName]); + execGit(["fetch", "origin", ...depthArgs, branchName]); } execGit(["checkout", branchName, "--"]); @@ -302,7 +310,7 @@ export async function setupBranch( // Ensure we're on the source branch console.log(`Fetching and checking out source branch: ${sourceBranch}`); validateBranchName(sourceBranch); - execGit(["fetch", "origin", sourceBranch, "--depth=1"]); + execGit(["fetch", "origin", sourceBranch, ...fetchDepthArgs(1)]); execGit(["checkout", sourceBranch, "--"]); return { @@ -320,7 +328,7 @@ export async function setupBranch( // Fetch and checkout the source branch first to ensure we branch from the correct base console.log(`Fetching and checking out source branch: ${sourceBranch}`); validateBranchName(sourceBranch); - execGit(["fetch", "origin", sourceBranch, "--depth=1"]); + execGit(["fetch", "origin", sourceBranch, ...fetchDepthArgs(1)]); execGit(["checkout", sourceBranch, "--"]); // Create and checkout the new branch from the source branch diff --git a/src/github/operations/fetch-depth.ts b/src/github/operations/fetch-depth.ts new file mode 100644 index 00000000..7352fe32 --- /dev/null +++ b/src/github/operations/fetch-depth.ts @@ -0,0 +1,40 @@ +import { execFileSync } from "child_process"; + +/** + * Builds the `--depth` argument for a `git fetch`, unless the checkout still + * has its full history. + * + * `--depth` does not only cap what gets downloaded. Against a complete checkout + * (`actions/checkout` with `fetch-depth: 0`) it also truncates the history that + * is already there and marks the repository shallow, which drops the merge base + * with the base branch: `git log origin/..HEAD` then quietly lists + * commits that are already merged, and `git diff origin/...HEAD` fails + * with "no merge base". Those are the commands the prompt tells Claude to run + * to scope its work to the PR. + * + * A shallow checkout (the `fetch-depth: 1` default) has no history left to + * lose, so the limit still applies there and large repositories keep the fetch + * savings it was added for. + */ +export function fetchDepthArgs(depth: number): string[] { + return isShallowRepository() ? [`--depth=${depth}`] : []; +} + +function isShallowRepository(): boolean { + try { + const output = execFileSync( + "git", + ["rev-parse", "--is-shallow-repository"], + { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }, + ); + return output.trim() === "true"; + } catch { + // No repository yet, or a git old enough not to know the flag. Treat the + // checkout as complete: fetching more than necessary is recoverable, + // truncating history is not. + return false; + } +} diff --git a/src/github/operations/restore-config.ts b/src/github/operations/restore-config.ts index f915c872..e04c6bb0 100644 --- a/src/github/operations/restore-config.ts +++ b/src/github/operations/restore-config.ts @@ -13,6 +13,7 @@ import { writeFileSync, } from "fs"; import { dirname, join, posix, relative, sep } from "path"; +import { fetchDepthArgs } from "./fetch-depth"; // Paths that are both PR-controllable and read from cwd at CLI startup. // @@ -305,7 +306,13 @@ export function restoreConfigFromBase(baseBranch: string): void { // fetch.recurseSubmodules config. Defense-in-depth alongside the delete above. execFileSync( "git", - ["fetch", "origin", baseBranch, "--depth=1", "--no-recurse-submodules"], + [ + "fetch", + "origin", + baseBranch, + ...fetchDepthArgs(1), + "--no-recurse-submodules", + ], { stdio: "inherit", env: process.env, diff --git a/test/fetch-depth.test.ts b/test/fetch-depth.test.ts new file mode 100644 index 00000000..57ba25b8 --- /dev/null +++ b/test/fetch-depth.test.ts @@ -0,0 +1,107 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { execFileSync } from "child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "fs"; +import { join } from "path"; +import { setupBranch } from "../src/github/operations/branch"; +import { fetchDepthArgs } from "../src/github/operations/fetch-depth"; +import { createMockContext } from "./mockContext"; + +const octokits = { + rest: { + repos: { get: async () => ({ data: { default_branch: "main" } }) }, + git: { getRef: async () => ({ data: { object: { sha: "abc1234" } } }) }, + }, +} as any; + +const githubData = { + contextData: { title: "Add feature", labels: { nodes: [] } }, +} as any; + +describe("setupBranch fetch depth", () => { + let originalCwd: string; + let tempDir = ""; + let repoDir: string; + + beforeEach(() => { + originalCwd = process.cwd(); + tempDir = mkdtempSync(join("/tmp", "fetch-depth-")); + repoDir = join(tempDir, "repo"); + const remoteDir = join(tempDir, "origin.git"); + + execFileSync("git", ["init", "--bare", remoteDir], { stdio: "pipe" }); + execFileSync("git", ["init", repoDir], { stdio: "pipe" }); + git(["checkout", "-b", "main"]); + git(["config", "user.email", "test@example.com"]); + git(["config", "user.name", "Test User"]); + + for (const message of ["first", "second", "third"]) { + writeFileSync(join(repoDir, `${message}.txt`), `${message}\n`); + git(["add", "."]); + git(["commit", "-m", message]); + } + + git(["remote", "add", "origin", remoteDir]); + git(["push", "-u", "origin", "main"]); + + process.chdir(repoDir); + }); + + afterEach(() => { + process.chdir(originalCwd); + if (tempDir) { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + for (const useCommitSigning of [false, true]) { + test(`keeps the full history of a complete checkout with use_commit_signing: ${useCommitSigning}`, async () => { + const context = createMockContext({ + isPR: false, + entityNumber: 7, + inputs: { useCommitSigning, branchPrefix: "claude/" }, + }); + + await setupBranch(octokits, githubData, context); + + expect(git(["rev-parse", "--is-shallow-repository"]).trim()).toBe( + "false", + ); + expect(git(["rev-list", "--count", "HEAD"]).trim()).toBe("3"); + }); + } + + test("still limits the depth on an already shallow checkout", () => { + const shallowDir = join(tempDir, "shallow"); + execFileSync( + "git", + [ + "clone", + "--depth=1", + `file://${join(tempDir, "origin.git")}`, + shallowDir, + ], + { stdio: "pipe" }, + ); + + process.chdir(shallowDir); + expect( + execFileSync("git", ["rev-parse", "--is-shallow-repository"], { + cwd: shallowDir, + encoding: "utf8", + }).trim(), + ).toBe("true"); + expect(fetchDepthArgs(20)).toEqual(["--depth=20"]); + }); + + test("drops the depth limit on a complete checkout", () => { + expect(fetchDepthArgs(20)).toEqual([]); + }); + + function git(args: string[]): string { + return execFileSync("git", args, { + cwd: repoDir, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + } +}); diff --git a/test/restore-config.test.ts b/test/restore-config.test.ts index c63eb4f6..093978e0 100644 --- a/test/restore-config.test.ts +++ b/test/restore-config.test.ts @@ -394,6 +394,37 @@ describe("restoreConfigFromBase", () => { expect(countClaudePrExcludeEntries()).toBe(1); }); + test("leaves a full checkout unshallow so base..HEAD stays scoped to the PR", () => { + // The damage only shows up once base has moved on since the PR branched: + // the merge base is then an older commit that a depth-limited fetch of base + // truncates away, and every base..HEAD comparison silently changes meaning. + git(["checkout", "main"]); + writeRepoFile("src/other.ts", "export const advanced = true;\n"); + git(["add", "src/other.ts"]); + git(["commit", "-m", "base advance"]); + git(["push", "origin", "main"]); + git(["checkout", "pr"]); + + expect(git(["rev-parse", "--is-shallow-repository"]).trim()).toBe("false"); + const mergeBaseBefore = git(["merge-base", "origin/main", "HEAD"]).trim(); + + restoreConfigFromBase("main"); + + expect(git(["rev-parse", "--is-shallow-repository"]).trim()).toBe("false"); + expect(git(["merge-base", "origin/main", "HEAD"]).trim()).toBe( + mergeBaseBefore, + ); + // These are the two commands the prompt tells Claude to run to scope its + // work to the PR: the log range must not pick up already-merged commits, + // and the three-dot diff must still resolve a merge base at all. + expect(git(["log", "--format=%s", "origin/main..HEAD"]).trim()).toBe( + "pr config", + ); + expect( + git(["diff", "--name-only", "origin/main...HEAD"]).trim().split("\n"), + ).toEqual([".claude/settings.json", "CLAUDE.md"]); + }); + function git(args: string[]): string { return execFileSync("git", args, { cwd: repoDir,