From 20659300d29475c99e010f3cedaeb4d437273499 Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:02:05 +0100 Subject: [PATCH 1/6] feat(verify-satisfied): definition of done for a PR, exit 5 if not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `squabble verify-satisfied ` answers "has the agent finished its part?" — distinct from "can this merge?". Agent items (conflict, draft, failing/missing required check, unresolved thread, bot CHANGES_REQUESTED, pending review bot, automerge unarmed or not squash, merge queue) make it exit 5; human-held items (required approval, deployment, a CLEAN PR GitHub will not automerge) are listed but do not. - squabble-core::done: the pure evaluator, 16 tests. - CheckRun::from_github: one mapping of GitHub status/conclusion, shared by fetch and the new reader. - squabble-forge::pr_done: one GraphQL query, two independently paginated connections (contexts, threads), fail-closed on errors or more than 20 pages; schema-validated against the snapshot. - fetch::base_gate: required contexts (rulesets ∪ classic) plus every ruleset rule type; empty is a real answer here, not NoGate. Check 4 (new code-scanning alerts) is not evaluated yet and says so in every verdict's notes rather than passing silently. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0136eszqrQ53Kj7aBH1D4rXK --- crates/squabble-cli/src/fetch.rs | 40 +- crates/squabble-cli/src/main.rs | 10 +- crates/squabble-cli/src/verify.rs | 150 +++++ crates/squabble-core/src/done.rs | 636 ++++++++++++++++++ crates/squabble-core/src/gate.rs | 27 + crates/squabble-core/src/lib.rs | 1 + crates/squabble-forge/graphql/pr_done.graphql | 53 ++ crates/squabble-forge/src/lib.rs | 4 +- crates/squabble-forge/src/pr_done.rs | 321 +++++++++ 9 files changed, 1226 insertions(+), 16 deletions(-) create mode 100644 crates/squabble-cli/src/verify.rs create mode 100644 crates/squabble-core/src/done.rs create mode 100644 crates/squabble-forge/graphql/pr_done.graphql create mode 100644 crates/squabble-forge/src/pr_done.rs diff --git a/crates/squabble-cli/src/fetch.rs b/crates/squabble-cli/src/fetch.rs index 2b09b28..59c8b20 100644 --- a/crates/squabble-cli/src/fetch.rs +++ b/crates/squabble-cli/src/fetch.rs @@ -68,19 +68,7 @@ struct RulesetContext { /// Parse a `gh pr view --json baseRefName,statusCheckRollup` payload into the /// realised-run half of a [`Gate`]. Pure — no IO, fully testable on fixtures. fn parse_rollup(entry: &RollupEntry) -> CheckRun { - match entry.conclusion.as_deref() { - Some("SUCCESS") => CheckRun::Passed, - Some("SKIPPED") | Some("NEUTRAL") => CheckRun::Skipped, - Some("FAILURE") - | Some("ERROR") - | Some("TIMED_OUT") - | Some("CANCELLED") - | Some("STARTUP_FAILURE") => CheckRun::Failed, - _ => match entry.status.as_deref() { - Some("COMPLETED") => CheckRun::Failed, // completed with no recognised conclusion - _ => CheckRun::Pending, - }, - } + CheckRun::from_github(entry.status.as_deref(), entry.conclusion.as_deref()) } /// Build a [`Gate`] from the required-context set and the realised rollup. @@ -658,6 +646,32 @@ pub fn run_bundle(slug: &str, pr: &str) -> Result { }) } +/// Every rule type the base branch's rulesets carry, deduplicated, in order. +fn rule_types_from_json(rules_json: &str) -> Result, String> { + let rules: Vec = serde_json::from_str(rules_json) + .map_err(|e| format!("could not parse ruleset response: {e}"))?; + let mut out: Vec = Vec::new(); + for t in rules.into_iter().map(|r| r.rule_type) { + if !out.contains(&t) { + out.push(t); + } + } + Ok(out) +} + +/// The base branch's gate as `verify-satisfied` needs it: the required-context +/// union (rulesets ∪ classic protection) and every ruleset rule type. +/// +/// 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 +/// protection is still a hard error, for the same vacuous-green reason. +pub fn base_gate(slug: &str, branch: &str) -> Result<(Vec, Vec), FetchError> { + 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)?)) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/squabble-cli/src/main.rs b/crates/squabble-cli/src/main.rs index 7050db8..6d5d9bb 100644 --- a/crates/squabble-cli/src/main.rs +++ b/crates/squabble-cli/src/main.rs @@ -18,6 +18,8 @@ //! - `2` — a genuine failure: bad usage, `gh` failed, unparseable input. //! - `4` — **blocking chain finding** (`squabble chains`): a dependency cycle //! or a dead upstream. Like `3`, a reportable finding, not a tool failure. +//! - `5` — **not done** (`squabble verify-satisfied`): agent items remain on +//! the PR (unresolved threads, a failing required check, automerge unarmed…). //! - `3` — **no gate**: the PR's base branch carries no `required_status_checks` //! ruleset rule, so there is nothing to triage. //! @@ -34,6 +36,7 @@ mod boj; mod chains; mod fetch; mod fight; +mod verify; use squabble_core::{diagnose, gate::Gate}; use std::process::ExitCode; @@ -57,6 +60,7 @@ fn main() -> ExitCode { }, Some("fight") => fight::run(&args[2..]), Some("chains") => chains::run(&args[2..]), + Some("verify-satisfied") => verify::run(&args[2..]), Some("--version") | Some("-V") => { println!("squabble {}", env!("CARGO_PKG_VERSION")); ExitCode::SUCCESS @@ -71,12 +75,14 @@ fn main() -> ExitCode { squabble diagnose \n \ squabble {}\n \ squabble {}\n \ + squabble {}\n \ squabble --version\n\n\ EXIT CODES:\n \ - 0 ok · 2 failure · 3 no `required_status_checks` rule on the base branch · 4 chains: blocking finding\n", + 0 ok · 2 failure · 3 no `required_status_checks` rule on the base branch · 4 chains: blocking finding · 5 verify-satisfied: agent items remain\n", env!("CARGO_PKG_VERSION"), fight::USAGE.trim_start_matches("usage: squabble "), - chains::USAGE.trim_start_matches("usage: squabble ") + chains::USAGE.trim_start_matches("usage: squabble "), + verify::USAGE.trim_start_matches("usage: squabble ") ); ExitCode::from(2) } diff --git a/crates/squabble-cli/src/verify.rs b/crates/squabble-cli/src/verify.rs new file mode 100644 index 0000000..fda8b99 --- /dev/null +++ b/crates/squabble-cli/src/verify.rs @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2026 Jonathan D.A. Jewell (hyperpolymath) +//! `squabble verify-satisfied ` — is this PR actually done? +//! +//! Reads the PR (GraphQL, `squabble-forge::pr_done`) and its base branch's +//! gate (REST, `fetch::base_gate`), then asks the pure evaluator in +//! `squabble-core::done`. The verdict goes to stdout as JSON; a human-readable +//! list goes to stderr. Exit `5` means agent work remains — the one answer a +//! hook needs, so hook and human read the same implementation. + +use crate::fetch; +use serde::Serialize; +use squabble_core::done::{evaluate, Verdict, DEFAULT_REVIEW_APPS}; +use squabble_forge::pr_done::fetch_pr_done; +use squabble_forge::GhTransport; +use std::process::ExitCode; + +pub const USAGE: &str = "usage: squabble verify-satisfied / "; + +/// Exit code for "the PR is not done: agent items remain". +pub const NOT_DONE_EXIT: u8 = 5; + +#[derive(Serialize)] +struct Report<'a> { + repo: &'a str, + pr: u64, + head_oid: String, + base_ref: String, + done: bool, + #[serde(flatten)] + verdict: Verdict, +} + +pub fn run(args: &[String]) -> ExitCode { + let (slug, pr) = match args { + [slug, pr] => match (slug.split_once('/'), pr.parse::()) { + (Some(_), Ok(n)) => (slug.as_str(), n), + _ => { + eprintln!("squabble verify-satisfied: {USAGE}"); + return ExitCode::from(2); + } + }, + _ => { + eprintln!("squabble verify-satisfied: {USAGE}"); + return ExitCode::from(2); + } + }; + let (owner, name) = slug.split_once('/').expect("checked above"); + + let read = match fetch_pr_done(&GhTransport, owner, name, pr) { + Ok(r) => r, + Err(e) => { + eprintln!("squabble verify-satisfied: {e}"); + return ExitCode::from(2); + } + }; + let mut facts = read.facts; + match fetch::base_gate(slug, &read.base_ref) { + Ok((contexts, rule_types)) => { + facts.required_contexts = contexts; + facts.rule_types = rule_types; + } + Err(e) => { + eprintln!("squabble verify-satisfied: {e}"); + return ExitCode::from(fetch::FetchError::FAILED_EXIT); + } + } + + let verdict = evaluate(&facts, DEFAULT_REVIEW_APPS); + let done = verdict.is_done(); + eprint!("{}", render(slug, pr, &verdict)); + let report = Report { + repo: slug, + pr, + head_oid: read.head_oid, + base_ref: read.base_ref, + done, + verdict, + }; + match serde_json::to_string_pretty(&report) { + Ok(json) => println!("{json}"), + Err(e) => { + eprintln!("squabble verify-satisfied: could not serialise verdict: {e}"); + return ExitCode::from(2); + } + } + if done { + ExitCode::SUCCESS + } else { + ExitCode::from(NOT_DONE_EXIT) + } +} + +fn render(slug: &str, pr: u64, v: &Verdict) -> String { + let mut s = format!( + "{slug}#{pr}: {}\n", + if v.is_done() { + "DONE (nothing left for the agent)" + } else { + "NOT DONE" + } + ); + let mut section = |title: &str, lines: Vec| { + if !lines.is_empty() { + s.push_str(&format!(" {title}:\n")); + for l in lines { + s.push_str(&format!(" - {l}\n")); + } + } + }; + section( + "agent must act", + v.agent_items.iter().map(|i| i.describe()).collect(), + ); + section( + "held by a human", + v.held_by_human.iter().map(|i| i.describe()).collect(), + ); + section( + "satisfied without evidence (skipped/neutral)", + v.evidence_free.clone(), + ); + section("notes", v.notes.clone()); + s +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A hook keys on this number; it must not collide with any other code the + /// binary already uses (2 failure, 3 no gate, 4 chains blocking). + #[test] + fn not_done_has_its_own_exit_code() { + for other in [0u8, 2, 3, 4] { + assert_ne!(NOT_DONE_EXIT, other); + } + } + + #[test] + fn malformed_arguments_are_a_usage_failure_not_a_verdict() { + for bad in [ + vec![], + vec!["norepo".to_string(), "1".to_string()], + vec!["o/r".to_string(), "x".to_string()], + ] { + assert_eq!(run(&bad), ExitCode::from(2)); + } + } +} diff --git a/crates/squabble-core/src/done.rs b/crates/squabble-core/src/done.rs new file mode 100644 index 0000000..0311f09 --- /dev/null +++ b/crates/squabble-core/src/done.rs @@ -0,0 +1,636 @@ +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2026 Jonathan D.A. Jewell (hyperpolymath) +//! `verify-satisfied` — the definition of done for a pull request, as a pure +//! function over facts already fetched. +//! +//! The question is not "can this merge?" but **"has the agent finished its part?"** +//! Those differ, and the difference is the whole design: +//! +//! - [`Verdict::agent_items`] — work the agent can and must still do: resolve a +//! conflict in-file, answer a review thread, act on a `CHANGES_REQUESTED`, +//! wait for a review bot to report, arm automerge, pick squash. Non-empty ⇒ +//! **not done** (the CLI exits 5). +//! - [`Verdict::held_by_human`] — what only a person can clear: a required +//! approval, a deployment gate, pressing merge on a PR that GitHub will not let +//! automerge (it refuses on a `CLEAN` status). Listed, but **done** — "armed and +//! waiting for the owner" is the intended terminal state, and a gate that failed +//! on it could never release a session. +//! +//! Required checks that are merely *pending* are not agent work when automerge is +//! armed: GitHub waits for them. Demanding green here would be jointly +//! unsatisfiable with demanding automerge, because GitHub refuses to arm +//! automerge on a PR whose checks are all green. +//! +//! A review bot is different. It is not required, so automerge will not wait for +//! it — and its output is exactly what the agent must have read. A pending review +//! bot is therefore agent work: the one place this gate waits on wall-clock. +//! +//! Not yet evaluated, and said so in [`Verdict::notes`] rather than passed +//! silently: new code-scanning alerts (the PR-minus-base set difference). + +use crate::gate::{CheckRun, Gate, RequiredCheck}; +use serde::{Deserialize, Serialize}; + +/// Review-producing Apps, by login with any `[bot]` suffix removed. Only these +/// make a pending check agent work; any other non-required pending check is +/// noise the agent need not wait for. +pub const DEFAULT_REVIEW_APPS: &[&str] = &[ + "coderabbitai", + "copilot-pull-request-reviewer", + "sonarqubecloud", + "codacy-production", +]; + +/// 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). +const ACCOUNTED_RULE_TYPES: &[&str] = &[ + "required_status_checks", + "pull_request", + "deletion", + "non_fast_forward", + "creation", + "update", + "required_linear_history", + "required_signatures", + "required_deployments", + "code_scanning", +]; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum PrState { + Open, + Merged, + Closed, +} + +/// GitHub's `mergeable`. `Unknown` is computed lazily and is common on the first +/// read after a push — it means "not yet", never "conflicting". +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum Mergeability { + Mergeable, + Conflicting, + Unknown, +} + +/// One context on the head commit, required or not. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct Observed { + pub name: String, + /// App slug (check run) or creator login (status), `[bot]` removed. + pub producer: String, + pub run: CheckRun, +} + +/// An unresolved review thread. An *outdated* one still blocks +/// `required_review_thread_resolution`, so it is counted and tagged, not skipped. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct Thread { + pub author: String, + pub path: Option, + pub line: Option, + pub outdated: bool, + pub url: String, +} + +/// Everything the verdict needs, fetched once. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PrFacts { + pub state: PrState, + pub is_draft: bool, + pub mergeable: Mergeability, + /// GitHub's `mergeStateStatus`, verbatim (`BLOCKED`, `CLEAN`, `BEHIND`, …). + pub merge_state: String, + /// `reviewDecision`: `APPROVED`, `REVIEW_REQUIRED`, `CHANGES_REQUESTED`, or none. + pub review_decision: Option, + /// Armed automerge's method (`SQUASH`, `MERGE`, `REBASE`), if armed. + pub auto_merge: Option, + /// Required contexts from rulesets ∪ classic protection. Empty is a real + /// answer ("nothing required"), not an error. + pub required_contexts: Vec, + /// Every context on the head, from a fully paginated read. + pub observed: Vec, + 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. + pub rule_types: Vec, +} + +/// One unmet condition. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "kind", rename_all = "snake_case")] +pub enum Item { + Conflicting, + MergeabilityUnknown, + Draft, + RequiredNotSatisfied { context: String, run: CheckRun }, + UnresolvedThread(Thread), + ChangesRequested { reviewer: String }, + ReviewBotPending { producer: String, context: String }, + AutoMergeNotArmed { merge_state: String }, + WrongMergeMethod { method: String }, + BranchBehind, + MergeQueue, + AwaitingApproval { review_decision: String }, + AwaitingDeployment, + AwaitingHumanMerge { merge_state: String }, +} + +impl Item { + /// One human line: what is wrong and what closes it. + pub fn describe(&self) -> String { + match self { + Self::Conflicting => "merge conflict — resolve it in the files and push".into(), + Self::MergeabilityUnknown => { + "mergeability not computed yet (common right after a push) — re-run".into() + } + Self::Draft => "PR is a draft — mark it ready for review".into(), + Self::RequiredNotSatisfied { context, run } => { + format!("required check `{context}` is {run:?} — fix it or the ruleset") + } + Self::UnresolvedThread(t) => format!( + "unresolved review thread by {}{}{} — act on it or reply and resolve: {}", + t.author, + t.path + .as_deref() + .map(|p| format!(" on {p}")) + .unwrap_or_default(), + if t.outdated { " (outdated)" } else { "" }, + t.url + ), + Self::ChangesRequested { reviewer } => format!( + "{reviewer} requests changes — act on them, or dismiss the review with a \ + reason (a finding deferred to an issue is dismissed with its link)" + ), + Self::ReviewBotPending { producer, context } => format!( + "review bot {producer} has not reported (`{context}` pending) — its output \ + cannot have been read yet; wait for it" + ), + Self::AutoMergeNotArmed { merge_state } => format!( + "automerge is not armed while the PR waits on requirements \ + (mergeStateStatus={merge_state}) — arm it with squash" + ), + Self::WrongMergeMethod { method } => { + format!("automerge is armed with {method} — re-arm with SQUASH") + } + Self::BranchBehind => "branch is behind its base — update it".into(), + Self::MergeQueue => "base branch uses a merge queue — verify-satisfied cannot \ + evaluate queue entry, so it refuses rather than pass vacuously" + .into(), + Self::AwaitingApproval { review_decision } => { + format!("awaiting a human review (reviewDecision={review_decision})") + } + Self::AwaitingDeployment => "awaiting a required deployment".into(), + Self::AwaitingHumanMerge { merge_state } => format!( + "nothing is pending, so GitHub will not arm automerge \ + (mergeStateStatus={merge_state}) — awaiting a human merge" + ), + } + } +} + +/// The answer. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct Verdict { + pub agent_items: Vec, + pub held_by_human: Vec, + /// Required contexts satisfied by a skip or neutral: met, but no evidence. + pub evidence_free: Vec, + /// What was not evaluated, or looks wrong without being a fail. + pub notes: Vec, +} + +impl Verdict { + /// Done ⇔ nothing is left for the agent. Human-held items do not count. + pub fn is_done(&self) -> bool { + self.agent_items.is_empty() + } +} + +fn strip_bot(login: &str) -> &str { + login.strip_suffix("[bot]").unwrap_or(login) +} + +/// The required half as a [`Gate`], matched on context name (a required check +/// names a context, not a producer). First match wins, as in `squabble fetch`. +pub fn required_gate(facts: &PrFacts) -> Gate { + Gate::new( + facts + .required_contexts + .iter() + .map(|c| { + let run = facts + .observed + .iter() + .find(|o| &o.name == c) + .map(|o| o.run) + .unwrap_or(CheckRun::Missing); + RequiredCheck::new(c.clone(), run) + }) + .collect(), + ) +} + +/// Evaluate the definition of done. +pub fn evaluate(facts: &PrFacts, review_apps: &[&str]) -> Verdict { + let mut agent = Vec::new(); + let mut human = Vec::new(); + let mut notes = Vec::new(); + + let gate = required_gate(facts); + let evidence_free = gate + .evidence_free() + .map(|c| c.required_context.clone()) + .collect(); + + match facts.state { + PrState::Closed => { + notes.push("PR is closed without merging — nothing further to land".into()); + return Verdict { + agent_items: agent, + held_by_human: human, + evidence_free, + notes, + }; + } + PrState::Merged => { + // Threads and requested changes left behind on a merged PR were never + // acted on; they are still the agent's to answer. Checks and + // automerge no longer mean anything. + agent.extend( + facts + .unresolved_threads + .iter() + .cloned() + .map(Item::UnresolvedThread), + ); + return Verdict { + agent_items: agent, + held_by_human: human, + evidence_free, + notes, + }; + } + PrState::Open => {} + } + + // 1. conflicts, resolved in-file + match facts.mergeable { + Mergeability::Conflicting => agent.push(Item::Conflicting), + Mergeability::Unknown => agent.push(Item::MergeabilityUnknown), + Mergeability::Mergeable => {} + } + if facts.is_draft { + agent.push(Item::Draft); + } + if facts.merge_state == "BEHIND" { + agent.push(Item::BranchBehind); + } + + // 2. no required check failed or missing; pending is GitHub's to wait on + let mut required_pending = false; + for c in &gate.checks { + match c.run { + CheckRun::Passed | CheckRun::Skipped => {} + CheckRun::Pending => required_pending = true, + CheckRun::Missing | CheckRun::Failed => agent.push(Item::RequiredNotSatisfied { + context: c.required_context.clone(), + run: c.run, + }), + } + } + if facts.required_contexts.is_empty() { + notes.push( + "no required status checks on the base branch — automerge would merge on \ + arming, before any review bot reports" + .into(), + ); + } + + // 3. review output read and acted on + agent.extend( + facts + .unresolved_threads + .iter() + .cloned() + .map(Item::UnresolvedThread), + ); + agent.extend( + facts + .changes_requested_by + .iter() + .map(|r| Item::ChangesRequested { + reviewer: strip_bot(r).to_string(), + }), + ); + for o in &facts.observed { + let producer = strip_bot(&o.producer); + if o.run == CheckRun::Pending && review_apps.contains(&producer) { + agent.push(Item::ReviewBotPending { + producer: producer.to_string(), + context: o.name.clone(), + }); + } + } + + // 4. new code-scanning alerts — not yet implemented, and not passed silently + notes.push( + "code-scanning alerts introduced by this PR are NOT yet evaluated \ + (PR-minus-base set difference is follow-up work)" + .into(), + ); + + // 7. the rule types, not just the required checks + let queue = facts.rule_types.iter().any(|t| t == "merge_queue"); + if queue { + agent.push(Item::MergeQueue); + } + if facts.rule_types.iter().any(|t| t == "required_deployments") { + human.push(Item::AwaitingDeployment); + } + let unaccounted: Vec<&str> = facts + .rule_types + .iter() + .map(String::as_str) + .filter(|t| *t != "merge_queue" && !ACCOUNTED_RULE_TYPES.contains(t)) + .collect(); + if !unaccounted.is_empty() { + notes.push(format!( + "rule types not evaluated here: {}", + unaccounted.join(", ") + )); + } + // A change request that only review bots made is already agent work above; + // calling it human-held too would tell the agent to wait on itself. + let human_requested_changes = facts + .changes_requested_by + .iter() + .any(|r| !review_apps.contains(&strip_bot(r))); + let awaiting_human = match facts.review_decision.as_deref() { + Some("REVIEW_REQUIRED") => true, + Some("CHANGES_REQUESTED") => { + human_requested_changes || facts.changes_requested_by.is_empty() + } + _ => false, + }; + if awaiting_human { + human.push(Item::AwaitingApproval { + review_decision: facts.review_decision.clone().unwrap_or_default(), + }); + } + + // 5 + 6. automerge armed where GitHub allows it, and with squash + if !queue { + match facts.auto_merge.as_deref() { + Some("SQUASH") => {} + Some(m) => agent.push(Item::WrongMergeMethod { + method: m.to_string(), + }), + None if facts.merge_state == "BLOCKED" || required_pending => { + agent.push(Item::AutoMergeNotArmed { + merge_state: facts.merge_state.clone(), + }) + } + None => human.push(Item::AwaitingHumanMerge { + merge_state: facts.merge_state.clone(), + }), + } + } + + // Safety net: everything evaluated is met, yet GitHub still says BLOCKED. + // Not a fail (BLOCKED can be a false block on a squash repo), but named. + if agent.is_empty() + && facts.auto_merge.is_some() + && !required_pending + && human.is_empty() + && facts.merge_state == "BLOCKED" + { + notes.push(format!( + "every evaluated condition is met and automerge is armed, yet GitHub reports \ + BLOCKED — something unevaluated holds it (rule types: {})", + facts.rule_types.join(", ") + )); + } + + Verdict { + agent_items: agent, + held_by_human: human, + evidence_free, + notes, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn obs(name: &str, producer: &str, run: CheckRun) -> Observed { + Observed { + name: name.into(), + producer: producer.into(), + run, + } + } + + /// An open, mergeable PR with one required check passed and squash armed. + fn done_pr() -> PrFacts { + PrFacts { + state: PrState::Open, + is_draft: false, + mergeable: Mergeability::Mergeable, + merge_state: "BLOCKED".into(), + review_decision: None, + auto_merge: Some("SQUASH".into()), + required_contexts: vec!["build".into()], + observed: vec![ + obs("build", "github-actions", CheckRun::Pending), + obs("CodeRabbit", "coderabbitai[bot]", CheckRun::Passed), + ], + unresolved_threads: vec![], + changes_requested_by: vec![], + rule_types: vec!["required_status_checks".into(), "deletion".into()], + } + } + + fn kinds(items: &[Item]) -> Vec { + items + .iter() + .map(|i| { + serde_json::to_value(i).unwrap()["kind"] + .as_str() + .unwrap() + .to_string() + }) + .collect() + } + + #[test] + fn armed_with_a_required_check_pending_is_done() { + let v = evaluate(&done_pr(), DEFAULT_REVIEW_APPS); + assert!(v.is_done(), "{:?}", v.agent_items); + } + + #[test] + fn a_pending_review_bot_is_agent_work() { + let mut f = done_pr(); + f.observed[1].run = CheckRun::Pending; + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert_eq!(kinds(&v.agent_items), ["review_bot_pending"]); + } + + #[test] + fn a_pending_non_review_check_is_not_agent_work() { + let mut f = done_pr(); + f.observed + .push(obs("lint", "github-actions", CheckRun::Pending)); + assert!(evaluate(&f, DEFAULT_REVIEW_APPS).is_done()); + } + + #[test] + fn an_unresolved_thread_blocks_even_when_outdated() { + let mut f = done_pr(); + f.unresolved_threads.push(Thread { + author: "coderabbitai".into(), + path: Some("src/x.rs".into()), + line: None, + outdated: true, + url: "https://example/t".into(), + }); + assert_eq!( + kinds(&evaluate(&f, DEFAULT_REVIEW_APPS).agent_items), + ["unresolved_thread"] + ); + } + + #[test] + fn a_failed_or_missing_required_check_is_agent_work_but_skipped_is_not() { + let mut f = done_pr(); + f.required_contexts = vec!["build".into(), "scan".into(), "gone".into()]; + f.observed[0].run = CheckRun::Failed; + f.observed + .push(obs("scan", "github-actions", CheckRun::Skipped)); + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert_eq!( + kinds(&v.agent_items), + ["required_not_satisfied", "required_not_satisfied"] + ); + assert_eq!(v.evidence_free, ["scan"]); + } + + #[test] + fn blocked_without_automerge_is_agent_work() { + let mut f = done_pr(); + f.auto_merge = None; + assert_eq!( + kinds(&evaluate(&f, DEFAULT_REVIEW_APPS).agent_items), + ["auto_merge_not_armed"] + ); + } + + /// GitHub refuses to arm automerge on a clean PR, so demanding it there + /// would make the gate unsatisfiable. The owner merges. + #[test] + fn clean_without_automerge_is_held_by_human_not_agent_work() { + let mut f = done_pr(); + f.auto_merge = None; + f.merge_state = "CLEAN".into(); + f.observed[0].run = CheckRun::Passed; + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert!(v.is_done(), "{:?}", v.agent_items); + assert_eq!(kinds(&v.held_by_human), ["awaiting_human_merge"]); + } + + #[test] + fn a_non_squash_automerge_is_agent_work() { + let mut f = done_pr(); + f.auto_merge = Some("REBASE".into()); + assert_eq!( + kinds(&evaluate(&f, DEFAULT_REVIEW_APPS).agent_items), + ["wrong_merge_method"] + ); + } + + #[test] + fn unknown_mergeability_fails_closed_but_is_not_called_a_conflict() { + let mut f = done_pr(); + f.mergeable = Mergeability::Unknown; + assert_eq!( + kinds(&evaluate(&f, DEFAULT_REVIEW_APPS).agent_items), + ["mergeability_unknown"] + ); + } + + #[test] + fn changes_requested_is_agent_work_and_a_required_approval_is_human() { + let mut f = done_pr(); + f.changes_requested_by = vec!["coderabbitai[bot]".into()]; + f.review_decision = Some("REVIEW_REQUIRED".into()); + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert_eq!(kinds(&v.agent_items), ["changes_requested"]); + assert_eq!(kinds(&v.held_by_human), ["awaiting_approval"]); + } + + /// Measured on hypatia#883: `reviewDecision=CHANGES_REQUESTED` there comes + /// from CodeRabbit alone, so it is agent work, not a wait on a human. + #[test] + fn a_change_request_only_a_bot_made_is_not_human_held() { + let mut f = done_pr(); + f.changes_requested_by = vec!["coderabbitai".into()]; + f.review_decision = Some("CHANGES_REQUESTED".into()); + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert_eq!(kinds(&v.agent_items), ["changes_requested"]); + assert!(!kinds(&v.held_by_human).contains(&"awaiting_approval".to_string())); + + f.changes_requested_by.push("a-maintainer".into()); + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert!(kinds(&v.held_by_human).contains(&"awaiting_approval".to_string())); + } + + /// With a merge queue, `autoMergeRequest` is the wrong field: a pass would + /// be vacuous, so the gate refuses. + #[test] + fn a_merge_queue_is_refused_not_passed() { + let mut f = done_pr(); + f.rule_types.push("merge_queue".into()); + assert_eq!( + kinds(&evaluate(&f, DEFAULT_REVIEW_APPS).agent_items), + ["merge_queue"] + ); + } + + #[test] + fn an_unknown_rule_type_is_named_in_the_notes() { + let mut f = done_pr(); + f.rule_types.push("file_path_restriction".into()); + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert!(v.is_done()); + assert!(v.notes.iter().any(|n| n.contains("file_path_restriction"))); + } + + #[test] + fn code_scanning_is_never_passed_silently() { + let v = evaluate(&done_pr(), DEFAULT_REVIEW_APPS); + assert!(v.notes.iter().any(|n| n.contains("NOT yet evaluated"))); + } + + #[test] + fn a_merged_pr_still_owes_its_unresolved_threads() { + let mut f = done_pr(); + f.state = PrState::Merged; + f.auto_merge = None; + f.unresolved_threads.push(Thread { + author: "owner".into(), + path: None, + line: None, + outdated: false, + url: "u".into(), + }); + assert_eq!( + kinds(&evaluate(&f, DEFAULT_REVIEW_APPS).agent_items), + ["unresolved_thread"] + ); + } +} diff --git a/crates/squabble-core/src/gate.rs b/crates/squabble-core/src/gate.rs index 3c0db66..19c6e85 100644 --- a/crates/squabble-core/src/gate.rs +++ b/crates/squabble-core/src/gate.rs @@ -33,6 +33,33 @@ pub enum CheckRun { Passed, } +impl CheckRun { + /// Map GitHub's rollup vocabulary onto a [`CheckRun`]. + /// + /// The rollup is a union: a check run carries `status` + `conclusion`, a + /// legacy commit status carries only `state` (pass it as `conclusion`). One + /// mapping, shared by every reader — `squabble fetch` and `verify-satisfied` + /// must not disagree about what a conclusion means. + /// + /// A completed run with an unrecognised conclusion (`ACTION_REQUIRED`, + /// `STALE`, anything GitHub adds later) is `Failed`, never `Passed`. + pub fn from_github(status: Option<&str>, conclusion: Option<&str>) -> Self { + match conclusion { + Some("SUCCESS") => Self::Passed, + Some("SKIPPED") | Some("NEUTRAL") => Self::Skipped, + Some("FAILURE") + | Some("ERROR") + | Some("TIMED_OUT") + | Some("CANCELLED") + | Some("STARTUP_FAILURE") => Self::Failed, + _ => match status { + Some("COMPLETED") => Self::Failed, + _ => Self::Pending, + }, + } + } +} + /// Why a required context shows [`CheckRun::Missing`]. /// /// `Missing` is the gate's most common stuck state and its least actionable one: diff --git a/crates/squabble-core/src/lib.rs b/crates/squabble-core/src/lib.rs index 101d100..3aa0230 100644 --- a/crates/squabble-core/src/lib.rs +++ b/crates/squabble-core/src/lib.rs @@ -16,6 +16,7 @@ pub mod admission; pub mod chains; +pub mod done; pub mod gate; pub mod moves; pub mod outcome; diff --git a/crates/squabble-forge/graphql/pr_done.graphql b/crates/squabble-forge/graphql/pr_done.graphql new file mode 100644 index 0000000..3a5a378 --- /dev/null +++ b/crates/squabble-forge/graphql/pr_done.graphql @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: MPL-2.0 +# 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) { + rateLimit { cost remaining resetAt } + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + state + isDraft + mergeable + mergeStateStatus + reviewDecision + baseRefName + headRefOid + autoMergeRequest { mergeMethod } + latestOpinionatedReviews(first: 100) { + nodes { state author { login } } + } + reviewThreads(first: 100, after: $thrAfter) { + pageInfo { hasNextPage endCursor } + nodes { + isResolved + isOutdated + path + line + comments(first: 1) { nodes { author { login } url } } + } + } + commits(last: 1) { + nodes { + commit { + statusCheckRollup { + contexts(first: 100, after: $ctxAfter) { + pageInfo { hasNextPage endCursor } + nodes { + __typename + ... on CheckRun { name status conclusion checkSuite { app { slug } } } + ... on StatusContext { context state creator { login } } + } + } + } + } + } + } + } + } +} diff --git a/crates/squabble-forge/src/lib.rs b/crates/squabble-forge/src/lib.rs index 0e835b9..8455138 100644 --- a/crates/squabble-forge/src/lib.rs +++ b/crates/squabble-forge/src/lib.rs @@ -21,6 +21,8 @@ //! becomes [`ScanStatus::NotFound`]; an exhausted rate budget marks the //! remaining repos unavailable rather than guessing. +pub mod pr_done; + use serde_json::{json, Value}; use squabble_core::chains::{RepoId, RepoSnapshot, ScanStatus, SourceCost, WorkflowFile}; use std::io::Write; @@ -354,7 +356,7 @@ mod tests { // --- schema validation ------------------------------------------------- - fn validate(doc: &str) -> Result<(), String> { + pub(crate) fn validate(doc: &str) -> Result<(), String> { use apollo_compiler::{ExecutableDocument, Schema}; let schema = Schema::parse_and_validate(SCHEMA, "github-schema.graphql") .map_err(|e| format!("schema: {}", e.errors))?; diff --git a/crates/squabble-forge/src/pr_done.rs b/crates/squabble-forge/src/pr_done.rs new file mode 100644 index 0000000..f4d2948 --- /dev/null +++ b/crates/squabble-forge/src/pr_done.rs @@ -0,0 +1,321 @@ +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) 2026 Jonathan D.A. Jewell (hyperpolymath) +//! The GraphQL half of `verify-satisfied`: one pull request's mergeability, +//! reviews, threads and head contexts, fully paginated. +//! +//! The REST-only half — required contexts (rulesets ∪ classic protection) and +//! the ruleset rule types — is filled in by the CLI, which already reads both. + +use crate::GraphQlTransport; +use serde_json::{json, Value}; +use squabble_core::done::{Mergeability, Observed, PrFacts, PrState, Thread}; +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. +pub const MAX_PAGES: usize = 20; + +/// What the GraphQL read yields. `facts.required_contexts` and +/// `facts.rule_types` are left empty for the caller to fill from REST. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PrRead { + pub base_ref: String, + pub head_oid: String, + pub facts: PrFacts, +} + +fn s<'a>(v: &'a Value, ptr: &str) -> Option<&'a str> { + v.pointer(ptr).and_then(Value::as_str) +} + +fn login(v: &Value, ptr: &str) -> String { + s(v, ptr) + .map(|l| l.strip_suffix("[bot]").unwrap_or(l).to_string()) + .unwrap_or_else(|| "ghost".into()) +} + +fn observed(node: &Value) -> Option { + match s(node, "/__typename")? { + "CheckRun" => Some(Observed { + name: s(node, "/name")?.to_string(), + producer: login(node, "/checkSuite/app/slug"), + run: CheckRun::from_github(s(node, "/status"), s(node, "/conclusion")), + }), + "StatusContext" => Some(Observed { + name: s(node, "/context")?.to_string(), + producer: login(node, "/creator/login"), + run: CheckRun::from_github(None, s(node, "/state")), + }), + _ => None, + } +} + +fn thread(node: &Value) -> Option { + if node.get("isResolved")?.as_bool()? { + return None; + } + let first = node + .pointer("/comments/nodes/0") + .cloned() + .unwrap_or(json!({})); + Some(Thread { + author: login(&first, "/author/login"), + path: s(node, "/path").map(str::to_string), + line: node.get("line").and_then(Value::as_u64), + outdated: node + .get("isOutdated") + .and_then(Value::as_bool) + .unwrap_or(false), + url: s(&first, "/url").unwrap_or_default().to_string(), + }) +} + +/// A connection's page: its nodes and, if more follow, the cursor. +fn page<'a>(conn: &'a Value) -> Result<(&'a [Value], Option), String> { + let nodes = conn + .get("nodes") + .and_then(Value::as_array) + .map(Vec::as_slice) + .ok_or("connection without nodes")?; + let more = conn + .pointer("/pageInfo/hasNextPage") + .and_then(Value::as_bool) + .ok_or("connection without pageInfo")?; + let next = if more { + Some( + s(conn, "/pageInfo/endCursor") + .ok_or("hasNextPage without endCursor")? + .to_string(), + ) + } else { + None + }; + Ok((nodes, next)) +} + +/// Read one PR. Fail-closed: GraphQL `errors`, a missing PR, or a connection +/// that will not finish within [`MAX_PAGES`] is an `Err`, never a partial read. +pub fn fetch_pr_done( + t: &dyn GraphQlTransport, + owner: &str, + name: &str, + number: u64, +) -> Result { + let mut ctx_after: Option = None; + let mut thr_after: Option = None; + let (mut ctx_done, mut thr_done) = (false, false); + let mut observed_all = Vec::new(); + let mut threads = Vec::new(); + let mut head: Option = None; + + for _ in 0..MAX_PAGES { + let body = json!({ + "query": PR_DONE, + "variables": { + "owner": owner, "name": name, "number": number, + "ctxAfter": ctx_after, "thrAfter": thr_after, + } + }); + let resp = t.execute(&body)?; + if let Some(errs) = resp.get("errors").and_then(Value::as_array) { + let msgs: Vec<&str> = errs + .iter() + .filter_map(|e| e.get("message").and_then(Value::as_str)) + .collect(); + return Err(format!("GraphQL errors: {}", msgs.join("; "))); + } + let pr = resp + .pointer("/data/repository/pullRequest") + .filter(|p| p.is_object()) + .ok_or_else(|| format!("{owner}/{name}#{number}: no such pull request"))?; + + if !thr_done { + let (nodes, next) = page(pr.get("reviewThreads").ok_or("no reviewThreads")?)?; + threads.extend(nodes.iter().filter_map(thread)); + thr_done = next.is_none(); + thr_after = next.or(thr_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") { + Some(conn) if conn.is_object() => { + let (nodes, next) = page(conn)?; + observed_all.extend(nodes.iter().filter_map(observed)); + ctx_done = next.is_none(); + ctx_after = next.or(ctx_after); + } + _ => ctx_done = true, + } + } + head.get_or_insert_with(|| pr.clone()); + if ctx_done && thr_done { + let pr = head.expect("set above"); + return Ok(PrRead { + base_ref: s(&pr, "/baseRefName").unwrap_or_default().to_string(), + head_oid: s(&pr, "/headRefOid").unwrap_or_default().to_string(), + facts: PrFacts { + state: match s(&pr, "/state") { + Some("MERGED") => PrState::Merged, + Some("CLOSED") => PrState::Closed, + _ => PrState::Open, + }, + is_draft: pr.get("isDraft").and_then(Value::as_bool).unwrap_or(false), + mergeable: match s(&pr, "/mergeable") { + Some("MERGEABLE") => Mergeability::Mergeable, + Some("CONFLICTING") => Mergeability::Conflicting, + _ => Mergeability::Unknown, + }, + merge_state: s(&pr, "/mergeStateStatus").unwrap_or("UNKNOWN").to_string(), + review_decision: s(&pr, "/reviewDecision").map(str::to_string), + auto_merge: s(&pr, "/autoMergeRequest/mergeMethod").map(str::to_string), + required_contexts: Vec::new(), + observed: observed_all, + unresolved_threads: threads, + changes_requested_by: pr + .pointer("/latestOpinionatedReviews/nodes") + .and_then(Value::as_array) + .map(|a| { + a.iter() + .filter(|r| s(r, "/state") == Some("CHANGES_REQUESTED")) + .map(|r| login(r, "/author/login")) + .collect() + }) + .unwrap_or_default(), + rule_types: Vec::new(), + }, + }); + } + } + Err(format!( + "{owner}/{name}#{number}: more than {MAX_PAGES} pages of contexts or threads — \ + refusing to judge a partial read" + )) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::cell::RefCell; + + /// Replays canned pages and records the cursors each request carried. + struct Pages { + pages: 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(), + )); + let mut p = self.pages.borrow_mut(); + if p.is_empty() { + return Err("no more pages".into()); + } + Ok(p.remove(0)) + } + } + + fn resp(ctx: Value, ctx_next: Option<&str>, thr: Value, thr_next: Option<&str>) -> Value { + json!({ "data": { "repository": { "pullRequest": { + "state": "OPEN", "isDraft": false, "mergeable": "MERGEABLE", + "mergeStateStatus": "BLOCKED", "reviewDecision": null, + "baseRefName": "main", "headRefOid": "abc", + "autoMergeRequest": { "mergeMethod": "SQUASH" }, + "latestOpinionatedReviews": { "nodes": [ + { "state": "CHANGES_REQUESTED", "author": { "login": "coderabbitai" } }, + { "state": "APPROVED", "author": { "login": "owner" } } + ]}, + "reviewThreads": { + "pageInfo": { "hasNextPage": thr_next.is_some(), "endCursor": thr_next }, + "nodes": thr + }, + "commits": { "nodes": [ { "commit": { "statusCheckRollup": { "contexts": { + "pageInfo": { "hasNextPage": ctx_next.is_some(), "endCursor": ctx_next }, + "nodes": ctx + }}}}]} + }}}}) + } + + fn run(name: &str) -> Value { + json!({ "__typename": "CheckRun", "name": name, "status": "COMPLETED", + "conclusion": "SUCCESS", "checkSuite": { "app": { "slug": "github-actions" } } }) + } + + #[test] + fn the_query_validates_against_the_schema_snapshot() { + crate::tests::validate(PR_DONE).unwrap(); + } + + /// The defect this module exists to avoid: a second page of contexts must + /// be read, and a connection that already finished must not be re-counted. + #[test] + fn contexts_past_the_first_page_are_read_and_finished_threads_are_not_repeated() { + let t1 = json!([{ "isResolved": false, "isOutdated": true, "path": "a.rs", "line": 3, + "comments": { "nodes": [ { "author": { "login": "coderabbitai" }, "url": "u1" } ] } }, + { "isResolved": true, "isOutdated": false, "path": "b.rs", "line": 1, + "comments": { "nodes": [] } }]); + let bot = json!({ "__typename": "StatusContext", "context": "CodeRabbit", + "state": "PENDING", "creator": { "login": "coderabbitai[bot]" } }); + let p = Pages { + pages: RefCell::new(vec![ + resp(json!([run("build")]), Some("C1"), t1.clone(), None), + resp(json!([bot]), None, t1, None), + ]), + seen: RefCell::new(vec![]), + }; + let r = fetch_pr_done(&p, "o", "r", 1).unwrap(); + let names: Vec<_> = r.facts.observed.iter().map(|o| o.name.as_str()).collect(); + assert_eq!(names, ["build", "CodeRabbit"]); + assert_eq!(r.facts.observed[1].producer, "coderabbitai"); + assert_eq!(r.facts.observed[1].run, CheckRun::Pending); + assert_eq!( + r.facts.unresolved_threads.len(), + 1, + "resolved skipped, no repeat" + ); + assert!(r.facts.unresolved_threads[0].outdated); + assert_eq!(r.facts.changes_requested_by, ["coderabbitai"]); + assert_eq!(r.facts.auto_merge.as_deref(), Some("SQUASH")); + assert_eq!( + p.seen.borrow()[1].0, + json!("C1"), + "second request carried the cursor" + ); + } + + #[test] + fn graphql_errors_fail_closed() { + let p = Pages { + pages: RefCell::new(vec![json!({ "errors": [ { "message": "rate limited" } ] })]), + seen: RefCell::new(vec![]), + }; + assert!(fetch_pr_done(&p, "o", "r", 1) + .unwrap_err() + .contains("rate limited")); + } + + #[test] + fn a_connection_that_never_ends_is_refused_not_truncated() { + let pages = (0..MAX_PAGES) + .map(|i| { + resp( + json!([run(&format!("c{i}"))]), + Some("more"), + json!([]), + None, + ) + }) + .collect(); + let p = Pages { + pages: RefCell::new(pages), + seen: RefCell::new(vec![]), + }; + assert!(fetch_pr_done(&p, "o", "r", 1) + .unwrap_err() + .contains("partial read")); + } +} From 813cc7b6ef9137ea1a7a44e35ef012f39155dd0a Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:09:03 +0100 Subject: [PATCH 2/6] feat(verify-satisfied): a red non-required check is agent work; worst same-named run decides Control 1 (cicd-squabbler#114) read DONE while `rust-ci / Cargo check + clippy + fmt` was red: the check is not required, so the evaluator never looked at it. "All the checkers have run" means their output was dealt with, not that the ruleset is satisfied. New `Item::CheckFailed`; skipped and neutral are not items. Re-run live: #114 now exits 5 naming the check, #116 (its cure) exits 0. `squabble fetch` and `verify-satisfied` both took the FIRST same-named run, so a passing twin could mask a failing one depending on rollup order. One shared rule, `CheckRun::for_context`: the worst run decides. Mutants killed: first-match ordering (2 tests red), the new loop disabled (1 test red). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0136eszqrQ53Kj7aBH1D4rXK --- crates/squabble-cli/src/fetch.rs | 11 ++--- crates/squabble-core/src/done.rs | 75 +++++++++++++++++++++++++++++--- crates/squabble-core/src/gate.rs | 34 +++++++++++++++ 3 files changed, 108 insertions(+), 12 deletions(-) diff --git a/crates/squabble-cli/src/fetch.rs b/crates/squabble-cli/src/fetch.rs index 59c8b20..6649086 100644 --- a/crates/squabble-cli/src/fetch.rs +++ b/crates/squabble-cli/src/fetch.rs @@ -78,11 +78,12 @@ fn build_gate(required_contexts: &[String], rollup: &[RollupEntry]) -> Gate { let checks = required_contexts .iter() .map(|required| { - let run = rollup - .iter() - .find(|r| &r.name == required) - .map(parse_rollup) - .unwrap_or(CheckRun::Missing); + let run = CheckRun::for_context( + rollup + .iter() + .filter(|r| &r.name == required) + .map(parse_rollup), + ); RequiredCheck::new(required.clone(), run) }) .collect(); diff --git a/crates/squabble-core/src/done.rs b/crates/squabble-core/src/done.rs index 0311f09..231f677 100644 --- a/crates/squabble-core/src/done.rs +++ b/crates/squabble-core/src/done.rs @@ -127,6 +127,7 @@ pub enum Item { MergeabilityUnknown, Draft, RequiredNotSatisfied { context: String, run: CheckRun }, + CheckFailed { context: String, producer: String }, UnresolvedThread(Thread), ChangesRequested { reviewer: String }, ReviewBotPending { producer: String, context: String }, @@ -151,6 +152,11 @@ impl Item { Self::RequiredNotSatisfied { context, run } => { format!("required check `{context}` is {run:?} — fix it or the ruleset") } + Self::CheckFailed { context, producer } => format!( + "check `{context}` ({producer}) failed — not required, but a red check is \ + still an unread finding; fix it here, or land the fix that cures it on the \ + base first (an inherited red is not yet told apart from a new one)" + ), Self::UnresolvedThread(t) => format!( "unresolved review thread by {}{}{} — act on it or reply and resolve: {}", t.author, @@ -215,19 +221,21 @@ fn strip_bot(login: &str) -> &str { } /// The required half as a [`Gate`], matched on context name (a required check -/// names a context, not a producer). First match wins, as in `squabble fetch`. +/// names a context, not a producer). Same-named runs resolve by +/// [`CheckRun::for_context`], the rule `squabble fetch` uses too. pub fn required_gate(facts: &PrFacts) -> Gate { Gate::new( facts .required_contexts .iter() .map(|c| { - let run = facts - .observed - .iter() - .find(|o| &o.name == c) - .map(|o| o.run) - .unwrap_or(CheckRun::Missing); + let run = CheckRun::for_context( + facts + .observed + .iter() + .filter(|o| &o.name == c) + .map(|o| o.run), + ); RequiredCheck::new(c.clone(), run) }) .collect(), @@ -310,6 +318,18 @@ pub fn evaluate(facts: &PrFacts, review_apps: &[&str]) -> Verdict { ); } + // 2b. a red check the ruleset does not require is still a finding: "all the + // checkers have run" means their output was read, not that the ruleset is + // satisfied. Required contexts are already named above. + for o in &facts.observed { + if o.run == CheckRun::Failed && !facts.required_contexts.contains(&o.name) { + agent.push(Item::CheckFailed { + context: o.name.clone(), + producer: strip_bot(&o.producer).to_string(), + }); + } + } + // 3. review output read and acted on agent.extend( facts @@ -520,6 +540,47 @@ mod tests { assert_eq!(v.evidence_free, ["scan"]); } + /// Measured on cicd-squabbler#114: `rust-ci / Cargo check + clippy + fmt` + /// was red and not required, and the verdict read DONE. A red checker's + /// output has not been dealt with just because the ruleset ignores it. + #[test] + fn a_failed_check_that_is_not_required_is_still_agent_work() { + let mut f = done_pr(); + f.observed + .push(obs("rust-ci / clippy", "github-actions", CheckRun::Failed)); + f.observed + .push(obs("optional-scan", "github-actions", CheckRun::Skipped)); + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert_eq!( + v.agent_items, + [Item::CheckFailed { + context: "rust-ci / clippy".into(), + producer: "github-actions".into(), + }] + ); + } + + /// Same-named runs from two workflows: the verdict must not depend on the + /// order the rollup happens to list them in. + #[test] + fn a_passing_twin_cannot_mask_a_failing_required_check() { + for failed_first in [true, false] { + let mut f = done_pr(); + f.observed[0].run = CheckRun::Passed; + let red = obs("build", "github-actions", CheckRun::Failed); + if failed_first { + f.observed.insert(0, red); + } else { + f.observed.push(red); + } + assert_eq!( + kinds(&evaluate(&f, DEFAULT_REVIEW_APPS).agent_items), + ["required_not_satisfied"], + "failed_first={failed_first}" + ); + } + } + #[test] fn blocked_without_automerge_is_agent_work() { let mut f = done_pr(); diff --git a/crates/squabble-core/src/gate.rs b/crates/squabble-core/src/gate.rs index 19c6e85..4eef486 100644 --- a/crates/squabble-core/src/gate.rs +++ b/crates/squabble-core/src/gate.rs @@ -58,6 +58,29 @@ impl CheckRun { }, } } + + /// The run that decides one required context, given every run on the head + /// whose name matches it. One rule, shared by `squabble fetch` and + /// `verify-satisfied`. + /// + /// Two workflows can emit the same job name, and the rollup's order is not a + /// promise. "First match wins" would let a passing twin mask a failing one + /// depending on API order, so the **worst** run decides: + /// `Failed` > `Pending` > `Skipped` > `Passed`. No match is `Missing`. + pub fn for_context(runs: impl IntoIterator) -> Self { + fn rank(r: CheckRun) -> u8 { + match r { + CheckRun::Failed => 4, + CheckRun::Missing => 3, + CheckRun::Pending => 2, + CheckRun::Skipped => 1, + CheckRun::Passed => 0, + } + } + runs.into_iter() + .max_by_key(|r| rank(*r)) + .unwrap_or(Self::Missing) + } } /// Why a required context shows [`CheckRun::Missing`]. @@ -249,6 +272,17 @@ mod tests { RequiredCheck::new(name, run) } + #[test] + fn a_passing_twin_never_masks_a_failing_one_in_either_order() { + use CheckRun::*; + assert_eq!(CheckRun::for_context([Passed, Failed]), Failed); + assert_eq!(CheckRun::for_context([Failed, Passed]), Failed); + assert_eq!(CheckRun::for_context([Passed, Pending]), Pending); + assert_eq!(CheckRun::for_context([Passed, Skipped]), Skipped); + assert_eq!(CheckRun::for_context([Passed]), Passed); + assert_eq!(CheckRun::for_context([]), Missing); + } + #[test] fn all_passed_is_green() { let g = Gate::new(vec![ck("a", CheckRun::Passed), ck("b", CheckRun::Passed)]); From ad6885f70c6af027b1170e1f951e29138b3b3e41 Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:09:27 +0100 Subject: [PATCH 3/6] style(pr_done): elide the lifetime clippy flags Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0136eszqrQ53Kj7aBH1D4rXK --- crates/squabble-forge/src/pr_done.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/squabble-forge/src/pr_done.rs b/crates/squabble-forge/src/pr_done.rs index f4d2948..b238e3b 100644 --- a/crates/squabble-forge/src/pr_done.rs +++ b/crates/squabble-forge/src/pr_done.rs @@ -74,7 +74,7 @@ fn thread(node: &Value) -> Option { } /// A connection's page: its nodes and, if more follow, the cursor. -fn page<'a>(conn: &'a Value) -> Result<(&'a [Value], Option), String> { +fn page(conn: &Value) -> Result<(&[Value], Option), String> { let nodes = conn .get("nodes") .and_then(Value::as_array) From 9faac5cf9b8f0d7c4f25b69b5fed77afdc549aab Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:13:35 +0100 Subject: [PATCH 4/6] feat(verify-satisfied): a red non-required check is cleared by an issue link in the PR body The previous commit made a red non-required check agent work with no way out but green, which inverts the owner's 2026-09-15 ruling: a new scanner finding becomes an issue with acceptance criteria, not a merge blocker. The ask was "checked and dismissed/acted on", not "green". A check run has no dismiss action, so the acknowledgement lives in the PR body: a line naming the context AND carrying an issue/PR reference (#N, /issues/N, /pull/N). It then moves to `notes` (still visible), not dropped. Naming the check without a link does not count. `body` added to pr_done.graphql and PrFacts (String; absent = no acks, fail-closed). Mutants killed: link requirement dropped, context match dropped, digit-after-# check dropped (1 test red each). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0136eszqrQ53Kj7aBH1D4rXK --- crates/squabble-core/src/done.rs | 92 +++++++++++++++++-- crates/squabble-forge/graphql/pr_done.graphql | 1 + crates/squabble-forge/src/pr_done.rs | 4 +- 3 files changed, 90 insertions(+), 7 deletions(-) diff --git a/crates/squabble-core/src/done.rs b/crates/squabble-core/src/done.rs index 231f677..15dee13 100644 --- a/crates/squabble-core/src/done.rs +++ b/crates/squabble-core/src/done.rs @@ -117,6 +117,9 @@ pub struct PrFacts { pub changes_requested_by: Vec, /// `[.[].type]` of the effective rules on the base branch. pub rule_types: Vec, + /// The PR description, verbatim. Where a red non-required check is + /// acknowledged (see [`acknowledged_in`]). Empty when there is none. + pub body: String, } /// One unmet condition. @@ -154,8 +157,8 @@ impl Item { } Self::CheckFailed { context, producer } => format!( "check `{context}` ({producer}) failed — not required, but a red check is \ - still an unread finding; fix it here, or land the fix that cures it on the \ - base first (an inherited red is not yet told apart from a new one)" + still a finding; fix it here, or file an issue and add a PR-body line \ + naming `{context}` with its link (#N)" ), Self::UnresolvedThread(t) => format!( "unresolved review thread by {}{}{} — act on it or reply and resolve: {}", @@ -198,6 +201,31 @@ impl Item { } } +/// The body line acknowledging a red non-required check, if any. +/// +/// A check run has no dismiss action, so the acknowledgement lives in the PR +/// body: a line naming the context **and** an issue or PR reference (`#N`, or an +/// `/issues/N` or `/pull/N` URL). That is the owner's 2026-09-15 ruling in +/// machine-readable form: a new finding becomes an issue, not a blocker, but it +/// must be looked at. Naming the check without a link does not count. +pub fn acknowledged_in<'a>(body: &'a str, context: &str) -> Option<&'a str> { + body.lines() + .map(str::trim) + .find(|l| l.contains(context) && has_issue_ref(l)) +} + +fn has_issue_ref(line: &str) -> bool { + let digit_after = |pat: &str| { + line.match_indices(pat).any(|(i, _)| { + line[i + pat.len()..] + .chars() + .next() + .is_some_and(|c| c.is_ascii_digit()) + }) + }; + digit_after("#") || digit_after("/issues/") || digit_after("/pull/") +} + /// The answer. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct Verdict { @@ -320,13 +348,22 @@ pub fn evaluate(facts: &PrFacts, review_apps: &[&str]) -> Verdict { // 2b. a red check the ruleset does not require is still a finding: "all the // checkers have run" means their output was read, not that the ruleset is - // satisfied. Required contexts are already named above. + // satisfied. Required contexts are already named above. Acknowledged in the + // body with an issue link, it is dealt with (the 09-15 ruling) and named in + // the notes rather than dropped. for o in &facts.observed { - if o.run == CheckRun::Failed && !facts.required_contexts.contains(&o.name) { - agent.push(Item::CheckFailed { + if o.run != CheckRun::Failed || facts.required_contexts.contains(&o.name) { + continue; + } + match acknowledged_in(&facts.body, &o.name) { + Some(line) => notes.push(format!( + "red check `{}` acknowledged in the PR body: {line}", + o.name + )), + None => agent.push(Item::CheckFailed { context: o.name.clone(), producer: strip_bot(&o.producer).to_string(), - }); + }), } } @@ -472,6 +509,7 @@ mod tests { unresolved_threads: vec![], changes_requested_by: vec![], rule_types: vec!["required_status_checks".into(), "deletion".into()], + body: String::new(), } } @@ -560,6 +598,48 @@ mod tests { ); } + /// The 09-15 ruling: a finding becomes an issue, not a blocker — but only + /// once someone has looked. Naming the check without a link is not looking. + #[test] + fn a_red_check_named_with_an_issue_link_in_the_body_is_dealt_with() { + let red = || { + let mut f = done_pr(); + f.observed + .push(obs("rust-ci / clippy", "github-actions", CheckRun::Failed)); + f + }; + for body in [ + "- `rust-ci / clippy` — inherited from main, cured by #116", + "rust-ci / clippy: https://github.com/o/r/issues/7", + "rust-ci / clippy fails upstream, see https://github.com/o/r/pull/116", + ] { + let mut f = red(); + f.body = format!("Summary\n\n{body}\n"); + let v = evaluate(&f, DEFAULT_REVIEW_APPS); + assert!(v.is_done(), "{body:?}: {:?}", v.agent_items); + assert!( + v.notes.iter().any(|n| n.contains("rust-ci / clippy")), + "an acknowledged red must stay visible: {:?}", + v.notes + ); + } + for body in [ + "", + "rust-ci / clippy is inherited from main", + "rust-ci / clippy is # not a link", + "see #116\nrust-ci / clippy is red", + "`rust-ci / fmt` — see #116", + ] { + let mut f = red(); + f.body = body.into(); + assert_eq!( + kinds(&evaluate(&f, DEFAULT_REVIEW_APPS).agent_items), + ["check_failed"], + "{body:?}" + ); + } + } + /// Same-named runs from two workflows: the verdict must not depend on the /// order the rollup happens to list them in. #[test] diff --git a/crates/squabble-forge/graphql/pr_done.graphql b/crates/squabble-forge/graphql/pr_done.graphql index 3a5a378..d0ad5ec 100644 --- a/crates/squabble-forge/graphql/pr_done.graphql +++ b/crates/squabble-forge/graphql/pr_done.graphql @@ -18,6 +18,7 @@ query PrDone($owner: String!, $name: String!, $number: Int!, $ctxAfter: String, reviewDecision baseRefName headRefOid + body autoMergeRequest { mergeMethod } latestOpinionatedReviews(first: 100) { nodes { state author { login } } diff --git a/crates/squabble-forge/src/pr_done.rs b/crates/squabble-forge/src/pr_done.rs index b238e3b..c0ab779 100644 --- a/crates/squabble-forge/src/pr_done.rs +++ b/crates/squabble-forge/src/pr_done.rs @@ -185,6 +185,7 @@ pub fn fetch_pr_done( }) .unwrap_or_default(), rule_types: Vec::new(), + body: s(&pr, "/body").unwrap_or_default().to_string(), }, }); } @@ -223,7 +224,7 @@ mod tests { json!({ "data": { "repository": { "pullRequest": { "state": "OPEN", "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "BLOCKED", "reviewDecision": null, - "baseRefName": "main", "headRefOid": "abc", + "baseRefName": "main", "headRefOid": "abc", "body": "ack `x` #1", "autoMergeRequest": { "mergeMethod": "SQUASH" }, "latestOpinionatedReviews": { "nodes": [ { "state": "CHANGES_REQUESTED", "author": { "login": "coderabbitai" } }, @@ -279,6 +280,7 @@ mod tests { ); assert!(r.facts.unresolved_threads[0].outdated); assert_eq!(r.facts.changes_requested_by, ["coderabbitai"]); + assert_eq!(r.facts.body, "ack `x` #1"); assert_eq!(r.facts.auto_merge.as_deref(), Some("SQUASH")); assert_eq!( p.seen.borrow()[1].0, From 0870d63cf75ec19bf1264dec1a006bfb312d349b Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:26:59 +0100 Subject: [PATCH 5/6] verify-satisfied: skip the base-gate REST read for a merged or closed PR The base gate feeds only the evidence-free listing once a PR is merged or closed, never an agent item; pinned by a new core test (killed by a mutant that routes every state through the open-PR path). Skipping the call keeps the commonest done-claim off the REST secondary limit this PAT shares with every other session -- live, 'landed hyperpolymath/hypatia#880' failed closed on exactly that 403. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0136eszqrQ53Kj7aBH1D4rXK --- crates/squabble-cli/src/verify.rs | 25 ++++++++++++++++--------- crates/squabble-core/src/done.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/crates/squabble-cli/src/verify.rs b/crates/squabble-cli/src/verify.rs index fda8b99..4f4087e 100644 --- a/crates/squabble-cli/src/verify.rs +++ b/crates/squabble-cli/src/verify.rs @@ -10,7 +10,7 @@ use crate::fetch; use serde::Serialize; -use squabble_core::done::{evaluate, Verdict, DEFAULT_REVIEW_APPS}; +use squabble_core::done::{evaluate, PrState, Verdict, DEFAULT_REVIEW_APPS}; use squabble_forge::pr_done::fetch_pr_done; use squabble_forge::GhTransport; use std::process::ExitCode; @@ -55,14 +55,21 @@ pub fn run(args: &[String]) -> ExitCode { } }; let mut facts = read.facts; - match fetch::base_gate(slug, &read.base_ref) { - Ok((contexts, rule_types)) => { - facts.required_contexts = contexts; - facts.rule_types = rule_types; - } - Err(e) => { - eprintln!("squabble verify-satisfied: {e}"); - return ExitCode::from(fetch::FetchError::FAILED_EXIT); + // The base gate only bears on a PR that can still merge: for a merged or + // closed PR it feeds nothing but the evidence-free listing, never an agent + // item (pinned by `the_base_gate_never_decides_a_closed_or_merged_verdict`). + // Skipping the REST call there keeps the commonest done-claim ("landed X") + // off the rate limit this PAT shares with every other session. + if facts.state == PrState::Open { + match fetch::base_gate(slug, &read.base_ref) { + Ok((contexts, rule_types)) => { + facts.required_contexts = contexts; + facts.rule_types = rule_types; + } + Err(e) => { + eprintln!("squabble verify-satisfied: {e}"); + return ExitCode::from(fetch::FetchError::FAILED_EXIT); + } } } diff --git a/crates/squabble-core/src/done.rs b/crates/squabble-core/src/done.rs index 15dee13..a7a3789 100644 --- a/crates/squabble-core/src/done.rs +++ b/crates/squabble-core/src/done.rs @@ -774,4 +774,34 @@ mod tests { ["unresolved_thread"] ); } + + /// `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 + /// open PR three ways over. + #[test] + fn the_base_gate_never_decides_a_closed_or_merged_verdict() { + for state in [PrState::Merged, PrState::Closed] { + let with_gate = { + let mut f = done_pr(); + f.state = state; + f.auto_merge = None; + f.required_contexts = vec!["build".into(), "absent".into()]; + f.observed + .push(obs("build", "github-actions", CheckRun::Failed)); + f.rule_types.push("required_deployments".into()); + f + }; + let without_gate = PrFacts { + required_contexts: vec![], + rule_types: vec![], + ..with_gate.clone() + }; + let a = evaluate(&with_gate, DEFAULT_REVIEW_APPS); + let b = evaluate(&without_gate, DEFAULT_REVIEW_APPS); + assert!(a.is_done(), "{state:?}: {:?}", kinds(&a.agent_items)); + assert_eq!(kinds(&a.agent_items), kinds(&b.agent_items)); + assert_eq!(a.held_by_human.len(), b.held_by_human.len()); + } + } } From 58d1af66c4a0661912e122f4731cdabc8b2ad92c Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Wed, 30 Sep 2026 15:23:15 +0100 Subject: [PATCH 6/6] verify-satisfied: a merge-commit automerge is not agent work, only rebase is Owner ruling 2026-09-30: merge commits stay allowed (proof PRs keep their commit series; GitHub signs the merge commit). Rebase is the form that replays commits unsigned and breaks required_signatures, so REBASE is now the only wrong automerge method. The test pins both halves; a mutant dropping "MERGE" from the accepted set turns it red. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0136eszqrQ53Kj7aBH1D4rXK --- crates/squabble-core/src/done.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/squabble-core/src/done.rs b/crates/squabble-core/src/done.rs index a7a3789..d676c07 100644 --- a/crates/squabble-core/src/done.rs +++ b/crates/squabble-core/src/done.rs @@ -183,7 +183,7 @@ impl Item { (mergeStateStatus={merge_state}) — arm it with squash" ), Self::WrongMergeMethod { method } => { - format!("automerge is armed with {method} — re-arm with SQUASH") + format!("automerge is armed with {method} — re-arm with SQUASH (or MERGE for a proof PR)") } Self::BranchBehind => "branch is behind its base — update it".into(), Self::MergeQueue => "base branch uses a merge queue — verify-satisfied cannot \ @@ -439,10 +439,12 @@ pub fn evaluate(facts: &PrFacts, review_apps: &[&str]) -> Verdict { }); } - // 5 + 6. automerge armed where GitHub allows it, and with squash + // 5 + 6. automerge armed where GitHub allows it, never with rebase: rebase + // replays commits unsigned. A merge commit stays allowed for proof PRs + // (owner ruling 2026-09-30), so only REBASE is the wrong method. if !queue { match facts.auto_merge.as_deref() { - Some("SQUASH") => {} + Some("SQUASH" | "MERGE") => {} Some(m) => agent.push(Item::WrongMergeMethod { method: m.to_string(), }), @@ -685,13 +687,15 @@ mod tests { } #[test] - fn a_non_squash_automerge_is_agent_work() { + fn a_rebase_automerge_is_agent_work_but_a_merge_commit_is_not() { let mut f = done_pr(); f.auto_merge = Some("REBASE".into()); assert_eq!( kinds(&evaluate(&f, DEFAULT_REVIEW_APPS).agent_items), ["wrong_merge_method"] ); + f.auto_merge = Some("MERGE".into()); + assert!(evaluate(&f, DEFAULT_REVIEW_APPS).agent_items.is_empty()); } #[test]