From 731cef4c2d7a0ddd534a45859a54a95423f6eb04 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 14 Oct 2026 23:31:46 +0200 Subject: [PATCH] review classifier: the review table shows a pass/fail symbol, certainty against threshold, and an emoji on the overall line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review body's table is now: result | check | certainty / threshold | reason. The result cell is a symbol for what the row did to the review, after polarity (✅ counted and passed, ❌ counted and failed, ❔ not counted or no answer), then the model's verdict. The overall line carries ✅ / ❌. Polarity, threshold and counted are folded into those cells. The prompt asks for a reason under 350 characters; the enforced cap stays 500, so an overshoot is truncated less often. --- docs/swarm/review-classifier.md | 24 ++- .../src/pages/review-tryout/TryOutResult.tsx | 5 +- .../src/review_classifier/review.rs | 167 ++++++++++++++---- swarm-review-classifier/src/answer.rs | 5 + swarm-review-classifier/src/prompt.rs | 14 +- 5 files changed, 172 insertions(+), 43 deletions(-) diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index 48ac2af6..127ba7bd 100644 --- a/docs/swarm/review-classifier.md +++ b/docs/swarm/review-classifier.md @@ -300,8 +300,8 @@ The model answers each classification in JSON. The answer holds: - a **verdict**, one of _match_, _no match_ or _not applicable to this PR_ - a **certainty** from 0 to 100, how sure the model is of that verdict -- a **reason**; the classifier truncates one over 500 characters to 499 plus - `…`, and keeps the verdict and certainty +- a **reason**; the prompt asks for under 350 characters, and the classifier + truncates one over 500 to 499 plus `…`, keeping the verdict and certainty An answer: @@ -339,14 +339,20 @@ all. The service account posts the verdict as a forge review, approve or request changes, and the table is the review's body. An illustrative table: -| classification | polarity | verdict | certainty | threshold | counted | reason | -| -------------- | -------- | -------------- | --------- | --------- | ------- | -------------------------------------------- | -| datetime | positive | no match | 92 | 80 | yes ❌ | Two new timestamps use naive datetimes. | -| no-ads | negative | no match | 99 | 90 | yes ✅ | No commit message carries an AI attribution. | -| bao-first | positive | not applicable | 95 | 70 | no | The PR adds no secret. | -| docs-updated | positive | match | 41 | 60 | no | Below threshold. | +| result | check | certainty / threshold | reason | +| ----------- | ------------ | --------------------- | -------------------------------------------- | +| ❌ no match | datetime | 92 / 80 | Two new timestamps use naive datetimes. | +| ✅ no match | no-ads | 99 / 90 | No commit message carries an AI attribution. | +| ❔ unsure | bao-first | 95 / 70 | The PR adds no secret. | +| ❔ match | docs-updated | 41 / 60 | Below threshold. | -**Overall: request changes.** +**Overall: ❌ request changes** + +The result cell is the symbol for what the row did to the review, then the +model's verdict. ✅ is a counted row that passed its polarity and ❌ a counted +row that failed it, so _match_ is ✅ on a positive rule and ❌ on a negative one. +❔ is a row that doesn't count: below its threshold, not applicable (shown as +_unsure_), or without an answer. The certainty cell reads _certainty / threshold_. The approval is a real one: it counts toward a repo's required approvals like any other ([`pr-review-gate.md`](../process/pr-review-gate.md)). The diff --git a/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutResult.tsx b/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutResult.tsx index 3c9ff858..519d4549 100644 --- a/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutResult.tsx +++ b/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutResult.tsx @@ -1,6 +1,7 @@ // — a finished try-out: one row per classification, the -// overall verdict under them, and the head commit the run read. The columns -// are the review's own table (`docs/swarm/review-classifier.md`); no token +// overall verdict under them, and the head commit the run read. One column per +// field of a row, where the posted review folds them into four +// (`docs/swarm/review-classifier.md`); no token // counts, cache counts or cost, here or anywhere in the UI. import { Badge } from "@hive/shared/badge.js"; import { Table, type TableColumn } from "../../ui/table/Table.js"; diff --git a/swarm-controller/src/review_classifier/review.rs b/swarm-controller/src/review_classifier/review.rs index acdb482b..e7bde6a8 100644 --- a/swarm-controller/src/review_classifier/review.rs +++ b/swarm-controller/src/review_classifier/review.rs @@ -681,13 +681,13 @@ fn review_for(outcome: &Outcome) -> Ruling { ); Ruling::no_verdict(reason, rows) } else { - let (event, said) = match overall { - Overall::Approve => (Event::Approve, "approve"), - Overall::RequestChanges => (Event::RequestChanges, "request changes"), + let event = match overall { + Overall::Approve => Event::Approve, + Overall::RequestChanges => Event::RequestChanges, }; Ruling { event, - body: format!("{}\n**Overall: {said}.**\n", table(rows)), + body: format!("{}\n{}", table(rows), overall_line(*overall)), reason: None, } } @@ -711,32 +711,18 @@ fn no_verdict(reason: &str, rows: &[Row]) -> String { /// 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", + "| result | check | certainty / threshold | 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 (certainty, reason) = match &r.error { + Some(error) => ("—".to_owned(), error.as_str()), + None => (r.certainty.to_string(), r.reason.as_str()), }; let _ = writeln!( out, - "| {} | {polarity} | {verdict} | {certainty} | {} | {counted} | {} |", + "| {} | {} | {certainty} / {} | {} |", + result(r), cell(&r.name), r.threshold, cell(reason) @@ -745,6 +731,32 @@ fn table(rows: &[Row]) -> String { out } +/// 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 +/// threshold, not applicable, or without an answer. So `match` is ✅ under a +/// positive rule and ❌ under a negative one. +fn result(r: &Row) -> &'static str { + match (r.error.is_some(), r.counted, r.passed, r.verdict) { + (true, ..) => "❔ no answer", + (_, true, true, Verdict::Match) => "✅ match", + (_, true, true, Verdict::NoMatch) => "✅ no match", + (_, true, false, Verdict::Match) => "❌ match", + (_, true, false, Verdict::NoMatch) => "❌ no match", + (_, _, _, Verdict::Match) => "❔ match", + (_, _, _, Verdict::NoMatch) => "❔ no match", + (_, _, _, Verdict::NotApplicable) => "❔ unsure", + } +} + +/// The closing line of a review: the overall verdict, with its symbol. +fn overall_line(overall: Overall) -> &'static str { + match overall { + Overall::Approve => "**Overall: ✅ approve**\n", + Overall::RequestChanges => "**Overall: ❌ request changes**\n", + } +} + /// `text` as one table cell's inline text: on one line, with every character /// that could end the cell, start markup, math (`$`) or raw HTML escaped, and a zero-width /// space after every `@`, `#`, `!`, `://` and `www.`, so it carries no live @@ -834,6 +846,97 @@ mod tests { } } + /// A row as the aggregation produces it for `polarity`, `verdict`, + /// `certainty` against `threshold`. + fn answered(polarity: Polarity, verdict: Verdict, certainty: u8, threshold: u8) -> Row { + let counted = verdict != Verdict::NotApplicable && certainty >= threshold; + let wanted = match polarity { + Polarity::Positive => Verdict::Match, + Polarity::Negative => Verdict::NoMatch, + }; + Row { + name: "no-ads".to_owned(), + polarity, + verdict, + certainty, + threshold, + counted, + passed: counted && verdict == wanted, + reason: "Because.".to_owned(), + error: None, + } + } + + #[test] + fn a_row_shows_whether_it_passed_not_the_raw_verdict() { + use Polarity::{Negative, Positive}; + use Verdict::{Match, NoMatch, NotApplicable}; + for (case, r, expected) in [ + ( + "positive match passes", + answered(Positive, Match, 85, 65), + "| ✅ match | no-ads | 85 / 65 | Because. |", + ), + ( + "negative match fails", + answered(Negative, Match, 85, 65), + "| ❌ match | no-ads | 85 / 65 | Because. |", + ), + ( + "negative no match passes", + answered(Negative, NoMatch, 97, 80), + "| ✅ no match | no-ads | 97 / 80 | Because. |", + ), + ( + "positive no match fails", + answered(Positive, NoMatch, 97, 80), + "| ❌ no match | no-ads | 97 / 80 | Because. |", + ), + ( + "not applicable is unsure", + answered(Negative, NotApplicable, 98, 70), + "| ❔ unsure | no-ads | 98 / 70 | Because. |", + ), + ( + "below the threshold does not count", + answered(Positive, Match, 41, 60), + "| ❔ match | no-ads | 41 / 60 | Because. |", + ), + ( + "no answer shows its error", + Row { + error: Some("timed out".to_owned()), + verdict: NotApplicable, + certainty: 0, + counted: false, + passed: false, + reason: String::new(), + ..answered(Positive, Match, 0, 65) + }, + "| ❔ no answer | no-ads | — / 65 | timed out |", + ), + ] { + let body = table(&[r]); + assert_eq!( + body, + format!( + "| result | check | certainty / threshold | reason |\n\ + | --- | --- | --- | --- |\n{expected}\n" + ), + "{case}" + ); + } + } + + #[test] + fn the_overall_line_carries_its_emoji() { + assert_eq!(overall_line(Overall::Approve), "**Overall: ✅ approve**\n"); + assert_eq!( + overall_line(Overall::RequestChanges), + "**Overall: ❌ request changes**\n" + ); + } + fn request(name: &str) -> Requested { Requested { owner: "o".to_owned(), @@ -1437,7 +1540,7 @@ mod tests { assert_eq!(commit.as_deref(), Some("abc123")); assert_eq!(review.event, "APPROVED"); assert!( - review.body.ends_with("**Overall: approve.**\n"), + review.body.ends_with("**Overall: ✅ approve**\n"), "{}", review.body ); @@ -1451,7 +1554,11 @@ mod tests { ) .await; assert_eq!(review.event, "REQUEST_CHANGES"); - assert!(review.body.contains("| yes ❌ |"), "{}", review.body); + assert!( + review.body.contains("| ❌ match | datetime | 90 / 80 |"), + "{}", + review.body + ); } #[tokio::test] @@ -1499,12 +1606,12 @@ mod tests { assert_eq!(body.lines().count(), 3, "{body}"); assert!(!line.contains("@mara"), "{line}"); assert!(line.contains("@\u{200b}mara"), "{line}"); - // Seven cells: eight unescaped pipes. + // Four cells: five 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_eq!(unescaped, 5, "{line}"); assert!( line.contains("\\!\u{200b}\\[\\]") && line.contains("\\"), "{line}" @@ -1531,7 +1638,7 @@ mod tests { .char_indices() .filter(|(i, c)| *c == '|' && (*i == 0 || line.as_bytes()[i - 1] != b'\\')) .count(); - assert_eq!(unescaped, 8, "{line}"); + assert_eq!(unescaped, 5, "{line}"); } #[test] diff --git a/swarm-review-classifier/src/answer.rs b/swarm-review-classifier/src/answer.rs index 2decc3b1..ecb69e5a 100644 --- a/swarm-review-classifier/src/answer.rs +++ b/swarm-review-classifier/src/answer.rs @@ -12,6 +12,11 @@ use swarm_queue_client::review_classifier::Verdict; /// truncated to this, ending in `…`. pub const REASON_MAX_CHARS: usize = 500; +/// The length the prompt asks a reason to stay under. Below `REASON_MAX_CHARS` +/// so a model that overshoots the ask still lands inside the cap, uncut. +pub const REASON_ASKED_CHARS: usize = 350; +const _: () = assert!(REASON_ASKED_CHARS < REASON_MAX_CHARS); + /// One validated answer. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Answer { diff --git a/swarm-review-classifier/src/prompt.rs b/swarm-review-classifier/src/prompt.rs index 0b488b4d..c72eb469 100644 --- a/swarm-review-classifier/src/prompt.rs +++ b/swarm-review-classifier/src/prompt.rs @@ -9,7 +9,7 @@ use std::fmt::Write as _; use swarm_queue_client::review_classifier::{Classification, Polarity, TryOut}; -use crate::answer::REASON_MAX_CHARS; +use crate::answer::REASON_ASKED_CHARS; /// The system prompt, the same for every run. pub const SYSTEM: &str = "\ @@ -89,7 +89,7 @@ pub fn classification_prompt<'a>(shared: &'a str, c: &Classification) -> Prompt< }; let question = format!( "## The question\n\nClassification `{}` ({wanted}):\n\n{}\n\n\ - Answer with the JSON object only. Keep the reason under {REASON_MAX_CHARS} characters.\n", + Answer with the JSON object only. Keep the reason under {REASON_ASKED_CHARS} characters.\n", c.name, c.description.trim() ); @@ -296,6 +296,16 @@ diff --git a/src/main.rs b/src/main.rs assert_eq!(shared, shared_context(&run())); } + #[test] + fn the_question_asks_for_a_reason_under_the_asked_length_not_the_cap() { + let c = classification("datetime", "d", Polarity::Positive); + let question = classification_prompt("", &c).question; + assert!( + question.contains("Keep the reason under 350 characters."), + "{question}" + ); + } + #[test] fn the_shared_context_has_the_full_commit_message_and_a_collapsed_lockfile() { let shared = shared_context(&run());