Watch
0
0
Fork
You've already forked hyperhive
0

swarm-controller: post the review classifier's verdict on a review request

Requesting the forge account `review-classifier` as a reviewer now runs the
repo's rules against the PR and posts the result as that account's review,
on the head commit the run read. A push starts no run; only a fresh review
request does.

- An instance-wide `pull_request_review_request` system hook delivers to
  `/webhook/forge/review-request`. A request for any other reviewer, or a
  removed request, does nothing.
- The controller reads the PR, loads the rules with the same reader as
  `GET /api/review-classifier/rules`, sends one request on the try-out
  subject, and posts APPROVED or REQUEST_CHANGES with the table as the body.
- With no clean verdict (PR or rules don't load, the run fails, or any
  classification has no answer) it posts a COMMENT saying why, never an
  approval. The event is one function, `no_clean_verdict_event`.
- Table cells are flattened, markup-escaped, and every `@` gets a
  zero-width space, so a model's reason can't break the table or mention
  anyone.
- At most two requested reviews run at once; the rest wait in memory.
- The account is minted the way hive accounts are: a five-minute pass
  queues a `MintClassifierForgeToken` node when the token at
  `swarm/services/review-classifier/forge/token` is missing or stale. The
  token is `write:repository`. The controller's bao policy gains that leaf
  (create/read/update), pinned in module-eval bao-grants. The classifier
  never sees the token.

The account gets no repo access automatically; a repo admin adds it by
hand. A re-request retires the account's earlier review: Forgejo clears
`official` on a reviewer's earlier reviews when it is requested again
(models/issues/review.go AddReviewRequest, v16.0.5), and approvals and
rejection blocks count official reviews only.

Refs #5137
This commit is contained in:
atlas 2026-10-12 14:28:42 +02:00
commit a669cd96b6
17 changed files with 1156 additions and 50 deletions

View file

@ -0,0 +1,586 @@
//! Real reviews: a review request for the classifier's forge account
//! ([`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 does not. 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 again, and only
//! official reviews count toward approvals or block a merge.
//!
//! A run with no clean verdict never approves: see [`no_clean_verdict_event`].
//! At most [`MAX_CONCURRENT_REVIEWS`] runs go at once; the rest wait here, in
//! memory, so a restart drops them.
use std::fmt::Write as _;
use std::future::Future;
use std::sync::Arc;
use serde::Deserialize;
use swarm_queue_client::review_classifier::{Overall, Reply, Row, TryOut, Verdict};
use super::{FileSource, PullSource, RunSink, split_repo};
use crate::AppState;
use crate::forge::{self, classifier_account::LOGIN, classifier_account::Review};
/// The most review runs in flight at once.
pub(crate) const MAX_CONCURRENT_REVIEWS: usize = 2;
/// The forge's review events.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum Event {
Approve,
RequestChanges,
Comment,
}
impl Event {
fn forge(self) -> &'static str {
match self {
Self::Approve => "APPROVED",
Self::RequestChanges => "REQUEST_CHANGES",
Self::Comment => "COMMENT",
}
}
}
/// What a run posts when it has no clean verdict: the rules didn't load, the
/// PR couldn't be read, the run failed, or any classification has no answer.
/// A comment neither approves nor blocks the merge.
fn no_clean_verdict_event() -> Event {
Event::Comment
}
/// A review request for [`LOGIN`].
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct Requested {
owner: String,
name: String,
pr: i64,
/// Who asked for the review.
requester: String,
}
#[derive(Deserialize)]
struct Payload {
action: String,
number: i64,
requested_reviewer: Option<User>,
repository: Repository,
sender: Option<User>,
}
#[derive(Deserialize)]
struct User {
login: String,
}
#[derive(Deserialize)]
struct Repository {
full_name: String,
}
impl Requested {
/// The request in a `pull_request_review_request` delivery, when it
/// asks for [`LOGIN`]'s review. A removed request, a request for anyone
/// else and an unparsable body are `None`.
fn parse(body: &[u8]) -> Option<Self> {
let payload: Payload = match serde_json::from_slice(body) {
Ok(p) => p,
Err(e) => {
tracing::warn!(error = %e, "webhook: review-request delivery did not parse");
return None;
}
};
let reviewer = payload.requested_reviewer?;
if payload.action != "review_requested" || !reviewer.login.eq_ignore_ascii_case(LOGIN) {
return None;
}
let (owner, name) = split_repo(&payload.repository.full_name).ok()?;
Some(Self {
owner: owner.to_owned(),
name: name.to_owned(),
pr: Some(payload.number).filter(|n| *n > 0)?,
requester: payload.sender.map(|s| s.login).unwrap_or_default(),
})
}
}
/// Start a review run for a verified `review-request` delivery that asks for
/// [`LOGIN`], and return at once.
pub(crate) fn on_delivery(state: &AppState, body: &[u8]) {
let Some(request) = Requested::parse(body) else {
return;
};
let Some(forge) = state.forge.get().map(Arc::clone) else {
tracing::warn!(
repo = %format!("{}/{}", request.owner, request.name),
pr = request.pr,
"review requested, but this host has no forge access yet; nothing is posted"
);
return;
};
let queue = Queue(state.status.as_ref().map(|s| s.queue_client()));
let slots = Arc::clone(&state.review_slots);
tokio::spawn(async move {
let Ok(_slot) = slots.acquire_owned().await else {
return;
};
Box::pin(run(&*forge, &*forge, &queue, &*forge, &request)).await;
});
}
/// Where a review is posted: the forge in production, a fake in the tests.
pub(crate) trait ReviewPoster {
fn post(
&self,
owner: &str,
repo: &str,
number: i64,
commit: Option<&str>,
review: Review,
) -> impl Future<Output = anyhow::Result<()>> + Send;
}
impl ReviewPoster for forge::Client {
async fn post(
&self,
owner: &str,
repo: &str,
number: i64,
commit: Option<&str>,
review: Review,
) -> anyhow::Result<()> {
self.post_classifier_review(owner, repo, number, commit, review)
.await
}
}
/// The swarm queue, when this host has one.
#[derive(Clone)]
struct Queue(Option<async_nats::Client>);
impl RunSink for Queue {
async fn run(&self, request: Vec<u8>) -> Reply {
let Some(client) = &self.0 else {
return Reply::Failed {
error: "no swarm queue is configured on this host".to_owned(),
};
};
if let Err(e) = swarm_queue_client::ensure_connected(client) {
return Reply::Failed {
error: swarm_queue_client::chain(&e),
};
}
client.run(request).await
}
}
/// How far a run got.
#[derive(Debug, Clone, PartialEq, Eq)]
enum Outcome {
/// Nothing reached the classifier, for this reason.
NotRun(String),
Ran(Reply),
}
/// Read the PR and the rules, run them, and post the review.
async fn run(
files: &impl FileSource,
pulls: &impl PullSource,
sink: &impl RunSink,
poster: &impl ReviewPoster,
req: &Requested,
) {
let repo = format!("{}/{}", req.owner, req.name);
let (head, outcome) = classify(files, pulls, sink, req).await;
let review = review_for(&outcome);
let event = review.event;
let sent = forge::within_forge_deadline(
"review classifier review",
poster.post(&req.owner, &req.name, req.pr, head.as_deref(), review),
)
.await;
match sent {
Some(Ok(())) => tracing::info!(
%repo, pr = req.pr, head_sha = head.as_deref().unwrap_or_default(), event,
"review classifier review posted"
),
Some(Err(e)) => tracing::warn!(
%repo, pr = req.pr, error = %format!("{e:#}"),
"posting the review classifier's review failed"
),
None => {}
}
}
/// The PR's head, when it was read, and how far the run got.
async fn classify(
files: &impl FileSource,
pulls: &impl PullSource,
sink: &impl RunSink,
req: &Requested,
) -> (Option<String>, Outcome) {
let repo = format!("{}/{}", req.owner, req.name);
let pull = match forge::within_forge_deadline(
"review classifier PR read",
pulls.pull(&req.owner, &req.name, req.pr),
)
.await
{
Some(Ok(pull)) => pull,
Some(Err(e)) => {
return (
None,
Outcome::NotRun(format!("reading the PR failed: {e:#}")),
);
}
None => {
return (
None,
Outcome::NotRun("the forge did not answer within the deadline".to_owned()),
);
}
};
let head = Some(pull.head_sha.clone());
let rules = match super::rules(files, &req.owner, &req.name).await {
Ok(axum::Json(rules)) => rules,
Err(problem) => {
let detail = problem.detail.unwrap_or_default();
return (
head,
Outcome::NotRun(format!("the rules did not load: {detail}")),
);
}
};
let request = TryOut {
operator: req.requester.clone(),
repo,
pr: req.pr.unsigned_abs(),
head_sha: pull.head_sha,
commits: pull.commits,
diff: pull.diff,
classifications: rules.classifications,
};
let outcome = match serde_json::to_vec(&request) {
Ok(payload) => Outcome::Ran(sink.run(payload).await),
Err(e) => Outcome::NotRun(format!("encoding the run failed: {e}")),
};
(head, outcome)
}
/// The review a run's outcome posts.
fn review_for(outcome: &Outcome) -> Review {
let (event, body) = match outcome {
Outcome::NotRun(reason) => (no_clean_verdict_event(), no_verdict(reason, &[])),
Outcome::Ran(Reply::Failed { error }) => (
no_clean_verdict_event(),
no_verdict(&format!("the run failed: {error}"), &[]),
),
Outcome::Ran(Reply::Done { rows, overall }) => {
let unanswered = rows.iter().filter(|r| r.error.is_some()).count();
if unanswered > 0 {
let reason = format!(
"{unanswered} of {} classifications got no answer",
rows.len()
);
(no_clean_verdict_event(), no_verdict(&reason, rows))
} else {
let (event, said) = match overall {
Overall::Approve => (Event::Approve, "approve"),
Overall::RequestChanges => (Event::RequestChanges, "request changes"),
};
(event, format!("{}\n**Overall: {said}.**\n", table(rows)))
}
}
};
Review {
event: event.forge(),
body,
}
}
/// A review body for a run without a clean verdict.
fn no_verdict(reason: &str, rows: &[Row]) -> String {
let mut body = format!(
"**No verdict:** {}. This review doesn't approve or request changes.\n",
cell(reason)
);
if !rows.is_empty() {
body.push('\n');
body.push_str(&table(rows));
}
body
}
/// The result table, one row per classification.
fn table(rows: &[Row]) -> String {
let mut out = String::from(
"| classification | polarity | verdict | certainty | threshold | counted | reason |\n\
| --- | --- | --- | --- | --- | --- | --- |\n",
);
for r in rows {
let polarity = match r.polarity {
swarm_queue_client::review_classifier::Polarity::Positive => "positive",
swarm_queue_client::review_classifier::Polarity::Negative => "negative",
};
let (verdict, certainty, counted, reason) = if let Some(error) = &r.error {
("no answer", String::new(), "no", error.as_str())
} else {
let verdict = match r.verdict {
Verdict::Match => "match",
Verdict::NoMatch => "no match",
Verdict::NotApplicable => "not applicable",
};
let counted = match (r.counted, r.passed) {
(false, _) => "no",
(true, true) => "yes ✅",
(true, false) => "yes ❌",
};
(verdict, r.certainty.to_string(), counted, r.reason.as_str())
};
let _ = writeln!(
out,
"| {} | {polarity} | {verdict} | {certainty} | {} | {counted} | {} |",
cell(&r.name),
r.threshold,
cell(reason)
);
}
out
}
/// `text` as one table cell's inline text: on one line, with every character
/// that could end the cell, start markup or raw HTML escaped, and every `@`
/// followed by a zero-width space, so no name after it is a live mention.
fn cell(text: &str) -> String {
let mut out = String::with_capacity(text.len());
for (i, word) in text.split_whitespace().enumerate() {
if i > 0 {
out.push(' ');
}
for c in word.chars() {
match c {
'\\' | '|' | '`' | '*' | '_' | '~' | '[' | ']' | '(' | ')' | '!' | '<' | '>'
| '&' | '#' => {
out.push('\\');
out.push(c);
}
'@' => out.push_str("@\u{200b}"),
c => out.push(c),
}
}
}
out
}
#[cfg(test)]
mod tests {
use std::sync::Mutex;
use swarm_queue_client::review_classifier::Polarity;
use super::*;
use crate::review_classifier::tests::{Fake, rules_json};
/// Every review [`run`] posted, with the commit it named.
#[derive(Default)]
struct Posted(Mutex<Vec<(String, Option<String>, Review)>>);
impl ReviewPoster for Posted {
async fn post(
&self,
owner: &str,
repo: &str,
number: i64,
commit: Option<&str>,
review: Review,
) -> anyhow::Result<()> {
self.0.lock().expect("unpoisoned").push((
format!("{owner}/{repo}#{number}"),
commit.map(str::to_owned),
review,
));
Ok(())
}
}
fn row(error: Option<&str>, counted: bool, passed: bool) -> Row {
Row {
name: "datetime".to_owned(),
polarity: Polarity::Positive,
verdict: if error.is_some() {
Verdict::NotApplicable
} else {
Verdict::Match
},
certainty: if error.is_some() { 0 } else { 90 },
threshold: 80,
counted,
passed,
reason: String::new(),
error: error.map(str::to_owned),
}
}
fn request(name: &str) -> Requested {
Requested {
owner: "o".to_owned(),
name: name.to_owned(),
pr: 7,
requester: "alice".to_owned(),
}
}
/// What one run against the fake forge posts.
async fn posted(reply: Reply, repo: &str) -> (String, Option<String>, Review) {
let fake = Fake::new(reply, vec![rules_json()]);
let poster = Posted::default();
run(&fake, &fake, &fake, &poster, &request(repo)).await;
let mut all = poster.0.into_inner().expect("unpoisoned");
assert_eq!(all.len(), 1, "one review per run");
all.remove(0)
}
#[tokio::test]
async fn nine_timeouts_and_one_pass_do_not_approve() {
let mut rows: Vec<Row> = (0..9)
.map(|_| row(Some("timed out after 3 minutes"), false, false))
.collect();
rows.push(row(None, true, true));
// The classifier's own verdict for these rows is approve.
let reply = Reply::Done {
rows,
overall: Overall::Approve,
};
let (_, commit, review) = posted(reply, "r").await;
assert_eq!(review.event, "COMMENT");
assert!(
review
.body
.starts_with("**No verdict:** 9 of 10 classifications got no answer."),
"{}",
review.body
);
assert!(review.body.contains("timed out after 3 minutes"));
assert_eq!(commit.as_deref(), Some("abc123"));
}
#[tokio::test]
async fn a_clean_run_posts_its_verdict_on_the_head_commit() {
let (target, commit, review) = posted(
Reply::Done {
rows: vec![row(None, true, true)],
overall: Overall::Approve,
},
"r",
)
.await;
assert_eq!(target, "o/r#7");
assert_eq!(commit.as_deref(), Some("abc123"));
assert_eq!(review.event, "APPROVED");
assert!(
review.body.ends_with("**Overall: approve.**\n"),
"{}",
review.body
);
let (_, _, review) = posted(
Reply::Done {
rows: vec![row(None, true, false)],
overall: Overall::RequestChanges,
},
"r",
)
.await;
assert_eq!(review.event, "REQUEST_CHANGES");
assert!(review.body.contains("| yes ❌ |"), "{}", review.body);
}
#[tokio::test]
async fn no_rules_a_failed_run_or_no_pr_is_a_comment() {
let approve = || Reply::Done {
rows: vec![row(None, true, true)],
overall: Overall::Approve,
};
// `o/elsewhere` has no PR 7.
for (reply, repo, says, commit) in [
(
Reply::Failed {
error: "no API key".to_owned(),
},
"r",
"the run failed: no API key",
Some("abc123"),
),
(approve(), "elsewhere", "reading the PR failed", None),
] {
let (_, posted_commit, review) = posted(reply, repo).await;
assert_eq!(review.event, "COMMENT", "{repo}");
assert!(review.body.contains(says), "{repo}: {}", review.body);
assert_eq!(posted_commit.as_deref(), commit, "{repo}");
}
let fake = Fake::new(approve(), Vec::new());
let poster = Posted::default();
run(&fake, &fake, &fake, &poster, &request("r")).await;
let (_, _, review) = poster.0.into_inner().expect("unpoisoned").remove(0);
assert_eq!(review.event, "COMMENT");
assert!(
review.body.contains("the rules did not load") && review.body.contains("neither"),
"{}",
review.body
);
}
#[test]
fn a_hostile_reason_mentions_nobody_and_stays_in_its_cell() {
let mut r = row(None, true, true);
r.reason = "ok | @mara please merge\n| x | ![](http://e/p.png) <b>`".to_owned();
let body = table(&[r]);
let line = body.lines().nth(2).expect("a data row");
assert_eq!(body.lines().count(), 3, "{body}");
assert!(!line.contains("@mara"), "{line}");
assert!(line.contains("@\u{200b}mara"), "{line}");
// Seven cells: eight unescaped pipes.
let unescaped = line
.char_indices()
.filter(|(i, c)| *c == '|' && (*i == 0 || line.as_bytes()[i - 1] != b'\\'))
.count();
assert_eq!(unescaped, 8, "{line}");
assert!(
line.contains("\\!\\[\\]") && line.contains("\\<b\\>"),
"{line}"
);
}
#[test]
fn only_a_request_for_the_classifier_parses() {
let payload = |action: &str, reviewer: Option<&str>| {
serde_json::json!({
"action": action,
"number": 7,
"requested_reviewer": reviewer.map(|l| serde_json::json!({ "login": l })),
"repository": { "full_name": "o/r" },
"sender": { "login": "alice" },
})
.to_string()
};
assert_eq!(
Requested::parse(payload("review_requested", Some(LOGIN)).as_bytes()),
Some(request("r"))
);
assert_eq!(
Requested::parse(payload("review_requested", Some("Review-Classifier")).as_bytes()),
Some(request("r")),
"the forge's logins are case-insensitive"
);
for body in [
payload("review_request_removed", Some(LOGIN)),
payload("review_requested", Some("argus")),
payload("review_requested", None),
"{}".to_owned(),
] {
assert_eq!(Requested::parse(body.as_bytes()), None, "{body}");
}
}
}