Skip to content

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 17 additions & 3 deletions actions/setup/js/push_signed_commits.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -588,9 +588,23 @@ async function pushSigned提交({
} catch {
// Ignore cleanup failures.
}
throw new Error(
`${ERR_SYSTEM}: pushSigned提交: failed to rebase commit range onto current GraphQL parent (${firstGraphqlParentOid}). ` + `Resolve conflicts by rebasing/cherry-picking locally and retry. Root cause: ${combinedOutput.trim()}`
);
const conflictMessage =
`${ERR_SYSTEM}: pushSigned提交: failed to rebase commit range onto current GraphQL parent (${firstGraphqlParentOid}). ` + `Resolve conflicts by rebasing/cherry-picking locally and retry. Root cause: ${combinedOutput.trim()}`;
if (allowGitPushFallback === false) {
throw new Error(conflictMessage);
}
// Genuine merge conflict (not a shallow/partial-clone object-fetch issue, and no custom
// resolver handled it): rebasing the commit range onto the current base cannot be done
// automatically, and replaying the stale-base commits through GraphQL would silently
Comment on lines +647 to +649
// synthesize file content against the wrong parent. Rather than failing the whole
// operation (which previously forced callers to fall back to opening an issue instead
// of a pull request), push the ORIGINAL un-rebased commits directly via unsigned
// `git push`. GitHub will still create the pull request; it will simply report the
// branch as having conflicts that need to be resolved, the same as any normal PR.
core.warning(`${conflictMessage} Falling back to an unsigned git push of the un-rebased commit(s) so the pull request can still be created (it will show as having merge conflicts with the base branch).`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] The warning string embeds conflictMessage verbatim, which includes "Resolve conflicts by rebasing/cherry-picking locally and retry." — but in this path the operation succeeds, so that instruction is misleading to anyone reading logs.

💡 Suggested fix

Split the message so the "retry" instruction only appears in the thrown error, not in the fallback-success warning:

const diagnosticMessage =
  `${ERR_SYSTEM}: pushSigned提交: failed to rebase commit range onto current GraphQL parent (${firstGraphqlParentOid}). ` +
  `Root cause: ${combinedOutput.trim()}`;
if (allowGitPushFallback === false) {
  throw new Error(`${diagnosticMessage} Resolve conflicts by rebasing/cherry-picking locally and retry.`);
}
core.warning(
  `${diagnosticMessage} Falling back to an unsigned git push of the un-rebased commit(s) so the pull request can still be created (it will show as having merge conflicts with the base branch).`
);

This also keeps the ERR_SYSTEM: prefix out of a warning that doesn't represent a system failure, which may otherwise trigger false monitoring alerts.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed as suggested: split the message so the "retry" instruction only appears on the throw path, and the fallback-success warning no longer implies a retry is needed.

const fallbackSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken });
core.info(`pushSigned提交: unsigned git push fallback (unresolved rebase conflict) completed, using pushed SHA ${fallbackSha}`);
return fallbackSha;
}
}
const { stdout: rebasedRevListOut } = await exec.getExecOutput("git", ["rev-list", "--parents", "--topo-order", "--reverse", `${firstGraphqlParentOid}..HEAD`], { cwd });
Expand Down
57 changes: 55 additions & 2 deletions actions/setup/js/push_signed_commits.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -1924,7 +1924,7 @@ describe("push_signed_commits integration tests", () => {
});

