Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions actions/setup/js/create_pull_request.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const { withRetry, isTransientError, RATE_LIMIT_RETRY_CONFIG } = require("./erro
const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs");
const { findAgent, getIssueDetails, assignAgentToIssue } = require("./assign_agent_helpers.cjs");
const { globPatternToRegex } = require("./glob_pattern_helpers.cjs");
const { ensureFullHistoryForBundle } = require("./git_helpers.cjs");

/**
* @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction
Expand Down Expand Up @@ -1266,6 +1267,8 @@ async function main(config = {}) {
core.info(`Applying changes from bundle: ${bundleFilePath}`);
const bundleBranchRef = originalAgentBranch || branchName;
try {
await ensureFullHistoryForBundle(exec);

// Fetch from bundle: creates a local branch pointing to the bundle's tip commit.
// The bundle contains refs/heads/<bundleBranchRef> which was the agent's working branch.
await exec.exec("git", ["fetch", bundleFilePath, `refs/heads/${bundleBranchRef}:refs/heads/${branchName}`]);
Expand Down
127 changes: 127 additions & 0 deletions actions/setup/js/create_pull_request.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,133 @@ describe("create_pull_request - draft policy enforcement", () => {
});
});

describe("create_pull_request - bundle transport shallow checkout", () => {
let tempDir;
let originalEnv;
let pushSignedSpy;

beforeEach(() => {
originalEnv = { ...process.env };
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
process.env.GITHUB_REPOSITORY = "test-owner/test-repo";
process.env.GITHUB_BASE_REF = "main";
delete process.env.GITHUB_TOKEN;
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-bundle-test-"));

global.core = {
info: vi.fn(),
warning: vi.fn(),
error: vi.fn(),
debug: vi.fn(),
setFailed: vi.fn(),
setOutput: vi.fn(),
startGroup: vi.fn(),
endGroup: vi.fn(),
summary: {
addRaw: vi.fn().mockReturnThis(),
write: vi.fn().mockResolvedValue(undefined),
},
};
global.github = {
rest: {
pulls: {
create: vi.fn().mockResolvedValue({ data: { number: 42, html_url: "https://github.com/test-owner/test-repo/pull/42" } }),
},
repos: {
get: vi.fn().mockResolvedValue({ data: { default_branch: "main" } }),
},
issues: {
addLabels: vi.fn().mockResolvedValue({}),
},
},
graphql: vi.fn(),
};
global.context = {
eventName: "workflow_dispatch",
repo: { owner: "test-owner", repo: "test-repo" },
payload: {},
};
global.exec = {
exec: vi.fn().mockResolvedValue(0),
getExecOutput: vi.fn().mockImplementation((cmd, args) => {
if (cmd === "git" && args[0] === "rev-parse" && args[1] === "--is-shallow-repository") {
return Promise.resolve({ exitCode: 0, stdout: "true\n", stderr: "" });
}
if (cmd === "git" && args[0] === "rev-list") {
return Promise.resolve({ exitCode: 0, stdout: "1\n", stderr: "" });
}
if (typeof cmd === "string" && cmd.startsWith("git ls-remote")) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] This mock branch is dead code. cmd is "git" (just the executable), so cmd.startsWith("git ls-remote") is always false. The catch-all return covers ls-remote calls, but the intent is invisible.

Fix to match by args:

if (cmd === 'git' && args && args[0] === 'ls-remote') {
  return Promise.resolve({ exitCode: 0, stdout: '', stderr: '' });
}

The pattern used in the existing tests at lines ~1848 (cmdStr.includes('ls-remote --heads origin')) is an alternative. Either way, the dead branch should be removed so the mock accurately documents its contract.

return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" });
}
return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" });
}),
};

const pushSignedCommitsModule = require("./push_signed_commits.cjs");
pushSignedSpy = vi.spyOn(pushSignedCommitsModule, "pushSignedCommits").mockResolvedValue("bundle-tip");
delete require.cache[require.resolve("./create_pull_request.cjs")];
});

afterEach(() => {
if (pushSignedSpy) {
pushSignedSpy.mockRestore();
}

for (const key of Object.keys(process.env)) {
if (!(key in originalEnv)) {
delete process.env[key];
}
}
Object.assign(process.env, originalEnv);

if (tempDir && fs.existsSync(tempDir)) {
fs.rmSync(tempDir, { recursive: true, force: true });
}

delete global.core;
delete global.github;
delete global.context;
delete global.exec;
vi.clearAllMocks();
});

it("should unshallow before fetching a bundle", async () => {
const patchPath = path.join(tempDir, "test.patch");
fs.writeFileSync(
patchPath,
`From abc123 Mon Sep 17 00:00:00 2001
From: Test Author <test@example.com>
Date: Mon, 1 Jan 2024 00:00:00 +0000
Subject: [PATCH] Test commit

diff --git a/test.txt b/test.txt
new file mode 100644
index 0000000..abc1234
--- /dev/null
+++ b/test.txt
@@ -0,0 +1 @@
+Hello World
--
2.34.1
`
);
const bundlePath = path.join(tempDir, "test.bundle");
fs.writeFileSync(bundlePath, "bundle content");

const { main } = require("./create_pull_request.cjs");
const handler = await main({ base_branch: "main", preserve_branch_name: true });
const result = await handler({ title: "Test PR", body: "Test body", branch: "feature/test", patch_path: patchPath, bundle_path: bundlePath }, {});

expect(result.success).toBe(true);
expect(global.exec.exec).toHaveBeenCalledWith("git", ["fetch", "--unshallow", "origin"], expect.any(Object));
expect(global.exec.exec).toHaveBeenCalledWith("git", ["fetch", bundlePath, "refs/heads/feature/test:refs/heads/feature/test"]);
const unshallowCallIndex = global.exec.exec.mock.calls.findIndex(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === "--unshallow");
const bundleFetchCallIndex = global.exec.exec.mock.calls.findIndex(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath);
expect(unshallowCallIndex).toBeGreaterThanOrEqual(0);
expect(bundleFetchCallIndex).toBeGreaterThan(unshallowCallIndex);
});
});

describe("create_pull_request - fallback-as-issue configuration", () => {
describe("configuration parsing", () => {
it("should default fallback_as_issue to true when not specified", () => {
Expand Down
22 changes: 22 additions & 0 deletions actions/setup/js/git_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,30 @@ function hasMergeCommitsInRange(baseRef, headRef, options = {}) {
}
}

/**
* Ensure the current repository has full history before fetching a git bundle.
*
* Bundles generated from a commit range can declare prerequisite commits. A
* depth-1 checkout may not contain those prerequisites, and `git fetch <bundle>`
* rejects the bundle before the caller can update refs. Unshallowing first makes
* the prerequisites available while avoiding a no-op network fetch for full
* checkouts.
*
* @param {{ getExecOutput: Function, exec: Function }} execApi - Exec API to run git commands.
* @param {Object} [options] - Options passed through to exec calls.
* @returns {Promise<void>}
*/
async function ensureFullHistoryForBundle(execApi, options = {}) {
const { stdout } = await execApi.getExecOutput("git", ["rev-parse", "--is-shallow-repository"], options);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] The getExecOutput call has no error handling for when git rev-parse --is-shallow-repository fails (e.g., unusual git environment). An error here propagates out and is caught by the outer bundle-fetch try/catch, silently aborting the PR creation with { success: false } — which looks like a bundle error, not a shallow-check error.

Consider treating any failure as non-shallow and logging a warning:

try {
  ({ stdout } = await execApi.getExecOutput(
    'git', ['rev-parse', '--is-shallow-repository'], options
  ));
} catch {
  core.warning('Could not determine shallow status; skipping unshallow');
  return;
}

Adding a test for this error path would lock in the fallback behaviour.

if (stdout.trim() === "true") {
core.info("Repository is shallow; fetching full history before applying bundle");
await execApi.exec("git", ["fetch", "--unshallow", "origin"], options);
}
}

