Fix create_pull_request rewrite/push parity for excluded files and base drift - #49474
Conversation
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Evaluation Summary✅ On-topic: This PR addresses two critical bugs in `create_pull_request's push path that allow files to reach committed changes when they should be excluded. This directly aligns with the project's focus on agentic workflows and file-protection guarantees. ✅ Focused: The PR targets a single, well-defined issue (#48934) with two related root-cause bugs in the push validation and bundle filtering logic.
FeedbackBefore this PR is ready for review:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ 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.
🟡 Not ready to approve
An existing bundle integration test still asserts the pre-rebase working-tree shape and will fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes validation/push parity by rebasing synthesized commits onto the current base tip.
Changes:
- Adds
rebaseOntosupport to commit linearization. - Replays bundle rewrites onto the live base.
- Promotes and adds parity regression coverage.
File summaries
| File | Description |
|---|---|
actions/setup/js/git_helpers.cjs |
Adds replay and rebase cleanup. |
actions/setup/js/git_helpers.test.cjs |
Tests base-drift replay. |
actions/setup/js/create_pull_request.cjs |
Rebases rewritten bundles onto the base tip. |
actions/setup/js/create_pull_request_validation_push_parity.test.cjs |
Enables rewrite parity regression. |
Review details
Suppressed comments (1)
actions/setup/js/create_pull_request.cjs:388
- Replaying onto
origin/mainchanges the resulting working tree to include base drift, but the existing regression inactions/setup/js/create_pull_request_bundle_integration.test.cjs:531-543still requiresbase-drift.txtto be absent. This new path therefore makes that integration test fail. Update that regression to assert the rebased shape (base drift present in the tree, absent fromorigin/main..HEAD, andHEAD^ == origin/main) so it validates the behavior introduced here.
const { exitCode } = await execApi.getExecOutput("git", ["cat-file", "-e", `${prereqSha}^{commit}`], { ignoreReturnCode: true, silent: true });
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Fix create_pull_request rewrite/push parity
The fix is correct and well-structured.
Correctness: linearizeRangeAsCommit correctly applies git rebase --onto rebaseOnto baseRef HEAD after synthesizing the single commit, replaying only the agent-authored delta onto the live base tip without absorbing base-branch drift. The guard (rebaseOnto.trim() !== baseRef) is a defensive no-op in practice (one is a SHA, the other is origin/<branch>).
Error recovery: The nested rebase --abort before reset --hard is the right sequence — it clears REBASE_HEAD state if the rebase started but failed mid-way, then the hard reset restores the original HEAD cleanly.
Test coverage: The new integration test covers exactly the regression scenario: base drift from a collaborator push, then linearizeRangeAsCommit with rebaseOnto, asserting origin/main..HEAD contains only the agent-authored file.
Parity test promotion: Promoting it.fails to it in the parity test is the right signal the fix is complete.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.5 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✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (4 tests)
✅ Quality SignalsExcellent Coverage:
Test Infrastructure:
Design Invariants Enforced:
Verdict
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking issues; three focused comments posted.
📋 Key Themes & Highlights
Key Themes
- Trim asymmetry (
git_helpers.cjsline 783):rebaseOntois trimmed butbaseRefis not, making the skip-same-ref guard asymmetric. - Double-restore on rebase failure (
git_helpers.cjsline 786): afterrebase --abortHEAD is already restored; the subsequentreset --hardis redundant and could confuse future maintainers. - Test fixture isolation (
git_helpers.test.cjsline 1648): the new integration test checks out a branch on the sharedrepoDirfixture without a cleanup guard, risking order-dependent test failures.
Positive Highlights
- ✅ Root cause properly addressed: linearizing against the bundle prerequisite commit and then replaying onto the live base tip is exactly the right two-phase approach.
- ✅
rebase --abortadded in the error path — good defensive recovery. - ✅
it.fails()promoted toit()with matching description update — clean signal that the regression is fully closed. - ✅ New integration test exercises the collaborator-drift scenario end-to-end with a real git remote, which is the hardest class of parity bugs to catch with unit mocks.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 34.5 AIC · ⊞ 7K
Comment /matt to run again
|
@copilot fix https://github.com/github/gh-aw/actions/runs/30687894145/job/91337199460?pr=49474 |
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request changes — the base-drift replay fix has a plausible silent-data-loss gap
Good direction (linearizing against the bundle prerequisite, then replaying onto the current base tip), but the new rebase --onto step lacks a post-rebase verification that the commit actually survived, and conflict/abort handling during that rebase is unverified by tests.
Themes
- Silent empty-rebase risk:
git rebase --ontodrops commits that become empty relative to the new base with no error surfaced — this could silently discard the agent's changes in edge cases (e.g. base drift happens to already contain equivalent content), which is exactly the parity bug class this PR is trying to close. - Unverified conflict path: the new integration test only covers the clean, non-conflicting replay. The failure/rollback path (
rebase --abortswallowing its own failure, thenreset --hard) has no coverage for a genuinely conflicting rebase, so the recovery logic added in this PR is unverified. - Positive: extending
linearizeRangeAsCommitwithrebaseOntoand rebasing againstfallbackBaseRefafter linearizing against the bundle prerequisite is a sound way to preserve the agent's actual diff while still publishing on a fresh tip.
🔎 Code quality review by PR Code Quality Reviewer · auto · 153.6 AIC · ⊞ 7.8K
Comment /review to run again
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
Core create_pull_request push/patch parity bug (data-loss risk on excluded files/base drift); 2 approvals.
|
|
@copilot add git integration tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ed files; fix git reset for new excluded files Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add more integration tests to cover all corner cases |
…ner cases Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added 5 integration tests in commit
|
|
@copilot add more tests to cover more cases |
- Add integration test for excluded modification of existing file (exposes and verifies the working-tree restoration fix below) - Add integration test for excluded files + base drift combined - Add integration test for no bundleFilePath (no-bundle fallback path) - Add integration test for file rename preservation - Add integration test for commit headline from latest commit Fix: when an excluded file is a modification of an existing base-branch file, git reset HEAD -- <file> restores the index but leaves the working tree with the agent's version. This unstaged change caused git rebase --onto to fail. After the index reset, detect affected files with git diff --name-only and restore their working tree with git checkout -- <files>. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added 5 more integration tests in commit
|
|
@copilot quick triage for this PR:
|
pr-finisher summary for HEAD
Remaining maintainer action: approve the pending CI run for |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot
|
pr-finisher summary for HEAD
Remaining maintainer action: approve the CJS run for |
create_pull_requestcould validate one object and push another. In practice this showed up in two ways: excluded paths could be removed from the.patchbut still present in the pushed commit, and the signed-push rewrite path could absorb or revert unrelated base-branch drift from shallow/long-running checkouts.Rewrite against the agent base, then replay onto the current base tip
rewriteBundleBranchAsSingleCommitnow linearizes against the bundle prerequisite commit, preserving the agent’s actual change set.origin/<base>tip so the final pushed diff stays relative to the live base branch instead of reintroducing drift.Extend linearization to support replay onto a newer base
linearizeRangeAsCommitnow acceptsrebaseOntofor the “compute diff from old base, publish on new base” case.Lock in validation/push parity with regressions
origin/main..HEADcontains only the agent-authored file set, not unrelated files added on base.Run: https://github.com/github/gh-aw/actions/runs/30700817659
Run URL: https://github.com/github/gh-aw/actions/runs/30719603092