From a7656d493bcfdbedc0c935431fa59d9d9454f6db Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Fri, 9 Oct 2026 13:26:49 +0100 Subject: [PATCH] fix(verify): report unverified commits under required_signatures `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 Claude-Session: https://claude.ai/code/session_01Wo32J8Ym7XpPr9EYdBCgVB --- CHANGELOG.adoc | 6 + crates/squabble-cli/src/fetch.rs | 44 ++++- crates/squabble-core/src/done.rs | 110 ++++++++++++- crates/squabble-forge/graphql/pr_done.graphql | 25 ++- crates/squabble-forge/src/pr_done.rs | 152 +++++++++++++++++- 5 files changed, 319 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index eebf918..8c68702 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -46,6 +46,12 @@ https://semver.org/spec/v2.0.0.html[Semantic Versioning]. ==== Fixed +* verify-satisfied: a PR with an unsigned or unverified commit no longer + reads as done when the base requires signed commits. GitHub blocks that + merge, squash included; it is now the agent item `unverified_commits`, + naming each commit with its signature state and author. Every PR commit's + signature is read, paged. A classic-protection `required_signatures` + counts as well as a ruleset one. * workflows: step-level `- uses:` refs were never read; only job-level `uses:` keys were. The issue #15 Actions-policy probes therefore missed every step action. Two existing tests witnessed it and were failing on `main`. diff --git a/crates/squabble-cli/src/fetch.rs b/crates/squabble-cli/src/fetch.rs index d24bdec..06ac64c 100644 --- a/crates/squabble-cli/src/fetch.rs +++ b/crates/squabble-cli/src/fetch.rs @@ -197,6 +197,14 @@ enum ProtectionProbe { #[derive(Debug, Deserialize)] struct ClassicProtection { required_status_checks: Option, + /// `{"enabled": bool}`; read as not required when absent. + required_signatures: Option, +} + +/// The `{url, enabled}` shape classic protection uses for a toggle setting. +#[derive(Debug, Deserialize)] +struct ClassicEnabled { + enabled: bool, } #[derive(Debug, Deserialize)] @@ -273,6 +281,15 @@ fn parse_classic_contexts(json: &str) -> Result, String> { Ok(out) } +/// Whether a classic-protection payload requires signed commits. The rules +/// API reports ruleset rules only, so this is the one place a classic +/// `required_signatures` becomes visible. +fn classic_requires_signatures(json: &str) -> Result { + let p: ClassicProtection = serde_json::from_str(json) + .map_err(|e| format!("could not parse branch-protection response: {e}"))?; + Ok(p.required_signatures.is_some_and(|s| s.enabled)) +} + /// The union of required contexts from the two protection APIs, in /// enforcement order: ruleset contexts first, then classic-only ones. /// `Hidden` is a hard error even when the ruleset yielded contexts — a gate @@ -656,7 +673,8 @@ fn rule_types_from_json(rules_json: &str) -> Result, String> { } /// The base branch's gate as `verify-satisfied` needs it: the required-context -/// union (rulesets ∪ classic protection) and every ruleset rule type. +/// union (rulesets ∪ classic protection) and every ruleset rule type, plus +/// `required_signatures` when classic protection requires signed commits. /// /// Unlike [`run_bundle`] an empty context set is *not* `NoGate` here — "done" /// is still a meaningful question on an ungated branch. A 403 on classic @@ -665,7 +683,15 @@ pub fn base_gate(slug: &str, branch: &str) -> Result<(Vec, Vec), let rules_json = run_gh(&["api", &format!("repos/{slug}/rules/branches/{branch}")])?; let protection = probe_classic_protection(slug, branch)?; let contexts = required_contexts_from_apis(&rules_json, &protection)?; - Ok((contexts, rule_types_from_json(&rules_json)?)) + let mut rule_types = rule_types_from_json(&rules_json)?; + if let ProtectionProbe::Protected(json) = &protection { + if classic_requires_signatures(json)? + && !rule_types.iter().any(|t| t == "required_signatures") + { + rule_types.push("required_signatures".into()); + } + } + Ok((contexts, rule_types)) } #[cfg(test)] @@ -899,6 +925,20 @@ mod tests { assert_eq!(probe_from_stderr("gh: validation failed"), None); } + /// Only `enabled: true` turns the rule on; an absent key or `false` leaves it off. + #[test] + fn classic_required_signatures_is_read_only_when_enabled() { + let on = r#"{"required_signatures": {"url": "u", "enabled": true}}"#; + let off = r#"{"required_signatures": {"url": "u", "enabled": false}}"#; + assert!(classic_requires_signatures(on).expect("parse")); + assert!(!classic_requires_signatures(off).expect("parse")); + assert!( + !classic_requires_signatures(r#"{"enforce_admins": {"enabled": true}}"#) + .expect("parse") + ); + assert!(classic_requires_signatures("not json").is_err()); + } + #[test] fn classic_checks_shape_is_read_without_contexts() { // Some payloads carry only the newer `checks` objects. diff --git a/crates/squabble-core/src/done.rs b/crates/squabble-core/src/done.rs index d676c07..6ee9c3a 100644 --- a/crates/squabble-core/src/done.rs +++ b/crates/squabble-core/src/done.rs @@ -42,8 +42,12 @@ pub const DEFAULT_REVIEW_APPS: &[&str] = &[ ]; /// Ruleset rule types whose effect this gate evaluates, or which cannot hold a -/// squash merge (a squash lands one GitHub-signed commit, so `required_signatures` -/// and `required_linear_history` are met by construction). +/// squash merge. `required_linear_history` is met by construction: a squash +/// lands one commit. `required_signatures` is **not**: GitHub holds the PR +/// `BLOCKED` while any commit on it lacks a verified signature, squash armed or +/// not (measured 2026-10-02 on boj-server-cartridges#155: automerge armed, +/// blocked by one unsigned `coderabbitai[bot]` autofix commit). It is evaluated +/// over [`PrFacts::unverified_commits`]. const ACCOUNTED_RULE_TYPES: &[&str] = &[ "required_status_checks", "pull_request", @@ -95,6 +99,17 @@ pub struct Thread { pub url: String, } +/// A commit on the PR whose signature GitHub does not verify. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct UnverifiedCommit { + pub oid: String, + /// GitHub's `GitSignatureState` (`INVALID`, `UNKNOWN_KEY`, …), or + /// `UNSIGNED` when the commit carries no signature at all. + pub state: String, + /// The author's login (`[bot]` removed), else their git name. + pub author: String, +} + /// Everything the verdict needs, fetched once. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct PrFacts { @@ -115,8 +130,13 @@ pub struct PrFacts { pub unresolved_threads: Vec, /// Reviewers whose latest opinionated review is `CHANGES_REQUESTED`. pub changes_requested_by: Vec, - /// `[.[].type]` of the effective rules on the base branch. + /// `[.[].type]` of the effective rules on the base branch, plus + /// `required_signatures` when classic branch protection requires it. pub rule_types: Vec, + /// Commits on the PR that GitHub does not mark verified, from a complete + /// read of the PR's commits. Agent work only under `required_signatures`. + #[serde(default)] + pub unverified_commits: Vec, /// The PR description, verbatim. Where a red non-required check is /// acknowledged (see [`acknowledged_in`]). Empty when there is none. pub body: String, @@ -138,6 +158,7 @@ pub enum Item { WrongMergeMethod { method: String }, BranchBehind, MergeQueue, + UnverifiedCommits { commits: Vec }, AwaitingApproval { review_decision: String }, AwaitingDeployment, AwaitingHumanMerge { merge_state: String }, @@ -189,6 +210,23 @@ impl Item { Self::MergeQueue => "base branch uses a merge queue — verify-satisfied cannot \ evaluate queue entry, so it refuses rather than pass vacuously" .into(), + Self::UnverifiedCommits { commits } => format!( + "the base requires signed commits and {} commit(s) here are not verified \ + ({}) — GitHub blocks the merge, squash included; re-sign them on the \ + branch (`git rebase --exec 'git commit --amend --no-edit -S' `) \ + and push with --force-with-lease", + commits.len(), + commits + .iter() + .map(|c| format!( + "{} {} by {}", + c.oid.get(..10).unwrap_or(&c.oid), + c.state, + c.author + )) + .collect::>() + .join(", ") + ), Self::AwaitingApproval { review_decision } => { format!("awaiting a human review (reviewDecision={review_decision})") } @@ -408,6 +446,15 @@ pub fn evaluate(facts: &PrFacts, review_apps: &[&str]) -> Verdict { if facts.rule_types.iter().any(|t| t == "required_deployments") { human.push(Item::AwaitingDeployment); } + // A squash does not satisfy `required_signatures` (see ACCOUNTED_RULE_TYPES): + // every commit on the PR must verify, and re-signing them is branch work. + if facts.rule_types.iter().any(|t| t == "required_signatures") + && !facts.unverified_commits.is_empty() + { + agent.push(Item::UnverifiedCommits { + commits: facts.unverified_commits.clone(), + }); + } let unaccounted: Vec<&str> = facts .rule_types .iter() @@ -511,10 +558,20 @@ mod tests { unresolved_threads: vec![], changes_requested_by: vec![], rule_types: vec!["required_status_checks".into(), "deletion".into()], + unverified_commits: vec![], body: String::new(), } } + /// A commit with no signature at all, authored by the CodeRabbit bot. + fn unsigned(oid: &str) -> UnverifiedCommit { + UnverifiedCommit { + oid: oid.into(), + state: "UNSIGNED".into(), + author: "coderabbitai".into(), + } + } + fn kinds(items: &[Item]) -> Vec { items .iter() @@ -779,6 +836,53 @@ mod tests { ); } + /// The planted positive: an armed squash PR that GitHub holds BLOCKED on one + /// unsigned bot commit (boj-server-cartridges#155, 2026-10-02) must not read + /// as done. + #[test] + fn an_unverified_commit_under_required_signatures_is_agent_work() { + let mut f = done_pr(); + f.rule_types.push("required_signatures".into()); + f.unverified_commits.push(unsigned("8546edbde6aa")); + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert_eq!(kinds(&v.agent_items), ["unverified_commits"]); + let line = v.agent_items[0].describe(); + assert!(line.contains("8546edbde6"), "{line}"); + assert!(line.contains("--force-with-lease"), "{line}"); + assert!( + !v.notes.iter().any(|n| n.contains("required_signatures")), + "an accounted rule type must not also be reported as unevaluated: {:?}", + v.notes + ); + } + + /// The rule is on, but every commit verifies: nothing is owed. + #[test] + fn verified_commits_under_required_signatures_are_done() { + let mut f = done_pr(); + f.rule_types.push("required_signatures".into()); + assert!(evaluate(&f, DEFAULT_REVIEW_APPS).is_done()); + } + + /// Without the rule, GitHub merges unsigned commits, so they are no work. + #[test] + fn an_unverified_commit_without_required_signatures_is_not_agent_work() { + let mut f = done_pr(); + f.unverified_commits.push(unsigned("8546edbde6aa")); + assert!(evaluate(&f, DEFAULT_REVIEW_APPS).is_done()); + } + + /// After the merge the signatures can no longer be changed, so they are no work. + #[test] + fn a_merged_pr_owes_nothing_for_its_unverified_commits() { + let mut f = done_pr(); + f.state = PrState::Merged; + f.auto_merge = None; + f.rule_types.push("required_signatures".into()); + f.unverified_commits.push(unsigned("8546edbde6aa")); + assert!(evaluate(&f, DEFAULT_REVIEW_APPS).is_done()); + } + /// `verify-satisfied` skips the base-gate REST read for a PR that is no /// longer open. That is sound only while the base gate cannot move a closed /// or merged verdict's agent items — pin it, with a gate that would fail an diff --git a/crates/squabble-forge/graphql/pr_done.graphql b/crates/squabble-forge/graphql/pr_done.graphql index d0ad5ec..e3d9d01 100644 --- a/crates/squabble-forge/graphql/pr_done.graphql +++ b/crates/squabble-forge/graphql/pr_done.graphql @@ -2,12 +2,15 @@ # Copyright (c) 2026 Jonathan D.A. Jewell (hyperpolymath) # # Everything `squabble verify-satisfied` reads about one pull request, in one -# query. Two connections page independently (head contexts, review threads): -# `fetch_pr_done` in src/pr_done.rs re-issues this document with each cursor until -# both are exhausted and ignores a finished connection's repeat nodes. Heads in -# this estate carry up to ~95 contexts; an unpaginated read reports present -# checks as absent, which would pass a pending review bot vacuously. -query PrDone($owner: String!, $name: String!, $number: Int!, $ctxAfter: String, $thrAfter: String) { +# query. Three connections page independently (head contexts, review threads, +# the PR's commits): `fetch_pr_done` in src/pr_done.rs re-issues this document +# with each cursor until all are exhausted and ignores a finished connection's +# repeat nodes. Heads in this estate carry up to ~95 contexts; an unpaginated +# read reports present checks as absent, which would pass a pending review bot +# vacuously. `prCommits` is aliased because `commits(last: 1)` already reads the +# head; every commit on the PR is read for `required_signatures`, which GitHub +# applies to all of them, not to the squash it lands. +query PrDone($owner: String!, $name: String!, $number: Int!, $ctxAfter: String, $thrAfter: String, $cmtAfter: String) { rateLimit { cost remaining resetAt } repository(owner: $owner, name: $name) { pullRequest(number: $number) { @@ -33,6 +36,16 @@ query PrDone($owner: String!, $name: String!, $number: Int!, $ctxAfter: String, comments(first: 1) { nodes { author { login } url } } } } + prCommits: commits(first: 100, after: $cmtAfter) { + pageInfo { hasNextPage endCursor } + nodes { + commit { + oid + author { name user { login } } + signature { isValid state } + } + } + } commits(last: 1) { nodes { commit { diff --git a/crates/squabble-forge/src/pr_done.rs b/crates/squabble-forge/src/pr_done.rs index c0ab779..0498698 100644 --- a/crates/squabble-forge/src/pr_done.rs +++ b/crates/squabble-forge/src/pr_done.rs @@ -8,14 +8,14 @@ use crate::GraphQlTransport; use serde_json::{json, Value}; -use squabble_core::done::{Mergeability, Observed, PrFacts, PrState, Thread}; +use squabble_core::done::{Mergeability, Observed, PrFacts, PrState, Thread, UnverifiedCommit}; use squabble_core::gate::CheckRun; /// The query document; see `graphql/pr_done.graphql`. pub const PR_DONE: &str = include_str!("../graphql/pr_done.graphql"); /// A safety bound on pages per connection. At 100 per page this is 2 000 -/// contexts or threads; past it the read fails rather than truncating. +/// contexts, threads or commits; past it the read fails rather than truncating. pub const MAX_PAGES: usize = 20; /// What the GraphQL read yields. `facts.required_contexts` and @@ -73,6 +73,27 @@ fn thread(node: &Value) -> Option { }) } +/// A PR commit GitHub does not mark verified: no signature at all, or one whose +/// `isValid` is not `true`. `None` for a verified commit. +fn unverified(node: &Value) -> Option { + let sig = node.pointer("/commit/signature").filter(|v| !v.is_null()); + if sig.and_then(|v| v.get("isValid")).and_then(Value::as_bool) == Some(true) { + return None; + } + let author = s(node, "/commit/author/user/login") + .map(|l| l.strip_suffix("[bot]").unwrap_or(l)) + .or_else(|| s(node, "/commit/author/name")) + .unwrap_or("ghost"); + Some(UnverifiedCommit { + oid: s(node, "/commit/oid").unwrap_or_default().to_string(), + state: sig + .and_then(|v| s(v, "/state")) + .unwrap_or("UNSIGNED") + .to_string(), + author: author.to_string(), + }) +} + /// A connection's page: its nodes and, if more follow, the cursor. fn page(conn: &Value) -> Result<(&[Value], Option), String> { let nodes = conn @@ -106,9 +127,11 @@ pub fn fetch_pr_done( ) -> Result { let mut ctx_after: Option = None; let mut thr_after: Option = None; - let (mut ctx_done, mut thr_done) = (false, false); + let mut cmt_after: Option = None; + let (mut ctx_done, mut thr_done, mut cmt_done) = (false, false, false); let mut observed_all = Vec::new(); let mut threads = Vec::new(); + let mut unverified_commits = Vec::new(); let mut head: Option = None; for _ in 0..MAX_PAGES { @@ -116,7 +139,7 @@ pub fn fetch_pr_done( "query": PR_DONE, "variables": { "owner": owner, "name": name, "number": number, - "ctxAfter": ctx_after, "thrAfter": thr_after, + "ctxAfter": ctx_after, "thrAfter": thr_after, "cmtAfter": cmt_after, } }); let resp = t.execute(&body)?; @@ -138,6 +161,12 @@ pub fn fetch_pr_done( thr_done = next.is_none(); thr_after = next.or(thr_after); } + if !cmt_done { + let (nodes, next) = page(pr.get("prCommits").ok_or("no prCommits")?)?; + unverified_commits.extend(nodes.iter().filter_map(unverified)); + cmt_done = next.is_none(); + cmt_after = next.or(cmt_after); + } if !ctx_done { // No rollup at all (a head nothing has reported on) is an empty set. match pr.pointer("/commits/nodes/0/commit/statusCheckRollup/contexts") { @@ -151,7 +180,7 @@ pub fn fetch_pr_done( } } head.get_or_insert_with(|| pr.clone()); - if ctx_done && thr_done { + if ctx_done && thr_done && cmt_done { let pr = head.expect("set above"); return Ok(PrRead { base_ref: s(&pr, "/baseRefName").unwrap_or_default().to_string(), @@ -185,13 +214,14 @@ pub fn fetch_pr_done( }) .unwrap_or_default(), rule_types: Vec::new(), + unverified_commits, body: s(&pr, "/body").unwrap_or_default().to_string(), }, }); } } Err(format!( - "{owner}/{name}#{number}: more than {MAX_PAGES} pages of contexts or threads — \ + "{owner}/{name}#{number}: more than {MAX_PAGES} pages of contexts, threads or commits — \ refusing to judge a partial read" )) } @@ -204,13 +234,14 @@ mod tests { /// Replays canned pages and records the cursors each request carried. struct Pages { pages: RefCell>, - seen: RefCell>, + seen: RefCell>, } impl GraphQlTransport for Pages { fn execute(&self, body: &Value) -> Result { self.seen.borrow_mut().push(( body["variables"]["ctxAfter"].clone(), body["variables"]["thrAfter"].clone(), + body["variables"]["cmtAfter"].clone(), )); let mut p = self.pages.borrow_mut(); if p.is_empty() { @@ -234,6 +265,7 @@ mod tests { "pageInfo": { "hasNextPage": thr_next.is_some(), "endCursor": thr_next }, "nodes": thr }, + "prCommits": { "pageInfo": { "hasNextPage": false, "endCursor": null }, "nodes": [] }, "commits": { "nodes": [ { "commit": { "statusCheckRollup": { "contexts": { "pageInfo": { "hasNextPage": ctx_next.is_some(), "endCursor": ctx_next }, "nodes": ctx @@ -241,6 +273,22 @@ mod tests { }}}}) } + /// `r` with its `prCommits` connection replaced by one page of `nodes`. + fn with_commits(mut r: Value, nodes: Value, next: Option<&str>) -> Value { + r["data"]["repository"]["pullRequest"]["prCommits"] = json!({ + "pageInfo": { "hasNextPage": next.is_some(), "endCursor": next }, + "nodes": nodes + }); + r + } + + /// A PR commit node; `sig` is the `signature` object, or `null` when unsigned. + fn commit(oid: &str, login: Option<&str>, name: &str, sig: Value) -> Value { + json!({ "commit": { "oid": oid, + "author": { "name": name, "user": login.map(|l| json!({ "login": l })) }, + "signature": sig } }) + } + fn run(name: &str) -> Value { json!({ "__typename": "CheckRun", "name": name, "status": "COMPLETED", "conclusion": "SUCCESS", "checkSuite": { "app": { "slug": "github-actions" } } }) @@ -289,6 +337,96 @@ mod tests { ); } + /// Only a signature GitHub verifies passes; an absent one and an invalid one + /// are both reported, and the commit list is read past its first page. + #[test] + fn unverified_commits_are_read_across_pages_and_verified_ones_are_not_reported() { + let base = || resp(json!([]), None, json!([]), None); + let p = Pages { + pages: RefCell::new(vec![ + with_commits( + base(), + json!([ + commit( + "aaaa", + Some("owner"), + "Owner", + json!({ "isValid": true, "state": "VALID" }) + ), + commit( + "bbbb", + Some("coderabbitai[bot]"), + "coderabbitai[bot]", + Value::Null + ), + ]), + Some("K1"), + ), + with_commits( + base(), + json!([commit( + "cccc", + None, + "Someone", + json!({ "isValid": false, "state": "UNKNOWN_KEY" }) + )]), + None, + ), + ]), + seen: RefCell::new(vec![]), + }; + let r = fetch_pr_done(&p, "o", "r", 1).unwrap(); + assert_eq!( + r.facts.unverified_commits, + [ + UnverifiedCommit { + oid: "bbbb".into(), + state: "UNSIGNED".into(), + author: "coderabbitai".into() + }, + UnverifiedCommit { + oid: "cccc".into(), + state: "UNKNOWN_KEY".into(), + author: "Someone".into() + }, + ] + ); + let seen = p.seen.borrow(); + assert_eq!( + seen.len(), + 2, + "the commit connection alone drove a second request" + ); + assert_eq!( + seen[1].2, + json!("K1"), + "second request carried the commit cursor" + ); + } + + /// A PR whose commits all verify reports an empty list. + #[test] + fn a_pr_whose_commits_all_verify_reports_none() { + let p = Pages { + pages: RefCell::new(vec![with_commits( + resp(json!([]), None, json!([]), None), + json!([commit( + "aaaa", + Some("owner"), + "Owner", + json!({ "isValid": true, "state": "VALID" }) + )]), + None, + )]), + seen: RefCell::new(vec![]), + }; + assert!(fetch_pr_done(&p, "o", "r", 1) + .unwrap() + .facts + .unverified_commits + .is_empty()); + } + #[test] fn graphql_errors_fail_closed() { let p = Pages {