Repository navigation
fix(verify): report unverified commits under required_signatures - #129
Merged
Merged
Conversation
`verify-satisfied` listed `required_signatures` among the rule types that cannot hold a squash merge, so a PR carrying an unsigned commit read as done. GitHub holds such a PR BLOCKED, squash armed or not: measured 2026-10-02 on boj-server-cartridges#155, armed and blocked by one unsigned coderabbitai[bot] autofix commit. - squabble-core: new agent item `unverified_commits`, raised when the base requires signatures and any PR commit lacks a verified signature. The describe line names each commit, its GitSignatureState (or UNSIGNED) and author, and the re-sign recipe. A merged or closed PR owes nothing. - squabble-forge: pr_done.graphql reads every PR commit's signature through a third paged connection (`prCommits`, 100 per page, MAX_PAGES bound). - squabble-cli: base_gate also reads classic protection's `required_signatures.enabled`; the rules API reports ruleset rules only. Tests: a planted positive (killed by an `if false` mutant of the rule), three negatives, paging across two commit pages with an absent and an invalid signature, and the classic toggle on/off/absent/invalid. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wo32J8Ym7XpPr9EYdBCgVB
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (23)
🧰 Additional context used📚 Code guidelines (1)📓 Path-based instructions (1)Source excerpt: SPDX: `MPL-2.0` on all new files.📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
🔇 Additional comments (1)
📝 SummarySummary by CodeRabbit
Walkthrough
ChangesCommit signature checks
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant verify_satisfied
participant fetch_pr_done
participant GitHub_GraphQL
participant evaluate
verify_satisfied->>fetch_pr_done: Fetch pull request facts
fetch_pr_done->>GitHub_GraphQL: Request PR commit page
GitHub_GraphQL-->>fetch_pr_done: Return commit signatures and page cursor
fetch_pr_done->>GitHub_GraphQL: Request next page when a cursor remains
fetch_pr_done-->>verify_satisfied: Return PrFacts with unverified commits
verify_satisfied->>evaluate: Evaluate PR facts and required rule types
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 3 files. (2 skipped: 2 … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title is concise, specific, and accurately describes the main change: reporting unverified commits under the required_signatures rule. |
| Description check | |
The description is complete and relevant. It covers the motivation, implementation changes, testing, live verification limits, checklist status, and changelog update. |
✨ Finishing Touches
📝 Generate docstrings
- Commit to this branch
- Create a new PR
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks each commit’s mark,
And hops through pages in the dark.
Unsigned ones get a clear report,
With names and states in brief retort.
Re-sign, then push; the checks restart.
Comment @coderabbitai help to get the list of available commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
squabble verify-satisfiednow reports a PR as not done when the base branch requires signed commits and any commit on the PR lacks a verified signature.Why.
ACCOUNTED_RULE_TYPESlistedrequired_signaturesamong the rule types that cannot hold a squash merge, so a PR carrying an unsigned commit came back DONE. GitHub holds such a PRBLOCKED, squash armed or not. Measured 2026-10-02 onboj-server-cartridges#155: automerge was armed, and the PR was blocked by one unsignedcoderabbitai[bot]autofix commit (8546edbde6). The same gap was logged on nextgen-databases#107, panll#136 and project-wharf#99 (dev-notesinbox/findings.md, 2026-10-02; VERIFICATION.md row P0-004).Closes: no issue. The open issues #117, #118 and #121 do not cover signatures; the finding lives in the owner's findings ledger.
Changes
done.rs):UnverifiedCommit { oid, state, author }andPrFacts::unverified_commits. The field is#[serde(default)], so older JSON still reads.unverified_commits, raised only whenrule_typescontainsrequired_signaturesand the list is non-empty. It names each commit with itsGitSignatureState, orUNSIGNEDwhen there is no signature at all, plus the author and the re-sign recipe (git rebase --exec 'git commit --amend --no-edit -S' <merge-base>, then--force-with-lease). The re-sign keeps the bot as author and makes the person re-signing the committer.ACCOUNTED_RULE_TYPESkeepsrequired_signatures; its doc now says the rule is evaluated overunverified_commitsrather than met by construction.pr_done.graphql,pr_done.rs):prCommits: commits(first: 100, after: $cmtAfter), readsoid, the author andsignature { isValid state }for every commit.commits(last: 1)for the head rollup.MAX_PAGESbound and its error.isValid == true.fetch.rs):base_gatealso reads classic protection'srequired_signatures.enabled. The rules API reports ruleset rules only, so a classic toggle was invisible. It is de-duplicated against the ruleset type.Fixedentry under Unreleased.📌 New pins
a7656d493bcfdbedc0c935431fa59d9d9454f6dbuses:,actions.lock,Cargo.lockor container digest change; no new crate dependency.RSR Quality Checklist
Required
just testrecipe, run directly becausecargoon PATH here resolves to a mise shim that refuses this checkout:cargo test --workspace: 60 + 127 + 64 + 35 passed, 0 failed;cargo test -p squabble-cli --features boj: 64 passed, 0 failed.cargo fmt --all -- --check, clean. Thejust fmt-checkrecipe is still a TODO stub, so this goes beyond whatjust qualitychecks.cargo clippy --workspace --all-targets -- -D warningsandcargo clippy -p squabble-cli --features boj --all-targets -- -D warnings, both exit 0. Thejust lintrecipe is also a stub.unsafeblocks: none added (git diff origin/main | grep -cE '^\+.*\bunsafe\b'→ 0).MPL-2.0header..envfiles.As Applicable
STATE.a2ml/ECOSYSTEM.a2ml/META.a2ml: not updated. A2ML is retired (D99, D269c), and no project state, integration or architecture decision changed.ACCOUNTED_RULE_TYPES,PrFactsandbase_gatedocs, and thedescribe()text that the CLI prints.TOPOLOGY.md: the repo has none, and the architecture did not change.CHANGELOGupdated.Testing
Unit and fixture, all on this head:
an_unverified_commit_under_required_signatures_is_agent_work.if falsefailed exactly this test (126 passed, 1 failed). The file was restored afterwards.unverified_commits_are_read_across_pages_and_verified_ones_are_not_reported.coderabbitai[bot]commit withsignature: null, and oneisValid: false, UNKNOWN_KEYcommit with no linked user.PR_DONEvalidates against the committed GitHub schema snapshot (pr_done.rstest, viacrate::tests::validate), andvalidator_actually_rejects_a_field_the_schema_lacksshows that validator can fail.classic_required_signatures_is_read_only_when_enabledcovers enabled true and false, the key absent, and invalid JSON (an error).Live, 2026-10-09 12:22–12:31Z. The parts are verified separately, not end to end:
pr_done.graphqlwas run againstboj-server-cartridges#155(merged). It returnedpageInfoandsignature: nullon8546edbde6bycoderabbitai[bot], the same shape as the fixture.squabble verify-satisfied hyperpolymath/boj-server 361returned rc=5 with onlyunresolved_thread.required_signatures, and all its commits are VALID, so nounverified_commitswas raised.branches/main/protectiononmetadatastician/sim-public-relationsreturned{"enabled":true,"url":…}; onmetadatastician/phi-LAM,{"enabled":false,…}. Same shape as the fixtures.branchProtectionRulesover all 465 repos of both owners found classic protection on 4 repos, 2 of which require signatures. Every other repo uses rulesets only.pullRequests(states: OPEN)counts) included no PR with an unverified commit. The#155positive is merged, andverify-satisfieddoes not read the base gate for a merged PR, by design. No PR was planted to manufacture a positive.Screenshots
Merge: squash.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Wo32J8Ym7XpPr9EYdBCgVB