diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index db0849cc..e2608ed1 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -121,18 +121,30 @@ pub(crate) fn pct_encode(s: &str) -> String { out } -/// 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> { +/// One reviewer's latest verdict on a PR, plus forgejo's `stale` bit. +/// +/// `stale` is set by forgejo when the PR head commit changed after this +/// review was submitted — branch protection then wants a fresh review, so +/// a `stale` APPROVED no longer satisfies the merge gate even though its +/// `state` string still reads `APPROVED`. Surfacing it stops the CLI from +/// reporting a stale-but-technically-approved review as still-good. +pub(crate) struct ReviewInfo { + pub login: String, + pub state: String, + pub stale: bool, +} + +/// 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> { let (owner, name) = crate::client::split_repo(repo)?; let pr = index(pr)?; // Paginate (50/page, 10-page runaway cap — same ceiling the raw @@ -151,7 +163,7 @@ pub(crate) fn latest_reviews( break; } } - let mut latest: Vec<(String, String)> = Vec::new(); + let mut latest: Vec = Vec::new(); for r in &reviews { let Some(login) = r.user.as_ref().and_then(|u| u.login.as_deref()) else { continue; @@ -160,10 +172,16 @@ pub(crate) fn latest_reviews( if st == "COMMENT" || st == "PENDING" || st.is_empty() { continue; } - if let Some(slot) = latest.iter_mut().find(|(l, _)| l == login) { - st.clone_into(&mut slot.1); + let stale = r.stale.unwrap_or(false); + if let Some(slot) = latest.iter_mut().find(|info| info.login == login) { + st.clone_into(&mut slot.state); + slot.stale = stale; } else { - latest.push((login.to_owned(), st.to_owned())); + latest.push(ReviewInfo { + login: login.to_owned(), + state: st.to_owned(), + stale, + }); } } Ok(latest) diff --git a/hive-forge/src/verbs/pr_merge.rs b/hive-forge/src/verbs/pr_merge.rs index 8038c446..d3663889 100644 --- a/hive-forge/src/verbs/pr_merge.rs +++ b/hive-forge/src/verbs/pr_merge.rs @@ -157,11 +157,13 @@ fn check_ready(client: &Client, repo: &str, number: u64, pull: &PullRequest) -> // Block only on reviewers whose *current* verdict requests changes // (latest-per-reviewer, so a later APPROVED clears an earlier - // REQUEST_CHANGES). Shares the verdict logic with `pr-status`. + // REQUEST_CHANGES). A stale REQUEST_CHANGES was made against an older + // head and no longer applies, so it doesn't block. Shares the verdict + // logic with `pr-status`. let blockers: Vec = super::latest_reviews(client, repo, number)? .into_iter() - .filter(|(_, st)| st == "REQUEST_CHANGES") - .map(|(login, _)| login) + .filter(|r| r.state == "REQUEST_CHANGES" && !r.stale) + .map(|r| r.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 1069d65b..5280bb5c 100644 --- a/hive-forge/src/verbs/pr_reviews.rs +++ b/hive-forge/src/verbs/pr_reviews.rs @@ -135,6 +135,13 @@ 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"), @@ -161,7 +168,18 @@ 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(); - println!("### review by {user} ({state})"); + // 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}"); 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 c67b4439..e02b6982 100644 --- a/hive-forge/src/verbs/pr_status.rs +++ b/hive-forge/src/verbs/pr_status.rs @@ -120,7 +120,7 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> { "requested_reviewers": requested, "reviews": reviews .iter() - .map(|(l, s)| serde_json::json!({"user": l, "state": s})) + .map(|r| serde_json::json!({"user": r.login, "state": r.state, "stale": r.stale})) .collect::>(), "last_comment": last .as_ref() @@ -143,9 +143,12 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> { } // Merge-readiness: CI green, mergeable, and nobody requesting changes. + // A *stale* REQUEST_CHANGES was made against an older head — the branch + // moved since, so forgejo marks it stale and it no longer blocks the + // current head; don't let it hold up the verdict. let changes_requested = reviews .iter() - .any(|(_, verdict)| verdict == "REQUEST_CHANGES"); + .any(|r| r.state == "REQUEST_CHANGES" && !r.stale); let ready = ci_state == "success" && mergeable == Some(true) && !changes_requested; if ready { Ok(()) @@ -280,7 +283,7 @@ fn print_pr( ci_state: &str, ci_statuses: &[Value], requested: &[String], - reviews: &[(String, String)], + reviews: &[super::ReviewInfo], last_comment: Option<&(String, String)>, ) { println!("PR #{pr}: {title}"); @@ -310,13 +313,23 @@ fn print_pr( } else { let rendered: Vec = reviews .iter() - .map(|(login, verdict)| { - let mark = match verdict.as_str() { + .map(|r| { + let mark = match r.state.as_str() { + // A stale approval no longer satisfies branch protection; + // flag it so the CLI doesn't read as still-good. + "APPROVED" if r.stale => "⚠️", "APPROVED" => "✅", "REQUEST_CHANGES" => "❌", _ => "•", }; - format!("{mark} {login}: {verdict}") + let login = &r.login; + let state = &r.state; + let suffix = if r.stale { + " (stale — needs re-review on current head)" + } else { + "" + }; + format!("{mark} {login}: {state}{suffix}") }) .collect(); println!(" reviews: {}", rendered.join(", "));