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

share the standing-review rule between hive-forge and the controller

Browse source 
pr assign-reviewer behaves as before; the latest-review fold and the
superseded check move to hive-types::review.
This commit is contained in:
atlas 2026-10-24 14:28:23 +02:00
commit 463c5cbfcc
7 changed files with 259 additions and 115 deletions
Download patch file Download diff file Expand all files Collapse all files

1
Cargo.lock generated
View file

@ -1844,6 +1844,7 @@ dependencies = [
"clap",
"clap-markdown",
"forgejo-api",
"hive-types",
"libc",
"reqwest",
"serde",

2
hive-forge/Cargo.toml
View file

@ -19,6 +19,8 @@ clap-markdown = "0.1"
# not about the workspace. See the pin note on `forgejo-api` in the root
# `Cargo.toml`.
forgejo-api = { workspace = true, features = ["sync"] }
# The review rule `pr assign-reviewer` shares with swarm-controller.
hive-types.workspace = true
url.workspace = true
time.workspace = true
reqwest = { workspace = true, features = [

125
hive-forge/src/verbs/mod.rs
View file

@ -346,66 +346,15 @@ fn edit_distance(a: &str, b: &str) -> usize {
prev[b.len()]
}
/// One reviewer's latest verdict on a PR, plus forgejo's `stale` /
/// `dismissed` bits.
///
/// `stale` is set by forgejo when the PR head commit changed after this
/// review was submitted (branch protection then wants a fresh review);
/// `dismissed` is set when the review was explicitly dismissed. Either way
/// the review no longer applies to the current head even though its `state`
/// string still reads `APPROVED` / `REQUEST_CHANGES` — so surfacing them
/// stops the CLI from reporting a no-longer-valid review as still-good, and
/// [`ReviewInfo::superseded`] rolls both into one "doesn't count" check.
///
/// ⚠️ **`stale` here is not forgejo's flag alone.** That flag is eventually
/// consistent: seconds after a push it still reports the pre-push answer, so
/// a verdict against the previous head reads as current in exactly the window
/// where someone runs the CLI right after pushing. [`latest_reviews`] therefore
/// ORs it with a direct comparison of the review's own `commit_id` against the
/// PR head — the flag is right *eventually*, the comparison is right
/// *immediately*, and either alone is worse than both.
pub(crate) struct ReviewInfo {
pub login: String,
pub state: String,
pub stale: bool,
pub dismissed: bool,
}
impl ReviewInfo {
/// True when the review no longer applies to the current head — stale
/// (head moved) or dismissed. Such a verdict neither blocks a merge nor
/// counts as a fresh approval.
pub(crate) fn superseded(&self) -> bool {
self.stale || self.dismissed
}
}
/// Whether this review was submitted against a commit that is no longer the
/// PR head — the race-free half of the staleness check.
///
/// Fails **closed on unknowns**, i.e. "not stale": either side missing means
/// we cannot show the head moved, and the cost of guessing wrong in that
/// direction is one redundant re-review, where the other direction would
/// silently void every verdict on the PR (blocking nothing, but reporting a
/// ready PR as unreviewed and inviting a re-request that dismisses a real
/// approval).
fn reviewed_older_head(reviewed_sha: Option<&str>, head_sha: Option<&str>) -> bool {
let (Some(head), Some(reviewed)) = (head_sha, reviewed_sha) else {
return false;
};
// Both emptiness checks matter, and for the same reason: a blank string
// is a value the forge sent, not a sha it has. Treating one as real would
// make every review compare unequal and mark the whole PR stale — the
// direction this whole function exists to avoid.
!head.is_empty() && !reviewed.is_empty() && reviewed != head
}
pub(crate) use hive_types::review::ReviewInfo;
/// Latest non-comment review per reviewer on a PR. Reviews come
/// oldest-first, so a later verdict from the same user supersedes an
/// earlier one; `COMMENT` / `PENDING` reviews carry no verdict and are
/// skipped. Shared by `pr status` (health view + readiness verdict) and
/// `pr merge` (pre-merge changes-requested gate) so the verdict semantics
/// stay in one place.
/// skipped. Shared by `pr status` (health view + readiness verdict),
/// `pr merge` (pre-merge changes-requested gate) and `pr assign-reviewer`,
/// and by swarm-controller's `request-reviewer` sink, through
/// [`hive_types::review`], so the verdict semantics stay in one place.
///
/// # Errors
///
@ -439,32 +388,16 @@ pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result<Vec
break;
}
}
let mut latest: Vec<ReviewInfo> = Vec::new();
for r in &reviews {
let Some(login) = r.user.as_ref().and_then(|u| u.login.as_deref()) else {
continue;
};
let st = r.state.as_deref().unwrap_or("");
if st == "COMMENT" || st == "PENDING" || st.is_empty() {
continue;
}
let stale = r.stale.unwrap_or(false)
|| reviewed_older_head(r.commit_id.as_deref(), head_sha.as_deref());
let dismissed = r.dismissed.unwrap_or(false);
if let Some(slot) = latest.iter_mut().find(|info| info.login == login) {
st.clone_into(&mut slot.state);
slot.stale = stale;
slot.dismissed = dismissed;
} else {
latest.push(ReviewInfo {
login: login.to_owned(),
state: st.to_owned(),
stale,
dismissed,
});
}
}
Ok(latest)
Ok(hive_types::review::latest_reviews(
reviews.iter().map(|r| hive_types::review::Listed {
login: r.user.as_ref().and_then(|u| u.login.as_deref()),
state: r.state.as_deref(),
commit_id: r.commit_id.as_deref(),
stale: r.stale,
dismissed: r.dismissed,
}),
head_sha.as_deref(),
))
}
/// The current dependency list for an issue or PR — each entry names
@ -684,7 +617,7 @@ pub(crate) fn attachment_json(assets: Option<&[Attachment]>) -> Vec<Value> {
#[cfg(test)]
mod tests {
use super::{NullableVec, pct_encode, refuse_json, reviewed_older_head};
use super::{NullableVec, pct_encode, refuse_json};
#[test]
fn refuse_json_bails_with_the_verb_and_reason() {
@ -715,32 +648,6 @@ mod tests {
assert_eq!(v.0, vec![1, 2, 3]);
}
#[test]
fn review_on_an_older_commit_is_stale() {
// The bug this exists for: forgejo still reports `stale: false` here
// in the seconds after a push, so the comparison has to catch it.
assert!(reviewed_older_head(Some("81292f14"), Some("f4c47088")));
}
#[test]
fn review_on_the_current_head_is_not_stale() {
assert!(!reviewed_older_head(Some("f4c47088"), Some("f4c47088")));
}
/// Every unknown fails *toward* keeping the verdict. Voiding every review
/// on a forge that stopped reporting one of these would be a far louder
/// wrong answer than one redundant re-review.
#[test]
fn unknown_commit_or_head_is_not_stale() {
assert!(!reviewed_older_head(None, Some("f4c47088")));
assert!(!reviewed_older_head(Some("81292f14"), None));
assert!(!reviewed_older_head(Some(""), Some("f4c47088")));
// Absent and blank have to behave the same on BOTH sides — a blank
// head that counted as real would mark every review on the PR stale.
assert!(!reviewed_older_head(Some("81292f14"), Some("")));
assert!(!reviewed_older_head(Some(""), Some("")));
}
#[test]
fn pct_encode_passes_unreserved_through() {
// Plain artifact names round-trip verbatim — no performance

6
hive-forge/src/verbs/pr_assign_reviewer.rs
View file

@ -55,10 +55,8 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
// Guard the add path only — withdrawing a request is always safe.
// Re-requesting from someone who already reviewed dismisses that review
// instead of no-op'ing (see the module doc-comment above).
if let Some(existing) = super::latest_reviews(client, client.repo()?, args.number)?
.into_iter()
.find(|r| r.login == args.user && !r.superseded())
{
let reviews = super::latest_reviews(client, client.repo()?, args.number)?;
if let Some(existing) = hive_types::review::standing(&reviews, &args.user) {
println!(
"not re-requesting: {} already has a standing {} review on #{} (re-requesting would dismiss it)",
args.user, existing.state, args.number

4
hive-types/README.md
View file

@ -17,6 +17,10 @@ Foundational shared **newtypes** for the hyperhive workspace. A zero-dependency
- **`term_msg`** — the icons and summary prefixes of a terminal row that sent a
message or started a turn from one, and the parsers that read the recipient
or sender back out.
- **`review`** — which of a PR's reviews still stand (latest verdict per
reviewer, stale or dismissed ones superseded). `hive-forge pr assign-reviewer`
and the notification router's `request-reviewer` sink share it, so both skip a
request on the same grounds.
The crate is deliberately a leaf with the smallest possible dependency footprint
so anything in the workspace can depend on it. See `docs/process/conventions.md`

5
hive-types/src/lib.rs
View file

@ -1,5 +1,5 @@
//! Foundational shared types for the hyperhive workspace: [`Ident`], [`Permission`]
//! and the [`term_msg`] row markers.
//! Foundational shared types for the hyperhive workspace: [`Ident`], [`Permission`],
//! the [`term_msg`] row markers and the [`review`] rule.
//!
//! A zero-dependency (bar `serde`) leaf crate so every wire-type crate
//! (`hive-sh4re`, `hive-host-sock`, `hive-core-agent-sock`) and both binaries
@ -11,6 +11,7 @@ mod permission;
pub use permission::Permission;
pub mod review;
pub mod term_msg;
/// The environment variable nix uses to hand this process the blacklist of

231
hive-types/src/review.rs Normal file
View file

@ -0,0 +1,231 @@
//! Which of a PR's reviews still stand: the rule `hive-forge pr
//! assign-reviewer` and the notification router's `request-reviewer` sink
//! share, so a request is skipped or made on the same grounds in both.
//!
//! The forge keeps a review's `state` (`APPROVED`, `REQUEST_CHANGES`, …) after
//! the head moves or the review is dismissed, so reading the state alone
//! reports a review that no longer applies as still good. [`latest_reviews`]
//! folds a PR's reviews to one verdict per reviewer, and
//! [`ReviewInfo::superseded`] says whether it still counts.
//!
//! Re-requesting a review from a reviewer whose review stands makes the forge
//! dismiss it, approval included. [`standing`] is the check a caller makes
//! before it requests.
/// One review as the forge lists it, reduced to what the rule reads. Every
/// field is optional because the forge's listing is.
#[derive(Clone, Copy, Debug, Default)]
pub struct Listed<'a> {
pub login: Option<&'a str>,
pub state: Option<&'a str>,
/// The commit the review was submitted against.
pub commit_id: Option<&'a str>,
/// The forge's own `stale` flag.
pub stale: Option<bool>,
pub dismissed: Option<bool>,
}
/// One reviewer's latest verdict on a PR, plus the `stale` / `dismissed` bits.
///
/// `stale` is set by the forge when the PR head commit changed after this
/// review was submitted; `dismissed` is set when the review was explicitly
/// dismissed. Either way the review no longer applies to the current head even
/// though its `state` string still reads `APPROVED` / `REQUEST_CHANGES`.
///
/// ⚠️ **`stale` here is not the forge's flag alone.** That flag is eventually
/// consistent: seconds after a push it still reports the pre-push answer, so a
/// verdict against the previous head reads as current in exactly the window
/// where a push has just happened. [`latest_reviews`] therefore ORs it with a
/// direct comparison of the review's own `commit_id` against the PR head: the
/// flag is right *eventually*, the comparison is right *immediately*.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct ReviewInfo {
pub login: String,
pub state: String,
pub stale: bool,
pub dismissed: bool,
}
impl ReviewInfo {
/// True when the review no longer applies to the current head: stale
/// (head moved) or dismissed. Such a verdict neither blocks a merge nor
/// counts as a fresh approval.
#[must_use]
pub fn superseded(&self) -> bool {
self.stale || self.dismissed
}
}
/// `login`'s review that still stands, if it has one. Its presence is the
/// reason not to request a review from `login` again.
#[must_use]
pub fn standing<'a>(reviews: &'a [ReviewInfo], login: &str) -> Option<&'a ReviewInfo> {
reviews.iter().find(|r| r.login == login && !r.superseded())
}
/// Whether this review was submitted against a commit that is no longer the PR
/// head: the race-free half of the staleness check.
///
/// Fails **closed on unknowns**, i.e. "not stale": either side missing means
/// the head's move can't be shown, and guessing wrong in that direction costs
/// one redundant re-review, where the other direction would void every
/// verdict on the PR and invite a re-request that dismisses a real approval.
#[must_use]
pub fn reviewed_older_head(reviewed_sha: Option<&str>, head_sha: Option<&str>) -> bool {
let (Some(head), Some(reviewed)) = (head_sha, reviewed_sha) else {
return false;
};
// Both emptiness checks matter, and for the same reason: a blank string is
// a value the forge sent, not a sha it has. Treating one as real would make
// every review compare unequal and mark the whole PR stale.
!head.is_empty() && !reviewed.is_empty() && reviewed != head
}
/// Latest non-comment review per reviewer. `reviews` come oldest first, so a
/// later verdict from the same reviewer supersedes an earlier one; `COMMENT`
/// and `PENDING` reviews carry no verdict and are skipped. `head_sha` is the
/// PR's current head, or `None` when it couldn't be read, which leaves the
/// forge's `stale` flag alone to decide.
#[must_use]
pub fn latest_reviews<'a>(
reviews: impl IntoIterator<Item = Listed<'a>>,
head_sha: Option<&str>,
) -> Vec<ReviewInfo> {
let mut latest: Vec<ReviewInfo> = Vec::new();
for r in reviews {
let Some(login) = r.login else { continue };
let verdict = r.state.unwrap_or("");
if verdict == "COMMENT" || verdict == "PENDING" || verdict.is_empty() {
continue;
}
let stale = r.stale.unwrap_or(false) || reviewed_older_head(r.commit_id, head_sha);
let dismissed = r.dismissed.unwrap_or(false);
if let Some(slot) = latest.iter_mut().find(|info| info.login == login) {
verdict.clone_into(&mut slot.state);
slot.stale = stale;
slot.dismissed = dismissed;
} else {
latest.push(ReviewInfo {
login: login.to_owned(),
state: verdict.to_owned(),
stale,
dismissed,
});
}
}
latest
}
#[cfg(test)]
mod tests {
use super::*;
fn review<'a>(login: &'a str, state: &'a str, commit: &'a str) -> Listed<'a> {
Listed {
login: Some(login),
state: Some(state),
commit_id: Some(commit),
..Listed::default()
}
}
#[test]
fn review_on_an_older_commit_is_stale() {
// The forge still reports `stale: false` here in the seconds after a
// push, so the comparison has to catch it.
assert!(reviewed_older_head(Some("81292f14"), Some("f4c47088")));
}
#[test]
fn review_on_the_current_head_is_not_stale() {
assert!(!reviewed_older_head(Some("f4c47088"), Some("f4c47088")));
}
/// Every unknown fails *toward* keeping the verdict. Voiding every review
/// on a forge that stopped reporting one of these would be a far louder
/// wrong answer than one redundant re-review.
#[test]
fn unknown_commit_or_head_is_not_stale() {
assert!(!reviewed_older_head(None, Some("f4c47088")));
assert!(!reviewed_older_head(Some("81292f14"), None));
assert!(!reviewed_older_head(Some(""), Some("f4c47088")));
// Absent and blank have to behave the same on BOTH sides: a blank head
// that counted as real would mark every review on the PR stale.
assert!(!reviewed_older_head(Some("81292f14"), Some("")));
assert!(!reviewed_older_head(Some(""), Some("")));
}
#[test]
fn a_review_of_the_current_head_stands_and_one_of_an_older_head_does_not() {
let reviews = latest_reviews(
[
review("review-classifier", "APPROVED", "new"),
review("argus", "APPROVED", "old"),
],
Some("new"),
);
assert_eq!(
standing(&reviews, "review-classifier").map(|r| r.state.as_str()),
Some("APPROVED")
);
// The forge's flag is still false for argus: only the commit says so.
assert!(standing(&reviews, "argus").is_none());
// Control: a reviewer who never reviewed has nothing standing.
assert!(standing(&reviews, "iris").is_none());
}
#[test]
fn a_dismissed_or_flagged_review_does_not_stand() {
let reviews = latest_reviews(
[
Listed {
dismissed: Some(true),
..review("a", "APPROVED", "new")
},
Listed {
stale: Some(true),
..review("b", "REQUEST_CHANGES", "new")
},
review("c", "APPROVED", "new"),
],
Some("new"),
);
assert!(standing(&reviews, "a").is_none());
assert!(standing(&reviews, "b").is_none());
assert!(standing(&reviews, "c").is_some(), "control");
}
#[test]
fn the_latest_verdict_per_reviewer_wins_and_comments_carry_none() {
let reviews = latest_reviews(
[
review("a", "REQUEST_CHANGES", "old"),
review("a", "APPROVED", "new"),
review("a", "COMMENT", "new"),
review("b", "PENDING", "new"),
Listed {
login: None,
..review("x", "APPROVED", "new")
},
],
Some("new"),
);
assert_eq!(
reviews,
[ReviewInfo {
login: "a".to_owned(),
state: "APPROVED".to_owned(),
stale: false,
dismissed: false,
}]
);
}
/// With the head unread, the forge's flag decides alone, so a review of an
/// unknown commit stands.
#[test]
fn an_unread_head_leaves_the_forges_flag_to_decide() {
let reviews = latest_reviews([review("a", "APPROVED", "old")], None);
assert!(standing(&reviews, "a").is_some());
}
}