From 4d8d43d6771411b727c52a6833ab3e0a8c8ffe63 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 2 Nov 2026 18:45:36 +0100 Subject: [PATCH] review-classifier: a requested review runs even on a draft pr --- docs/scheduler/observability.md | 14 +++---- docs/swarm/review-classifier.md | 8 ++-- swarm-controller/src/forge.rs | 5 --- swarm-controller/src/review_classifier.rs | 1 - .../src/review_classifier/metrics.rs | 3 -- .../src/review_classifier/review.rs | 37 ------------------- 6 files changed, 11 insertions(+), 57 deletions(-) diff --git a/docs/scheduler/observability.md b/docs/scheduler/observability.md index b3980b29..70437f48 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`, `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` | +| 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` | 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/review-classifier.md b/docs/swarm/review-classifier.md index 4c8e525e..e60810ac 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -92,10 +92,10 @@ 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)). 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. +requests the review once, for the head the PR has then. That edit recognises +only the default `WIP:` and `[WIP]` title prefixes: after taking a PR out of +draft that used another prefix, request the review by hand. A request that +reaches the controller is always reviewed, whether or not the PR is a draft. 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 9bed95fa..2615003c 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -157,9 +157,6 @@ 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 @@ -1385,7 +1382,6 @@ 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) @@ -1447,7 +1443,6 @@ impl Client { commits, diff, narrowed, - draft, }) } diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index fa14ee57..d971ee2e 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -870,7 +870,6 @@ 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 5b7695ed..7162c16b 100644 --- a/swarm-controller/src/review_classifier/metrics.rs +++ b/swarm-controller/src/review_classifier/metrics.rs @@ -32,8 +32,6 @@ 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 { @@ -45,7 +43,6 @@ 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 03291de7..0ab2cc16 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/swarm-controller/src/review_classifier/review.rs @@ -678,16 +678,6 @@ 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!( @@ -1661,7 +1651,6 @@ mod tests { struct Heads { fake: Fake, head_of: Mutex>, - drafts: Mutex>, } impl Heads { @@ -1674,14 +1663,9 @@ mod tests { vec![rules_json()], ), 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.head_of .lock() @@ -1709,7 +1693,6 @@ mod tests { commits: Vec::new(), diff: String::new(), narrowed: None, - draft: self.drafts.lock().expect("unpoisoned").contains(&number), }) } } @@ -2041,26 +2024,6 @@ 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;