Fix bundle transport in shallow checkouts#31346
Conversation
Co-authored-by: mrjf <180956+mrjf@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes bundle transport in push_to_pull_request_branch for shallow checkouts by avoiding git merge --ff-only (which can fail ancestry verification) and instead moving the branch ref to the bundle tip, then hard-resetting the worktree.
Changes:
- Replace bundle application from
git merge --ff-onlyto a guardedgit update-refon the checked-out branch. - Hard reset the worktree (
git reset --hard) after updating the branch ref to reflect the new tip. - Add a regression test asserting bundle transport uses
update-ref/resetand does not invokemerge --ff-only.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/push_to_pull_request_branch.cjs | Switch bundle application from merge to direct ref update + worktree reset for shallow checkout robustness. |
| actions/setup/js/push_to_pull_request_branch.test.cjs | Add regression coverage ensuring bundle transport no longer calls git merge --ff-only. |
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: 2
| // checkouts, merge --ff-only can fail to discover the ancestry even | ||
| // when the bundle was created from origin/<branch>..branch and the | ||
| // prerequisite exists locally. |
| it("should apply bundle transport by updating the branch ref instead of merging", async () => { | ||
| const bundlePath = path.join(tempDir, "test.bundle"); | ||
| const patchPath = createPatchFile("small patch content"); | ||
| fs.writeFileSync(bundlePath, "bundle content"); | ||
|
|
||
| const pushSignedCommitsModule = require("./push_signed_commits.cjs"); | ||
| const pushSignedSpy = vi.spyOn(pushSignedCommitsModule, "pushSignedCommits").mockResolvedValue("bundle-tip"); |
![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 bug fix with a regression test, the canonical use case for both skills.
Key Themes
- Fix is correct and well-reasoned: replacing
merge --ff-onlywithupdate-ref+reset --harddirectly addresses the root cause (shallow-clone ancestry gaps) rather than masking it. The guarded old-value argument preserves safety by making the ref update atomic against concurrent remote advances. - Regression test covers the primary path: the new test verifies that
merge --ff-onlyis no longer called and thatupdate-ref/resetare invoked. One coverage gap flagged inline: the!remoteHeadBeforePatchbranch (update-ref without old-value guard) is untested. - Existing review comments (misleading inline comment wording, test placed inside the wrong
describeblock) are accurate — worth addressing before merge for long-term maintainability.
Positive Highlights
- ✅ Regression test is present and makes a clear negative assertion (
expect(mockExec.exec).not.toHaveBeenCalledWith(..."merge"...)) - ✅ The conditional old-value guard is a thoughtful safety mechanism — keeps
update-reftransactional where possible - ✅ PR description clearly explains the root cause and the chosen approach
Verdict
Approving — the fix is sound. The inline comment and test location issues noted by prior reviewers, and the uncovered branch I flagged, are polish items that can be addressed here or in follow-up.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 3.7M
| 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 |
There was a problem hiding this comment.
[/tdd] The if (remoteHeadBeforePatch) guard means update-ref can be called without the old-value safety argument, but this code path is untested — the new test always exercises the non-empty case.
Consider adding a complementary case:
it("should update branch ref without old-value guard when remoteHeadBeforePatch is absent", async () => {
mockExec.getExecOutput
.mockResolvedValueOnce({ exitCode: 0, stdout: "\trefs/heads/feature-branch\n", stderr: "" }) // ls-remote (no SHA)
.mockResolvedValueOnce({ exitCode: 0, stdout: "", stderr: "" }) // rev-parse: empty
.mockResolvedValueOnce({ exitCode: 0, stdout: "2\n", stderr: "" }); // rev-list count
// ... exercise handler ...
expect(mockExec.exec).toHaveBeenCalledWith(
"git",
["update-ref", "refs/heads/feature-branch", "refs/bundles/push-feature-branch"],
expect.any(Object)
);
// No 4th arg: intentional, but documents the safety trade-off
});Without this, the branch that skips the old-value guard is invisible to the test suite, and the intentional trade-off (ref can be force-updated if remote has advanced concurrently) goes undocumented.
|
@copilot review all comments |
🧪 Test Quality Sentinel ReportTest Quality Score: 60/100
Test Classification Details
Flagged Tests — Requires Review
|
| Component | Max | Earned | Reason |
|---|---|---|---|
| Behavioral Coverage | 40 | 40 | 1/1 tests are design tests |
| Error/Edge Case Coverage | 30 | 0 | 0/1 tests include an error or edge case |
| Low Duplication | 20 | 20 | No duplicate clusters detected |
| Proportional Growth | 10 | 0 | Inflation ratio 2.5:1 exceeds 2:1 threshold |
| Total | 100 | 60 |
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §25634715413
🧪 Test quality analysis by Test Quality Sentinel · ● 9.6M · ◷
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Bug Fix
push_to_pull_request_branchbundle transport failed insafe_outputsshallow checkouts becausegit merge --ff-onlycould not prove ancestry and reported unrelated histories. The bundle ref already represents the intended branch tip, so applying it should not depend on merge-base discovery.What changed
git merge --ff-onlywith a guarded ref update.merge --ff-only.Core flow