diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index 5a12797b..ec1d1d42 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -237,6 +237,14 @@ fn edit_distance(a: &str, b: &str) -> usize { /// 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. +/// +/// ⚠️ **`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 login: 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 /// oldest-first, so a later verdict from the same user supersedes an /// 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> { let (owner, name) = crate::client::split_repo(repo)?; 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 // client used) so a heavily re-reviewed PR doesn't truncate. let mut reviews = Vec::new(); @@ -291,7 +325,8 @@ pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result