fix(#3129): age out a review against the PR head, not forgejo's stale bit
Forgejo's per-review `stale` flag is eventually consistent: seconds after a push it still reports the pre-push answer, so a verdict against the previous head reads as current in exactly the window where the CLI gets run right after pushing. latest_reviews now fetches the PR head once and ORs a direct comparison of the review's own commit_id into the flag. Fixing it at construction rather than at the call sites means superseded() is unchanged and all three consumers are corrected together: assign-reviewer's refusal, pr_merge's changes-requested gate, and — the one that matters most — pr_status's readiness verdict, which could otherwise report a dead approval as valid. Unknowns fail toward keeping the verdict: a missing head or commit_id degrades to today's behaviour instead of voiding every review on the PR.
This commit is contained in:
parent
f4c470881e
commit
178eb13993
1 changed files with 59 additions and 2 deletions
|
|
@ -237,6 +237,14 @@ fn edit_distance(a: &str, b: &str) -> usize {
|
||||||
/// string still reads `APPROVED` / `REQUEST_CHANGES` — so surfacing them
|
/// string still reads `APPROVED` / `REQUEST_CHANGES` — so surfacing them
|
||||||
/// stops the CLI from reporting a no-longer-valid review as still-good, and
|
/// stops the CLI from reporting a no-longer-valid review as still-good, and
|
||||||
/// [`ReviewInfo::superseded`] rolls both into one "doesn't count" check.
|
/// [`ReviewInfo::superseded`] rolls both into one "doesn't count" check.
|
||||||
|
///
|
||||||
|
/// ⚠️ **`stale` here is not forgejo's flag alone.** That flag is eventually
|
||||||
|
/// consistent: seconds after a push it still reports the pre-push answer, so
|
||||||
|
/// a verdict against the previous head reads as current in exactly the window
|
||||||
|
/// where someone runs the CLI right after pushing. [`latest_reviews`] therefore
|
||||||
|
/// ORs it with a direct comparison of the review's own `commit_id` against the
|
||||||
|
/// PR head — the flag is right *eventually*, the comparison is right
|
||||||
|
/// *immediately*, and either alone is worse than both.
|
||||||
pub(crate) struct ReviewInfo {
|
pub(crate) struct ReviewInfo {
|
||||||
pub login: String,
|
pub login: String,
|
||||||
pub state: String,
|
pub state: String,
|
||||||
|
|
@ -253,6 +261,22 @@ impl ReviewInfo {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Whether this review was submitted against a commit that is no longer the
|
||||||
|
/// PR head — the race-free half of the staleness check.
|
||||||
|
///
|
||||||
|
/// Fails **closed on unknowns**, i.e. "not stale": either side missing means
|
||||||
|
/// we cannot show the head moved, and the cost of guessing wrong in that
|
||||||
|
/// direction is one redundant re-review, where the other direction would
|
||||||
|
/// silently void every verdict on the PR (blocking nothing, but reporting a
|
||||||
|
/// ready PR as unreviewed and inviting a re-request that dismisses a real
|
||||||
|
/// approval).
|
||||||
|
fn reviewed_older_head(reviewed_sha: Option<&str>, head_sha: Option<&str>) -> bool {
|
||||||
|
let (Some(head), Some(reviewed)) = (head_sha, reviewed_sha) else {
|
||||||
|
return false;
|
||||||
|
};
|
||||||
|
!reviewed.is_empty() && reviewed != head
|
||||||
|
}
|
||||||
|
|
||||||
/// Latest non-comment review per reviewer on a PR. Reviews come
|
/// Latest non-comment review per reviewer on a PR. Reviews come
|
||||||
/// oldest-first, so a later verdict from the same user supersedes an
|
/// oldest-first, so a later verdict from the same user supersedes an
|
||||||
/// earlier one; `COMMENT` / `PENDING` reviews carry no verdict and are
|
/// earlier one; `COMMENT` / `PENDING` reviews carry no verdict and are
|
||||||
|
|
@ -266,6 +290,16 @@ impl ReviewInfo {
|
||||||
pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result<Vec<ReviewInfo>> {
|
pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result<Vec<ReviewInfo>> {
|
||||||
let (owner, name) = crate::client::split_repo(repo)?;
|
let (owner, name) = crate::client::split_repo(repo)?;
|
||||||
let pr = index(pr)?;
|
let pr = index(pr)?;
|
||||||
|
// The head this PR currently points at, used to age out verdicts forgejo
|
||||||
|
// has not marked stale yet (see `ReviewInfo`). Best-effort: on any failure
|
||||||
|
// we fall back to forgejo's flag alone, which is today's behaviour — a
|
||||||
|
// missing head must never make every review look superseded.
|
||||||
|
let head_sha = client
|
||||||
|
.api()
|
||||||
|
.repo_get_pull_request(owner, name, pr)
|
||||||
|
.send()
|
||||||
|
.ok()
|
||||||
|
.and_then(|pull| pull.head.as_ref().and_then(|h| h.sha.clone()));
|
||||||
// Paginate (50/page, 10-page runaway cap — same ceiling the raw
|
// Paginate (50/page, 10-page runaway cap — same ceiling the raw
|
||||||
// client used) so a heavily re-reviewed PR doesn't truncate.
|
// client used) so a heavily re-reviewed PR doesn't truncate.
|
||||||
let mut reviews = Vec::new();
|
let mut reviews = Vec::new();
|
||||||
|
|
@ -291,7 +325,8 @@ pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result<Vec
|
||||||
if st == "COMMENT" || st == "PENDING" || st.is_empty() {
|
if st == "COMMENT" || st == "PENDING" || st.is_empty() {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
let stale = r.stale.unwrap_or(false);
|
let stale = r.stale.unwrap_or(false)
|
||||||
|
|| reviewed_older_head(r.commit_id.as_deref(), head_sha.as_deref());
|
||||||
let dismissed = r.dismissed.unwrap_or(false);
|
let dismissed = r.dismissed.unwrap_or(false);
|
||||||
if let Some(slot) = latest.iter_mut().find(|info| info.login == login) {
|
if let Some(slot) = latest.iter_mut().find(|info| info.login == login) {
|
||||||
st.clone_into(&mut slot.state);
|
st.clone_into(&mut slot.state);
|
||||||
|
|
@ -338,7 +373,29 @@ pub(crate) fn dependency_summaries(
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::pct_encode;
|
use super::{pct_encode, reviewed_older_head};
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn review_on_an_older_commit_is_stale() {
|
||||||
|
// The bug this exists for: forgejo still reports `stale: false` here
|
||||||
|
// in the seconds after a push, so the comparison has to catch it.
|
||||||
|
assert!(reviewed_older_head(Some("81292f14"), Some("f4c47088")));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn review_on_the_current_head_is_not_stale() {
|
||||||
|
assert!(!reviewed_older_head(Some("f4c47088"), Some("f4c47088")));
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Every unknown fails *toward* keeping the verdict. Voiding every review
|
||||||
|
/// on a forge that stopped reporting one of these would be a far louder
|
||||||
|
/// wrong answer than one redundant re-review.
|
||||||
|
#[test]
|
||||||
|
fn unknown_commit_or_head_is_not_stale() {
|
||||||
|
assert!(!reviewed_older_head(None, Some("f4c47088")));
|
||||||
|
assert!(!reviewed_older_head(Some("81292f14"), None));
|
||||||
|
assert!(!reviewed_older_head(Some(""), Some("f4c47088")));
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn pct_encode_passes_unreserved_through() {
|
fn pct_encode_passes_unreserved_through() {
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue