review-classifier: skip draft prs, classify once when they leave draft

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` |
+| `hyperhive.review_classifier.requests`     | —    | counter   | `outcome`: `queued`, `queue_failed`, `unparsable`, `deferred`, `started`, `reposted`, `draft` |
-| `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/notify-router.md

@@ -80,8 +80,8 @@ field holds (`assignees_added`, `todo_holders`, …) or to one named agent.
 
 The router ships the three flows above, each a source → match on `action` →
 todo, and `head-classifier`: source `pull_request` and `pull_request_sync` →
-match on `action` (`opened`, `reopened`, `synchronized`) → `request_reviewer`
+match on `action` (`opened`, `reopened`, `synchronized`) and `draft` = `false`, or on
-for `review-classifier` ([below](#requesting-a-review)). The code defines them
+`undrafted` → `request_reviewer` for `review-classifier` ([below](#requesting-a-review)). The code defines them
 (`swarm_wire::notify_flows::builtins`), so a build that changes one reaches
 every swarm. Each switches off on `/notifications`. A custom flow has the same type and
 the same evaluator, so it can do anything a built-in does.
@@ -152,7 +152,12 @@ harmless: a reviewer with a pending request is a no-op for the forge, and one
 who has reviewed gets `skipped`.
 
 `head-classifier` requests the review classifier's review when someone opens or
-reopens a PR and on every push to an open PR. The classifier then runs as it does
+reopens a PR and on every push to an open PR that isn't a draft. A draft is a
+PR the forge reports with `draft` true, which it does for a title starting
+`WIP:` or `[WIP]`. A title edit that drops that prefix (`undrafted` is true:
+the event is `edited`, the PR is no draft, and the old title in the payload
+started with the prefix) requests the review once, so a PR whose last push
+came as a draft gets a classification. The classifier then runs as it does
 for any requested review
 ([`review-classifier.md`](review-classifier.md#trigger-and-lifecycle)), through
 its own request queue, which replaces a waiting request for the same PR. A
@@ -194,7 +199,9 @@ document of a version it doesn't know.
 **Deploying in any order.** The controller, the router and the UI can be on
 different hosts. A router that predates `request_reviewer` reads a document
 holding such a node (a custom flow with one) as one it can't read and keeps the
-flows it had; it also has no `head-classifier`. A router that has it, with a
+flows it had; it also has no `head-classifier`. A router that has `head-classifier`
+but predates the `draft` and `undrafted` fields requests the review of a draft PR
+as before. A router that has `request_reviewer`, with a
 controller that predates it, gets no answer to the request and logs that. A controller that predates this leaves the key absent, and the
 router runs the built-ins as before. A router that predates this ignores the
 document, so the page and the API show a state the router isn't yet running.

docs/swarm/review-classifier.md

@@ -89,7 +89,13 @@ as a reviewer on a PR starts a run:
 Opening or pushing to a PR starts no run by itself. The notification router's
 `head-classifier` flow requests the account's review when someone opens or
 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)).
+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
+the controller for a PR that's a draft when the run place frees up (queued,
+or made by hand) runs nothing, posts nothing and counts as `draft`; request
+the review again once the PR is ready.
 
 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,6 +157,9 @@ 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
@@ -1382,6 +1385,7 @@ 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)
@@ -1443,6 +1447,7 @@ impl Client {
             commits,
             diff,
             narrowed,
+            draft,
         })
     }
 

swarm-controller/src/review_classifier.rs

@@ -870,6 +870,7 @@ 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,6 +32,8 @@ 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 {
@@ -43,6 +45,7 @@ impl Request {
             Self::Deferred => "deferred",
             Self::Started => "started",
             Self::Reposted => "reposted",
+            Self::Draft => "draft",
         }
     }
 }

swarm-controller/src/review_classifier/review.rs

