From b1a00a32223e369a47535770ed5e80ee2ca92ee5 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 28 Oct 2026 03:31:38 +0100 Subject: [PATCH] request the review classifier when a pr is opened or reopened --- docs/swarm/notify-router.md | 8 +- docs/swarm/review-classifier.md | 7 +- .../src/review_classifier/review.rs | 2 +- swarm-notify-router/src/dry_run.rs | 2 +- swarm-notify-router/src/flow.rs | 23 +++++ swarm-notify-router/src/route.rs | 13 ++- swarm-wire/src/notify_flows.rs | 93 +++++++++++++------ 7 files changed, 110 insertions(+), 38 deletions(-) diff --git a/docs/swarm/notify-router.md b/docs/swarm/notify-router.md index 85309cde..8f5298bc 100644 --- a/docs/swarm/notify-router.md +++ b/docs/swarm/notify-router.md @@ -79,7 +79,8 @@ levels deep, and 100 custom flows. A `todo` node sends to the agents an event 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_sync` → `request_reviewer` +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 (`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 @@ -150,8 +151,9 @@ not retried: the next push asks again. A request repeated for the same head is 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 on a push to any open -PR. The classifier then runs as it does for any requested review +`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 +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 repo the classifier's account can't read, or a PR that account opened, answers diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index 007e9f73..5923781d 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -86,9 +86,10 @@ as a reviewer on a PR starts a run: 4. The controller posts the result on the PR as the service account's review, on the commit it read. -A push starts no run by itself. The notification router's `head-classifier` -flow requests the account's review 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)). +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)). 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/review_classifier/review.rs b/swarm-controller/src/review_classifier/review.rs index f99095ad..be3ae18c 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/swarm-controller/src/review_classifier/review.rs @@ -2,7 +2,7 @@ //! ([`LOGIN`]) runs the repo's rules against the PR and posts the result as //! that account's review, on the commit the run read. //! -//! Only a review request starts a run. A push makes none itself: the +//! Only a review request starts a run. Opening or pushing to a PR makes none itself: the //! notification router's `head-classifier` flow requests the review. A fresh //! request retires the account's earlier review: the forge clears the //! `official` flag on a reviewer's previous reviews when they are requested diff --git a/swarm-notify-router/src/dry_run.rs b/swarm-notify-router/src/dry_run.rs index e0f147a8..5a5775e2 100644 --- a/swarm-notify-router/src/dry_run.rs +++ b/swarm-notify-router/src/dry_run.rs @@ -118,7 +118,7 @@ mod tests { assert_eq!(count(&r, "closed", "sink"), 1); // No pull request push: the classifier flow counts zero. assert_eq!(count(&r, "head-classifier", "source"), 0); - assert_eq!(count(&r, "head-classifier", "request"), 0); + assert_eq!(count(&r, "head-classifier", "request-opened"), 0); } /// A condition on a field that depends on the previous event of the diff --git a/swarm-notify-router/src/flow.rs b/swarm-notify-router/src/flow.rs index c2ceb1df..f038e3a1 100644 --- a/swarm-notify-router/src/flow.rs +++ b/swarm-notify-router/src/flow.rs @@ -435,6 +435,29 @@ mod tests { assert!(evaluate(&f, "pull_request_sync", &pull("assigned", "PR")).is_empty()); } + /// The built-in classifier flow requests on a push, an open and a reopen, + /// and on no other action of a `pull_request` event. + #[test] + fn the_classifier_flow_requests_on_open_reopen_and_push_only() { + let f = swarm_wire::notify_flows::builtins() + .into_iter() + .find(|f| f.id == "head-classifier") + .expect("the built-in"); + assert_eq!(f.validate(), Ok(())); + let pull = |action: &str| { + let mut fields = event(action, &[], &[], &[]); + fields.insert("kind", text("PR")); + fields + }; + let got = |event_type: &str, action: &str| requests(&f, event_type, &pull(action)).len(); + assert_eq!(got("pull_request", "opened"), 1); + assert_eq!(got("pull_request", "reopened"), 1); + assert_eq!(got("pull_request_sync", "synchronized"), 1); + assert_eq!(got("pull_request", "closed"), 0); + assert_eq!(got("pull_request", "edited"), 0); + assert_eq!(got("pull_request_assign", "opened"), 0); + } + #[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 97c728c7..e6360e59 100644 --- a/swarm-notify-router/src/route.rs +++ b/swarm-notify-router/src/route.rs @@ -212,7 +212,7 @@ mod tests { r.requests, [ReviewRequest { flow: "head-classifier".to_owned(), - node: "request".to_owned(), + node: "request-synchronized".to_owned(), repo: "o/r".to_owned(), number: 9, reviewer: "review-classifier".to_owned(), @@ -222,10 +222,19 @@ mod tests { assert_eq!(r.next, before); // The source gates: another PR event requests nothing. - for other in ["pull_request", "pull_request_assign", "issues"] { + for other in ["pull_request_assign", "issues"] { let r = route(other, &pull, &before, &flows, |_| true); assert!(r.requests.is_empty(), "{other}"); } + // Opening a PR is one `pull_request` event with action `opened`. + let opened = Delivery { + is_pull: true, + ..delivery("opened", &[]) + }; + let r = route("pull_request", &opened, &before, &flows, |_| false); + assert_eq!(r.requests.len(), 1); + assert_eq!(r.requests[0].node, "request-opened"); + assert!(r.actions.is_empty() && r.skipped.is_empty()); // Only a PR can be asked to review. let issue = delivery("synchronized", &[]); let r = route("pull_request_sync", &issue, &before, &flows, |_| true); diff --git a/swarm-wire/src/notify_flows.rs b/swarm-wire/src/notify_flows.rs index 4b78268b..fc35fc43 100644 --- a/swarm-wire/src/notify_flows.rs +++ b/swarm-wire/src/notify_flows.rs @@ -771,7 +771,8 @@ impl TodoSink { /// then tells each agent that held one with a `closed` todo. The clear is the /// first child of the match and runs first: its prefix also covers the /// `closed` key. -/// - `head-classifier`: a push to a PR requests the review classifier's review. +/// - `head-classifier`: a PR opened, reopened or pushed to requests the review +/// classifier's review. #[must_use] pub fn builtins() -> Vec { let assignment_key = "forge:{repo}#{number}:assignment"; @@ -837,30 +838,48 @@ fn closed() -> Flow { flow } -/// source(`pull_request_sync`) → request the review classifier's review. +/// source(`pull_request`, `pull_request_sync`) → match(`action` = `opened`, +/// `reopened` or `synchronized`) → request the review classifier's review. fn head_classifier() -> Flow { + let mut nodes = vec![Node { + id: "source".to_owned(), + kind: NodeKind::Source { + events: vec!["pull_request".to_owned(), "pull_request_sync".to_owned()], + }, + }]; + let mut edges = Vec::new(); + for action in ["opened", "reopened", "synchronized"] { + let (matched, request) = (format!("match-{action}"), format!("request-{action}")); + nodes.push(Node { + id: matched.clone(), + kind: NodeKind::Match { + when: vec![Condition { + field: "action".to_owned(), + equals: action.to_owned(), + }], + }, + }); + nodes.push(Node { + id: request.clone(), + kind: NodeKind::RequestReviewer { + reviewer: CLASSIFIER_ACCOUNT.to_owned(), + }, + }); + edges.push(Edge { + from: "source".to_owned(), + to: matched.clone(), + }); + edges.push(Edge { + from: matched, + to: request, + }); + } Flow { id: "head-classifier".to_owned(), name: "head-classifier".to_owned(), enabled: true, - nodes: vec![ - Node { - id: "source".to_owned(), - kind: NodeKind::Source { - events: vec!["pull_request_sync".to_owned()], - }, - }, - Node { - id: "request".to_owned(), - kind: NodeKind::RequestReviewer { - reviewer: CLASSIFIER_ACCOUNT.to_owned(), - }, - }, - ], - edges: vec![Edge { - from: "source".to_owned(), - to: "request".to_owned(), - }], + nodes, + edges, } } @@ -1214,24 +1233,42 @@ mod tests { assert!(matches!(node.kind, NodeKind::RequestReviewer { .. })); } - /// The built-in that requests the classifier on every push. + /// The built-in that requests the classifier on an open, a reopen and a push. #[test] - fn head_classifier_requests_the_classifier_on_a_push() { + fn head_classifier_requests_the_classifier_on_open_reopen_and_push() { let f = builtin("head-classifier"); assert_eq!(f.validate(), Ok(())); assert_eq!( f.nodes[0].kind, NodeKind::Source { - events: vec!["pull_request_sync".to_owned()] + events: vec!["pull_request".to_owned(), "pull_request_sync".to_owned()] } ); assert_eq!( - f.nodes[1].kind, - NodeKind::RequestReviewer { - reviewer: CLASSIFIER_ACCOUNT.to_owned() - } + f.children("source"), + ["match-opened", "match-reopened", "match-synchronized"] ); - assert_eq!(f.children("source"), ["request"]); + for action in ["opened", "reopened", "synchronized"] { + 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() + }] + }) + ); + assert_eq!( + f.node(&format!("request-{action}")).map(|n| &n.kind), + Some(&NodeKind::RequestReviewer { + reviewer: CLASSIFIER_ACCOUNT.to_owned() + }) + ); + assert_eq!( + f.children(&format!("match-{action}")), + [format!("request-{action}")] + ); + } } #[test]