describe("stale-base and synthesized payload safety", () => {
it("should fail signed replay when rebasing stale commits onto current base conflicts", async () => {
it("should fall back to an unsigned push of the un-rebased commits when rebasing stale commits onto current base conflicts", async () => {
// Base branch starts with shared file.
fs.writeFileSync(path.join(workDir, "shared.txt"), "base\n");
execGit(["add", "shared.txt"], { cwd: workDir });
Expand All @@ -1936,6 +1936,7 @@ describe("push_signed_commits integration tests", () => {
fs.writeFileSync(path.join(workDir, "shared.txt"), "agent change\n");
execGit(["add", "shared.txt"], { cwd: workDir });
execGit(["commit", "-m", "Agent edit shared"], { cwd: workDir });
const localOidBeforePush = execGit(["rev-parse", "HEAD"], { cwd: workDir }).stdout.trim();

// Base branch advances with conflicting edit.
execGit(["checkout", "main"], { cwd: workDir });
Expand All @@ -1949,18 +1950,69 @@ describe("push_signed_commits integration tests", () => {
global.exec = makeRealExec(workDir);
const githubClient = makeMockGithubClient();

const result = await pushSigned提交({
githubClient,
owner: "test-owner",
repo: "test-repo",
branch: "stale-conflict-branch",
baseRef: "origin/main",
cwd: workDir,
});

// GraphQL is never invoked for this path — the un-rebased commits are pushed directly.
expect(githubClient.graphql).not.toHaveBeenCalled();
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("failed to rebase commit range onto current GraphQL parent"));
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Falling back to an unsigned git push of the un-rebased commit(s)"));

// The branch was pushed as-is (still based on the stale parent) so GitHub can create the
// pull request and surface the conflict for manual resolution, instead of the whole
// operation failing outright.
expect(result).toBe(localOidBeforePush);
const lsRemote = execGit(["ls-remote", bareDir, "refs/heads/stale-conflict-branch"], { cwd: workDir });
const remoteOid = lsRemote.stdout.trim().split(/\s+/)[0];
expect(remoteOid).toBe(localOidBeforePush);
});

it("should still fail (without pushing) when git push fallback is explicitly disabled and rebasing stale commits conflicts", async () => {
// Base branch starts with shared file.
fs.writeFileSync(path.join(workDir, "shared.txt"), "base\n");
execGit(["add", "shared.txt"], { cwd: workDir });
execGit(["commit", "-m", "Add shared file"], { cwd: workDir });
execGit(["push", "origin", "main"], { cwd: workDir });

// Agent branch diverges from old main and edits shared.txt.
execGit(["checkout", "-b", "stale-conflict-no-fallback-branch"], { cwd: workDir });
fs.writeFileSync(path.join(workDir, "shared.txt"), "agent change\n");
execGit(["add", "shared.txt"], { cwd: workDir });
execGit(["commit", "-m", "Agent edit shared"], { cwd: workDir });

// Base branch advances with conflicting edit.
execGit(["checkout", "main"], { cwd: workDir });
fs.writeFileSync(path.join(workDir, "shared.txt"), "upstream change\n");
execGit(["add", "shared.txt"], { cwd: workDir });
execGit(["commit", "-m", "Upstream edit shared"], { cwd: workDir });
execGit(["push", "origin", "main"], { cwd: workDir });

execGit(["checkout", "stale-conflict-no-fallback-branch"], { cwd: workDir });

global.exec = makeRealExec(workDir);
const githubClient = makeMockGithubClient();

await expect(
pushSigned提交({
githubClient,
owner: "test-owner",
repo: "test-repo",
branch: "stale-conflict-branch",
branch: "stale-conflict-no-fallback-branch",
baseRef: "origin/main",
cwd: workDir,
allowGitPushFallback: false,
})
).rejects.toThrow("failed to rebase commit range onto current GraphQL parent");

expect(githubClient.graphql).not.toHaveBeenCalled();
const lsRemote = execGit(["ls-remote", bareDir, "refs/heads/stale-conflict-no-fallback-branch"], { cwd: workDir });
expect(lsRemote.stdout.trim()).toBe("");
});

it("should recover from a partial-clone object failure by backfilling the exact commit objects and retrying the rebase", async () => {
Expand Down Expand Up @@ -2086,6 +2138,7 @@ describe("push_signed_commits integration tests", () => {
branch: "conflict-no-backfill-branch",
baseRef: "origin/main",
cwd: workDir,
allowGitPushFallback: false,
})
).rejects.toThrow("failed to rebase commit range onto current GraphQL parent");

Expand Down
Loading