diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index 038fabff..ac9d97f4 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -18,7 +18,7 @@ that reviewer. ## Repo configuration -A repo opts in with a rules file at its root, in YAML as +A repo defines its rules in a file at its root, in YAML as `.hyperhive/review-classifier.yml` or in JSON as `.hyperhive/review-classifier.json`. It holds one of the two; with both, the rules don't load. `swarm-controller` always reads the file from the repo's @@ -37,8 +37,14 @@ with exactly these four keys: Any other key, at the top or in an entry, is an error: a misspelt key fails the load instead of vanishing. An empty list, more than ten entries, an empty -name or description, and two entries with one name are errors too. Nothing -falls back to a pass: a repo whose file doesn't load has no rules. +name or description, and two entries with one name are errors too. A file +that exists but fails these checks never falls back to a pass: the run posts +no verdict. +A repo whose default branch holds neither file has no rules. When someone +requests the classifier's review on its PR, the account approves at once. The +body reads `no review rules are configured for owner/name; approved without +checks`. The controller makes no model call. The instance has no per-repo +filter yet, so every repo without a rules file gets this approval. An example: @@ -421,7 +427,8 @@ Repos that rely on it typically raise their required approval count to account for it. A run with no clean verdict never approves. When the PR or the rules don't -load, the run fails, the classifier answers a different number of +load (a repo with no rules file gets an approval instead), the run fails, the classifier +answers a different number of classifications than the controller sent it, or **any** classification has no answer, the account posts a **comment** instead, starting with `**No verdict:**` and the reason, followed by the table when there is one. A comment neither approves nor diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index b2a59299..d091d005 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -468,6 +468,34 @@ async fn rules( owner: &str, name: &str, ) -> Result, ProblemDetails> { + match load_rules(files, owner, name).await? { + Loaded::Rules(rules) => Ok(Json(rules)), + Loaded::Absent { branch } => { + let [yml, json] = RULES_PATHS; + Err(error_problem( + StatusCode::NOT_FOUND, + &format!("{owner}/{name}'s default branch {branch} holds neither {yml} nor {json}"), + )) + } + } +} + +/// What the default branch holds of a repo's rules. +enum Loaded { + Rules(Rules), + /// Neither rules file exists on `branch`. + Absent { + branch: String, + }, +} + +/// Read the rules file of `owner/name` and parse it; a repo whose default +/// branch holds no rules file is [`Loaded::Absent`], not an error. +async fn load_rules( + files: &impl FileSource, + owner: &str, + name: &str, +) -> Result { let repo = format!("{owner}/{name}"); let read = forge::within_forge_deadline( "review rules", @@ -490,13 +518,9 @@ async fn rules( let [yml, json] = RULES_PATHS; let (path, bytes) = match read.files.as_slice() { [] => { - return Err(error_problem( - StatusCode::NOT_FOUND, - &format!( - "{repo}'s default branch {} holds neither {yml} nor {json}", - read.branch - ), - )); + return Ok(Loaded::Absent { + branch: read.branch, + }); } [file] => (&file.path, &file.bytes), _ => { @@ -512,7 +536,7 @@ async fn rules( let classifications = parse_rules(path, bytes) .map_err(|d| error_problem(StatusCode::UNPROCESSABLE_ENTITY, &d))?; tracing::info!(%repo, branch = %read.branch, commit = %read.commit, %path, "review rules read"); - Ok(Json(Rules { + Ok(Loaded::Rules(Rules { path: path.clone(), branch: read.branch, commit: read.commit, diff --git a/swarm-controller/src/review_classifier/review.rs b/swarm-controller/src/review_classifier/review.rs index 84f0480b..343e6ead 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/swarm-controller/src/review_classifier/review.rs @@ -825,6 +825,8 @@ impl SettingsSource for Queue { /// How far a run got. #[derive(Debug, Clone, PartialEq, Eq)] enum Outcome { + /// The repo has no rules file: nothing was asked of the classifier. + NoRules { repo: String }, /// Nothing reached the classifier, for this reason. NotRun(String), /// The classifier answered a run of `sent` classifications. @@ -859,7 +861,7 @@ async fn run( let sent = post(poster, req, head.as_deref(), ruling.review()).await; let answer = match &outcome { Outcome::Ran { reply, .. } => runs::answer(reply), - Outcome::NotRun(_) => (None, None), + Outcome::NotRun(_) | Outcome::NoRules { .. } => (None, None), }; record( req, @@ -1033,8 +1035,9 @@ async fn classify( }; let head = Some(pull.head_sha.clone()); let narrowed = pull.narrowed; - let rules = match super::rules(files, &req.owner, &req.name).await { - Ok(axum::Json(rules)) => rules, + let rules = match super::load_rules(files, &req.owner, &req.name).await { + Ok(super::Loaded::Rules(rules)) => rules, + Ok(super::Loaded::Absent { .. }) => return (head, narrowed, Outcome::NoRules { repo }), Err(problem) => { let detail = problem.detail.unwrap_or_default(); return ( @@ -1115,6 +1118,11 @@ impl Ruling { /// The ruling on a run's outcome. fn review_for(outcome: &Outcome) -> Ruling { match outcome { + Outcome::NoRules { repo } => Ruling { + event: Event::Approve, + body: format!("no review rules are configured for {repo}; approved without checks\n"), + reason: None, + }, Outcome::NotRun(reason) => Ruling::no_verdict(reason.clone(), &[]), Outcome::Ran { reply: Reply::Failed { error }, @@ -2310,7 +2318,8 @@ mod tests { } } - let fake = Fake::new(failed, Vec::new()); + let [yml, _] = crate::review_classifier::RULES_PATHS; + let fake = Fake::new(failed, vec![rules_json(), (yml, String::new())]); let run = run_read(&fake, &fake, &Posted::default(), &request("r")).await; assert_eq!(run.outcome, RunOutcome::NoVerdict); assert!( @@ -2731,13 +2740,65 @@ mod tests { assert_eq!(posted_commit.as_deref(), commit, "{repo}"); } - let fake = Fake::new(approve(), Vec::new()); + let [yml, _] = crate::review_classifier::RULES_PATHS; + let fake = Fake::new(approve(), vec![rules_json(), (yml, String::new())]); let poster = Posted::default(); run_read(&fake, &fake, &poster, &request("r")).await; let (_, _, review) = poster.0.into_inner().expect("unpoisoned").remove(0); assert_eq!(review.event, "COMMENT"); assert!( - review.body.contains("the rules did not load") && review.body.contains("neither"), + review.body.contains("the rules did not load") && review.body.contains("holds both"), + "{}", + review.body + ); + } + + #[tokio::test] + async fn a_repo_without_rules_is_approved_without_a_model_call() { + let fake = Fake::new( + Reply::Failed { + error: "must not run".to_owned(), + }, + Vec::new(), + ); + let poster = Posted::default(); + let run = run_read(&fake, &fake, &poster, &request("r")).await; + assert_eq!(run.outcome, RunOutcome::Approved); + assert_eq!(run.detail, None); + assert_eq!(run.head_sha.as_deref(), Some("abc123")); + assert!(fake.sent.lock().expect("unpoisoned").is_empty()); + let reviews = poster.0.into_inner().expect("unpoisoned"); + assert_eq!(reviews.len(), 1); + let (pr, commit, review) = &reviews[0]; + assert_eq!(pr, "o/r#7"); + assert_eq!(commit.as_deref(), Some("abc123")); + assert_eq!(review.event, "APPROVED"); + assert_eq!( + review.body, + "no review rules are configured for o/r; approved without checks\n" + ); + + // A request for the same head approves again, still without a run. + let poster = Posted::default(); + let again = run_read(&fake, &fake, &poster, &request("r")).await; + assert_eq!(again.outcome, RunOutcome::Approved); + assert!(fake.sent.lock().expect("unpoisoned").is_empty()); + assert_eq!(poster.0.into_inner().expect("unpoisoned").len(), 1); + + // Control: a repo with rules still runs the classifier and posts its verdict. + let approve = Reply::Done { + rows: vec![row(None, true, true)], + overall: Overall::Approve, + }; + let with = Fake::new(approve, vec![rules_json()]); + let poster = Posted::default(); + let run = run_read(&with, &with, &poster, &request("r")).await; + assert_eq!(run.outcome, RunOutcome::Approved); + assert_eq!(with.sent.lock().expect("unpoisoned").len(), 1); + let (_, _, review) = poster.0.into_inner().expect("unpoisoned").remove(0); + assert_eq!(review.event, "APPROVED"); + assert!( + review.body.contains("Overall: ✅ approve"), "{}", review.body ); @@ -2893,7 +2954,7 @@ mod tests { let body = narrowed(None, true).await; assert!(!body.contains("forge's diff"), "{body}"); let body = narrowed(reason(), false).await; - assert!(body.starts_with("**No verdict:**"), "{body}"); + assert!(body.starts_with("no review rules are configured"), "{body}"); assert!( !body.contains("forge's diff"), "a run that never ran read no diff: {body}"