vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

hive-forge: don't re-request a review from someone who already reviewed

Browse source 
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.
This commit is contained in:
damocles 2026-07-29 20:35:03 +02:00 • committed by mara
commit 4aa710bd18
1 changed files with 33 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

42
hive-forge/src/verbs/pr_assign_reviewer.rs
View file

@ -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(())
}