diff --git a/docs/swarm/review-classifier.md b/docs/swarm/review-classifier.md index a3184d95..48ac2af6 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 prompt asks for under 350 characters, and the classifier - truncates one over 500 to 499 plus `…`, keeping the verdict and certainty +- a **reason**; the classifier truncates one over 500 characters to 499 plus + `…`, and keeps the verdict and certainty An answer: @@ -339,20 +339,14 @@ 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: -| result | check | certainty | 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. | +| 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. | -**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_. +**Overall: request changes.** 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/docs/swarm/ui.md b/docs/swarm/ui.md index 7052a663..8cd3539e 100644 --- a/docs/swarm/ui.md +++ b/docs/swarm/ui.md @@ -115,9 +115,9 @@ doesn't parse, or it holds more than ten rules, the page shows the reason and leaves your classifications as they were. A run takes a minute or more, and up to half an hour for ten classifications. -The page says it's running and shows how long it has been, then lists each classification with its verdict, the model's certainty, the -threshold, whether the verdict counted and the reason; the posted review folds -those fields into four columns ([`review-classifier.md`](review-classifier.md)). A classification that couldn't run shows its error where the reason +The page says it's running and shows how long it has been, then shows the review's table: for each classification the verdict, +the model's certainty, the threshold, whether the verdict counted and the +reason. A classification that couldn't run shows its error where the reason would be. Under the table are the overall verdict, approve or request changes, and the head commit the run read. A failed run shows its error, for example when the swarm has no API key yet. 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 519d4549..3c9ff858 100644 --- a/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutResult.tsx +++ b/frontend/packages/swarm-ui/src/pages/review-tryout/TryOutResult.tsx @@ -1,7 +1,6 @@ // — a finished try-out: one row per classification, the -// 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 +// 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 // 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 b49a4680..acdb482b 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 = match overall { - Overall::Approve => Event::Approve, - Overall::RequestChanges => Event::RequestChanges, + let (event, said) = match overall { + Overall::Approve => (Event::Approve, "approve"), + Overall::RequestChanges => (Event::RequestChanges, "request changes"), }; Ruling { event, - body: format!("{}\n{}", table(rows), overall_line(*overall)), + body: format!("{}\n**Overall: {said}.**\n", table(rows)), reason: None, } } @@ -711,18 +711,32 @@ 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( - "| result | check | certainty | reason |\n\ - | --- | --- | --- | --- |\n", + "| classification | polarity | verdict | certainty | threshold | counted | reason |\n\ + | --- | --- | --- | --- | --- | --- | --- |\n", ); for r in rows { - let (certainty, reason) = match &r.error { - Some(error) => ("—".to_owned(), error.as_str()), - None => (r.certainty.to_string(), r.reason.as_str()), + 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, - "| {} | {} | {certainty} / {} | {} |", - result(r), + "| {} | {polarity} | {verdict} | {certainty} | {} | {counted} | {} |", cell(&r.name), r.threshold, cell(reason) @@ -731,32 +745,6 @@ 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 @@ -846,97 +834,6 @@ 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 | 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(), @@ -1540,7 +1437,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 ); @@ -1554,11 +1451,7 @@ mod tests { ) .await; assert_eq!(review.event, "REQUEST_CHANGES"); - assert!( - review.body.contains("| ❌ match | datetime | 90 / 80 |"), - "{}", - review.body - ); + assert!(review.body.contains("| yes ❌ |"), "{}", review.body); } #[tokio::test] @@ -1606,12 +1499,12 @@ mod tests { assert_eq!(body.lines().count(), 3, "{body}"); assert!(!line.contains("@mara"), "{line}"); assert!(line.contains("@\u{200b}mara"), "{line}"); - // Four cells: five unescaped pipes. + // 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, 5, "{line}"); + assert_eq!(unescaped, 8, "{line}"); assert!( line.contains("\\!\u{200b}\\[\\]") && line.contains("\\"), "{line}" @@ -1638,7 +1531,7 @@ mod tests { .char_indices() .filter(|(i, c)| *c == '|' && (*i == 0 || line.as_bytes()[i - 1] != b'\\')) .count(); - assert_eq!(unescaped, 5, "{line}"); + assert_eq!(unescaped, 8, "{line}"); } #[test] diff --git a/swarm-review-classifier/README.md b/swarm-review-classifier/README.md index 70a29dcd..aa918514 100644 --- a/swarm-review-classifier/README.md +++ b/swarm-review-classifier/README.md @@ -20,7 +20,7 @@ forge and holds no forge credential. (`--tools ""`), no MCP server, its own empty directory and `ANTHROPIC_API_KEY` in its environment only. - An answer is a JSON object with a verdict, a certainty and a reason; the - prompt asks for a reason under 350 characters, and the classifier truncates one over the 500 cap to 499 plus `…`. A failed run, a + classifier truncates a reason over 500 characters to 499 plus `…`. A failed run, a timeout or an answer that doesn't validate leaves that row without a verdict, and it doesn't count. - A run with no API key, or with no classification answered, fails as a whole: the caller sees an error, never a verdict. diff --git a/swarm-review-classifier/src/answer.rs b/swarm-review-classifier/src/answer.rs index ecb69e5a..2decc3b1 100644 --- a/swarm-review-classifier/src/answer.rs +++ b/swarm-review-classifier/src/answer.rs @@ -12,11 +12,6 @@ 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 c72eb469..0b488b4d 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_ASKED_CHARS; +use crate::answer::REASON_MAX_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_ASKED_CHARS} characters.\n", + Answer with the JSON object only. Keep the reason under {REASON_MAX_CHARS} characters.\n", c.name, c.description.trim() ); @@ -296,16 +296,6 @@ 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());