From 0c17c2f7fb0044b570aacc0e01679c0218b85c34 Mon Sep 17 00:00:00 2001 From: vkodithala Date: Sat, 30 May 2026 00:34:33 -0400 Subject: [PATCH 1/2] fix: request human reviewer on bot-authored and external-contributor PRs Decouples reviewer selection from is_non_member by introducing requires_human_reviewer = is_automation_user(pr.user) or _is_non_member_pr(pr), so /oz-review escalates every Oz-authored PR (spec or implementation) and every external-contributor PR to a human reviewer. is_non_member still gates the REJECT -> REQUEST_CHANGES decision. Internal member PRs are unaffected. Co-Authored-By: Oz --- core/workflows/review_pr.py | 24 +++- tests/test_review_pr_reviewer_sampling.py | 144 ++++++++++++++++++++++ 2 files changed, 162 insertions(+), 6 deletions(-) diff --git a/core/workflows/review_pr.py b/core/workflows/review_pr.py index 45a3e30..99c7092 100644 --- a/core/workflows/review_pr.py +++ b/core/workflows/review_pr.py @@ -797,8 +797,8 @@ def _format_non_member_review_section( if ownership_areas_loaded: return dedent( f""" - Non-Member Reviewer Selection: - - The PR author (@{pr_author_login or 'unknown'}) is not a repository member or collaborator, so the workflow should request exactly one human reviewer when your `verdict` is `"APPROVE"`. + Reviewer Selection: + - This pull request needs a human reviewer (its author @{pr_author_login or 'unknown'} is an automation account or an external contributor), so the workflow should request exactly one human reviewer when your `verdict` is `"APPROVE"`. - If your `verdict` is `"REJECT"`, the workflow will post a GitHub `REQUEST_CHANGES` review and will not request a human reviewer. - Return a `recommended_area` field alongside `verdict`, `body`, and `comments`. Example: {{"recommended_area": "MCP (Model Context Protocol)"}}. - `recommended_area` must be EXACTLY ONE area name copied verbatim from the "Ownership Areas" list below. Match the PR title, body, and changed files to the area whose `matches:` description best describes the change. @@ -817,8 +817,8 @@ def _format_non_member_review_section( ).strip() return dedent( f""" - Non-Member Reviewer Selection: - - The PR author (@{pr_author_login or 'unknown'}) is not a repository member or collaborator, so the workflow should request exactly one human reviewer when your `verdict` is `"APPROVE"`. + Reviewer Selection: + - This pull request needs a human reviewer (its author @{pr_author_login or 'unknown'} is an automation account or an external contributor), so the workflow should request exactly one human reviewer when your `verdict` is `"APPROVE"`. - If your `verdict` is `"REJECT"`, the workflow will post a GitHub `REQUEST_CHANGES` review and will not request a human reviewer. - Return a `recommended_reviewers` field alongside `verdict`, `body`, and `comments`. - `recommended_reviewers` must be a JSON list with exactly one bare GitHub login string, for example: {{"recommended_reviewers": ["octocat"]}}. @@ -951,6 +951,7 @@ class ReviewContext(TypedDict): diff_line_map: dict[str, dict[str, list[int]]] diff_content_map: dict[str, dict[str, dict[str, str]]] is_non_member: bool + requires_human_reviewer: bool spec_only: bool pr_author_login: str stakeholder_logins: list[str] @@ -1069,6 +1070,10 @@ def gather_review_context( else "" ) is_non_member = _is_non_member_pr(pr) and not spec_only + # Bot-authored and external-contributor PRs require a human reviewer. + requires_human_reviewer = ( + is_automation_user(getattr(pr, "user", None)) or _is_non_member_pr(pr) + ) pr_author_login = str( getattr(getattr(pr, "user", None), "login", "") or "" ) @@ -1077,7 +1082,7 @@ def gather_review_context( pr_assignee_reviewers: list[str] = [] ownership_areas_serialized: list[dict[str, Any]] = [] ownership_areas_loaded = False - if is_non_member: + if requires_human_reviewer: pr_assignee_reviewers = _reviewer_from_pr_assignee( pr, pr_author_login=pr_author_login, @@ -1170,6 +1175,7 @@ def gather_review_context( diff_line_map=_serialize_diff_line_map(diff_line_map), diff_content_map=_serialize_diff_content_map(diff_content_map), is_non_member=bool(is_non_member), + requires_human_reviewer=bool(requires_human_reviewer), spec_only=bool(spec_only), pr_author_login=pr_author_login, stakeholder_logins=sorted(_stakeholder_logins(stakeholders_entries)), @@ -1323,6 +1329,12 @@ def apply_review_result( pr_number = int(context["pr_number"]) requester = str(context.get("requester") or "") is_non_member = bool(context.get("is_non_member")) + # Human reviewer escalation for bot- and external-contributor PRs is now controlled by the + # requires_human_reviewer flag. For backward compatibility with older serialized contexts, + # is_non_member is used as a fallback if the flag is missing. + requires_human_reviewer = bool( + context.get("requires_human_reviewer", is_non_member) + ) pr_author_login = str(context.get("pr_author_login") or "") raw_stakeholder_entries = context.get("stakeholder_entries") or [] stakeholder_entries = [ @@ -1365,7 +1377,7 @@ def apply_review_result( if is_non_member and verdict == _VERDICT_REJECT else "COMMENT" ) - if is_non_member and verdict == _VERDICT_APPROVE: + if requires_human_reviewer and verdict == _VERDICT_APPROVE: recommended_reviewers = _reviewer_from_pr_assignee( pr, pr_author_login=pr_author_login, diff --git a/tests/test_review_pr_reviewer_sampling.py b/tests/test_review_pr_reviewer_sampling.py index 87885a8..b58221b 100644 --- a/tests/test_review_pr_reviewer_sampling.py +++ b/tests/test_review_pr_reviewer_sampling.py @@ -3,6 +3,7 @@ from __future__ import annotations import unittest +from pathlib import Path from types import SimpleNamespace from unittest.mock import MagicMock, patch @@ -846,5 +847,148 @@ def test_progress_comment_mentions_reviewers_on_success(self) -> None: self.assertIn("requested human review", message) +class RequiresHumanReviewerEscalationTest(unittest.TestCase): + """``requires_human_reviewer`` keys on author identity.""" + + def _make_pr( + self, + *, + login: str, + user_type: str, + association: str, + filenames: list[str], + ) -> SimpleNamespace: + files = [ + SimpleNamespace( + filename=name, + previous_filename=None, + status="added", + patch=None, + ) + for name in filenames + ] + return SimpleNamespace( + user=SimpleNamespace(login=login, type=user_type), + author_association=association, + title="feat: change", + body="body", + base=SimpleNamespace(ref="main"), + head=SimpleNamespace(ref="feature"), + get_files=lambda: list(files), + ) + + def _gather(self, pr: SimpleNamespace) -> dict: + github = MagicMock() + github.get_pull.return_value = pr + with patch( + "workflows.review_pr.resolve_issue_number_for_pr", return_value=None + ), patch( + "workflows.review_pr.repo_local_skill_path_for_dispatch", + return_value=None, + ), patch( + "workflows.review_pr.resolve_spec_context_for_pr_via_api", + return_value={}, + ), patch( + "workflows.review_pr.load_stakeholders_from_repo", + return_value=STAKEHOLDERS, + ), patch( + "workflows.review_pr._build_diff_maps", return_value=({}, {}) + ): + return gather_review_context( + github, + owner="acme", + repo="widgets", + pr_number=7, + trigger_source="pull_request", + requester="alice", + workspace_path=Path("/tmp"), + ) + + def _apply_approve(self, context: dict) -> MagicMock: + pr = MagicMock() + github = MagicMock() + github.get_pull.return_value = pr + with patch( + "workflows.review_pr.load_stakeholders_from_repo", + return_value=STAKEHOLDERS, + ): + apply_review_result( + github, + context=context, + run=MagicMock(), + result={ + "verdict": "APPROVE", + "summary": "Looks good", + "comments": [], + "recommended_area": "", + }, + progress=MagicMock(), + ) + return pr + + def test_bot_and_external_authors_request_reviewer_on_approve(self) -> None: + cases = [ + ("oz-spec", "oz-for-oss[bot]", "Bot", "NONE", ["specs/GH1/product.md"]), + ("oz-impl", "oz-for-oss[bot]", "Bot", "NONE", ["core/app.py"]), + ("external-spec", "contributor", "User", "CONTRIBUTOR", ["specs/GH1/product.md"]), + ("external-impl", "contributor", "User", "CONTRIBUTOR", ["core/app.py"]), + ] + for label, login, user_type, association, filenames in cases: + with self.subTest(case=label): + context = self._gather( + self._make_pr( + login=login, + user_type=user_type, + association=association, + filenames=filenames, + ) + ) + self.assertTrue(context["requires_human_reviewer"]) + self.assertTrue(context["non_member_review_section"]) + pr = self._apply_approve(context) + pr.create_review_request.assert_called_once() + + def test_member_pr_never_requires_human_reviewer(self) -> None: + for filenames in (["specs/GH1/product.md"], ["core/app.py"]): + with self.subTest(filenames=filenames): + context = self._gather( + self._make_pr( + login="maintainer", + user_type="User", + association="MEMBER", + filenames=filenames, + ) + ) + self.assertFalse(context["requires_human_reviewer"]) + self.assertEqual(context["non_member_review_section"], "") + pr = self._apply_approve(context) + pr.create_review_request.assert_not_called() + + def test_oz_authored_reject_stays_comment_event(self) -> None: + context = self._gather( + self._make_pr( + login="oz-for-oss[bot]", + user_type="Bot", + association="NONE", + filenames=["core/app.py"], + ) + ) + self.assertTrue(context["requires_human_reviewer"]) + self.assertFalse(context["is_non_member"]) + pr = MagicMock() + github = MagicMock() + github.get_pull.return_value = pr + apply_review_result( + github, + context=context, + run=MagicMock(), + result={"verdict": "REJECT", "summary": "Needs work", "comments": []}, + progress=MagicMock(), + ) + pr.create_review.assert_called_once() + self.assertEqual(pr.create_review.call_args.kwargs["event"], "COMMENT") + pr.create_review_request.assert_not_called() + + if __name__ == "__main__": unittest.main() From 076749cc1413f665bda23e673e132425c5a5b064 Mon Sep 17 00:00:00 2001 From: vkodithala Date: Thu, 4 Jun 2026 15:26:21 -0400 Subject: [PATCH 2/2] Scope reviewer escalation to the Oz bot and align REJECT prompt wording Narrow human-reviewer escalation so only the configured Oz bot (via GH_APP_SLUG) and external-contributor PRs trigger it; other automation accounts like dependabot keep their own reviewer conventions. Parameterize the reviewer-selection prompt's REJECT outcome so it matches apply_review_result: REQUEST_CHANGES for non-members, COMMENT for the Oz bot's own PRs. Co-Authored-By: Oz --- core/workflows/review_pr.py | 32 ++++++++++++++----- oz/helpers.py | 25 +++++++++++++-- tests/test_prompts.py | 2 ++ tests/test_review_pr_reviewer_sampling.py | 39 ++++++++++++++++++++++- 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/core/workflows/review_pr.py b/core/workflows/review_pr.py index 99c7092..57cdbd1 100644 --- a/core/workflows/review_pr.py +++ b/core/workflows/review_pr.py @@ -18,6 +18,7 @@ get_label_name, get_login, is_automation_user, + is_oz_bot_user, is_spec_only_pr, ORG_MEMBER_ASSOCIATIONS, POWERED_BY_SUFFIX, @@ -780,11 +781,12 @@ def _dismiss_stale_oz_changes_requested_reviews( def _format_non_member_review_section( *, pr_author_login: str, + is_non_member: bool, ownership_areas_block: str = "", ownership_areas_loaded: bool = False, stakeholders_block: str = "", ) -> str: - """Render the non-member reviewer-selection section of the agent prompt. + """Render the reviewer-selection section of the agent prompt. When ``ownership_areas_loaded`` is true, instruct the agent to return a single ``recommended_area`` from the parsed ``warpdotdev/warp-ownership`` @@ -793,13 +795,23 @@ def _format_non_member_review_section( source. When ownership areas are unavailable (fetch failure, empty repo), fall back to the legacy STAKEHOLDERS prompt block where the agent returns ``recommended_reviewers`` directly. + + ``is_non_member`` controls the REJECT outcome wording so it matches + :func:`apply_review_result`: non-member PRs get a blocking + ``REQUEST_CHANGES`` review on reject, while the Oz bot's own PRs only + get a ``COMMENT`` so they never appear merge-blocked. """ + reject_outcome_line = ( + '- If your `verdict` is `"REJECT"`, the workflow will post a GitHub `REQUEST_CHANGES` review and will not request a human reviewer.' + if is_non_member + else '- If your `verdict` is `"REJECT"`, the workflow will post your review as a GitHub `COMMENT` and will not request a human reviewer.' + ) if ownership_areas_loaded: return dedent( f""" Reviewer Selection: - This pull request needs a human reviewer (its author @{pr_author_login or 'unknown'} is an automation account or an external contributor), so the workflow should request exactly one human reviewer when your `verdict` is `"APPROVE"`. - - If your `verdict` is `"REJECT"`, the workflow will post a GitHub `REQUEST_CHANGES` review and will not request a human reviewer. + {reject_outcome_line} - Return a `recommended_area` field alongside `verdict`, `body`, and `comments`. Example: {{"recommended_area": "MCP (Model Context Protocol)"}}. - `recommended_area` must be EXACTLY ONE area name copied verbatim from the "Ownership Areas" list below. Match the PR title, body, and changed files to the area whose `matches:` description best describes the change. - Do NOT invent area names. Do NOT return multiple names, a list, or owner handles. The workflow itself looks up the owners for the chosen area and randomly selects one. @@ -819,7 +831,7 @@ def _format_non_member_review_section( f""" Reviewer Selection: - This pull request needs a human reviewer (its author @{pr_author_login or 'unknown'} is an automation account or an external contributor), so the workflow should request exactly one human reviewer when your `verdict` is `"APPROVE"`. - - If your `verdict` is `"REJECT"`, the workflow will post a GitHub `REQUEST_CHANGES` review and will not request a human reviewer. + {reject_outcome_line} - Return a `recommended_reviewers` field alongside `verdict`, `body`, and `comments`. - `recommended_reviewers` must be a JSON list with exactly one bare GitHub login string, for example: {{"recommended_reviewers": ["octocat"]}}. - Choose that single reviewer from `.github/STAKEHOLDERS` by matching the changed file paths against the STAKEHOLDERS rules. Later rules override earlier rules, and more specific matching rules should be preferred over catch-all rules. @@ -1070,9 +1082,11 @@ def gather_review_context( else "" ) is_non_member = _is_non_member_pr(pr) and not spec_only - # Bot-authored and external-contributor PRs require a human reviewer. + # The Oz bot's own PRs and external-contributor PRs require a human + # reviewer. Other automation accounts are excluded + # so Oz does not override their own reviewer-assignment conventions. requires_human_reviewer = ( - is_automation_user(getattr(pr, "user", None)) or _is_non_member_pr(pr) + is_oz_bot_user(getattr(pr, "user", None)) or _is_non_member_pr(pr) ) pr_author_login = str( getattr(getattr(pr, "user", None), "login", "") or "" @@ -1121,6 +1135,7 @@ def gather_review_context( ) non_member_review_section = _format_non_member_review_section( pr_author_login=pr_author_login, + is_non_member=is_non_member, ownership_areas_block=ownership_areas_block, ownership_areas_loaded=True, stakeholders_block=stakeholders_block, @@ -1128,6 +1143,7 @@ def gather_review_context( else: non_member_review_section = _format_non_member_review_section( pr_author_login=pr_author_login, + is_non_member=is_non_member, stakeholders_block=stakeholders_block, ownership_areas_loaded=False, ) @@ -1329,9 +1345,9 @@ def apply_review_result( pr_number = int(context["pr_number"]) requester = str(context.get("requester") or "") is_non_member = bool(context.get("is_non_member")) - # Human reviewer escalation for bot- and external-contributor PRs is now controlled by the - # requires_human_reviewer flag. For backward compatibility with older serialized contexts, - # is_non_member is used as a fallback if the flag is missing. + # Human reviewer escalation for the Oz bot's own PRs and external-contributor PRs is + # controlled by the requires_human_reviewer flag. For backward compatibility with older + # serialized contexts, is_non_member is used as a fallback if the flag is missing. requires_human_reviewer = bool( context.get("requires_human_reviewer", is_non_member) ) diff --git a/oz/helpers.py b/oz/helpers.py index 4eb268c..38a69a0 100644 --- a/oz/helpers.py +++ b/oz/helpers.py @@ -120,6 +120,28 @@ def is_automation_user(user: Any) -> bool: ) +def oz_bot_login() -> str: + """Return the configured Oz GitHub App bot login, or empty when unset. + + Derived from ``GH_APP_SLUG`` as ``[bot]`` to match the login + GitHub stamps on App-authored actions. + """ + app_slug = optional_env("GH_APP_SLUG") + return f"{app_slug}[bot]" if app_slug else "" + + +def is_oz_bot_user(user: Any) -> bool: + """Return whether *user* is this repo's Oz GitHub App bot. + + Narrower than :func:`is_automation_user`: only the configured Oz bot + matches, so other automation accounts (e.g. dependabot) are excluded. + """ + bot_login = oz_bot_login() + if not bot_login: + return False + return get_login(user).strip().lower() == bot_login.lower() + + def get_timestamp_text(value: Any) -> str: if isinstance(value, datetime): @@ -610,8 +632,7 @@ def __init__( run_id=self.run_id, ) self._workflow_prefix = _workflow_metadata_prefix(workflow, issue_number) - app_slug = optional_env("GH_APP_SLUG") - self._bot_login = f"{app_slug}[bot]" if app_slug else "" + self._bot_login = oz_bot_login() self.comment_id: int | None = ( int(comment_id) if comment_id is not None and int(comment_id) > 0 else None ) diff --git a/tests/test_prompts.py b/tests/test_prompts.py index 4341891..94de751 100644 --- a/tests/test_prompts.py +++ b/tests/test_prompts.py @@ -307,6 +307,7 @@ def test_prompt_includes_ownership_area_selection_instructions(self) -> None: context = self._base_context() context["non_member_review_section"] = _format_non_member_review_section( pr_author_login="contributor", + is_non_member=True, ownership_areas_block=( "- MCP (Model Context Protocol)\n" " owners: @peicodes, @vkodithala\n" @@ -333,6 +334,7 @@ def test_prompt_includes_stakeholders_fallback_instructions(self) -> None: context = self._base_context() context["non_member_review_section"] = _format_non_member_review_section( pr_author_login="contributor", + is_non_member=True, stakeholders_block="- /docs/ → @docs-owner", ownership_areas_loaded=False, ) diff --git a/tests/test_review_pr_reviewer_sampling.py b/tests/test_review_pr_reviewer_sampling.py index b58221b..192acd8 100644 --- a/tests/test_review_pr_reviewer_sampling.py +++ b/tests/test_review_pr_reviewer_sampling.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os import unittest from pathlib import Path from types import SimpleNamespace @@ -264,6 +265,7 @@ class OwnershipAreasPromptSectionTest(unittest.TestCase): def test_prompt_requires_single_area_and_gates_on_verdict(self) -> None: prompt = _format_non_member_review_section( pr_author_login="contributor", + is_non_member=True, ownership_areas_block=( "- Docs API\n" " owners: @api-owner, @backup-owner\n" @@ -285,6 +287,7 @@ def test_prompt_requires_single_area_and_gates_on_verdict(self) -> None: def test_fallback_prompt_keeps_legacy_stakeholders_contract(self) -> None: prompt = _format_non_member_review_section( pr_author_login="contributor", + is_non_member=True, stakeholders_block="- /docs/ → @docs-owner", ownership_areas_loaded=False, ) @@ -296,6 +299,21 @@ def test_fallback_prompt_keeps_legacy_stakeholders_contract(self) -> None: prompt, ) + def test_bot_pr_reject_posts_comment_not_request_changes(self) -> None: + # The Oz bot's own PRs (is_non_member=False) must not advertise a + # blocking REQUEST_CHANGES outcome, matching apply_review_result. + for ownership_areas_loaded in (True, False): + with self.subTest(ownership_areas_loaded=ownership_areas_loaded): + prompt = _format_non_member_review_section( + pr_author_login="oz-for-oss[bot]", + is_non_member=False, + ownership_areas_block="- Docs API\n owners: @api-owner", + stakeholders_block="- /docs/ → @docs-owner", + ownership_areas_loaded=ownership_areas_loaded, + ) + self.assertNotIn("REQUEST_CHANGES", prompt) + self.assertIn("`COMMENT`", prompt) + class FormatReviewCompletionMessageTest(unittest.TestCase): def test_comment_with_recommended_reviewer_mentions_them(self) -> None: @@ -880,7 +898,9 @@ def _make_pr( def _gather(self, pr: SimpleNamespace) -> dict: github = MagicMock() github.get_pull.return_value = pr - with patch( + with patch.dict( + os.environ, {"GH_APP_SLUG": "oz-for-oss"} + ), patch( "workflows.review_pr.resolve_issue_number_for_pr", return_value=None ), patch( "workflows.review_pr.repo_local_skill_path_for_dispatch", @@ -948,6 +968,23 @@ def test_bot_and_external_authors_request_reviewer_on_approve(self) -> None: pr = self._apply_approve(context) pr.create_review_request.assert_called_once() + def test_non_oz_bot_pr_does_not_require_human_reviewer(self) -> None: + # dependabot and other automation accounts keep their own + # reviewer-assignment conventions; only the configured Oz bot + # escalates. + context = self._gather( + self._make_pr( + login="dependabot[bot]", + user_type="Bot", + association="NONE", + filenames=["requirements.txt"], + ) + ) + self.assertFalse(context["requires_human_reviewer"]) + self.assertEqual(context["non_member_review_section"], "") + pr = self._apply_approve(context) + pr.create_review_request.assert_not_called() + def test_member_pr_never_requires_human_reviewer(self) -> None: for filenames in (["specs/GH1/product.md"], ["core/app.py"]): with self.subTest(filenames=filenames):