diff --git a/docs/scheduler/observability.md b/docs/scheduler/observability.md index 70437f48..b3980b29 100644 --- a/docs/scheduler/observability.md +++ b/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 | -| ------------------------------------------ | ---- | --------- | ------------------------------------------------------------------------------------ | -| `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.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.ask.retries` | — | counter | `runtime`: `api_key`, `acp` | +| Metric | Unit | Kind | Attributes | +| ------------------------------------------ | ---- | --------- | --------------------------------------------------------------------------------------------- | +| `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.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.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 diff --git a/docs/swarm/notify-router.md b/docs/swarm/notify-router.md index 8f5298bc..2feebddf 100644 --- a/docs/swarm/notify-router.md +++ b/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` -for `review-classifier` ([below](#requesting-a-review)). The code defines them +match on `action` (`opened`, `reopened`, `synchronized`) and `draft` = `false`, or on +`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. diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index 5923781d..4c8e525e 100644 --- a/docs/swarm/review-classifier.md +++ b/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 diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 2615003c..9bed95fa 100644 --- a/swarm-controller/src/forge.rs +++ b/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, + /// 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, }) } diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index d971ee2e..fa14ee57 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/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()) diff --git a/swarm-controller/src/review_classifier/metrics.rs b/swarm-controller/src/review_classifier/metrics.rs index 7162c16b..5b7695ed 100644 --- a/swarm-controller/src/review_classifier/metrics.rs +++ b/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", } } } diff --git a/swarm-controller/src/review_classifier/review.rs b/swarm-controller/src/review_classifier/review.rs index be3ae18c..03291de7 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/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>, + head_of: Mutex>, + drafts: Mutex>, } 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 { 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; diff --git a/swarm-notify-router/src/event.rs b/swarm-notify-router/src/event.rs index cf36532e..f446a440 100644 --- a/swarm-notify-router/src/event.rs +++ b/swarm-notify-router/src/event.rs @@ -28,6 +28,13 @@ pub struct Delivery { pub sender: String, pub is_pull: bool, pub assignees: BTreeSet, + /// 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 { + 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"); diff --git a/swarm-notify-router/src/flow.rs b/swarm-notify-router/src/flow.rs index f038e3a1..9cd4bb3a 100644 --- a/swarm-notify-router/src/flow.rs +++ b/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"], &[], &[]); diff --git a/swarm-notify-router/src/route.rs b/swarm-notify-router/src/route.rs index e6360e59..760f8b92 100644 --- a/swarm-notify-router/src/route.rs +++ 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 { diff --git a/swarm-wire/src/notify_flows.rs b/swarm-wire/src/notify_flows.rs index fc35fc43..0d634349 100644 --- a/swarm-wire/src/notify_flows.rs +++ b/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"] { - let (matched, request) = (format!("match-{action}"), format!("request-{action}")); + for (name, field, equals) in branches { + 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 { - when: vec![Condition { - field: "action".to_owned(), - equals: action.to_owned(), - }], - }, + kind: NodeKind::Match { when }, }); 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 { - when: vec![Condition { - field: "action".to_owned(), - equals: action.to_owned() - }] - }) + Some(&NodeKind::Match { when }) ); assert_eq!( f.node(&format!("request-{action}")).map(|n| &n.kind),