Skip to content

Fix git push failures misattributed as "Failed to apply patch" #18987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Mar 1, 2026
Merged

Fix git push failures misattributed as "Failed to apply patch" #18987

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8b5d436
Initial plan
Copilot Mar 1, 2026
f8c1216
Fix push failure misattributed as 'Failed to apply patch' in push_to_…
Copilot Mar 1, 2026
fcf80a2
Fix remoteHeadBeforePatch scope: hoist declaration outside patch try/…
Copilot Mar 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 28 additions & 17 deletions actions/setup/js/push_to_pull_request_branch.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ async function main(config = {}) {
// to branches with exactly one new commit (security: prevents use of CI trigger
// token on multi-commit branches where workflow files may have been modified).
let newCommitCount = 0;
let remoteHeadBeforePatch = "";
if (hasChanges) {
core.info("Applying patch...");
try {
Expand Down Expand Up @@ -356,7 +357,6 @@ async function main(config = {}) {

// Apply patch
// Capture HEAD before applying patch to compute new-commit count later
let remoteHeadBeforePatch = "";
try {
const { stdout } = await exec.getExecOutput("git", ["rev-parse", "HEAD"]);
remoteHeadBeforePatch = stdout.trim();
Expand All @@ -368,22 +368,6 @@ async function main(config = {}) {
// This allows git to resolve create-vs-modify mismatches when a file exists in target but not source
await exec.exec(`git am --3way ${patchFilePath}`);
core.info("Patch applied successfully");

// Push the applied commits to the branch
await exec.exec(`git push origin ${branchName}`);
core.info(`Changes committed and pushed to branch: ${branchName}`);

// Count new commits pushed for the CI trigger decision
if (remoteHeadBeforePatch) {
try {
const { stdout: countStr } = await exec.getExecOutput("git", ["rev-list", "--count", `${remoteHeadBeforePatch}..HEAD`]);
newCommitCount = parseInt(countStr.trim(), 10);
core.info(`${newCommitCount} new commit(s) pushed to branch`);
} catch {
// Non-fatal - newCommitCount stays 0, extra empty commit will be skipped
core.info("Could not count new commits - extra empty commit will be skipped");
}
}
} catch (error) {
core.error(`Failed to apply patch: ${getErrorMessage(error)}`);

Expand Down Expand Up @@ -416,6 +400,33 @@ async function main(config = {}) {

return { success: false, error: "Failed to apply patch" };
}

// Push the applied commits to the branch (outside patch try/catch so push failures are not misattributed)
try {
await exec.exec(`git push origin ${branchName}`);
core.info(`Changes committed and pushed to branch: ${branchName}`);
} catch (pushError) {
const pushErrorMessage = getErrorMessage(pushError);
core.error(`Failed to push changes: ${pushErrorMessage}`);
const nonFastForwardPatterns = ["non-fast-forward", "rejected", "fetch first", "Updates were rejected"];
const isNonFastForward = nonFastForwardPatterns.some(pattern => pushErrorMessage.includes(pattern));
Comment on lines +411 to +412
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-fast-forward detection is currently case-sensitive (includes) while the pattern list mixes casing (e.g., "Updates were rejected"). To make this more robust and easier to maintain, consider normalizing pushErrorMessage to a single case (e.g., .toLowerCase()) and keeping all patterns lowercase before matching.

Suggested change
const nonFastForwardPatterns = ["non-fast-forward", "rejected", "fetch first", "Updates were rejected"];
const isNonFastForward = nonFastForwardPatterns.some(pattern => pushErrorMessage.includes(pattern));
const pushErrorMessageLower = pushErrorMessage.toLowerCase();
const nonFastForwardPatterns = ["non-fast-forward", "rejected", "fetch first", "updates were rejected"];
const isNonFastForward = nonFastForwardPatterns.some(pattern => pushErrorMessageLower.includes(pattern));

Copilot uses AI. Check for mistakes.
const userMessage = isNonFastForward
? "Failed to push changes: remote PR branch changed while the workflow was running (non-fast-forward). Re-run the workflow on the latest PR branch state."
: `Failed to push changes: ${pushErrorMessage}`;
return { success: false, error_type: "push_failed", error: userMessage };
Comment on lines +409 to +416
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new push-failure return shape ({ success: false, error_type: "push_failed", ... }) and the non-fast-forward actionable message aren’t asserted by tests. The existing push-rejection scenario in push_to_pull_request_branch.test.cjs currently only checks success === false, so it won’t catch regressions back to "Failed to apply patch" or accidentally running patch-investigation logging on push failures. Please add/update tests to validate error_type, the user-facing message, and that patch investigation isn’t triggered on push errors.

Copilot uses AI. Check for mistakes.
}

// Count new commits pushed for the CI trigger decision
if (remoteHeadBeforePatch) {
try {
const { stdout: countStr } = await exec.getExecOutput("git", ["rev-list", "--count", `${remoteHeadBeforePatch}..HEAD`]);
newCommitCount = parseInt(countStr.trim(), 10);
Comment on lines +419 to +423
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remoteHeadBeforePatch is declared with let inside the patch-apply try block, but it’s referenced here outside that block. In Node this will throw a ReferenceError on the success path (e.g., after a successful push), breaking the handler. Declare remoteHeadBeforePatch in the outer scope (before the try) and assign to it inside the try so it’s available for the commit-count logic.

Copilot uses AI. Check for mistakes.
core.info(`${newCommitCount} new commit(s) pushed to branch`);
} catch {
// Non-fatal - newCommitCount stays 0, extra empty commit will be skipped
core.info("Could not count new commits - extra empty commit will be skipped");
}
}
} else {
core.info("Skipping patch application (empty patch)");

Expand Down
Loading