diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index 2a438aa4..db0849cc 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -121,43 +121,18 @@ pub(crate) fn pct_encode(s: &str) -> String { out } -/// 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. -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 - } -} - -/// 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. -/// -/// # Errors -/// -/// Propagates the forge API errors from listing the PR's reviews. -pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result> { +/// Latest non-comment review verdict per reviewer on a PR, as +/// `(login, state)`. 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` (for +/// its health view + readiness verdict) and `pr-merge` (for its +/// pre-merge changes-requested gate) so the verdict semantics stay in +/// one place. +pub(crate) fn latest_reviews( + client: &Client, + repo: &str, + pr: u64, +) -> Result> { let (owner, name) = crate::client::split_repo(repo)?; let pr = index(pr)?; // Paginate (50/page, 10-page runaway cap — same ceiling the raw @@ -176,7 +151,7 @@ pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result = Vec::new(); + let mut latest: Vec<(String, String)> = Vec::new(); for r in &reviews { let Some(login) = r.user.as_ref().and_then(|u| u.login.as_deref()) else { continue; @@ -185,19 +160,10 @@ pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result // Block only on reviewers whose *current* verdict requests changes // (latest-per-reviewer, so a later APPROVED clears an earlier - // REQUEST_CHANGES). A superseded REQUEST_CHANGES — stale (older head) or - // dismissed — no longer applies, so it doesn't block. Shares the verdict - // logic with `pr-status`. + // REQUEST_CHANGES). Shares the verdict logic with `pr-status`. let blockers: Vec = super::latest_reviews(client, repo, number)? .into_iter() - .filter(|r| r.state == "REQUEST_CHANGES" && !r.superseded()) - .map(|r| r.login) + .filter(|(_, st)| st == "REQUEST_CHANGES") + .map(|(login, _)| login) .collect(); if !blockers.is_empty() { bail!( diff --git a/hive-forge/src/verbs/pr_reviews.rs b/hive-forge/src/verbs/pr_reviews.rs index 5280bb5c..1069d65b 100644 --- a/hive-forge/src/verbs/pr_reviews.rs +++ b/hive-forge/src/verbs/pr_reviews.rs @@ -135,13 +135,6 @@ fn list_reviews_json(client: &Client, number: u64, reviews: &[Value]) -> Result< json!({ "id": r.get("id"), "state": r.get("state"), - // forgejo's staleness bits: `stale` = head moved since the - // review (branch protection wants a fresh one); `dismissed` - // = explicitly dismissed. A stale/dismissed APPROVED no - // longer satisfies the merge gate despite `state` reading - // APPROVED — surface them so callers don't over-trust it. - "stale": r.get("stale"), - "dismissed": r.get("dismissed"), "user": r.get("user").and_then(|u| u.get("login")), "body": r.get("body"), "comments_count": r.get("comments_count"), @@ -168,18 +161,7 @@ fn list_reviews_text(client: &Client, number: u64, reviews: &[Value]) { .unwrap_or("?"); let state = r.get("state").and_then(Value::as_str).unwrap_or("?"); let body = r.get("body").and_then(Value::as_str).unwrap_or("").trim(); - // Flag reviews forgejo considers no-longer-current so a stale - // APPROVED doesn't read as still-satisfying branch protection. - let is_stale = r.get("stale").and_then(Value::as_bool).unwrap_or(false); - let is_dismissed = r.get("dismissed").and_then(Value::as_bool).unwrap_or(false); - let flag = if is_stale { - " [stale — needs re-review]" - } else if is_dismissed { - " [dismissed]" - } else { - "" - }; - println!("### review by {user} ({state}){flag}"); + println!("### review by {user} ({state})"); if !body.is_empty() { println!("{body}"); } diff --git a/hive-forge/src/verbs/pr_status.rs b/hive-forge/src/verbs/pr_status.rs index 1703381d..c67b4439 100644 --- a/hive-forge/src/verbs/pr_status.rs +++ b/hive-forge/src/verbs/pr_status.rs @@ -120,10 +120,7 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> { "requested_reviewers": requested, "reviews": reviews .iter() - .map(|r| serde_json::json!({ - "user": r.login, "state": r.state, - "stale": r.stale, "dismissed": r.dismissed, - })) + .map(|(l, s)| serde_json::json!({"user": l, "state": s})) .collect::>(), "last_comment": last .as_ref() @@ -146,12 +143,9 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> { } // Merge-readiness: CI green, mergeable, and nobody requesting changes. - // A *superseded* REQUEST_CHANGES — stale (branch moved since) or - // dismissed — no longer applies to the current head, so it doesn't hold - // up the verdict. let changes_requested = reviews .iter() - .any(|r| r.state == "REQUEST_CHANGES" && !r.superseded()); + .any(|(_, verdict)| verdict == "REQUEST_CHANGES"); let ready = ci_state == "success" && mergeable == Some(true) && !changes_requested; if ready { Ok(()) @@ -286,7 +280,7 @@ fn print_pr( ci_state: &str, ci_statuses: &[Value], requested: &[String], - reviews: &[super::ReviewInfo], + reviews: &[(String, String)], last_comment: Option<&(String, String)>, ) { println!("PR #{pr}: {title}"); @@ -316,26 +310,13 @@ fn print_pr( } else { let rendered: Vec = reviews .iter() - .map(|r| { - let mark = match r.state.as_str() { - // A superseded approval (stale/dismissed) no longer - // satisfies branch protection; flag it so the CLI doesn't - // read as still-good. - "APPROVED" if r.superseded() => "⚠️", + .map(|(login, verdict)| { + let mark = match verdict.as_str() { "APPROVED" => "✅", "REQUEST_CHANGES" => "❌", _ => "•", }; - let login = &r.login; - let state = &r.state; - let suffix = if r.stale { - " (stale — needs re-review on current head)" - } else if r.dismissed { - " (dismissed)" - } else { - "" - }; - format!("{mark} {login}: {state}{suffix}") + format!("{mark} {login}: {verdict}") }) .collect(); println!(" reviews: {}", rendered.join(", "));