diff --git a/.changeset/patch-fix-topo-order-merge-fallback.md b/.changeset/patch-fix-topo-order-merge-fallback.md new file mode 100644 index 00000000000..7682f4dede2 --- /dev/null +++ b/.changeset/patch-fix-topo-order-merge-fallback.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fixed `push_signed_commits.cjs` to preserve commit replay order with `--topo-order` and fall back to `git push` when merge commits are detected, preventing incorrect signed-commit push behavior. diff --git a/actions/setup/js/push_signed_commits.cjs b/actions/setup/js/push_signed_commits.cjs index ce652f233be..feaebffed84 100644 --- a/actions/setup/js/push_signed_commits.cjs +++ b/actions/setup/js/push_signed_commits.cjs @@ -109,9 +109,13 @@ function unquoteCPath(s) { * @returns {Promise} */ async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, cwd, gitAuthEnv }) { - // Collect the commits introduced (oldest-first) - const { stdout: revListOut } = await exec.getExecOutput("git", ["rev-list", "--reverse", `${baseRef}..HEAD`], { cwd }); - const shas = revListOut.trim().split("\n").filter(Boolean); + // Collect the commits introduced (oldest-first) using topological order to ensure + // correct sequencing even when commit dates are out of sync (e.g. after rebase --committer-date-is-author-date). + // Using --parents emits each line as " [ ...]", which lets us detect merge commits + // (more than one parent) in a single subprocess call without iterating each SHA individually. + const { stdout: revListOut } = await exec.getExecOutput("git", ["rev-list", "--parents", "--topo-order", "--reverse", `${baseRef}..HEAD`], { cwd }); + const revListLines = revListOut.trim().split("\n").filter(Boolean); + const shas = revListLines.map(line => line.split(" ")[0]); if (shas.length === 0) { core.info("pushSignedCommits: no new commits to push via GraphQL"); @@ -121,6 +125,19 @@ async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, c core.info(`pushSignedCommits: replaying ${shas.length} commit(s) via GraphQL createCommitOnBranch (branch: ${branch}, repo: ${owner}/${repo})`); try { + // Pre-flight check: detect merge commits. Each --parents output line is " [ ...]". + // A line with 3+ space-separated fields means the commit has 2+ parents (i.e. a merge commit). + // The GitHub GraphQL createCommitOnBranch mutation does not support multiple parents, so fall back + // to git push for the entire series if any merge commit is found. + for (const line of revListLines) { + const fields = line.split(" "); + if (fields.length > 2) { + const sha = fields[0]; + core.warning(`pushSignedCommits: merge commit ${sha} detected, falling back to git push`); + throw new Error("merge commit detected"); + } + } + // Pre-scan ALL commits: collect file changes and check for unsupported file modes // BEFORE starting any GraphQL mutations. If a symlink is found mid-loop after some // commits have already been signed, the remote branch diverges and the git push diff --git a/actions/setup/js/push_signed_commits.test.cjs b/actions/setup/js/push_signed_commits.test.cjs index d3c9e6f5084..117b296cf04 100644 --- a/actions/setup/js/push_signed_commits.test.cjs +++ b/actions/setup/js/push_signed_commits.test.cjs @@ -883,4 +883,152 @@ describe("push_signed_commits integration tests", () => { expect(Buffer.from(callArg.fileChanges.additions[0].contents, "base64").toString()).toBe("copy source\n"); }); }); + + // ────────────────────────────────────────────────────── + // Topological ordering (--topo-order) + // ────────────────────────────────────────────────────── + + describe("topological commit ordering", () => { + it("should replay commits in DAG order even when commit dates are out of sync", async () => { + execGit(["checkout", "-b", "topo-order-branch"], { cwd: workDir }); + + // Create two commits where the second commit has an earlier author/committer date + // than the first, simulating the situation after `git rebase --committer-date-is-author-date` + // or manual date manipulation. Without --topo-order git would return them in wrong order. + const laterDate = "2020-01-02T00:00:00+00:00"; + const earlierDate = "2020-01-01T00:00:00+00:00"; + + fs.writeFileSync(path.join(workDir, "first.txt"), "first\n"); + execGit(["add", "first.txt"], { cwd: workDir }); + // Commit A has a later date (chronologically second) + spawnSync("git", ["commit", "-m", "First commit (later date)"], { + cwd: workDir, + encoding: "utf8", + env: { + ...process.env, + GIT_CONFIG_NOSYSTEM: "1", + HOME: os.tmpdir(), + GIT_AUTHOR_DATE: laterDate, + GIT_COMMITTER_DATE: laterDate, + }, + }); + + fs.writeFileSync(path.join(workDir, "second.txt"), "second\n"); + execGit(["add", "second.txt"], { cwd: workDir }); + // Commit B has an earlier date (chronologically first) – without --topo-order + // a date-based sort would put this before commit A, which would be wrong. + spawnSync("git", ["commit", "-m", "Second commit (earlier date)"], { + cwd: workDir, + encoding: "utf8", + env: { + ...process.env, + GIT_CONFIG_NOSYSTEM: "1", + HOME: os.tmpdir(), + GIT_AUTHOR_DATE: earlierDate, + GIT_COMMITTER_DATE: earlierDate, + }, + }); + + execGit(["push", "-u", "origin", "topo-order-branch"], { cwd: workDir }); + + global.exec = makeRealExec(workDir); + const githubClient = makeMockGithubClient(); + + await pushSignedCommits({ + githubClient, + owner: "test-owner", + repo: "test-repo", + branch: "topo-order-branch", + baseRef: "origin/main", + cwd: workDir, + }); + + // Both commits must be replayed via GraphQL in DAG order: first → second + expect(githubClient.graphql).toHaveBeenCalledTimes(2); + const headlines = githubClient.graphql.mock.calls.map(c => c[1].input.message.headline); + expect(headlines).toEqual(["First commit (later date)", "Second commit (earlier date)"]); + }); + }); + + // ────────────────────────────────────────────────────── + // Merge commit fallback + // ────────────────────────────────────────────────────── + + describe("merge commit fallback", () => { + it("should fall back to git push and warn when the commit range contains a merge commit", async () => { + // Set up: main already has an initial commit. Create a side branch with an extra commit, + // then merge it back into a feature branch to produce a merge commit in the range. + execGit(["checkout", "-b", "side-branch"], { cwd: workDir }); + fs.writeFileSync(path.join(workDir, "side.txt"), "side branch content\n"); + execGit(["add", "side.txt"], { cwd: workDir }); + execGit(["commit", "-m", "Side branch commit"], { cwd: workDir }); + + // Back to main, create feature branch, and merge side-branch into it + execGit(["checkout", "main"], { cwd: workDir }); + execGit(["checkout", "-b", "merge-test-branch"], { cwd: workDir }); + fs.writeFileSync(path.join(workDir, "feature.txt"), "feature content\n"); + execGit(["add", "feature.txt"], { cwd: workDir }); + execGit(["commit", "-m", "Feature commit"], { cwd: workDir }); + + // Merge side-branch – this creates a merge commit with two parents + execGit(["merge", "--no-ff", "side-branch", "-m", "Merge side-branch into merge-test-branch"], { cwd: workDir }); + + // Push so ls-remote can resolve the OID + execGit(["push", "-u", "origin", "merge-test-branch"], { cwd: workDir }); + + global.exec = makeRealExec(workDir); + const githubClient = makeMockGithubClient(); + + await pushSignedCommits({ + githubClient, + owner: "test-owner", + repo: "test-repo", + branch: "merge-test-branch", + baseRef: "origin/main", + cwd: workDir, + }); + + // GraphQL must NOT have been called – merge commit triggers git push fallback + expect(githubClient.graphql).not.toHaveBeenCalled(); + + // Warning about the merge commit must be emitted + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringMatching(/merge commit [0-9a-f]{7,40} detected/)); + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("falling back to git push")); + + // All commits (including the merge commit) must be present on the remote via git push + const lsRemote = execGit(["ls-remote", bareDir, "refs/heads/merge-test-branch"], { cwd: workDir }); + const remoteOid = lsRemote.stdout.trim().split(/\s+/)[0]; + const localOid = execGit(["rev-parse", "HEAD"], { cwd: workDir }).stdout.trim(); + expect(remoteOid).toBe(localOid); + }); + + it("should not trigger merge-commit fallback for a commit message that starts with 'parent '", async () => { + // Regression test: a commit whose message body starts with "parent " must not be misidentified + // as a merge commit. The old cat-file approach would have counted this as an extra parent. + execGit(["checkout", "-b", "tricky-message-branch"], { cwd: workDir }); + fs.writeFileSync(path.join(workDir, "tricky.txt"), "tricky content\n"); + execGit(["add", "tricky.txt"], { cwd: workDir }); + // Write the multi-line commit message to a file to avoid shell interpretation issues + const msgFile = path.join(workDir, ".git", "TRICKY_MSG"); + fs.writeFileSync(msgFile, "Normal headline\n\nparent this line starts with parent but is not a git parent header\n"); + execGit(["commit", "-F", msgFile], { cwd: workDir }); + execGit(["push", "-u", "origin", "tricky-message-branch"], { cwd: workDir }); + + global.exec = makeRealExec(workDir); + const githubClient = makeMockGithubClient(); + + await pushSignedCommits({ + githubClient, + owner: "test-owner", + repo: "test-repo", + branch: "tricky-message-branch", + baseRef: "origin/main", + cwd: workDir, + }); + + // Must proceed via GraphQL – not incorrectly fallen back to git push + expect(githubClient.graphql).toHaveBeenCalledTimes(1); + expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("merge commit")); + }); + }); });