@@ -678,6 +678,16 @@ 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!(
@@ -1650,7 +1660,8 @@ mod tests {
     /// [`Heads::move_to`] moves it.
     struct Heads {
         fake: Fake,
-        heads: Mutex<std::collections::HashMap<i64, String>>,
+        head_of: Mutex<std::collections::HashMap<i64, String>>,
+        drafts: Mutex<std::collections::HashSet<i64>>,
     }
 
     impl Heads {
@@ -1662,12 +1673,17 @@ mod tests {
                     },
                     vec![rules_json()],
                 ),
-                heads: Mutex::default(),
+                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.heads
+            self.head_of
                 .lock()
                 .expect("unpoisoned")
                 .insert(pr, head.to_owned());
@@ -1682,12 +1698,18 @@ mod tests {
             number: i64,
         ) -> anyhow::Result<PullForReview> {
             anyhow::ensure!((owner, repo) == ("o", "r"), "no such repo");
-            let head = self.heads.lock().expect("unpoisoned").get(&number).cloned();
+            let head = self
+                .head_of
+                .lock()
+                .expect("unpoisoned")
+                .get(&number)
+                .cloned();
             Ok(PullForReview {
                 head_sha: head.unwrap_or_else(|| "abc123".to_owned()),
                 commits: Vec::new(),
                 diff: String::new(),
                 narrowed: None,
+                draft: self.drafts.lock().expect("unpoisoned").contains(&number),
             })
         }
     }
@@ -2019,6 +2041,26 @@ 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;

swarm-notify-router/src/event.rs

@@ -28,6 +28,13 @@ pub struct Delivery {
     pub sender: String,
     pub is_pull: bool,
     pub assignees: BTreeSet<String>,
+    /// The PR is a draft: the forge's `pull_request.draft`, true for a title
+    /// that starts with one of its work-in-progress prefixes.
+    pub draft: bool,
+    /// The delivery is a title edit that took a PR out of draft: `edited`, no
+    /// longer a draft, and the old title in `changes.title.from` starts with a
+    /// work-in-progress prefix.
+    pub undrafted: bool,
 }
 
 impl Delivery {
@@ -55,8 +62,17 @@ impl Delivery {
                     .collect()
             })
             .unwrap_or_default();
+        let action = text(&json, "action");
+        let draft = is_pull && item.get("draft").and_then(Json::as_bool).unwrap_or(false);
+        let undrafted = is_pull
+            && action == "edited"
+            && !draft
+            && json
+                .pointer("/changes/title/from")
+                .and_then(Json::as_str)
+                .is_some_and(is_wip_title);
         Some(Self {
-            action: text(&json, "action"),
+            action,
             repo: text(repository, "full_name"),
             repo_id: repository.get("id")?.as_i64()?,
             number: item.get("number")?.as_i64()?,
@@ -68,6 +84,8 @@ impl Delivery {
                 .unwrap_or_default(),
             is_pull,
             assignees,
+            draft,
+            undrafted,
         })
     }
 
@@ -78,6 +96,22 @@ impl Delivery {
     }
 }
 
+/// The prefixes the forge treats as work-in-progress (its default
+/// `WORK_IN_PROGRESS_PREFIXES`), compared without regard to case.
+const WIP_PREFIXES: [&str; 2] = ["WIP:", "[WIP]"];
+
+/// Whether `title` starts with a work-in-progress prefix. A title edit's
+/// payload carries the old title and no flag for it, so the prefix is the only
+/// way to tell what the PR was before.
+fn is_wip_title(title: &str) -> bool {
+    let title = title.trim_start();
+    WIP_PREFIXES.iter().any(|p| {
+        title
+            .get(..p.len())
+            .is_some_and(|head| head.eq_ignore_ascii_case(p))
+    })
+}
+
 /// What the router keeps per issue in its state bucket.
 #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)]
 pub struct Snapshot {
@@ -119,6 +153,8 @@ pub fn fields(event_type: &str, delivery: &Delivery, snapshot: &Snapshot) -> Fie
         ("title", text(&delivery.title)),
         ("url", text(&delivery.url)),
         ("sender", text(&delivery.sender)),
+        ("draft", text(bool_text(delivery.draft))),
+        ("undrafted", text(bool_text(delivery.undrafted))),
         (
             "assignees",
             Value::List(delivery.assignees.iter().cloned().collect()),
@@ -132,6 +168,10 @@ pub fn fields(event_type: &str, delivery: &Delivery, snapshot: &Snapshot) -> Fie
     ])
 }
 
+const fn bool_text(b: bool) -> &'static str {
+    if b { "true" } else { "false" }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -212,6 +252,55 @@ mod tests {
         assert_eq!(Delivery::parse(b"{\"ref\":\"main\"}"), None);
     }
 
