From e16ed0374df3397f9343ca86c69c8a007dfd85ff Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Mon, 21 Sep 2026 19:11:23 +0100 Subject: [PATCH] fix(fetch): give "no gate" its own exit code so callers can tell it apart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `squabble fetch` returned `ExitCode::from(2)` for every failure path: bad usage, a malformed slug, `gh` failing, unparseable JSON, a serialisation error — and "this PR's base branch has no `required_status_checks` rule". That last one is not a malfunction. It is a true, useful answer: there is no gate here, so there is nothing to triage. Collapsing them into one code forced every caller into a false choice. MetaManifold-WebUI's `Gate triage` job hit exactly this: its `Fetch the live gate for this PR` step went red on an unprotected base branch, and the only ways out were to fail on a non-finding or to swallow rc=2 and mute genuine breakage with it. A consumer cannot ask a question the producer never answers, so answer it here. - `FetchError::{NoGate, Failed}` replaces the bare `String` error. `From` keeps `?` working on every helper that still yields one, and always produces `Failed` — a real error can never become a NoGate. - `NoGate` exits 3; everything else keeps exiting 2, so callers that only know about 2 keep failing on precisely what they failed on before. - `fight` propagates it too, via `load_gate`, so both live subcommands agree on what "no gate" means. - The contract is documented in the module docs and printed in `--help`. Also corrects the message itself. It claimed "an unprotected branch has no gate to squabble over", but this code path reads only the rulesets API (`repos/{slug}/rules/branches/{base}`). Measured 2026-09-21: a branch with a live 6-context `required_status_checks` ruleset returns 404 "Branch not protected" from the classic endpoint, so the two surfaces are disjoint and the absence of a ruleset rule is not evidence a branch is unprotected. The message now names the surface it queried and says what it cannot see. Tests: four, each killed by its own mutant — collapsing the two constants to one value (2 fail), making `From` yield `NoGate` (1 fail), and restoring the "unprotected" wording (1 fail). Verified live: no-gate -> 3, malformed slug / missing repo / missing PR -> 2, and two genuinely gated PRs still fetch and diagnose at 0. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm --- crates/squabble-cli/src/fetch.rs | 140 +++++++++++++++++++++++++++++-- crates/squabble-cli/src/fight.rs | 10 +-- crates/squabble-cli/src/main.rs | 21 ++++- 3 files changed, 157 insertions(+), 14 deletions(-) diff --git a/crates/squabble-cli/src/fetch.rs b/crates/squabble-cli/src/fetch.rs index 6545421..9ff0b57 100644 --- a/crates/squabble-cli/src/fetch.rs +++ b/crates/squabble-cli/src/fetch.rs @@ -207,11 +207,76 @@ fn run_gh(args: &[&str]) -> Result { Ok(String::from_utf8_lossy(&out.stdout).into_owned()) } +/// Why a fetch produced no gate. +/// +/// `NoGate` is a **finding**, not a malfunction: the base branch carries no +/// `required_status_checks` ruleset rule, so there is genuinely nothing to +/// triage. Everything else — `gh` failing, a malformed slug, JSON that will +/// not parse — is `Failed`. +/// +/// They are separate variants because they were previously the same one. +/// `fetch` returned a bare `String` for both, the CLI mapped every error to +/// exit 2, and so a caller had to choose between treating a real breakage as +/// a clean skip or treating a true non-finding as a broken build. Both are +/// wrong. A consumer cannot ask a question the producer never answers, so the +/// answer is given here rather than guessed downstream. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum FetchError { + /// No `required_status_checks` rule applies to the PR's base branch. + NoGate { slug: String, branch: String }, + /// Any other failure. Carries the message it always carried. + Failed(String), +} + +impl FetchError { + /// Exit code for "there is no gate here" — a reportable non-finding. + pub const NO_GATE_EXIT: u8 = 3; + /// Exit code for a genuine malfunction. Unchanged, so existing callers + /// that only know about 2 keep failing on exactly what they failed on. + pub const FAILED_EXIT: u8 = 2; + + /// The process exit code this error should produce. + pub fn exit_code(&self) -> u8 { + match self { + Self::NoGate { .. } => Self::NO_GATE_EXIT, + Self::Failed(_) => Self::FAILED_EXIT, + } + } +} + +impl std::fmt::Display for FetchError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + // Deliberately says "ruleset rule", not "unprotected branch". + // Classic branch protection lives behind a different endpoint + // (`repos/{slug}/branches/{b}/protection`) and is invisible to the + // rules API this function queries — measured 2026-09-21 on a repo + // whose live 6-context ruleset gate reads as 404 "Branch not + // protected" there. Calling the branch unprotected on this + // evidence would be a false statement about a gated branch. + Self::NoGate { slug, branch } => write!( + f, + "no `required_status_checks` ruleset rule applies to `{slug}` branch \ + `{branch}` — nothing to squabble over. (Classic branch protection is a \ + separate API and is not visible to this query.)" + ), + Self::Failed(msg) => f.write_str(msg), + } + } +} + +/// Lets `?` keep working on the many helpers that still yield `String`. +impl From for FetchError { + fn from(msg: String) -> Self { + Self::Failed(msg) + } +} + /// Fetch a live PR's gate from GitHub via `gh` and return it as a [`Gate`]. /// /// `slug` is `owner/repo`. Requires `gh` to be authenticated for that repo — /// the same precondition every other `gh`-based estate tool already has. -pub fn run(slug: &str, pr: &str) -> Result { +pub fn run(slug: &str, pr: &str) -> Result { run_with_greens(slug, pr).map(|(gate, _greens)| gate) } @@ -221,7 +286,7 @@ pub fn run(slug: &str, pr: &str) -> Result { /// The green set is what [`squabble_core::polarity`] classifies. `fight` only /// ever looks at reds, so a gate that could not run reports green and is never /// inspected — that is the whole fake-green class. -pub fn run_with_greens(slug: &str, pr: &str) -> Result<(Gate, Vec), String> { +pub fn run_with_greens(slug: &str, pr: &str) -> Result<(Gate, Vec), FetchError> { let (owner, repo) = slug .split_once('/') .ok_or_else(|| format!("expected `owner/repo`, got `{slug}`"))?; @@ -257,11 +322,10 @@ pub fn run_with_greens(slug: &str, pr: &str) -> Result<(Gate, Vec), .collect(); if required_contexts.is_empty() { - return Err(format!( - "no `required_status_checks` rule found on `{owner}/{repo}` branch `{}` — \ - an unprotected branch has no gate to squabble over", - pr_view.base_ref_name - )); + return Err(FetchError::NoGate { + slug: format!("{owner}/{repo}"), + branch: pr_view.base_ref_name.clone(), + }); } Ok(( @@ -274,6 +338,68 @@ pub fn run_with_greens(slug: &str, pr: &str) -> Result<(Gate, Vec), mod tests { use super::*; + // --- FetchError: the whole point is that these two are distinguishable --- + + /// The mutant this guards against: someone "simplifying" the codes back to + /// a single value. If both constants become 2, every caller silently + /// returns to being unable to tell a non-finding from a breakage — the + /// exact defect this type exists to remove — and nothing else in the suite + /// would notice. + #[test] + fn no_gate_and_failure_do_not_share_an_exit_code() { + assert_ne!( + FetchError::NO_GATE_EXIT, + FetchError::FAILED_EXIT, + "a shared exit code makes the two outcomes indistinguishable to any caller" + ); + } + + #[test] + fn each_variant_maps_to_its_own_exit_code() { + let no_gate = FetchError::NoGate { + slug: "o/r".into(), + branch: "main".into(), + }; + assert_eq!(no_gate.exit_code(), 3); + assert_eq!(FetchError::Failed("gh exploded".into()).exit_code(), 2); + } + + /// `?` converts every `String` error in this module through `From`. If that + /// conversion ever produced `NoGate`, a genuine breakage would be reported + /// as a clean non-finding and the build would go green on a broken tool. + #[test] + fn an_arbitrary_error_string_becomes_failed_never_no_gate() { + let e: FetchError = String::from("could not parse ruleset response").into(); + assert_eq!( + e, + FetchError::Failed("could not parse ruleset response".into()) + ); + assert_eq!(e.exit_code(), FetchError::FAILED_EXIT); + } + + /// Measured 2026-09-21: a branch with a live 6-context `required_status_checks` + /// ruleset reads as 404 "Branch not protected" on the classic protection + /// endpoint. The two APIs are disjoint, so absence of a *ruleset* rule is not + /// evidence the branch is unprotected, and this message must not say it is. + #[test] + fn the_no_gate_message_does_not_claim_the_branch_is_unprotected() { + let msg = FetchError::NoGate { + slug: "hyperpolymath/MetaManifold-WebUI".into(), + branch: "main".into(), + } + .to_string(); + assert!(msg.contains("hyperpolymath/MetaManifold-WebUI"), "{msg}"); + assert!(msg.contains("main"), "{msg}"); + assert!( + msg.contains("ruleset"), + "must say which surface it queried: {msg}" + ); + assert!( + !msg.contains("unprotected"), + "claims the branch is unprotected on evidence that cannot show it: {msg}" + ); + } + #[test] fn mixed_check_runs_and_commit_statuses_parse_without_losing_failures() { let json = r#"{"baseRefName":"main","statusCheckRollup":[ diff --git a/crates/squabble-cli/src/fight.rs b/crates/squabble-cli/src/fight.rs index 96b818f..7a90089 100644 --- a/crates/squabble-cli/src/fight.rs +++ b/crates/squabble-cli/src/fight.rs @@ -46,7 +46,7 @@ pub fn run(rest: &[String]) -> ExitCode { Ok(g) => g, Err(e) => { eprintln!("squabble fight: {e}"); - return ExitCode::from(2); + return ExitCode::from(e.exit_code()); } }; @@ -221,7 +221,7 @@ fn attach_vacuity(outcome: &mut Outcome, moves: &[Move]) { } } -fn load_gate(args: &FightArgs) -> Result<(Gate, Vec), String> { +fn load_gate(args: &FightArgs) -> Result<(Gate, Vec), fetch::FetchError> { if let Some(path) = &args.gate_file { let text = std::fs::read_to_string(path).map_err(|e| format!("cannot read `{path}`: {e}"))?; @@ -229,13 +229,13 @@ fn load_gate(args: &FightArgs) -> Result<(Gate, Vec), String> // classify — an honest empty set, not a silent skip. return serde_json::from_str(&text) .map(|g| (g, Vec::new())) - .map_err(|e| format!("`{path}` is not a valid gate: {e}")); + .map_err(|e| fetch::FetchError::Failed(format!("`{path}` is not a valid gate: {e}"))); } match &args.pr { Some(pr) => fetch::run_with_greens(&args.slug, pr), - None => Err(format!( + None => Err(fetch::FetchError::Failed(format!( "need a PR number (live) or `--gate ` (offline).\n{USAGE}" - )), + ))), } } diff --git a/crates/squabble-cli/src/main.rs b/crates/squabble-cli/src/main.rs index 560aef2..b6f737d 100644 --- a/crates/squabble-cli/src/main.rs +++ b/crates/squabble-cli/src/main.rs @@ -11,6 +11,21 @@ //! the report — the CLI's only network I/O beyond `gh`. Applying a move is //! still the next implementation step (see docs/CHARTER.adoc) — this binary //! fails loudly rather than pretending to land anything. +//! +//! # Exit codes +//! +//! - `0` — success. +//! - `2` — a genuine failure: bad usage, `gh` failed, unparseable input. +//! - `3` — **no gate**: the PR's base branch carries no `required_status_checks` +//! ruleset rule, so there is nothing to triage. +//! +//! `3` exists because `2` used to cover both. A caller that cannot separate +//! "there is nothing here to triage" from "the tool is broken" must either mute +//! real breakage or fail the build on a true non-finding. CI callers should +//! treat `3` as a reportable finding and any other non-zero as a failure. +//! +//! Note `3` says nothing about *classic* branch protection, which lives behind +//! a different endpoint and is invisible to the rules API this binary queries. #[cfg(feature = "boj")] mod boj; @@ -51,7 +66,9 @@ fn main() -> ExitCode { squabble fetch / \n \ squabble diagnose \n \ squabble {}\n \ - squabble --version\n", + squabble --version\n\n\ + EXIT CODES:\n \ + 0 ok · 2 failure · 3 no `required_status_checks` rule on the base branch\n", env!("CARGO_PKG_VERSION"), fight::USAGE.trim_start_matches("usage: squabble ") ); @@ -74,7 +91,7 @@ fn run_fetch(slug: &str, pr: &str) -> ExitCode { }, Err(e) => { eprintln!("squabble fetch: {e}"); - ExitCode::from(2) + ExitCode::from(e.exit_code()) } } }