mara/hyperhive
mara/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

review classifier: a setting makes the PR review list only the failed rules

Browse source 
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.
This commit is contained in:
iris 2026-11-11 00:55:46 +01:00 • committed by mara
commit cf5746188a
3 changed files with 138 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

1
swarm-controller/src/review_classifier.rs
View file

@ -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()),
},

85
swarm-controller/src/review_classifier/review.rs
View file

@ -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<Row> = 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<Row>, 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()),
};

26
swarm-controller/src/review_classifier/settings.rs
View file

@ -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<usize>)]
max_classifications: Option<usize>,
#[serde(default, deserialize_with = "present")]
#[schema(value_type = Option<bool>)]
failures_only: Option<bool>,
#[serde(default, deserialize_with = "present")]
#[schema(value_type = Option<RuntimeKind>)]
runtime: Option<RuntimeKind>,
#[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");