fix(#2327): handle dismissed reviews like stale (superseded helper)

This commit is contained in:
damocles 2026-07-10 15:15:05 +02:00 committed by mara
commit ce909a00a2
3 changed files with 38 additions and 16 deletions

View file

@ -121,17 +121,30 @@ pub(crate) fn pct_encode(s: &str) -> String {
out out
} }
/// One reviewer's latest verdict on a PR, plus forgejo's `stale` bit. /// One reviewer's latest verdict on a PR, plus forgejo's `stale` /
/// `dismissed` bits.
/// ///
/// `stale` is set by forgejo when the PR head commit changed after this /// `stale` is set by forgejo when the PR head commit changed after this
/// review was submitted — branch protection then wants a fresh review, so /// review was submitted (branch protection then wants a fresh review);
/// a `stale` APPROVED no longer satisfies the merge gate even though its /// `dismissed` is set when the review was explicitly dismissed. Either way
/// `state` string still reads `APPROVED`. Surfacing it stops the CLI from /// the review no longer applies to the current head even though its `state`
/// reporting a stale-but-technically-approved review as still-good. /// 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.
pub(crate) struct ReviewInfo { pub(crate) struct ReviewInfo {
pub login: String, pub login: String,
pub state: String, pub state: String,
pub stale: bool, pub stale: bool,
pub dismissed: bool,
}
impl ReviewInfo {
/// True when the review no longer applies to the current head — stale
/// (head moved) or dismissed. Such a verdict neither blocks a merge nor
/// counts as a fresh approval.
pub(crate) fn superseded(&self) -> bool {
self.stale || self.dismissed
}
} }
/// Latest non-comment review per reviewer on a PR. Reviews come /// Latest non-comment review per reviewer on a PR. Reviews come
@ -173,14 +186,17 @@ pub(crate) fn latest_reviews(client: &Client, repo: &str, pr: u64) -> Result<Vec
continue; continue;
} }
let stale = r.stale.unwrap_or(false); let stale = r.stale.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);
slot.stale = stale; slot.stale = stale;
slot.dismissed = dismissed;
} else { } else {
latest.push(ReviewInfo { latest.push(ReviewInfo {
login: login.to_owned(), login: login.to_owned(),
state: st.to_owned(), state: st.to_owned(),
stale, stale,
dismissed,
}); });
} }
} }

View file

@ -157,12 +157,12 @@ fn check_ready(client: &Client, repo: &str, number: u64, pull: &PullRequest) ->
// Block only on reviewers whose *current* verdict requests changes // Block only on reviewers whose *current* verdict requests changes
// (latest-per-reviewer, so a later APPROVED clears an earlier // (latest-per-reviewer, so a later APPROVED clears an earlier
// REQUEST_CHANGES). A stale REQUEST_CHANGES was made against an older // REQUEST_CHANGES). A superseded REQUEST_CHANGES — stale (older head) or
// head and no longer applies, so it doesn't block. Shares the verdict // dismissed — no longer applies, so it doesn't block. Shares the verdict
// logic with `pr-status`. // logic with `pr-status`.
let blockers: Vec<String> = super::latest_reviews(client, repo, number)? let blockers: Vec<String> = super::latest_reviews(client, repo, number)?
.into_iter() .into_iter()
.filter(|r| r.state == "REQUEST_CHANGES" && !r.stale) .filter(|r| r.state == "REQUEST_CHANGES" && !r.superseded())
.map(|r| r.login) .map(|r| r.login)
.collect(); .collect();
if !blockers.is_empty() { if !blockers.is_empty() {

View file

@ -120,7 +120,10 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> {
"requested_reviewers": requested, "requested_reviewers": requested,
"reviews": reviews "reviews": reviews
.iter() .iter()
.map(|r| serde_json::json!({"user": r.login, "state": r.state, "stale": r.stale})) .map(|r| serde_json::json!({
"user": r.login, "state": r.state,
"stale": r.stale, "dismissed": r.dismissed,
}))
.collect::<Vec<_>>(), .collect::<Vec<_>>(),
"last_comment": last "last_comment": last
.as_ref() .as_ref()
@ -143,12 +146,12 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> {
} }
// Merge-readiness: CI green, mergeable, and nobody requesting changes. // Merge-readiness: CI green, mergeable, and nobody requesting changes.
// A *stale* REQUEST_CHANGES was made against an older head — the branch // A *superseded* REQUEST_CHANGES — stale (branch moved since) or
// moved since, so forgejo marks it stale and it no longer blocks the // dismissed — no longer applies to the current head, so it doesn't hold
// current head; don't let it hold up the verdict. // up the verdict.
let changes_requested = reviews let changes_requested = reviews
.iter() .iter()
.any(|r| r.state == "REQUEST_CHANGES" && !r.stale); .any(|r| r.state == "REQUEST_CHANGES" && !r.superseded());
let ready = ci_state == "success" && mergeable == Some(true) && !changes_requested; let ready = ci_state == "success" && mergeable == Some(true) && !changes_requested;
if ready { if ready {
Ok(()) Ok(())
@ -315,9 +318,10 @@ fn print_pr(
.iter() .iter()
.map(|r| { .map(|r| {
let mark = match r.state.as_str() { let mark = match r.state.as_str() {
// A stale approval no longer satisfies branch protection; // A superseded approval (stale/dismissed) no longer
// flag it so the CLI doesn't read as still-good. // satisfies branch protection; flag it so the CLI doesn't
"APPROVED" if r.stale => "⚠️", // read as still-good.
"APPROVED" if r.superseded() => "⚠️",
"APPROVED" => "", "APPROVED" => "",
"REQUEST_CHANGES" => "", "REQUEST_CHANGES" => "",
_ => "", _ => "",
@ -326,6 +330,8 @@ fn print_pr(
let state = &r.state; let state = &r.state;
let suffix = if r.stale { let suffix = if r.stale {
" (stale — needs re-review on current head)" " (stale — needs re-review on current head)"
} else if r.dismissed {
" (dismissed)"
} else { } else {
"" ""
}; };