From 8fba074591a990fe7afb51f59749f3cb9e44ffb5 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Fri, 1 May 2026 03:17:26 +0000 Subject: [PATCH] feat(review-pr): request reviewer from stakeholders for spec PRs For spec-only PRs (all changed files under specs/), load .github/STAKEHOLDERS and include a prompt section instructing the agent to extract code paths mentioned in the spec content (e.g. 'Relevant code' sections in tech specs) and match them against the stakeholder roster to recommend a single reviewer. On APPROVE verdict, apply_review_result() calls pr.create_review_request() with the resolved reviewer, mirroring the existing non-member flow. On REJECT, no reviewer is requested (spec PRs always use COMMENT, not REQUEST_CHANGES). The deterministic STAKEHOLDERS fallback is used when the agent returns an empty or invalid recommended_reviewers list. Co-Authored-By: Oz --- core/workflows/review_pr.py | 45 ++++++++- tests/test_review_pr_reviewer_sampling.py | 113 +++++++++++++++++++++- 2 files changed, 156 insertions(+), 2 deletions(-) diff --git a/core/workflows/review_pr.py b/core/workflows/review_pr.py index 8e7e9ec..7bc2a79 100644 --- a/core/workflows/review_pr.py +++ b/core/workflows/review_pr.py @@ -607,6 +607,30 @@ def _format_non_member_review_section( ).strip() +def _format_spec_reviewer_section( + *, + pr_author_login: str, + stakeholders_block: str, +) -> str: + return dedent( + f""" + Spec PR Reviewer Selection: + - This PR only modifies files under `specs/`. After completing your review, request exactly one human reviewer from `.github/STAKEHOLDERS` when your `verdict` is `"APPROVE"`. + - If your `verdict` is `"REJECT"`, the workflow will post feedback but will not request a human reviewer. + - Return a `recommended_reviewers` field alongside `verdict`, `summary`, and `comments`. + - `recommended_reviewers` must be a JSON list with exactly one bare GitHub login string, for example: {{"recommended_reviewers": ["octocat"]}}. + - To select the reviewer, read the spec content and identify which source files, directories, or subsystems are mentioned as being changed or affected (e.g. in "Relevant code" or "Proposed changes" sections of a tech spec). Match those code paths against the STAKEHOLDERS rules. Later rules override earlier rules, and more specific matching rules should be preferred over catch-all rules. + - Strip any leading `@` from the login and exclude the PR author (@{pr_author_login or 'unknown'}); GitHub rejects self-review requests. + - Do not return more than one reviewer, and do not return multiple candidates for the workflow to choose from. + - If you genuinely cannot identify one matching eligible stakeholder from the spec content, set `recommended_reviewers` to an empty list. The workflow will deterministically choose a fallback reviewer from `.github/STAKEHOLDERS`; do not invent or copy unrelated logins to satisfy the field. + - Do not call GitHub yourself to post the review or request reviewers. + + Stakeholders (from `.github/STAKEHOLDERS`): + {stakeholders_block} + """ + ).strip() + + def _format_pr_description( *, pr_number: int, @@ -719,6 +743,7 @@ class ReviewContext(TypedDict): supplemental_skill_line: str repo_local_section: str non_member_review_section: str + spec_reviewer_section: str pr_description_text: str pr_diff_text: str spec_context_text: str @@ -877,6 +902,7 @@ def gather_review_context( getattr(getattr(pr, "user", None), "login", "") or "" ) non_member_review_section = "" + spec_reviewer_section = "" stakeholders_entries: list[dict[str, Any]] = [] if is_non_member: # Load ``.github/STAKEHOLDERS`` directly from the repository @@ -890,6 +916,18 @@ def gather_review_context( pr_author_login=pr_author_login, stakeholders_block=stakeholders_block, ) + elif spec_only: + # Load ``.github/STAKEHOLDERS`` so the agent can match the code + # paths mentioned in the spec against the stakeholder roster and + # recommend a human reviewer. The agent uses spec content (e.g. + # "Relevant code" sections) rather than the PR diff paths, because + # spec PRs only touch files under ``specs/``. + stakeholders_entries = load_stakeholders_from_repo(github) + stakeholders_block = format_stakeholders_for_prompt(stakeholders_entries) + spec_reviewer_section = _format_spec_reviewer_section( + pr_author_login=pr_author_login, + stakeholders_block=stakeholders_block, + ) pr_description_text = _format_pr_description( pr_number=pr_number, pr_title=str(pr.title or ""), @@ -928,6 +966,7 @@ def gather_review_context( supplemental_skill_line=supplemental_skill_line, repo_local_section=repo_local_section, non_member_review_section=non_member_review_section, + spec_reviewer_section=spec_reviewer_section, pr_description_text=pr_description_text, pr_diff_text=pr_diff_text, spec_context_text=spec_context_text, @@ -1006,6 +1045,9 @@ def build_review_prompt_for_dispatch(context: Mapping[str, Any]) -> str: non_member_section = str(context.get("non_member_review_section") or "").rstrip() if non_member_section: prompt = prompt + "\n\n" + non_member_section + spec_reviewer_section = str(context.get("spec_reviewer_section") or "").rstrip() + if spec_reviewer_section: + prompt = prompt + "\n\n" + spec_reviewer_section return prompt @@ -1076,7 +1118,8 @@ def apply_review_result( if is_non_member and verdict == _VERDICT_REJECT else "COMMENT" ) - if is_non_member and verdict == _VERDICT_APPROVE: + spec_only = bool(context.get("spec_only")) + if (is_non_member or spec_only) and verdict == _VERDICT_APPROVE: recommended_reviewers = _resolve_recommended_reviewers( result, stakeholder_entries=stakeholder_entries, diff --git a/tests/test_review_pr_reviewer_sampling.py b/tests/test_review_pr_reviewer_sampling.py index 13cef23..779dc93 100644 --- a/tests/test_review_pr_reviewer_sampling.py +++ b/tests/test_review_pr_reviewer_sampling.py @@ -10,6 +10,7 @@ from workflows.review_pr import ( # type: ignore[import-not-found] _deterministic_reviewer_from_stakeholders, _format_non_member_review_section, + _format_spec_reviewer_section, _format_review_completion_message, _parse_verdict, _resolve_recommended_reviewers, @@ -194,16 +195,43 @@ def test_non_string_verdict_defaults_to_approve(self) -> None: self.assertEqual(_parse_verdict({"verdict": None}), "APPROVE") +class SpecReviewerSectionTest(unittest.TestCase): + def test_prompt_includes_spec_content_guidance(self) -> None: + prompt = _format_spec_reviewer_section( + pr_author_login="contributor", + stakeholders_block="- /core/ → @core-owner", + ) + self.assertIn("specs/", prompt) + self.assertIn("Relevant code", prompt) + self.assertIn("exactly one bare GitHub login", prompt) + self.assertIn("Do not return more than one reviewer", prompt) + + def test_prompt_gates_reviewer_request_on_approve_verdict(self) -> None: + prompt = _format_spec_reviewer_section( + pr_author_login="contributor", + stakeholders_block="- /core/ → @core-owner", + ) + self.assertIn("`verdict` is `\"APPROVE\"`", prompt) + + def test_prompt_excludes_pr_author(self) -> None: + prompt = _format_spec_reviewer_section( + pr_author_login="alice", + stakeholders_block="- /core/ → @core-owner", + ) + self.assertIn("@alice", prompt) + + class ApplyReviewResultVerdictTest(unittest.TestCase): """Verify ``apply_review_result`` honors the agent-supplied verdict.""" - def _make_context(self, *, is_non_member: bool) -> dict: + def _make_context(self, *, is_non_member: bool, spec_only: bool = False) -> dict: return { "owner": "acme", "repo": "widgets", "pr_number": 7, "requester": "alice", "is_non_member": is_non_member, + "spec_only": spec_only, "pr_author_login": "contributor", "stakeholder_entries": STAKEHOLDERS, "stakeholder_logins": ["api-owner", "docs-owner", "fallback", "python-owner"], @@ -434,5 +462,88 @@ def test_member_approve_and_reject_use_identical_review_body_text(self) -> None: ) + def test_approve_spec_pr_requests_reviewer(self) -> None: + pr = MagicMock() + github = self._make_github(pr) + progress = MagicMock() + apply_review_result( + github, + context=self._make_context(is_non_member=False, spec_only=True), + run=MagicMock(), + result={ + "verdict": "APPROVE", + "summary": "Spec looks good", + "comments": [], + "recommended_reviewers": ["api-owner"], + }, + progress=progress, + ) + pr.create_review.assert_called_once() + self.assertEqual(pr.create_review.call_args.kwargs["event"], "COMMENT") + pr.create_review_request.assert_called_once_with(reviewers=["api-owner"]) + + def test_reject_spec_pr_does_not_request_reviewer(self) -> None: + pr = MagicMock() + github = self._make_github(pr) + progress = MagicMock() + apply_review_result( + github, + context=self._make_context(is_non_member=False, spec_only=True), + run=MagicMock(), + result={ + "verdict": "REJECT", + "summary": "Spec needs work", + "comments": [], + "recommended_reviewers": ["api-owner"], + }, + progress=progress, + ) + # Spec PRs use COMMENT (not REQUEST_CHANGES) even on REJECT + pr.create_review.assert_called_once() + self.assertEqual(pr.create_review.call_args.kwargs["event"], "COMMENT") + pr.create_review_request.assert_not_called() + + def test_approve_spec_pr_with_no_feedback_still_requests_reviewer(self) -> None: + """A spec PR APPROVE with no summary/comments should still request a reviewer.""" + pr = MagicMock() + github = self._make_github(pr) + progress = MagicMock() + apply_review_result( + github, + context=self._make_context(is_non_member=False, spec_only=True), + run=MagicMock(), + result={ + "verdict": "APPROVE", + "summary": "", + "comments": [], + "recommended_reviewers": ["api-owner"], + }, + progress=progress, + ) + # No review body posted (no summary or comments), but reviewer IS requested + pr.create_review.assert_not_called() + pr.create_review_request.assert_called_once_with(reviewers=["api-owner"]) + + def test_approve_spec_pr_falls_back_to_deterministic_reviewer(self) -> None: + """When agent provides no recommended_reviewers, fall back to STAKEHOLDERS.""" + pr = MagicMock() + github = self._make_github(pr) + progress = MagicMock() + apply_review_result( + github, + context=self._make_context(is_non_member=False, spec_only=True), + run=MagicMock(), + result={ + "verdict": "APPROVE", + "summary": "Spec looks good", + "comments": [], + "recommended_reviewers": [], + }, + progress=progress, + ) + # Falls back to first eligible owner from STAKEHOLDERS ("fallback") + pr.create_review_request.assert_called_once_with(reviewers=["fallback"]) + + if __name__ == "__main__": unittest.main()