review-classifier: a requested review runs even on a draft pr

docs/scheduler/observability.md

@@ -567,13 +567,13 @@ the same exporter as the VCS metrics above. The classifier service adds the
 retry counter beside its cost counters
 ([`review-classifier.md`](../swarm/review-classifier.md#observability-and-cost)).
 
-| Metric                                     | Unit | Kind      | Attributes                                                                                    |
+| Metric                                     | Unit | Kind      | Attributes                                                                           |
-| ------------------------------------------ | ---- | --------- | --------------------------------------------------------------------------------------------- |
+| ------------------------------------------ | ---- | --------- | ------------------------------------------------------------------------------------ |
-| `hyperhive.review_classifier.requests`     | —    | counter   | `outcome`: `queued`, `queue_failed`, `unparsable`, `deferred`, `started`, `reposted`, `draft` |
+| `hyperhive.review_classifier.requests`     | —    | counter   | `outcome`: `queued`, `queue_failed`, `unparsable`, `deferred`, `started`, `reposted` |
-| `hyperhive.review_classifier.verdicts`     | —    | counter   | `outcome`: `approve`, `request_changes`, `no_verdict`                                         |
+| `hyperhive.review_classifier.verdicts`     | —    | counter   | `outcome`: `approve`, `request_changes`, `no_verdict`                                |
-| `hyperhive.review_classifier.checks`       | —    | counter   | `check` (the rule's name), `result`: `match`, `no_match`, `unsure`, `no_answer`               |
+| `hyperhive.review_classifier.checks`       | —    | counter   | `check` (the rule's name), `result`: `match`, `no_match`, `unsure`, `no_answer`      |
-| `hyperhive.review_classifier.run.duration` | `s`  | histogram | `outcome`: the verdict names, or `post_failed`                                                |
+| `hyperhive.review_classifier.run.duration` | `s`  | histogram | `outcome`: the verdict names, or `post_failed`                                       |
-| `hyperhive.review_classifier.ask.retries`  | —    | counter   | `runtime`: `api_key`, `acp`                                                                   |
+| `hyperhive.review_classifier.ask.retries`  | —    | counter   | `runtime`: `api_key`, `acp`                                                          |
 
 A verdict means the classifier decided, before it posts the review: a run
 whose post fails still adds to `verdicts`, and shows as `post_failed` in

docs/swarm/review-classifier.md

@@ -92,10 +92,10 @@ reopens a PR and on every push to an open PR, unless its review of the new head
 already stands ([`notify-router.md`](notify-router.md#requesting-a-review)). It
 requests nothing for a draft PR, one whose title starts with `WIP:` or `[WIP]`
 and which the forge reports as `draft`. The edit that takes a PR out of draft
-requests the review once, for the head the PR has then. A request that reaches
+requests the review once, for the head the PR has then. That edit recognises
-the controller for a PR that's a draft when the run place frees up (queued,
+only the default `WIP:` and `[WIP]` title prefixes: after taking a PR out of
-or made by hand) runs nothing, posts nothing and counts as `draft`; request
+draft that used another prefix, request the review by hand. A request that
-the review again once the PR is ready.
+reaches the controller is always reviewed, whether or not the PR is a draft.
 
 When a post can't reach the secret store or the forge, because the
 connection fails or a try gets no answer within two minutes, the controller

swarm-controller/src/forge.rs

@@ -157,9 +157,6 @@ pub struct PullForReview {
     /// Why `diff` is the forge's `.diff`, with 3 lines of context, rather
     /// than [`pull_diff::wide`]'s. `None` when it is the wide one.
     pub narrowed: Option<String>,
-    /// The forge calls the PR a draft (a title starting with a
-    /// work-in-progress prefix).
-    pub draft: bool,
 }
 
 /// Files on a repo's branch, as [`Client::default_branch_files`] and
@@ -1385,7 +1382,6 @@ impl Client {
             .await
             .with_context(|| format!("read PR {number} of {owner}/{repo}"))?;
         let changed_files = pull.changed_files;
-        let draft = pull.draft.unwrap_or(false);
         let head_sha = pull
             .head
             .and_then(|h| h.sha)
@@ -1447,7 +1443,6 @@ impl Client {
             commits,
             diff,
             narrowed,
-            draft,
         })
     }
 

swarm-controller/src/review_classifier.rs

@@ -870,7 +870,6 @@ pub(crate) mod tests {
                     commits: Vec::new(),
                     diff: String::new(),
                     narrowed: self.narrowed.clone(),
-                    draft: false,
                 })
             } else {
                 Err(ForgejoError::ApiError(ApiErrorKind::NotFound { errors: None }.into()).into())

swarm-controller/src/review_classifier/metrics.rs

@@ -32,8 +32,6 @@ pub(super) enum Request {
     Started,
     /// Taken for a head already reviewed; its verdict was posted again.
     Reposted,
-    /// Taken for a PR the forge calls a draft; nothing ran or posted.
-    Draft,
 }
 
 impl Request {
@@ -45,7 +43,6 @@ impl Request {
             Self::Deferred => "deferred",
             Self::Started => "started",
             Self::Reposted => "reposted",
-            Self::Draft => "draft",
         }
     }
 }

swarm-controller/src/review_classifier/review.rs

@@ -678,16 +678,6 @@ async fn review(
     slot: Slot,
 ) {
     let pull = read_pull(forge, req).await;
-    if matches!(&pull, Ok(pull) if pull.draft) {
-        drop(slot);
-        metrics.request(Request::Draft);
-        tracing::info!(
-            repo = %format!("{}/{}", req.owner, req.name), pr = req.pr,
-            head_sha = req.head.as_str(),
-            "review requested on a draft PR; skipping it"
-        );
-        return;
-    }
     let moved = matches!(&pull, Ok(pull) if !req.head.is_empty() && pull.head_sha != req.head);
     if moved {
         tracing::info!(
@@ -1661,7 +1651,6 @@ mod tests {
     struct Heads {
         fake: Fake,
         head_of: Mutex<std::collections::HashMap<i64, String>>,
-        drafts: Mutex<std::collections::HashSet<i64>>,
     }
 
     impl Heads {
@@ -1674,14 +1663,9 @@ mod tests {
                     vec![rules_json()],
                 ),
                 head_of: Mutex::default(),
-                drafts: Mutex::default(),
             }
         }
 
-        fn make_draft(&self, pr: i64) {
-            self.drafts.lock().expect("unpoisoned").insert(pr);
-        }
-
         fn move_to(&self, pr: i64, head: &str) {
             self.head_of
                 .lock()
@@ -1709,7 +1693,6 @@ mod tests {
                 commits: Vec::new(),
                 diff: String::new(),
                 narrowed: None,
-                draft: self.drafts.lock().expect("unpoisoned").contains(&number),
             })
         }
     }
@@ -2041,26 +2024,6 @@ mod tests {
         assert_eq!(q.rig.gate.reached(), 2, "no run after the ack");
     }
 
-    /// A request taken from the queue for a PR that is a draft by then runs
-    /// nothing and posts nothing; the other PR in the queue is reviewed.
-    #[tokio::test]
-    async fn a_queued_request_for_a_draft_pr_is_skipped() {
-        let q = Queued::new().await;
-        q.rig.forge.make_draft(7);
-        q.request(7, "abc123").await;
-        q.request(8, "abc123").await;
-
-        let _consumer = q.consume(&q.rig.reviews);
-        q.rig.gate.open(2);
-        until("the other PR is reviewed", || q.rig.posted().len() == 1).await;
-        tokio::time::sleep(TEST_TIMING.ack_wait * 2).await;
-        let posted = q.rig.posted();
-        assert_eq!(posted.len(), 1, "the draft posts nothing");
-        assert_eq!(posted[0].0, "o/r#8");
-        assert_eq!(q.rig.gate.reached(), 1, "the draft starts no run");
-        assert_eq!(q.waiting().await, 0, "the draft request is acked, not kept");
-    }
-
     #[tokio::test]
     async fn a_newer_request_for_a_pr_replaces_the_waiting_one() {
         let q = Queued::new().await;