diff --git a/hive-forge/src/verbs/mod.rs b/hive-forge/src/verbs/mod.rs index 38050701..5a12797b 100644 --- a/hive-forge/src/verbs/mod.rs +++ b/hive-forge/src/verbs/mod.rs @@ -237,14 +237,6 @@ 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, @@ -261,26 +253,6 @@ 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; - }; - // Both emptiness checks matter, and for the same reason: a blank string - // is a value the forge sent, not a sha it has. Treating one as real would - // make every review compare unequal and mark the whole PR stale — the - // direction this whole function exists to avoid. - !head.is_empty() && !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 @@ -294,16 +266,6 @@ fn reviewed_older_head(reviewed_sha: Option<&str>, head_sha: Option<&str>) -> bo 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(); @@ -329,8 +291,7 @@ pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result