From f633abbdc44aebc2ecb92b5df2d686100990cfcf Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 15 Jun 2026 11:39:19 +0200 Subject: [PATCH] refactor(hive-forge): share latest-per-reviewer logic between pr-status and pr-merge pr-status and pr-merge both computed 'latest non-comment review verdict per reviewer' independently (identical oldest-first, COMMENT/PENDING-skipping, supersede-by-later loop). Extract it to a shared verbs::latest_reviews helper so the verdict semantics live in one place and can't drift between the health view and the pre-merge changes-requested gate. Pure dedup, no behaviour change. --- hive-forge/src/verbs/mod.rs | 37 +++++++++++++++++++++++++++++++ hive-forge/src/verbs/pr_merge.rs | 28 ++++------------------- hive-forge/src/verbs/pr_status.rs | 30 +------------------------ 3 files changed, 42 insertions(+), 53 deletions(-) diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index 9f3da25b..4f1f3a79 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -36,6 +36,8 @@ pub mod view; use anyhow::Result; use serde_json::Value; +use crate::client::Client; + /// Pretty-print a `serde_json` value to stdout with a trailing newline, /// matching the bash script's `| jq` output shape. pub(crate) fn print_json(v: &Value) -> Result<()> { @@ -43,3 +45,38 @@ pub(crate) fn print_json(v: &Value) -> Result<()> { println!("{s}"); Ok(()) } + +/// 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 reviews = client.get_json_all(&format!("/repos/{repo}/pulls/{pr}/reviews"), 10)?; + let mut latest: Vec<(String, String)> = Vec::new(); + for r in &reviews { + let Some(login) = r + .get("user") + .and_then(|u| u.get("login")) + .and_then(Value::as_str) + else { + continue; + }; + let st = r.get("state").and_then(Value::as_str).unwrap_or(""); + 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); + } else { + latest.push((login.to_owned(), st.to_owned())); + } + } + Ok(latest) +} diff --git a/hive-forge/src/verbs/pr_merge.rs b/hive-forge/src/verbs/pr_merge.rs index c222e4a0..e7c20558 100644 --- a/hive-forge/src/verbs/pr_merge.rs +++ b/hive-forge/src/verbs/pr_merge.rs @@ -136,30 +136,10 @@ fn check_ready(client: &Client, repo: &str, number: u64, pull: &Value) -> Result } } - // Latest verdict per reviewer wins (reviews page oldest-first), so a - // later APPROVED supersedes an earlier REQUEST_CHANGES. Block only on - // reviewers whose *current* verdict requests changes. - let reviews = client.get_json_all(&format!("/repos/{repo}/pulls/{number}/reviews"), 10)?; - let mut latest: Vec<(String, String)> = Vec::new(); - for r in &reviews { - let Some(login) = r - .get("user") - .and_then(|u| u.get("login")) - .and_then(Value::as_str) - else { - continue; - }; - let st = r.get("state").and_then(Value::as_str).unwrap_or(""); - 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); - } else { - latest.push((login.to_owned(), st.to_owned())); - } - } - let blockers: Vec = latest + // 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`. + let blockers: Vec = super::latest_reviews(client, repo, number)? .into_iter() .filter(|(_, st)| st == "REQUEST_CHANGES") .map(|(login, _)| login) diff --git a/hive-forge/src/verbs/pr_status.rs b/hive-forge/src/verbs/pr_status.rs index 4ed9da40..c947cc01 100644 --- a/hive-forge/src/verbs/pr_status.rs +++ b/hive-forge/src/verbs/pr_status.rs @@ -85,7 +85,7 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> { .unwrap_or_default(); let (ci_state, ci_statuses) = fetch_combined(client, repo, &sha)?; - let reviews = latest_reviews(client, repo, pr)?; + let reviews = super::latest_reviews(client, repo, pr)?; let last = last_comment(client, repo, pr)?; if client.json_mode() { @@ -151,34 +151,6 @@ fn fetch_combined(client: &Client, repo: &str, sha: &str) -> Result<(String, Vec Ok((state, statuses)) } -/// Latest non-comment review verdict per reviewer, as `(login, state)`. -/// Reviews come oldest-first; a later one supersedes an earlier one from -/// the same user. `COMMENT` / `PENDING` reviews carry no verdict and are -/// skipped. -fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result> { - let reviews = client.get_json_all(&format!("/repos/{repo}/pulls/{pr}/reviews"), 10)?; - let mut latest: Vec<(String, String)> = Vec::new(); - for r in &reviews { - let Some(login) = r - .get("user") - .and_then(|u| u.get("login")) - .and_then(Value::as_str) - else { - continue; - }; - let st = r.get("state").and_then(Value::as_str).unwrap_or(""); - 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); - } else { - latest.push((login.to_owned(), st.to_owned())); - } - } - Ok(latest) -} - /// The most recent issue comment on the PR, as `(login, created_at)`. /// Comments page oldest-first; we drain (bounded) and take the max /// timestamp so a long thread still reports the genuinely-latest one.