babysit_merge.py computes whether the base ruleset requires signed commits, reports it in effectiveRules, and then never checks whether the head is actually signed. When an unsigned commit is the reason a PR cannot merge, the wrapper's own diagnosis names four other causes and omits the real one.
The state of the code on main
requireSignatures appears exactly twice — the default and the assignment:
256: "requireSignatures": False,
304: summary["requireSignatures"] = True
It is emitted in the effectiveRules block and consumed by nothing. A grep for .commit.verification, "verification", or verified across all 1504 lines returns nothing — the wrapper never reads commit verification at all.
What the operator sees
The blocker list (lines ~934-1003) covers thirteen conditions: owner scope, state, draft, mergeable, mergeStateStatus, CHANGES_REQUESTED, missing approvals, unresolved threads, failing checks, pending checks, unmet required contexts, merge queue, moved head, dependency author, unprotected base. Signatures are absent from all of them.
So on a branch whose checks are green, threads resolved, reviews satisfied, and head pinned — but carrying one unsigned or mis-authored commit — GitHub returns mergeStateStatus=BLOCKED, every specific blocker is empty, and the only line emitted is the generic one:
mergeStateStatus=BLOCKED (need CLEAN/HAS_HOOKS: integrates required checks,
up-to-date, approvals, conversation resolution)
That parenthetical enumerates four causes. None of them is the actual cause. This is worse than reporting nothing: an operator who trusts it goes and re-reads checks, approvals, and threads — all of which are already fine — while the signature problem sits unnamed.
Why this is worth fixing rather than documenting
It has already cost real time on this repo. The failure mode is nastier than "unsigned", because a commit can be signed and still rejected:
A valid signature does not rescue a bad author identity — t@t.test yields verified:false, reason:no_user. It needs --reset-author too.
That is verified=false with a perfectly good signature attached, and no tool in the loop says so. Issue #2162 (test harnesses writing t@t.test and commit.gpgsign false into whatever directory they are handed) is the upstream cause that keeps producing exactly this state, and #631 covers unregistered signing keys producing it a different way. The wrapper is the one place an operator looks when a merge is held, and it is silent on the one condition those two issues generate.
Suggested fix
The data is one call the wrapper is already positioned to make:
gh api repos/{owner}/{repo}/commits/<sha> --jq '.commit.verification'
-> {"verified": true|false, "reason": "valid" | "no_user" | "unsigned" | ...}
When rules["requireSignatures"] is true, walk the PR's commits (paginated, per_page=100) and emit a blocker naming each offending commit with its reason — unsigned, no_user, unknown_key are different remedies and should not be collapsed into one message. no_user in particular should say what it means, since "your signature is valid but your author email is not linked to the account" is not guessable from the reason string.
Two smaller points worth folding in:
- Line ~946's enumeration should list signatures regardless. Even if the per-commit walk is judged too expensive for the default read-only pass, the generic
mergeStateStatus message currently misdirects by omission. Adding "signatures" to that list is a one-line honesty fix.
- Report it in the read-only pass, not only at merge time. The wrapper's documented purpose is to report readiness "so the caller can react"; a blocker discovered only when
--merge is attempted defeats that.
Related
babysit_merge.pycomputes whether the base ruleset requires signed commits, reports it ineffectiveRules, and then never checks whether the head is actually signed. When an unsigned commit is the reason a PR cannot merge, the wrapper's own diagnosis names four other causes and omits the real one.The state of the code on
mainrequireSignaturesappears exactly twice — the default and the assignment:It is emitted in the
effectiveRulesblock and consumed by nothing. A grep for.commit.verification,"verification", orverifiedacross all 1504 lines returns nothing — the wrapper never reads commit verification at all.What the operator sees
The blocker list (lines ~934-1003) covers thirteen conditions: owner scope, state, draft,
mergeable,mergeStateStatus,CHANGES_REQUESTED, missing approvals, unresolved threads, failing checks, pending checks, unmet required contexts, merge queue, moved head, dependency author, unprotected base. Signatures are absent from all of them.So on a branch whose checks are green, threads resolved, reviews satisfied, and head pinned — but carrying one unsigned or mis-authored commit — GitHub returns
mergeStateStatus=BLOCKED, every specific blocker is empty, and the only line emitted is the generic one:That parenthetical enumerates four causes. None of them is the actual cause. This is worse than reporting nothing: an operator who trusts it goes and re-reads checks, approvals, and threads — all of which are already fine — while the signature problem sits unnamed.
Why this is worth fixing rather than documenting
It has already cost real time on this repo. The failure mode is nastier than "unsigned", because a commit can be signed and still rejected:
That is
verified=falsewith a perfectly good signature attached, and no tool in the loop says so. Issue #2162 (test harnesses writingt@t.testandcommit.gpgsign falseinto whatever directory they are handed) is the upstream cause that keeps producing exactly this state, and #631 covers unregistered signing keys producing it a different way. The wrapper is the one place an operator looks when a merge is held, and it is silent on the one condition those two issues generate.Suggested fix
The data is one call the wrapper is already positioned to make:
When
rules["requireSignatures"]is true, walk the PR's commits (paginated,per_page=100) and emit a blocker naming each offending commit with itsreason—unsigned,no_user,unknown_keyare different remedies and should not be collapsed into one message.no_userin particular should say what it means, since "your signature is valid but your author email is not linked to the account" is not guessable from the reason string.Two smaller points worth folding in:
mergeStateStatusmessage currently misdirects by omission. Adding "signatures" to that list is a one-line honesty fix.--mergeis attempted defeats that.Related
required_signaturesblocking PRs from machines with unregistered signing keys.