From 4aa710bd18807891f64d37800612ed718a22fb06 Mon Sep 17 00:00:00 2001
From: damocles <damocles@hyperhive.local>
Date: Wed, 29 Jul 2026 20:35:03 +0200
Subject: [PATCH] hive-forge: don't re-request a review from someone who
 already reviewed

fixes hyperhive/hyperhive#2839. pr_assign_reviewer's own doc-comment
claimed full idempotency (already-requested = no-op), but conflated
'still pending' with 'already reviewed' - Forgejo clears a fulfilled
reviewer from requested_reviewers, so re-requesting them isn't a
no-op, it dismisses the standing review. now checks latest_reviews
for a non-superseded review from the target user first and skips
the request instead of blindly posting it.
---
 hive-forge/src/verbs/pr_assign_reviewer.rs | 42 +++++++++++++++++-----
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/hive-forge/src/verbs/pr_assign_reviewer.rs b/hive-forge/src/verbs/pr_assign_reviewer.rs
index b0d14f9a..3a0a1cc7 100644
--- a/hive-forge/src/verbs/pr_assign_reviewer.rs
+++ b/hive-forge/src/verbs/pr_assign_reviewer.rs
@@ -2,9 +2,20 @@
 //! requested reviewer on a PR. Unlike `pr assign-committer` (assignees,
 //! mutate + PATCH the issue) Forgejo has a dedicated endpoint pair for review
 //! requests: `POST /repos/{repo}/pulls/{index}/requested_reviewers`
-//! adds, `DELETE` (same path + body) removes. Requesting a review from
-//! a user who is already requested is a Forgejo-side no-op, so the verb
-//! is idempotent in both directions.
+//! adds, `DELETE` (same path + body) removes.
+//!
+//! Requesting a review from a user who is already *pending* (requested but
+//! hasn't reviewed yet) is a Forgejo-side no-op. Requesting from a user who
+//! has already **submitted** a review is a different story: Forgejo doesn't
+//! no-op that case, it dismisses the existing review (including an
+//! `APPROVED` one), because the reviewer is no longer in the pending-request
+//! list once they've reviewed. A caller re-requesting on the strength of
+//! "already requested = no-op" (the old, wrong claim this module made)
+//! silently destroys a standing approval — this is how a coverage sweep that
+//! re-requests reviews indiscriminately can dismiss a ready PR's approval
+//! with no new commit in sight. So this verb checks for an existing
+//! non-superseded review from `user` first and skips the request instead of
+//! blindly posting it.
 
 use anyhow::Result;
 use clap::Args as ClapArgs;
@@ -39,12 +50,25 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
             "review request withdrawn: {} on #{}",
             args.user, args.number
         );
-    } else {
-        client
-            .api()
-            .repo_create_pull_review_requests(owner, name, idx, body)
-            .send()?;
-        println!("review requested: {} on #{}", args.user, args.number);
+        return Ok(());
     }
+    // Guard the add path only — withdrawing a request is always safe.
+    // Re-requesting from someone who already reviewed dismisses that review
+    // instead of no-op'ing (see the module doc-comment above).
+    if let Some(existing) = super::latest_reviews(client, client.repo(), args.number)?
+        .into_iter()
+        .find(|r| r.login == args.user && !r.superseded())
+    {
+        println!(
+            "not re-requesting: {} already has a standing {} review on #{} (re-requesting would dismiss it)",
+            args.user, existing.state, args.number
+        );
+        return Ok(());
+    }
+    client
+        .api()
+        .repo_create_pull_review_requests(owner, name, idx, body)
+        .send()?;
+    println!("review requested: {} on #{}", args.user, args.number);
     Ok(())
 }