diff --git a/internal/app/app.go b/internal/app/app.go index f982c34..7d68252 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -287,13 +287,27 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { if err != nil { a.printWarn("WARNING: Error getting currently requested owners for re-request: %v\n", err) } else { + // Get reviewers who have reviewed but not approved (requested changes or commented) + reviewedButNotApproved, err := a.client.GetReviewedButNotApproved() + if err != nil { + a.printWarn("WARNING: Error getting reviewed but not approved owners for re-request: %v\n", err) + } + currentlyRequestedSet := make(map[string]struct{}, len(currentlyRequestedOwners)) for _, owner := range currentlyRequestedOwners { currentlyRequestedSet[owner.Normalized()] = struct{}{} } + + reviewedButNotApprovedSet := make(map[string]struct{}, len(reviewedButNotApproved)) + for _, owner := range reviewedButNotApproved { + reviewedButNotApprovedSet[owner.Normalized()] = struct{}{} + } + ownersToReRequest := f.Filtered(allRequiredOwnerNames, func(owner codeowners.Slug) bool { - _, exists := currentlyRequestedSet[owner.Normalized()] - return !exists + _, isCurrentlyRequested := currentlyRequestedSet[owner.Normalized()] + _, hasReviewedButNotApproved := reviewedButNotApprovedSet[owner.Normalized()] + // Exclude if already requested or if a member has reviewed but not approved + return !isCurrentlyRequested && !hasReviewedButNotApproved }) if len(ownersToReRequest) > 0 { a.printDebug("Re-requesting Reviews from satisfied team(s) to meet min_reviews: %s\n", codeowners.OriginalStrings(ownersToReRequest)) diff --git a/internal/github/gh.go b/internal/github/gh.go index bd14108..982f57a 100644 --- a/internal/github/gh.go +++ b/internal/github/gh.go @@ -39,6 +39,7 @@ type Client interface { FindUserApproval(ghUser string) (*CurrentApproval, error) GetCurrentReviewerApprovals() ([]*CurrentApproval, error) GetAlreadyReviewed() ([]codeowners.Slug, error) + GetReviewedButNotApproved() ([]codeowners.Slug, error) GetCurrentlyRequested() ([]codeowners.Slug, error) DismissStaleReviews(staleApprovals []*CurrentApproval) error RequestReviewers(reviewers []string) error @@ -322,6 +323,52 @@ func reviewerAlreadyReviewed(reviews []*github.PullRequestReview, userReviewerMa return slices.Collect(maps.Values(reviewsReviewers)) } +func (gh *GHClient) GetReviewedButNotApproved() ([]codeowners.Slug, error) { + if gh.pr == nil { + return nil, &NoPRError{} + } + if gh.userReviewerMap == nil { + return nil, &UserReviewerMapNotInitError{} + } + if gh.reviews == nil { + err := gh.InitReviews() + if err != nil { + return nil, err + } + } + return reviewerReviewedButNotApproved(gh.reviews, gh.userReviewerMap), nil +} + +func reviewerReviewedButNotApproved(reviews []*github.PullRequestReview, userReviewerMap ghUserReviewerMap) []codeowners.Slug { + // Track the most recent review state for each user + // Since reviews are in descending chronological order (most recent first), + // the first occurrence of each user is their most recent review + userReviewStates := make(map[string]string) + for _, review := range reviews { + reviewingUser := strings.ToLower(review.GetUser().GetLogin()) + // Only track if we haven't seen this user yet (most recent review) + if _, seen := userReviewStates[reviewingUser]; !seen { + state := review.GetState() + // Only track non-approved states (CHANGES_REQUESTED or COMMENTED) + if state == github.ReviewStateChangesRequested || state == github.ReviewStateCommented { + userReviewStates[reviewingUser] = state + } + } + } + + reviewsReviewers := make(map[string]codeowners.Slug) + for reviewingUserLower := range userReviewStates { + // Include reviewers whose most recent review is CHANGES_REQUESTED or COMMENTED + if reviewers, ok := userReviewerMap[reviewingUserLower]; ok { + for _, reviewer := range reviewers { + reviewsReviewers[reviewer.Normalized()] = reviewer + } + } + } + + return slices.Collect(maps.Values(reviewsReviewers)) +} + func (gh *GHClient) GetCurrentlyRequested() ([]codeowners.Slug, error) { if gh.pr == nil { return nil, &NoPRError{}