diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 3e2d8cae..db449164 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -192,7 +192,9 @@ One-stop PR health view (``, positional — `--pr ` also still works): mergeable state, CI checks, requested reviewers + review verdicts, and the last-comment timestamp — the things you need to know whether a PR is ready to merge (CI must pass before merge). `--sha ` is a CI-only fast path -for a raw commit. +for a raw commit. The `reviewers` line lists pending requests only: a +reviewer whose verdict still stands is left out even though the forge keeps +them in its requested list, while one whose review is stale or dismissed stays. A failing/erroring CI context prints its job link. The process exit code is a **merge-readiness** verdict for a PR number (0 only when CI is green diff --git a/hive-forge/src/verbs/pr_status.rs b/hive-forge/src/verbs/pr_status.rs index a860fa5c..ef3c7b75 100644 --- a/hive-forge/src/verbs/pr_status.rs +++ b/hive-forge/src/verbs/pr_status.rs @@ -12,6 +12,7 @@ use anyhow::{Context, Result, bail}; use clap::Args as ClapArgs; use forgejo_api::structs::IssueGetCommentsQuery; +use hive_types::review::{ReviewInfo, standing}; use serde_json::Value; use crate::client::{Client, index}; @@ -100,6 +101,16 @@ fn sha_status(client: &Client, sha: &str) -> Result<()> { } } +/// The forge keeps a reviewer who has already answered in `requested_reviewers`, +/// so a login only counts as a pending request when it has no review that still +/// stands. A stale or dismissed review needs a fresh one, so that reviewer stays. +fn pending_requests(listed: Vec, reviews: &[ReviewInfo]) -> Vec { + listed + .into_iter() + .filter(|login| standing(reviews, login).is_none_or(|r| r.state == "REQUEST_REVIEW")) + .collect() +} + /// Full PR health view. Exit code is a merge-readiness verdict. fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> { let (owner, name) = client.owner_repo()?; @@ -122,7 +133,7 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> { .and_then(|h| h.sha.clone()) .with_context(|| format!("pr status: PR #{pr} has no head.sha"))?; - let requested: Vec = pull + let listed: Vec = pull .requested_reviewers .as_deref() .unwrap_or_default() @@ -132,6 +143,7 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> { let (ci_state, ci_statuses) = fetch_combined(client, &sha)?; let reviews = super::latest_reviews(client, repo, pr)?; + let requested = pending_requests(listed, &reviews); let last = last_comment(client, pr)?; if client.json_mode() { @@ -402,6 +414,33 @@ mod tests { use super::*; use clap::Parser; + fn info(login: &str, verdict: &str, old_head: bool) -> ReviewInfo { + ReviewInfo { + login: login.to_owned(), + state: verdict.to_owned(), + stale: old_head, + dismissed: false, + } + } + + /// A reviewer whose verdict still stands has answered; one with no + /// review, a bare re-request, or a verdict on an older head is pending. + #[test] + fn only_reviewers_without_a_standing_verdict_are_pending() { + let reviews = [ + info("answered", "APPROVED", false), + info("rerequested", "REQUEST_REVIEW", false), + info("stale", "REQUEST_CHANGES", true), + ]; + let listed = ["answered", "rerequested", "stale", "never-reviewed"] + .map(String::from) + .to_vec(); + assert_eq!( + pending_requests(listed, &reviews), + ["rerequested", "stale", "never-reviewed"] + ); + } + /// `Args` is a flattened arg group, not a `Parser`, so parsing it /// standalone needs a wrapper. Without one these cases could only be /// written against the whole binary's clap tree.