From 6b669f1dd5b6787036befacabbe0538cc84e2cedda243fcd357cc7f6fa065c13 Mon Sep 17 00:00:00 2001 From: Andrii Nikitin Date: Wed, 25 Feb 2026 12:33:30 +0100 Subject: [PATCH 1/2] fix: correctly handle multiple reviews from the same user --- common/reviews.go | 16 ++++++++-------- integration/test-plan.md | 4 ++-- integration/tests/workflow_pr_merge_test.py | 1 - integration/tests/workflow_pr_review_test.py | 2 -- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/common/reviews.go b/common/reviews.go index 5da3c25..adeecd5 100644 --- a/common/reviews.go +++ b/common/reviews.go @@ -187,8 +187,6 @@ func (r *PRReviews) HasPendingReviewBy(reviewer string) bool { switch r.State { case ReviewStateRequestReview, ReviewStatePending: return true - default: - return false } } } @@ -201,18 +199,20 @@ func (r *PRReviews) IsReviewedBy(reviewer string) bool { return false } - for _, r := range r.Reviews { - if r.User.UserName == reviewer && !r.Stale { - switch r.State { + res := false + + for _, i := range r.Reviews { + if i.User.UserName == reviewer && !i.Stale { + switch i.State { case ReviewStateApproved, ReviewStateRequestChanges: - return true - default: + res = true + case ReviewStateRequestReview, ReviewStatePending: return false } } } - return false + return res } func (r *PRReviews) IsReviewedByOneOf(reviewers ...string) bool { diff --git a/integration/test-plan.md b/integration/test-plan.md index c9ae045..0a708de 100644 --- a/integration/test-plan.md +++ b/integration/test-plan.md @@ -65,11 +65,11 @@ The testing will be conducted in a dedicated test environment that mimics the pr | **TC-REVIEW-001** | P | **Add mandatory reviewers** | 1. Create a new PackageGit PR. | 1. All mandatory reviewers are added to both the PackageGit and ProjectGit PRs. | High | | **TC-REVIEW-002** | - | **Add advisory reviewers** | 1. Create a new PackageGit PR with advisory reviewers defined in the configuration. | 1. Advisory reviewers are added to the PR, but their approval is not required for merging. | Medium | | **TC-REVIEW-003** | - | **Re-add reviewers** | 1. Push a new commit to a PackageGit PR after it has been approved. | 1. The original reviewers are re-added to the PR. | Medium | -| **TC-REVIEW-004** | x | **Package PR created by a maintainer** | 1. Create a PackageGit PR from the account of a package maintainer. | 1. No review is requested from other package maintainers. | High | +| **TC-REVIEW-004** | P | **Package PR created by a maintainer** | 1. Create a PackageGit PR from the account of a package maintainer. | 1. No review is requested from other package maintainers. | High | | **TC-REVIEW-005** | P | **Package PR created by an external user (approve)** | 1. Create a PackageGit PR from the account of a user who is not a package maintainer.
2. One of the package maintainers approves the PR. | 1. All package maintainers are added as reviewers.
2. Once one maintainer approves the PR, the other maintainers are removed as reviewers. | High | | **TC-REVIEW-006** | P | **Package PR created by an external user (reject)** | 1. Create a PackageGit PR from the account of a user who is not a package maintainer.
2. One of the package maintainers rejects the PR. | 1. All package maintainers are added as reviewers.
2. Once one maintainer rejects the PR, the other maintainers are removed as reviewers. | High | | **TC-REVIEW-007** | P | **Package PR created by a maintainer with ReviewRequired=true** | 1. Set `ReviewRequired = true` in `workflow.config`.
2. Create a PackageGit PR from the account of a package maintainer. | 1. A review is requested from other package maintainers if available. | High | -| **TC-MERGE-001** | x | **Automatic Merge** | 1. Create a PackageGit PR.
2. Ensure all mandatory reviews are completed on both project and package PRs. | 1. The PR is automatically merged. | High | +| **TC-MERGE-001** | P | **Automatic Merge** | 1. Create a PackageGit PR.
2. Ensure all mandatory reviews are completed on both project and package PRs. | 1. The PR is automatically merged. | High | | **TC-MERGE-002** | - | **ManualMergeOnly with Package Maintainer** | 1. Create a PackageGit PR with `ManualMergeOnly` set to `true`.
2. Ensure all mandatory reviews are completed on both project and package PRs.
3. Comment "merge ok" on the package PR from the account of a package maintainer for that package. | 1. The PR is merged. | High | | **TC-MERGE-003** | - | **ManualMergeOnly with unauthorized user** | 1. Create a PackageGit PR with `ManualMergeOnly` set to `true`.
2. Ensure all mandatory reviews are completed on both project and package PRs.
3. Comment "merge ok" on the package PR from the account of a user who is not a maintainer for that package. | 1. The PR is not merged. | High | | **TC-MERGE-004** | - | **ManualMergeOnly with multiple packages** | 1. Create a ProjectGit PR that references multiple PackageGit PRs with `ManualMergeOnly` set to `true`.
2. Ensure all mandatory reviews are completed on both project and package PRs.
3. Comment "merge ok" on each package PR from the account of a package maintainer. | 1. The PR is merged only after "merge ok" is commented on all associated PackageGit PRs. | High | diff --git a/integration/tests/workflow_pr_merge_test.py b/integration/tests/workflow_pr_merge_test.py index 2d09c7e..d297ed8 100644 --- a/integration/tests/workflow_pr_merge_test.py +++ b/integration/tests/workflow_pr_merge_test.py @@ -5,7 +5,6 @@ from pathlib import Path from tests.lib.common_test_utils import GiteaAPIClient @pytest.mark.t001 -@pytest.mark.xfail(reason="The bot sometimes re-request reviews despite having all the approvals") def test_001_automerge(automerge_env, test_user_client): """ Test scenario: diff --git a/integration/tests/workflow_pr_review_test.py b/integration/tests/workflow_pr_review_test.py index a6f2d45..788dbfa 100644 --- a/integration/tests/workflow_pr_review_test.py +++ b/integration/tests/workflow_pr_review_test.py @@ -43,7 +43,6 @@ index 0000000..e69de29 @pytest.mark.t004 -@pytest.mark.xfail(reason="the bot sometimes re-requests review from autogits_obs_staging_bot despite having the approval") def test_004_maintainer(maintainer_env, ownerA_client): """ Test scenario: @@ -152,7 +151,6 @@ index 0000000..e69de29 @pytest.mark.t005 -# @pytest.mark.xfail(reason="TBD troubleshoot") def test_005_any_maintainer_approval_sufficient(maintainer_env, ownerA_client, ownerBB_client): """ Test scenario: -- 2.51.1 From 833b679c966316cb82904cceb8a200151fff3c21bb140728a99b13fe9a5044cf Mon Sep 17 00:00:00 2001 From: Andrii Nikitin Date: Wed, 25 Feb 2026 14:47:28 +0100 Subject: [PATCH 2/2] fix reviews may be ahead of timeline The bot may be confused if he has all the reviews, but some of them are missing on the timeline. This may happen if a new review arrives inbetween of gitea api call. Changing orders of API calls ensures that state of reviews is not ahead of timeline. --- common/reviews.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/common/reviews.go b/common/reviews.go index adeecd5..80737da 100644 --- a/common/reviews.go +++ b/common/reviews.go @@ -17,15 +17,16 @@ type PRReviews struct { } func FetchGiteaReviews(rf GiteaReviewTimelineFetcher, org, repo string, no int64) (*PRReviews, error) { + rawReviews, err := rf.GetPullRequestReviews(org, repo, no) + if err != nil { + return nil, err + } + timeline, err := rf.GetTimeline(org, repo, no) if err != nil { return nil, err } - rawReviews, err := rf.GetPullRequestReviews(org, repo, no) - if err != nil { - return nil, err - } reviews := make([]*models.PullReview, 0, 10) needNewReviews := []string{} -- 2.51.1