fix(comment): resolve inaccurate comment checks preventing merge

pr-cli/pkg/handler/pr_handler.go

@@ -149,6 +149,9 @@ func (h *PRHandler) GetCommentsWithCache() ([]git.Comment, error) {
 
 	h.commentsCache = comments
 	h.Debugf("Cached comments (%d comments)", len(h.commentsCache))
+	for i := range comments {
+		h.Debugf("Comment %d by %d %+v: %s", i+1, comments[i].ID, comments[i].User, comments[i].Body)
+	}
 	return comments, nil
 }
 

pr-cli/pkg/platforms/github/client.go

@@ -677,15 +677,19 @@ func (c *Client) processReviewVotes(lgtmUsers map[string]string) (map[string]*gi
 	if err != nil {
 		return nil, fmt.Errorf("failed to get reviews: %w", err)
 	}
-
-	userLatestReviews := c.findLatestReviews(reviews)
-	c.addApprovedReviews(lgtmUsers, userLatestReviews)
+	userLatestReviews := c.findLatestActionableReviews(reviews)
+	for i := range userLatestReviews {
+		review := userLatestReviews[i]
+		c.Logger.Debugf("Latest actionable review for %s: state=%s submitted_at=%s", review.User.Login, review.State, review.SubmittedAt)
+	}
+	userApprovedReviews := c.findEffectiveApprovals(reviews)
+	c.addApprovedReviews(lgtmUsers, userLatestReviews, userApprovedReviews)
 
 	return userLatestReviews, nil
 }
 
