review classifier: the review table shows a pass/fail symbol, certainty against threshold, and an emoji on the overall line
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.
This commit is contained in:
parent
46fd6e502a
commit
731cef4c2d
5 changed files with 172 additions and 43 deletions
|
|
@ -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 **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 **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
|
- a **reason**; the prompt asks for under 350 characters, and the classifier
|
||||||
`…`, and keeps the verdict and certainty
|
truncates one over 500 to 499 plus `…`, keeping the verdict and certainty
|
||||||
|
|
||||||
An answer:
|
An answer:
|
||||||
|
|
||||||
|
|
@ -339,14 +339,20 @@ all.
|
||||||
The service account posts the verdict as a forge review, approve or request
|
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:
|
changes, and the table is the review's body. An illustrative table:
|
||||||
|
|
||||||
| classification | polarity | verdict | certainty | threshold | counted | reason |
|
| result | check | certainty / threshold | reason |
|
||||||
| -------------- | -------- | -------------- | --------- | --------- | ------- | -------------------------------------------- |
|
| ----------- | ------------ | --------------------- | -------------------------------------------- |
|
||||||
| datetime | positive | no match | 92 | 80 | yes ❌ | Two new timestamps use naive datetimes. |
|
| ❌ no match | datetime | 92 / 80 | Two new timestamps use naive datetimes. |
|
||||||
| no-ads | negative | no match | 99 | 90 | yes ✅ | No commit message carries an AI attribution. |
|
| ✅ no match | no-ads | 99 / 90 | No commit message carries an AI attribution. |
|
||||||
| bao-first | positive | not applicable | 95 | 70 | no | The PR adds no secret. |
|
| ❔ unsure | bao-first | 95 / 70 | The PR adds no secret. |
|
||||||
| docs-updated | positive | match | 41 | 60 | no | Below threshold. |
|
| ❔ 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
|
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
|
like any other ([`pr-review-gate.md`](../process/pr-review-gate.md)). The
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,7 @@
|
||||||
// <TryOutResult> — a finished try-out: one row per classification, the
|
// <TryOutResult> — a finished try-out: one row per classification, the
|
||||||
// overall verdict under them, and the head commit the run read. The columns
|
// overall verdict under them, and the head commit the run read. One column per
|
||||||
// are the review's own table (`docs/swarm/review-classifier.md`); no token
|
// 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.
|
// counts, cache counts or cost, here or anywhere in the UI.
|
||||||
import { Badge } from "@hive/shared/badge.js";
|
import { Badge } from "@hive/shared/badge.js";
|
||||||
import { Table, type TableColumn } from "../../ui/table/Table.js";
|
import { Table, type TableColumn } from "../../ui/table/Table.js";
|
||||||
|
|
|
||||||
|
|
@ -681,13 +681,13 @@ fn review_for(outcome: &Outcome) -> Ruling {
|
||||||
);
|
);
|
||||||
Ruling::no_verdict(reason, rows)
|
Ruling::no_verdict(reason, rows)
|
||||||
} else {
|
} else {
|
||||||
let (event, said) = match overall {
|
let event = match overall {
|
||||||
Overall::Approve => (Event::Approve, "approve"),
|
Overall::Approve => Event::Approve,
|
||||||
Overall::RequestChanges => (Event::RequestChanges, "request changes"),
|
Overall::RequestChanges => Event::RequestChanges,
|
||||||
};
|
};
|
||||||
Ruling {
|
Ruling {
|
||||||
event,
|
event,
|
||||||
body: format!("{}\n**Overall: {said}.**\n", table(rows)),
|
body: format!("{}\n{}", table(rows), overall_line(*overall)),
|
||||||
reason: None,
|
reason: None,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -711,32 +711,18 @@ fn no_verdict(reason: &str, rows: &[Row]) -> String {
|
||||||
/// The result table, one row per classification.
|
/// The result table, one row per classification.
|
||||||
fn table(rows: &[Row]) -> String {
|
fn table(rows: &[Row]) -> String {
|
||||||
let mut out = String::from(
|
let mut out = String::from(
|
||||||
"| classification | polarity | verdict | certainty | threshold | counted | reason |\n\
|
"| result | check | certainty / threshold | reason |\n\
|
||||||
| --- | --- | --- | --- | --- | --- | --- |\n",
|
| --- | --- | --- | --- |\n",
|
||||||
);
|
);
|
||||||
for r in rows {
|
for r in rows {
|
||||||
let polarity = match r.polarity {
|
let (certainty, reason) = match &r.error {
|
||||||
swarm_queue_client::review_classifier::Polarity::Positive => "positive",
|
Some(error) => ("—".to_owned(), error.as_str()),
|
||||||
swarm_queue_client::review_classifier::Polarity::Negative => "negative",
|
None => (r.certainty.to_string(), r.reason.as_str()),
|
||||||
};
|
|
||||||
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!(
|
let _ = writeln!(
|
||||||
out,
|
out,
|
||||||
"| {} | {polarity} | {verdict} | {certainty} | {} | {counted} | {} |",
|
"| {} | {} | {certainty} / {} | {} |",
|
||||||
|
result(r),
|
||||||
cell(&r.name),
|
cell(&r.name),
|
||||||
r.threshold,
|
r.threshold,
|
||||||
cell(reason)
|
cell(reason)
|
||||||
|
|
@ -745,6 +731,32 @@ fn table(rows: &[Row]) -> String {
|
||||||
out
|
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
|
/// `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
|
/// 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
|
/// 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 {
|
fn request(name: &str) -> Requested {
|
||||||
Requested {
|
Requested {
|
||||||
owner: "o".to_owned(),
|
owner: "o".to_owned(),
|
||||||
|
|
@ -1437,7 +1540,7 @@ mod tests {
|
||||||
assert_eq!(commit.as_deref(), Some("abc123"));
|
assert_eq!(commit.as_deref(), Some("abc123"));
|
||||||
assert_eq!(review.event, "APPROVED");
|
assert_eq!(review.event, "APPROVED");
|
||||||
assert!(
|
assert!(
|
||||||
review.body.ends_with("**Overall: approve.**\n"),
|
review.body.ends_with("**Overall: ✅ approve**\n"),
|
||||||
"{}",
|
"{}",
|
||||||
review.body
|
review.body
|
||||||
);
|
);
|
||||||
|
|
@ -1451,7 +1554,11 @@ mod tests {
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
assert_eq!(review.event, "REQUEST_CHANGES");
|
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]
|
#[tokio::test]
|
||||||
|
|
@ -1499,12 +1606,12 @@ mod tests {
|
||||||
assert_eq!(body.lines().count(), 3, "{body}");
|
assert_eq!(body.lines().count(), 3, "{body}");
|
||||||
assert!(!line.contains("@mara"), "{line}");
|
assert!(!line.contains("@mara"), "{line}");
|
||||||
assert!(line.contains("@\u{200b}mara"), "{line}");
|
assert!(line.contains("@\u{200b}mara"), "{line}");
|
||||||
// Seven cells: eight unescaped pipes.
|
// Four cells: five unescaped pipes.
|
||||||
let unescaped = line
|
let unescaped = line
|
||||||
.char_indices()
|
.char_indices()
|
||||||
.filter(|(i, c)| *c == '|' && (*i == 0 || line.as_bytes()[i - 1] != b'\\'))
|
.filter(|(i, c)| *c == '|' && (*i == 0 || line.as_bytes()[i - 1] != b'\\'))
|
||||||
.count();
|
.count();
|
||||||
assert_eq!(unescaped, 8, "{line}");
|
assert_eq!(unescaped, 5, "{line}");
|
||||||
assert!(
|
assert!(
|
||||||
line.contains("\\!\u{200b}\\[\\]") && line.contains("\\<b\\>"),
|
line.contains("\\!\u{200b}\\[\\]") && line.contains("\\<b\\>"),
|
||||||
"{line}"
|
"{line}"
|
||||||
|
|
@ -1531,7 +1638,7 @@ mod tests {
|
||||||
.char_indices()
|
.char_indices()
|
||||||
.filter(|(i, c)| *c == '|' && (*i == 0 || line.as_bytes()[i - 1] != b'\\'))
|
.filter(|(i, c)| *c == '|' && (*i == 0 || line.as_bytes()[i - 1] != b'\\'))
|
||||||
.count();
|
.count();
|
||||||
assert_eq!(unescaped, 8, "{line}");
|
assert_eq!(unescaped, 5, "{line}");
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|
|
||||||
|
|
@ -12,6 +12,11 @@ use swarm_queue_client::review_classifier::Verdict;
|
||||||
/// truncated to this, ending in `…`.
|
/// truncated to this, ending in `…`.
|
||||||
pub const REASON_MAX_CHARS: usize = 500;
|
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.
|
/// One validated answer.
|
||||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||||
pub struct Answer {
|
pub struct Answer {
|
||||||
|
|
|
||||||
|
|
@ -9,7 +9,7 @@ use std::fmt::Write as _;
|
||||||
|
|
||||||
use swarm_queue_client::review_classifier::{Classification, Polarity, TryOut};
|
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.
|
/// The system prompt, the same for every run.
|
||||||
pub const SYSTEM: &str = "\
|
pub const SYSTEM: &str = "\
|
||||||
|
|
@ -89,7 +89,7 @@ pub fn classification_prompt<'a>(shared: &'a str, c: &Classification) -> Prompt<
|
||||||
};
|
};
|
||||||
let question = format!(
|
let question = format!(
|
||||||
"## The question\n\nClassification `{}` ({wanted}):\n\n{}\n\n\
|
"## 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.name,
|
||||||
c.description.trim()
|
c.description.trim()
|
||||||
);
|
);
|
||||||
|
|
@ -296,6 +296,16 @@ diff --git a/src/main.rs b/src/main.rs
|
||||||
assert_eq!(shared, shared_context(&run()));
|
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]
|
#[test]
|
||||||
fn the_shared_context_has_the_full_commit_message_and_a_collapsed_lockfile() {
|
fn the_shared_context_has_the_full_commit_message_and_a_collapsed_lockfile() {
|
||||||
let shared = shared_context(&run());
|
let shared = shared_context(&run());
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue