fix: handle HEAD-only bundles in create_pull_request safe-output fallback#35989
Conversation
When git bundle list-heads returns only a HEAD entry (no refs/heads/*), the previous fallback threw "expected exactly 1 refs/heads entry, found 0". Extend the else-if ladder so a zero-refs/heads result tries fetching with `HEAD:<temp-ref>` instead of aborting. The HEAD SHA is validated against the 40-hex pattern before the fetch is attempted; if even HEAD is absent the error message now clearly says so. Also add: - Unit test in create_pull_request.test.cjs for the HEAD-only mock path - Integration test in create_pull_request_bundle_integration.test.cjs using a real HEAD-only bundle (git bundle create <f> HEAD) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add git integration test |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35989 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR fixes create_pull_request bundle application when bundle fallback inspection finds only HEAD and no refs/heads/* entry.
Changes:
- Adds a HEAD-only fallback path in
applyBundleToBranch. - Adds unit coverage for the mocked HEAD-only fallback.
- Adds integration coverage using a real HEAD-only git bundle.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/create_pull_request.cjs |
Adds fallback fetching from HEAD:<tempRef> when no branch refs are listed. |
actions/setup/js/create_pull_request.test.cjs |
Verifies the fallback issues a HEAD-based bundle fetch. |
actions/setup/js/create_pull_request_bundle_integration.test.cjs |
Validates applying a real HEAD-only bundle end-to-end. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
| const headLine = bundleHeadsOutput | ||
| .split("\n") | ||
| .map(line => line.trim()) | ||
| .find(line => /^[0-9a-f]{40}\s+HEAD$/.test(line)); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Correct, well-scoped fix. The HEAD-only bundle fallback is properly guarded, and both unit and integration tests validate the real git behavior.
### Review summary
Grumpy-coder findings adjudicated:
- git fetch
<bundle>HEAD:<dst>will fail — DROPPED. The integration test runs a realgit bundle create ... HEAD+ fetch cycle and proves this works. The claim is incorrect. - SHA-256 regex fragility (
{40}vs{64})* — Valid future concern, non-blocking. GitHub infra still uses SHA-1 object storage. headLineSHA unused in log — Left as a minor inline suggestion (non-blocking).- case-sensitive hex regex — DROPPED. Git consistently outputs lowercase hex SHAs.
Code is correct. The fix handles the edge case cleanly.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.1M
| await execApi.exec("git", ["fetch", bundleFilePath, `HEAD:${bundleTempRef}`]); | ||
| } else { | ||
| throw new Error(`Failed to resolve bundle branch ref from list-heads: bundle contains no refs/heads entries and no HEAD ref`); | ||
| } |
There was a problem hiding this comment.
Non-blocking: headLine SHA is checked for existence but not surfaced in the log message, missing a useful debugging signal when this fallback fires in production.
💡 Suggestion
Extract the SHA from headLine and include it in the core.info call:
const headSha = headLine.split(/\s+/)[0];
core.info(`Bundle has no refs/heads entries; fetching HEAD (${headSha}) directly into ${bundleTempRef}`);This makes it trivial to correlate the fetched commit against what was expected when reading GitHub Actions logs. Non-blocking — the current code is functionally correct.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Test assertion style: one test uses
throw new Error(...)instead of a properexpect()assertion (minor) - Uncovered error branch: the
branchRefs.length === 0+ no-HEAD-line error path has no unit test - Diagnostic logging: the resolved HEAD SHA is parsed but not surfaced in logs
Positive Highlights
- ✅ Root cause correctly identified and surgically fixed (
else if (branchRefs.length === 0)guard) - ✅ Both a mocked unit test and a real-git integration test are included — good dual coverage
- ✅ Integration test explicitly asserts the bundle shape that reproduces the bug (no
refs/heads/inlist-heads) - ✅ Strict SHA regex (
/^[0-9a-f]{40}\s+HEAD$/) avoids false positives - ✅ Zero deletions — purely additive, low regression risk
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.2M
| // Should have fetched using HEAD:<temp-ref> as the refspec | ||
| const headFetchCall = global.exec.exec.mock.calls.find( | ||
| ([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && typeof args[2] === "string" && args[2].startsWith("HEAD:") | ||
| ); |
There was a problem hiding this comment.
[/tdd] Using throw new Error(...) for assertion failure is unusual in a test — prefer expect(headFetchCall).toBeTruthy(...) so Jest/Vitest can format the failure consistently and count it as a proper assertion.
💡 Suggested replacement
// Instead of:
if (!headFetchCall) {
throw new Error("expected HEAD-based bundle fetch call");
}
expect(headFetchCall[1][2]).toMatch(...);
// Prefer:
expect(headFetchCall).toBeTruthy("expected HEAD-based bundle fetch call using HEAD:<tempRef> refspec");
expect(headFetchCall[1][2]).toMatch(...);This also removes the manual null-guard before the .toMatch assertion.
| .split("\n") | ||
| .map(line => line.trim()) | ||
| .find(line => /^[0-9a-f]{40}\s+HEAD$/.test(line)); | ||
| if (headLine) { |
There was a problem hiding this comment.
[/diagnose] headLine is parsed to extract the SHA but the SHA value itself is never used or logged — only the line's truthiness matters. Consider adding a core.info with the resolved SHA to aid future diagnostics.
💡 Suggestion
if (headLine) {
const headSha = headLine.split(/\s+/)[0];
core.info(`Bundle has no refs/heads entries; fetching HEAD (${headSha}) directly into ${bundleTempRef}`);
await execApi.exec("git", ["fetch", bundleFilePath, `HEAD:${bundleTempRef}`]);
}Logging the SHA makes it easy to verify the correct commit was fetched when reviewing workflow logs.
| @@ -423,6 +423,73 @@ index 0000000..abc1234 | |||
| expect(resolvedFetchCall[1][2]).toMatch(/^refs\/heads\/main:refs\/bundles\/create-pr-ops-review-may09-2026-[a-f0-9]{8}$/); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
[/tdd] The error branch — branchRefs.length === 0 with no HEAD line in the output — has no test coverage. A bundle producing empty list-heads output would silently hit this path.
💡 Suggested test skeleton
it("should throw when bundle has no refs/heads entries and no HEAD ref", async () => {
// mock git bundle list-heads to return empty output
global.exec.getExecOutput.mockImplementation((cmd, args) => {
if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads") {
return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" });
}
// ... other mocks ...
});
await expect(handler(...)).rejects.toThrow(
"bundle contains no refs/heads entries and no HEAD ref"
);
});
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
Addressed in |
|
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
|
|
When
git bundle list-headsreturns only aHEADentry (norefs/heads/*), the fallback path inapplyBundleToBranchaborted withexpected exactly 1 refs/heads entry, found 0, breaking everycreate_pull_requestrun whose agent produced a HEAD-only bundle.Changes
create_pull_request.cjs— extends thebranchRefs.length === 0case in thelist-headsfallback to fetch usingHEAD:<tempRef>instead of throwing:create_pull_request.test.cjs— unit test covering the HEAD-only mock path, assertingexecis called with aHEAD:<tempRef>refspec.create_pull_request_bundle_integration.test.cjs— integration test using a realgit bundle create <f> HEAD(norefs/heads/*), verifying the target branch lands at the expected SHA.