mirror of
https://github.com/anthropics/claude-code-action.git
synced 2026-08-22 03:18:54 +08:00
fix: only limit fetch depth when the checkout is already shallow (#1647)
restoreConfigFromBase and setupBranch pass --depth to every git fetch. On a checkout made with fetch-depth: 0 that does not just cap the download: it truncates the history already present and marks the repository shallow, which drops the merge base with the base branch. `git log origin/<base>..HEAD` then silently includes commits that are already merged, and `git diff origin/<base>...HEAD` fails with "no merge base" — the two commands the prompt tells Claude to run to scope its work to the PR. Gate the flag on `git rev-parse --is-shallow-repository`, so a checkout that is already shallow (the fetch-depth: 1 default) keeps the same depth behaviour and the fetch savings it was added for, while a full checkout stays full. Fixes #1642
This commit is contained in:
@@ -13,6 +13,7 @@ import type { GitHubPullRequest } from "../types";
|
|||||||
import type { Octokits } from "../api/client";
|
import type { Octokits } from "../api/client";
|
||||||
import type { FetchDataResult } from "../data/fetcher";
|
import type { FetchDataResult } from "../data/fetcher";
|
||||||
import { generateBranchName } from "../../utils/branch-template";
|
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
|
* 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;
|
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 commitCount = prData.commits.totalCount;
|
||||||
const fetchDepth = Math.max(commitCount, 20);
|
const fetchDepth = Math.max(commitCount, 20);
|
||||||
|
const depthArgs = fetchDepthArgs(fetchDepth);
|
||||||
|
|
||||||
console.log(
|
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
|
// Validate branch names before use to prevent command injection
|
||||||
@@ -195,13 +203,13 @@ export async function setupBranch(
|
|||||||
execGit([
|
execGit([
|
||||||
"fetch",
|
"fetch",
|
||||||
"origin",
|
"origin",
|
||||||
`--depth=${fetchDepth}`,
|
...depthArgs,
|
||||||
`pull/${entityNumber}/head:${branchName}`,
|
`pull/${entityNumber}/head:${branchName}`,
|
||||||
]);
|
]);
|
||||||
} else {
|
} else {
|
||||||
// Execute git commands to checkout PR branch (dynamic depth based on PR size)
|
// Execute git commands to checkout PR branch (dynamic depth based on PR size)
|
||||||
// Using execFileSync instead of shell template literals for security
|
// Using execFileSync instead of shell template literals for security
|
||||||
execGit(["fetch", "origin", `--depth=${fetchDepth}`, branchName]);
|
execGit(["fetch", "origin", ...depthArgs, branchName]);
|
||||||
}
|
}
|
||||||
execGit(["checkout", branchName, "--"]);
|
execGit(["checkout", branchName, "--"]);
|
||||||
|
|
||||||
@@ -302,7 +310,7 @@ export async function setupBranch(
|
|||||||
// Ensure we're on the source branch
|
// Ensure we're on the source branch
|
||||||
console.log(`Fetching and checking out source branch: ${sourceBranch}`);
|
console.log(`Fetching and checking out source branch: ${sourceBranch}`);
|
||||||
validateBranchName(sourceBranch);
|
validateBranchName(sourceBranch);
|
||||||
execGit(["fetch", "origin", sourceBranch, "--depth=1"]);
|
execGit(["fetch", "origin", sourceBranch, ...fetchDepthArgs(1)]);
|
||||||
execGit(["checkout", sourceBranch, "--"]);
|
execGit(["checkout", sourceBranch, "--"]);
|
||||||
|
|
||||||
return {
|
return {
|
||||||
@@ -320,7 +328,7 @@ export async function setupBranch(
|
|||||||
// Fetch and checkout the source branch first to ensure we branch from the correct base
|
// Fetch and checkout the source branch first to ensure we branch from the correct base
|
||||||
console.log(`Fetching and checking out source branch: ${sourceBranch}`);
|
console.log(`Fetching and checking out source branch: ${sourceBranch}`);
|
||||||
validateBranchName(sourceBranch);
|
validateBranchName(sourceBranch);
|
||||||
execGit(["fetch", "origin", sourceBranch, "--depth=1"]);
|
execGit(["fetch", "origin", sourceBranch, ...fetchDepthArgs(1)]);
|
||||||
execGit(["checkout", sourceBranch, "--"]);
|
execGit(["checkout", sourceBranch, "--"]);
|
||||||
|
|
||||||
// Create and checkout the new branch from the source branch
|
// Create and checkout the new branch from the source branch
|
||||||
|
|||||||
@@ -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/<base>..HEAD` then quietly lists
|
||||||
|
* commits that are already merged, and `git diff origin/<base>...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;
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -13,6 +13,7 @@ import {
|
|||||||
writeFileSync,
|
writeFileSync,
|
||||||
} from "fs";
|
} from "fs";
|
||||||
import { dirname, join, posix, relative, sep } from "path";
|
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.
|
// 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.
|
// fetch.recurseSubmodules config. Defense-in-depth alongside the delete above.
|
||||||
execFileSync(
|
execFileSync(
|
||||||
"git",
|
"git",
|
||||||
["fetch", "origin", baseBranch, "--depth=1", "--no-recurse-submodules"],
|
[
|
||||||
|
"fetch",
|
||||||
|
"origin",
|
||||||
|
baseBranch,
|
||||||
|
...fetchDepthArgs(1),
|
||||||
|
"--no-recurse-submodules",
|
||||||
|
],
|
||||||
{
|
{
|
||||||
stdio: "inherit",
|
stdio: "inherit",
|
||||||
env: process.env,
|
env: process.env,
|
||||||
|
|||||||
@@ -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"],
|
||||||
|
});
|
||||||
|
}
|
||||||
|
});
|
||||||
@@ -394,6 +394,37 @@ describe("restoreConfigFromBase", () => {
|
|||||||
expect(countClaudePrExcludeEntries()).toBe(1);
|
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 {
|
function git(args: string[]): string {
|
||||||
return execFileSync("git", args, {
|
return execFileSync("git", args, {
|
||||||
cwd: repoDir,
|
cwd: repoDir,
|
||||||
|
|||||||
Reference in New Issue
Block a user