approve a requested review when the repo has no review rules

On deploy: a review requested of the review classifier on a PR whose repo
holds neither .hyperhive/review-classifier.yml nor .json on its default
branch now posts an APPROVED review at once, saying no review rules are
configured for the repo and it was approved without checks. No model call
is made. Before, that case posted a "No verdict" comment. A rules file that
exists but fails to load or parse still posts no verdict. The rule model is
unchanged and instance-wide: every repo without a rules file is approved.

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
+name or description, and two entries with one name are errors too. A file
-falls back to a pass: a repo whose file doesn't load has no rules.
+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

swarm-controller/src/review_classifier.rs

@@ -468,6 +468,34 @@ async fn rules(
     owner: &str,
     name: &str,
 ) -> Result<Json<Rules>, 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<Loaded, ProblemDetails> {
     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(
+            return Ok(Loaded::Absent {
-                StatusCode::NOT_FOUND,
+                branch: read.branch,
-                &format!(
+            });
-                    "{repo}'s default branch {} holds neither {yml} nor {json}",
-                    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,

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 {
+    let rules = match super::load_rules(files, &req.owner, &req.name).await {
-        Ok(axum::Json(rules)) => rules,
+        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}"