From fc0f31afffedb94845a78081cf654610fc031bd2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 17:13:35 +0000 Subject: [PATCH] Show re-review requests in the WaitingOnMe filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FilterWaitingOnMe included a personally-requested PR only when I had not acted since the last push or the last action by someone else. That test is exactly wrong for a re-review: GitHub clears a review request the moment the reviewer submits a review, and "re-request review" puts them back into RequestedReviewers — so on a re-request my prior review is newer than both the last commit and the last comment, and every re-requested PR was filtered out. A live entry in RequestedReviewers is itself proof that the request is outstanding, so being requested now includes the PR outright. Three other paths dropped PRs the same section should have surfaced: - A dismissed review (stale-review dismissal, CODEOWNERS re-request) throws away my approval without adding me back to RequestedReviewers, so nothing in the filter noticed I owed a re-review. InteractionState now carries MyReviewDismissed, computed from the state of my most recent review. - GetInteractionState fetched reviews, review comments, issue comments and commits with no pagination options — 30 entries each, oldest first. On an active PR my latest review, the reply waiting on me, and the newest commit all fell off the end, skewing every timestamp the state is built from. All four lists now page through (100 per page, 5 pages max), and the last push time scans for the maximum commit date instead of trusting the final element, which pagination and rebases both make unreliable. - GetPRs only ever reads the first two pages of a repo's open PRs. Under the default created-desc ordering, an older PR that just had a review re-requested never reached the filters at all; sorting by updated-desc makes the window track activity instead. FilterWaitingOnAuthor now excludes PRs where I have an open request or a dismissed review, so a re-requested PR does not land in both sections, and FilterWaitingOnMe logs its per-PR decision at debug level. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AbmVRyFX9qnTSoHkgodZBh --- docs/filters.md | 14 ++- git_tools/git_tools.go | 174 +++++++++++++++++++++++++------- git_tools/waiting_on_me_test.go | 171 ++++++++++++++++++++++++++++++- 3 files changed, 316 insertions(+), 43 deletions(-) diff --git a/docs/filters.md b/docs/filters.md index 9d75a93..44bc0a5 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -13,12 +13,24 @@ Each workflow can use the available filters to customize which PRs are included. * `FilterCIFailing` - Only include PRs with failing CI/Actions * `FilterStale` - PRs with no activity for more than 3 days * `FilterNotStale` - PRs with activity within the last 3 days -* `FilterWaitingOnMe` - PRs where you are a requested reviewer and need to act +* `FilterWaitingOnMe` - PRs that need action from you (see below) * `FilterWaitingOnAuthor` - PRs where you were the last to act and are waiting on the author * `FilterByLabel:` - Only include PRs with the specified label (e.g., `FilterByLabel:bug`) * `FilterByAuthor:` - Only include PRs authored by the specified user * `FilterExcludeAuthor:` - Exclude PRs authored by the specified user +## FilterWaitingOnMe + +A PR is included when any of the following is true: + +1. **You have an open review request.** GitHub clears a review request as soon as you submit a review, and re-requesting a review puts you back in the PR's requested reviewers — so being listed there means the request is outstanding, no matter how recently you reviewed or commented. This is what makes re-review requests show up. +2. **Your most recent review was dismissed.** Stale-review dismissal and CODEOWNERS re-requests throw away your approval *without* adding you back to the requested reviewers, so this is the only signal that you owe a re-review. +3. **You have unresponded comments.** You participated in a review thread (or the PR conversation) and someone else replied after you. + +`FilterWaitingOnAuthor` is the complement: it excludes PRs where you have an open request or a dismissed review, so a PR never lands in both sections. + +Only *personal* review requests count. If your review is requested through a team, use the `Teams` field described below to build a section for it. + ## Team-Based Filtering You can filter PRs by team reviewers by adding a `Teams` field to your workflow configuration. When `Teams` is specified, only PRs where one of those teams is requested as a reviewer will be included. Each workflow can specify its own list of teams, allowing different workflows to target different teams. diff --git a/git_tools/git_tools.go b/git_tools/git_tools.go index 3431cdd..a666c47 100644 --- a/git_tools/git_tools.go +++ b/git_tools/git_tools.go @@ -27,7 +27,18 @@ type PRFilter func([]*github.PullRequest) []*github.PullRequest func GetPRs(client *github.Client, state string, owner string, repo string) ([]*github.PullRequest, error) { per_page := 100 - options := github.PullRequestListOptions{State: state, ListOptions: github.ListOptions{PerPage: per_page, Page: 1}} + // Sort by recent activity, not creation date. We only ever look at the first + // max_additional_calls+1 pages, so on a repo with more open PRs than that + // window the ordering decides what is even eligible for filtering. A PR that + // just had a review re-requested has a fresh updated_at but may have been + // created months ago, and under the default created-desc ordering it fell off + // the end of the window and never reached the filters at all. + options := github.PullRequestListOptions{ + State: state, + Sort: "updated", + Direction: "desc", + ListOptions: github.ListOptions{PerPage: per_page, Page: 1}, + } // Use make so a successful fetch with 0 results returns a non-nil empty slice. // Callers use nil to detect fetch failure vs success (nil == failed, non-nil == succeeded). prs := make([]*github.PullRequest, 0) @@ -857,6 +868,11 @@ type InteractionState struct { LastOthersTime time.Time LastCommitTime time.Time HasUnrespondedComments bool + // MyReviewDismissed is true when the most recent review I submitted has been + // dismissed (stale-review dismissal, a CODEOWNERS re-request, or a manual + // dismissal). GitHub does not put me back in RequestedReviewers in that case, + // so this is the only signal that I owe a re-review. + MyReviewDismissed bool } func CalculateInteractionState(myLogin string, pr *github.PullRequest, reviews []*github.PullRequestReview, reviewComments []*github.PullRequestComment, issueComments []*github.IssueComment) InteractionState { @@ -867,11 +883,15 @@ func CalculateInteractionState(myLogin string, pr *github.PullRequest, reviews [ } // Fetch Reviews + var myLatestReview *github.PullRequestReview for _, r := range reviews { if r == nil || r.SubmittedAt == nil || r.User == nil || r.User.Login == nil { continue } if strings.EqualFold(*r.User.Login, myLogin) { + if myLatestReview == nil || r.SubmittedAt.After(myLatestReview.SubmittedAt.Time) { + myLatestReview = r + } if r.SubmittedAt.After(state.LastMeTime) { state.LastMeTime = r.SubmittedAt.Time } @@ -881,6 +901,7 @@ func CalculateInteractionState(myLogin string, pr *github.PullRequest, reviews [ } } } + state.MyReviewDismissed = myLatestReview != nil && myLatestReview.GetState() == "DISMISSED" // Fetch Review Comments // Group comments by their "thread" @@ -942,6 +963,56 @@ func CalculateInteractionState(myLogin string, pr *github.PullRequest, reviews [ return state } +// interactionPageSize / interactionMaxPages bound the paginated fetches behind +// GetInteractionState. GitHub caps a PR's commit list at 250 entries, and a PR +// with more than 500 comments is far past the point where any of this matters, +// so five pages is plenty while keeping the worst case at five calls per list. +const ( + interactionPageSize = 100 + interactionMaxPages = 5 +) + +// listAllPages walks the paginated endpoint fetch until it runs out of pages +// (or hits interactionMaxPages), returning everything it collected. On error it +// returns the pages gathered so far alongside the error, so callers can still +// work with a partial history rather than falling back to nothing. +func listAllPages[T any](fetch func(opts *github.ListOptions) ([]T, *github.Response, error)) ([]T, error) { + var all []T + opts := &github.ListOptions{PerPage: interactionPageSize, Page: 1} + for i := 0; i < interactionMaxPages; i++ { + page, resp, err := fetch(opts) + if err != nil { + return all, err + } + all = append(all, page...) + if resp == nil || resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage + } + return all, nil +} + +// latestCommitTime returns the newest commit timestamp in commits. It scans for +// the maximum rather than trusting the last element: pagination and rebases both +// hand back lists whose final entry is not the most recent commit, and an +// under-reported "last push" time makes a PR look like I have already responded +// to it. +func latestCommitTime(commits []*github.RepositoryCommit) time.Time { + var latest time.Time + for _, c := range commits { + if c == nil || c.Commit == nil { + continue + } + for _, ts := range []*github.CommitAuthor{c.Commit.Committer, c.Commit.Author} { + if ts != nil && ts.Date != nil && ts.Date.After(latest) { + latest = ts.Date.Time + } + } + } + return latest +} + func GetInteractionState(owner, repo string, pr *github.PullRequest) InteractionState { if pr == nil || pr.Number == nil { return InteractionState{} @@ -969,9 +1040,17 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction resultChan := make(chan result, 4) + // Every list below is paginated. GitHub's default page size is 30, and all of + // these endpoints return oldest-first, so a single unpaginated call on an + // active PR hands back only the *start* of its history: my latest review, the + // reply that is waiting on me, and the newest commit all fall off the end. + // That silently skews every timestamp this state is built from. + // Fetch Reviews go func() { - reviews, _, err := client.PullRequests.ListReviews(ctx, owner, repo, *pr.Number, nil) + reviews, err := listAllPages(func(opts *github.ListOptions) ([]*github.PullRequestReview, *github.Response, error) { + return client.PullRequests.ListReviews(ctx, owner, repo, *pr.Number, opts) + }) if err != nil { slog.Warn("Error listing reviews", "owner", owner, "repo", repo, "number", *pr.Number, "error", err) } @@ -980,7 +1059,10 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction // Fetch Review Comments go func() { - reviewComments, _, err := client.PullRequests.ListComments(ctx, owner, repo, *pr.Number, nil) + reviewComments, err := listAllPages(func(opts *github.ListOptions) ([]*github.PullRequestComment, *github.Response, error) { + return client.PullRequests.ListComments(ctx, owner, repo, *pr.Number, + &github.PullRequestListCommentsOptions{ListOptions: *opts}) + }) if err != nil { slog.Warn("Error listing review comments", "owner", owner, "repo", repo, "number", *pr.Number, "error", err) } @@ -989,7 +1071,10 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction // Fetch Issue Comments go func() { - issueComments, _, err := client.Issues.ListComments(ctx, owner, repo, *pr.Number, nil) + issueComments, err := listAllPages(func(opts *github.ListOptions) ([]*github.IssueComment, *github.Response, error) { + return client.Issues.ListComments(ctx, owner, repo, *pr.Number, + &github.IssueListCommentsOptions{ListOptions: *opts}) + }) if err != nil { slog.Warn("Error listing issue comments", "owner", owner, "repo", repo, "number", *pr.Number, "error", err) } @@ -998,7 +1083,9 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction // Fetch Commits go func() { - commits, _, err := client.PullRequests.ListCommits(ctx, owner, repo, *pr.Number, nil) + commits, err := listAllPages(func(opts *github.ListOptions) ([]*github.RepositoryCommit, *github.Response, error) { + return client.PullRequests.ListCommits(ctx, owner, repo, *pr.Number, opts) + }) if err != nil { slog.Warn("Error listing commits", "owner", owner, "repo", repo, "number", *pr.Number, "error", err) } @@ -1034,12 +1121,7 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction state := CalculateInteractionState(myLogin, pr, reviews, reviewComments, issueComments) // Set LastCommitTime from commits - if len(commits) > 0 { - latestCommit := commits[len(commits)-1] - if latestCommit.Commit != nil && latestCommit.Commit.Committer != nil && latestCommit.Commit.Committer.Date != nil { - state.LastCommitTime = latestCommit.Commit.Committer.Date.Time - } - } + state.LastCommitTime = latestCommitTime(commits) // Cache the result even if there were errors (with shorter TTL) // This prevents retry storms @@ -1079,6 +1161,19 @@ func GetMyTeams() []string { return slugs } +// reviewRequestedFrom reports whether login has an open personal review request +// on the PR. GitHub logins are case-insensitive, so the comparison uses +// EqualFold: a casing mismatch between the configured GithubUsername and the +// login returned by the API would otherwise match nobody. +func reviewRequestedFrom(pr *github.PullRequest, login string) bool { + for _, r := range pr.RequestedReviewers { + if r != nil && r.Login != nil && strings.EqualFold(*r.Login, login) { + return true + } + } + return false +} + func FilterWaitingOnMe(prs []*github.PullRequest) []*github.PullRequest { myLogin := config.C().GithubUsername @@ -1102,34 +1197,29 @@ func FilterWaitingOnMe(prs []*github.PullRequest) []*github.PullRequest { state := GetInteractionState(*pr.Base.Repo.Owner.Login, *pr.Base.Repo.Name, pr) - // Is Personally Requested? - isRequested := false - for _, r := range pr.RequestedReviewers { - // GitHub logins are case-insensitive, so compare with EqualFold: - // a casing mismatch between the configured GithubUsername and the - // login returned by the API would otherwise make isRequested - // always false and the filter would match nothing. - if r != nil && r.Login != nil && strings.EqualFold(*r.Login, myLogin) { - isRequested = true - break - } - } + isRequested := reviewRequestedFrom(pr, myLogin) // A PR is "Waiting on me" if: - // 1. I am individually requested AND I haven't acted since the last push or last other action. - // 2. OR I have unresponded comments. - shouldFilter := false - - if isRequested { - actedSinceOthers := !state.LastMeTime.IsZero() && state.LastMeTime.After(state.LastOthersTime) && state.LastMeTime.After(state.LastCommitTime) - if !actedSinceOthers { - shouldFilter = true - } - } - - if state.HasUnrespondedComments { - shouldFilter = true - } + // 1. I have a pending review request on it, OR + // 2. my last review was dismissed (so I owe a re-review), OR + // 3. I have unresponded comments. + // + // Case 1 is deliberately unconditional. GitHub clears a review request + // the moment the requested reviewer submits a review, and "re-request + // review" puts them back into RequestedReviewers — so a login sitting in + // RequestedReviewers *now* always means an outstanding request, however + // recently that person reviewed or commented. This used to be gated on + // "have I acted since the last push / last other action", which hid + // exactly the re-review case: the prior review is newer than both the + // last commit and the last comment, so every re-request was filtered out. + shouldFilter := isRequested || state.MyReviewDismissed || state.HasUnrespondedComments + + slog.Debug("FilterWaitingOnMe", + "repo", *pr.Base.Repo.Name, "number", *pr.Number, "included", shouldFilter, + "requested", isRequested, "review_dismissed", state.MyReviewDismissed, + "unresponded_comments", state.HasUnrespondedComments, + "last_me", state.LastMeTime, "last_others", state.LastOthersTime, + "last_commit", state.LastCommitTime) resultChan <- prResult{pr: pr, shouldFilter: shouldFilter} }(pr) @@ -1153,6 +1243,8 @@ func FilterWaitingOnMe(prs []*github.PullRequest) []*github.PullRequest { } func FilterWaitingOnAuthor(prs []*github.PullRequest) []*github.PullRequest { + myLogin := config.C().GithubUsername + // Process PRs concurrently to avoid sequential API calls type prResult struct { pr *github.PullRequest @@ -1174,7 +1266,13 @@ func FilterWaitingOnAuthor(prs []*github.PullRequest) []*github.PullRequest { state := GetInteractionState(*pr.Base.Repo.Owner.Login, *pr.Base.Repo.Name, pr) actedSinceOthers := !state.LastMeTime.IsZero() && state.LastMeTime.After(state.LastOthersTime) && state.LastMeTime.After(state.LastCommitTime) - resultChan <- prResult{pr: pr, shouldFilter: actedSinceOthers} + // An open review request on me — or a dismissal of my last review — + // outranks the timestamps: the ball is back in my court, not the + // author's. Without this a re-requested PR would show up in both the + // "Waiting on Me" and "Waiting on Author" sections. + owedByMe := reviewRequestedFrom(pr, myLogin) || state.MyReviewDismissed + + resultChan <- prResult{pr: pr, shouldFilter: actedSinceOthers && !owedByMe} }(pr) } diff --git a/git_tools/waiting_on_me_test.go b/git_tools/waiting_on_me_test.go index 085d738..5080750 100644 --- a/git_tools/waiting_on_me_test.go +++ b/git_tools/waiting_on_me_test.go @@ -54,14 +54,17 @@ func TestFilterWaitingOnMe(t *testing.T) { shouldInclude: true, }, { - name: "Personally requested, I acted after push", + // A pending review request outlives whatever I did before it: GitHub + // removes the request when I submit a review, so a login still sitting + // in RequestedReviewers means the request was (re-)made and is open. + name: "Personally requested with a newer prior review (re-review request)", pr: makePR(2, myLogin), state: InteractionState{ LastMeTime: time.Now().Add(-10 * time.Minute), LastOthersTime: time.Now().Add(-20 * time.Minute), LastCommitTime: time.Now().Add(-30 * time.Minute), }, - shouldInclude: false, + shouldInclude: true, }, { name: "Not personally requested, but unresponded comments", @@ -80,13 +83,36 @@ func TestFilterWaitingOnMe(t *testing.T) { shouldInclude: false, }, { - name: "Personally requested, but I acted after others", + name: "Personally requested, and I commented since (request still open)", pr: makePR(5, myLogin), state: InteractionState{ LastMeTime: time.Now().Add(-5 * time.Minute), LastOthersTime: time.Now().Add(-10 * time.Minute), LastCommitTime: time.Now().Add(-20 * time.Minute), }, + shouldInclude: true, + }, + { + // Stale-review dismissal (CODEOWNERS, "dismiss stale approvals on push") + // drops my approval without adding me back to RequestedReviewers. + name: "Not requested, but my latest review was dismissed", + pr: makePR(8, "other"), + state: InteractionState{ + LastMeTime: time.Now().Add(-5 * time.Minute), + LastOthersTime: time.Now().Add(-10 * time.Minute), + LastCommitTime: time.Now().Add(-20 * time.Minute), + MyReviewDismissed: true, + }, + shouldInclude: true, + }, + { + name: "Not requested, my review stands, nothing outstanding", + pr: makePR(9, "other"), + state: InteractionState{ + LastMeTime: time.Now().Add(-5 * time.Minute), + LastOthersTime: time.Now().Add(-10 * time.Minute), + LastCommitTime: time.Now().Add(-20 * time.Minute), + }, shouldInclude: false, }, { @@ -392,6 +418,115 @@ func TestCalculateInteractionState(t *testing.T) { } } +// TestCalculateInteractionStateReviewDismissed covers the re-review signal that +// never reaches RequestedReviewers: a dismissed review means my verdict was +// thrown away and the PR is waiting on me again, while a later review of any +// other state means I have already weighed in since. +func TestCalculateInteractionStateReviewDismissed(t *testing.T) { + myLogin := "myself" + now := time.Now() + + makeReview := func(user, state string, submittedAt time.Time) *github.PullRequestReview { + return &github.PullRequestReview{ + User: &github.User{Login: github.String(user)}, + State: github.String(state), + SubmittedAt: &github.Timestamp{Time: submittedAt}, + } + } + + tests := []struct { + name string + reviews []*github.PullRequestReview + expectDismissed bool + }{ + { + name: "No reviews", + expectDismissed: false, + }, + { + name: "My approval still stands", + reviews: []*github.PullRequestReview{makeReview(myLogin, "APPROVED", now.Add(-time.Hour))}, + expectDismissed: false, + }, + { + name: "My latest review was dismissed", + reviews: []*github.PullRequestReview{makeReview(myLogin, "DISMISSED", now.Add(-time.Hour))}, + expectDismissed: true, + }, + { + name: "Dismissed, then I reviewed again", + reviews: []*github.PullRequestReview{ + makeReview(myLogin, "DISMISSED", now.Add(-2*time.Hour)), + makeReview(myLogin, "APPROVED", now.Add(-time.Hour)), + }, + expectDismissed: false, + }, + { + name: "Someone else's review was dismissed", + reviews: []*github.PullRequestReview{ + makeReview("other", "DISMISSED", now.Add(-time.Hour)), + }, + expectDismissed: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + state := CalculateInteractionState(myLogin, &github.PullRequest{}, tt.reviews, nil, nil) + if state.MyReviewDismissed != tt.expectDismissed { + t.Errorf("MyReviewDismissed: expected %v, got %v", tt.expectDismissed, state.MyReviewDismissed) + } + }) + } +} + +// TestLatestCommitTime guards the "last push" timestamp against list orderings +// where the newest commit is not the final element — force-pushes and rebases +// both produce them, and an under-reported push time makes a PR look answered. +func TestLatestCommitTime(t *testing.T) { + now := time.Now().Truncate(time.Second) + + commit := func(committed time.Time) *github.RepositoryCommit { + return &github.RepositoryCommit{ + Commit: &github.Commit{ + Committer: &github.CommitAuthor{Date: &github.Timestamp{Time: committed}}, + }, + } + } + + tests := []struct { + name string + commits []*github.RepositoryCommit + expected time.Time + }{ + {name: "No commits", expected: time.Time{}}, + { + name: "Newest commit is last", + commits: []*github.RepositoryCommit{commit(now.Add(-2 * time.Hour)), commit(now.Add(-time.Hour))}, + expected: now.Add(-time.Hour), + }, + { + name: "Newest commit is not last", + commits: []*github.RepositoryCommit{commit(now.Add(-time.Hour)), commit(now.Add(-2 * time.Hour))}, + expected: now.Add(-time.Hour), + }, + { + name: "Nil entries are skipped", + commits: []*github.RepositoryCommit{nil, {}, {Commit: &github.Commit{}}, commit(now.Add(-time.Hour))}, + expected: now.Add(-time.Hour), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := latestCommitTime(tt.commits) + if !got.Equal(tt.expected) { + t.Errorf("expected %v, got %v", tt.expected, got) + } + }) + } +} + func TestFilterWaitingOnAuthor(t *testing.T) { cfg := config.C() cfg.GithubUsername = "myself" @@ -400,7 +535,11 @@ func TestFilterWaitingOnAuthor(t *testing.T) { owner := "owner" repo := "repo" - makePR := func(number int) *github.PullRequest { + makePR := func(number int, reviewers ...string) *github.PullRequest { + requestedReviewers := []*github.User{} + for _, r := range reviewers { + requestedReviewers = append(requestedReviewers, &github.User{Login: github.String(r)}) + } return &github.PullRequest{ Number: &number, User: &github.User{Login: github.String("author")}, @@ -410,6 +549,7 @@ func TestFilterWaitingOnAuthor(t *testing.T) { Name: &repo, }, }, + RequestedReviewers: requestedReviewers, } } @@ -464,6 +604,29 @@ func TestFilterWaitingOnAuthor(t *testing.T) { state: InteractionState{}, shouldInclude: false, }, + { + // Same timestamps as the first case, but the author re-requested my + // review — the PR is waiting on me, not on them. + name: "I acted last but my review was re-requested", + pr: makePR(105, "myself"), + state: InteractionState{ + LastMeTime: time.Now().Add(-5 * time.Minute), + LastOthersTime: time.Now().Add(-10 * time.Minute), + LastCommitTime: time.Now().Add(-20 * time.Minute), + }, + shouldInclude: false, + }, + { + name: "I acted last but my review was dismissed", + pr: makePR(106), + state: InteractionState{ + LastMeTime: time.Now().Add(-5 * time.Minute), + LastOthersTime: time.Now().Add(-10 * time.Minute), + LastCommitTime: time.Now().Add(-20 * time.Minute), + MyReviewDismissed: true, + }, + shouldInclude: false, + }, { name: "Nil PR ignored", pr: nil,