-// findLatestReviews finds the latest review for each user
-func (c *Client) findLatestReviews(reviews []git.Review) map[string]*git.Review {
+// findLatestActionableReviews finds the latest review with actionable state for each user
+func (c *Client) findLatestActionableReviews(reviews []git.Review) map[string]*git.Review {
 	userLatestReviews := make(map[string]*git.Review)
 
 	for i := range reviews {
@@ -696,6 +700,11 @@ func (c *Client) findLatestReviews(reviews []git.Review) map[string]*git.Review
 			continue
 		}
 
+		if !isActionableReviewState(review.State) {
+			c.Debugf("Skipping non-actionable review from user: %s (state: %s)", user, review.State)
+			continue
+		}
+
 		if existingReview, exists := userLatestReviews[user]; !exists || review.SubmittedAt > existingReview.SubmittedAt {
 			userLatestReviews[user] = review
 		}
@@ -704,14 +713,65 @@ func (c *Client) findLatestReviews(reviews []git.Review) map[string]*git.Review
 	return userLatestReviews
 }
 
+func isActionableReviewState(state string) bool {
+	switch strings.ToUpper(state) {
+	case "APPROVED", "CHANGES_REQUESTED", "REQUEST_CHANGES", "DISMISSED":
+		return true
+	default:
+		return false
+	}
+}
+
 // addApprovedReviews adds users who have approved via reviews
-func (c *Client) addApprovedReviews(lgtmUsers map[string]string, userLatestReviews map[string]*git.Review) {
+func (c *Client) addApprovedReviews(lgtmUsers map[string]string, userLatestReviews map[string]*git.Review, userApprovedReviews map[string]*git.Review) {
 	for user, latestReview := range userLatestReviews {
-		if latestReview.State == "APPROVED" {
+		if approval, approved := userApprovedReviews[user]; approved {
 			lgtmUsers[user] = ""
-			c.Debugf("Found APPROVED from user: %s (latest review)", user)
+			if approval == latestReview {
+				c.Debugf("Found APPROVED from user: %s (latest review)", user)
+			} else {
+				c.Debugf("Found APPROVED from user: %s (retained approval submitted_at=%s despite latest state: %s)", user, approval.SubmittedAt, latestReview.State)
+			}
+			continue
+		}
+		c.Debugf("Ignoring latest review from user: %s (state: %s)", user, latestReview.State)
+	}
+}
+
+// findEffectiveApprovals determines the latest non-revoked approval for each user
+func (c *Client) findEffectiveApprovals(reviews []git.Review) map[string]*git.Review {
+	latestApprovals := make(map[string]*git.Review)
+	latestRevocations := make(map[string]*git.Review)
+
+	for i := range reviews {
+		review := &reviews[i]
+		user := strings.ToLower(review.User.Login)
+
+		if strings.EqualFold(user, c.prSender) {
+			continue
+		}
+
+		switch {
+		case strings.EqualFold(review.State, "APPROVED"):
+			if existing, exists := latestApprovals[user]; !exists || review.SubmittedAt > existing.SubmittedAt {
+				latestApprovals[user] = review
+			}
+		case strings.EqualFold(review.State, "CHANGES_REQUESTED"), strings.EqualFold(review.State, "REQUEST_CHANGES"), strings.EqualFold(review.State, "DISMISSED"):
+			if existing, exists := latestRevocations[user]; !exists || review.SubmittedAt > existing.SubmittedAt {
+				latestRevocations[user] = review
+			}
+		}
+	}
+
+	effectiveApprovals := make(map[string]*git.Review)
+	for user, approval := range latestApprovals {
+		if revocation, revoked := latestRevocations[user]; revoked && revocation.SubmittedAt >= approval.SubmittedAt {
+			continue
 		}
+		effectiveApprovals[user] = approval
 	}
+
+	return effectiveApprovals
 }
 
 // processCommentVotesWithComments processes LGTM commands from provided or fetched comments

pr-cli/pkg/platforms/github/review_approvals_test.go

@@ -0,0 +1,106 @@
+package github
+
+import (
+	"sort"
+	"strings"
+
+	pkgtesting "github.com/AlaudaDevops/pkg/testing"
+	"github.com/AlaudaDevops/toolbox/pr-cli/pkg/git"
+	"github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"github.com/sirupsen/logrus"
+)
+
+type reviewFixture struct {
+	Description string             `json:"description" yaml:"description"`
+	PRSender    string             `json:"pr_sender" yaml:"pr_sender"`
+	Reviews     []reviewFixtureRow `json:"reviews" yaml:"reviews"`
+}
+
+type reviewFixtureRow struct {
+	User        string `json:"user" yaml:"user"`
+	State       string `json:"state" yaml:"state"`
+	SubmittedAt string `json:"submitted_at" yaml:"submitted_at"`
+}
+
+type expectedApprovalsFixture struct {
+	Description   string   `json:"description" yaml:"description"`
+	ApprovedUsers []string `json:"approved_users" yaml:"approved_users"`
+}
+
+var _ = ginkgo.Describe("GitHub review approvals", func() {
+	ginkgo.Describe("effective approval aggregation", func() {
+		type testCase struct {
+			description  string
+			reviewsFile  string
+			expectedFile string
+		}
+
+		ginkgo.DescribeTable("collects LGTM users from reviews",
+			func(tc testCase) {
+				reviewData := reviewFixture{}
+				pkgtesting.MustLoadYaml(tc.reviewsFile, &reviewData)
+
+				expected := expectedApprovalsFixture{}
+				pkgtesting.MustLoadYaml(tc.expectedFile, &expected)
+
+				client := &Client{
+					Logger:   logrus.New(),
+					prSender: reviewData.PRSender,
+				}
+
+				reviews := make([]git.Review, len(reviewData.Reviews))
+				for i := range reviewData.Reviews {
+					row := reviewData.Reviews[i]
+					reviews[i] = git.Review{
+						User: git.User{
+							Login: row.User,
+						},
+						State:       row.State,
+						SubmittedAt: row.SubmittedAt,
+					}
+				}
+
+				latestReviews := client.findLatestActionableReviews(reviews)
+				approvedReviews := client.findEffectiveApprovals(reviews)
+
+				lgtmUsers := make(map[string]string)
+				client.addApprovedReviews(lgtmUsers, latestReviews, approvedReviews)
+
+				collectedUsers := make([]string, 0, len(lgtmUsers))
+				for user := range lgtmUsers {
+					collectedUsers = append(collectedUsers, user)
+				}
+				sort.Strings(collectedUsers)
+
+				expectedUsers := make([]string, len(expected.ApprovedUsers))
+				for i, user := range expected.ApprovedUsers {
+					expectedUsers[i] = strings.ToLower(user)
+				}
+				sort.Strings(expectedUsers)
+
+				Expect(collectedUsers).To(Equal(expectedUsers), "test case: %s", tc.description)
+			},
+			ginkgo.Entry("retains approval after comment", testCase{
+				description:  "should retain approval when the latest review is only a comment",
+				reviewsFile:  "testdata/review_effective_approvals/retain_after_comment/reviews.yaml",
+				expectedFile: "testdata/review_effective_approvals/retain_after_comment/expected.yaml",
+			}),
+			ginkgo.Entry("removes approval after changes requested", testCase{
+				description:  "should remove approval when changes are requested after approval",
+				reviewsFile:  "testdata/review_effective_approvals/revoked_by_changes/reviews.yaml",
+				expectedFile: "testdata/review_effective_approvals/revoked_by_changes/expected.yaml",
+			}),
+			ginkgo.Entry("restores approval when re-approved after changes", testCase{
+				description:  "should restore approval when a new approval follows requested changes",
+				reviewsFile:  "testdata/review_effective_approvals/reapprove_after_changes/reviews.yaml",
+				expectedFile: "testdata/review_effective_approvals/reapprove_after_changes/expected.yaml",
+			}),
+			ginkgo.Entry("removes approval after dismissal", testCase{
+				description:  "should remove approval when the latest review is dismissed",
+				reviewsFile:  "testdata/review_effective_approvals/revoked_by_dismissed/reviews.yaml",
+				expectedFile: "testdata/review_effective_approvals/revoked_by_dismissed/expected.yaml",
+			}),
+		)
+	})
+})

pr-cli/pkg/platforms/github/testdata/review_effective_approvals/reapprove_after_changes/expected.yaml

@@ -0,0 +1,4 @@
+description: approval is valid again after the second approval
+approved_users:
+
+  - reviewer-b

pr-cli/pkg/platforms/github/testdata/review_effective_approvals/reapprove_after_changes/reviews.yaml

@@ -0,0 +1,15 @@
+description: restore approval when a new approval is submitted after requested changes
+pr_sender: author
+reviews:
+
+  - user: reviewer-b
+    state: APPROVED
+    submitted_at: 2025-10-24T08:00:00Z
+
+  - user: reviewer-b
+    state: CHANGES_REQUESTED
+    submitted_at: 2025-10-24T08:10:00Z
+
+  - user: reviewer-b
+    state: APPROVED
+    submitted_at: 2025-10-24T08:20:00Z

pr-cli/pkg/platforms/github/testdata/review_effective_approvals/retain_after_comment/expected.yaml

@@ -0,0 +1,4 @@
+description: approver should remain valid after a plain comment
+approved_users:
+
+  - kycheng

pr-cli/pkg/platforms/github/testdata/review_effective_approvals/retain_after_comment/reviews.yaml

@@ -0,0 +1,11 @@
+description: retain approval when only a comment follows the approval
+pr_sender: author
+reviews:
+
+  - user: kycheng
+    state: APPROVED
+    submitted_at: 2025-10-24T09:16:07Z
+
+  - user: kycheng
+    state: COMMENTED
+    submitted_at: 2025-10-24T09:20:38Z

pr-cli/pkg/platforms/github/testdata/review_effective_approvals/revoked_by_changes/expected.yaml

@@ -0,0 +1,2 @@
+description: no approvers remain after changes are requested
+approved_users: []

pr-cli/pkg/platforms/github/testdata/review_effective_approvals/revoked_by_changes/reviews.yaml

@@ -0,0 +1,15 @@
+description: remove approval when changes are requested after the approval
+pr_sender: author
+reviews:
+
+  - user: reviewer-a
+    state: APPROVED
+    submitted_at: 2025-10-24T09:00:00Z
+
+  - user: reviewer-a
+    state: COMMENTED
+    submitted_at: 2025-10-24T09:05:00Z
+
+  - user: reviewer-a
+    state: CHANGES_REQUESTED
+    submitted_at: 2025-10-24T09:10:00Z

pr-cli/pkg/platforms/github/testdata/review_effective_approvals/revoked_by_dismissed/expected.yaml

@@ -0,0 +1,2 @@
+description: the dismissal should remove the approver
+approved_users: []

pr-cli/pkg/platforms/github/testdata/review_effective_approvals/revoked_by_dismissed/reviews.yaml

@@ -0,0 +1,11 @@
+description: remove approval when it is dismissed after submission
+pr_sender: author
+reviews:
+
+  - user: reviewer-c
+    state: APPROVED
+    submitted_at: 2025-10-24T07:00:00Z
+
+  - user: reviewer-c
+    state: DISMISSED
+    submitted_at: 2025-10-24T07:05:00Z