feat(review): request human reviewers for spec PRs from external contributors#387
feat(review): request human reviewers for spec PRs from external contributors#387captainsafia wants to merge 1 commit intomainfrom
Conversation
…ributors When a spec-only PR (markdown-only) is opened by an external contributor, the review agent now identifies 1-2 human reviewers from .github/STAKEHOLDERS based on the spec content and path patterns, and requests their review. Previously, spec PRs from non-members received an automated COMMENT review but no human ping. This change adds a new is_spec_non_member path that keeps the COMMENT event (no approval gate) while also asking the review-spec agent to populate recommended_reviewers in review.json. The workflow then calls create_review_request with up to _MAX_SPEC_REVIEWERS (2) logins so the right maintainers are always notified when external contributors submit spec changes. Changes: - Add _MAX_SPEC_REVIEWERS = 2 constant for spec reviewer cap - Add _resolve_spec_reviewer_request() helper to extract reviewers for spec PRs - Add is_spec_non_member variable to main() alongside existing is_non_member - Load STAKEHOLDERS and build Spec Reviewer Request prompt section for spec non-member PRs instead of the existing Non-Member Review Action section - Extract recommended_reviewers from review.json for spec non-member PRs and call create_review_request after posting the COMMENT review - Update short-circuit condition to not skip posting when there are reviewers - Update _format_review_completion_message to handle COMMENT+reviewers case - Add tests for _resolve_spec_reviewer_request and COMMENT+reviewers message Co-Authored-By: Oz <oz-agent@warp.dev>
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This adds a dedicated spec-only non-member path so the workflow can keep COMMENT reviews for spec PRs while still requesting 1-2 human reviewers from .github/STAKEHOLDERS. The split between the implementation-PR gate and the new spec-reviewer flow is clear, and I did not find any separate security issues in the changed code.
Concerns
_normalize_reviewer_logins()still de-duplicates case-sensitively, so the new spec-reviewer path can preserve mixed-case duplicates likeAlice/aliceand hand the same GitHub reviewer tocreate_review_request()twice. Because reviewer requests are best-effort here, a GitHub validation failure would silently drop the human ping this change is trying to add.- I could not execute
test_review_pr.pyin this environment because thegithubdependency is not installed, so the review is based on the diff plus the added unit coverage.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Powered by Oz
| Reviews are capped at ``_MAX_SPEC_REVIEWERS`` to avoid over-notifying | ||
| maintainers on a single spec change. | ||
| """ | ||
| return _normalize_reviewer_logins( |
There was a problem hiding this comment.
_normalize_reviewer_logins()'s case-sensitive dedupe, so a reviewer list like ['Alice', 'alice'] will survive normalization and can hand the same GitHub account to create_review_request() twice. Because reviewer requests are best-effort here, that can silently drop the human ping this PR is trying to add. Please normalize the dedupe key to lowercase and add a mixed-case regression test for the spec reviewer path.
2 participants
Summary
Spec-only PRs (markdown-only) from external contributors previously received an automated agent review as a
COMMENTbut no human reviewer was ever pinged. This PR wires up reviewer pinging for spec PRs from non-members so the right maintainers are notified.Changes
_MAX_SPEC_REVIEWERS = 2— new constant capping reviewers pinged for a single spec PR_resolve_spec_reviewer_request()— new helper that extractsrecommended_reviewersfromreview.jsonfor spec-only non-member PRs (capped at 2, filtered to STAKEHOLDERS)is_spec_non_member— new flag inmain()(spec_only and _is_non_member_pr(pr)) that selects the spec reviewer path without entering the APPROVE/REQUEST_CHANGES gateis_spec_non_member, the agent receives a "Spec Reviewer Request" section instructing it to identify 1-2 best-fit reviewers from.github/STAKEHOLDERSbased on the spec content and path patternscreate_review_requestwith the normalized reviewer list (best-effort; errors are logged but don't fail the workflow)_format_review_completion_messagenow returns a spec-specific message when event is COMMENT but reviewers are presentResolveSpecReviewerRequestTestclass (6 cases) and a newtest_comment_with_reviewers_mentions_loginscase; all 59 tests passBehavior
Conversation: https://staging.warp.dev/conversation/dec82960-2e0e-456d-ae79-8870b7433ae3
Run: https://oz.staging.warp.dev/runs/019dd0a6-6894-78c9-868b-0481c96f3ec2
This PR was generated with Oz.