+    fn pull_body(action: &str, title: &str, draft: bool, was: Option<&str>) -> Vec<u8> {
+        let mut body = serde_json::json!({
+            "action": action,
+            "number": 7,
+            "pull_request": { "number": 7, "title": title, "html_url": "u", "draft": draft },
+            "repository": { "id": 1, "full_name": "o/r" },
+            "sender": { "login": "s" },
+        });
+        if let Some(was) = was {
+            body["changes"] = serde_json::json!({ "title": { "from": was } });
+        }
+        serde_json::to_vec(&body).expect("encodes")
+    }
+
+    #[test]
+    fn the_forges_draft_flag_marks_a_draft() {
+        let d = Delivery::parse(&pull_body("synchronized", "WIP: t", true, None)).expect("parses");
+        assert!(d.draft && !d.undrafted);
+        let d = Delivery::parse(&pull_body("synchronized", "t", false, None)).expect("parses");
+        assert!(!d.draft && !d.undrafted);
+        let f = fields("pull_request_sync", &d, &Snapshot::default());
+        assert_eq!(f.get("draft"), Some(&Value::Text("false".to_owned())));
+    }
+
+    /// Only an edit that drops a WIP prefix from the title and leaves the PR
+    /// not a draft is an un-draft.
+    #[test]
+    fn a_title_edit_that_drops_the_wip_prefix_is_an_undraft() {
+        for was in ["WIP: t", "[WIP] t", "wip: t", "  [wip] t"] {
+            let d = Delivery::parse(&pull_body("edited", "t", false, Some(was))).expect("parses");
+            assert!(d.undrafted, "{was}");
+        }
+        for (action, title, draft, was) in [
+            ("edited", "t", false, Some("old title")),
+            ("edited", "t", false, None),
+            ("edited", "WIP: t", true, Some("WIP: u")),
+            ("opened", "t", false, Some("WIP: t")),
+        ] {
+            let d = Delivery::parse(&pull_body(action, title, draft, was)).expect("parses");
+            assert!(!d.undrafted, "{action} {title} {was:?}");
+        }
+        let f = fields(
+            "pull_request",
+            &Delivery::parse(&pull_body("edited", "t", false, Some("WIP: t"))).expect("parses"),
+            &Snapshot::default(),
+        );
+        assert_eq!(f.get("undrafted"), Some(&Value::Text("true".to_owned())));
+    }
+
     #[test]
     fn the_snapshot_turns_the_after_list_into_added_and_removed() {
         let d = Delivery::parse(&issue_assign("unassigned", &["iris"])).expect("parses");

swarm-notify-router/src/flow.rs

@@ -221,6 +221,8 @@ mod tests {
             ("number", text("7")),
             ("title", text("router")),
             ("url", text("https://forge/7")),
+            ("draft", text("false")),
+            ("undrafted", text("false")),
             ("assignees_added", list(added)),
             ("assignees_removed", list(removed)),
             ("todo_holders", list(holders)),
@@ -458,6 +460,44 @@ mod tests {
         assert_eq!(got("pull_request_assign", "opened"), 0);
     }
 
+    /// A draft PR gets no request on open, reopen or push; the edit that takes
+    /// it out of draft gets exactly one, and a title edit that does not does not.
+    #[test]
+    fn the_classifier_flow_skips_a_draft_and_requests_once_when_it_leaves_draft() {
+        let f = swarm_wire::notify_flows::builtins()
+            .into_iter()
+            .find(|f| f.id == "head-classifier")
+            .expect("the built-in");
+        let pull = |action: &str, draft: bool, undrafted: bool| {
+            let mut fields = event(action, &[], &[], &[]);
+            fields.insert("kind", text("PR"));
+            fields.insert("draft", text(if draft { "true" } else { "false" }));
+            fields.insert("undrafted", text(if undrafted { "true" } else { "false" }));
+            fields
+        };
+        let got = |event_type: &str, fields: &Fields| requests(&f, event_type, fields);
+        for (event_type, action) in [
+            ("pull_request", "opened"),
+            ("pull_request", "reopened"),
+            ("pull_request_sync", "synchronized"),
+        ] {
+            assert!(
+                got(event_type, &pull(action, true, false)).is_empty(),
+                "{action}"
+            );
+            assert_eq!(
+                got(event_type, &pull(action, false, false)).len(),
+                1,
+                "{action}"
+            );
+        }
+        let left = got("pull_request", &pull("edited", false, true));
+        assert_eq!(left.len(), 1);
+        assert_eq!(left[0].node, "request-undrafted");
+        assert!(got("pull_request", &pull("edited", false, false)).is_empty());
+        assert!(got("pull_request", &pull("edited", true, false)).is_empty());
+    }
+
     #[test]
     fn a_placeholder_expands_to_its_field_and_an_unknown_one_is_an_error() {
         let fields = event("assigned", &["a", "b"], &[], &[]);

swarm-notify-router/src/route.rs

@@ -91,6 +91,8 @@ mod tests {
             sender: "mara".to_owned(),
             is_pull: false,
             assignees: assignees.iter().map(|a| (*a).to_owned()).collect(),
+            draft: false,
+            undrafted: false,
         }
     }
 
@@ -241,6 +243,29 @@ mod tests {
         assert!(r.requests.is_empty());
     }
 
+    /// A push to a draft PR requests nothing; the title edit that takes it out
+    /// of draft requests once.
+    #[test]
+    fn a_draft_gets_no_request_until_it_leaves_draft() {
+        let flows = builtins();
+        let before = Snapshot::default();
+        let draft = Delivery {
+            is_pull: true,
+            draft: true,
+            ..delivery("synchronized", &[])
+        };
+        let r = route("pull_request_sync", &draft, &before, &flows, |_| false);
+        assert!(r.requests.is_empty());
+        let ready = Delivery {
+            is_pull: true,
+            undrafted: true,
+            ..delivery("edited", &[])
+        };
+        let r = route("pull_request", &ready, &before, &flows, |_| false);
+        assert_eq!(r.requests.len(), 1);
+        assert_eq!(r.requests[0].node, "request-undrafted");
+    }
+
     #[test]
     fn an_event_no_flow_matches_changes_nothing() {
         let before = Snapshot {

swarm-wire/src/notify_flows.rs

@@ -87,6 +87,14 @@ pub const FIELDS: &[(&str, &str)] = &[
     ("title", "The issue or pull request title"),
     ("url", "The web link to the issue or pull request"),
     ("sender", "The account that caused the event"),
+    (
+        "draft",
+        "true when the pull request is a draft (its title starts WIP: or [WIP]), else false",
+    ),
+    (
+        "undrafted",
+        "true when the event is a title edit that took the pull request out of draft, else false",
+    ),
     ("assignees", "The agents assigned after the event (a list)"),
     ("assignees_added", "The agents the event assigned (a list)"),
     (
@@ -839,8 +847,16 @@ fn closed() -> Flow {
 }
 
 /// source(`pull_request`, `pull_request_sync`) → match(`action` = `opened`,
-/// `reopened` or `synchronized`) → request the review classifier's review.
+/// `reopened` or `synchronized`, and `draft` = `false`) → request the review
+/// classifier's review; and match(`undrafted` = `true`) → the same request,
+/// for a PR whose last push came while it was a draft.
 fn head_classifier() -> Flow {
+    let branches = [
+        ("opened", "action", "opened"),
+        ("reopened", "action", "reopened"),
+        ("synchronized", "action", "synchronized"),
+        ("undrafted", "undrafted", "true"),
+    ];
     let mut nodes = vec![Node {
         id: "source".to_owned(),
         kind: NodeKind::Source {
@@ -848,16 +864,19 @@ fn head_classifier() -> Flow {
         },
     }];
     let mut edges = Vec::new();
-    for action in ["opened", "reopened", "synchronized"] {
+    for (name, field, equals) in branches {
-        let (matched, request) = (format!("match-{action}"), format!("request-{action}"));
+        let (matched, request) = (format!("match-{name}"), format!("request-{name}"));
+        let condition = |field: &str, equals: &str| Condition {
+            field: field.to_owned(),
+            equals: equals.to_owned(),
+        };
+        let mut when = vec![condition(field, equals)];
+        if field == "action" {
+            when.push(condition("draft", "false"));
+        }
         nodes.push(Node {
             id: matched.clone(),
-            kind: NodeKind::Match {
+            kind: NodeKind::Match { when },
-                when: vec![Condition {
-                    field: "action".to_owned(),
-                    equals: action.to_owned(),
-                }],
-            },
         });
         nodes.push(Node {
             id: request.clone(),
@@ -1238,6 +1257,10 @@ mod tests {
     fn head_classifier_requests_the_classifier_on_open_reopen_and_push() {
         let f = builtin("head-classifier");
         assert_eq!(f.validate(), Ok(()));
+        let cond = |field: &str, equals: &str| Condition {
+            field: field.to_owned(),
+            equals: equals.to_owned(),
+        };
         assert_eq!(
             f.nodes[0].kind,
             NodeKind::Source {
@@ -1246,17 +1269,22 @@ mod tests {
         );
         assert_eq!(
             f.children("source"),
-            ["match-opened", "match-reopened", "match-synchronized"]
+            [
+                "match-opened",
+                "match-reopened",
+                "match-synchronized",
+                "match-undrafted"
+            ]
         );
-        for action in ["opened", "reopened", "synchronized"] {
+        for action in ["opened", "reopened", "synchronized", "undrafted"] {
+            let when = if action == "undrafted" {
+                vec![cond("undrafted", "true")]
+            } else {
+                vec![cond("action", action), cond("draft", "false")]
+            };
             assert_eq!(
                 f.node(&format!("match-{action}")).map(|n| &n.kind),
-                Some(&NodeKind::Match {
+                Some(&NodeKind::Match { when })
-                    when: vec![Condition {
-                        field: "action".to_owned(),
-                        equals: action.to_owned()
-                    }]
-                })
             );
             assert_eq!(
                 f.node(&format!("request-{action}")).map(|n| &n.kind),