docs: pr review policy and report#9679
Draft
matthewkeil wants to merge 3 commits into
Draft
Conversation
matthewkeil
requested review from
nflaig and
wemeetagain
and removed request for
nflaig
July 18, 2026 16:01
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Pull Request Review Policy (PR_REVIEWS.md) outlining guidelines for review assignments, SLAs, post-merge follow-ups, and a daily automated tracking report. Feedback on the proposal highlights several edge cases and technical improvements for the automation, including: addressing a flaw in the lastProcessedPr watermark that would miss newly undrafted PRs, correcting trailing commas in the JSON example, clarifying how holidays are programmatically excluded for a distributed team, and handling team-level review requests in the report grouping.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| ### New-Since-Last-Run Detection | ||
|
|
||
| - **New PRs**: any PR with a number greater than the previous snapshot's `lastProcessedPr` watermark |
Contributor
There was a problem hiding this comment.
Using a simple lastProcessedPr watermark based on the highest PR number seen will fail to detect newly undrafted PRs. Since draft PRs are excluded from snapshots (as stated in Section 11/Exclusions), a draft PR (e.g., #9683) will not update the watermark. If a subsequent non-draft PR (e.g., #9684) is processed, the watermark rises to 9684. When #9683 is later undrafted, its number is less than the watermark, so it will be missed.
Consider using the PR's createdAt or tracking the transition state (e.g., readyForReview events) to reliably detect newly active PRs.
Comment on lines
+117
to
+131
| { | ||
| "generatedAt": "2026-07-18T01:00:00Z", | ||
| "lastProcessedPr": 9682, // watermark: highest PR number seen | ||
| "openPrs": [ | ||
| { | ||
| "number": 9679, | ||
| "title": "...", | ||
| "author": "...", | ||
| "createdAt": "...", | ||
| "reviewRequests": [{"reviewer": "...", "requestedAt": "..."}], | ||
| "reviews": [{"reviewer": "...", "submittedAt": "...", "state": "APPROVED", "requested": false}], | ||
| }, | ||
| ], | ||
| "closedPrs": [{"number": 9670, "author": "...", "createdAt": "...", "closedAt": "...", "merged": true}], | ||
| } |
Contributor
There was a problem hiding this comment.
The JSON example contains trailing commas (on lines 128 and 130). Since the schema specifies saving these files as standard .json files (e.g., review-metrics/YYYY-MM-DD.json), trailing commas will cause parsing errors in standard JSON parsers. It is best to use strictly compliant JSON in the documentation and output.
Suggested change
| { | |
| "generatedAt": "2026-07-18T01:00:00Z", | |
| "lastProcessedPr": 9682, // watermark: highest PR number seen | |
| "openPrs": [ | |
| { | |
| "number": 9679, | |
| "title": "...", | |
| "author": "...", | |
| "createdAt": "...", | |
| "reviewRequests": [{"reviewer": "...", "requestedAt": "..."}], | |
| "reviews": [{"reviewer": "...", "submittedAt": "...", "state": "APPROVED", "requested": false}], | |
| }, | |
| ], | |
| "closedPrs": [{"number": 9670, "author": "...", "createdAt": "...", "closedAt": "...", "merged": true}], | |
| } | |
| { | |
| "generatedAt": "2026-07-18T01:00:00Z", | |
| "lastProcessedPr": 9682, // watermark: highest PR number seen | |
| "openPrs": [ | |
| { | |
| "number": 9679, | |
| "title": "...", | |
| "author": "...", | |
| "createdAt": "...", | |
| "reviewRequests": [{"reviewer": "...", "requestedAt": "..."}], | |
| "reviews": [{"reviewer": "...", "submittedAt": "...", "state": "APPROVED", "requested": false}] | |
| } | |
| ], | |
| "closedPrs": [{"number": 9670, "author": "...", "createdAt": "...", "closedAt": "...", "merged": true}] | |
| } |
|
|
||
| ## 3. Review SLA | ||
|
|
||
| - Only working days are counted for SLA. Weekends and holidays are excluded as non-working days |
Contributor
There was a problem hiding this comment.
Excluding holidays programmatically for a globally distributed team (with members in Asia, North America, etc.) can be highly complex and error-prone to automate.
Consider clarifying how holidays are defined (e.g., using a specific region's calendar, or a shared team calendar file in the repository), or simplify the automation to only exclude weekends to reduce maintenance overhead.
|
|
||
| 1. **Past SLA review window**: review requests older than 2 business days (weekends and holidays excluded), listed as PR link/title plus requested reviewer. No ages, no ordering by lateness; just what is lagging. Empty section renders as "None" | ||
| 2. **New review requests** (since the last run): PR link/title, author, requested reviewer(s) | ||
| 3. **Open review requests by team member**: one list per person of the PRs currently waiting on them. The grouping is derived from whoever appears as a requested reviewer; there is no team roster to maintain |
Contributor
There was a problem hiding this comment.
GitHub allows requesting reviews from Teams (e.g., @org/team-name) in addition to individual users. If the script groups requests solely by individual requested reviewers, team-level requests might either be missed, fail to resolve, or be listed under the team name rather than the individual members.
Consider specifying how team-level review requests should be handled or expanded in the report.
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Way to help distribute the PR load a bit and document the process