feat: remove stale review requests (remove_stale_review_requests) - #182
feat: remove stale review requests (remove_stale_review_requests)#182C-Hipple wants to merge 2 commits into
Conversation
Codeowners Plus only ever added review requests, so when a push changed which files a PR touches, the requests made for the old owner set stayed on the PR while the review status comment updated to the new, smaller one. Add an opt-in `remove_stale_review_requests` setting which drops the review requests the action made for owners the current diff no longer involves. The owner set is taken pre-approval and includes optional reviewers, so an owner who already approved keeps their review request instead of having it pulled out from under them. Only requests whose most recent `review_requested` timeline event was performed by the token user are removed, so reviewers somebody added by hand survive a push. Telling the two apart needs a resolvable token user, so removal is skipped with a warning when it cannot be read (e.g. GITHUB_TOKEN), and in quiet mode. The timeline is only fetched when there is at least one candidate, so an ordinary run costs no extra API calls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Codeowners approval required for this PR: |
Greptile SummaryThis PR adds an opt-in cleanup pass for stale review requests, preserving current required and optional owners and attempting to remove only requests attributed to the token user.
Confidence Score: 4/5The PR should not merge until manual review requests made by the token-owning account can no longer be mistaken for Codeowners Plus requests and deleted. Timeline actor identity records which account performed a request, not whether that account acted interactively or through this application, so the new deletion path can remove a request the feature promises to preserve. Files Needing Attention: internal/github/gh.go, internal/app/app.go
|
| Filename | Overview |
|---|---|
| internal/app/app.go | Adds the opt-in stale-request cleanup orchestration and protects required and optional owners before requesting new reviewers. |
| internal/github/gh.go | Adds raw reviewer retrieval, timeline attribution, and deletion; actor-only attribution cannot distinguish automation from manual actions by the token owner. |
| internal/config/config.go | Adds the disabled-by-default remove_stale_review_requests configuration field. |
| README.md | Documents setup, token requirements, preservation semantics, and quiet-mode behavior. |
| internal/app/app_test.go | Covers cleanup policy and wiring but omits manual requests made interactively by the token-owning account. |
| internal/github/gh_test.go | Covers API payloads and timeline attribution but models token-owner events exclusively as action-owned requests. |
Sequence Diagram
sequenceDiagram
participant App as Codeowners Plus
participant PR as GitHub Pull Request
participant Timeline as Issue Timeline
App->>PR: Read currently requested reviewers
App->>App: Subtract required and optional owners
alt stale candidates exist
App->>Timeline: Read review request events
App->>App: Compare latest actor with token user
App->>PR: Delete attributed stale requests
end
App->>PR: Request missing required reviewers
Reviews (1): Last reviewed commit: "feat: remove stale review requests (remo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| return f.Filtered(requestedReviewers(pr, owner), func(reviewer codeowners.Slug) bool { | ||
| requester, found := requesters[reviewer.Normalized()] | ||
| return found && tokenUserSlug.EqualsString(requester) |
There was a problem hiding this comment.
When a human-owned PAT is used and its owner manually requests a reviewer, actor-only attribution classifies that request as one made by Codeowners Plus, causing the new cleanup path to delete the manually maintained request after the reviewer stops owning changed files.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to automatically remove stale review requests made by Codeowners Plus when a PR's changed files no longer require those owners. This is controlled by a new configuration option remove_stale_review_requests. The changes include implementing the cleanup logic in the application and GitHub client, along with comprehensive unit tests. The feedback suggests a minor improvement in internal/github/gh.go to explicitly return nil instead of err at the end of RemoveReviewers for better readability.
| defer func() { | ||
| _ = res.Body.Close() | ||
| }() | ||
| return err |
There was a problem hiding this comment.
Returning err at the end of the function when it is already guaranteed to be nil is non-idiomatic and can be confusing to readers. It is much clearer to explicitly return nil to signal successful execution.
| return err | |
| return nil |
- Downgrade a RemoveReviewers failure to a warning. Removal is an opt-in cosmetic cleanup, so a transient GitHub API error should not fail the check. This also makes the function consistent: both of its network calls now warn and continue, leaving only the nil-PR guard fatal. - Cache the token user on GHClient so GetTokenUser costs at most one GET /user per run. It can now be called twice, from processTokenOwnerApproval and GetSelfRequestedReviewers. - Fix the `indvidualReviewers` typo in RemoveReviewers along with the two pre-existing occurrences in RequestReviewers and splitReviewers. NewClient now uses named struct fields - adding tokenUser to a ten-field positional literal was asking for a silently misassigned value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary / Background
Codeowners Plus only ever added review requests.
requestReviews()computed required owners minus already-requested minus already-reviewed and calledRequestReviewers; nothing ever ran in the other direction. So when a push changes which files a PR touches, the requests made for the old owner set stay on the PR while the review status comment updates to the new, smaller one — the PR ends up showing more teams as requested reviewers than are actually required.This adds an opt-in
remove_stale_review_requestssetting that cleans those up, mirroring the existing request behavior.How it decides what to remove
Running just before
requestReviews(), it:review_requestedevent was performed by the token user.DELETE /requested_reviewersfor the survivors.Step 3 is the important one. A blanket "remove everything not required" would also strip reviewers a human added by hand — including, on repos using
enforcement.approval, the token owner that a PR author manually requested to satisfyCODEOWNERS. Real timeline from a PR that hit this bug:Only
affiliate-program-expertswas still required after the author's update. With this change the three stale teams are removed andmmllc-gh— requested by a human — is left alone.The timeline is only fetched when step 2 finds at least one candidate, so an ordinary run costs no extra API calls.
Behavior notes
remove_stale_review_requests = true.GET /user, which 403s forGITHUB_TOKEN. In that case removal is skipped with a warning rather than guessing. Fine for the teams use case, which already requires a PAT, but it means the feature is a no-op onGITHUB_TOKEN-only setups — called out in the README.RemoveReviewerserror warns and continues — a cosmetic cleanup shouldn't block a PR's check on a transient API error. (This started out fatal to mirrorRequestReviewers; PRism pushed back and it was right — requesting reviews is functionally required for the check, removal isn't.)Code Changes
internal/github/gh.go— three newClientmethods:GetRequestedReviewers— raw requested reviewers as owner slugs (@login,@owner/team-slug)GetSelfRequestedReviewers— the subset whose latest request event was performed by the token user, via a paginated timeline walkRemoveReviewers—DELETE /requested_reviewersinternal/app/app.go—removeStaleReviewRequests, called fromprocessApprovalsAndReviewerswith the pre-approval required ∪ optional owner set.internal/config/config.go—remove_stale_review_requests(defaultfalse).README.md— new "Review Request Removal" section, TOC entry, config docs, quiet-mode note.Follow-up commit (
a1a25aa) from PRism review feedback:GetTokenUsernow caches the login, so it costs at most oneGET /userper run rather than one per caller.NewClientswitched to named struct fields — adding a field to a ten-field positional literal was asking for a silently misassigned value.indvidualReviewerstypo, including the two pre-existing occurrences inRequestReviewersandsplitReviewers.Tests
removeStaleReviewRequests: not-enabled, quiet, no candidates, self-requested vs. other-requested, optional owners kept, case-insensitive matching, lookup failure, and both error paths.review_request_removed, reviewers no longer on the PR, reviewers with no request event, case-insensitive actor.NoPRErrorcoverage.Mutation-checked three ways — dropping optional owners from the keep set, using the post-approval required set, and removing the opt-in gate each make a test fail.
Coverage badge regenerated (82.6% → 83.4%).
🤖 Generated with Claude Code