-
Notifications
You must be signed in to change notification settings - Fork 387
Handle missing bundle prerequisite commits in safe-output create_pull_request
#32220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f72d8ca
f6bb941
2b3bf40
7faae3f
f5ba2c2
b2d60d1
d6f47cc
6ff5259
fd94f6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,32 @@ function createBundleTempRef(branchName) { | |
| return `refs/bundles/create-pr-${branchName.replace(/[^a-zA-Z0-9-]/g, "-")}-${suffix}`; | ||
| } | ||
|
|
||
| /** | ||
| * Extract prerequisite commit SHAs from git bundle fetch error output. | ||
| * @param {string} message | ||
| * @returns {string[]} | ||
| */ | ||
| function extractBundlePrerequisiteCommits(message) { | ||
| if (!message || !/lacks these prerequisite commits/i.test(message)) { | ||
| return []; | ||
| } | ||
| return [...new Set((message.match(/\b[0-9a-f]{40}\b/gi) || []).map(sha => sha.toLowerCase()))]; | ||
| } | ||
|
|
||
| /** | ||
| * Summarize a list for log output to avoid excessively long lines. | ||
| * @param {string[]} values | ||
| * @param {number} limit | ||
| * @returns {string} | ||
| */ | ||
| function summarizeListForLog(values, limit = 10) { | ||
| if (!Array.isArray(values) || values.length === 0) { | ||
| return "(none)"; | ||
| } | ||
| const preview = values.slice(0, limit).join(", "); | ||
| return values.length > limit ? `${preview} ... and ${values.length - limit} more` : preview; | ||
| } | ||
|
|
||
| /** | ||
| * Apply a git bundle to a local branch without fetching directly into the branch ref. | ||
| * Fetching directly into refs/heads/<branch> fails when that branch is currently checked out. | ||
|
|
@@ -99,28 +125,54 @@ async function applyBundleToBranch(bundleFilePath, branchName, originalAgentBran | |
|
|
||
| try { | ||
| await ensureFullHistoryForBundle(execApi); | ||
| core.info(`Applying bundle ${bundleFilePath} to ${bundleTargetRef} using temp ref ${bundleTempRef} from ${bundleBranchRef}`); | ||
|
|
||
| // Fetch from bundle into a temporary ref, then update the target branch. | ||
| // bundleBranchRef is the source ref inside the bundle (typically refs/heads/<agent-branch>). | ||
| try { | ||
| core.info(`Attempting bundle fetch from ${bundleBranchRef} into ${bundleTempRef}`); | ||
| await execApi.exec("git", ["fetch", bundleFilePath, `${bundleBranchRef}:${bundleTempRef}`]); | ||
| } catch (initialFetchError) { | ||
| // Fallback: resolve the source ref directly from the bundle contents. | ||
| // Some agents may emit a JSONL branch name that differs from the ref embedded in the bundle. | ||
| const initialFetchErrorMessage = initialFetchError instanceof Error ? initialFetchError.message : String(initialFetchError); | ||
| core.warning(`Bundle fetch with ${bundleBranchRef} failed: ${initialFetchErrorMessage}; resolving branch ref from bundle heads`); | ||
| const { stdout: bundleHeadsOutput } = await execApi.getExecOutput("git", ["bundle", "list-heads", bundleFilePath]); | ||
| const branchRefs = bundleHeadsOutput | ||
| .split("\n") | ||
| .map(line => line.trim().split(/\s+/)[1] || "") | ||
| .filter(ref => /^refs\/heads\/[A-Za-z0-9._][A-Za-z0-9._/-]*$/.test(ref)); | ||
|
|
||
| if (branchRefs.length === 1) { | ||
| bundleBranchRef = branchRefs[0]; | ||
| core.info(`Resolved bundle source ref from list-heads: ${bundleBranchRef}`); | ||
| await execApi.exec("git", ["fetch", bundleFilePath, `${bundleBranchRef}:${bundleTempRef}`]); | ||
|
|
||
| // Recovery path for bundle prerequisite failures: fetch missing prerequisite | ||
| // commit objects, then retry with the original bundle ref. | ||
| const prerequisiteCommits = extractBundlePrerequisiteCommits(initialFetchErrorMessage); | ||
| if (prerequisiteCommits.length > 0) { | ||
| core.warning(`Bundle fetch with ${bundleBranchRef} failed due to ${prerequisiteCommits.length} missing prerequisite commit(s); fetching prerequisites from origin and retrying`); | ||
| core.info(`Prerequisite commits: ${summarizeListForLog(prerequisiteCommits)}`); | ||
| core.info(`Fetching ${prerequisiteCommits.length} prerequisite commit(s) from origin`); | ||
| await execApi.exec("git", ["fetch", "origin", ...prerequisiteCommits]); | ||
| core.info("Fetched prerequisite commits from origin successfully"); | ||
| try { | ||
| core.info(`Retrying bundle fetch from ${bundleBranchRef} into ${bundleTempRef} after prerequisite recovery`); | ||
| await execApi.exec("git", ["fetch", bundleFilePath, `${bundleBranchRef}:${bundleTempRef}`]); | ||
| core.info("Bundle fetch retry succeeded after prerequisite recovery"); | ||
| } catch (retryError) { | ||
| throw new Error(`Bundle fetch failed after fetching ${prerequisiteCommits.length} prerequisite commit(s): ${retryError instanceof Error ? retryError.message : String(retryError)}`, { cause: retryError }); | ||
| } | ||
| } else { | ||
| throw new Error(`Failed to resolve bundle branch ref from list-heads: expected exactly 1 refs/heads entry, found ${branchRefs.length}`, { cause: initialFetchError }); | ||
| // Fallback: resolve the source ref directly from the bundle contents. | ||
| // Some agents may emit a JSONL branch name that differs from the ref embedded in the bundle. | ||
| core.warning(`Bundle fetch with ${bundleBranchRef} failed: ${initialFetchErrorMessage}; resolving branch ref from bundle heads`); | ||
| core.info(`Inspecting bundle heads from ${bundleFilePath}`); | ||
| const { stdout: bundleHeadsOutput } = await execApi.getExecOutput("git", ["bundle", "list-heads", bundleFilePath]); | ||
| const branchRefs = bundleHeadsOutput | ||
| .split("\n") | ||
| .map(line => line.trim().split(/\s+/)[1] || "") | ||
| .filter(ref => /^refs\/heads\/[A-Za-z0-9._][A-Za-z0-9._/-]*$/.test(ref)); | ||
| core.info(`Bundle list-heads returned ${branchRefs.length} candidate branch ref(s): ${summarizeListForLog(branchRefs)}`); | ||
|
|
||
| if (branchRefs.length === 1) { | ||
| bundleBranchRef = branchRefs[0]; | ||
| core.info(`Resolved bundle source ref from list-heads: ${bundleBranchRef}`); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnose] The retry Consider re-throwing with added context: try {
await execApi.exec("git", ["fetch", bundleFilePath, `${bundleBranchRef}:${bundleTempRef}`]);
} catch (retryError) {
throw new Error(
`Bundle fetch failed after fetching ${prerequisiteCommits.length} prerequisite commit(s): ${retryError.message}`,
{ cause: retryError }
);
}Preserving the diagnostic context ("we tried the prerequisite-recovery path") will make future failures much easier to triage. |
||
| core.info(`Fetching resolved bundle ref ${bundleBranchRef} into ${bundleTempRef}`); | ||
| await execApi.exec("git", ["fetch", bundleFilePath, `${bundleBranchRef}:${bundleTempRef}`]); | ||
| } else { | ||
| throw new Error(`Failed to resolve bundle branch ref from list-heads: expected exactly 1 refs/heads entry, found ${branchRefs.length}`, { | ||
| cause: initialFetchError, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| core.info(`Fetched bundle to ${bundleTempRef}`); | ||
|
|
@@ -133,8 +185,9 @@ async function applyBundleToBranch(bundleFilePath, branchName, originalAgentBran | |
| } finally { | ||
| try { | ||
| await execApi.exec("git", ["update-ref", "-d", bundleTempRef]); | ||
| } catch { | ||
| } catch (cleanupError) { | ||
| // Non-fatal cleanup | ||
| core.warning(`Non-fatal cleanup: failed to delete temporary bundle ref ${bundleTempRef}: ${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -329,6 +329,188 @@ index 0000000..abc1234 | |
| expect(resolvedFetchCall[1][2]).toMatch(/^refs\/heads\/main:refs\/bundles\/create-pr-ops-review-may09-2026-[a-f0-9]{8}$/); | ||
| }); | ||
|
|
||
| it("should fetch prerequisite commits and retry bundle fetch when prerequisites are missing", 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 missingSha = "256f08b38d9ce40cfa5d46385551caba8642a9df"; | ||
| let firstBundleFetchAttempt = true; | ||
| global.exec.exec.mockImplementation((cmd, args) => { | ||
| if (cmd === "git" && Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && firstBundleFetchAttempt) { | ||
| firstBundleFetchAttempt = false; | ||
| throw new Error(`error: Repository lacks these prerequisite commits:\nerror: ${missingSha}`); | ||
| } | ||
| return Promise.resolve(0); | ||
| }); | ||
|
|
||
| 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", "origin", missingSha]); | ||
| const bundleFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); | ||
| expect(bundleFetchCalls.length).toBe(2); | ||
| expect(global.exec.getExecOutput).not.toHaveBeenCalledWith("git", ["bundle", "list-heads", bundlePath]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test covers a single missing SHA, but the it("should fetch all prerequisite commits when multiple are missing", async () => {
const sha1 = "256f08b38d9ce40cfa5d46385551caba8642a9df";
const sha2 = "aabbccddee1122334455667788990011aabbccdd";
// error message containing both SHAs
// assert exec called with sha1, then sha2 (or together)
});Without this, a regression that silently drops all-but-the-first SHA would go undetected.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] There is no test for the failure path: what happens when it("should propagate error when fetching prerequisite commit fails", async () => {
global.exec.exec.mockImplementation((cmd, args) => {
if (args[0] === "fetch" && args[1] === "origin") throw new Error("fatal: couldn't connect to 'origin'");
if (/* first bundle fetch */ ...) throw new Error(`...lacks these prerequisite commits...`);
return Promise.resolve(0);
});
// expect result.success === false or thrown error
});would ensure the recovery path doesn't accidentally swallow the error when |
||
| }); | ||
|
|
||
| it("should fetch all prerequisite commits in a single origin fetch and retry bundle fetch", 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 missingSha1 = "256f08b38d9ce40cfa5d46385551caba8642a9df"; | ||
| const missingSha2 = "aabbccddee1122334455667788990011aabbccdd"; | ||
| let firstBundleFetchAttempt = true; | ||
| global.exec.exec.mockImplementation((cmd, args) => { | ||
| if (cmd === "git" && Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && firstBundleFetchAttempt) { | ||
| firstBundleFetchAttempt = false; | ||
| throw new Error(`error: Repository lacks these prerequisite commits:\nerror: ${missingSha1}\nerror: ${missingSha2}`); | ||
| } | ||
| return Promise.resolve(0); | ||
| }); | ||
|
|
||
| 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", "origin", missingSha1, missingSha2]); | ||
| const bundleFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); | ||
| expect(bundleFetchCalls.length).toBe(2); | ||
| expect(global.exec.getExecOutput).not.toHaveBeenCalledWith("git", ["bundle", "list-heads", bundlePath]); | ||
| }); | ||
|
|
||
| it("should fail when fetching prerequisite commits from origin fails", 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 missingSha = "256f08b38d9ce40cfa5d46385551caba8642a9df"; | ||
| let firstBundleFetchAttempt = true; | ||
| global.exec.exec.mockImplementation((cmd, args) => { | ||
| if (cmd === "git" && Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && firstBundleFetchAttempt) { | ||
| firstBundleFetchAttempt = false; | ||
| throw new Error(`error: Repository lacks these prerequisite commits:\nerror: ${missingSha}`); | ||
| } | ||
| if (cmd === "git" && Array.isArray(args) && args[0] === "fetch" && args[1] === "origin" && args[2] === missingSha) { | ||
| throw new Error("fatal: couldn't connect to 'origin'"); | ||
| } | ||
| return Promise.resolve(0); | ||
| }); | ||
|
|
||
| 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(false); | ||
| expect(result.error).toBe("Failed to apply bundle"); | ||
| expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("Failed to apply bundle: fatal: couldn't connect to 'origin'")); | ||
| const bundleFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); | ||
| expect(bundleFetchCalls.length).toBe(1); | ||
| }); | ||
|
|
||
| it("should include retry context when bundle fetch still fails after prerequisite recovery", 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 missingSha = "256f08b38d9ce40cfa5d46385551caba8642a9df"; | ||
| let bundleFetchAttempts = 0; | ||
| global.exec.exec.mockImplementation((cmd, args) => { | ||
| if (cmd === "git" && Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath) { | ||
| bundleFetchAttempts += 1; | ||
| if (bundleFetchAttempts === 1) { | ||
| throw new Error(`error: Repository lacks these prerequisite commits:\nerror: ${missingSha}`); | ||
| } | ||
| throw new Error("fatal: failed to read bundle"); | ||
| } | ||
| return Promise.resolve(0); | ||
| }); | ||
|
|
||
| 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(false); | ||
| expect(result.error).toBe("Failed to apply bundle"); | ||
| expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("Bundle fetch failed after fetching 1 prerequisite commit(s): fatal: failed to read bundle")); | ||
| expect(bundleFetchAttempts).toBe(2); | ||
| }); | ||
|
|
||
| it("should not fetch a bundle directly into the target branch", async () => { | ||
| const patchPath = path.join(tempDir, "test.patch"); | ||
| fs.writeFileSync( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/diagnose] SHAs are fetched serially in a
for...ofloop.git fetchaccepts multiple refspecs in a single invocation, so all missing commits can be retrieved in one network round-trip:This is a minor efficiency concern today, but a bundle that diverged significantly from
origincould have dozens of prerequisite SHAs, making serial fetches noticeably slow in CI.