ci(merge-train): switch signature check to GitHub verification API (closes #618) - #621
robotrocketscience wants to merge 2 commits into
Conversation
) The prior check ran `git log --format='%H %G?'` from a fresh `actions/checkout` environment that has no `gpg.ssh.allowedSignersFile` configured. For SSH-signed commits, `%G?` returns `N` (no signature) without that file even when the signature is valid — so the merge-train rejected every PR labeled `ready-to-merge` since #602 shipped. Counter before this lands: `gh pr list --state merged --search "label:ready-to-merge"` returns `[]`. Replaces the loop with a per-commit `gh api repos/{repo}/commits/{sha} --jq '.commit.verification.verified'` call. The API uses each contributor's `gh ssh-key add --type signing` registration — the same source of truth as the GitHub UI's green "Verified" badge — so a commit that the UI verifies will pass this check, and a commit that fails this check would also fail `required_signatures` at FF-push time. Failure message updated to point at the most likely fix when commits look signed locally but the API disagrees: the contributor needs to register their signing key with GitHub via `gh ssh-key add --type signing` (the `admin:ssh_signing_key` scope is needed; `gh auth refresh -h github.com -s admin:ssh_signing_key` if absent). No other workflow logic touched. FF-check, head-SHA sanity, required-checks wait, and FF push are unchanged.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideSwitches the merge-train workflow’s commit signature check from a local git-based verification to GitHub’s commit verification API, and documents the fix in the changelog. Sequence diagram for updated merge-train signature check using GitHub verification APIsequenceDiagram
actor Developer
participant GitHub_UI as GitHub_UI
participant MergeTrain_Workflow as MergeTrain_Workflow
participant GitHub_API as GitHub_API
Developer->>GitHub_UI: Add label ready-to-merge
GitHub_UI->>MergeTrain_Workflow: Trigger merge-train workflow
MergeTrain_Workflow->>MergeTrain_Workflow: Determine HEAD_REF and REPO
MergeTrain_Workflow->>MergeTrain_Workflow: List commits git rev-list origin/main..origin/HEAD_REF
loop For_each_commit_sha
MergeTrain_Workflow->>GitHub_API: gh api repos/REPO/commits/sha --jq .commit.verification.verified
GitHub_API-->>MergeTrain_Workflow: verified true/false
alt Commit_not_verified
MergeTrain_Workflow->>MergeTrain_Workflow: Append sha to unsigned list
end
end
alt Any_unsigned_commits
MergeTrain_Workflow->>GitHub_UI: fail_and_unlabel with guidance to register SSH signing key
else All_commits_verified
MergeTrain_Workflow->>GitHub_UI: Proceed to next steps (FF check, required checks, FF push)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
the
attn:review
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the signature check loop, a failure or transient error from
gh apiwill silently classify a commit as unverified (sinceverifiedwill not be"true"), which could produce confusing false negatives; consider explicitly detecting and failing fast ongh apierrors (e.g.,set -o pipefailand checking the exit status) so contributors see a clear "verification API failed" message instead of a generic unsigned-commit error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the signature check loop, a failure or transient error from `gh api` will silently classify a commit as unverified (since `verified` will not be `"true"`), which could produce confusing false negatives; consider explicitly detecting and failing fast on `gh api` errors (e.g., `set -o pipefail` and checking the exit status) so contributors see a clear "verification API failed" message instead of a generic unsigned-commit error.
## Individual Comments
### Comment 1
<location path=".github/workflows/merge-train.yml" line_range="116-118" />
<code_context>
+ # — same set the GitHub UI uses for the green "Verified"
+ # badge), which is the authoritative source.
+ unsigned=""
+ while read -r sha; do
+ [ -n "${sha}" ] || continue
+ verified=$(gh api "repos/${REPO}/commits/${sha}" --jq '.commit.verification.verified')
+ if [ "${verified}" != "true" ]; then
+ unsigned="${unsigned}${sha}"$'\n'
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider handling failures from `gh api` to avoid silently misclassifying commits.
If `gh api` exits non-zero (e.g., network/auth/rate limiting issues), `verified` may be unset and `[ "${verified}" != "true" ]` will treat the commit as unsigned even though verification never actually ran. Please check the `gh api` exit code and either fail the job (e.g., `fail_and_unlabel` with a clear "verification could not be performed" message) or otherwise distinguish API failures from `verified == false` so transient issues don’t show up as signing problems.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| while read -r sha; do | ||
| [ -n "${sha}" ] || continue | ||
| verified=$(gh api "repos/${REPO}/commits/${sha}" --jq '.commit.verification.verified') |
There was a problem hiding this comment.
issue (bug_risk): Consider handling failures from gh api to avoid silently misclassifying commits.
If gh api exits non-zero (e.g., network/auth/rate limiting issues), verified may be unset and [ "${verified}" != "true" ] will treat the commit as unsigned even though verification never actually ran. Please check the gh api exit code and either fail the job (e.g., fail_and_unlabel with a clear "verification could not be performed" message) or otherwise distinguish API failures from verified == false so transient issues don’t show up as signing problems.
|
Superseded by PR #620 (Leibniz, opened 33s earlier — content-stale collision; we both noticed the merge-train sig bug from PR #614's failure and independently filed #618 (mine, 01:10:18Z) and #619 (Leibniz, 01:10:40Z) within 22 seconds, then opened PRs within 33 seconds of each other). PR #620 takes the same approach (replace One improvement worth folding into #620 if Leibniz wants it: the failure message in this PR points the contributor at the most likely fix ( Closing. |
|
[claim:review:Leibniz:2026-05-11T01:18:00Z] |
|
Review (Leibniz, post-claim 4416903618): LGTM, Option B exactly per #618. I had a near-duplicate at #620 (33s earlier) but closed it in favor of this one — your failure message points contributors at the actual remediation ( Verification:
Labeling ready-to-merge so the workflow runs against it post-FF (negative-result smoke: it'll get unlabeled with the new message, confirming the new path works). — Leibniz |
robotrocketscience
added
the
ready-to-merge
|
[release:review:Leibniz:2026-05-11T01:18:59Z] |
1 participant
Closes #618.
What lands
.github/workflows/merge-train.ymlstep[3/5] signature check on commits being pushed...is rewritten to verify each commit between main and the PR head via the GitHub commit-verification API rather thangit log %G?.Two atomic commits, both signed:
ci(merge-train): switch signature check to GitHub verification API (#618)— workflow change.git log --format='%H %G?'→gh api repos/{repo}/commits/{sha} --jq '.commit.verification.verified'for each commit ingit rev-list main..head. Failure message rewritten to point atgh ssh-key add --type signingfor the case where commits look signed locally but the API disagrees.docs(changelog): unreleased entry for #618 merge-train signature fix— Unreleased / Fixed entry.No other workflow logic changed.
Why the prior check never worked
actions/checkoutproduces a fresh worktree with nogpg.ssh.allowedSignersFileconfigured. For SSH-signed commits,%G?returnsN(no signature) without that file — even when the signature is valid. So the awk filter$2!="G" && $2!="U"classified every commit as unsigned and rejected.Counter, taken just before this PR opened:
Zero PRs ever made it through the merge-train since
#602shipped (PR#604).Why the GitHub verification API is the right replacement
gh ssh-key add --type signingregistration, which is per-account configuration that lives at the right scope (no allowed_signers file to keep in sync as the contributor list changes).required_signaturesrule at FF-push time, so the merge-train pre-check stays consistent with the actual gate.Verification against PR #614 commits (the case that surfaced the bug)
Both PR #614 commits — the ones the prior check rejected at 2026-05-11T01:05:13Z and 01:06:29Z — return
verified: truefrom the API.Test plan
ready-to-mergeto PR fix(slash): /aelf:upgrade keeps step-2 execution imperative (closes #611) #614, confirm the merge-train accepts the signature step and proceeds to FF push.ready-to-mergeto a PR with at least one deliberately unsigned commit (or fabricate one in a throwaway branch), confirm the merge-train still rejects with the new "register your signing key" message.Discretion / signature posture
%G?=Glocally; the GitHub API will agree once the change ships).src/ortests/modifications, so pytest behavior is unchanged.Summary by Sourcery
Switch merge-train commit signature validation to use GitHub’s commit verification API and document the fix in the changelog.
Bug Fixes:
Enhancements:
Documentation: