vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

swarm-controller: record the review classifier's runs and list them

Browse source 
Every run is recorded when it ends: a try-out when its reply settles it,
a requested review once its review is posted or the post fails, and a
request turned away by a full queue once its comment is posted. A
duplicate request that starts nothing is not recorded. The newest 100
are one JSON array under `recent` in a new controller-owned KV bucket,
`review-classifier-runs`, written with compare-and-set as
`schedule-definitions` is (five attempts). A failed write is logged; the
review is already posted by then.

Each recorded run is also one info event, "review classifier run
finished", with the record's fields as tracing fields. The controller
logs natively to the journal, so they reach VictoriaLogs as F_<NAME>.

GET /api/review-classifier/runs returns the list, newest first, gated
by the gateway like the try-out GET. post_classifier_review now returns
the posted review's html_url, which the record keeps as review_url.

Closes #5153, part of #5096.
This commit is contained in:
atlas 2026-10-12 19:23:28 +02:00
commit 2fc0166427
14 changed files with 1138 additions and 66 deletions
Download patch file Download diff file Expand all files Collapse all files

443
swarm-controller/src/review_classifier/review.rs
View file

@ -22,8 +22,10 @@ use swarm_queue_client::review_classifier::{Overall, Reply, Row, TryOut, Verdict
use tokio::sync::Semaphore;
use tokio::task::JoinHandle;
use super::runs::{self, RunOutcome, RunRecord, Runs, Trigger};
use super::{FileSource, PullSource, RunSink, split_repo};
use crate::AppState;
use crate::agent_renewal::unix_now;
use crate::forge::{self, classifier_account::LOGIN, classifier_account::Review};
/// The most review runs in flight at once.
@ -215,16 +217,26 @@ pub(crate) fn on_delivery(state: &AppState, body: &[u8]) {
return;
};
let queue = Queue(state.status.as_ref().map(|s| s.queue_client()));
start(&state.reviews, Arc::clone(&forge), queue, forge, request);
let runs = state.runs.clone();
start(
&state.reviews,
Arc::clone(&forge),
queue,
forge,
runs,
request,
);
}
/// Accept `request` into `reviews` and spawn its run, or the no-verdict
/// comment when there's no place for it. The task, if one was spawned.
/// comment when there's no place for it, and record it in `runs` when it
/// ends. The task, if one was spawned.
fn start<S, Q, P>(
reviews: &Arc<Reviews>,
forge: Arc<S>,
queue: Q,
poster: Arc<P>,
runs: Runs,
request: Requested,
) -> Option<JoinHandle<()>>
where
@ -240,8 +252,9 @@ where
let Ok(_slot) = slots.acquire_owned().await else {
return;
};
Box::pin(run(&*forge, &*forge, &queue, &*poster, &request)).await;
let run = Box::pin(run(&*forge, &*forge, &queue, &*poster, &request)).await;
drop(accepted);
runs.record(run).await;
}))
}
Err(Refused::Duplicate) => {
@ -257,28 +270,34 @@ where
"review requested, but every run and wait place is taken; posting a no-verdict comment"
);
Some(tokio::spawn(async move {
post(&*poster, &request, None, queue_full_review()).await;
let started_unix = unix_now();
let ruling = queue_full_ruling();
let reason = ruling.reason.clone();
let sent = post(&*poster, &request, None, ruling.review()).await;
let ended = ended(RunOutcome::QueueFull, reason, sent);
runs.record(record(&request, None, started_unix, ended, (None, None)))
.await;
}))
}
}
}
/// The review for a request [`Reviews`] had no place for.
fn queue_full_review() -> Review {
Review {
event: no_clean_verdict_event().forge(),
body: no_verdict(
&format!(
"{MAX_CONCURRENT_REVIEWS} reviews are running and {MAX_WAITING_REVIEWS} are \
waiting, so this request starts no run; request the review again later"
),
&[],
),
/// The ruling for a request [`Reviews`] had no place for.
fn queue_full_ruling() -> Ruling {
let reason = format!(
"{MAX_CONCURRENT_REVIEWS} reviews are running and {MAX_WAITING_REVIEWS} are waiting, so \
this request starts no run; request the review again later"
);
Ruling {
event: no_clean_verdict_event(),
body: no_verdict(&reason, &[]),
reason: Some(reason),
}
}
/// Where a review is posted: the forge in production, a fake in the tests.
pub(crate) trait ReviewPoster {
/// The posted review's URL, when the forge names one.
fn post(
&self,
owner: &str,
@ -286,7 +305,7 @@ pub(crate) trait ReviewPoster {
number: i64,
commit: Option<&str>,
review: Review,
) -> impl Future<Output = anyhow::Result<()>> + Send;
) -> impl Future<Output = anyhow::Result<Option<String>>> + Send;
}
impl ReviewPoster for forge::Client {
@ -297,7 +316,7 @@ impl ReviewPoster for forge::Client {
number: i64,
commit: Option<&str>,
review: Review,
) -> anyhow::Result<()> {
) -> anyhow::Result<Option<String>> {
self.post_classifier_review(owner, repo, number, commit, review)
.await
}
@ -332,20 +351,46 @@ enum Outcome {
Ran { sent: usize, reply: Reply },
}
/// Read the PR and the rules, run them, and post the review.
/// Read the PR and the rules, run them, and post the review. The finished
/// run.
async fn run(
files: &impl FileSource,
pulls: &impl PullSource,
sink: &impl RunSink,
poster: &impl ReviewPoster,
req: &Requested,
) {
) -> RunRecord {
let started_unix = unix_now();
let (head, outcome) = classify(files, pulls, sink, req).await;
post(poster, req, head.as_deref(), review_for(&outcome)).await;
let ruling = review_for(&outcome);
let posted_as = match ruling.event {
Event::Approve => RunOutcome::Approved,
Event::RequestChanges => RunOutcome::ChangesRequested,
Event::Comment => RunOutcome::NoVerdict,
};
let reason = ruling.reason.clone();
let sent = post(poster, req, head.as_deref(), ruling.review()).await;
let answer = match &outcome {
Outcome::Ran { reply, .. } => runs::answer(reply),
Outcome::NotRun(_) => (None, None),
};
record(
req,
head,
started_unix,
ended(posted_as, reason, sent),
answer,
)
}
/// Post `review` on `req`'s PR, naming `head` when there is one.
async fn post(poster: &impl ReviewPoster, req: &Requested, head: Option<&str>, review: Review) {
/// Post `review` on `req`'s PR, naming `head` when there is one. The posted
/// review's URL when the forge names one, or why the post failed.
async fn post(
poster: &impl ReviewPoster,
req: &Requested,
head: Option<&str>,
review: Review,
) -> Result<Option<String>, String> {
let repo = format!("{}/{}", req.owner, req.name);
let event = review.event;
let sent = forge::within_forge_deadline(
@ -354,15 +399,79 @@ async fn post(poster: &impl ReviewPoster, req: &Requested, head: Option<&str>, r
)
.await;
match sent {
Some(Ok(())) => tracing::info!(
%repo, pr = req.pr, head_sha = head.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 => {}
Some(Ok(url)) => {
tracing::info!(
%repo, pr = req.pr, head_sha = head.unwrap_or_default(), event,
"review classifier review posted"
);
Ok(url)
}
Some(Err(e)) => {
let error = format!("{e:#}");
tracing::warn!(
%repo, pr = req.pr, %error,
"posting the review classifier's review failed"
);
Err(format!("posting the review failed: {error}"))
}
None => Err("the forge did not answer the review's post within the deadline".to_owned()),
}
}
/// How a run ended, as its record holds it.
struct Ended {
outcome: RunOutcome,
detail: Option<String>,
review_url: Option<String>,
}
/// `posted_as` when the post went through, `post-failed` otherwise. `reason`
/// is why the run has no clean verdict, if it has none.
fn ended(
posted_as: RunOutcome,
reason: Option<String>,
posted: Result<Option<String>, String>,
) -> Ended {
match posted {
Ok(review_url) => Ended {
outcome: posted_as,
detail: reason,
review_url,
},
Err(error) => Ended {
outcome: RunOutcome::PostFailed,
detail: Some(match reason {
Some(reason) => format!("{reason}; {error}"),
None => error,
}),
review_url: None,
},
}
}
/// The record of a run on `req` that read `head`.
fn record(
req: &Requested,
head: Option<String>,
started_unix: i64,
ended: Ended,
(rows, overall): (Option<Vec<Row>>, Option<Overall>),
) -> RunRecord {
let repo = format!("{}/{}", req.owner, req.name);
RunRecord {
id: super::mint_id().unwrap_or_else(|_| format!("{repo}#{}@{started_unix}", req.pr)),
trigger: Trigger::Review,
repo,
pr: req.pr.unsigned_abs(),
requester: req.requester.clone(),
head_sha: head,
started_unix,
finished_unix: unix_now(),
outcome: ended.outcome,
detail: ended.detail,
rows,
overall,
review_url: ended.review_url,
}
}
@ -425,17 +534,39 @@ async fn classify(
(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, &[])),
/// What a run posts: the event, the review body, and why it has no clean
/// verdict when it has none.
struct Ruling {
event: Event,
body: String,
reason: Option<String>,
}
impl Ruling {
fn review(self) -> Review {
Review {
event: self.event.forge(),
body: self.body,
}
}
fn no_verdict(reason: String, rows: &[Row]) -> Self {
Self {
event: no_clean_verdict_event(),
body: no_verdict(&reason, rows),
reason: Some(reason),
}
}
}
/// The ruling on a run's outcome.
fn review_for(outcome: &Outcome) -> Ruling {
match outcome {
Outcome::NotRun(reason) => Ruling::no_verdict(reason.clone(), &[]),
Outcome::Ran {
reply: Reply::Failed { error },
..
} => (
no_clean_verdict_event(),
no_verdict(&format!("the run failed: {error}"), &[]),
),
} => Ruling::no_verdict(format!("the run failed: {error}"), &[]),
Outcome::Ran {
sent,
reply: Reply::Done { rows, overall },
@ -446,25 +577,25 @@ fn review_for(outcome: &Outcome) -> Review {
"the classifier answered {} of {sent} classifications",
rows.len()
);
(no_clean_verdict_event(), no_verdict(&reason, rows))
Ruling::no_verdict(reason, rows)
} else if unanswered > 0 {
let reason = format!(
"{unanswered} of {} classifications got no answer",
rows.len()
);
(no_clean_verdict_event(), no_verdict(&reason, rows))
Ruling::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)))
Ruling {
event,
body: format!("{}\n**Overall: {said}.**\n", table(rows)),
reason: None,
}
}
}
};
Review {
event: event.forge(),
body,
}
}
@ -560,6 +691,7 @@ mod tests {
use swarm_queue_client::review_classifier::Polarity;
use super::*;
use crate::review_classifier::runs::tests::FakeDoc;
use crate::review_classifier::tests::{Fake, rules_json};
/// Every review [`run`] posted, with the commit it named.
@ -574,13 +706,15 @@ mod tests {
number: i64,
commit: Option<&str>,
review: Review,
) -> anyhow::Result<()> {
) -> anyhow::Result<Option<String>> {
self.0.lock().expect("unpoisoned").push((
format!("{owner}/{repo}#{number}"),
commit.map(str::to_owned),
review,
));
Ok(())
Ok(Some(format!(
"https://forge/{owner}/{repo}/pulls/{number}#review"
)))
}
}
@ -694,14 +828,14 @@ mod tests {
// A run of no classifications never reaches the classifier; an
// empty answer to one still has no verdict.
let review = review_for(&Outcome::Ran {
let ruling = review_for(&Outcome::Ran {
sent: 0,
reply: Reply::Done {
rows: Vec::new(),
overall: Overall::Approve,
},
});
assert_eq!(review.event, "COMMENT");
assert_eq!(ruling.event, Event::Comment);
}
/// A classifier that answers approve once [`Gate::open`] lets it through.
@ -742,12 +876,15 @@ mod tests {
));
let gate = Gate(Arc::new(tokio::sync::Semaphore::new(0)));
let poster = Arc::new(Posted::default());
let doc = Arc::new(FakeDoc::default());
let store = Runs::Fake(Arc::clone(&doc));
let start_at = |head: &str| {
start(
&reviews,
Arc::clone(&fake),
gate.clone(),
Arc::clone(&poster),
store.clone(),
requested_at(head),
)
};
@ -764,10 +901,16 @@ mod tests {
task.await.expect("ran");
}
assert_eq!(poster.0.lock().expect("unpoisoned").len(), 3);
assert_eq!(
doc.runs().len(),
3,
"a repeat that started nothing is no run"
);
// A finished run frees its head.
start_at("a").expect("accepted again").await.expect("ran");
assert_eq!(poster.0.lock().expect("unpoisoned").len(), 4);
assert_eq!(doc.runs().len(), 4);
}
#[tokio::test]
@ -781,12 +924,15 @@ mod tests {
));
let gate = Gate(Arc::new(tokio::sync::Semaphore::new(0)));
let poster = Arc::new(Posted::default());
let doc = Arc::new(FakeDoc::default());
let store = Runs::Fake(Arc::clone(&doc));
let start_at = |head: String| {
start(
&reviews,
Arc::clone(&fake),
gate.clone(),
Arc::clone(&poster),
store.clone(),
requested_at(&head),
)
.expect("a run or a comment")
@ -813,6 +959,22 @@ mod tests {
review.body
);
}
let refused = doc.runs();
assert_eq!(refused.len(), 1);
assert_eq!(refused[0].outcome, RunOutcome::QueueFull);
assert_eq!(refused[0].head_sha, None);
assert_eq!(
refused[0].review_url.as_deref(),
Some("https://forge/o/r/pulls/7#review")
);
assert!(
refused[0]
.detail
.as_deref()
.is_some_and(|d| d.starts_with("2 reviews are running and 8 are waiting")),
"{:?}",
refused[0].detail
);
gate.open();
for run in runs {
@ -821,6 +983,187 @@ mod tests {
let all = poster.0.lock().expect("unpoisoned");
assert_eq!(all.len(), places + 1);
assert!(all[1..].iter().all(|(_, _, r)| r.event == "APPROVED"));
let recorded = doc.runs();
assert_eq!(recorded.len(), places + 1);
assert!(
recorded[..places]
.iter()
.all(|r| r.outcome == RunOutcome::Approved)
);
}
/// The record of one run against the fake forge, holding `rules`, posted
/// by `poster`.
async fn recorded_for(
reply: Reply,
repo: &str,
rules: (&'static str, String),
poster: &impl ReviewPoster,
) -> RunRecord {
let fake = Fake::new(reply, vec![rules]);
run(&fake, &fake, &fake, poster, &request(repo)).await
}
#[tokio::test]
async fn each_way_a_run_ends_has_its_outcome() {
let answered = |rows: Vec<Row>, overall| Reply::Done { rows, overall };
let failed = Reply::Failed {
error: "no API key".to_owned(),
};
for (case, reply, repo, rules, outcome, detail) in [
(
"approve",
answered(vec![row(None, true, true)], Overall::Approve),
"r",
rules_json(),
RunOutcome::Approved,
None,
),
(
"request changes",
answered(vec![row(None, true, false)], Overall::RequestChanges),
"r",
rules_json(),
RunOutcome::ChangesRequested,
None,
),
(
"a failed run",
failed.clone(),
"r",
rules_json(),
RunOutcome::NoVerdict,
Some("the run failed: no API key"),
),
(
"a short answer",
answered(vec![row(None, true, true)], Overall::Approve),
"r",
rules_of(2),
RunOutcome::NoVerdict,
Some("the classifier answered 1 of 2 classifications"),
),
(
"an unanswered classification",
answered(vec![row(Some("timed out"), false, false)], Overall::Approve),
"r",
rules_json(),
RunOutcome::NoVerdict,
Some("1 of 1 classifications got no answer"),
),
(
"an unreadable PR",
answered(vec![row(None, true, true)], Overall::Approve),
"elsewhere",
rules_json(),
RunOutcome::NoVerdict,
Some("reading the PR failed"),
),
] {
let run = recorded_for(reply.clone(), repo, rules, &Posted::default()).await;
assert_eq!(run.outcome, outcome, "{case}");
assert_eq!(run.trigger, Trigger::Review, "{case}");
assert_eq!(run.requester, "alice", "{case}");
match detail {
None => assert_eq!(run.detail, None, "{case}"),
Some(want) => assert!(
run.detail.as_deref().is_some_and(|d| d.starts_with(want)),
"{case}: {:?}",
run.detail
),
}
assert_eq!(
run.review_url.as_deref(),
Some(format!("https://forge/o/{repo}/pulls/7#review").as_str()),
"{case}"
);
if repo == "elsewhere" {
assert_eq!(run.head_sha, None, "{case}");
assert_eq!((run.rows, run.overall), (None, None), "{case}");
} else {
assert_eq!(run.head_sha.as_deref(), Some("abc123"), "{case}");
assert_eq!((run.rows, run.overall), runs::answer(&reply), "{case}");
}
}
let fake = Fake::new(failed, Vec::new());
let run = run(&fake, &fake, &fake, &Posted::default(), &request("r")).await;
assert_eq!(run.outcome, RunOutcome::NoVerdict);
assert!(
run.detail
.as_deref()
.is_some_and(|d| d.starts_with("the rules did not load")),
"{:?}",
run.detail
);
}
/// A forge that refuses every review.
struct Refusing;
impl ReviewPoster for Refusing {
async fn post(
&self,
_owner: &str,
_repo: &str,
_number: i64,
_commit: Option<&str>,
_review: Review,
) -> anyhow::Result<Option<String>> {
anyhow::bail!("the forge answered 500")
}
}
#[tokio::test]
async fn a_failed_post_is_post_failed_with_its_reason() {
let approve = Reply::Done {
rows: vec![row(None, true, true)],
overall: Overall::Approve,
};
let run = recorded_for(approve, "r", rules_json(), &Refusing).await;
assert_eq!(run.outcome, RunOutcome::PostFailed);
assert_eq!(
run.detail.as_deref(),
Some("posting the review failed: the forge answered 500")
);
assert_eq!(run.review_url, None);
let failed = Reply::Failed {
error: "no API key".to_owned(),
};
let run = recorded_for(failed, "r", rules_json(), &Refusing).await;
assert_eq!(run.outcome, RunOutcome::PostFailed);
assert_eq!(
run.detail.as_deref(),
Some("the run failed: no API key; posting the review failed: the forge answered 500")
);
}
#[tokio::test]
async fn a_failed_record_write_still_posts_the_review() {
let fake = Arc::new(Fake::new(
Reply::Done {
rows: vec![row(None, true, true)],
overall: Overall::Approve,
},
vec![rules_json()],
));
let poster = Arc::new(Posted::default());
let store = Runs::Fake(Arc::new(FakeDoc::failing()));
start(
&Arc::new(Reviews::default()),
Arc::clone(&fake),
(*fake).clone(),
Arc::clone(&poster),
store,
request("r"),
)
.expect("accepted")
.await
.expect("ran to the end");
let all = poster.0.lock().expect("unpoisoned");
assert_eq!(all.len(), 1);
assert_eq!(all[0].2.event, "APPROVED");
}
#[tokio::test]