module.exports = {
execGitSync,
ensureFullHistoryForBundle,
getGitAuthEnv,
hasMergeCommitsInRange,
};
30 changes: 29 additions & 1 deletion actions/setup/js/git_helpers.test.cjs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { describe, it, expect, beforeEach, afterEach } from "vitest";
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";

describe("git_helpers.cjs", () => {
let originalCore;
Expand Down Expand Up @@ -276,4 +276,32 @@ describe("git_helpers.cjs", () => {
expect(env.GIT_CONFIG_KEY_0).toBe("http.https://github.example.com/.extraheader");
});
});

describe("ensureFullHistoryForBundle", () => {
it("should fetch full history when the repository is shallow", async () => {
const { ensureFullHistoryForBundle } = await import("./git_helpers.cjs");
const execApi = {
getExecOutput: vi.fn().mockResolvedValue({ stdout: "true\n" }),
exec: vi.fn().mockResolvedValue(0),
};
const options = { cwd: "/tmp/repo" };

await ensureFullHistoryForBundle(execApi, options);

expect(execApi.getExecOutput).toHaveBeenCalledWith("git", ["rev-parse", "--is-shallow-repository"], options);
expect(execApi.exec).toHaveBeenCalledWith("git", ["fetch", "--unshallow", "origin"], options);
});

it("should not fetch full history when the repository is not shallow", async () => {
const { ensureFullHistoryForBundle } = await import("./git_helpers.cjs");
const execApi = {
getExecOutput: vi.fn().mockResolvedValue({ stdout: "false\n" }),
exec: vi.fn().mockResolvedValue(0),
};

await ensureFullHistoryForBundle(execApi);

expect(execApi.exec).not.toHaveBeenCalled();
});
});
});
7 changes: 6 additions & 1 deletion actions/setup/js/push_to_pull_request_branch.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { checkFileProtection } = require("./manifest_file_helpers.cjs");
const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");
const { renderTemplateFromFile, buildProtectedFileList, getPromptPath } = require("./messages_core.cjs");
const { getGitAuthEnv } = require("./git_helpers.cjs");
const { ensureFullHistoryForBundle, getGitAuthEnv } = require("./git_helpers.cjs");
const { findRepoCheckout } = require("./find_repo_checkout.cjs");

