From ce909a00a2d437a3fdff06f29abefc97e249c701 Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 10 Jul 2026 15:15:05 +0200 Subject: [PATCH] fix(#2327): handle dismissed reviews like stale (superseded helper) --- hive-forge/src/verbs/mod.rs | 26 +++++++++++++++++++++----- hive-forge/src/verbs/pr_merge.rs | 6 +++--- hive-forge/src/verbs/pr_status.rs | 22 ++++++++++++++-------- 3 files changed, 38 insertions(+), 16 deletions(-) diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index e2608ed1..2a438aa4 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -121,17 +121,30 @@ pub(crate) fn pct_encode(s: &str) -> String { out } -/// One reviewer's latest verdict on a PR, plus forgejo's `stale` bit. +/// 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, 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. +/// 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 @@ -173,14 +186,17 @@ 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 stale REQUEST_CHANGES was made against an older - // head and no longer applies, so it doesn't block. Shares the verdict + // 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`. let blockers: Vec = super::latest_reviews(client, repo, number)? .into_iter() - .filter(|r| r.state == "REQUEST_CHANGES" && !r.stale) + .filter(|r| r.state == "REQUEST_CHANGES" && !r.superseded()) .map(|r| r.login) .collect(); if !blockers.is_empty() { diff --git a/hive-forge/src/verbs/pr_status.rs b/hive-forge/src/verbs/pr_status.rs index e02b6982..1703381d 100644 --- a/hive-forge/src/verbs/pr_status.rs +++ b/hive-forge/src/verbs/pr_status.rs @@ -120,7 +120,10 @@ 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})) + .map(|r| serde_json::json!({ + "user": r.login, "state": r.state, + "stale": r.stale, "dismissed": r.dismissed, + })) .collect::>(), "last_comment": last .as_ref() @@ -143,12 +146,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. + // 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.stale); + .any(|r| r.state == "REQUEST_CHANGES" && !r.superseded()); let ready = ci_state == "success" && mergeable == Some(true) && !changes_requested; if ready { Ok(()) @@ -315,9 +318,10 @@ fn print_pr( .iter() .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 => "⚠️", + // 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() => "⚠️", "APPROVED" => "✅", "REQUEST_CHANGES" => "❌", _ => "•", @@ -326,6 +330,8 @@ fn print_pr( let state = &r.state; let suffix = if r.stale { " (stale — needs re-review on current head)" + } else if r.dismissed { + " (dismissed)" } else { "" };