From cf5746188a20798f134b067fa3a6f64f061c0f8b Mon Sep 17 00:00:00 2001 From: iris Date: Wed, 11 Nov 2026 00:55:46 +0100 Subject: [PATCH] review classifier: a setting makes the PR review list only the failed rules failures_only (off by default, also for a stored document without it) makes the review the classifier posts on a PR list just the rows that failed their rule, or say that no check failed on an approval. A review with no clean verdict and a try-out keep every row. The classifier settings form gets a checkbox for it. --- docs/2-handbook/swarm/ui.md | 5 +- .../swarm/review-classifier.md | 6 ++ .../review-settings/ClassifierSettings.tsx | 7 ++ .../pages/review-settings/SettingsTypes.ts | 7 ++ .../swarm-ui/test/settings-types.test.mjs | 10 +++ swarm-controller/src/review_classifier.rs | 1 + .../src/review_classifier/review.rs | 85 ++++++++++++++++++- .../src/review_classifier/settings.rs | 26 ++++-- 8 files changed, 138 insertions(+), 9 deletions(-) diff --git a/docs/2-handbook/swarm/ui.md b/docs/2-handbook/swarm/ui.md index 69a29debb..b78b03ebd 100644 --- a/docs/2-handbook/swarm/ui.md +++ b/docs/2-handbook/swarm/ui.md @@ -297,7 +297,10 @@ a repo admin adds it as a collaborator with write access The **classifier settings** section shows the runtime, model and effort the [review classifier](../../4-implementation/swarm/review-classifier.md) runs with and how many -requested reviews run at once, and lets you change them. The runtime is **API +requested reviews run at once, and lets you change them. The **PR reviews list +only failed rules** checkbox makes the review the classifier posts on a PR leave +out the rules that passed or didn't apply; try-outs always show every rule. The +runtime is **API key** (claude on the Anthropic key) or **ACP** (opencode on a provider). On the API key runtime the model is free text, one word as claude's `--model` takes it, such as `haiku` or `sonnet`. Choosing ACP shows the provider's fields in diff --git a/docs/4-implementation/swarm/review-classifier.md b/docs/4-implementation/swarm/review-classifier.md index 170dc94de..182718cf0 100644 --- a/docs/4-implementation/swarm/review-classifier.md +++ b/docs/4-implementation/swarm/review-classifier.md @@ -216,6 +216,7 @@ replaces it. "effort": null, "max_concurrent_reviews": 2, "max_classifications": 10, + "failures_only": false, "runtime": "api-key", "acp": null } @@ -238,6 +239,11 @@ replaces it. three minutes, so the controller waits a minute plus three per classification for the reply (31 minutes for ten), and the classifier may answer for as long as the biggest run the limit allows +- `failures_only`: a boolean, `false` by default (also for a document stored + before the field existed). When `true`, a posted PR review lists only the + rows that failed their rule, the ones that make it ask for changes, and an + approval says `No check failed.` in place of the table. A review with no + verdict, and a try-out, always show every row - `runtime`: `api-key` or `acp` (see [Runtimes](#runtimes)) - `acp`: the provider the ACP runtime runs on, or `null`. It stays stored while the runtime is `api-key`, so switching back and forth needs no diff --git a/frontend/packages/swarm-ui/src/pages/review-settings/ClassifierSettings.tsx b/frontend/packages/swarm-ui/src/pages/review-settings/ClassifierSettings.tsx index dc2ffe4e0..3b95934b1 100644 --- a/frontend/packages/swarm-ui/src/pages/review-settings/ClassifierSettings.tsx +++ b/frontend/packages/swarm-ui/src/pages/review-settings/ClassifierSettings.tsx @@ -15,6 +15,7 @@ import { Button } from "../../ui/button/Button.js"; import { FieldGrid, FieldGridFull } from "../../ui/field-grid/FieldGrid.js"; import { Hint } from "../../ui/hint/Hint.js"; import { Disclosure } from "../../ui/disclosure/Disclosure.js"; +import { CheckboxField } from "../../ui/checkbox-field/CheckboxField.js"; import { SelectField } from "../../ui/select-field/SelectField.js"; import { TextField } from "../../ui/text-field/TextField.js"; import { @@ -187,6 +188,12 @@ export function ClassifierSettings() { pattern="[0-9]+" onInput={(maxRules) => edit({ ...draft, maxRules })} /> + edit({ ...draft, failuresOnly })} + /> {draft.runtime === "acp" ? (
diff --git a/frontend/packages/swarm-ui/src/pages/review-settings/SettingsTypes.ts b/frontend/packages/swarm-ui/src/pages/review-settings/SettingsTypes.ts index e874be27c..418d97b70 100644 --- a/frontend/packages/swarm-ui/src/pages/review-settings/SettingsTypes.ts +++ b/frontend/packages/swarm-ui/src/pages/review-settings/SettingsTypes.ts @@ -35,6 +35,8 @@ export interface ClassifierSettingsBody { max_concurrent_reviews: number; /** The most classifications (rules) one run carries. */ max_classifications: number; + /** Whether a posted PR review lists only the rules that failed. */ + failures_only: boolean; runtime: Runtime; /** The provider record. It stays stored while the runtime is `api-key`. */ acp: AcpSettings | null; @@ -48,6 +50,8 @@ export interface SettingsDraft { max: string; /** The most classifications one run carries. */ maxRules: string; + /** Whether a posted PR review lists only the failed rules. */ + failuresOnly: boolean; runtime: Runtime; acpUrl: string; acpModel: string; @@ -70,6 +74,7 @@ export function draftOf(s: ClassifierSettingsBody): SettingsDraft { effort: s.effort ?? "", max: String(s.max_concurrent_reviews), maxRules: String(s.max_classifications), + failuresOnly: s.failures_only, runtime: s.runtime, acpUrl: acp?.base_url ?? "", acpModel: acp?.model ?? "", @@ -173,6 +178,7 @@ export function toSettings( effort: d.effort === "" ? null : (d.effort as Effort), max_concurrent_reviews: maxValue(d.max) ?? MIN_REVIEWS, max_classifications: rulesValue(d.maxRules) ?? MIN_RULES, + failures_only: d.failuresOnly, runtime: d.runtime, }; return d.runtime === "acp" ? { ...body, acp: toAcp(d) } : body; @@ -190,6 +196,7 @@ export function changed( d.effort !== now.effort || d.max.trim() !== now.max || d.maxRules.trim() !== now.maxRules || + d.failuresOnly !== now.failuresOnly || d.runtime !== now.runtime; if (base) return true; // The provider fields only save on the ACP runtime. diff --git a/frontend/packages/swarm-ui/test/settings-types.test.mjs b/frontend/packages/swarm-ui/test/settings-types.test.mjs index 031d9f497..4b7f9da95 100644 --- a/frontend/packages/swarm-ui/test/settings-types.test.mjs +++ b/frontend/packages/swarm-ui/test/settings-types.test.mjs @@ -20,6 +20,7 @@ const apiKey = { effort: null, max_concurrent_reviews: 2, max_classifications: 10, + failures_only: false, runtime: "api-key", acp: null, }; @@ -60,6 +61,7 @@ test("the API key request leaves the provider out, so the stored one stays", () effort: null, max_concurrent_reviews: 2, max_classifications: 10, + failures_only: false, runtime: "api-key", }); assert.equal("acp" in body, false); @@ -166,3 +168,11 @@ test("under ACP the summary names the provider's model, not the claude one", () "acp · glm · 2 at once · up to 10 rules", ); }); + +test("the failures-only switch round-trips and counts as a change", () => { + const on = { ...draftOf(apiKey), failuresOnly: true }; + assert.equal(draftOf(apiKey).failuresOnly, false); + assert.equal(toSettings(on).failures_only, true); + assert.equal(changed(on, apiKey), true); + assert.equal(changed(draftOf(apiKey), apiKey), false); +}); diff --git a/swarm-controller/src/review_classifier.rs b/swarm-controller/src/review_classifier.rs index 80436e96d..6fb81fb6d 100644 --- a/swarm-controller/src/review_classifier.rs +++ b/swarm-controller/src/review_classifier.rs @@ -1053,6 +1053,7 @@ pub(crate) mod tests { effort: Some(Effort::High), max_concurrent_reviews: 2, max_classifications: 10, + failures_only: false, runtime: settings::RuntimeKind::Acp, acp: Some(acp.clone()), }, diff --git a/swarm-controller/src/review_classifier/review.rs b/swarm-controller/src/review_classifier/review.rs index cde18cd0b..ead6226aa 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/swarm-controller/src/review_classifier/review.rs @@ -855,7 +855,12 @@ enum Outcome { /// Nothing reached the classifier, for this reason. NotRun(String), /// The classifier answered a run of `sent` classifications. - Ran { sent: usize, reply: Reply }, + Ran { + sent: usize, + reply: Reply, + /// Whether the review lists only the rules that failed. + failures_only: bool, + }, } /// Read the rules, run them on `pull`, the PR as it was read, and post the @@ -1106,6 +1111,7 @@ async fn classify( ); } }; + let failures_only = settings.failures_only; let request = TryOut { operator: req.requester.clone(), repo, @@ -1123,6 +1129,7 @@ async fn classify( Ok(payload) => Outcome::Ran { sent, reply: sink.run(payload, run_deadline(sent)).await, + failures_only, }, Err(e) => Outcome::NotRun(format!("encoding the run failed: {e}")), }; @@ -1170,6 +1177,7 @@ fn review_for(outcome: &Outcome) -> Ruling { Outcome::Ran { sent, reply: Reply::Done { rows, overall }, + failures_only, } => { let unanswered = rows.iter().filter(|r| r.error.is_some()).count(); if rows.is_empty() || rows.len() != *sent { @@ -1191,7 +1199,15 @@ fn review_for(outcome: &Outcome) -> Ruling { }; Ruling { event, - body: format!("{}\n{}", table(rows), overall_line(*overall)), + body: format!( + "{}\n{}", + if *failures_only { + failures(rows) + } else { + table(rows) + }, + overall_line(*overall) + ), reason: None, } } @@ -1235,6 +1251,21 @@ fn table(rows: &[Row]) -> String { out } +/// The result table of only the rows that failed their rule, the ones that +/// make the review ask for changes, or a line saying none did. +fn failures(rows: &[Row]) -> String { + let failed: Vec = rows + .iter() + .filter(|r| r.counted && !r.passed) + .cloned() + .collect(); + if failed.is_empty() { + "No check failed.\n".to_owned() + } else { + table(&failed) + } +} + /// A row's result cell: the symbol for what the row did to the review, then /// the model's verdict. ✅ is a counted row that passed its polarity, ❌ a /// counted row that failed it, ❔ a row that doesn't count: below its @@ -1456,6 +1487,54 @@ mod tests { ); } + fn done(failures_only: bool, rows: Vec, overall: Overall) -> Ruling { + review_for(&Outcome::Ran { + sent: rows.len(), + failures_only, + reply: Reply::Done { rows, overall }, + }) + } + + #[test] + fn failures_only_lists_just_the_rows_that_failed() { + let mut failed = row(None, true, false); + failed.name = "no-secrets".to_owned(); + let rows = vec![row(None, true, true), failed, row(None, false, true)]; + + let all = done(false, rows.clone(), Overall::RequestChanges); + assert!(all.body.contains("datetime"), "{}", all.body); + assert!(all.body.contains("no-secrets"), "{}", all.body); + + let only = done(true, rows, Overall::RequestChanges); + assert_eq!(only.event, Event::RequestChanges); + assert!(only.body.contains("no-secrets"), "{}", only.body); + assert!(!only.body.contains("datetime"), "{}", only.body); + assert!(only.body.ends_with("**Overall: ❌ request changes**\n")); + } + + #[test] + fn failures_only_on_an_approval_says_no_check_failed() { + let ruling = done( + true, + vec![row(None, true, true), row(None, false, true)], + Overall::Approve, + ); + assert_eq!(ruling.event, Event::Approve); + assert_eq!(ruling.body, "No check failed.\n\n**Overall: ✅ approve**\n"); + } + + #[test] + fn failures_only_keeps_the_full_table_when_a_row_got_no_answer() { + let ruling = done( + true, + vec![row(None, true, true), row(Some("timed out"), false, false)], + Overall::Approve, + ); + assert_eq!(ruling.event, Event::Comment); + assert!(ruling.body.contains("datetime"), "{}", ruling.body); + assert!(ruling.body.contains("timed out"), "{}", ruling.body); + } + fn request(name: &str) -> Requested { Requested { owner: "o".to_owned(), @@ -1568,6 +1647,7 @@ mod tests { // empty answer to one still has no verdict. let ruling = review_for(&Outcome::Ran { sent: 0, + failures_only: false, reply: Reply::Done { rows: Vec::new(), overall: Overall::Approve, @@ -2727,6 +2807,7 @@ mod tests { effort: Some(Effort::Medium), max_concurrent_reviews: 2, max_classifications: 10, + failures_only: false, runtime: super::super::settings::RuntimeKind::Acp, acp: Some(acp.clone()), }; diff --git a/swarm-controller/src/review_classifier/settings.rs b/swarm-controller/src/review_classifier/settings.rs index 8efa9b932..462e96198 100644 --- a/swarm-controller/src/review_classifier/settings.rs +++ b/swarm-controller/src/review_classifier/settings.rs @@ -64,6 +64,12 @@ pub(crate) struct Settings { /// minutes. A document stored before this field has the default. #[serde(default = "default_max_classifications")] pub(crate) max_classifications: usize, + /// Whether a posted PR review lists only the rules that failed, rather than + /// every rule with its result. A review with no failed rule says so in a + /// line. A try-out shows every rule either way. A document stored before this + /// field has `false`. + #[serde(default)] + pub(crate) failures_only: bool, /// What every classification runs on. #[serde(default)] pub(crate) runtime: RuntimeKind, @@ -91,6 +97,7 @@ impl Default for Settings { effort: None, max_concurrent_reviews: 2, max_classifications: DEFAULT_MAX_CLASSIFICATIONS, + failures_only: false, runtime: RuntimeKind::ApiKey, acp: None, } @@ -191,6 +198,9 @@ pub(crate) struct SettingsUpdate { #[schema(value_type = Option)] max_classifications: Option, #[serde(default, deserialize_with = "present")] + #[schema(value_type = Option)] + failures_only: Option, + #[serde(default, deserialize_with = "present")] #[schema(value_type = Option)] runtime: Option, #[serde(default)] @@ -244,6 +254,7 @@ impl SettingsUpdate { max_classifications: self .max_classifications .unwrap_or(stored.max_classifications), + failures_only: self.failures_only.unwrap_or(stored.failures_only), runtime: self.runtime.unwrap_or(stored.runtime), acp: self.acp.or(stored.acp), } @@ -277,6 +288,7 @@ const FIELDS: &[&str] = &[ "effort", "max_concurrent_reviews", "max_classifications", + "failures_only", "runtime", "acp", ]; @@ -329,7 +341,7 @@ fn queue_problem(e: &anyhow::Error) -> ProblemDetails { responses( (status = 200, description = "the settings in force; with none stored, \ `{\"model\":\"haiku\",\"effort\":null,\"max_concurrent_reviews\":2,\ - \"max_classifications\":10,\"runtime\":\"api-key\",\"acp\":null}`", + \"max_classifications\":10,\"failures_only\":false,\"runtime\":\"api-key\",\"acp\":null}`", body = Settings), (status = 503, description = "no swarm queue is configured on this host, or it is \ not connected (problem+json)", body = String), @@ -360,7 +372,7 @@ pub(crate) async fn get_settings( (status = 400, description = "not JSON or not this shape: an unknown field, a \ `null` other than for `effort` or `acp`, an effort other than the five \ levels, a negative or non-integer `max_concurrent_reviews` or \ - `max_classifications`, a runtime other \ + `max_classifications`, a `failures_only` that is not a boolean, a runtime other \ than `api-key` or `acp`, an `acp` without `base_url`, `model` or `prices`; or, \ once applied, a model that is empty, has whitespace or starts with `-`, \ `max_concurrent_reviews` outside 1 to 8, `max_classifications` outside 1 to \ @@ -449,7 +461,7 @@ mod tests { serde_json::to_value(Settings::default()).unwrap(), serde_json::json!({ "model": "haiku", "effort": null, "max_concurrent_reviews": 2, - "max_classifications": 10, "runtime": "api-key", "acp": null, + "max_classifications": 10, "failures_only": false, "runtime": "api-key", "acp": null, }) ); assert_eq!(Settings::default().runtime(), Ok(Runtime::ApiKey)); @@ -464,6 +476,7 @@ mod tests { effort: Some(Effort::High), max_concurrent_reviews: 5, max_classifications: 40, + failures_only: true, runtime: RuntimeKind::Acp, acp: Some(provider()), }; @@ -637,6 +650,7 @@ mod tests { effort: Some(Effort::High), max_concurrent_reviews: 3, max_classifications: 25, + failures_only: true, runtime: RuntimeKind::Acp, acp: Some(provider()), }; @@ -732,7 +746,7 @@ mod tests { read, serde_json::json!({ "model": "sonnet", "effort": "high", "max_concurrent_reviews": 3, - "max_classifications": 10, "runtime": "api-key", "acp": null, + "max_classifications": 10, "failures_only": false, "runtime": "api-key", "acp": null, }) ); @@ -770,7 +784,7 @@ mod tests { let wanted = serde_json::json!({ "model": "sonnet", "effort": "high", "max_concurrent_reviews": 5, - "max_classifications": 10, "runtime": "api-key", "acp": null, + "max_classifications": 10, "failures_only": false, "runtime": "api-key", "acp": null, }); let (status, stored, _) = send(&app, put(&wanted.to_string())).await; assert_eq!(status, StatusCode::OK, "{stored}"); @@ -811,7 +825,7 @@ mod tests { after, serde_json::json!({ "model": "sonnet", "effort": null, "max_concurrent_reviews": 4, - "max_classifications": 10, "runtime": "acp", "acp": acp, + "max_classifications": 10, "failures_only": false, "runtime": "acp", "acp": acp, }) ); let read = nats.client().await.settings().await.expect("readable");