fix: linearize against bundle prerequisite SHA, not moving origin/<base> tip - #49002
Conversation
… to avoid base-branch drift When the base branch advances after the agent's checkout, soft-resetting to origin/<base> tip computes diff(new_tip_tree, agent_head_tree) which reverses unrelated base commits in the synthesized commit. Fix: export getBundlePrerequisites from git_helpers.cjs, accept bundleFilePath in rewriteBundleBranchAsSingleCommit, and use the bundle's prerequisite SHA as the linearization base when exactly one prerequisite is recorded. Falls back to origin/<baseBranch> for bundles with zero or multiple prerequisites. Also export rewriteBundleBranchAsSingleCommit for direct testing, and add 7 unit tests for getBundlePrerequisites plus an integration test that simulates a moving-base-branch scenario with a merge commit forcing the rewrite path. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage: fix: linearize against bundle prerequisite SHA
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions in default business logic dirs). |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Updates bundle linearization to use the agent’s recorded prerequisite commit rather than a moving base-branch tip.
Changes:
- Exports and tests bundle prerequisite extraction.
- Uses the prerequisite SHA during signed-push rewrite recovery.
- Adds moving-base integration coverage.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/git_helpers.cjs |
Exports prerequisite extraction. |
actions/setup/js/git_helpers.test.cjs |
Tests prerequisite parsing. |
actions/setup/js/create_pull_request.cjs |
Linearizes against the bundle prerequisite. |
actions/setup/js/create_pull_request_bundle_integration.test.cjs |
Adds moving-base regression coverage. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| // The diff introduced by HEAD must NOT show base-drift.txt as deleted. | ||
| const diffStat = execGit(["diff", "--name-status", "HEAD^", "HEAD"], { cwd: safeOutputsRepo }).stdout; | ||
| expect(diffStat).not.toMatch(/^D\s+base-drift\.txt/m); |
| // 4. Agent (unaware of drift) merges main to reconcile — creates merge commit. | ||
| execGit(["checkout", branchName], { cwd: agentRepo }); | ||
| execGit(["merge", "--no-ff", "main", "-m", "reconcile: merge updated main"], { cwd: agentRepo }); |
![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 /diagnosing-bugs and /tdd — the fix is well-scoped and the test coverage is strong. Leaving as COMMENT (not APPROVE) due to two targeted observations worth addressing.
📋 Key Themes & Highlights
Issues Raised
-
Silent degradation path (
create_pull_request.cjs~line 340): WhenbundleFilePathis not passed, the fallback toorigin/<baseBranch>is completely silent — the original bug reappears without any log signal. Acore.warningcall whenbundleFilePathis absent would make this visible in Action logs. -
Fragile regex in
getBundlePrerequisites(git_helpers.cjsline 322): The pattern includesrecordswhich is not a standard git output token. A silent parse failure here returns[]and triggers the fallback (i.e., the original bug). The integration test catches the happy path, but a unit test asserting on the exact git-emitted strings would pin this contract.
Positive Highlights
- ✅ Root cause is correctly identified and addressed at the right layer (linearization base, not the rebase path)
- ✅ Fallback strategy is sound — multiple prerequisites is a legitimate edge case and deserves the conservative
origin/<base>fallback - ✅
rewriteBundleBranchAsSingleCommitis now exported, enabling direct unit testing — good testability improvement - ✅ Integration test scenario is thorough: seeds bare remote, simulates collaborator drift, creates bundle, applies, rewrites, and asserts at both file-system and git-stat level
- ✅ 7 unit tests for
getBundlePrerequisitescover the key edge cases including errors, empty bundles, multiple prereqs, and deduplication
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.5 AIC · ⌖ 5.06 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/create_pull_request.cjs:340
[/diagnosing-bugs] The fallback to origin/<baseBranch> degrades silently when bundleFilePath is absent — the original bug reappears with no log signal.
<details>
<summary>💡 Suggestion</summary>
Add a warning when bundleFilePath is not supplied so future call sites that forget the argument are immediately visible in Action logs:
if (!bundleFilePath) {
core.warning(`rewriteBundleBranchAsSingleCommit: no bundleFilePath provided; falling back to ${fallbackBaseRef} (base-drift …
</details>
<details><summary>actions/setup/js/git_helpers.cjs:322</summary>
**[/diagnosing-bugs]** The regex `/the bundle (requires|records) (this|these)/i` matches on `records` which is not standard git output. If git ever changes wording, silently returning `[]` would reactivate the original bug with no error.
<details>
<summary>💡 Suggestion</summary>
A test against the actual `git bundle verify` output format would guard this — the integration test partially covers it, but a unit test asserting on the exact literal strings git emits (e.g. `'The bundle requires th…
</details>
There was a problem hiding this comment.
The fix is correct and well-tested.
Root cause addressed: rewriteBundleBranchAsSingleCommit previously always soft-reset to origin/<base>, which absorbs any base-branch commits added after the agent checkout. The fix uses the bundle prerequisite SHA (the exact commit the agent started from) as the linearization base.
Code quality:
- Proper fallback to
origin/<baseBranch>for bundles with 0 or multiple prerequisites — covers degenerate cases safely. - Error path uses
core.warning(non-fatal) with clear fallback, which is appropriate. getBundlePrerequisiteswas already implemented; the integration test clearly demonstrates the bug and verifies the fix.
Test coverage: 7 unit tests for getBundlePrerequisites and one integration test that reproduces the exact race condition (base advancing after agent checkout).
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.8 AIC · ⌖ 4.8 AIC · ⊞ 5.3K
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report
📊 Metrics (8 tests)
📋 Test Breakdown
|
|
@copilot please run the Failed checks to investigate: Recent reviewer feedback to address:
Run: https://github.com/github/gh-aw/actions/runs/30520508562
|
Triage Summary
Fixes file-deletion bug when base drifts after agent checkout. Batch B-git-checkout (with #49003, #49059) — overlapping edits to git_helpers.cjs; review together to avoid conflicts.
|
- Fix integration test to model the production race faithfully: bundle is now created BEFORE main advances (not after agent merges drift), so the bundle's prereq SHA accurately reflects the agent's actual base commit. - Strengthen drift-absorption assertion: changed from checking base-drift.txt is not deleted to checking it does not appear in the diff at all. The old assertion was trivially satisfied when base drift was absorbed as an addition rather than a deletion. - Add SHA reachability guard in rewriteBundleBranchAsSingleCommit: before using the bundle prereq SHA as the linearization base, verify the commit object is accessible locally (git cat-file -e). Falls back to origin/<baseBranch> when the prereq is not available (e.g., in a shallow clone). - Add direct unit tests for hasMergeCommitsInRange in git_helpers.test.cjs: covers null/empty inputs, git failure (unreachable refs), a range with no merge commits, and a range containing a merge commit (uses a real temp repo). Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit
|
|
@copilot add git integration tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…izeRangeAsCommit Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added 6 git integration tests in
All tests use real |
When base branch advances after agent checkout,
rewriteBundleBranchAsSingleCommit'sgit reset --soft origin/<base>computesdiff(new_tip_tree, agent_head_tree)— absorbing base drift into the synthesized commit and deleting files added to base after checkout.Changes
git_helpers.cjs: ExportgetBundlePrerequisites(was internal-only).create_pull_request.cjs—rewriteBundleBranchAsSingleCommit: Accept optionalbundleFilePath; usegetBundlePrerequisitesto resolve the prerequisite SHA recorded in the bundle (the agent's actual base at checkout time) as thelinearizeRangeAsCommitbase. Falls back toorigin/<baseBranch>when the bundle has 0 or multiple prerequisites. Update call site to passbundleFilePath.pushSignedCommits's existing rebase path (lines 463–536) then handles rebasing the prerequisite-anchored commit onto the currentorigin/<base>tip without absorbing drift.create_pull_request.cjs: ExportrewriteBundleBranchAsSingleCommitfor direct testing.git_helpers.test.cjs: 7 unit tests forgetBundlePrerequisites.create_pull_request_bundle_integration.test.cjs: Integration test — simulates base advancing after agent checkout, agent creating a merge commit, and the rewrite path — asserts synthesized commit contains only agent's file and does not delete the base-drift file.