/**
Expand Down Expand Up @@ -599,6 +599,11 @@ async function main(config = {}) {
core.info(`Applying changes from bundle: ${bundleFilePath}`);
const bundleRef = `refs/bundles/push-${branchName.replace(/[^a-zA-Z0-9-]/g, "-")}`;
try {
await ensureFullHistoryForBundle(exec, {
env: { ...process.env, ...gitAuthEnv },
...baseGitOpts,
});

// Fetch from bundle into a temporary ref
await exec.exec("git", ["fetch", bundleFilePath, `refs/heads/${message.branch}:${bundleRef}`], baseGitOpts);
core.info(`Fetched bundle to ${bundleRef}`);
Expand Down
24 changes: 20 additions & 4 deletions actions/setup/js/push_to_pull_request_branch.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -1213,20 +1213,36 @@ index 0000000..abc1234
const pushSignedSpy = vi.spyOn(pushSignedCommitsModule, "pushSignedCommits").mockResolvedValue("bundle-tip");

try {
mockExec.getExecOutput
.mockResolvedValueOnce({ exitCode: 0, stdout: "remote-head\trefs/heads/feature-branch\n", stderr: "" }) // preflight ls-remote
.mockResolvedValueOnce({ exitCode: 0, stdout: "remote-head\n", stderr: "" }) // rev-parse HEAD before bundle
.mockResolvedValueOnce({ exitCode: 0, stdout: "2\n", stderr: "" }); // rev-list --count
mockExec.getExecOutput.mockImplementation((cmd, args) => {
if (cmd === "git" && args[0] === "ls-remote") {
return Promise.resolve({ exitCode: 0, stdout: "remote-head\trefs/heads/feature-branch\n", stderr: "" });
}
if (cmd === "git" && args[0] === "rev-parse" && args[1] === "HEAD") {
return Promise.resolve({ exitCode: 0, stdout: "remote-head\n", stderr: "" });
}
if (cmd === "git" && args[0] === "rev-parse" && args[1] === "--is-shallow-repository") {
return Promise.resolve({ exitCode: 0, stdout: "true\n", stderr: "" });
}
if (cmd === "git" && args[0] === "rev-list") {
return Promise.resolve({ exitCode: 0, stdout: "2\n", stderr: "" });
}
return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" });
});

const module = await loadModule();
const handler = await module.main({});
const result = await handler({ branch: "feature-branch", patch_path: patchPath, bundle_path: bundlePath, diff_size: 5 * 1024 }, {});

expect(result.success).toBe(true);
expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", "--unshallow", "origin"], expect.any(Object));
expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", bundlePath, "refs/heads/feature-branch:refs/bundles/push-feature-branch"], expect.any(Object));
expect(mockExec.exec).toHaveBeenCalledWith("git", ["update-ref", "refs/heads/feature-branch", "refs/bundles/push-feature-branch", "remote-head"], expect.any(Object));
expect(mockExec.exec).toHaveBeenCalledWith("git", ["reset", "--hard"], expect.any(Object));
expect(mockExec.exec).not.toHaveBeenCalledWith("git", ["merge", "--ff-only", "refs/bundles/push-feature-branch"], expect.any(Object));
const unshallowCallIndex = mockExec.exec.mock.calls.findIndex(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === "--unshallow");
const bundleFetchCallIndex = mockExec.exec.mock.calls.findIndex(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath);
expect(unshallowCallIndex).toBeGreaterThanOrEqual(0);
expect(bundleFetchCallIndex).toBeGreaterThan(unshallowCallIndex);
} finally {
pushSignedSpy.mockRestore();
}
Expand Down
Loading