diff --git a/crates/squabble-cli/src/fetch.rs b/crates/squabble-cli/src/fetch.rs index 2cb4d14..d24bdec 100644 --- a/crates/squabble-cli/src/fetch.rs +++ b/crates/squabble-cli/src/fetch.rs @@ -15,7 +15,9 @@ //! //! A required context with no matching rollup entry is [`CheckRun::Missing`]; //! matching-but-incomplete is [`CheckRun::Pending`]; a `SUCCESS` conclusion is -//! [`CheckRun::Passed`]; anything else that completed is [`CheckRun::Failed`]. +//! [`CheckRun::Passed`]; `SKIPPED`/`NEUTRAL` is [`CheckRun::Skipped`] (satisfies +//! the ruleset, carries no evidence); anything else that completed is +//! [`CheckRun::Failed`]. use serde::Deserialize; use squabble_core::gate::{CheckRun, Gate, RequiredCheck}; @@ -63,21 +65,15 @@ struct RulesetContext { context: String, } -/// Parse a `gh pr view --json baseRefName,statusCheckRollup` payload into the -/// realised-run half of a [`Gate`]. Pure — no IO, fully testable on fixtures. +/// Classify one entry from `gh pr view`'s `statusCheckRollup`. +/// +/// Recognised conclusions take precedence over status: `SUCCESS` yields +/// [`CheckRun::Passed`], `SKIPPED` or `NEUTRAL` yields [`CheckRun::Skipped`], +/// and `FAILURE`, `ERROR`, `TIMED_OUT`, `CANCELLED` or `STARTUP_FAILURE` yields +/// [`CheckRun::Failed`]. With an absent or unrecognised conclusion, `COMPLETED` +/// status yields [`CheckRun::Failed`]; any other status yields [`CheckRun::Pending`]. fn parse_rollup(entry: &RollupEntry) -> CheckRun { - match entry.conclusion.as_deref() { - Some("SUCCESS") => CheckRun::Passed, - 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. @@ -87,11 +83,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(); @@ -645,6 +642,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::*; @@ -947,6 +970,20 @@ mod tests { ); } + #[test] + fn skipped_and_neutral_map_to_skipped_not_failed() { + // Both COMPLETED with a conclusion GitHub accepts for a required check; + // before the Skipped variant they fell through to Failed and the gate + // read Red on PRs GitHub would merge. + for c in ["SKIPPED", "NEUTRAL"] { + assert_eq!( + parse_rollup(&entry("x", Some("COMPLETED"), Some(c))), + CheckRun::Skipped, + "{c}" + ); + } + } + #[test] fn in_progress_maps_to_pending() { assert_eq!( diff --git a/crates/squabble-cli/src/main.rs b/crates/squabble-cli/src/main.rs index 0f4cc06..e9c78f1 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…). //! - `6` — **incomplete board** (`squabble board`): rendered and published, but //! a repo was unreadable or had more open PRs than one page; the gap is //! stated at the top of the board. @@ -39,6 +41,7 @@ mod chains; mod fetch; mod fight; mod inbox; +mod verify; use squabble_core::{diagnose, gate::Gate}; use std::process::ExitCode; @@ -62,6 +65,7 @@ fn main() -> ExitCode { }, Some("fight") => fight::run(&args[2..]), Some("chains") => chains::run(&args[2..]), + Some("verify-satisfied") => verify::run(&args[2..]), Some("board") => board::run(&args[2..]), Some("inbox-sweep") => inbox::run(&args[2..]), Some("--version") | Some("-V") => { @@ -80,14 +84,16 @@ fn main() -> ExitCode { squabble {}\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 · 6 board or inbox-sweep: incomplete\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 · 6 board or inbox-sweep: incomplete\n", env!("CARGO_PKG_VERSION"), fight::USAGE.trim_start_matches("usage: squabble "), chains::USAGE.trim_start_matches("usage: squabble "), board::USAGE.trim_start_matches("usage: squabble "), - inbox::USAGE.trim_start_matches("usage: squabble ") + inbox::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..4f4087e --- /dev/null +++ b/crates/squabble-cli/src/verify.rs @@ -0,0 +1,157 @@ +// 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, PrState, 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; + // 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); + } + } + } + + 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..d676c07 --- /dev/null +++ b/crates/squabble-core/src/done.rs @@ -0,0 +1,811 @@ +// 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, + /// 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. +#[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 }, + CheckFailed { context: String, producer: String }, + 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::CheckFailed { context, producer } => format!( + "check `{context}` ({producer}) failed — not required, but a red check is \ + 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: {}", + 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 (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 \ + 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 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 { + 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). 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 = CheckRun::for_context( + facts + .observed + .iter() + .filter(|o| &o.name == c) + .map(|o| o.run), + ); + 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(), + ); + } + + // 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. 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) { + 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(), + }), + } + } + + // 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, 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" | "MERGE") => {} + 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()], + body: String::new(), + } + } + + 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"]); + } + + /// 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(), + }] + ); + } + + /// 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] + 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(); + 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_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] + 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"] + ); + } + + /// `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()); + } + } +} diff --git a/crates/squabble-core/src/gate.rs b/crates/squabble-core/src/gate.rs index 1e424da..4eef486 100644 --- a/crates/squabble-core/src/gate.rs +++ b/crates/squabble-core/src/gate.rs @@ -3,7 +3,9 @@ //! The gate model — the formal heart of `squabble ≠ bypass`, expressed in the //! Rust type system. The SPARK sibling (`spark/`) proves the same invariant //! mechanically: **the only transition into `Green` is a required check that -//! actually ran and passed.** +//! actually ran and was satisfied** — passed, or concluded skipped/neutral, +//! which GitHub's ruleset itself accepts. The latter is modelled as +//! [`CheckRun::Skipped`], never folded into `Passed`, so it stays visible. //! //! There is deliberately no constructor, method, or transition on [`Gate`] that //! reaches [`GateState::Green`] by removing a required context, renaming a check @@ -22,10 +24,65 @@ pub enum CheckRun { Pending, /// The check ran to completion and failed. Failed, - /// The check ran to completion and passed. The *only* green-bearing state. + /// The run concluded `SKIPPED` or `NEUTRAL`. GitHub counts this as meeting the + /// requirement, so the gate does too — but it carries **no evidence**. It is + /// deliberately not `Passed`: a skipped required scan must never read as a + /// real green. [`Gate::evidence_free`] lists them. + Skipped, + /// The check ran to completion and passed. The only *evidence-bearing* green. 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, + }, + } + } + + /// 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`]. /// /// `Missing` is the gate's most common stuck state and its least actionable one: @@ -93,7 +150,7 @@ impl MissingCause { /// A context the branch ruleset requires before a PR may land, paired with the /// realised run that is meant to satisfy it. A requirement is satisfied **iff** /// a run is bound to it (correct name, on the head commit) and that run -/// [`CheckRun::Passed`]. +/// [`CheckRun::Passed`] or was [`CheckRun::Skipped`]. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct RequiredCheck { /// The exact context name the ruleset requires (e.g. `scan / gitleaks`). @@ -115,9 +172,6 @@ impl RequiredCheck { } } - /// A requirement is satisfied only by a bound run that passed. This is the - /// single predicate the whole engine trusts; everything else is plumbing. - #[inline] /// Attach a diagnosis for a missing context. pub fn with_cause(mut self, cause: MissingCause) -> Self { self.missing_cause = Some(cause); @@ -132,8 +186,12 @@ impl RequiredCheck { } } + /// A requirement is satisfied only by a bound run that passed or was skipped + /// (GitHub's own rule). This is the single predicate the whole engine + /// trusts; everything else is plumbing. + #[inline] pub fn is_satisfied(&self) -> bool { - matches!(self.run, CheckRun::Passed) + matches!(self.run, CheckRun::Passed | CheckRun::Skipped) } } @@ -145,7 +203,8 @@ pub enum GateState { Blocked, /// At least one required check ran and failed. Red, - /// Every required check ran and passed. Landing is legitimate. + /// Every required check is satisfied (passed, or skipped/neutral — see + /// [`Gate::evidence_free`]). Landing is legitimate by the ruleset. Green, } @@ -163,7 +222,7 @@ impl Gate { /// Compute the gate state from the realised runs. This function is the Rust /// mirror of the SPARK `Evaluate` and carries the load-bearing invariant: /// - /// * `Green` ⇔ every required check `Passed`. + /// * `Green` ⇔ every required check `Passed` or `Skipped` (non-empty set). /// * `Red` ⇔ some required check `Failed` (and none missing/pending). /// * `Blocked` otherwise (something missing or pending). /// @@ -189,6 +248,15 @@ impl Gate { GateState::Blocked } + /// Satisfied requirements that carry no evidence ([`CheckRun::Skipped`]). + /// A `Green` gate whose checks all appear here is green by GitHub's rule + /// but proved nothing — reports must surface this list, not hide it. + pub fn evidence_free(&self) -> impl Iterator { + self.checks + .iter() + .filter(|c| matches!(c.run, CheckRun::Skipped)) + } + /// The named requirements that are not yet satisfied — the squabbler's work /// list. Ordering is stable (declaration order) for reproducible reports. pub fn unsatisfied(&self) -> impl Iterator { @@ -204,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)]); @@ -216,6 +295,26 @@ mod tests { assert_eq!(g.evaluate(), GateState::Red); } + #[test] + fn a_skipped_check_satisfies_but_is_listed_as_evidence_free() { + // GitHub merges over a SKIPPED/NEUTRAL required check, so the gate must + // not call it Red — but it proved nothing, so it must stay nameable. + let g = Gate::new(vec![ck("a", CheckRun::Passed), ck("b", CheckRun::Skipped)]); + assert_eq!(g.evaluate(), GateState::Green); + let free: Vec<_> = g + .evidence_free() + .map(|c| c.required_context.as_str()) + .collect(); + assert_eq!(free, ["b"]); + assert_eq!(g.unsatisfied().count(), 0); + } + + #[test] + fn a_skip_does_not_mask_a_failure() { + let g = Gate::new(vec![ck("a", CheckRun::Skipped), ck("b", CheckRun::Failed)]); + assert_eq!(g.evaluate(), GateState::Red); + } + #[test] fn a_missing_check_is_blocked_not_green() { // The deadlock class v0.1 targets: a required context with no run bound. diff --git a/crates/squabble-core/src/lib.rs b/crates/squabble-core/src/lib.rs index 5508d56..c1ab68b 100644 --- a/crates/squabble-core/src/lib.rs +++ b/crates/squabble-core/src/lib.rs @@ -17,6 +17,7 @@ pub mod admission; pub mod board; pub mod chains; +pub mod done; pub mod gate; pub mod inbox; pub mod moves; diff --git a/crates/squabble-core/src/moves.rs b/crates/squabble-core/src/moves.rs index 74c9056..ec5b67b 100644 --- a/crates/squabble-core/src/moves.rs +++ b/crates/squabble-core/src/moves.rs @@ -92,7 +92,8 @@ pub enum Move { /// [`EscalationKind`] and evidence. /// /// This is a **hand-off, never a win**: applying it constructs no - /// [`crate::gate::CheckRun::Passed`] and drops no required context, so it + /// [`crate::gate::CheckRun::Passed`] or [`crate::gate::CheckRun::Skipped`] + /// and drops no required context, so it /// cannot move the gate to [`crate::gate::GateState::Green`] by itself. It /// exists so the squabbler can assemble the case for its "big guns" instead /// of either faking a green or silently giving up (`fail-closed`, diff --git a/crates/squabble-forge/graphql/pr_done.graphql b/crates/squabble-forge/graphql/pr_done.graphql new file mode 100644 index 0000000..d0ad5ec --- /dev/null +++ b/crates/squabble-forge/graphql/pr_done.graphql @@ -0,0 +1,54 @@ +# 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 + body + 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 06255d6..daf4e1b 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; @@ -357,7 +359,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..c0ab779 --- /dev/null +++ b/crates/squabble-forge/src/pr_done.rs @@ -0,0 +1,323 @@ +// 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(conn: &Value) -> Result<(&[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(), + body: s(&pr, "/body").unwrap_or_default().to_string(), + }, + }); + } + } + 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", "body": "ack `x` #1", + "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.body, "ack `x` #1"); + 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")); + } +} diff --git a/spark/src/gate_machine.adb b/spark/src/gate_machine.adb index a26a72c..6e6d380 100644 --- a/spark/src/gate_machine.adb +++ b/spark/src/gate_machine.adb @@ -15,7 +15,7 @@ is end if; for I in C'Range loop - if C (I) /= Passed then + if not Satisfies (C (I)) then All_Pass := False; end if; if C (I) = Failed then @@ -23,7 +23,7 @@ is end if; pragma Loop_Invariant - (All_Pass = (for all J in C'First .. I => C (J) = Passed)); + (All_Pass = (for all J in C'First .. I => Satisfies (C (J)))); pragma Loop_Invariant (Saw_Failure = (for some J in C'First .. I => C (J) = Failed)); end loop; diff --git a/spark/src/gate_machine.ads b/spark/src/gate_machine.ads index 021dc41..73aab88 100644 --- a/spark/src/gate_machine.ads +++ b/spark/src/gate_machine.ads @@ -14,10 +14,17 @@ package Gate_Machine with SPARK_Mode => On is - -- The realised result of one required check on the head commit. `Passed` - -- is the only green-bearing value; `Missing` models a required context with - -- no run bound to it (the v0.1 deadlock class). - type Check_Run is (Missing, Pending, Failed, Passed); + -- The realised result of one required check on the head commit. `Missing` + -- models a required context with no run bound to it (the v0.1 deadlock + -- class). `Skipped` is a run that concluded SKIPPED or NEUTRAL: GitHub + -- counts it as satisfying the requirement, so the gate must too, but it + -- carries NO evidence — it is kept distinct from `Passed` so that a + -- skipped scan can never be reported as a real green. + type Check_Run is (Missing, Pending, Failed, Skipped, Passed); + + -- The ruleset's own notion of a met requirement. + function Satisfies (R : Check_Run) return Boolean is + (R = Passed or else R = Skipped); -- Where the gate sits. Green is COMPUTED from the runs, never asserted. type Gate_State is (Blocked, Red, Green); @@ -28,13 +35,16 @@ is -- Evaluate the gate. The postcondition is the load-bearing theorem: -- the result is Green IFF the required set is non-empty AND every required - -- check passed. Proving this body against this contract is the machine - -- check that a squabble can only reach green by satisfying the gate. + -- check is satisfied (Passed or Skipped). Proving this body against this + -- contract is the machine check that a squabble can only reach green by + -- satisfying the gate. + -- Any Failed check yields Red, even if others are Missing or Pending. + -- Otherwise, an empty set or any Missing or Pending check yields Blocked. function Evaluate (C : Check_Array) return Gate_State with Post => (Evaluate'Result = Green) = (C'Length > 0 - and then (for all I in C'Range => C (I) = Passed)); + and then (for all I in C'Range => Satisfies (C (I)))); end Gate_Machine;