Fix release docs reviewer routing - #1124
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR updates the .github/workflows/upstream-release-docs.yml reviewer assignment logic to avoid pinging external (non-collaborator) contributors for “no docs impact” commits, and to add a stand-in reviewer fallback when GitHub rejects review requests for non-collaborator docs-facing contributors.
Changes:
- Restricts “No docs impact identified” @-mentions to docs-website collaborators; counts external contributors without mentioning them.
- Adds stand-in reviewer resolution by finding the merger of the upstream PR containing a contributor’s first commit, and requests review from that merger when the contributor can’t be assigned.
- Extends PR-body augmentation to include stand-in explanations and separate member vs external FYI counts.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Use GitHub's review-request result as the repository-access check and route rejected docs-facing contributors to the human mergers of their specific upstream PRs. Keep owner and noisy fallback behavior, avoid contributor pings, and cover the routing cases with tests.
7c238d2 to
71a23f8
Compare
danbarr
changed the title
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
LGTM overall. I found a few routing edge cases worth addressing before this workflow becomes the source of truth for review assignment.
| "-f", | ||
| f"reviewers[]={login}", | ||
| ) | ||
| return result.returncode == 0 |
There was a problem hiding this comment.
request_review() returns only a boolean, so every nonzero gh api result is handled as though GitHub rejected this contributor's repository access. A secondary rate limit, transient 5xx, or network failure will therefore trigger merger fallback and allow the workflow to succeed with a potentially unrelated reviewer.
Could we preserve the response status here and distinguish an expected reviewer-access rejection from an operational failure? Use merger fallback only for the access-rejection case. For rate limits, 5xx responses, malformed responses, and transport failures, retry where appropriate or fail the routing step so the release cannot report a successful routing decision based on an unavailable GitHub API.
Please add tests for both an access rejection and a transient API failure.
There was a problem hiding this comment.
Good catch. Fixed in 00746ba. request_review() now treats only GitHub’s specific 422 collaborator-access rejection as recoverable. Other HTTP or transport failures raise ReviewRequestError and fail the routing step. Tests cover both access rejection and a transient 503.
| [login for login in selection.non_docs_facing if login != config.owner] | ||
| ) | ||
|
|
||
| for contributor, shas in selection.docs_facing.items(): |
There was a problem hiding this comment.
This unconditional owner skip leaves a gap when the release owner is also docs-facing: if the owner's direct review request fails above, their SHAs never enter the existing merger-stand-in path. The workflow records an unresolved note even when an eligible human merger exists.
Could we skip the owner here only after their reviewer request succeeds? When it fails and the owner has docs-facing SHAs, route those SHAs through the same commit-to-merged-PR fallback used for other docs-facing contributors. A focused regression test can assert that a rejected docs-facing owner request produces a reviewer request for the eligible merger.
There was a problem hiding this comment.
Fixed in 00746ba. A rejected docs-facing owner now follows the same commit-to-PR-to-merger fallback as other contributors. The regression test verifies that the eligible merger receives the request without leaving an unresolved routing note.
| candidates, | ||
| commits, | ||
| "REVIEWERS.json omitted release contributors: " + ", ".join(missing), | ||
| ) |
There was a problem hiding this comment.
Repeated contributor records silently overwrite the earlier entry in by_login. A later docs_facing: false record can erase a prior docs-facing SHA list, allowing a malformed REVIEWERS.json to suppress both direct review and merger fallback for that contributor.
Could we detect duplicate candidate logins while validating the artifact and use the existing "request all contributors" fallback? This keeps malformed classification noisy instead of trusting the last record. Please add a fixture where conflicting duplicate entries trigger the fallback.
There was a problem hiding this comment.
Fixed in 00746ba. Duplicate candidate logins now invalidate REVIEWERS.json and trigger the noisy request-all fallback. The new test covers conflicting docs-facing and non-docs-facing records.
| f"Release owner `{config.owner}` could not be requested as a reviewer. " | ||
| "A docs maintainer must route this review." | ||
| ) | ||
| else: |
There was a problem hiding this comment.
An unresolved release owner produces a warning but does not contribute to unresolved_count. The PR body can consequently report Action required: None even though it says a docs maintainer must adopt the unassigned PR.
Could we record absent owner resolution as an unresolved routing outcome, then include it in the existing action-required calculation? Add a test for an app-authored release with no eligible owner, asserting that the rendered release PR requires manual action.
There was a problem hiding this comment.
Fixed in 00746ba. An unresolved owner now creates an unresolved routing outcome and emits unresolved_count=1, which drives the generated PR’s action-required row. Added coverage for that output.
|
@jhrozek thanks for the review! I've addressed the items you pointed out. |
Description
Fixes reviewer-routing regressions introduced by #1111 and exposed by #1120, without adding a PAT or organization-membership permission.
The workflow now treats GitHub's review-request response as the repository-access check and fails closed when GitHub cannot provide a reliable decision:
REVIEWERS.jsonfalls back to requesting every release contributor.Reviewer assignment now lives in a focused Python helper instead of inline workflow Bash. Regression tests cover access rejection versus operational API failure, unconditional owner review, docs-facing owner fallback, missing and invalid classification fallbacks, conflicting duplicate classifications, multiple docs-facing PRs, stand-in deduplication, non-docs-facing counts, unresolved owner handling, and unresolved stand-in routing.
Type of change
Related issues/PRs
Testing
python3 -m unittest discover -s .github/scripts -p 'test_*.py'actionlint .github/workflows/upstream-release-docs.yml .github/workflows/_static-checks.yamlzizmor .github/workflows/upstream-release-docs.yml .github/workflows/_static-checks.yamlnpm run prettiernpm run eslintnpm run buildSubmitter checklist
Content and formatting
Reviewer checklist
Content