Handle missing bundle prerequisite commits in safe-output create_pull_request#32220
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
create_pull_request
There was a problem hiding this comment.
Pull request overview
Adds a recovery path in the safe-output create_pull_request bundle fetch logic to handle "Repository lacks these prerequisite commits" errors by fetching the missing commit SHAs from origin and retrying the bundle fetch before falling back to bundle list-heads ref resolution.
Changes:
- Adds
extractBundlePrerequisiteCommitshelper to parse 40-hex SHAs from git error output guarded by the prerequisite error phrase. - Wraps existing list-heads fallback with a prerequisite-recovery branch that fetches missing SHAs from
originand retries the original bundle fetch. - Adds a unit test that simulates a prerequisite-missing failure and verifies the
origin <sha>fetch + retry path without invokingbundle list-heads.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/create_pull_request.cjs | Adds prerequisite-commit extraction and recovery path before list-heads fallback. |
| actions/setup/js/create_pull_request.test.cjs | New test case covering the prerequisite recovery + retry behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent test quality
Test Classification Details
Test Analysis✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — this is a targeted bug fix with accompanying tests, which is exactly the right shape for these two skills.
Key Themes
- Missing error context on retry — the second bundle fetch (after prerequisites are pulled) has no
try-catch, so if it fails the diagnostic context ("we attempted prerequisite recovery") is lost in the stack trace. - Serial SHA fetches — the
for...ofloop fetches one SHA pergit fetchcall; a single call with all SHAs as arguments would be more efficient. - Test edge-case gaps — the new test is well-structured, but two scenarios are missing: multiple prerequisite SHAs in a single error message, and failure of the
git fetch origin <sha>step itself.
Positive Highlights
- ✅ The
extractBundlePrerequisiteCommitshelper is cleanly separated, well-JSDoc'd, and independently testable — good deep-module thinking. - ✅ The recovery path is correctly scoped to only the prerequisite-commit error pattern, leaving all other fallback logic untouched. This is conservative and safe.
- ✅ The existing test uses
expect(global.exec.getExecOutput).not.toHaveBeenCalledWith("git", ["bundle", "list-heads", ...])to confirm the fast path was taken — that's a useful negative assertion. - ✅ The PR description is clear and includes the relevant code snippet inline.
Verdict
Commentary only — no blocking issues. The retry error-context gap (comment 1) is the most impactful item; the rest are incremental improvements to make the recovery path more robust against future regressions.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.8M
|
|
||
| if (branchRefs.length === 1) { | ||
| bundleBranchRef = branchRefs[0]; | ||
| core.info(`Resolved bundle source ref from list-heads: ${bundleBranchRef}`); |
There was a problem hiding this comment.
[/diagnose] The retry exec after fetching prerequisites has no error handling. If the second bundle fetch still fails (e.g., origin had some but not all prerequisite commits), the error propagates without any context indicating this was the retry path.
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.
| .split("\n") | ||
| .map(line => line.trim().split(/\s+/)[1] || "") | ||
| .filter(ref => /^refs\/heads\/[A-Za-z0-9._][A-Za-z0-9._/-]*$/.test(ref)); | ||
|
|
There was a problem hiding this comment.
[/diagnose] SHAs are fetched serially in a for...of loop. git fetch accepts multiple refspecs in a single invocation, so all missing commits can be retrieved in one network round-trip:
await execApi.exec("git", ["fetch", "origin", ...prerequisiteCommits]);This is a minor efficiency concern today, but a bundle that diverged significantly from origin could have dozens of prerequisite SHAs, making serial fetches noticeably slow in CI.
| 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]); |
There was a problem hiding this comment.
[/tdd] The test covers a single missing SHA, but the extractBundlePrerequisiteCommits function can return multiple SHAs and the loop iterates over all of them. A test with two prerequisite SHAs would both verify the multi-SHA code path and lock in the (potential) batching change suggested elsewhere:
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.
| 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]); |
There was a problem hiding this comment.
[/tdd] There is no test for the failure path: what happens when git fetch origin <sha> itself throws? Per /diagnose Phase 5, error paths should be exercised so regressions surface immediately. A test like:
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 origin is unavailable.
|
@copilot review all comments and all pull request review comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed all PR comments and review threads, and addressed the actionable items in
|
|
https://gh.io/copilot-coding-agent-docs review all comments and all pull request review comments |
|
@copilot review all comments and all pull request review comments |
Reviewed all PR comments and review threads again. The actionable review items were already addressed in
Completed another full pass over PR comments and review threads. There are no remaining actionable items beyond what was fixed in |
|
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/25889685241
|
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in |
|
@copilot add extensive logging |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in |
safe_outputscould fail increate_pull_requestwhen applying a bundle against a checkout missing prerequisite commits (Repository lacks these prerequisite commits). This change adds an explicit recovery path so bundle application can proceed when the referenced base commit object is absent locally.Bundle prerequisite recovery
originand retries the same bundle fetch before falling back to branch-ref resolution logic.Failure handling behavior
list-headsref resolution).Test coverage
origin <sha>fetch, and confirms successful retry without invokingbundle list-heads.pr-sous-chef: automated branch update attempt — run: https://github.com/github/gh-aw/actions/runs/25886945106