From dfbba151fa648db0b64a3fff634173acf9ea9e1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 24 Sep 2026 17:53:38 +0200 Subject: [PATCH] Upgrade tentative approvals if the PR was already fully approved --- src/bors/approval.rs | 21 +++++++--- src/bors/comment.rs | 37 ++++++++++++---- src/bors/handlers/review.rs | 84 ++++++++++++++++++++++++++++++------- 3 files changed, 112 insertions(+), 30 deletions(-) diff --git a/src/bors/approval.rs b/src/bors/approval.rs index f4cfb3a2..1ed89675 100644 --- a/src/bors/approval.rs +++ b/src/bors/approval.rs @@ -18,6 +18,15 @@ pub(super) fn check_unknown_reviewers(repo: &RepositoryState, approvers: &str) - .collect() } +/// Note that can be attached to an approval comment. +pub enum ApprovalNote { + /// The pull request was force approved while its PR CI is failing. + PrCiIsFailing, + /// A previously fully approved PR was approved again *tentatively*. + /// This tentative approval was automatically upgraded to a full approval. + TentativeApprovalUpgraded, +} + /// Finalize an approval and prepare the pull request for the merge queue. /// /// Clears any failed auto build so it can be retried, wakes the merge queue, posts the approval @@ -30,17 +39,17 @@ pub(super) async fn finalize_approval( approver: &str, priority: Option, merge_queue_tx: &MergeQueueSender, - failed_pr_ci: bool, + note: Option, ) -> anyhow::Result<()> { let unknown_reviewers = check_unknown_reviewers(repo, approver); - let was_failed = pr + let had_failed_auto_build = pr .db .auto_build .as_ref() .map(|b| b.status.is_failure()) .unwrap_or(false); // Re-approval should act as a retry - if was_failed { + if had_failed_auto_build { ctx.db.clear_auto_build(pr.db).await?; } @@ -74,8 +83,8 @@ pub(super) async fn finalize_approval( approver, unknown_reviewers, tree_state, - was_failed, - failed_pr_ci, + had_failed_auto_build, + note, ), &ctx.db, ) @@ -142,6 +151,6 @@ pub(super) async fn try_resolve_tentative_approval( } ctx.db.confirm_tentative_approval(pr.db).await?; - finalize_approval(ctx, repo, pr, approver, priority, merge_queue_tx, false).await?; + finalize_approval(ctx, repo, pr, approver, priority, merge_queue_tx, None).await?; Ok(TentativeApprovalOutcome::Resolved) } diff --git a/src/bors/comment.rs b/src/bors/comment.rs index cfb17608..23c10b89 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -1,3 +1,4 @@ +use crate::bors::approval::ApprovalNote; use crate::bors::command::CommandPrefix; use crate::bors::{FailedWorkflowRun, WorkflowRun}; use crate::database::PullRequestModel; @@ -296,8 +297,8 @@ pub fn approved_comment( reviewer: &str, unknown_reviewers: Vec, tree_state: TreeState, - was_failed: bool, - failed_pr_ci: bool, + had_failed_auto_build: bool, + note: Option, ) -> Comment { let approve_emoji = if is_holiday_season() { "star2" @@ -312,7 +313,7 @@ It is now in the [queue]({web_url}/queue/{}) for this repository. repo.name() ); - if was_failed { + if had_failed_auto_build { writeln!( comment, "\nA failed build status on this PR was cleared due to the approval." @@ -320,12 +321,30 @@ It is now in the [queue]({web_url}/queue/{}) for this repository. .unwrap(); } - if failed_pr_ci { - writeln!( - comment, - "\n> [!WARNING]\n> This PR was force-approved despite failing PR CI." - ) - .unwrap(); + if let Some(note) = note { + match note { + ApprovalNote::PrCiIsFailing => { + writeln!( + comment, + "\n> [!WARNING]\n> This PR was force-approved despite failing PR CI." + ) + .unwrap(); + } + ApprovalNote::TentativeApprovalUpgraded => { + writeln!( + comment, + r#" +> [!WARNING] +> This PR was already fully approved previously, so this tentative approval was treated as a full approval. If you want to instead downgrade the PR to be only tentatively approved, unapprove it first and then re-approve it again: +> ``` +> @bors r- +> @bors r+ +> ``` +"# + ) + .unwrap(); + } + } } if !unknown_reviewers.is_empty() { diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index baf806ef..cd2d5c0c 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -1,6 +1,6 @@ use crate::bors::RepositoryState; use crate::bors::approval::{ - TentativeApprovalOutcome, check_unknown_reviewers, finalize_approval, + ApprovalNote, TentativeApprovalOutcome, check_unknown_reviewers, finalize_approval, try_resolve_tentative_approval, }; use crate::bors::command::{Approver, CommandPrefix, Delegatee}; @@ -84,6 +84,18 @@ pub(super) async fn command_approve( sha: pr.github.head.sha.to_string(), }; + // It is possible that the PR was already (fully) approved before. + // If we are now doing a tentative approval, we will "upgrade" it to a full approval, which is + // usually what the user wants. + // This situation should be very rare anyway. + let already_approved = pr.db.is_approved(); + let (approval_mode, approval_upgraded) = + if already_approved && matches!(approval_mode, ApprovalMode::Tentative) { + (ApprovalMode::Eager, true) + } else { + (approval_mode, false) + }; + db.approve( pr.db, approval_info, @@ -130,18 +142,27 @@ pub(super) async fn command_approve( Ok(()) } ApprovalMode::Eager => { - let failed_pr_ci = match repo_state - .client - .get_workflow_runs_for_commit_sha(WorkflowSource::PullRequest(pr.github)) - .await - { - Ok(runs) => runs.iter().any(|run| run.status == WorkflowStatus::Failure), - Err(error) => { - tracing::warn!( - "Failed to get pull request CI status for commit {}: {error:?}", - pr.github.head.sha - ); - false + let note = if approval_upgraded { + Some(ApprovalNote::TentativeApprovalUpgraded) + } else { + let pr_ci_fails = match repo_state + .client + .get_workflow_runs_for_commit_sha(WorkflowSource::PullRequest(pr.github)) + .await + { + Ok(runs) => runs.iter().any(|run| run.status == WorkflowStatus::Failure), + Err(error) => { + tracing::warn!( + "Failed to get pull request CI status for commit {}: {error:?}", + pr.github.head.sha + ); + false + } + }; + if pr_ci_fails { + Some(ApprovalNote::PrCiIsFailing) + } else { + None } }; finalize_approval( @@ -151,7 +172,7 @@ pub(super) async fn command_approve( &approver, priority, merge_queue_tx, - failed_pr_ci, + note, ) .await } @@ -2302,7 +2323,7 @@ labels_blocking_approval = ["proposed-final-comment-period", "final-comment-peri ctx.start_auto_build(()).await?; ctx.workflow_full_failure(ctx.auto_workflow()).await?; ctx.expect_comments((), 1).await; // build failed - ctx.post_comment("@bors r+").await?; + ctx.post_comment("@bors r+ force").await?; insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @" :pushpin: Commit pr-1-sha has been approved by `default-user` @@ -2316,4 +2337,37 @@ labels_blocking_approval = ["proposed-final-comment-period", "final-comment-peri }) .await; } + + #[sqlx::test(migrator = "crate::MIGRATOR")] + async fn tentative_approval_upgrade(pool: sqlx::PgPool) { + run_test(pool, async |ctx: &mut BorsTester| { + // Full approval + ctx.post_comment("@bors r+ force").await?; + ctx.expect_comments((), 1).await; + + // Cause PR CI to fail + ctx.pr_workflow_failure(ctx.pr_ci_workflow(())).await?; + + // Tentative approval + ctx.post_comment("@bors r+").await?; + insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @" + :pushpin: Commit pr-1-sha has been approved by `default-user` + + It is now in the [queue](https://bors-test.com/queue/borstest) for this repository. + + > [!WARNING] + > This PR was already fully approved previously, so this tentative approval was treated as a full approval. If you want to instead downgrade the PR to be only tentatively approved, unapprove it first and then re-approve it again: + > ``` + > @bors r- + > @bors r+ + > ``` + "); + + // The tentative approval should not unapprove the PR + ctx.pr(()).await.expect_approved_by(&User::default_pr_author().name); + + Ok(()) + }) + .await; + } }