fix: --topo-order and merge commit fallback in push_signed_commits.cjs#26306
fix: --topo-order and merge commit fallback in push_signed_commits.cjs#26306
Conversation
…h_signed_commits.cjs Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5c32795c-82f0-4b61-83c4-2a57c3a67eb7 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes incorrect signed-commit replay behavior when commit timestamps don’t reflect the DAG order, and prevents silently incorrect results when merge commits are present by falling back to a normal git push.
Changes:
- Add
--topo-ordertogit rev-list --reverseto ensure DAG-topological ordering (oldest-first) independent of timestamps. - Add a pre-flight merge-commit detector (multi-parent commits) and fall back to
git pushfor the full series when detected.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/push_signed_commits.cjs | Enforces topological commit ordering and adds merge-commit detection to trigger a safe git push fallback. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 3
| @@ -121,6 +122,18 @@ 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 (more than one parent). The 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 sha of shas) { | |||
| const { stdout: catFileOut } = await exec.getExecOutput("git", ["cat-file", "-p", sha], { cwd }); | |||
| const parentCount = (catFileOut.match(/^parent /gm) || []).length; | |||
| if (parentCount > 1) { | |||
| core.warning(`pushSignedCommits: merge commit ${sha} detected, falling back to git push`); | |||
| throw new Error("merge commit detected"); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
New behavior is introduced here (topo-order rev-list sequencing and merge-commit fallback to git push), but actions/setup/js/push_signed_commits.test.cjs currently has no coverage for either scenario. Please add tests that (1) create out-of-sync commit dates and assert the replay order is still correct, and (2) create a merge commit in the range and assert GraphQL is skipped and git push is used.
| const { stdout: catFileOut } = await exec.getExecOutput("git", ["cat-file", "-p", sha], { cwd }); | ||
| const parentCount = (catFileOut.match(/^parent /gm) || []).length; | ||
| if (parentCount > 1) { | ||
| core.warning(`pushSignedCommits: merge commit ${sha} detected, falling back to git push`); | ||
| throw new Error("merge commit detected"); | ||
| } |
There was a problem hiding this comment.
The merge-commit detection counts ^parent occurrences in the full git cat-file -p output, which includes the commit message after a blank line. A commit message line starting with parent would be miscounted as an extra parent and incorrectly trigger the fallback. Consider using a git format that outputs only parents (e.g., git show -s --format=%P <sha> or git rev-list --parents -n 1 <sha>), or restrict parsing to the header section before the first blank line.
| for (const sha of shas) { | ||
| const { stdout: catFileOut } = await exec.getExecOutput("git", ["cat-file", "-p", sha], { cwd }); | ||
| const parentCount = (catFileOut.match(/^parent /gm) || []).length; | ||
| if (parentCount > 1) { | ||
| core.warning(`pushSignedCommits: merge commit ${sha} detected, falling back to git push`); | ||
| throw new Error("merge commit detected"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This pre-flight check runs git cat-file -p in a separate subprocess for every commit SHA, which can be noticeably slow for larger series. You can detect merge commits in a single git invocation (e.g., git rev-list --parents --reverse --topo-order ${baseRef}..HEAD and treat any line with more than 2 fields as a merge) to reduce process spawn overhead.
There was a problem hiding this comment.
The new --parents approach in this PR directly addresses this concern — merge detection is now done inline from the same rev-list output without any extra subprocess per commit. And the commit message regression test confirms no false positives. 🎯
📰 BREAKING: Report filed by Smoke Copilot · ● 644.7K
|
@copilot review all comments |
…o-order and merge fallback Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9c5e299c-424a-49f8-bf1e-720f35988603 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9c5e299c-424a-49f8-bf1e-720f35988603 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All three reviewer comments have been addressed in commit
|
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
|
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke test summary (run 24426417596) Warning
|
|
Smoke Test Results for this PR — run §24426417572
Overall: PASS ✅ PR Author:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR cleanly addresses the topo-order and merge-commit fallback by reusing the single --parents rev-list output. Both new scenarios are covered by integration tests including a regression test for commit messages that start with "parent ". Looks good overall!
📰 BREAKING: Report filed by Smoke Copilot · ● 644.7K
| expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("merge commit")); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Nice regression test for the "parent " commit message body false-positive — this directly validates that the --parents output approach (parsing only the rev-list line format) doesn't get confused by commit message content. 👍
| core.warning(`pushSignedCommits: merge commit ${sha} detected, falling back to git push`); | ||
| throw new Error("merge commit detected"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The merge commit pre-flight detection using --parents output is a clean single-subprocess approach. One minor note: the comment says "3+ space-separated fields" but fields.length > 2 accurately captures this (sha + parent1 + parent2 = 3 fields). ✅
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Smoke Test Results — Run §24426417551 — ✅ PASSCore Tests (#1–12): ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ PR Review Tests (#13–19):
Overall: PARTIAL (all non-skipped tests passed)
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! Run 24426417551
💥 [THE END] — Illustrated by Smoke Claude · ● 177.2K
| core.warning(`pushSignedCommits: merge commit ${sha} detected, falling back to git push`); | ||
| throw new Error("merge commit detected"); | ||
| } | ||
| } |
There was a problem hiding this comment.
✅ Smoke test review comment #1 — The --parents --topo-order approach correctly combines merge-commit detection with topological ordering in a single git rev-list call. Efficient and correct. Run 24426417551.
| @@ -883,4 +883,152 @@ describe("push_signed_commits integration tests", () => { | |||
| expect(Buffer.from(callArg.fileChanges.additions[0].contents, "base64").toString()).toBe("copy source\n"); | |||
| }); | |||
| }); | |||
There was a problem hiding this comment.
✅ Smoke test review comment #2 — Good coverage of the topological ordering edge case using swapped commit dates. The test clearly validates the fix. Run 24426417551.
|
Commit pushed:
|
push_signed_commits.cjscould replay commits in the wrong order when timestamps diverge (e.g. afterrebase --committer-date-is-author-date), and would silently produce incorrect results when a merge commit was included in the series.Changes
git rev-list: Switched togit rev-list --parents --topo-order --reverseto guarantee graph-topology ordering regardless of commit timestamps, and to obtain parent information for all commits in a single subprocess call.Merge commit pre-flight check: Parses the
--parentsoutput from therev-listcall already made. Each output line is<sha> <parent1> [<parent2> ...]; if any line has 3+ space-separated fields, the commit has ≥2 parents (merge commit) and the entire series falls back togit pushwith a warning — sincecreateCommitOnBranchdoes not support multiple parents. This approach is both efficient (no extra subprocess per SHA) and robust (immune to"parent "text appearing in a commit message body).Non-merge commit series continue through the existing GraphQL signed-commit path unchanged.
Tests
New integration tests added in
push_signed_commits.test.cjs:--no-ffmerge commit and asserts GraphQL is not called andgit pushis used instead."parent "is not misidentified as a merge commit.✨ PR Review Safe Output Test - Run 24426417551
Changeset
push_signed_commits.cjscommit replay ordering with--topo-orderand adds merge-commit fallback togit pushwhen multi-parent commits are detected.Warning
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comchatgpt.cominvalid.example.invalidTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.