From dd2f0d3eceed21c0014731c48f45cf906ab817db Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 25 Oct 2026 16:16:07 +0100 Subject: [PATCH] let notification flows start from submitted pull request reviews --- docs/swarm/README.md | 3 +- docs/swarm/notify-router.md | 24 +++++- .../src/pages/notifications/FlowTypes.ts | 4 + swarm-controller/src/forge.rs | 20 ++++- swarm-controller/src/webhook.rs | 4 +- swarm-notify-router/src/route.rs | 86 ++++++++++++++++++- swarm-wire/src/notify_flows.rs | 12 +++ 7 files changed, 142 insertions(+), 11 deletions(-) diff --git a/docs/swarm/README.md b/docs/swarm/README.md index cef2c113..b9714262 100644 --- a/docs/swarm/README.md +++ b/docs/swarm/README.md @@ -420,7 +420,8 @@ itself, all under `https:///webhook/forge/`: - an instance-wide `pull_request_review_request` system hook, which starts the [review classifier](review-classifier.md#trigger-and-lifecycle) - instance-wide `issues` + `issue_assign` and `pull_request` + - `pull_request_assign` + `pull_request_sync` system hooks, whose deliveries it + `pull_request_assign` + `pull_request_sync` + `pull_request_review_approved` + + `pull_request_review_rejected` + `pull_request_review_comment` system hooks, whose deliveries it keeps in the `SWARM_EVENTS` stream for the [notification router](notify-router.md#the-path) diff --git a/docs/swarm/notify-router.md b/docs/swarm/notify-router.md index 3931aba3..252b5f9d 100644 --- a/docs/swarm/notify-router.md +++ b/docs/swarm/notify-router.md @@ -41,8 +41,9 @@ router notices an unassign only on an issue it has seen an assign event for. 1. swarm-controller registers two instance-wide hooks, one for the `issues` and `issue_assign` events at `/webhook/forge/issues` and one for - `pull_request`, `pull_request_assign` and `pull_request_sync` (a push to an - open PR's branch) at `/webhook/forge/pull-request`. At each start it adds + `pull_request`, `pull_request_assign`, `pull_request_sync` (a push to an + open PR's branch) and the submitted-review events `pull_request_review_approved`, + `pull_request_review_rejected` and `pull_request_review_comment` at `/webhook/forge/pull-request`. At each start it adds any of these events a registered hook lacks. 2. On each delivery the controller checks the HMAC and publishes the body, unchanged, to the `SWARM_EVENTS` stream on @@ -90,8 +91,8 @@ switched off, and the custom flows. Only the controller writes it, through `GET` and `PUT /api/notify/flows`. A `PUT` replaces the whole document in one compare-and-set. The controller answers 400 unless every flow passes the tree check above and reads only what exists: a source event from `issues`, -`issue_assign`, `pull_request`, `pull_request_assign` or `pull_request_sync`, -and condition and recipient fields from the event's field list. The same goes for the `{field}` +`issue_assign`, `pull_request`, `pull_request_assign`, `pull_request_sync` or one of the +three `pull_request_review_*` events, and condition and recipient fields from the event's field list. The same goes for the `{field}` placeholders in a todo's key and text. A `PUT` must carry the `revision` it read, `0` for no document, and gets 409 when the stored revision is another. Swarm UI's [`/notifications`](ui.md#notification-flows) page shows the document and @@ -156,6 +157,21 @@ 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 `error:`. +**Reviews as a source.** A submitted review arrives as a pull request +payload: `pull_request` and `repository` as for any PR event, `action` is +`reviewed`, and `sender` is the reviewer. Approving, requesting changes and +commenting are three event types, so a flow picks the verdict with its source +and reads the reviewer with a condition on `sender`. A flow that asks argus to +review once the classifier has approved: + +| node | setting | +| ---------------- | ----------------------------------- | +| source | `pull_request_review_approved` | +| match | `sender` equals `review-classifier` | +| request_reviewer | `reviewer` = `argus` | + +The skip rule above applies: the router asks for argus's review once per head. + **What the router runs.** It reads the key at most every 10 seconds, when an event arrives, and runs the built-ins you left on, then the custom flows that are on and pass the tree check. It logs a custom flow that fails the check, diff --git a/frontend/packages/swarm-ui/src/pages/notifications/FlowTypes.ts b/frontend/packages/swarm-ui/src/pages/notifications/FlowTypes.ts index 1ec956ae..b467417f 100644 --- a/frontend/packages/swarm-ui/src/pages/notifications/FlowTypes.ts +++ b/frontend/packages/swarm-ui/src/pages/notifications/FlowTypes.ts @@ -119,6 +119,10 @@ export const FALLBACK_SCHEMA: FlowSchema = { "issue_assign", "pull_request", "pull_request_assign", + "pull_request_sync", + "pull_request_review_approved", + "pull_request_review_rejected", + "pull_request_review_comment", ].map((name) => ({ name, description: "" })), fields: [ "event", diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 7685dc7a..2b89d2d1 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -1780,12 +1780,19 @@ fn hook_registration(kind: DeliveryKind) -> (&'static [&'static str], HookScope< // classifier's account access can request its review. DeliveryKind::ReviewRequest => (&["pull_request_review_request"], HookScope::Instance), // Instance-wide, for the notification router, which reads the assign - // events and a push to a PR's branch. They are named: Forgejo's web + // events, a push to a PR's branch and submitted reviews. They are named: Forgejo's web // form shows each as a box of its own, so a hook edited there can drop // one while keeping `issues` or `pull_request`. DeliveryKind::Issues => (&["issues", "issue_assign"], HookScope::Instance), DeliveryKind::PullRequest => ( - &["pull_request", "pull_request_assign", "pull_request_sync"], + &[ + "pull_request", + "pull_request_assign", + "pull_request_sync", + "pull_request_review_approved", + "pull_request_review_rejected", + "pull_request_review_comment", + ], HookScope::Instance, ), } @@ -2131,7 +2138,14 @@ mod tests { assert_eq!(events(DeliveryKind::Issues), ["issues", "issue_assign"]); assert_eq!( events(DeliveryKind::PullRequest), - ["pull_request", "pull_request_assign", "pull_request_sync"] + [ + "pull_request", + "pull_request_assign", + "pull_request_sync", + "pull_request_review_approved", + "pull_request_review_rejected", + "pull_request_review_comment", + ] ); // The events a stored flow's source may name are the ones the hooks // deliver to the router. diff --git a/swarm-controller/src/webhook.rs b/swarm-controller/src/webhook.rs index 6ba7f25e..80acb52f 100644 --- a/swarm-controller/src/webhook.rs +++ b/swarm-controller/src/webhook.rs @@ -233,8 +233,8 @@ pub(super) enum DeliveryKind { /// assigned, …), kept for the notification router (`crate::forge_events`). #[strum(serialize = "issues")] Issues, - /// Every `pull_request` event on every repo in the instance, kept the - /// same way. + /// Every `pull_request` event, and every submitted review, on every repo in + /// the instance, kept the same way. #[strum(serialize = "pull-request")] PullRequest, } diff --git a/swarm-notify-router/src/route.rs b/swarm-notify-router/src/route.rs index 83ae39ef..97c728c7 100644 --- a/swarm-notify-router/src/route.rs +++ b/swarm-notify-router/src/route.rs @@ -78,7 +78,7 @@ pub fn route( #[cfg(test)] mod tests { use super::*; - use swarm_wire::notify_flows::builtins; + use swarm_wire::notify_flows::{Condition, Edge, Node, NodeKind, builtins}; fn delivery(action: &str, assignees: &[&str]) -> Delivery { Delivery { @@ -248,4 +248,88 @@ mod tests { assert!(r.actions.is_empty()); assert_eq!(r.next, before); } + + /// The body Forgejo sends for a submitted review: a pull request payload + /// whose `sender` is the reviewer. + fn review_body(reviewer: &str) -> Vec { + serde_json::to_vec(&serde_json::json!({ + "action": "reviewed", + "number": 9, + "pull_request": { + "number": 9, + "title": "t", + "html_url": "u", + "assignees": [], + }, + "repository": { "id": 1, "full_name": "o/r" }, + "sender": { "login": reviewer }, + "review": { "type": "pull_request_review_approved", "content": "" }, + })) + .expect("encodes") + } + + /// "A review by the classifier on a PR → request argus's review", as a + /// user builds it: valid, routed from the delivery body, and gated on the + /// reviewer. + #[test] + fn a_review_by_the_classifier_requests_argus() { + let id = |s: &str| s.to_owned(); + let flow = Flow { + id: id("classifier-then-argus"), + name: id("classifier-then-argus"), + enabled: true, + nodes: vec![ + Node { + id: id("source"), + kind: NodeKind::Source { + events: vec![id("pull_request_review_approved")], + }, + }, + Node { + id: id("match"), + kind: NodeKind::Match { + when: vec![Condition { + field: id("sender"), + equals: id("review-classifier"), + }], + }, + }, + Node { + id: id("request"), + kind: NodeKind::RequestReviewer { + reviewer: id("argus"), + }, + }, + ], + edges: vec![ + Edge { + from: id("source"), + to: id("match"), + }, + Edge { + from: id("match"), + to: id("request"), + }, + ], + }; + assert_eq!(flow.validate(), Ok(())); + let flows = [flow]; + let routed = |event: &str, reviewer: &str| { + let d = Delivery::parse(&review_body(reviewer)).expect("parses"); + route(event, &d, &Snapshot::default(), &flows, |_| true).requests + }; + assert_eq!( + routed("pull_request_review_approved", "review-classifier"), + [ReviewRequest { + flow: "classifier-then-argus".to_owned(), + node: "request".to_owned(), + repo: "o/r".to_owned(), + number: 9, + reviewer: "argus".to_owned(), + }] + ); + // Another reviewer, and another review event, request nothing. + assert!(routed("pull_request_review_approved", "mara").is_empty()); + assert!(routed("pull_request_review_rejected", "review-classifier").is_empty()); + } } diff --git a/swarm-wire/src/notify_flows.rs b/swarm-wire/src/notify_flows.rs index a5542c88..4b78268b 100644 --- a/swarm-wire/src/notify_flows.rs +++ b/swarm-wire/src/notify_flows.rs @@ -62,6 +62,18 @@ pub const SOURCE_EVENTS: &[(&str, &str)] = &[ "pull_request_sync", "A push to the branch of an open pull request", ), + ( + "pull_request_review_approved", + "A review submitted on a pull request that approves it", + ), + ( + "pull_request_review_rejected", + "A review submitted on a pull request that asks for changes", + ), + ( + "pull_request_review_comment", + "A review submitted on a pull request that only comments", + ), ]; /// The fields of an event a condition or a recipient can name, with what each